[libcamera-devel] pipeline: rkisp1: Match sensor aspect ratio when generating configurations
diff mbox series

Message ID 20220324130506.27360-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 074fa98ac4ea903049ba9d7386cdb2f050ea3b48
Headers show
Series
  • [libcamera-devel] pipeline: rkisp1: Match sensor aspect ratio when generating configurations
Related show

Commit Message

Laurent Pinchart March 24, 2022, 1:05 p.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Kieran Bingham March 24, 2022, 5:24 p.m. UTC | #1 
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@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@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
>
Laurent Pinchart March 24, 2022, 6:49 p.m. UTC | #2 
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@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@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);
Nicolas Dufresne via libcamera-devel March 25, 2022, 5:29 a.m. UTC | #3 
Hi Laurent,

On Thu, Mar 24, 2022 at 03:05:06PM +0200, Laurent Pinchart wrote:
> 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@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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);
> +	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>  
>  	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
>

Patch
diff mbox series 

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);
+	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
 
 	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);