[libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper functions
David Plowman
david.plowman at raspberrypi.com
Mon Oct 12 12:19:09 CEST 2020
On Sun, 11 Oct 2020 at 03:11, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> This looks good overall, but I'm afraid I have quite a few small
> comments, possibly on the nitpicking side, given that I think these
> functions need to be very explicit. Once we agree on the details I can
> help with the implementation if desired.
>
> Please also note that many of the comments below are open questions,
> especially when they refer to naming of functions, so feel free to tell
> me when you think my proposals are not good.
>
> On Tue, Sep 29, 2020 at 05:39:58PM +0100, David Plowman wrote:
> > These functions are aimed at making it easier to calculate cropping
> > rectangles, particularly in order to implement digital zoom.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/geometry.h | 20 ++++++
> > src/libcamera/geometry.cpp | 129 +++++++++++++++++++++++++++++++++++
>
> We need test cases for the new functions, in test/geometry.cpp.
>
> > 2 files changed, 149 insertions(+)
> >
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index 02fb63c0..0447ee3e 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,17 @@ public:
> > std::max(height, expand.height)
> > };
> > }
> > +
> > + Size alignedDownToAspectRatio(const Size &ratio) const;
> > + Size alignedUpToAspectRatio(const Size &ratio) const;
>
> The names are a bit long but I don't have better alternatives to
> propose.
Maybe "Ar" instead of "AspectRatio"? e.g. alignedDownToAr
But I'm not convinced. Nor do I think it matters greatly...
In line with your comment just below ("To be complete"), do you think
we should include in-place versions of all these functions,
alginUpToAspectRatio, centreTo etc.?
>
> > +
> > + Rectangle centredTo(const Rectangle ®ion,
> > + int offsetX = 0, int offsetY = 0) const;
> > };
> >
> > +Size operator*(const Size &size, float f);
> > +Size operator/(const Size &size, float f);
>
> To be complete, should we also have member operators that modify the
> size in-place ?
>
> Size &operator*=(float f);
> Size &operator/=(float f);
Agreed - why not!
>
> > +
> > bool operator==(const Size &lhs, const Size &rhs);
> > bool operator<(const Size &lhs, const Size &rhs);
> >
> > @@ -176,6 +187,11 @@ public:
> > {
> > }
> >
> > + constexpr explicit Rectangle(const Size &size)
>
> Not saying it's a bad idea, but why do you think this constructor should
> be explicit ?
I think I'm a bit paranoid. If you did a calculation with various
Rectangles and Sizes flying around, and then you want (for example,
and borrowing the translate function from below...)
answer = rect.translate(dx, dy)
but wrote
answer = size.translate(dx, dy)
It's a question of whether you want the compiler to catch those and
force you to say exactly what you mean. Maybe you end up with
something a little clearer to understand too. Again, I don't really
mind...
>
> > + : x(0), y(0), width(size.width), height(size.height)
> > + {
> > + }
> > +
> > int x;
> > int y;
> > unsigned int width;
> > @@ -183,6 +199,10 @@ public:
> >
> > bool isNull() const { return !width && !height; }
> > const std::string toString() const;
> > +
> > + Size size() const;
> > +
> > + Rectangle boundedTo(const Rectangle &boundary) const;
> > };
> >
> > bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index b12e1a62..649ee179 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -143,6 +143,88 @@ const std::string Size::toString() const
> > * height of this size and the \a expand size
> > */
> >
> > +/**
> > + * \brief Align down to the aspect ratio given by \a ratio
>
> Aligning down to an aspect ratio sounds a bit strange to me, as it's not
> really about alignments. Would shrunkToAspectRatio() be a better name ?
> The documentation could then become
>
> * \brief Compute the size shrunk to achieve the given aspect \a ratio
> * \param[in] ratio The desired aspect ratio
> *
> * This function computes the largest size that fits in this size and
> * that matches the aspect ratio of \ratio.
> *
> * \return The size shrunk to the aspect \ratio
>
> Same for the next function, named expandedToAspectRatio().
Yes, shrunk/expanded sounds good to me. An alternative might be
boundedToAspectRatio (instead of shrunk), as we tend to use bounded
elsewhere.
>
> > + * \param[in] ratio The size whose aspect ratio to align down to
> > + * \return A Size whose width and height are equal to the width and height
> > + * of this Size aligned down to the aspect ratio of \a ratio
> > + */
> > +Size Size::alignedDownToAspectRatio(const Size &ratio) const
> > +{
> > + uint64_t ratio1 = static_cast<uint64_t>(width) *
> > + static_cast<uint64_t>(ratio.height);
> > + uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> > + static_cast<uint64_t>(height);
> > +
> > + if (ratio1 > ratio2)
> > + return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> > + else
> > + return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> > +}
> > +
> > +/**
> > + * \brief Align up to the aspect ratio given by \a ratio
> > + * \param[in] ratio The size whose aspect ratio to align up to
> > + * \return A Size whose width and height are equal to the width and height
> > + * of this Size aligned up to the aspect ratio of \a ratio
> > + */
> > +Size Size::alignedUpToAspectRatio(const Size &ratio) const
> > +{
> > + uint64_t ratio1 = static_cast<uint64_t>(width) *
> > + static_cast<uint64_t>(ratio.height);
> > + uint64_t ratio2 = static_cast<uint64_t>(ratio.width) *
> > + static_cast<uint64_t>(height);
> > +
> > + if (ratio1 < ratio2)
> > + return { static_cast<unsigned int>(ratio2 / ratio.height), height };
> > + else
> > + return { width, static_cast<unsigned int>(ratio1 / ratio.width) };
> > +}
> > +
> > +/**
> > + * \brief Centre a rectangle of this size within another rectangular region,
> > + * 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
> > + *
> > + * A Rectangle of this object's size is positioned within the Rectangle
> > + * 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 with the horizontal and vertical sizes of
> > + * this Size instance, centred with offsets within a region
> > + */
>
> This feels very ad-hoc, and the name isn't very explicit. Would it make
> sense to split it in the following basic operations ? I'll leave any
> documentation out as an exercise to see if their names are explicit.
>
> Before:
>
> Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const;
>
> Size size{ ... };
> Rectangle region{ ... };
> int dx = ..., dy = ...;
>
> Rectangle r = size.centredTo(region, dx, dy);
>
> After:
>
> class Point
> {
> public:
> Point()
> : x(0), y(0)
> {
> }
>
> int x;
> int y;
> };
>
> Point Rectangle::center() const;
> Rectangle &Rectangle::moveCenter(const Point &position);
> Rectangle &Rectangle::translate(int dx, int dx);
>
> Size size{ ... };
> Rectangle region{ ... };
> int dx = ..., dy = ...;
>
> Rectangle r{size};
> r.moveCenter(region.center()).translate(dx, dy);
>
> or
>
> Rectangle r = Rectangle{size}.moveCenter(region.center()).translate(dx, dy);
>
> It's a bit longer, but I think it's move explicit, and the operations
> can be reused for other purposes.
I think that's a fair cop, as we say!
BTW, do we have a policy on UK vs. American spelling? (centre/center)
I think I still prefer something like "centredTo" instead of
"moveCenter", it feels more like the other functions. An in-place
version could be "centreTo". How about "translateBy" and
"translatedBy" instead of "translate"...?
>
> > +Rectangle Size::centredTo(const Rectangle ®ion, int offsetX, int offsetY) const
> > +{
> > + int x = (region.width - width) / 2 + region.x + offsetX;
> > + int y = (region.height - height) / 2 + region.y + offsetY;
> > +
> > + return Rectangle(x, y, width, height);
> > +}
> > +
> > +/**
> > + * \brief Scale size up by the given factor
>
> * \param[in] size The size to scale
> * \param[in] f The scaling factor
>
> > + * \return The scaled Size
> > + */
> > +Size operator*(const Size &size, float f)
>
> I'd name the parameter factor.
>
> > +{
> > + return Size(size.width * f, size.height * f);
> > +}
> > +
> > +/**
> > + * \brief Scale size down by the given factor
> > + * \return The scaled Size
> > + */
> > +Size operator/(const Size &size, float f)
>
> Same comments here.
>
> > +{
> > + return Size(size.width / f, size.height / f);
> > +}
> > +
> > /**
> > * \brief Compare sizes for equality
> > * \return True if the two sizes are equal, false otherwise
> > @@ -365,6 +447,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 zero offsets and the given \a size
>
> We don't use a concept of "offset" for the Rectangle class at the
> moment. I'd write this
>
> * \brief Construct a Rectangle of \a size located at (0,0)
Sure. I might like "top left corner located at..." given that there's
increasing amounts of "centering" going-on.
>
> > + * \param[in] size The desired Rectangle size
> > + */
> > +
> > /**
> > * \var Rectangle::x
> > * \brief The horizontal coordinate of the rectangle's top-left corner
> > @@ -404,6 +492,47 @@ const std::string Rectangle::toString() const
> > return ss.str();
> > }
> >
> > +/**
> > + * \brief Retrieve the size of this rectangle
> > + * \return A Size reporting the Rectangle horizontal and vertical sizes
>
> I think "The Rectangle size" should be enough.
>
> > + */
> > +Size Rectangle::size() const
> > +{
> > + return Size(width, height);
> > +}
> > +
> > +/**
> > + * \brief Bound a Rectangle so as not to exceeed another Rectangle
>
> this operation is also commonly referred to as computing the
> intersection of two rectangles. We would thus name the function
> intersection(), intersected() or intersectedWith(). The related
> operation of computing the union of two rectangles would then be named
> union(), united() or unitedWith() I suppose, while with bountedTo() it
> would be named expandedTo(). I don't have a strong preference.
>
> On a separate note, if we want to shorten the code, we could also define
> operator& as an alias for this function (and operator| for the union).
>
> > + * \param[in] boundary The limit that the returned Rectangle will not exceed
> > + *
> > + * The Rectangle is bounded 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
>
> s/bounary/boundary/
>
> > + * 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.
>
> So much for my question above :-)
>
> Hmmmm... I think the name is a bit misleading then, or at least not
> self-explicit. I fear this will cause confusion. I wonder if we could
> find a better name, or a different set of standard operations that would
> result in the same outcome when composed. I'll try to propose
> alternatives when reviewing the patch that uses this function.
I guess there are two operations going on:
* Limiting the size of the rectangle (to be no larger than the boundary).
* Translating it so that no part of it exceeds the boundary.
For the first one... well, it could stay as "boundedTo". Might take a
Size rather than a Rectangle as the parameter? However it's not very
clear what happens to the top left corner, though in this case it
doesn't matter.
For the second one... "restrictedTo"? or even "pannedTo" (in the sense
that the source Rectangle is being "panned" to lie within the
boundary)? perhaps "shiftedTo" is less weird.
Tricky!
Thanks and best regards
David
>
> > + *
> > + * \return A Rectangle that does not extend beyond a boundary Rectangle
> > + */
> > +Rectangle Rectangle::boundedTo(const Rectangle &boundary) const
> > +{
> > + 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;
> > +}
> > +
> > /**
> > * \brief Compare rectangles for equality
> > * \return True if the two rectangles are equal, false otherwise
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list