[v2,7/8] pipeline: rkisp1: Fix ScalerCrop to be in sensor coordinates
diff mbox series

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

Commit Message

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

---
Changes in v2:
- Initialize dewarperSensorCrop_ to sane defaults
- Collected tags
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 50 +++++++++++++++++++-----
 1 file changed, 41 insertions(+), 9 deletions(-)

Comments

Jacopo Mondi Nov. 25, 2024, 7:30 p.m. UTC | #1 
HI Stefan

On Mon, Nov 25, 2024 at 04:14:16PM +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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v2:
> - Initialize dewarperSensorCrop_ to sane defaults
> - Collected 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 1ba416aaa545..0a044b08bc87 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_;
> @@ -863,6 +864,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.
> +				 */
> +				dewarperSensorCrop_ =
> +					outputCrop.transformedBetween(inputCrop,
> +								      sensorInfo.analogCrop);

I have a little trouble with the name dewarperSensorCrop_.

It represent the maximum crop rectangle in sensor's coordinate, right?

Why not call it scalerMaxCrop_ and initialize the ScalerCropMaximum
control as well with it ?

>  			}
>  		} else if (hasSelfPath_) {
>  			ret = selfPath_.configure(cfg, format);
> @@ -1225,10 +1235,19 @@ 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.transformedBetween(cropLimits.second,
> +								    dewarperSensorCrop_);
> +		Rectangle max = cropLimits.second.transformedBetween(cropLimits.second,
> +								     dewarperSensorCrop_);
> +
> +		controls[&controls::ScalerCrop] = ControlInfo(min, max, max);
> +		activeCrop_ = max;
>  	}
>
>  	/* Add the IPA registered controls to list of camera controls. */
> @@ -1256,6 +1275,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>
> +	dewarperSensorCrop_ = Rectangle(data->sensor_->resolution());
> +
>  	/*
>  	 * \todo Read delay values from the sensor itself or from a
>  	 * a sensor database. For now use generic values taken from
> @@ -1479,22 +1500,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(dewarperSensorCrop_, cropLimits.second);

Does this call for a Rectangle::transformBetween() ?

>  		int ret = dewarper_->setInputCrop(&data->mainPathStream_,
> -						  &appliedRect);
> -		if (!ret && appliedRect != crop.value()) {
> +						  &rect);
> +		rect = rect.transformedBetween(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
>
Stefan Klug Nov. 28, 2024, 1:09 p.m. UTC | #2 
Hi Jacopo,

On Mon, Nov 25, 2024 at 08:30:50PM +0100, Jacopo Mondi wrote:
> HI Stefan
> 
> On Mon, Nov 25, 2024 at 04:14:16PM +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@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > Changes in v2:
> > - Initialize dewarperSensorCrop_ to sane defaults
> > - Collected 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 1ba416aaa545..0a044b08bc87 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_;
> > @@ -863,6 +864,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.
> > +				 */
> > +				dewarperSensorCrop_ =
> > +					outputCrop.transformedBetween(inputCrop,
> > +								      sensorInfo.analogCrop);
> 
> I have a little trouble with the name dewarperSensorCrop_.
> 
> It represent the maximum crop rectangle in sensor's coordinate, right?
> 
> Why not call it scalerMaxCrop_ and initialize the ScalerCropMaximum
> control as well with it ?

You're completely right. In this case they are equal. I renamed it to
scalerMaxCrop_. That variable is only temporary and will vanish again in
the upcoming series with full dw100 dewarp/pan/zoom/scale support.

> 
> >  			}
> >  		} else if (hasSelfPath_) {
> >  			ret = selfPath_.configure(cfg, format);
> > @@ -1225,10 +1235,19 @@ 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.transformedBetween(cropLimits.second,
> > +								    dewarperSensorCrop_);
> > +		Rectangle max = cropLimits.second.transformedBetween(cropLimits.second,
> > +								     dewarperSensorCrop_);
> > +
> > +		controls[&controls::ScalerCrop] = ControlInfo(min, max, max);
> > +		activeCrop_ = max;
> >  	}
> >
> >  	/* Add the IPA registered controls to list of camera controls. */
> > @@ -1256,6 +1275,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >  	/* Initialize the camera properties. */
> >  	data->properties_ = data->sensor_->properties();
> >
> > +	dewarperSensorCrop_ = Rectangle(data->sensor_->resolution());
> > +
> >  	/*
> >  	 * \todo Read delay values from the sensor itself or from a
> >  	 * a sensor database. For now use generic values taken from
> > @@ -1479,22 +1500,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(dewarperSensorCrop_, cropLimits.second);
> 
> Does this call for a Rectangle::transformBetween() ?

Maybe :-) Do we generally duplicate every function in geometry? I
believe this line also vanishes in the upcoming series.

Cheers,
Stefan

> 
> >  		int ret = dewarper_->setInputCrop(&data->mainPathStream_,
> > -						  &appliedRect);
> > -		if (!ret && appliedRect != crop.value()) {
> > +						  &rect);
> > +		rect = rect.transformedBetween(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
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 1ba416aaa545..0a044b08bc87 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_;
@@ -863,6 +864,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.
+				 */
+				dewarperSensorCrop_ =
+					outputCrop.transformedBetween(inputCrop,
+								      sensorInfo.analogCrop);
 			}
 		} else if (hasSelfPath_) {
 			ret = selfPath_.configure(cfg, format);
@@ -1225,10 +1235,19 @@  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.transformedBetween(cropLimits.second,
+								    dewarperSensorCrop_);
+		Rectangle max = cropLimits.second.transformedBetween(cropLimits.second,
+								     dewarperSensorCrop_);
+
+		controls[&controls::ScalerCrop] = ControlInfo(min, max, max);
+		activeCrop_ = max;
 	}
 
 	/* Add the IPA registered controls to list of camera controls. */
@@ -1256,6 +1275,8 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 	/* Initialize the camera properties. */
 	data->properties_ = data->sensor_->properties();
 
+	dewarperSensorCrop_ = Rectangle(data->sensor_->resolution());
+
 	/*
 	 * \todo Read delay values from the sensor itself or from a
 	 * a sensor database. For now use generic values taken from
@@ -1479,22 +1500,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(dewarperSensorCrop_, cropLimits.second);
 		int ret = dewarper_->setInputCrop(&data->mainPathStream_,
-						  &appliedRect);
-		if (!ret && appliedRect != crop.value()) {
+						  &rect);
+		rect = rect.transformedBetween(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;
 	}
 
 	/*