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

[Laurent Pinchart]

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 ?

> > + * \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 });

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

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

-- 
Regards,

Laurent Pinchart