[libcamera-devel] [RFC PATCH 3/4] libcamera: Add geometry helper functions
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 21 18:19:29 CEST 2020
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 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 ®ion,
> > > + 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 ®ion, 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 at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list