[PATCH 6/7] pipeline: rkisp1: Fix ScalerCrop to be in sensor coordinates

[Paul Elder]

On Wed, Nov 20, 2024 at 09:57:45AM +0100, Stefan Klug wrote:
> ScalerCrop is specified as beeing in sensor coordinates. The current

s/beeing/being/

> dewarper implementation on the imx8mp handles ScalerCrop in dewarper
> coordinates. This leads to unexpected results and a unusable ScalerCrop

s/ a / an /

> control in camshark. Fix that by transforming back and forth between
> sensor coordinates and dewarper coordinates.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>

Looks good.

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 +++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4a226d9b809f..c2ce38b1c253 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -204,6 +204,7 @@ private:
>  	RkISP1SelfPath selfPath_;
>  
>  	std::unique_ptr<V4L2M2MConverter> dewarper_;
> +	Rectangle dewarperSensorCrop_;
>  	bool useDewarper_;
>  
>  	std::optional<Rectangle> activeCrop_;
> @@ -862,6 +863,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  				outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg));
>  				ret = dewarper_->configure(cfg, outputCfgs);
>  				useDewarper_ = ret ? false : true;
> +
> +				/*
> +				 * Calculate the crop rectangle of the data
> +				 * flowing into the dewarper in sensor
> +				 * coordinates.
> +				 */
> +				dewarperSensorCrop_ = outputCrop.mappedBetween(inputCrop,
> +									       ipaConfig.sensorInfo.analogCrop);
>  			}
>  		} else if (hasSelfPath_) {
>  			ret = selfPath_.configure(cfg, format);
> @@ -1224,10 +1233,21 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
>  		std::pair<Rectangle, Rectangle> cropLimits =
>  			dewarper_->inputCropBounds(&data->mainPathStream_);
>  
> -		controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first,
> -							      cropLimits.second,
> -							      cropLimits.second);
> -		activeCrop_ = cropLimits.second;
> +		/*
> +		 * ScalerCrop is specified to be in Sensor coordinates.
> +		 * So we need to transform the limits to sensor coordinates.
> +		 * We can safely assume that the maximum crop limit contains the
> +		 * full fov of the dewarper.
> +		 */
> +		Rectangle min = cropLimits.first.mappedBetween(cropLimits.second,
> +							       dewarperSensorCrop_);
> +		Rectangle max = cropLimits.second.mappedBetween(cropLimits.second,
> +								dewarperSensorCrop_);
> +
> +		controls[&controls::ScalerCrop] = ControlInfo(min,
> +							      max,
> +							      max);
> +		activeCrop_ = max;
>  	}
>  
>  	/* Add the IPA registered controls to list of camera controls. */
> @@ -1478,22 +1498,33 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)
>  	/* Handle scaler crop control. */
>  	const auto &crop = request->controls().get(controls::ScalerCrop);
>  	if (crop) {
> -		Rectangle appliedRect = crop.value();
> +		Rectangle rect = crop.value();
> +
> +		/*
> +		 * ScalerCrop is specified to be in Sensor coordinates.
> +		 * So we need to transform it into dewarper coordinates.
> +		 * We can safely assume that the maximum crop limit contains the
> +		 * full fov of the dewarper.
> +		 */
> +		std::pair<Rectangle, Rectangle> cropLimits =
> +			dewarper_->inputCropBounds(&data->mainPathStream_);
>  
> +		rect = rect.mappedBetween(dewarperSensorCrop_, cropLimits.second);
>  		int ret = dewarper_->setInputCrop(&data->mainPathStream_,
> -						  &appliedRect);
> -		if (!ret && appliedRect != crop.value()) {
> +						  &rect);
> +		rect = rect.mappedBetween(cropLimits.second, dewarperSensorCrop_);
> +		if (!ret && rect != crop.value()) {
>  			/*
>  			 * If the rectangle is changed by setInputCrop on the
>  			 * dewarper, log a debug message and cache the actual
>  			 * applied rectangle for metadata reporting.
>  			 */
>  			LOG(RkISP1, Debug)
> -				<< "Applied rectangle " << appliedRect.toString()
> +				<< "Applied rectangle " << rect.toString()
>  				<< " differs from requested " << crop.value().toString();
>  		}
>  
> -		activeCrop_ = appliedRect;
> +		activeCrop_ = rect;
>  	}
>  
>  	/*
> -- 
> 2.43.0
>