[libcamera-devel] [PATCH] pipeline: rkisp1: Match sensor aspect ratio when generating configurations

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2022-03-24 13:05:06)
> The RkISP1Path::generateConfiguration() function limits the maximum
> resolution to the sensor resolution, to avoid upscaling. It however
> doesn't take the sensor aspect ratio into account, which leads to a
> maximum (and default) resolution of 1920x1920 when using the self path
> with a sensor that has a higher resolution.
> 
> Fix it by constraining the minimum and maximum resolutions to match the
> sensor's aspect ratio.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index f8d471204d2e..f195f91ead1f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -50,12 +50,13 @@ bool RkISP1Path::init(MediaDevice *media)
>  
>  StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
>  {
> -       Size maxResolution = resolution;
> -       maxResolution.boundTo(maxResolution_);
> +       Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> +                                          .boundedTo(resolution);

Should the .boundedToAspectRatio() come afterwards? Feels like the
boundedTo() could end up losing the aspect ratio after:

Checking:

	resolution = { 3200, 1800 };

	maxResolution_ = { 1600 , 1600 };
	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
		== { 1600, 900 } .boundedTo(resolution)
		== { 1600, 900 };

Passes

	resolution = { 320, 180 };

	maxResolution_ = { 1600 , 1600 };
	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
		== { 1600, 900 } .boundedTo(resolution)
		== { 320, 180 };

Passes.

	resolution = { 1024, 1024 };

	maxResolution_ = { 4000 , 3000 };
	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
		== { 3000, 3000 } .boundedTo(resolution)
		== { 1024, 1024 };

Passes

	resolution = { 5000, 5000 };

	maxResolution_ = { 4000 , 3000 };
	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
		== { 3000, 3000 } .boundedTo(resolution)
		== { 3000, 3000 };

Also passes, so I don't know what I was imagining. Oh well.

So I think this is fine.


> +       Size minResolution = minResolution_.expandedToAspectRatio(resolution);

I'm so glad we have these expressive helpers. It makes the intent clear.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  
>         std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
>         for (const PixelFormat &format : formats_)
> -               streamFormats[format] = { { minResolution_, maxResolution } };
> +               streamFormats[format] = { { minResolution, maxResolution } };
>  
>         StreamFormats formats(streamFormats);
>         StreamConfiguration cfg(formats);
> -- 
> Regards,
> 
> Laurent Pinchart
>