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

[Niklas Söderlund]

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?

    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

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

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