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

[Laurent Pinchart]

Hi Kieran,

On Thu, Mar 24, 2022 at 05:24:20PM +0000, Kieran Bingham wrote:
> 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:

In theory yes, but given than the target aspect ratio is the one of
resolution, it will be preserved. The value returned by
boundedToAspectRatio() can't have only one of with and height larger
than resolution's, and the other one smaller.

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

Yes, I'm pretty pleased with them too :-)

> 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