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

[Jacopo Mondi]

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


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

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

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