| Message ID | 20190612004359.15772-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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. Apart from this Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards, > Niklas Söderlund
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 */ + /** * \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 + * \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)) + return false; + + return true; +} + /** * \brief Assemble and return a string describing the size range * \return A string describing the SizeRange
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/geometry.h | 2 ++ src/libcamera/geometry.cpp | 17 +++++++++++++++++ 2 files changed, 19 insertions(+)