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

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> On Thu, Apr 04, 2024 at 10:46:38AM +0200, 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>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> 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 | 40 ++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 01f2a977..d2b88795 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -882,6 +882,33 @@ 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);
>
> Nitpicking, I think this would be more readable aligning the dots:

I agree, but LSP has different opinion (seems to prefer tab alignment).
I outsource formatting to LSP, which usually matches libcamera formatting very
well, and live with its recommendations, unless it suggests something clearly
wrong.

It could look better with line breaks after requestedSize and `adjusted' but if
anybody uses a more sane tab width than 8 then it doesn't matter much anyway.

Regards,
Milan

> 	Size adjusted = requestedSize.boundedTo(supportedSizes.max)
> 				     .expandedTo(supportedSizes.min);
>
> 	return adjusted.shrunkBy(supportedSizes.min)
> 		       .alignedDownTo(hStep, vStep)
> 		       .grownBy(supportedSizes.min);
>
> No need to resubmit just for that.
>
>> +}
>> +
>> +} /* namespace */
>> +
>>  CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>  {
>>  	const CameraSensor *sensor = data_->sensor_.get();
>> @@ -998,10 +1025,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;
>>  		}
>>