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

22070 diff mbox series

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@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@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(+)

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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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 ?

> + *
> + * \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

> +	return r;
> +}
> +

nits and nitpicking apart
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

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

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..

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@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@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 ?
>
> > + *
> > + * \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
>
> > +	return r;
> > +}
> > +
>
> nits and nitpicking apart
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>   j
>
> >  /**
> >   * \brief Compare rectangles for equality
> >   * \return True if the two rectangles are equal, false otherwise
> > --
> > 2.43.0
> >

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@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@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@ideasonboard.com>
> >
> > Thanks
> >   j
> >
> > >  /**
> > >   * \brief Compare rectangles for equality
> > >   * \return True if the two rectangles are equal, false otherwise
> > > --
> > > 2.43.0
> > >