[PATCH v2 2/8] libcamera: rkisp1: Keep aspect ratio on imx8mp
Stefan Klug
stefan.klug at ideasonboard.com
Thu Nov 28 13:55:49 CET 2024
Hi Jacopo,
On Mon, Nov 25, 2024 at 08:27:12PM +0100, Jacopo Mondi wrote:
> Hi Stefan
>
> On Mon, Nov 25, 2024 at 04:14:11PM +0100, Stefan Klug wrote:
> > In the current code, the input stage of the image resizer is used to
> > apply a crop to keep the aspect ratio in cases where the requested
> > output aspect ratio differs from the one of the selected sensor mode. On
> > the imx8mp the resizer hardware is not capable of cropping (for
> > reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the
> > linux kernel v6.10).
> >
> > Therefore apply the necessary cropping on the output of the ISP (on the
> > image stabilization block). The cropping code on the image resizer
> > doesn't need modifications as the requested crop gets ignored by the
> > kernel.
> >
> > While at it, remove a todo comment that is no longer needed.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > ---
> >
> > Changes in v2:
> > - Fixed alignment
> > - Improved comments
> > - Removed todo comment
> > - Collected tags
> > ---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++
> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++-----
> > 2 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 5ffcfbce56be..a66b9dda83ab 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -811,6 +811,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > if (!isRaw_)
> > format.code = MEDIA_BUS_FMT_YUYV8_2X8;
> >
> > + /*
> > + * On devices without DUAL_CROP (like the imx8mp) cropping needs to be
> > + * done on the ISP/IS output.
> > + */
> > + if (media_->hwRevision() == RKISP1_V_IMX8MP) {
> > + /* imx8mp has only a single path. */
> > + const auto &cfg = config->at(0);
> > + Size ispCrop = format.size.boundedToAspectRatio(cfg.size)
> > + .alignedUpTo(2, 2);
> > + rect = ispCrop.centeredTo(Rectangle(format.size).center());
> > + if (ispCrop != format.size)
> > + LOG(RkISP1, Info) << "ISP output needs to be cropped to "
>
> very minor nit:
>
> What do you think about dropping "needs to be" from the Info message.
>
> Also, this is a fairly typical thing, cropping to the output aspect
> ratio before scaling, is it worth a Info message ?
Yes you are right. The info is available in the debug logs. I dropped
the message for v3.
Cheers,
Stefan
>
> Thanks
> j
>
> > + << rect;
> > + format.size = ispCrop;
> > + }
> > +
> > LOG(RkISP1, Debug)
> > << "Configuring ISP output pad with " << format
> > << " crop " << rect;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index 236d05af7a2f..eee5b09e2ff0 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -417,11 +417,14 @@ int RkISP1Path::configure(const StreamConfiguration &config,
> > /*
> > * 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.
> > + * Note that this does not apply to devices without DUAL_CROP support
> > + * (like imx8mp) , where the cropping needs to be done on the
> > + * ImageStabilizer block on the ISP source pad and therefore is
> > + * configured before this stage. For simplicity we still set the crop.
> > + * This gets ignored by the kernel driver because the hardware is
> > + * missing the capability.
> > + *
> > + * Alignment to a multiple of 2 pixels is required by the resizer.
> > */
> > Size ispCrop = inputFormat.size.boundedToAspectRatio(config.size)
> > .alignedUpTo(2, 2);
> > --
> > 2.43.0
> >
More information about the libcamera-devel
mailing list