[libcamera-devel] [PATCH] libcamera: geometry: Add Size members to clamp to a min/max
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Apr 8 09:34:51 CEST 2022
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 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.
> > >
> > > 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 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
More information about the libcamera-devel
mailing list