| Message ID | 20220331120556.2147745-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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;
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@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@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;
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@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@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;
Quoting Laurent Pinchart (2022-04-05 15:18:07) > 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 ? I did yes. I mean the same restriction of the minimum and maximum Size but as a const. > > > Signed-off-by: Kieran Bingham <kieran.bingham@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. Yes, a caller could call .expandTo.shrinkTo instead already but I don't think that should be the implementation detail here. > > > > 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 ? I was trying to avoid using the term I'm describing to describe itself in the description. Re-reading now it still sounds fine to me, but I can change it. Do you mean you prefer: Clamp the size to be constrained within the minimum and maximum or Restrict the size to be clamped within the minimum and 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. > > 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 :-) Is it because of the word 'constraints' as you mentioned above? or something else? Documenting a function called 'clamp' as 'it will clamp the values' doesn't add any value either. > > 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/ Ack. > > > > */ > > > 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@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
Hi Kieran, On Thu, Apr 07, 2022 at 03:38:56PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2022-04-05 15:18:07) > > 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 ? > > I did yes. I mean the same restriction of the minimum and maximum Size > but as a const. > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@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. > > Yes, a caller could call .expandTo.shrinkTo instead already but I don't > think that should be the implementation detail here. > > > > > 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 ? > > I was trying to avoid using the term I'm describing to describe itself > in the description. The fact that the function is named from the term that best describes its purpose means we picked the right name :-) > Re-reading now it still sounds fine to me, but I can change it. > > Do you mean you prefer: > > Clamp the size to be constrained within the minimum and maximum I meant this, yes. Or maybe Clamp the size to the \a minimum and \a maximum values (with s/to/between/, and/or adding the word "range" somewhere, if desired) > or > > Restrict the size to be clamped within the minimum and 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. > > > > 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 :-) > > Is it because of the word 'constraints' as you mentioned above? or > something else? Yes, it's "restricts to the constraints" that sounds convoluted to me. > Documenting a function called 'clamp' as 'it will clamp the values' > doesn't add any value either. > > > 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/ > > Ack. > > > > > */ > > > > 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@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;
Quoting Laurent Pinchart (2022-04-07 15:58:09) > Hi Kieran, > > On Thu, Apr 07, 2022 at 03:38:56PM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2022-04-05 15:18:07) > > > 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 ? > > > > I did yes. I mean the same restriction of the minimum and maximum Size > > but as a const. > > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@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. > > > > Yes, a caller could call .expandTo.shrinkTo instead already but I don't > > think that should be the implementation detail here. > > > > > > > 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 ? > > > > I was trying to avoid using the term I'm describing to describe itself > > in the description. > > The fact that the function is named from the term that best describes > its purpose means we picked the right name :-) Perhaps you only know the term 'clamp' in regards to the coding implementation. Describing the function 'clamp()' with the word 'clamp' conveys the wrong meaning to me. (And maybe other english speakers who have used clamps?) But to me - a clamp is a mechanical device used to temporarily fix two (or more) objects such that they remain in the same position, usually while you then perform some other operation like screwing the two items together, or marking them. You 'could' use a clamp to squash something ... but I don't think that quite fits the real definition of std::clamp() (At least I don't think it can constrain to a non-zero minimum). google-translate tells me that a clamp might be known as 'serrer' in French or 'Morsetto' in Italian, so perhaps 'clamp' is only known as it's use in code ... but I haven't validated those usages in a sentence ;-) Anyway, I presume someone who wants to use this - knows what the coding term is - but my point is - I don't think the function name is automatically the best way to describe what it does. If it was using both the term restrict and contrain in the same sentence I could offer: "Constrain the Size to be within the dimensions of both the minimum and maximum values." Which also works with 's/Constrain/Restrict/' > > Re-reading now it still sounds fine to me, but I can change it. > > > > Do you mean you prefer: > > > > Clamp the size to be constrained within the minimum and maximum > > I meant this, yes. Or maybe > > Clamp the size to the \a minimum and \a maximum values > > (with s/to/between/, and/or adding the word "range" somewhere, if > desired) > > > or > > > > Restrict the size to be clamped within the minimum and 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. > > > > > > 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 :-) > > > > Is it because of the word 'constraints' as you mentioned above? or > > something else? > > Yes, it's "restricts to the constraints" that sounds convoluted to me. > > > Documenting a function called 'clamp' as 'it will clamp the values' > > doesn't add any value either. > > > > > 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/ > > > > Ack. > > > > > > > */ > > > > > 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@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
Hi Kieran, On Thu, Apr 07, 2022 at 10:47:40PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2022-04-07 15:58:09) > > On Thu, Apr 07, 2022 at 03:38:56PM +0100, Kieran Bingham wrote: > > > Quoting Laurent Pinchart (2022-04-05 15:18:07) > > > > 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 ? > > > > > > I did yes. I mean the same restriction of the minimum and maximum Size > > > but as a const. > > > > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@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. > > > > > > Yes, a caller could call .expandTo.shrinkTo instead already but I don't > > > think that should be the implementation detail here. > > > > > > > > > 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 ? > > > > > > I was trying to avoid using the term I'm describing to describe itself > > > in the description. > > > > The fact that the function is named from the term that best describes > > its purpose means we picked the right name :-) > > Perhaps you only know the term 'clamp' in regards to the coding > implementation. > > Describing the function 'clamp()' with the word 'clamp' conveys the > wrong meaning to me. (And maybe other english speakers who have used > clamps?) > > But to me - a clamp is a mechanical device used to temporarily fix two > (or more) objects such that they remain in the same position, usually > while you then perform some other operation like screwing the two items > together, or marking them. You 'could' use a clamp to squash something > ... but I don't think that quite fits the real definition of > std::clamp() (At least I don't think it can constrain to a non-zero > minimum). > > google-translate tells me that a clamp might be known as 'serrer' in > French or 'Morsetto' in Italian, so perhaps 'clamp' is only known as > it's use in code ... but I haven't validated those usages in a sentence > ;-) We could call the function puristaa() but we will then restrict our audience. Jokes aside, the word has multiple meanings, with "to modify a numeric value so it lies within a specific range" being one of them only. Perhaps I am more familiar with that meaning than the average developer without being aware of that (it's widely used in the kernel after all). > Anyway, I presume someone who wants to use this - knows what the coding > term is - but my point is - I don't think the function name is > automatically the best way to describe what it does. > > If it was using both the term restrict and contrain in the same sentence > I could offer: > > > "Constrain the Size to be within the dimensions of both the minimum and > maximum values." > > Which also works with 's/Constrain/Restrict/' I think we've spent enough time bikeshedding, I'm fine with this, or any variation that would be preferred by the team. > > > Re-reading now it still sounds fine to me, but I can change it. > > > > > > Do you mean you prefer: > > > > > > Clamp the size to be constrained within the minimum and maximum > > > > I meant this, yes. Or maybe > > > > Clamp the size to the \a minimum and \a maximum values > > > > (with s/to/between/, and/or adding the word "range" somewhere, if > > desired) > > > > > or > > > > > > Restrict the size to be clamped within the minimum and 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. > > > > > > > > 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 :-) > > > > > > Is it because of the word 'constraints' as you mentioned above? or > > > something else? > > > > Yes, it's "restricts to the constraints" that sounds convoluted to me. > > > > > Documenting a function called 'clamp' as 'it will clamp the values' > > > doesn't add any value either. > > > > > > > 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/ > > > > > > Ack. > > > > > > > > > */ > > > > > > 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@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;
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 }); + 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;
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@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 ;-) include/libcamera/geometry.h | 16 ++++++++++++++++ src/libcamera/geometry.cpp | 21 +++++++++++++++++++++ test/geometry.cpp | 24 ++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 2 deletions(-)