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

[Laurent Pinchart]

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).

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;
>  		}
>  

-- 
Regards,

Laurent Pinchart