[PATCH v4 11/20] pipeline: rkisp1: Fix ScalerCrop to be in sensor coordinates

[Stefan Klug]

Hi Jacopo,

Thank you for the review.

On Mon, Dec 16, 2024 at 07:29:56PM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Mon, Dec 16, 2024 at 04:40:51PM +0100, Stefan Klug wrote:
> > ScalerCrop is specified as being in sensor coordinates. The current
> > dewarper implementation on the imx8mp handles ScalerCrop in dewarper
> > coordinates. This leads to unexpected results and an unusable ScalerCrop
> > 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>
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v3:
> > - Rename dewarpSensorCrop_ to scalerMaxCrop_
> 
> Why not scalerCropMax_ ?
> :)

Haha... I actually thought about renaming it to scalerCropMax_ because
I also like it better. But in Message-ID:
<3i57xvgmtl7ey26isg5t7xubghj26pitm37szvyq6q5ingi7wz at d2udte44eqsq> you
proposed scalerMaxCrop_ and I confirmed it afterwards and then didn't
want to bring that up again :-). Good to know that we actually preferred
the same. Let's leave it like that for now. In the upcoming dewarper
series the variable will be removed anyways.

> 
> The rest, according to my memories of your clarifications seems good
> to me
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks,
Stefan

> 
> Thanks
>   j
> 
> > - Remove unnecessary ScalerCrop max calculation
> >
> > Changes in v2:
> > - Initialize dewarperSensorCrop_ to sane defaults
> > - Collect tags
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 50 +++++++++++++++++++-----
> >  1 file changed, 41 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 56192451eb3c..ef4aa38478f5 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -205,6 +205,7 @@ private:
> >  	RkISP1SelfPath selfPath_;
> >
> >  	std::unique_ptr<V4L2M2MConverter> dewarper_;
> > +	Rectangle scalerMaxCrop_;
> >  	bool useDewarper_;
> >
> >  	std::optional<Rectangle> activeCrop_;
> > @@ -861,6 +862,15 @@ 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.
> > +				 */
> > +				scalerMaxCrop_ =
> > +					outputCrop.transformedBetween(inputCrop,
> > +								      sensorInfo.analogCrop);
> >  			}
> >  		} else if (hasSelfPath_) {
> >  			ret = selfPath_.configure(cfg, format);
> > @@ -1226,10 +1236,19 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> >  		else
> >  			cropLimits = dewarper_->inputCropBounds();
> >
> > -		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.transformedBetween(cropLimits.second,
> > +								    scalerMaxCrop_);
> > +
> > +		controls[&controls::ScalerCrop] = ControlInfo(min,
> > +							      scalerMaxCrop_,
> > +							      scalerMaxCrop_);
> > +		activeCrop_ = scalerMaxCrop_;
> >  	}
> >
> >  	/* Add the IPA registered controls to list of camera controls. */
> > @@ -1257,6 +1276,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >  	/* Initialize the camera properties. */
> >  	data->properties_ = data->sensor_->properties();
> >
> > +	scalerMaxCrop_ = Rectangle(data->sensor_->resolution());
> > +
> >  	const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();
> >  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> >  		{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> > @@ -1476,22 +1497,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.transformedBetween(scalerMaxCrop_, cropLimits.second);
> >  		int ret = dewarper_->setInputCrop(&data->mainPathStream_,
> > -						  &appliedRect);
> > -		if (!ret && appliedRect != crop.value()) {
> > +						  &rect);
> > +		rect = rect.transformedBetween(cropLimits.second, scalerMaxCrop_);
> > +		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
> >