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

[Umang Jain]

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.
>
> 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.
>
> 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
> + * \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.
> + *
> + * \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()
>   		 */
>   		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,

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;