[v2,2/8] libcamera: rkisp1: Keep aspect ratio on imx8mp
diff mbox series

Message ID 20241125151430.2437285-3-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • rkisp1: Fix aspect ratio and ScalerCrop
Related show

Commit Message

Stefan Klug Nov. 25, 2024, 3:14 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@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(-)

Comments

Jacopo Mondi Nov. 25, 2024, 7:27 p.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@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 ?

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
>
Stefan Klug Nov. 28, 2024, 12:55 p.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@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
> >

Patch
diff mbox series 

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 "
+					  << 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);