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

[David Plowman]

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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel