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

[Laurent Pinchart]

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:

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

-- 
Regards,

Laurent Pinchart