[PATCH v2 3/8] libcamera: geometry: Add Rectangle::transformedBetween()

[Stefan Klug]

Hi Jacopo,

On Mon, Nov 25, 2024 at 08:28:27PM +0100, Jacopo Mondi wrote:
> Ups, I think you'll need a test for this though, even a simple one
> that tests the example you have made in the documentation..

Right that was forgotten in v2, now added for v3.

> 
> On Mon, Nov 25, 2024 at 07:20:46PM +0100, Jacopo Mondi wrote:
> > Hi Stefan
> >
> > On Mon, Nov 25, 2024 at 04:14:12PM +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).
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - Renamed mappedBetween() to transformedBetween()
> > > - Improved documentation
> > > - Collected tags
> > > ---
> > >  include/libcamera/geometry.h |  3 +++
> > >  src/libcamera/geometry.cpp   | 37 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> > >
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > index 9ca5865a3d0d..e5f0a843d314 100644
> > > --- a/include/libcamera/geometry.h
> > > +++ b/include/libcamera/geometry.h
> > > @@ -299,6 +299,9 @@ public:
> > >  	__nodiscard Rectangle scaledBy(const Size &numerator,
> > >  				       const Size &denominator) const;
> > >  	__nodiscard Rectangle translatedBy(const Point &point) const;
> > > +
> > > +	Rectangle transformedBetween(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..2c9308cb25ee 100644
> > > --- a/src/libcamera/geometry.cpp
> > > +++ b/src/libcamera/geometry.cpp
> > > @@ -837,6 +837,43 @@ Rectangle Rectangle::translatedBy(const Point &point) const
> > >  	return { x + point.x, y + point.y, width, height };
> > >  }
> > >
> > > +/**
> > > + * \brief Transforms a Rectangle from one reference frame to another
> >
> > nit: documentation is usually in imperative form:
> > Transforms -> Transform
> >
> > > + * \param[in] source The \a source reference frame
> > > + * \param[in] destination The \a destination reference frame
> >
> > I'm not sure I 100% like 'frame' here. The geometry library is not
> > only about frames or images...
> >
> >
> > > + *
> > > + * The params source and destinations specify two reference frames for the same
> >
> > Let's spell out terms in full in documentation.
> >
> > "The \a source and \a destination parameters "
> >
> > Also, I would find it cleaner if we talk about Rectangles here.
> >
> > "The \a source and \a destination parameters describe two rectangles
> > defined in different reference systems. The Rectangle is translated
> > from the source reference system into the destination reference
> > system."
> >
> >
> > > + * data. The rectangle is translated from the source reference frame into the
> > > + * destination reference frame.
> > > + *
> >
> > We can say
> >
> > "The typical use case for this function is to translate a selection
> > rectangle specified in a reference system, in example the sensor's
> > pixel array, into the same rectangle re-scaled and translated into
> > a different reference system, in example the output frame on
> > which the selection rectangle is applied to."
> >
> > > + * For example, consider a sensor with a resolution of 4040x2360 pixels and a
> > > + * assume a rectangle of (100, 100)/3840x2160 (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:
> >
> > And then provide a concrete example ? What do you think ? is is too
> > much details ?

Thanks for the suggestions. I took all of them. Let's see how you like
v3.

> >
> > > + *
> > > + * \code{.cpp}
> > > + * // Bottom right quarter in sensor coordinates
> > > + * Rectangle sensorRect(2020, 100, 1920, 1080);
> > > + * displayRect = sensorRect.transformedBetween(sensorFrame, displayFrame);
> > > + * // displayRect is now (960, 540)/960x540
> > > + * sensorRect = displayRect.transformedBetween(displayFrame, sensorFrame);
> > > + * \endcode
> > > + */
> > > +Rectangle Rectangle::transformedBetween(const Rectangle &source,
> > > +					const Rectangle &destination) const
> > > +{
> > > +	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);
> >
> > nit: I would add a blank line here
> >

Done.

Cheers,
Stefan

> > > +	return r;
> > > +}
> > > +
> >
> > nits and nitpicking apart
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Thanks
> >   j
> >
> > >  /**
> > >   * \brief Compare rectangles for equality
> > >   * \return True if the two rectangles are equal, false otherwise
> > > --
> > > 2.43.0
> > >