[libcamera-devel] [PATCH] libcamera: geometry: Add Size members to clamp to a min/max

[Umang Jain]

Hi,

On 4/5/22 19:48, Laurent Pinchart wrote:
> On Tue, Apr 05, 2022 at 07:11:15PM +0530, Umang Jain via libcamera-devel wrote:
>> Hi Kieran,
>>
>> Thank you for the patch.
>>
>> On 3/31/22 17:35, Kieran Bingham via libcamera-devel wrote:
>>> Provide two new operations to support clamping a Size to a given
>>> minimum or maximum Size, or returning a const variant of the same
>>> restriction.
> Did you really mean "restriction" here ?
>
>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>> ---
>>>
>>> I was expecting to use this for clamping the block width and height
>>> for the AF hardware restrictions on the IPU3 ... but it turns out to be
>>> not quite appropriate to use a Size there, as the clamped values are
>>> stored in an IPU3 struct directly.
>>>
>>> However, having made this - I think it is likely to be useful elsewhere
>>> so posting so it doesn't get lost.
> .clamp(min, max) could be implemented as .expandTo(min).shrinkTo(max),
> but Size::clamp() is likely more explicit and better.
>
>>> Tests added, so it could be merged already even if there is no current
>>> user yet. I expect it's more likely to get used if it exists, than if it
>>> doesn't ;-)
>> +1
>>
>>>    include/libcamera/geometry.h | 16 ++++++++++++++++
>>>    src/libcamera/geometry.cpp   | 21 +++++++++++++++++++++
>>>    test/geometry.cpp            | 24 ++++++++++++++++++++++--
>>>    3 files changed, 59 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
>>> index 7838b6793617..a447beb55965 100644
>>> --- a/include/libcamera/geometry.h
>>> +++ b/include/libcamera/geometry.h
>>> @@ -93,6 +93,13 @@ public:
>>>    		return *this;
>>>    	}
>>>    
>>> +	Size &clamp(const Size &minimum, const Size &maximum)
>>> +	{
>>> +		width = std::clamp(width, minimum.width, maximum.width);
>>> +		height = std::clamp(height, minimum.height, maximum.height);
>>> +		return *this;
>>> +	}
>>> +
>>>    	Size &growBy(const Size &margins)
>>>    	{
>>>    		width += margins.width;
>>> @@ -141,6 +148,15 @@ public:
>>>    		};
>>>    	}
>>>    
>>> +	__nodiscard constexpr Size clampedTo(const Size &minimum,
>>> +					     const Size &maximum) const
>>> +	{
>>> +		return {
>>> +			std::clamp(width, minimum.width, maximum.width),
>>> +			std::clamp(height, minimum.height, maximum.height)
>>> +		};
>>> +	}
>>> +
>>>    	__nodiscard constexpr Size grownBy(const Size &margins) const
>>>    	{
>>>    		return {
>>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
>>> index cb3c2de18d5e..7ac053a536c1 100644
>>> --- a/src/libcamera/geometry.cpp
>>> +++ b/src/libcamera/geometry.cpp
>>> @@ -173,6 +173,18 @@ const std::string Size::toString() const
>>>     * \return A reference to this object
>>>     */
>>>    
>>> +/**
>>> + * \fn Size::clamp(const Size &minimum, const Size &maximum)
>>> + * \brief Restrict the size to be constrained within the \a minimum and \a maximum
> "Restrict the size to be constrainted" sounds quite weird to me. Maybe
> using the word "clamp" would be better ?


Now that I am reading it, I agree with Laurent :S

>
>>> + * \param[in] minimum The minimum size
>>> + * \param[in] maximum The maximum size
>>> + *
>>> + * This function restricts the width and height to the constraints of the \a
>>> + * minimum and the \a maximum sizes given.
> And here too, the help text doesn't make it clear what the function
> does. I get more information from the function name than from the
> documentation :-)
>
> Same for clampedTo().
>
>>> + *
>>> + * \return A reference to this object
>>> + */
>>> +
>>>    /**
>>>     * \fn Size::growBy(const Size &margins)
>>>     * \brief Grow the size by \a margins in place
>>> @@ -231,6 +243,15 @@ const std::string Size::toString() const
>>>     * height of this size and the \a expand size
>>>     */
>>>    
>>> +/**
>>> + * \fn Size::clampedTo(const Size &minimum, const Size &maximum)
>>> + * \brief Restrict the size to be constrained within the \a minimum and \a maximum
>>> + * \param[in] minimum The minimum size
>>> + * \param[in] maximum The maximum size
>>> + * \return A size whose width and height match this size within the constraints
>>> + * of the \a minimum and \a maximum sizes given.
>>> + */
>>> +
>>>    /**
>>>     * \fn Size::grownBy(const Size &margins)
>>>     * \brief Grow the size by \a margins
>>> diff --git a/test/geometry.cpp b/test/geometry.cpp
>>> index 5125692496b3..1d3d3cad7174 100644
>>> --- a/test/geometry.cpp
>>> +++ b/test/geometry.cpp
>>> @@ -106,7 +106,7 @@ protected:
>>>    
>>>    		/*
>>>    		 * Test alignDownTo(), alignUpTo(), boundTo(), expandTo(),
>>> -		 * growBy() and shrinkBy()
>>> +		 * clamp() growBy() and shrinkBy()
> s/ growBy/, growBy/
>
>>>    		 */
>>>    		Size s(50, 50);
>>>    
>>> @@ -134,6 +134,18 @@ protected:
>>>    			return TestFail;
>>>    		}
>>>    
>>> +		s.clamp({ 80, 120 }, { 160, 240 });
>>
>> is it okay to ignore the reference returned by .clamp() ? I think so,
>> since it's doesn't have __nodiscard anyways,
> The __nodiscard attribute was added to the "-ed" versions of the
> functions, to make sure that someone will not accidentally write
>
> 	s.clampedTo({ 80, 120 }, { 160, 240 });
>
> when they meant
>
> 	s.clamp({ 80, 120 }, { 160, 240 });


Make sense.

>
> clamp() is meant to be called with its return value potentially ignored,
> otherwise it would be hard to clamp a Size() in-place.


Yeah, I wasn't sure if the complier warns out(or not) when we are 
ignoring the returned references. Yes, the in-place op. makes and I have 
written them a numerous times (with ignoring the reference), but 
something I didn't give a thought about, until now.

>
>> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
>>
>>> +		if (s != Size(80, 120)) {
>>> +			cout << "Size::clamp (minium) test failed" << endl;
>>> +			return TestFail;
>>> +		}
>>> +
>>> +		s.clamp({ 20, 30 }, { 50, 50 });
>>> +		if (s != Size(50, 50)) {
>>> +			cout << "Size::clamp (maximum) test failed" << endl;
>>> +			return TestFail;
>>> +		}
>>> +
>>>    		s.growBy({ 10, 20 });
>>>    		if (s != Size(60, 70)) {
>>>    			cout << "Size::growBy() test failed" << endl;
>>> @@ -162,7 +174,7 @@ protected:
>>>    
>>>    		/*
>>>    		 * Test alignedDownTo(), alignedUpTo(), boundedTo(),
>>> -		 * expandedTo(), grownBy() and shrunkBy()
>>> +		 * expandedTo(), clampedTo(), grownBy() and shrunkBy()
>>>    		 */
>>>    		if (Size(0, 0).alignedDownTo(16, 8) != Size(0, 0) ||
>>>    		    Size(1, 1).alignedDownTo(16, 8) != Size(0, 0) ||
>>> @@ -192,6 +204,14 @@ protected:
>>>    			return TestFail;
>>>    		}
>>>    
>>> +		if (Size(0, 0).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 40) ||
>>> +		    Size(100, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 90) ||
>>> +		    Size(30, 100).clampedTo({ 60, 40 }, { 80, 90 }) != Size(60, 90) ||
>>> +		    Size(100, 30).clampedTo({ 60, 40 }, { 80, 90 }) != Size(80, 40)) {
>>> +			cout << "Size::clampedTo() test failed" << endl;
>>> +			return TestFail;
>>> +		}
>>> +
>>>    		if (Size(0, 0).grownBy({ 10, 20 }) != Size(10, 20) ||
>>>    		    Size(200, 50).grownBy({ 10, 20 }) != Size(210, 70)) {
>>>    			cout << "Size::grownBy() test failed" << endl;