[libcamera-devel] [PATCH v2 01/18] libcamera: pipeline: simple: fix size adjustment in validate()

[Andrei Konovalov]

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 at 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 at linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel at 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;
>>   		}
>>   
>