[libcamera-devel] [PATCH] libcamera: geometry: Add helper functions to the Size class

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jul 10, 2020 at 02:43:21PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 10, 2020 at 02:30:54PM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 10, 2020 at 12:05:16PM +0200, Jacopo Mondi wrote:
> > > On Fri, Jul 10, 2020 at 11:24:55AM +0300, Laurent Pinchart wrote:
> > > > Pipeline handlers commonly have to calculate the minimum or maximum of
> > > > multiple sizes, or align a size's width and height. Add helper functions
> > > > to the Size class to perform those tasks.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > >  include/libcamera/geometry.h | 25 +++++++++++++++++++++++++
> > > >  src/libcamera/geometry.cpp   | 26 ++++++++++++++++++++++++++
> > > >  test/geometry.cpp            | 22 ++++++++++++++++++++++
> > > >  3 files changed, 73 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > > index 7d4b8bcfe3d8..c1411290f163 100644
> > > > --- a/include/libcamera/geometry.h
> > > > +++ b/include/libcamera/geometry.h
> > > > @@ -8,6 +8,7 @@
> > > >  #ifndef __LIBCAMERA_GEOMETRY_H__
> > > >  #define __LIBCAMERA_GEOMETRY_H__
> > > >
> > > > +#include <algorithm>
> > > >  #include <string>
> > > >
> > > >  namespace libcamera {
> > > > @@ -43,6 +44,30 @@ struct Size {
> > > >
> > > >  	bool isNull() const { return !width && !height; }
> > > >  	const std::string toString() const;
> > > > +
> > > > +	Size alignedTo(unsigned int hAlignment, unsigned int vAlignment) const
> > >
> > > Should this take a Size ?
> >
> > I've thought about it, but it's really two alignments, not a size.
> 
> As soon as I used it, I realized using alignments is more natural.
> 
> > > Also, is aligned always assumed to happen to next largest integer ?
> > > Shouldn't this be alignIncrease or similar ?
> >
> > alignedUpTo and alignedDownTo ? roundedDownTo and roundedUpTo ? Do you
> > need both in the IPU3 pipeline handler ?
> 
> I don't and I don't think the 'down' versions are required. Just not
> assume alignment happens to the next larger integer. Are floor and
> ceil mis-leading as names in your opinion ?

Floor and ceil have established meanings in mathematical operations, so
I don't think they would be misleading, but floorTo() would sound weird,
and would also not convey that we're dealing with alignments.
alignedFloorTo() or alignedToFloor() also sounds weird.

> Have I forgot my tag in the previous reply?
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > > > +	{
> > > > +		return {
> > > > +			(width + hAlignment - 1) / hAlignment * hAlignment,
> > > > +			(height + vAlignment - 1) / vAlignment * vAlignment
> > > > +		};
> > > > +	}
> > > > +
> > > > +	Size boundedTo(const Size &bound) const
> > > > +	{
> > > > +		return {
> > > > +			std::min(width, bound.width),
> > > > +			std::min(height, bound.height)
> > > > +		};
> > > > +	}
> > > > +
> > > > +	Size expandedTo(const Size &expand) const
> > > > +	{
> > > > +		return {
> > > > +			std::max(width, expand.width),
> > > > +			std::max(height, expand.height)
> > > > +		};
> > > > +	}
> > > >  };
> > > >
> > > >  bool operator==(const Size &lhs, const Size &rhs);
> > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > > > index 24c44fb43acf..d647c5efbdb1 100644
> > > > --- a/src/libcamera/geometry.cpp
> > > > +++ b/src/libcamera/geometry.cpp
> > > > @@ -122,6 +122,32 @@ const std::string Size::toString() const
> > > >  	return std::to_string(width) + "x" + std::to_string(height);
> > > >  }
> > > >
> > > > +/**
> > > > + * \fn Size::alignedTo(unsigned int hAlignment, unsigned int vAlignment)
> > > > + * \brief Align the size horizontally and vertically
> > > > + * \param[in] hAlignment Horizontal alignment
> > > > + * \param[in] vAlignment Vertical alignment
> > > > + * \return A Size whose width and height are equal to the width and height of
> > > > + * this size aligned to the next multiple of \a hAlignment and \a vAlignment
> > > > + * respectively
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn Size::boundedTo(const Size &bound)
> > > > + * \brief Bound the size to \a bound
> > > > + * \param[in] bound The maximum size
> > > > + * \return A Size whose width and height are the minimum of the width and
> > > > + * height of this size and the \a bound size
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn Size::expandedTo(const Size &expand)
> > > > + * \brief Expand the size to \a expand
> > > > + * \param[in] expand The minimum size
> > > > + * \return A Size whose width and height are the maximum of the width and
> > > > + * height of this size and the \a expand size
> > > > + */
> > > > +
> > > >  /**
> > > >   * \brief Compare sizes for equality
> > > >   * \return True if the two sizes are equal, false otherwise
> > > > diff --git a/test/geometry.cpp b/test/geometry.cpp
> > > > index 904ad92c9448..5ef7cb7c9014 100644
> > > > --- a/test/geometry.cpp
> > > > +++ b/test/geometry.cpp
> > > > @@ -46,6 +46,28 @@ protected:
> > > >  			return TestFail;
> > > >  		}
> > > >
> > > > +		/* Test alignedTo(), boundedTo() and expandedTo() */
> > > > +		if (Size(0, 0).alignedTo(16, 8) != Size(0, 0) ||
> > > > +		    Size(1, 1).alignedTo(16, 8) != Size(16, 8) ||
> > > > +		    Size(16, 8).alignedTo(16, 8) != Size(16, 8)) {
> > > > +			cout << "Size::alignedTo() test failed" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		if (Size(0, 0).boundedTo({ 100, 100 }) != Size(0, 0) ||
> > > > +		    Size(200, 50).boundedTo({ 100, 100 }) != Size(100, 50) ||
> > > > +		    Size(50, 200).boundedTo({ 100, 100 }) != Size(50, 100)) {
> > > > +			cout << "Size::boundedTo() test failed" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		if (Size(0, 0).expandedTo({ 100, 100 }) != Size(100, 100) ||
> > > > +		    Size(200, 50).expandedTo({ 100, 100 }) != Size(200, 100) ||
> > > > +		    Size(50, 200).expandedTo({ 100, 100 }) != Size(100, 200)) {
> > > > +			cout << "Size::expandedTo() test failed" << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > >
> > > Looks good and will use it in the IPU3 series.
> > >
> > > >  		/* Test Size equality and inequality. */
> > > >  		if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true))
> > > >  			return TestFail;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

-- 
Regards,

Laurent Pinchart