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

[Laurent Pinchart]

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.

> 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.
			 */

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 ?

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

-- 
Regards,

Laurent Pinchart