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

[Kieran Bingham]

Hi Laurent, Jacopo,

On 10/07/2020 13:43, Laurent Pinchart wrote:
> 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.
>>>>>

Ohhh I love a good helper ...

Potentially foreseeing some growth in the Rectangle helpers too (see
recent threads).

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

Actually I suspect I might need an alignedDown for JPEG streams, as I
need to have sizes that are to JPEG requirements, aand and I can't
necessarily 'increase' the image size ... so it would have to crop down...

Even without both up and down variants, I'd rather see an explicit
direction in the function name, because otherwise it may be ambiguous
which way the alignment occurs.

(ok, so someone can read the function-doc ... but ...)

However, these helpers are certainly going to be helpful...

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> 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
--
Kieran