[libcamera-devel] [PATCH v2 04/16] libcamera: geometry: SizeRange: Add contains()

[Jacopo Mondi]

Hi Niklas,

On Sun, Jun 16, 2019 at 02:51:06PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2019-06-13 16:51:17 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Wed, Jun 12, 2019 at 02:43:47AM +0200, Niklas Söderlund wrote:
> > > Add a method to check if a Size can fit inside a SizeRange. When
> > > determining if a size is containable take step values into account if
> > > they are not set to 0.
> >
> > Now they're initialized to 1 by default
>
> Yes, but what they are initialized as 1 by the consturctors which do not
> initialize the step values. So one still need to consider the case
>
>     SizeRange(unsigned int minW, unsigned int minH,
>               unsigned int maxW, unsigned int maxH,
>               unsigned int hstep, unsigned int vstep)
>             : min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep)
>
> Where steps might be set to 0 do avoid a division by zero problem.
>
> >
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  include/libcamera/geometry.h |  2 ++
> > >  src/libcamera/geometry.cpp   | 17 +++++++++++++++++
> > >  2 files changed, 19 insertions(+)
> > >
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > index 03962d7d09fac2b0..52f4d010de76f840 100644
> > > --- a/include/libcamera/geometry.h
> > > +++ b/include/libcamera/geometry.h
> > > @@ -92,6 +92,8 @@ public:
> > >  	{
> > >  	}
> > >
> > > +	bool contains(const Size &size) const;
> > > +
> > >  	std::string toString() const;
> > >
> > >  	Size min;
> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > > index 6a1d1d78870b23ec..a4d472d9fce2e226 100644
> > > --- a/src/libcamera/geometry.cpp
> > > +++ b/src/libcamera/geometry.cpp
> > > @@ -107,6 +107,7 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs)
> > >   * \brief The Size height
> > >   */
> > >
> > > +
> >
> > Ups
> >
> > >  /**
> > >   * \brief Assemble and return a string describing the size
> > >   * \return A string describing the size
> > > @@ -261,6 +262,22 @@ bool operator<(const Size &lhs, const Size &rhs)
> > >   * \brief The vertical step
> > >   */
> > >
> > > +/**
> > > + * \brief Test if a size can be contained in the range
> >
> > s/can be/is
> >
> > > + * \param[in] size Size to check
> > > + * \returns True if \a size can be contained
> > > + */
> > > +bool SizeRange::contains(const Size &size) const
> > > +{
> > > +	if (size.width < min.width || size.width > max.width ||
> > > +	    size.height < min.height || size.height > max.height ||
> > > +	    (hStep && (size.width - min.width) % hStep) ||
> > > +	    (vStep && (size.height - min.height) % vStep))
> >
> > This returns false for ranges that describes a single size, which is
> > fine (even if we could decide that a size is "contained" in a range if
> > both width and height are minor). In any case, I would document that.
>
> Does it ? If a size describes a single range 100x100 with step values of
> either 1 or 0 this will return true if compared to a size of 100x100,
> right?

A Size will always describe a single size. My point was that a Range
can describe a single size and in that case what happens is ambiguous.
>
>     size.width < min.width || -> 100 < 100 -> false
>     size.width > max.width || -> 100 > 100 -> false
>     size.height < min.height || -> 100 < 100 -> false
>     size.height > max.height || -> 100 > 100 -> false

 What I meant to ask was: "is 99x99 contained in 100x100" ?

        (99 < 100 -> true ||
        99 < 100 -> false ||
        99 < 100 -> true ||
        99 < 199 -> false)
                -> contained  = false

I think it's fine if we consider Ranges that represent a single size
not to contain any Size, or if we decide that a Size is contained in
a Range that represent a single size if both width and height are
minors, but we should document it.

>
>     < if we have step values of 0 >
>     (hStep && (size.width - min.width) % hStep) || -> (0 && ...) -> false
>     (vStep && (size.height - min.height) % vStep)  -> (0 && ...) -> false
>
>     < if we have step values of 1 >
>     (hStep && (size.width - min.width) % hStep) || -> (1 && (100 - 100) % 1) -> false
>     (vStep && (size.height - min.height) % vStep)  -> (1 && (100 - 100) % 1) -> false
>
> >
> > Apart from this
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> I corrected all comments but the last as I don't fully understand what
> you are trying to say. I will hold of collecting your tag until we can
> sort that comment out.
>

Please go ahead and include it with the comment clarified.

Thanks
  j

> >
> >
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  /**
> > >   * \brief Assemble and return a string describing the size range
> > >   * \return A string describing the SizeRange
> > > --
> > > 2.21.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190616/48e8e63f/attachment.sig>