[libcamera-devel,v4,2/4] libcamera: rkisp1: Crop on ISP before downscaling

18424 diff mbox series

Crop on the resizer sink pad before downscaling to the aspect ratio
of the desired output size.

Cropping the input frame to the output aspect ratio allows to maintain
the correct picture proportions, as otherwise downscaling would change
the image geometry.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Hi Jacopo,

Thank you for the patch.

On 3/21/23 10:50 PM, Jacopo Mondi via libcamera-devel wrote:
> Crop on the resizer sink pad before downscaling to the aspect ratio
> of the desired output size.
>
> Cropping the input frame to the output aspect ratio allows to maintain
> the correct picture proportions, as otherwise downscaling would change
> the image geometry.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

The code indeed makes sense to me.

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 5079b268c464..5547cc32b6ca 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -314,7 +314,10 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>   	if (ret < 0)
>   		return ret;
>   
> -	Rectangle rect(0, 0, ispFormat.size);
> +	/* Crop on the resizer input to maintain FOV before downscaling */
> +	Size ispCrop = inputFormat.size.boundedToAspectRatio(config.size)
> +				       .alignedUpTo(2, 2);
> +	Rectangle rect(ispCrop);
>   	ret = resizer_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
>   	if (ret < 0)
>   		return ret;

Quoting Jacopo Mondi via libcamera-devel (2023-03-21 17:20:02)
> Crop on the resizer sink pad before downscaling to the aspect ratio
> of the desired output size.
> 
> Cropping the input frame to the output aspect ratio allows to maintain
> the correct picture proportions, as otherwise downscaling would change
> the image geometry.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 5079b268c464..5547cc32b6ca 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -314,7 +314,10 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>         if (ret < 0)
>                 return ret;
>  
> -       Rectangle rect(0, 0, ispFormat.size);
> +       /* Crop on the resizer input to maintain FOV before downscaling */
> +       Size ispCrop = inputFormat.size.boundedToAspectRatio(config.size)
> +                                      .alignedUpTo(2, 2);
> +       Rectangle rect(ispCrop);

Should this be centered ? Otherwise I can imagine a letterbox effect at
the *top* of the image, or a left aligned rectangle?


>         ret = resizer_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
>         if (ret < 0)
>                 return ret;
> -- 
> 2.40.0
>

Hello,

On Tue, May 23, 2023 at 09:50:59AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi via libcamera-devel (2023-03-21 17:20:02)
> > Crop on the resizer sink pad before downscaling to the aspect ratio
> > of the desired output size.
> > 
> > Cropping the input frame to the output aspect ratio allows to maintain
> > the correct picture proportions, as otherwise downscaling would change
> > the image geometry.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index 5079b268c464..5547cc32b6ca 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -314,7 +314,10 @@ int RkISP1Path::configure(const StreamConfiguration &config,
> >         if (ret < 0)
> >                 return ret;
> >  
> > -       Rectangle rect(0, 0, ispFormat.size);
> > +       /* Crop on the resizer input to maintain FOV before downscaling */
> > +       Size ispCrop = inputFormat.size.boundedToAspectRatio(config.size)
> > +                                      .alignedUpTo(2, 2);
> > +       Rectangle rect(ispCrop);
> 
> Should this be centered ? Otherwise I can imagine a letterbox effect at
> the *top* of the image, or a left aligned rectangle?

Yes, it should.

I would also add a todo comment:

	/*
	 * Crop on the resizer input to maintain FOV before downscaling.
	 *
	 * \todo The alignment to a multiple of 2 pixels is required but may
	 * change the aspect ratio very slightly. A more advanced algorithm to
	 * compute the resizer input crop rectangle is needed, and it should
	 * also take into account the need to crop away the edge pixels affected
	 * by the ISP processing blocks.
	 */

> >         ret = resizer_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
> >         if (ret < 0)
> >                 return ret;