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

[Andrei Konovalov]

Hi Laurent and Milan,

On 20.03.2024 15:17, Laurent Pinchart wrote:
> Hi Milan,
> 
> Thank you for the patch.
> 
> On Tue, Mar 19, 2024 at 01:35:48PM +0100, Milan Zamazal 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.
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel at ucw.cz>
>> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Pavel Machek <pavel at ucw.cz>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>   src/libcamera/pipeline/simple/simple.cpp | 37 ++++++++++++++++++++++--
>>   1 file changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 01f2a977..04e77f7e 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -882,6 +882,30 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
>>   {
>>   }
>>   
>> +namespace {
>> +
>> +static Size adjustSize(const Size &requestedSize, const SizeRange &supportedSizes)
>> +{
>> +	ASSERT(supportedSizes.min <= supportedSizes.max);
>> +
>> +	if (supportedSizes.min == supportedSizes.max)
>> +		return supportedSizes.max;
>> +
>> +	unsigned int hStep = supportedSizes.hStep;
>> +	unsigned int vStep = supportedSizes.vStep;
>> +
>> +	if (hStep == 0)
>> +		hStep = supportedSizes.max.width - supportedSizes.min.width;
>> +	if (vStep == 0)
>> +		vStep = supportedSizes.max.height - supportedSizes.min.height;
>> +
>> +	Size adjusted = requestedSize.boundedTo(supportedSizes.max).expandedTo(supportedSizes.min);
>> +
>> +	return adjusted.shrunkBy(supportedSizes.min).alignedDownTo(hStep, vStep).grownBy(supportedSizes.min);
> 
> Those lines are getting long. You can write
> 
> 	Size adjusted = requestedSize.boundedTo(supportedSizes.max)
> 				     .expandedTo(supportedSizes.min);
> 	return adjusted.shrunkBy(supportedSizes.min)
> 		       .alignedDownTo(hStep, vStep)
> 		       .grownBy(supportedSizes.min);
> 
> or even
> 
> 	return requestedSize.boundedTo(supportedSizes.max)
> 			    .expandedTo(supportedSizes.min)
> 			    .shrunkBy(supportedSizes.min)
> 			    .alignedDownTo(hStep, vStep)
> 			    .grownBy(supportedSizes.min);
> 
> I can change this when applying if you tell me which version you like
> best (if any).

As for me, the both versions look good.
The first version matching my taste a tiny bit better probably.

Thanks,
Andrei

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> +}
>> +
>> +} /* namespace */
>> +
>>   CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>   {
>>   	const CameraSensor *sensor = data_->sensor_.get();
>> @@ -998,10 +1022,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>   		}
>>   
>>   		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>> +			Size adjustedSize = pipeConfig_->captureSize;
>> +			/*
>> +			 * 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 smaller valid output size closest to the requested.
>> +			 */
>> +			if (!pipeConfig_->outputSizes.contains(adjustedSize))
>> +				adjustedSize = adjustSize(cfg.size, pipeConfig_->outputSizes);
>>   			LOG(SimplePipeline, Debug)
>>   				<< "Adjusting size from " << cfg.size
>> -				<< " to " << pipeConfig_->captureSize;
>> -			cfg.size = pipeConfig_->captureSize;
>> +				<< " to " << adjustedSize;
>> +			cfg.size = adjustedSize;
>>   			status = Adjusted;
>>   		}
>>   
>