[libcamera-devel,v4,4/5] libcamera: Add geometry helper functions

10098 diff mbox series

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@raspberrypi.com>
---
 include/libcamera/geometry.h |  53 +++++++
 src/libcamera/geometry.cpp   | 296 +++++++++++++++++++++++++++++++++++
 2 files changed, 349 insertions(+)

Hi David,

Thank you for the patch.

On Mon, Oct 19, 2020 at 01:51:55PM +0100, David Plowman wrote:
> These functions are aimed at making it easier to calculate cropping
> rectangles, particularly in order to implement digital zoom.

After reviewing how they're used, they definitely fulfil their job :-)

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/geometry.h |  53 +++++++
>  src/libcamera/geometry.cpp   | 296 +++++++++++++++++++++++++++++++++++
>  2 files changed, 349 insertions(+)
> 
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 02fb63c0..5935cc63 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -13,6 +13,25 @@
>  
>  namespace libcamera {
>  
> +class Point
> +{
> +public:
> +	constexpr Point()
> +		: x(0), y(0)
> +	{
> +	}
> +
> +	constexpr Point(int xpos, int ypos)
> +		: x(xpos), y(ypos)
> +	{
> +	}

If you want to use this as an offset for the translate operation, you'll
need a

	Point operator-() const
	{
		return { -x, -y };
	}

> +
> +	int x;
> +	int y;
> +};
> +
> +class Rectangle;

Nitpicking, I'd move this before Point, to have all forward declarations
(even if there's one only) at the beginning.

> +
>  class Size
>  {
>  public:
> @@ -32,6 +51,8 @@ public:
>  	bool isNull() const { return !width && !height; }
>  	const std::string toString() const;
>  
> +	Point center() const;
> +
>  	Size &alignDownTo(unsigned int hAlignment, unsigned int vAlignment)
>  	{
>  		width = width / hAlignment * hAlignment;
> @@ -93,8 +114,19 @@ public:
>  			std::max(height, expand.height)
>  		};
>  	}
> +
> +	Size boundedToAspectRatio(const Size &ratio) const;
> +	Size expandedToAspectRatio(const Size &ratio) const;
> +
> +	Rectangle centeredTo(const Point &center) const;
>  };
>  
> +Size operator*(const Size &size, float factor);
> +Size operator/(const Size &size, float factor);
> +
> +Size &operator*=(Size &size, float factor);
> +Size &operator/=(Size &size, float factor);

By the way, the reason why the comparison operators are defined outside
of the class is to allow implicit construction of either side of the
operator. As this isn't an issue for the above arithmetic operators, you
can also define them as class members if you prefer. Entirely up to you.

> +
>  bool operator==(const Size &lhs, const Size &rhs);
>  bool operator<(const Size &lhs, const Size &rhs);
>  
> @@ -176,6 +208,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 +220,22 @@ public:
>  
>  	bool isNull() const { return !width && !height; }
>  	const std::string toString() const;
> +
> +	Point center() const;
> +

We try to add blank lines between groups of related functions, but not
necessarily between all functions. You could thus drop this one I think.

> +	Size size() const;
> +
> +	Rectangle &translateBy(int dx, int dy);
> +
> +	Rectangle &rescaleTo(const Size &from, const Size &to);

Maybe scaleTo instead of rescaleTo to make it slightly shorter ? It's no
big deal in this line, but there operations have a tendency to be
chained, creating fairly long lines. I would also declare this before
translateBy(), as in the absence of any particular constraint on the
ordering, I tend to go for alphabetical.

> +
> +	Rectangle translatedBy(int dx, int dy) const;
> +
> +	Rectangle rescaledTo(const Size &from, const Size &to) const;
> +
> +	Rectangle boundedTo(const Rectangle &bound) const;
> +
> +	Rectangle clampedTo(const Rectangle &boundary) const;

Same for these functions, I'd order them alphabetically, and remove the
blank lines in between.

>  };
>  
>  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index b12e1a62..75741c1e 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -17,6 +17,36 @@
>  
>  namespace libcamera {
>  
> +/**
> + * \struct Point

Point is a class.

> + * \brief Describe a point in two-dimensional space
> + *
> + * The Point structure defines a point in two-dimensional space with integer
> + * precision. The coordinates of a Point may be negative as well as positive.
> + */
> +
> +/**
> + * \fn Point::Point()
> + * \brief Construct a Point with x and y set to 0
> + */
> +
> +/**
> + * \fn Point::Point(int x, int y)

s/x/xpos/ and s/y/ypos/

> + * \brief Construct a Point at given \a x and \a y values
> + * \param[in] xpos The x-coordinate
> + * \param[in] ypos The y-coordinate
> + */
> +
> +/**
> + * \var Point::x
> + * \brief The x-coordinate of the Point
> + */
> +
> +/**
> + * \var Point::y
> + * \brief The y-coordinate of the Point
> + */
> +
>  /**
>   * \struct Size
>   * \brief Describe a two-dimensional size
> @@ -61,6 +91,15 @@ const std::string Size::toString() const
>  	return std::to_string(width) + "x" + std::to_string(height);
>  }
>  
> +/**
> + * \brief Retrieve the center point of this size
> + * \return The center Point
> + */
> +Point Size::center() const
> +{
> +	return { static_cast<int>(width / 2), static_cast<int>(height / 2) };

No need to cast, int / int gives you an int.

> +}
> +
>  /**
>   * \fn Size::alignDownTo(unsigned int hAlignment, unsigned int vAlignment)
>   * \brief Align the size down horizontally and vertically in place
> @@ -143,6 +182,101 @@ const std::string Size::toString() const
>   * height of this size and the \a expand size
>   */
>  
> +/**
> + * \brief Bound the size down to match the aspect ratio given by \a ratio
> + * \param[in] ratio The size whose aspect ratio must be matched
> + * \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::boundedToAspectRatio(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 Expand the size to match the aspect ratio given by \a ratio
> + * \param[in] ratio The size whose aspect ratio must be matched
> + * \return A Size whose width and height are equal to the width and height
> + * of this Size expanded up to the aspect ratio of \a ratio
> + */
> +Size Size::expandedToAspectRatio(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 Center a rectangle of this size at a given Point
> + * \param[in] center The center point the Rectangle is to have
> + *
> + * A Rectangle of this object's size is positioned so that its center
> + * is at the given Point.
> + *
> + * \return A Rectangle of this size, centered at the given Point.
> + */
> +Rectangle Size::centeredTo(const Point &center) const
> +{
> +	int x = center.x - width / 2;
> +	int y = center.y - height / 2;
> +
> +	return { x, y, width, height };
> +}
> +
> +/**
> + * \brief Scale size up by the given factor

 \param[in] size The size
 \param[in] factor The factor

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

Same here and in the functions below.

> + * \return The scaled Size
> + */
> +Size operator/(const Size &size, float factor)
> +{
> +	return Size(size.width / factor, size.height / factor);
> +}
> +
> +/**
> + * \brief Scale size up by the given factor in place
> + * \return The scaled Size
> + */
> +Size &operator*=(Size &size, float factor)
> +{
> +	size.width *= factor;
> +	size.height *= factor;
> +	return size;
> +}
> +
> +/**
> + * \brief Scale size down by the given factor in place
> + * \return The scaled Size
> + */
> +Size &operator/=(Size &size, float factor)
> +{
> +	size.width /= factor;
> +	size.height /= factor;
> +	return size;
> +}
> +
>  /**
>   * \brief Compare sizes for equality
>   * \return True if the two sizes are equal, false otherwise
> @@ -365,6 +499,13 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
>   * \param[in] height The height
>   */
>  
> +/**
> + * \fn Rectangle::Rectangle(const Size &size)
> + * \brief Construct a Rectangle of \a size with its top left corner located
> + * at (0,0).
> + * \param[in] size The desired Rectangle size
> + */
> +
>  /**
>   * \var Rectangle::x
>   * \brief The horizontal coordinate of the rectangle's top-left corner
> @@ -404,6 +545,161 @@ const std::string Rectangle::toString() const
>  	return ss.str();
>  }
>  
> +/**
> + * \brief Retrieve the center point of this rectangle
> + * \return The center Point
> + */
> +Point Rectangle::center() const
> +{
> +	return { x + static_cast<int>(width / 2), y + static_cast<int>(height / 2) };

No need for casts either.

> +}
> +
> +/**
> + * \brief Retrieve the size of this rectangle
> + * \return The Rectangle size
> + */
> +Size Rectangle::size() const
> +{
> +	return Size(width, height);
> +}
> +
> +/**
> + * \brief Translate this Rectangle by the given amounts in place
> + * \param[in] dx The amount to translate the Rectangle in the x direction
> + * \param[in] dy The amount to translate the Rectangle in the y direction
> + *
> + * The Rectangle is translated by the given amounts \a dx and \a dy.
> + *
> + * \return A reference to this object
> + */
> +Rectangle &Rectangle::translateBy(int dx, int dy)
> +{
> +	x += dx;
> +	y += dy;
> +
> +	return *this;
> +}
> +
> +/**
> + * \brief Rescale this Rectangle in place from one coordinate frame to another
> + * \param[in] from The current coordinate frame of the Rectangle
> + * \param[in] to The new coordinate frame for the Rectangle
> + *
> + * The Rectangle can be regarded as being embedded within a coordinate frame
> + * given by \a from. It is rescaled so as to occupy the equivalent part of the
> + * new coordinate frame given by \a to.
> + *
> + * This is purely a scaling operation, and the Rectangle does not need to lie
> + * inside the coordinate frames.

I find the coordinate frame explanation a bit unclear. Would it be
clearer if the from and to parameters were denumerator and numerator
respectively ? We could then document the function as applying a
non-uniform homothety centered at (0,0) and whose ratio is given by the
numerator and denominator.

Bonus point if we could convey through the function name this it applies
an homothety whose centre is not the centre of the rectangle, but I
can't think of a good name :-/

Same comments for rescaledTo().

> + *
> + * \return A reference to this object
> + */
> +Rectangle &Rectangle::rescaleTo(const Size &from, const Size &to)
> +{
> +	x = static_cast<int64_t>(x) * to.width / from.width;
> +	y = static_cast<int64_t>(y) * to.height / from.height;
> +	width = static_cast<uint64_t>(width) * to.width / from.width;
> +	height = static_cast<uint64_t>(height) * to.height / from.height;
> +
> +	return *this;
> +}
> +
> +/**
> + * \brief Translate a Rectangle by the given amounts
> + * \param[in] dx The amount to translate the Rectangle in the x direction
> + * \param[in] dy The amount to translate the Rectangle in the y direction
> + *
> + * The Rectangle is translated by the given amounts \a dx and \a dy.
> + *
> + * \return The translated Rectangle
> + */
> +Rectangle Rectangle::translatedBy(int dx, int dy) const
> +{
> +	return { x + dx, y + dy, width, height };
> +}
> +
> +/**
> + * \brief Rescale a Rectangle from one coordinate frame to another
> + * \param[in] from The coordinate frame of the given Rectangle
> + * \param[in] to The new coordinate frame for the new Rectangle
> + *
> + * The Rectangle can be regarded as being embedded within a coordinate frame
> + * given by \a from. A new Rectangle, rescaled so as to occupy the equivalent
> + * part of the new coordinate frame given by \a to, is created and returned.
> + *
> + * This is purely a scaling operation, and the Rectangle does not need to lie
> + * inside the coordinate frames.
> + *
> + * \return The rescaled Rectangle
> + */
> +Rectangle Rectangle::rescaledTo(const Size &from, const Size &to) const
> +{
> +	int scaledX = static_cast<int64_t>(x) * to.width / from.width;
> +	int scaledY = static_cast<int64_t>(y) * to.height / from.height;
> +	unsigned int scaledWidth = static_cast<uint64_t>(width) * to.width / from.width;
> +	unsigned int scaledHeight = static_cast<uint64_t>(height) * to.height / from.height;
> +
> +	return { scaledX, scaledY, scaledWidth, scaledHeight };
> +}
> +
> +/**
> + * \brief Calculate the intersection of this Rectangle with another
> + * \param[in] bound The Rectangle that is intersected with this Rectangle
> + *
> + * This method calculates the standard intersection of two Rectangles. If the
> + * Rectangles do not overlap in either the x or y direction, then the size
> + * of that dimension in the result (its width or height) is set to zero. Even
> + * when one dimension is set to zero, note that the other dimension may still
> + * have a positive value if there was some overlap.
> + *
> + * \return A Rectangle that is the intersection of the input Rectangles

We try not to pluralize class names, so I'd write 'rectangles' instead
of 'Rectangles'.

> + */
> +Rectangle Rectangle::boundedTo(const Rectangle &bound) const
> +{
> +	int topLeftX = std::max(x, bound.x);
> +	int topLeftY = std::max(y, bound.y);
> +	int bottomRightX = std::min<int>(x + width, bound.x + bound.width);
> +	int bottomRightY = std::min<int>(y + height, bound.y + bound.height);
> +
> +	int newWidth = bottomRightX - topLeftX;
> +	int newHeight = bottomRightY - topLeftY;
> +
> +	return { topLeftX, topLeftY,
> +		 static_cast<unsigned int>(std::max(newWidth, 0)),
> +		 static_cast<unsigned int>(std::max(newHeight, 0)) };

I think you can drop the cast, in which case this could be written

	unsigned int newWidth = std::max(bottomRightX - topLeftX, 0);
	unsigned int newHeight = std::mx(bottomRightY - topLeftY, 0);

	return { topLeftX, topLeftY, newWidth, newHeight };

Up to you.

> +}
> +
> +/**
> + * \brief Clamp a Rectangle so as not to exceed another Rectangle
> + * \param[in] boundary The limit that the returned Rectangle will not exceed
> + *
> + * The Rectangle is modified 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 boundary, 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

Maybe s/We note/Note/ ?

> + * \sa Rectangle::boundedTo

And "... intersection function, which is provided by boundedTo()." ?

> + *
> + * \return A Rectangle that does not extend beyond a boundary Rectangle
> + */
> +Rectangle Rectangle::clampedTo(const Rectangle &boundary) const

Finding good names is difficult :-S I'm not sure if it's better, but
enclosedIn() or containtedIn() could also be candidates, although they
may sound like they would check if the rectangle is enclosed in the
boundary, not perform the action of enclosing it (on the other hand, we
prefix check functions with 'is', so maybe there's no risk of
confusion).

I think we're finally converging. I know it's not fun to write these
helpers (and even less so to document them), but your work will really
make a difference. Thank you for that.

By the way, I'm not sure I would have dared reminding about test cases
if you hadn't mentioned them in the cover letter :-) (Or maybe I would
have, but I would be more ashamed.)

> +{
> +	/* We can't be bigger than the boundary rectangle. */
> +	Rectangle result = boundedTo(Rectangle{ x, y, boundary.size() });
> +
> +	result.x = std::clamp<int>(result.x, boundary.x,
> +				   boundary.x + boundary.width - result.width);
> +	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