[libcamera-devel,RFC,3/4] libcamera: Add geometry helper functions

9514 diff mbox series

These functions are aimed at making it easier to calculate cropping
rectangles, particularly in order to implement digital zoom.
---
 include/libcamera/geometry.h |  18 +++++
 src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+)

Hi David,

On Mon, Sep 07, 2020 at 05:44:49PM +0100, David Plowman wrote:
> These functions are aimed at making it easier to calculate cropping
> rectangles, particularly in order to implement digital zoom.
> ---
>  include/libcamera/geometry.h |  18 +++++
>  src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
>  2 files changed, 147 insertions(+)
>
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 02fb63c..8f6a9a0 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -13,6 +13,8 @@
>
>  namespace libcamera {
>
> +class Rectangle;
> +
>  class Size
>  {
>  public:
> @@ -93,8 +95,16 @@ public:
>  			std::max(height, expand.height)
>  		};
>  	}
> +
> +	Size aspectRatioDown(const Size &ar) const;
> +	Size aspectRatioUp(const Size &ar) const;
> +	Rectangle centre(const Rectangle &region,
> +			 int offsetX = 0, int offsetY = 0) const;

These all seems to be good ideas looking at example usages in 0/4 the
resulting API is nice! However I have a few comments, mostly related
to the distinction we have made for Size in methods that modify the
current instance and methods that create a new one.


>  };
>
> +Size operator*(const Size &size, float f);
> +Size operator/(const Size &size, float f);
> +
>  bool operator==(const Size &lhs, const Size &rhs);
>  bool operator<(const Size &lhs, const Size &rhs);
>
> @@ -176,6 +186,11 @@ public:
>  	{
>  	}
>
> +	constexpr explicit Rectangle(const Size &size)
> +		: x(0), y(0), width(size.width), height(size.height)
> +	{
> +	}
> +
>  	int x;
>  	int y;
>  	unsigned int width;
> @@ -183,6 +198,9 @@ public:
>
>  	bool isNull() const { return !width && !height; }
>  	const std::string toString() const;
> +
> +	Size size() const;
> +	Rectangle clamp(const Rectangle &boundary) const;

We have 'boundedTo' for Size which has the same semantic. I prefer clamp
but for simmerty I would use 'boundedTo' here as well.

we also there have a distinction between [boundedTo|alignedTo] and
[boundTo|alignTo] with the formers returning a new Size
bounded/expanded and the latters bounding/expanding the instance the
method has been called on. For simmerty I would call this method
boundedTo() as well.


>  };
>
>  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index b12e1a6..3e26167 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -143,6 +143,89 @@ const std::string Size::toString() const
>   * height of this size and the \a expand size
>   */
>
> +/**
> + * \brief Clip the given size to have a given aspect ratio

This accepts a size of arbitrary dimensions and return a new Size with
the sizes of the instance it has been called on aligned to the given
ratio. For the same reasons explained above the 'best' name would be
an unreadable

        alignedDownToAspectRatio(const Size &ratio)

Also "given aspect ratio" makes me think you expect something like
Size{4,3} but it's actually the aspect ratio of the Size provided as
argument.

Regardless of the method chosen name I would:

        \brief Align down to the aspect ration of \a ratio
        \param[in] ratio The size whose aspect ratio align down to
        \return A Size whose width and heigh are equal to the width
        and height of this Size aligned to the aspect ratio of \a
        ratio

The documentation of boundedTo() vs boundTo() provides an example

> + * \param[in] ar The aspect ratio the result is to have

missing \return Doxygen should complain

> + */
> +Size Size::aspectRatioDown(const Size &ar) const
> +{
> +	Size result;
> +
> +	uint64_t ratio1 = static_cast<uint64_t>(width) *
> +			  static_cast<uint64_t>(ar.height);
> +	uint64_t ratio2 = static_cast<uint64_t>(ar.width) *
> +			  static_cast<uint64_t>(height);
> +
> +	if (ratio1 > ratio2)
> +		result = Size(ratio2 / ar.height, height);
> +	else
> +		result = Size(width, ratio1 / ar.width);

nit: you could return {width, ratio1 / ar.width} and save a copy ?

> +
> +	return result;
> +}
> +
> +/**
> + * \brief Expand the given size to have a given aspect ratio
> + * \param[in] ar The aspect ratio the result is to have
> + */
> +Size Size::aspectRatioUp(const Size &ar) const
> +{
> +	Size result;
> +
> +	uint64_t ratio1 = static_cast<uint64_t>(width) *
> +			  static_cast<uint64_t>(ar.height);
> +	uint64_t ratio2 = static_cast<uint64_t>(ar.width) *
> +			  static_cast<uint64_t>(height);
> +
> +	if (ratio1 < ratio2)
> +		result = Size(ratio2 / ar.height, height);
> +	else
> +		result = Size(width, ratio1 / ar.width);
> +
> +	return result;
> +}
> +
> +/**
> + * \brief centre a rectangle of this size within another rectangular region,

Centre with capital 'C'

also this one could be 'centeredTo' as it returns a new Rectangle

> + * with optional offsets
> + * \param[in] region The rectangular region relative to which the returned
> + * rectangle can be position
> + * \param[in] offsetX the X offset of the mid-point of the returned rectangle
> + * relative to the mid-point of the region
> + * \param[in] offsetY the Y offset of the mid-point of the returned rectangle
> + * relative to the mid-point of the region

The X offset
The Y offset

with capital The

> + *
> + * A rectangle of this object's size is positioned within the rectangle

These can be Rectangle they refer to the class

> + * given by \a region. It is positioned so that its mid-point coincides
> + * with the mid-point of \a region, and is then further offset by the
> + * values \a offsetX and \a offsetY.
> + *
> + * \return A Rectangle offset within a region

      \return A Rectangle with the horizontal and vertical sizes of
      this Size instance, centered with offset within a region

      ? or something like this

> + */
> +Rectangle Size::centre(const Rectangle &region, int offsetX, int offsetY) const
> +{
> +	int x = (region.width - width) / 2 + region.x + offsetX;

Shouldn't you add offsetX to "(region.width - width) / 2" ? I might
have missed why you use region.x


> +	int y = (region.height - height) / 2 + region.y + offsetY;

Same

Do we trust region.widht > width ? and offsetX < region.width ?

I would make

        static Rectangle empty;

        unsigned int x = (region.width - width) / 2;
        if (!x)
                return empty;

Same for y, if you think this might be an issue.
Also mention that an empty rectangle can be returned in the
documentation.

> +
> +	return Rectangle(x, y, width, height);
> +}
> +
> +/**
> + * \brief Scale size up by the given factor
> + */
> +Size operator*(const Size &size, float f)
> +{
> +	return Size(size.width * f, size.height * f);
> +}

Ah! I would expect operator* to scale up the current instance...
multipledBy() ? scaledUp() ?

> +
> +/**
> + * \brief Scale size down by the given factor
> + */
> +Size operator/(const Size &size, float f)
> +{
> +	return Size(size.width / f, size.height / f);
> +}

Same comment

> +
>  /**
>   * \brief Compare sizes for equality
>   * \return True if the two sizes are equal, false otherwise
> @@ -365,6 +448,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
>   * \param[in] height The height
>   */
>
> +/**
> + * \fn Rectangle::Rectangle(const Size &size)
> + * \brief Construct a Rectangle with the zero offsets and size

Construct a rectangle whose sizes are the same as \a size and zero
offset ?
> + * \param[in] size The size

The desired Rectangle size

> + */
> +
>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> @@ -404,6 +493,46 @@ const std::string Rectangle::toString() const
>  	return ss.str();
>  }
>
> +/**
> + * \brief Return the size of this rectangle

we use 'retrieve' when documenting getters
and missing
        \return A Size reporting the Rectangle horizontal and vertical
        sizes

> + */
> +Size Rectangle::size() const
> +{
> +	return Size(width, height);
> +}
> +
> +/**
> + * \brief Clamp a rectangle so as not to exceeed another rectangle
> + * \param[in] boundary the limit that the returned rectangle will not exceed

The limit with capital 'T'
And you can use Rectangle (we don't have a really strict rule on when
capitalizing class names, I know)


> + *
> + * The rectangle is clamped so that it does not exceeed the given \a boundary.
> + * This process involves translating the rectangle if any of its edges
> + * lie beyond \a boundary, so that those edges then lie along the boundary
> + * instead.
> + *
> + * If either width or height are larger than \a bounary, then the returned
> + * rectangle is clipped to be no larger. But other than this, the
> + * rectangle is not clipped or reduced in size, merely translated.
> + *
> + * We note that this is not a conventional rectangle intersection function.
> + *
> + * \return A rectangle that does not extend beyond a boundary rectangle
> + */
> +Rectangle Rectangle::clamp(const Rectangle &boundary) const

Same concern.. clampedTo() ?
Otherwise documentation is very nice.

> +{
> +	Rectangle result(*this);
> +
> +	result.width = std::min(result.width, boundary.width);
> +	result.x = std::clamp<int>(result.x, boundary.x,
> +				   boundary.x + boundary.width - result.width);
> +
> +	result.height = std::min(result.height, boundary.height);
> +	result.y = std::clamp<int>(result.y, boundary.y,
> +				   boundary.y + boundary.height - result.height);
> +
> +	return result;
> +}
> +

Sorry the long comments, it's mostly about documentation and naming,
the actual content looks really nice!

Thanks
  j

>  /**
>   * \brief Compare rectangles for equality
>   * \return True if the two rectangles are equal, false otherwise
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo

Thanks for all the detailed comments, I shall try to incorporate them
all in my next patch set!

Just a couple of clarifications, if I may...

On Mon, 21 Sep 2020 at 15:21, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> On Mon, Sep 07, 2020 at 05:44:49PM +0100, David Plowman wrote:
> > These functions are aimed at making it easier to calculate cropping
> > rectangles, particularly in order to implement digital zoom.
> > ---
> >  include/libcamera/geometry.h |  18 +++++
> >  src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 147 insertions(+)
> >
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index 02fb63c..8f6a9a0 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -13,6 +13,8 @@
> >
> >  namespace libcamera {
> >
> > +class Rectangle;
> > +
> >  class Size
> >  {
> >  public:
> > @@ -93,8 +95,16 @@ public:
> >                       std::max(height, expand.height)
> >               };
> >       }
> > +
> > +     Size aspectRatioDown(const Size &ar) const;
> > +     Size aspectRatioUp(const Size &ar) const;
> > +     Rectangle centre(const Rectangle &region,
> > +                      int offsetX = 0, int offsetY = 0) const;
>
> These all seems to be good ideas looking at example usages in 0/4 the
> resulting API is nice! However I have a few comments, mostly related
> to the distinction we have made for Size in methods that modify the
> current instance and methods that create a new one.
>
>
> >  };
> >
> > +Size operator*(const Size &size, float f);
> > +Size operator/(const Size &size, float f);
> > +
> >  bool operator==(const Size &lhs, const Size &rhs);
> >  bool operator<(const Size &lhs, const Size &rhs);
> >
> > @@ -176,6 +186,11 @@ public:
> >       {
> >       }
> >
> > +     constexpr explicit Rectangle(const Size &size)
> > +             : x(0), y(0), width(size.width), height(size.height)
> > +     {
> > +     }
> > +
> >       int x;
> >       int y;
> >       unsigned int width;
> > @@ -183,6 +198,9 @@ public:
> >
> >       bool isNull() const { return !width && !height; }
> >       const std::string toString() const;
> > +
> > +     Size size() const;
> > +     Rectangle clamp(const Rectangle &boundary) const;
>
> We have 'boundedTo' for Size which has the same semantic. I prefer clamp
> but for simmerty I would use 'boundedTo' here as well.
>
> we also there have a distinction between [boundedTo|alignedTo] and
> [boundTo|alignTo] with the formers returning a new Size
> bounded/expanded and the latters bounding/expanding the instance the
> method has been called on. For simmerty I would call this method
> boundedTo() as well.
>
>
> >  };
> >
> >  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index b12e1a6..3e26167 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -143,6 +143,89 @@ const std::string Size::toString() const
> >   * height of this size and the \a expand size
> >   */
> >
> > +/**
> > + * \brief Clip the given size to have a given aspect ratio
>
> This accepts a size of arbitrary dimensions and return a new Size with
> the sizes of the instance it has been called on aligned to the given
> ratio. For the same reasons explained above the 'best' name would be
> an unreadable
>
>         alignedDownToAspectRatio(const Size &ratio)
>
> Also "given aspect ratio" makes me think you expect something like
> Size{4,3} but it's actually the aspect ratio of the Size provided as
> argument.
>
> Regardless of the method chosen name I would:
>
>         \brief Align down to the aspect ration of \a ratio
>         \param[in] ratio The size whose aspect ratio align down to
>         \return A Size whose width and heigh are equal to the width
>         and height of this Size aligned to the aspect ratio of \a
>         ratio
>
> The documentation of boundedTo() vs boundTo() provides an example
>
> > + * \param[in] ar The aspect ratio the result is to have
>
> missing \return Doxygen should complain
>
> > + */
> > +Size Size::aspectRatioDown(const Size &ar) const
> > +{
> > +     Size result;
> > +
> > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > +                       static_cast<uint64_t>(ar.height);
> > +     uint64_t ratio2 = static_cast<uint64_t>(ar.width) *
> > +                       static_cast<uint64_t>(height);
> > +
> > +     if (ratio1 > ratio2)
> > +             result = Size(ratio2 / ar.height, height);
> > +     else
> > +             result = Size(width, ratio1 / ar.width);
>
> nit: you could return {width, ratio1 / ar.width} and save a copy ?
>
> > +
> > +     return result;
> > +}
> > +
> > +/**
> > + * \brief Expand the given size to have a given aspect ratio
> > + * \param[in] ar The aspect ratio the result is to have
> > + */
> > +Size Size::aspectRatioUp(const Size &ar) const
> > +{
> > +     Size result;
> > +
> > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > +                       static_cast<uint64_t>(ar.height);
> > +     uint64_t ratio2 = static_cast<uint64_t>(ar.width) *
> > +                       static_cast<uint64_t>(height);
> > +
> > +     if (ratio1 < ratio2)
> > +             result = Size(ratio2 / ar.height, height);
> > +     else
> > +             result = Size(width, ratio1 / ar.width);
> > +
> > +     return result;
> > +}
> > +
> > +/**
> > + * \brief centre a rectangle of this size within another rectangular region,
>
> Centre with capital 'C'
>
> also this one could be 'centeredTo' as it returns a new Rectangle
>
> > + * with optional offsets
> > + * \param[in] region The rectangular region relative to which the returned
> > + * rectangle can be position
> > + * \param[in] offsetX the X offset of the mid-point of the returned rectangle
> > + * relative to the mid-point of the region
> > + * \param[in] offsetY the Y offset of the mid-point of the returned rectangle
> > + * relative to the mid-point of the region
>
> The X offset
> The Y offset
>
> with capital The
>
> > + *
> > + * A rectangle of this object's size is positioned within the rectangle
>
> These can be Rectangle they refer to the class
>
> > + * given by \a region. It is positioned so that its mid-point coincides
> > + * with the mid-point of \a region, and is then further offset by the
> > + * values \a offsetX and \a offsetY.
> > + *
> > + * \return A Rectangle offset within a region
>
>       \return A Rectangle with the horizontal and vertical sizes of
>       this Size instance, centered with offset within a region
>
>       ? or something like this
>
> > + */
> > +Rectangle Size::centre(const Rectangle &region, int offsetX, int offsetY) const
> > +{
> > +     int x = (region.width - width) / 2 + region.x + offsetX;
>
> Shouldn't you add offsetX to "(region.width - width) / 2" ? I might
> have missed why you use region.x

So I think I'm correct here. For example, imagine we have an image
that is large and "region" is a Rectangle size 500x500 at offset
(500,500) within it.

Now, let's suppose I have a Size that is 1000x1000 and I now want a
Rectangle of this size (1000x1000) centred in the same place as
"region". This new rectangle will have to have offsets of (250,250) to
have the same mid-point. Thus the new offset being "(region.width -
width) / 2 + region.x + offsetX" gives (500 - 1000) / 2 + 500 + 0
which is 250. Does that make sense?

>
>
> > +     int y = (region.height - height) / 2 + region.y + offsetY;
>
> Same
>
> Do we trust region.widht > width ? and offsetX < region.width ?
>
> I would make
>
>         static Rectangle empty;
>
>         unsigned int x = (region.width - width) / 2;
>         if (!x)
>                 return empty;
>
> Same for y, if you think this might be an issue.
> Also mention that an empty rectangle can be returned in the
> documentation.

Again I think I'm OK here. x and y are offsets and can by design be
zero or negative.

>
> > +
> > +     return Rectangle(x, y, width, height);
> > +}
> > +
> > +/**
> > + * \brief Scale size up by the given factor
> > + */
> > +Size operator*(const Size &size, float f)
> > +{
> > +     return Size(size.width * f, size.height * f);
> > +}
>
> Ah! I would expect operator* to scale up the current instance...
> multipledBy() ? scaledUp() ?

I wasn't quite sure what you meant here. I'd expect "Size * float" to
give me a new (scaled) Size without affecting my original Size object.
If I wanted to change the original size I'd define an operator*=
(though I didn't actually do that). Or do you mean that you'd rather
have separate methods with different names (e.g. scaledUp) and avoid
overloading the operator*?

Thanks again
David

>
> > +
> > +/**
> > + * \brief Scale size down by the given factor
> > + */
> > +Size operator/(const Size &size, float f)
> > +{
> > +     return Size(size.width / f, size.height / f);
> > +}
>
> Same comment
>
> > +
> >  /**
> >   * \brief Compare sizes for equality
> >   * \return True if the two sizes are equal, false otherwise
> > @@ -365,6 +448,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
> >   * \param[in] height The height
> >   */
> >
> > +/**
> > + * \fn Rectangle::Rectangle(const Size &size)
> > + * \brief Construct a Rectangle with the zero offsets and size
>
> Construct a rectangle whose sizes are the same as \a size and zero
> offset ?
> > + * \param[in] size The size
>
> The desired Rectangle size
>
> > + */
> > +
> >  /**
> >   * \var Rectangle::x
> >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > @@ -404,6 +493,46 @@ const std::string Rectangle::toString() const
> >       return ss.str();
> >  }
> >
> > +/**
> > + * \brief Return the size of this rectangle
>
> we use 'retrieve' when documenting getters
> and missing
>         \return A Size reporting the Rectangle horizontal and vertical
>         sizes
>
> > + */
> > +Size Rectangle::size() const
> > +{
> > +     return Size(width, height);
> > +}
> > +
> > +/**
> > + * \brief Clamp a rectangle so as not to exceeed another rectangle
> > + * \param[in] boundary the limit that the returned rectangle will not exceed
>
> The limit with capital 'T'
> And you can use Rectangle (we don't have a really strict rule on when
> capitalizing class names, I know)
>
>
> > + *
> > + * The rectangle is clamped so that it does not exceeed the given \a boundary.
> > + * This process involves translating the rectangle if any of its edges
> > + * lie beyond \a boundary, so that those edges then lie along the boundary
> > + * instead.
> > + *
> > + * If either width or height are larger than \a bounary, then the returned
> > + * rectangle is clipped to be no larger. But other than this, the
> > + * rectangle is not clipped or reduced in size, merely translated.
> > + *
> > + * We note that this is not a conventional rectangle intersection function.
> > + *
> > + * \return A rectangle that does not extend beyond a boundary rectangle
> > + */
> > +Rectangle Rectangle::clamp(const Rectangle &boundary) const
>
> Same concern.. clampedTo() ?
> Otherwise documentation is very nice.
>
> > +{
> > +     Rectangle result(*this);
> > +
> > +     result.width = std::min(result.width, boundary.width);
> > +     result.x = std::clamp<int>(result.x, boundary.x,
> > +                                boundary.x + boundary.width - result.width);
> > +
> > +     result.height = std::min(result.height, boundary.height);
> > +     result.y = std::clamp<int>(result.y, boundary.y,
> > +                                boundary.y + boundary.height - result.height);
> > +
> > +     return result;
> > +}
> > +
>
> Sorry the long comments, it's mostly about documentation and naming,
> the actual content looks really nice!
>
> Thanks
>   j
>
> >  /**
> >   * \brief Compare rectangles for equality
> >   * \return True if the two rectangles are equal, false otherwise
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi David,

On Mon, Sep 21, 2020 at 04:38:33PM +0100, David Plowman wrote:
> Hi Jacopo
>
> Thanks for all the detailed comments, I shall try to incorporate them
> all in my next patch set!
>
> Just a couple of clarifications, if I may...
>
> On Mon, 21 Sep 2020 at 15:21, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David,
> >
> > On Mon, Sep 07, 2020 at 05:44:49PM +0100, David Plowman wrote:
> > > These functions are aimed at making it easier to calculate cropping
> > > rectangles, particularly in order to implement digital zoom.
> > > ---
> > >  include/libcamera/geometry.h |  18 +++++
> > >  src/libcamera/geometry.cpp   | 129 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 147 insertions(+)
> > >
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > index 02fb63c..8f6a9a0 100644
> > > --- a/include/libcamera/geometry.h
> > > +++ b/include/libcamera/geometry.h
> > > @@ -13,6 +13,8 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class Rectangle;
> > > +
> > >  class Size
> > >  {
> > >  public:
> > > @@ -93,8 +95,16 @@ public:
> > >                       std::max(height, expand.height)
> > >               };
> > >       }
> > > +
> > > +     Size aspectRatioDown(const Size &ar) const;
> > > +     Size aspectRatioUp(const Size &ar) const;
> > > +     Rectangle centre(const Rectangle &region,
> > > +                      int offsetX = 0, int offsetY = 0) const;
> >
> > These all seems to be good ideas looking at example usages in 0/4 the
> > resulting API is nice! However I have a few comments, mostly related
> > to the distinction we have made for Size in methods that modify the
> > current instance and methods that create a new one.
> >
> >
> > >  };
> > >
> > > +Size operator*(const Size &size, float f);
> > > +Size operator/(const Size &size, float f);
> > > +
> > >  bool operator==(const Size &lhs, const Size &rhs);
> > >  bool operator<(const Size &lhs, const Size &rhs);
> > >
> > > @@ -176,6 +186,11 @@ public:
> > >       {
> > >       }
> > >
> > > +     constexpr explicit Rectangle(const Size &size)
> > > +             : x(0), y(0), width(size.width), height(size.height)
> > > +     {
> > > +     }
> > > +
> > >       int x;
> > >       int y;
> > >       unsigned int width;
> > > @@ -183,6 +198,9 @@ public:
> > >
> > >       bool isNull() const { return !width && !height; }
> > >       const std::string toString() const;
> > > +
> > > +     Size size() const;
> > > +     Rectangle clamp(const Rectangle &boundary) const;
> >
> > We have 'boundedTo' for Size which has the same semantic. I prefer clamp
> > but for simmerty I would use 'boundedTo' here as well.
> >
> > we also there have a distinction between [boundedTo|alignedTo] and
> > [boundTo|alignTo] with the formers returning a new Size
> > bounded/expanded and the latters bounding/expanding the instance the
> > method has been called on. For simmerty I would call this method
> > boundedTo() as well.
> >
> >
> > >  };
> > >
> > >  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > > index b12e1a6..3e26167 100644
> > > --- a/src/libcamera/geometry.cpp
> > > +++ b/src/libcamera/geometry.cpp
> > > @@ -143,6 +143,89 @@ const std::string Size::toString() const
> > >   * height of this size and the \a expand size
> > >   */
> > >
> > > +/**
> > > + * \brief Clip the given size to have a given aspect ratio
> >
> > This accepts a size of arbitrary dimensions and return a new Size with
> > the sizes of the instance it has been called on aligned to the given
> > ratio. For the same reasons explained above the 'best' name would be
> > an unreadable
> >
> >         alignedDownToAspectRatio(const Size &ratio)
> >
> > Also "given aspect ratio" makes me think you expect something like
> > Size{4,3} but it's actually the aspect ratio of the Size provided as
> > argument.
> >
> > Regardless of the method chosen name I would:
> >
> >         \brief Align down to the aspect ration of \a ratio
> >         \param[in] ratio The size whose aspect ratio align down to
> >         \return A Size whose width and heigh are equal to the width
> >         and height of this Size aligned to the aspect ratio of \a
> >         ratio
> >
> > The documentation of boundedTo() vs boundTo() provides an example
> >
> > > + * \param[in] ar The aspect ratio the result is to have
> >
> > missing \return Doxygen should complain
> >
> > > + */
> > > +Size Size::aspectRatioDown(const Size &ar) const
> > > +{
> > > +     Size result;
> > > +
> > > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > > +                       static_cast<uint64_t>(ar.height);
> > > +     uint64_t ratio2 = static_cast<uint64_t>(ar.width) *
> > > +                       static_cast<uint64_t>(height);
> > > +
> > > +     if (ratio1 > ratio2)
> > > +             result = Size(ratio2 / ar.height, height);
> > > +     else
> > > +             result = Size(width, ratio1 / ar.width);
> >
> > nit: you could return {width, ratio1 / ar.width} and save a copy ?
> >
> > > +
> > > +     return result;
> > > +}
> > > +
> > > +/**
> > > + * \brief Expand the given size to have a given aspect ratio
> > > + * \param[in] ar The aspect ratio the result is to have
> > > + */
> > > +Size Size::aspectRatioUp(const Size &ar) const
> > > +{
> > > +     Size result;
> > > +
> > > +     uint64_t ratio1 = static_cast<uint64_t>(width) *
> > > +                       static_cast<uint64_t>(ar.height);
> > > +     uint64_t ratio2 = static_cast<uint64_t>(ar.width) *
> > > +                       static_cast<uint64_t>(height);
> > > +
> > > +     if (ratio1 < ratio2)
> > > +             result = Size(ratio2 / ar.height, height);
> > > +     else
> > > +             result = Size(width, ratio1 / ar.width);
> > > +
> > > +     return result;
> > > +}
> > > +
> > > +/**
> > > + * \brief centre a rectangle of this size within another rectangular region,
> >
> > Centre with capital 'C'
> >
> > also this one could be 'centeredTo' as it returns a new Rectangle
> >
> > > + * with optional offsets
> > > + * \param[in] region The rectangular region relative to which the returned
> > > + * rectangle can be position
> > > + * \param[in] offsetX the X offset of the mid-point of the returned rectangle
> > > + * relative to the mid-point of the region
> > > + * \param[in] offsetY the Y offset of the mid-point of the returned rectangle
> > > + * relative to the mid-point of the region
> >
> > The X offset
> > The Y offset
> >
> > with capital The
> >
> > > + *
> > > + * A rectangle of this object's size is positioned within the rectangle
> >
> > These can be Rectangle they refer to the class
> >
> > > + * given by \a region. It is positioned so that its mid-point coincides
> > > + * with the mid-point of \a region, and is then further offset by the
> > > + * values \a offsetX and \a offsetY.
> > > + *
> > > + * \return A Rectangle offset within a region
> >
> >       \return A Rectangle with the horizontal and vertical sizes of
> >       this Size instance, centered with offset within a region
> >
> >       ? or something like this
> >
> > > + */
> > > +Rectangle Size::centre(const Rectangle &region, int offsetX, int offsetY) const
> > > +{
> > > +     int x = (region.width - width) / 2 + region.x + offsetX;
> >
> > Shouldn't you add offsetX to "(region.width - width) / 2" ? I might
> > have missed why you use region.x
>
> So I think I'm correct here. For example, imagine we have an image
> that is large and "region" is a Rectangle size 500x500 at offset
> (500,500) within it.

Ok, I was considering the case when 'region' is always larger than the
current size, and in that case the offsetX was to me relative to the
'region' top-left corner in position (0, 0)

>
> Now, let's suppose I have a Size that is 1000x1000 and I now want a
> Rectangle of this size (1000x1000) centred in the same place as
> "region". This new rectangle will have to have offsets of (250,250) to
> have the same mid-point. Thus the new offset being "(region.width -
> width) / 2 + region.x + offsetX" gives (500 - 1000) / 2 + 500 + 0
> which is 250. Does that make sense?
>

Yes, it does :)
Now I wonder if it's relevant to specify the reference point of the 'region'
top-left corner. Probably not, this function is just about centering
two rectangles, what do they represent is context dependent.

> >
> >
> > > +     int y = (region.height - height) / 2 + region.y + offsetY;
> >
> > Same
> >
> > Do we trust region.widht > width ? and offsetX < region.width ?
> >
> > I would make
> >
> >         static Rectangle empty;
> >
> >         unsigned int x = (region.width - width) / 2;
> >         if (!x)
> >                 return empty;
> >
> > Same for y, if you think this might be an issue.
> > Also mention that an empty rectangle can be returned in the
> > documentation.
>
> Again I think I'm OK here. x and y are offsets and can by design be
> zero or negative.
>

In case 'region' is smaller, yes, having region.width < width makes
sense an actually, negative offsets made sense anyway.

> >
> > > +
> > > +     return Rectangle(x, y, width, height);
> > > +}
> > > +
> > > +/**
> > > + * \brief Scale size up by the given factor
> > > + */
> > > +Size operator*(const Size &size, float f)
> > > +{
> > > +     return Size(size.width * f, size.height * f);
> > > +}
> >
> > Ah! I would expect operator* to scale up the current instance...
> > multipledBy() ? scaledUp() ?
>
> I wasn't quite sure what you meant here. I'd expect "Size * float" to
> give me a new (scaled) Size without affecting my original Size object.

I was again pointing out the naming scheme we have for methods that
modifies an instance in place instead of returning a new one

> If I wanted to change the original size I'd define an operator*=
> (though I didn't actually do that). Or do you mean that you'd rather
> have separate methods with different names (e.g. scaledUp) and avoid
> overloading the operator*?

But with operator* and operator*= we have this distinction made clear
enough I think. So this is fine the way it is!

Thanks
  j

>
> Thanks again
> David
>
> >
> > > +
> > > +/**
> > > + * \brief Scale size down by the given factor
> > > + */
> > > +Size operator/(const Size &size, float f)
> > > +{
> > > +     return Size(size.width / f, size.height / f);
> > > +}
> >
> > Same comment
> >
> > > +
> > >  /**
> > >   * \brief Compare sizes for equality
> > >   * \return True if the two sizes are equal, false otherwise
> > > @@ -365,6 +448,12 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
> > >   * \param[in] height The height
> > >   */
> > >
> > > +/**
> > > + * \fn Rectangle::Rectangle(const Size &size)
> > > + * \brief Construct a Rectangle with the zero offsets and size
> >
> > Construct a rectangle whose sizes are the same as \a size and zero
> > offset ?
> > > + * \param[in] size The size
> >
> > The desired Rectangle size
> >
> > > + */
> > > +
> > >  /**
> > >   * \var Rectangle::x
> > >   * \brief The horizontal coordinate of the rectangle's top-left corner
> > > @@ -404,6 +493,46 @@ const std::string Rectangle::toString() const
> > >       return ss.str();
> > >  }
> > >
> > > +/**
> > > + * \brief Return the size of this rectangle
> >
> > we use 'retrieve' when documenting getters
> > and missing
> >         \return A Size reporting the Rectangle horizontal and vertical
> >         sizes
> >
> > > + */
> > > +Size Rectangle::size() const
> > > +{
> > > +     return Size(width, height);
> > > +}
> > > +
> > > +/**
> > > + * \brief Clamp a rectangle so as not to exceeed another rectangle
> > > + * \param[in] boundary the limit that the returned rectangle will not exceed
> >
> > The limit with capital 'T'
> > And you can use Rectangle (we don't have a really strict rule on when
> > capitalizing class names, I know)
> >
> >
> > > + *
> > > + * The rectangle is clamped so that it does not exceeed the given \a boundary.
> > > + * This process involves translating the rectangle if any of its edges
> > > + * lie beyond \a boundary, so that those edges then lie along the boundary
> > > + * instead.
> > > + *
> > > + * If either width or height are larger than \a bounary, then the returned
> > > + * rectangle is clipped to be no larger. But other than this, the
> > > + * rectangle is not clipped or reduced in size, merely translated.
> > > + *
> > > + * We note that this is not a conventional rectangle intersection function.
> > > + *
> > > + * \return A rectangle that does not extend beyond a boundary rectangle
> > > + */
> > > +Rectangle Rectangle::clamp(const Rectangle &boundary) const
> >
> > Same concern.. clampedTo() ?
> > Otherwise documentation is very nice.
> >
> > > +{
> > > +     Rectangle result(*this);
> > > +
> > > +     result.width = std::min(result.width, boundary.width);
> > > +     result.x = std::clamp<int>(result.x, boundary.x,
> > > +                                boundary.x + boundary.width - result.width);
> > > +
> > > +     result.height = std::min(result.height, boundary.height);
> > > +     result.y = std::clamp<int>(result.y, boundary.y,
> > > +                                boundary.y + boundary.height - result.height);
> > > +
> > > +     return result;
> > > +}
> > > +
> >
> > Sorry the long comments, it's mostly about documentation and naming,
> > the actual content looks really nice!
> >
> > Thanks
> >   j
> >
> > >  /**
> > >   * \brief Compare rectangles for equality
> > >   * \return True if the two rectangles are equal, false otherwise
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel