[PATCH 4/7] pipeline: rkisp1: Split inputCrop and outputCrop

[Jacopo Mondi]

Hi Stefan

On Wed, Nov 20, 2024 at 09:57:43AM +0100, Stefan Klug wrote:
> One Rectangle instance is used to calculate the inputCrop and the
> outputCrop of the ISP in the rkisp1 pipeline. Split that into two
> distinct variables, because both values will be needed in the upcoming
> patches. This patch does not contain any functional changes.
>
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 9d36554cec6e..6491a48ee83c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -794,14 +794,15 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	if (ret < 0)
>  		return ret;
>
> -	Rectangle rect(0, 0, format.size);
> -	ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
> +	Rectangle inputCrop(0, 0, format.size);
> +	Rectangle outputCrop = inputCrop;
> +	ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &inputCrop);
>  	if (ret < 0)
>  		return ret;
>
>  	LOG(RkISP1, Debug)
>  		<< "ISP input pad configured with " << format
> -		<< " crop " << rect;
> +		<< " crop " << inputCrop;
>
>  	const PixelFormat &streamFormat = config->at(0).pixelFormat;
>  	const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
> @@ -819,18 +820,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  		const auto &cfg = config->at(0);
>  		Size ispCrop = format.size.boundedToAspectRatio(cfg.size)
>  				       .alignedUpTo(2, 2);
> -		rect = ispCrop.centeredTo(Rectangle(format.size).center());
> +		outputCrop = ispCrop.centeredTo(Rectangle(format.size).center());

If we don't enter the if() {} outputCrop will not be updated, and will
be initialized with the initial value of inputCrop, which could be
updated by the setSelection() call.

I would move the declaration

        Rectangle outputCrop = inputCrop;

after

	ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &inputCrop);
  	if (ret < 0)
  		return ret;

With that
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>  		if (ispCrop != format.size)
>  			LOG(RkISP1, Info) << "ISP output needs to be cropped to "
> -					  << rect;
> +					  << outputCrop;
>  		format.size = ispCrop;
>  	}
>
>  	LOG(RkISP1, Debug)
>  		<< "Configuring ISP output pad with " << format
> -		<< " crop " << rect;
> +		<< " crop " << outputCrop;
>
> -	ret = isp_->setSelection(2, V4L2_SEL_TGT_CROP, &rect);
> +	ret = isp_->setSelection(2, V4L2_SEL_TGT_CROP, &outputCrop);
>  	if (ret < 0)
>  		return ret;
>
> @@ -841,7 +842,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>
>  	LOG(RkISP1, Debug)
>  		<< "ISP output pad configured with " << format
> -		<< " crop " << rect;
> +		<< " crop " << outputCrop;
>
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	std::vector<std::reference_wrapper<StreamConfiguration>> outputCfgs;
> --
> 2.43.0
>