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

[Laurent Pinchart]

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 at 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;

-- 
Regards,

Laurent Pinchart