[libcamera-devel] [PATCH v3 4/6] libcamera: Add geometry helper functions

[Laurent Pinchart]

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.

> +
> +	Rectangle centredTo(const Rectangle &region,
> +			    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);

> +
>  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 ?

> +		: 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().

> + * \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 &region, 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.

> +Rectangle Size::centredTo(const Rectangle &region, 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)

> + * \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.

> + *
> + * \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