[PATCH 3/7] libcamera: geometry: Add Rectangle::mappedBetween()

[Stefan Klug]

Hi Jacopo,

Thank you for the review. 

On Thu, Nov 21, 2024 at 12:45:47PM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Wed, Nov 20, 2024 at 09:57:42AM +0100, Stefan Klug wrote:
> > Handling cropping and scaling within a complicated pipeline involves
> > transformations of rectangles between different coordinate systems. For
> > example the full input of the dewarper (0,0)/1920x1080 might correspond
> > to the rectangle (0, 243)/2592x1458 in sensor coordinates (of a
> > 2592x1944 sensor). Add a function that allows the transformation of a
> > rectangle defined in one reference frame (dewarper) into the coordinates
> > of a second reference frame (sensor).
> 
> Let me try to clear my thoughts on this point, I've gone through
> re-scaling ScalerCrops too many times.
> 
> Currently the way all pipline handlers do this is
> 
> 	Rectangle ispCrop = nativeCrop.translatedBy(-sensor.topLeft());
> 	ispCrop.scaleBy(display.size(), sensor.size());
> 
> with:
>         nativeCrop = The crop rectangles expressed on the sensor
>                      native pixel array (controls::ScalerCrop)
>         sensor = The sensor analog crop rectangle
>         display = The destination reference system
> 
> The assumption is that nativeCrop is always contained in the
> analogCrop, as per the definition of the ScalerCrop control and of
> ScalerCropMaximum.
> 
> Good news is that
> 
> 	Rectangle sensor = { 0, 243, 2592, 1458 };
> 	Rectangle display = { 0, 0, 1920, 1080 };
> 	Rectangle nativeCrop = { 400, 400, 640, 480 };
> 
> 	Rectangle scaled = nativeCrop.mappedBetween(sensor, display);
> 
> 	cout << "Scaled: " << scaled << endl;
> 
> 	Rectangle ispCrop = nativeCrop.translatedBy(-sensor.topLeft());
> 	ispCrop.scaleBy(display.size(), sensor.size());
> 
> 	cout << "ISP Crop: " << ispCrop << endl;
> 
> Give the same result
> 
> Scaled: (296, 116)/474x355
> ISP Crop: (296, 116)/474x355
> 
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  include/libcamera/geometry.h |  2 ++
> >  src/libcamera/geometry.cpp   | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index 9ca5865a3d0d..8c5f34f4dec1 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -299,6 +299,8 @@ public:
> >  	__nodiscard Rectangle scaledBy(const Size &numerator,
> >  				       const Size &denominator) const;
> >  	__nodiscard Rectangle translatedBy(const Point &point) const;
> > +
> > +	Rectangle mappedBetween(const Rectangle &source, const Rectangle &target) const;
> >  };
> >
> >  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index 90ccf8c19f97..7e29fe35801e 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -837,6 +837,39 @@ Rectangle Rectangle::translatedBy(const Point &point) const
> >  	return { x + point.x, y + point.y, width, height };
> >  }
> >
> > +/**
> > + * \brief Maps a Rectangle from one reference frame to another
> > + * \param[in] source The source reference frame
> > + * \param[in] destination The destination reference frame
> > + *
> > + * The params source and destinations specify two reference frames for the same
> > + * data. The rectangle is translated from the source reference frame into the
> > + * destination reference frame.
> > + *
> > + * For example, consider a sensor with a resolution of 2592x1458 pixels and a
> > + * assume a rectangle of (0, 243)/2592x1458 (sensorFrame) in sensor coordinates
> > + * is mapped to a rectangle (0,0)/(1920,1080) (displayFrame) in display
> > + * coordinates. This function can be used to transform an arbitrary rectangle
> > + * from display coordinates to sensor coordinates or vice versa:
> > + *
> > + * \code{.cpp}
> > + * displayRect = sensorRect.mappedBetween(sensorFrame, displayFrame);
> > + * \endcode
> > + */
> > +Rectangle Rectangle::mappedBetween(const Rectangle &source,
> > +				   const Rectangle &destination) const
> > +{
> 
> Looking at the code of the two examples above, the sequence
> 
> 	Rectangle ispCrop = nativeCrop.translatedBy(-sensor.topLeft());
> 	ispCrop.scaleBy(display.size(), sensor.size());



> 
> translates to
> 
> 	r.x = x + destination.x;
>         r.y = y + destination.y;

Was this meant to be source.x/y? As this stems from
translatedBy(-sensor.topLeft()).

> 
> 	r.x = static_cast<int64_t>(r.x) * sx;
> 	r.y = static_cast<int64_t>(r.y) * sy;
> 	width = static_cast<uint64_t>(width) * sx;
> 	height = static_cast<uint64_t>(height) * sy;
> 
> > +	Rectangle r;
> > +	double sx = static_cast<double>(destination.width) / source.width;
> > +	double sy = static_cast<double>(destination.height) / source.height;
> > +
> > +	r.x = static_cast<int>((x - source.x) * sx) + destination.x;
> > +	r.y = static_cast<int>((y - source.y) * sy) + destination.y;
> > +	r.width = static_cast<int>(width * sx);
> > +	r.height = static_cast<int>(height * sy);
> > +	return r;
> 
> Which is equivalent to your code here if not for the '+ destination.x'
> and '+ destination.y' in:
> 
> 	r.x = static_cast<int>((x - source.x) * sx) + destination.x;
> 	r.y = static_cast<int>((y - source.y) * sy) + destination.y;
> 
> Now, Rectangle::scaleBy() takes two sizes instead of rectangles, so it
> assumes that the top-left point of the destination is not relevant.
> Is it in the case you want to support ?

The + destination.x/y makes the function generic and allows to map in
both directions. So to continue from your example above, you can do:

Rectangle scaled = nativeCrop.mappedBetween(sensor, display);
Rectangle native = scaled.mappedBetween(display, sensor);

This is helpful when mapping back the scaledCrop into sensor coordinates
for the metadata calculation. For my brain it is easier to think in
reference frames/coordinate systems than to manually get the sequence of
transformations in the right order. I also thought about representing
that as a matrix transformation. That felt like an overkill, but might
come in handy when our processing chains get longer.

> 
> Also, can
> 
> 	r.x = static_cast<int>((x - source.x) * sx) + destination.x;
> 	r.y = static_cast<int>((y - source.y) * sy) + destination.y;
> 
> result in any negative value ?

Yes that is possible for badly defined rectangles. But as x and y are
signed ints I don't see an issue here.

Best regards,
Stefan

> 
> 
> > +}
> > +
> >  /**
> >   * \brief Compare rectangles for equality
> >   * \return True if the two rectangles are equal, false otherwise
> > --
> > 2.43.0
> >