[libcamera-devel,v2,01/18] libcamera: pipeline: simple: fix size adjustment in validate()
diff mbox series

Message ID 20240113142218.28063-2-hdegoede@redhat.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2,01/18] libcamera: pipeline: simple: fix size adjustment in validate()
Related show

Commit Message

Hans de Goede Jan. 13, 2024, 2:22 p.m. UTC 
From: Andrey Konovalov <andrey.konovalov@linaro.org>

SimpleCameraConfiguration::validate() adjusts the configuration
of its streams (if the size is not in the outputSizes) to
the captureSize. But the captureSize itself can be not in the
outputSizes, and then the adjusted configuration won't be
valid resulting in camera configuration failure.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
---
 src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Milan Zamazal Jan. 23, 2024, 12:58 p.m. UTC | #1 
Hans de Goede <hdegoede@redhat.com> writes:

> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>
> SimpleCameraConfiguration::validate() adjusts the configuration
> of its streams (if the size is not in the outputSizes) to
> the captureSize. But the captureSize itself can be not in the
> outputSizes, and then the adjusted configuration won't be
> valid resulting in camera configuration failure.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 911051b2..4d0e7255 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -997,10 +997,13 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  		}
>  
>  		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
> +			Size adjustedSize = pipeConfig_->captureSize;
> +			if (!pipeConfig_->outputSizes.contains(adjustedSize))
> +				adjustedSize = pipeConfig_->outputSizes.max;
>  			LOG(SimplePipeline, Debug)
>  				<< "Adjusting size from " << cfg.size
> -				<< " to " << pipeConfig_->captureSize;
> -			cfg.size = pipeConfig_->captureSize;
> +				<< " to " << adjustedSize;
> +			cfg.size = adjustedSize;
>  			status = Adjusted;
>  		}
Laurent Pinchart Jan. 23, 2024, 2:06 p.m. UTC | #2 
Hi Andrey,

Thank you for the patch.

On Sat, Jan 13, 2024 at 03:22:01PM +0100, 📷-dev wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> SimpleCameraConfiguration::validate() adjusts the configuration
> of its streams (if the size is not in the outputSizes) to
> the captureSize. But the captureSize itself can be not in the
> outputSizes, and then the adjusted configuration won't be
> valid resulting in camera configuration failure.

I've always thought the git commit message line length limit of 72
characters was small, let's not make it even smaller.

> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 911051b2..4d0e7255 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -997,10 +997,13 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  		}
>  
>  		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
> +			Size adjustedSize = pipeConfig_->captureSize;

This deserves a comment to explain the logic.

			/*
			 * The converter (when present) may not be able to
			 * output a size identical to its input size. The
			 * capture size is thus not guaranteed to be a valid
			 * output size. In such cases, use the maximum output
			 * size instead.
			 */

I'm however wondering, shouldn't we pick the max only when the requested
size is larger than outputSizes.max, and pick the min otherwise ?

> +			if (!pipeConfig_->outputSizes.contains(adjustedSize))
> +				adjustedSize = pipeConfig_->outputSizes.max;
>  			LOG(SimplePipeline, Debug)
>  				<< "Adjusting size from " << cfg.size
> -				<< " to " << pipeConfig_->captureSize;
> -			cfg.size = pipeConfig_->captureSize;
> +				<< " to " << adjustedSize;
> +			cfg.size = adjustedSize;
>  			status = Adjusted;
>  		}
>
Andrei Konovalov Jan. 23, 2024, 2:31 p.m. UTC | #3 
Hi Laurent,

Thanks for the review!

On 23.01.2024 17:06, Laurent Pinchart wrote:
> Hi Andrey,
> 
> Thank you for the patch.
> 
> On Sat, Jan 13, 2024 at 03:22:01PM +0100, 📷-dev wrote:
>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>>
>> SimpleCameraConfiguration::validate() adjusts the configuration
>> of its streams (if the size is not in the outputSizes) to
>> the captureSize. But the captureSize itself can be not in the
>> outputSizes, and then the adjusted configuration won't be
>> valid resulting in camera configuration failure.
> 
> I've always thought the git commit message line length limit of 72
> characters was small, let's not make it even smaller.

I was always having hard time calculating the line lengths. Will fix that.

>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel@ucw.cz>
>> ---
>>   src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 911051b2..4d0e7255 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -997,10 +997,13 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>   		}
>>   
>>   		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>> +			Size adjustedSize = pipeConfig_->captureSize;
> 
> This deserves a comment to explain the logic.
> 
> 			/*
> 			 * The converter (when present) may not be able to
> 			 * output a size identical to its input size. The
> 			 * capture size is thus not guaranteed to be a valid
> 			 * output size. In such cases, use the maximum output
> 			 * size instead.
> 			 */

Makes sense, thanks!

> I'm however wondering, shouldn't we pick the max only when the requested
> size is larger than outputSizes.max, and pick the min otherwise ?

Yes, this is better than always using outputSizes.max.

Thanks,
Andrei

>> +			if (!pipeConfig_->outputSizes.contains(adjustedSize))
>> +				adjustedSize = pipeConfig_->outputSizes.max;
>>   			LOG(SimplePipeline, Debug)
>>   				<< "Adjusting size from " << cfg.size
>> -				<< " to " << pipeConfig_->captureSize;
>> -			cfg.size = pipeConfig_->captureSize;
>> +				<< " to " << adjustedSize;
>> +			cfg.size = adjustedSize;
>>   			status = Adjusted;
>>   		}
>>   
>
Laurent Pinchart Jan. 23, 2024, 2:44 p.m. UTC | #4 
On Tue, Jan 23, 2024 at 05:31:04PM +0300, Andrei Konovalov wrote:
> On 23.01.2024 17:06, Laurent Pinchart wrote:
> > On Sat, Jan 13, 2024 at 03:22:01PM +0100, 📷-dev wrote:
> >> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> >>
> >> SimpleCameraConfiguration::validate() adjusts the configuration
> >> of its streams (if the size is not in the outputSizes) to
> >> the captureSize. But the captureSize itself can be not in the
> >> outputSizes, and then the adjusted configuration won't be
> >> valid resulting in camera configuration failure.
> > 
> > I've always thought the git commit message line length limit of 72
> > characters was small, let's not make it even smaller.
> 
> I was always having hard time calculating the line lengths. Will fix that.

If you use vim, it should be configured correctly by default :-)

> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> >> Tested-by: Pavel Machek <pavel@ucw.cz>
> >> ---
> >>   src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 911051b2..4d0e7255 100644
> >> --- a/src/libcamera/pipeline/simple/simple.cpp
> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> @@ -997,10 +997,13 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >>   		}
> >>   
> >>   		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
> >> +			Size adjustedSize = pipeConfig_->captureSize;
> > 
> > This deserves a comment to explain the logic.
> > 
> > 			/*
> > 			 * The converter (when present) may not be able to
> > 			 * output a size identical to its input size. The
> > 			 * capture size is thus not guaranteed to be a valid
> > 			 * output size. In such cases, use the maximum output
> > 			 * size instead.
> > 			 */
> 
> Makes sense, thanks!
> 
> > I'm however wondering, shouldn't we pick the max only when the requested
> > size is larger than outputSizes.max, and pick the min otherwise ?
> 
> Yes, this is better than always using outputSizes.max.
> 
> >> +				adjustedSize = pipeConfig_->outputSizes.max;
> >>   			LOG(SimplePipeline, Debug)
> >>   				<< "Adjusting size from " << cfg.size
> >> -				<< " to " << pipeConfig_->captureSize;
> >> -			cfg.size = pipeConfig_->captureSize;
> >> +				<< " to " << adjustedSize;
> >> +			cfg.size = adjustedSize;
> >>   			status = Adjusted;
> >>   		}
> >>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 911051b2..4d0e7255 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -997,10 +997,13 @@  CameraConfiguration::Status SimpleCameraConfiguration::validate()
 		}
 
 		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
+			Size adjustedSize = pipeConfig_->captureSize;
+			if (!pipeConfig_->outputSizes.contains(adjustedSize))
+				adjustedSize = pipeConfig_->outputSizes.max;
 			LOG(SimplePipeline, Debug)
 				<< "Adjusting size from " << cfg.size
-				<< " to " << pipeConfig_->captureSize;
-			cfg.size = pipeConfig_->captureSize;
+				<< " to " << adjustedSize;
+			cfg.size = adjustedSize;
 			status = Adjusted;
 		}