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

18710 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>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Hi Jacopo,

Thank you for the patch.

On Tue, Jun 06, 2023 at 12:33:34PM +0200, 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>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 5079b268c464..0f728cea72b7 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -314,7 +314,18 @@ 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.
> +	 *
> +	 * \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.
> +	 */
> +	Size ispCrop = inputFormat.size.boundedToAspectRatio(config.size)
> +				       .alignedUpTo(2, 2);
> +	Rectangle rect = ispCrop.centeredTo(Rectangle(inputFormat.size).center());

I like the expressiveness of the geometry helpers :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

Hi Jacopo,

On 6/6/23 4:03 PM, Jacopo Mondi 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>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 5079b268c464..0f728cea72b7 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -314,7 +314,18 @@ 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.
> +	 *
> +	 * \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.
> +	 */
> +	Size ispCrop = inputFormat.size.boundedToAspectRatio(config.size)
> +				       .alignedUpTo(2, 2);
> +	Rectangle rect = ispCrop.centeredTo(Rectangle(inputFormat.size).center());

Testing this bit yesterday and saw a slight and sudden shrink of the 
frames when scaler crop rectangles are applied to the resizer.

The issue was that the rectangles applied as scaler crop were 
.boundedToAspectRatio(default_crop) [default_crop comes from the analog 
crop]. So when I adjusted the scaler crop rectangles to be applied, to 
be .boundedToAspectRatio[output.size], same as what you have done at the 
resizer input, it reconciled again.

So,

Tested-by: Umang Jain <umang.jain@ideasonboard.com>
>   	ret = resizer_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
>   	if (ret < 0)
>   		return ret;