| Message ID | 20200625012330.7639-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 7fc65e9680f604099d5d4f294b37fe7eb93acb03 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for the patch. On 6/25/20 6:53 AM, Laurent Pinchart wrote: > It's common for code to check if a size is null. Add a helper function > to do so. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Add test > --- > include/libcamera/geometry.h | 1 + > src/libcamera/geometry.cpp | 6 ++++++ > test/geometry.cpp | 10 ++++++++++ > 3 files changed, 17 insertions(+) > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > index edda42cf34cc..7d4b8bcfe3d8 100644 > --- a/include/libcamera/geometry.h > +++ b/include/libcamera/geometry.h > @@ -41,6 +41,7 @@ struct Size { > unsigned int width; > unsigned int height; > > + bool isNull() const { return !width && !height; } > const std::string toString() const; > }; > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > index fd78cf2c0ab7..24c44fb43acf 100644 > --- a/src/libcamera/geometry.cpp > +++ b/src/libcamera/geometry.cpp > @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > * \brief The Size height > */ > > +/** > + * \fn bool Size::isNull() const > + * \brief Check if the size is null > + * \return True if both the width and height are 0, or false otherwise > + */ > + > /** > * \brief Assemble and return a string describing the size > * \return A string describing the size > diff --git a/test/geometry.cpp b/test/geometry.cpp > index 27e65565d7c6..904ad92c9448 100644 > --- a/test/geometry.cpp > +++ b/test/geometry.cpp > @@ -36,6 +36,16 @@ protected: > > int run() > { > + if (!Size().isNull() || !Size(0, 0).isNull()) { > + cout << "Null size incorrectly reported as not null" << endl; > + return TestFail; > + } > + > + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > + cout << "Non-null size incorrectly reported as null" << endl; > + return TestFail; > + } > + > /* Test Size equality and inequality. */ > if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > return TestFail; Looks good to me. Reviewed-by: Umang Jain <email@uajain.com>
Hi Laurent On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > It's common for code to check if a size is null. Add a helper function > to do so. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Add test > --- > include/libcamera/geometry.h | 1 + > src/libcamera/geometry.cpp | 6 ++++++ > test/geometry.cpp | 10 ++++++++++ > 3 files changed, 17 insertions(+) > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > index edda42cf34cc..7d4b8bcfe3d8 100644 > --- a/include/libcamera/geometry.h > +++ b/include/libcamera/geometry.h > @@ -41,6 +41,7 @@ struct Size { > unsigned int width; > unsigned int height; > > + bool isNull() const { return !width && !height; } Is isNull() a good name ? We have isValid() for PixelFormat() should we stay consistent ? Otherwise Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > const std::string toString() const; > }; > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > index fd78cf2c0ab7..24c44fb43acf 100644 > --- a/src/libcamera/geometry.cpp > +++ b/src/libcamera/geometry.cpp > @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > * \brief The Size height > */ > > +/** > + * \fn bool Size::isNull() const > + * \brief Check if the size is null > + * \return True if both the width and height are 0, or false otherwise > + */ > + > /** > * \brief Assemble and return a string describing the size > * \return A string describing the size > diff --git a/test/geometry.cpp b/test/geometry.cpp > index 27e65565d7c6..904ad92c9448 100644 > --- a/test/geometry.cpp > +++ b/test/geometry.cpp > @@ -36,6 +36,16 @@ protected: > > int run() > { > + if (!Size().isNull() || !Size(0, 0).isNull()) { > + cout << "Null size incorrectly reported as not null" << endl; > + return TestFail; > + } > + > + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > + cout << "Non-null size incorrectly reported as null" << endl; > + return TestFail; > + } > + > /* Test Size equality and inequality. */ > if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > return TestFail; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > > It's common for code to check if a size is null. Add a helper function > > to do so. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Add test > > --- > > include/libcamera/geometry.h | 1 + > > src/libcamera/geometry.cpp | 6 ++++++ > > test/geometry.cpp | 10 ++++++++++ > > 3 files changed, 17 insertions(+) > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > index edda42cf34cc..7d4b8bcfe3d8 100644 > > --- a/include/libcamera/geometry.h > > +++ b/include/libcamera/geometry.h > > @@ -41,6 +41,7 @@ struct Size { > > unsigned int width; > > unsigned int height; > > > > + bool isNull() const { return !width && !height; } > > Is isNull() a good name ? We have isValid() for PixelFormat() should > we stay consistent ? I initially had isEmpty(), and I've taken inspiration from QRect the defines isNull() as !width && !height (or rather width <= 0 && height <= 0, but our width and height are unsigned), and isEmpty() as !width || !height. > Otherwise > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > const std::string toString() const; > > }; > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > index fd78cf2c0ab7..24c44fb43acf 100644 > > --- a/src/libcamera/geometry.cpp > > +++ b/src/libcamera/geometry.cpp > > @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > > * \brief The Size height > > */ > > > > +/** > > + * \fn bool Size::isNull() const > > + * \brief Check if the size is null > > + * \return True if both the width and height are 0, or false otherwise > > + */ > > + > > /** > > * \brief Assemble and return a string describing the size > > * \return A string describing the size > > diff --git a/test/geometry.cpp b/test/geometry.cpp > > index 27e65565d7c6..904ad92c9448 100644 > > --- a/test/geometry.cpp > > +++ b/test/geometry.cpp > > @@ -36,6 +36,16 @@ protected: > > > > int run() > > { > > + if (!Size().isNull() || !Size(0, 0).isNull()) { > > + cout << "Null size incorrectly reported as not null" << endl; > > + return TestFail; > > + } > > + > > + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > > + cout << "Non-null size incorrectly reported as null" << endl; > > + return TestFail; > > + } > > + > > /* Test Size equality and inequality. */ > > if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > > return TestFail;
Hi Laurent, On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > > On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > > > It's common for code to check if a size is null. Add a helper function > > > to do so. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Changes since v1: > > > > > > - Add test > > > --- > > > include/libcamera/geometry.h | 1 + > > > src/libcamera/geometry.cpp | 6 ++++++ > > > test/geometry.cpp | 10 ++++++++++ > > > 3 files changed, 17 insertions(+) > > > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > > index edda42cf34cc..7d4b8bcfe3d8 100644 > > > --- a/include/libcamera/geometry.h > > > +++ b/include/libcamera/geometry.h > > > @@ -41,6 +41,7 @@ struct Size { > > > unsigned int width; > > > unsigned int height; > > > > > > + bool isNull() const { return !width && !height; } > > > > Is isNull() a good name ? We have isValid() for PixelFormat() should > > we stay consistent ? > > I initially had isEmpty(), and I've taken inspiration from QRect the > defines isNull() as !width && !height (or rather width <= 0 && height <= > 0, but our width and height are unsigned), and isEmpty() as !width || > !height. > I was more concerned about the fact we have a number of classes using isValid() already, and consistency with the library is important, otherwise it's a constant "go to look at the class definition", much similar to when we had Rectangle::width and Size::w include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } include/libcamera/internal/ipa_module.h: bool isValid() const; include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } For the record we have one isEmpty() nclude/libcamera/internal/formats.h: bool isEmpty() const; but no isNull(). I would avoid introducing another term for a similar, if not the same, concept. Thanks j > > Otherwise > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > const std::string toString() const; > > > }; > > > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > > index fd78cf2c0ab7..24c44fb43acf 100644 > > > --- a/src/libcamera/geometry.cpp > > > +++ b/src/libcamera/geometry.cpp > > > @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > > > * \brief The Size height > > > */ > > > > > > +/** > > > + * \fn bool Size::isNull() const > > > + * \brief Check if the size is null > > > + * \return True if both the width and height are 0, or false otherwise > > > + */ > > > + > > > /** > > > * \brief Assemble and return a string describing the size > > > * \return A string describing the size > > > diff --git a/test/geometry.cpp b/test/geometry.cpp > > > index 27e65565d7c6..904ad92c9448 100644 > > > --- a/test/geometry.cpp > > > +++ b/test/geometry.cpp > > > @@ -36,6 +36,16 @@ protected: > > > > > > int run() > > > { > > > + if (!Size().isNull() || !Size(0, 0).isNull()) { > > > + cout << "Null size incorrectly reported as not null" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > > > + cout << "Non-null size incorrectly reported as null" << endl; > > > + return TestFail; > > > + } > > > + > > > /* Test Size equality and inequality. */ > > > if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > > > return TestFail; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: > On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > > On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > > > On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > > > > It's common for code to check if a size is null. Add a helper function > > > > to do so. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > Changes since v1: > > > > > > > > - Add test > > > > --- > > > > include/libcamera/geometry.h | 1 + > > > > src/libcamera/geometry.cpp | 6 ++++++ > > > > test/geometry.cpp | 10 ++++++++++ > > > > 3 files changed, 17 insertions(+) > > > > > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > > > index edda42cf34cc..7d4b8bcfe3d8 100644 > > > > --- a/include/libcamera/geometry.h > > > > +++ b/include/libcamera/geometry.h > > > > @@ -41,6 +41,7 @@ struct Size { > > > > unsigned int width; > > > > unsigned int height; > > > > > > > > + bool isNull() const { return !width && !height; } > > > > > > Is isNull() a good name ? We have isValid() for PixelFormat() should > > > we stay consistent ? > > > > I initially had isEmpty(), and I've taken inspiration from QRect the > > defines isNull() as !width && !height (or rather width <= 0 && height <= > > 0, but our width and height are unsigned), and isEmpty() as !width || > > !height. > > I was more concerned about the fact we have a number of classes using > isValid() already, and consistency with the library is important, > otherwise it's a constant "go to look at the class definition", much > similar to when we had Rectangle::width and Size::w > > include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } > include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } > include/libcamera/internal/ipa_module.h: bool isValid() const; > include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } > include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } > include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } > include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } > > For the record we have one isEmpty() > nclude/libcamera/internal/formats.h: bool isEmpty() const; > > but no isNull(). I would avoid introducing another term for a similar, > if not the same, concept. That boils down to the question of is a Size(0, 0) "invalid" ? > > > Otherwise > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > const std::string toString() const; > > > > }; > > > > > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > > > index fd78cf2c0ab7..24c44fb43acf 100644 > > > > --- a/src/libcamera/geometry.cpp > > > > +++ b/src/libcamera/geometry.cpp > > > > @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > > > > * \brief The Size height > > > > */ > > > > > > > > +/** > > > > + * \fn bool Size::isNull() const > > > > + * \brief Check if the size is null > > > > + * \return True if both the width and height are 0, or false otherwise > > > > + */ > > > > + > > > > /** > > > > * \brief Assemble and return a string describing the size > > > > * \return A string describing the size > > > > diff --git a/test/geometry.cpp b/test/geometry.cpp > > > > index 27e65565d7c6..904ad92c9448 100644 > > > > --- a/test/geometry.cpp > > > > +++ b/test/geometry.cpp > > > > @@ -36,6 +36,16 @@ protected: > > > > > > > > int run() > > > > { > > > > + if (!Size().isNull() || !Size(0, 0).isNull()) { > > > > + cout << "Null size incorrectly reported as not null" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > > > > + cout << "Non-null size incorrectly reported as null" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > /* Test Size equality and inequality. */ > > > > if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > > > > return TestFail;
Hi Laurent, On Thu, Jun 25, 2020 at 12:07:39PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: > > On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > > > On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > > > > On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > > > > > It's common for code to check if a size is null. Add a helper function > > > > > to do so. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > Changes since v1: > > > > > > > > > > - Add test > > > > > --- > > > > > include/libcamera/geometry.h | 1 + > > > > > src/libcamera/geometry.cpp | 6 ++++++ > > > > > test/geometry.cpp | 10 ++++++++++ > > > > > 3 files changed, 17 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > > > > index edda42cf34cc..7d4b8bcfe3d8 100644 > > > > > --- a/include/libcamera/geometry.h > > > > > +++ b/include/libcamera/geometry.h > > > > > @@ -41,6 +41,7 @@ struct Size { > > > > > unsigned int width; > > > > > unsigned int height; > > > > > > > > > > + bool isNull() const { return !width && !height; } > > > > > > > > Is isNull() a good name ? We have isValid() for PixelFormat() should > > > > we stay consistent ? > > > > > > I initially had isEmpty(), and I've taken inspiration from QRect the > > > defines isNull() as !width && !height (or rather width <= 0 && height <= > > > 0, but our width and height are unsigned), and isEmpty() as !width || > > > !height. > > > > I was more concerned about the fact we have a number of classes using > > isValid() already, and consistency with the library is important, > > otherwise it's a constant "go to look at the class definition", much > > similar to when we had Rectangle::width and Size::w > > > > include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } > > include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } > > include/libcamera/internal/ipa_module.h: bool isValid() const; > > include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } > > include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } > > include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } > > include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } > > > > For the record we have one isEmpty() > > nclude/libcamera/internal/formats.h: bool isEmpty() const; > > > > but no isNull(). I would avoid introducing another term for a similar, > > if not the same, concept. > > That boils down to the question of is a Size(0, 0) "invalid" ? Sorry about this, but how would you define a "valid" size ? To me whatever size that has -both- width and height > 0 is valid. Everything else is not. I don't really see a need for a distinction between a "valid" (w || h) and a "!null" (or zero) one (w && h). A size with any of its members set to 0 is not valid. Is this too harsh ? > > > > > Otherwise > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > const std::string toString() const; > > > > > }; > > > > > > > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > > > > index fd78cf2c0ab7..24c44fb43acf 100644 > > > > > --- a/src/libcamera/geometry.cpp > > > > > +++ b/src/libcamera/geometry.cpp > > > > > @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > > > > > * \brief The Size height > > > > > */ > > > > > > > > > > +/** > > > > > + * \fn bool Size::isNull() const > > > > > + * \brief Check if the size is null > > > > > + * \return True if both the width and height are 0, or false otherwise > > > > > + */ > > > > > + > > > > > /** > > > > > * \brief Assemble and return a string describing the size > > > > > * \return A string describing the size > > > > > diff --git a/test/geometry.cpp b/test/geometry.cpp > > > > > index 27e65565d7c6..904ad92c9448 100644 > > > > > --- a/test/geometry.cpp > > > > > +++ b/test/geometry.cpp > > > > > @@ -36,6 +36,16 @@ protected: > > > > > > > > > > int run() > > > > > { > > > > > + if (!Size().isNull() || !Size(0, 0).isNull()) { > > > > > + cout << "Null size incorrectly reported as not null" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > > > > > + cout << "Non-null size incorrectly reported as null" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > /* Test Size equality and inequality. */ > > > > > if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > > > > > return TestFail; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Jun 25, 2020 at 11:26:17AM +0200, Jacopo Mondi wrote: > On Thu, Jun 25, 2020 at 12:07:39PM +0300, Laurent Pinchart wrote: > > On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: > >> On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > >>> On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > >>>> On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > >>>>> It's common for code to check if a size is null. Add a helper function > >>>>> to do so. > >>>>> > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>> --- > >>>>> Changes since v1: > >>>>> > >>>>> - Add test > >>>>> --- > >>>>> include/libcamera/geometry.h | 1 + > >>>>> src/libcamera/geometry.cpp | 6 ++++++ > >>>>> test/geometry.cpp | 10 ++++++++++ > >>>>> 3 files changed, 17 insertions(+) > >>>>> > >>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > >>>>> index edda42cf34cc..7d4b8bcfe3d8 100644 > >>>>> --- a/include/libcamera/geometry.h > >>>>> +++ b/include/libcamera/geometry.h > >>>>> @@ -41,6 +41,7 @@ struct Size { > >>>>> unsigned int width; > >>>>> unsigned int height; > >>>>> > >>>>> + bool isNull() const { return !width && !height; } > >>>> > >>>> Is isNull() a good name ? We have isValid() for PixelFormat() should > >>>> we stay consistent ? > >>> > >>> I initially had isEmpty(), and I've taken inspiration from QRect the > >>> defines isNull() as !width && !height (or rather width <= 0 && height <= > >>> 0, but our width and height are unsigned), and isEmpty() as !width || > >>> !height. > >> > >> I was more concerned about the fact we have a number of classes using > >> isValid() already, and consistency with the library is important, > >> otherwise it's a constant "go to look at the class definition", much > >> similar to when we had Rectangle::width and Size::w > >> > >> include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } > >> include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } > >> include/libcamera/internal/ipa_module.h: bool isValid() const; > >> include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } > >> include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } > >> include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } > >> include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } > >> > >> For the record we have one isEmpty() > >> nclude/libcamera/internal/formats.h: bool isEmpty() const; > >> > >> but no isNull(). I would avoid introducing another term for a similar, > >> if not the same, concept. > > > > That boils down to the question of is a Size(0, 0) "invalid" ? > > Sorry about this, but how would you define a "valid" size ? To me > whatever size that has -both- width and height > 0 is valid. > Everything else is not. I don't really see a need for a distinction > between a "valid" (w || h) and a "!null" (or zero) one (w && h). > > A size with any of its members set to 0 is not valid. Is this too > harsh ? That wasn't the question :-) My point is that the name "valid" doesn't seem to be a good candidate to me here, I wouldn't call a size that has a width or height (or both) equal to 0 as "invalid". It's a zero/null/empty size, but not intrinsicly invalid. Whether such a size can be valid for a given usage depends on the usage, but isn't a property of the Size class in my opinion. > >>>> Otherwise > >>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> > >>>>> const std::string toString() const; > >>>>> }; > >>>>> > >>>>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > >>>>> index fd78cf2c0ab7..24c44fb43acf 100644 > >>>>> --- a/src/libcamera/geometry.cpp > >>>>> +++ b/src/libcamera/geometry.cpp > >>>>> @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > >>>>> * \brief The Size height > >>>>> */ > >>>>> > >>>>> +/** > >>>>> + * \fn bool Size::isNull() const > >>>>> + * \brief Check if the size is null > >>>>> + * \return True if both the width and height are 0, or false otherwise > >>>>> + */ > >>>>> + > >>>>> /** > >>>>> * \brief Assemble and return a string describing the size > >>>>> * \return A string describing the size > >>>>> diff --git a/test/geometry.cpp b/test/geometry.cpp > >>>>> index 27e65565d7c6..904ad92c9448 100644 > >>>>> --- a/test/geometry.cpp > >>>>> +++ b/test/geometry.cpp > >>>>> @@ -36,6 +36,16 @@ protected: > >>>>> > >>>>> int run() > >>>>> { > >>>>> + if (!Size().isNull() || !Size(0, 0).isNull()) { > >>>>> + cout << "Null size incorrectly reported as not null" << endl; > >>>>> + return TestFail; > >>>>> + } > >>>>> + > >>>>> + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > >>>>> + cout << "Non-null size incorrectly reported as null" << endl; > >>>>> + return TestFail; > >>>>> + } > >>>>> + > >>>>> /* Test Size equality and inequality. */ > >>>>> if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > >>>>> return TestFail;
On Thu, Jun 25, 2020 at 12:51:15PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Jun 25, 2020 at 11:26:17AM +0200, Jacopo Mondi wrote: > > On Thu, Jun 25, 2020 at 12:07:39PM +0300, Laurent Pinchart wrote: > > > On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: > > >> On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > > >>> On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > > >>>> On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > > >>>>> It's common for code to check if a size is null. Add a helper function > > >>>>> to do so. > > >>>>> > > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>>>> --- > > >>>>> Changes since v1: > > >>>>> > > >>>>> - Add test > > >>>>> --- > > >>>>> include/libcamera/geometry.h | 1 + > > >>>>> src/libcamera/geometry.cpp | 6 ++++++ > > >>>>> test/geometry.cpp | 10 ++++++++++ > > >>>>> 3 files changed, 17 insertions(+) > > >>>>> > > >>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > >>>>> index edda42cf34cc..7d4b8bcfe3d8 100644 > > >>>>> --- a/include/libcamera/geometry.h > > >>>>> +++ b/include/libcamera/geometry.h > > >>>>> @@ -41,6 +41,7 @@ struct Size { > > >>>>> unsigned int width; > > >>>>> unsigned int height; > > >>>>> > > >>>>> + bool isNull() const { return !width && !height; } > > >>>> > > >>>> Is isNull() a good name ? We have isValid() for PixelFormat() should > > >>>> we stay consistent ? > > >>> > > >>> I initially had isEmpty(), and I've taken inspiration from QRect the > > >>> defines isNull() as !width && !height (or rather width <= 0 && height <= > > >>> 0, but our width and height are unsigned), and isEmpty() as !width || > > >>> !height. > > >> > > >> I was more concerned about the fact we have a number of classes using > > >> isValid() already, and consistency with the library is important, > > >> otherwise it's a constant "go to look at the class definition", much > > >> similar to when we had Rectangle::width and Size::w > > >> > > >> include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } > > >> include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } > > >> include/libcamera/internal/ipa_module.h: bool isValid() const; > > >> include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } > > >> include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } > > >> include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } > > >> include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } > > >> > > >> For the record we have one isEmpty() > > >> nclude/libcamera/internal/formats.h: bool isEmpty() const; > > >> > > >> but no isNull(). I would avoid introducing another term for a similar, > > >> if not the same, concept. > > > > > > That boils down to the question of is a Size(0, 0) "invalid" ? > > > > Sorry about this, but how would you define a "valid" size ? To me > > whatever size that has -both- width and height > 0 is valid. > > Everything else is not. I don't really see a need for a distinction > > between a "valid" (w || h) and a "!null" (or zero) one (w && h). > > > > A size with any of its members set to 0 is not valid. Is this too > > harsh ? > > That wasn't the question :-) My point is that the name "valid" doesn't > seem to be a good candidate to me here, I wouldn't call a size that has > a width or height (or both) equal to 0 as "invalid". It's a > zero/null/empty size, but not intrinsicly invalid. Whether such a size > can be valid for a given usage depends on the usage, but isn't a > property of the Size class in my opinion. > As you wish. This is going into the philosophical territory. To me a Size with any of its member set to 0 is empty, null, invalid or whatever as I don't see a need to distinguish between an "empty" a "null" or a "valid" size. I just wanted to use the most widespread name we have to avoid having to look it up every time, which is just annoying. That said, up to you :) Thanks j
Ohhhh a bikeshedding thread :-D /me jumps in ... On 25/06/2020 11:17, Jacopo Mondi wrote: > On Thu, Jun 25, 2020 at 12:51:15PM +0300, Laurent Pinchart wrote: >> Hi Jacopo, >> >> On Thu, Jun 25, 2020 at 11:26:17AM +0200, Jacopo Mondi wrote: >>> On Thu, Jun 25, 2020 at 12:07:39PM +0300, Laurent Pinchart wrote: >>>> On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: >>>>> On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: >>>>>> On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: >>>>>>> On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: >>>>>>>> It's common for code to check if a size is null. Add a helper function >>>>>>>> to do so. >>>>>>>> >>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>>>> --- >>>>>>>> Changes since v1: >>>>>>>> >>>>>>>> - Add test >>>>>>>> --- >>>>>>>> include/libcamera/geometry.h | 1 + >>>>>>>> src/libcamera/geometry.cpp | 6 ++++++ >>>>>>>> test/geometry.cpp | 10 ++++++++++ >>>>>>>> 3 files changed, 17 insertions(+) >>>>>>>> >>>>>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h >>>>>>>> index edda42cf34cc..7d4b8bcfe3d8 100644 >>>>>>>> --- a/include/libcamera/geometry.h >>>>>>>> +++ b/include/libcamera/geometry.h >>>>>>>> @@ -41,6 +41,7 @@ struct Size { >>>>>>>> unsigned int width; >>>>>>>> unsigned int height; >>>>>>>> >>>>>>>> + bool isNull() const { return !width && !height; } >>>>>>> >>>>>>> Is isNull() a good name ? We have isValid() for PixelFormat() should >>>>>>> we stay consistent ? >>>>>> >>>>>> I initially had isEmpty(), and I've taken inspiration from QRect the >>>>>> defines isNull() as !width && !height (or rather width <= 0 && height <= >>>>>> 0, but our width and height are unsigned), and isEmpty() as !width || >>>>>> !height. >>>>> >>>>> I was more concerned about the fact we have a number of classes using >>>>> isValid() already, and consistency with the library is important, >>>>> otherwise it's a constant "go to look at the class definition", much >>>>> similar to when we had Rectangle::width and Size::w >>>>> >>>>> include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } >>>>> include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } >>>>> include/libcamera/internal/ipa_module.h: bool isValid() const; >>>>> include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } >>>>> include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } >>>>> include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } >>>>> include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } >>>>> >>>>> For the record we have one isEmpty() >>>>> nclude/libcamera/internal/formats.h: bool isEmpty() const; >>>>> >>>>> but no isNull(). I would avoid introducing another term for a similar, >>>>> if not the same, concept. >>>> >>>> That boils down to the question of is a Size(0, 0) "invalid" ? >>> >>> Sorry about this, but how would you define a "valid" size ? To me >>> whatever size that has -both- width and height > 0 is valid. >>> Everything else is not. I don't really see a need for a distinction >>> between a "valid" (w || h) and a "!null" (or zero) one (w && h). >>> >>> A size with any of its members set to 0 is not valid. Is this too >>> harsh ? >> >> That wasn't the question :-) My point is that the name "valid" doesn't >> seem to be a good candidate to me here, I wouldn't call a size that has >> a width or height (or both) equal to 0 as "invalid". It's a >> zero/null/empty size, but not intrinsicly invalid. Whether such a size >> can be valid for a given usage depends on the usage, but isn't a >> property of the Size class in my opinion. >> > > As you wish. This is going into the philosophical territory. To me a > Size with any of its member set to 0 is empty, null, invalid or > whatever as I don't see a need to distinguish between an "empty" a > "null" or a "valid" size. I just wanted to use the most widespread > name we have to avoid having to look it up every time, which is just > annoying. That said, up to you :) Personally I would say that a size of 0 is 'valid'. Maybe it depends on context, but I can attest that the size of my cherry-bakewell is now 0 in both width, height (and depth): if (cherry-bakewell.size == Size(0, 0)) { std::cout << "Was it yummy?" << std::endl; } else { std::cout << "C'mon - eat up..." << std::endl; } isNull, isZero, would both be fine with me (I'd actually use isZero) But for some reason - not isEmpty(). I don't think you'd have an empty size, as the 'size' is not a container. -- Kieran > > Thanks > j > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Laurent, Thanks for your work. On 2020-06-25 04:23:30 +0300, Laurent Pinchart wrote: > It's common for code to check if a size is null. Add a helper function > to do so. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > Changes since v1: > > - Add test > --- > include/libcamera/geometry.h | 1 + > src/libcamera/geometry.cpp | 6 ++++++ > test/geometry.cpp | 10 ++++++++++ > 3 files changed, 17 insertions(+) > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > index edda42cf34cc..7d4b8bcfe3d8 100644 > --- a/include/libcamera/geometry.h > +++ b/include/libcamera/geometry.h > @@ -41,6 +41,7 @@ struct Size { > unsigned int width; > unsigned int height; > > + bool isNull() const { return !width && !height; } > const std::string toString() const; > }; > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > index fd78cf2c0ab7..24c44fb43acf 100644 > --- a/src/libcamera/geometry.cpp > +++ b/src/libcamera/geometry.cpp > @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) > * \brief The Size height > */ > > +/** > + * \fn bool Size::isNull() const > + * \brief Check if the size is null > + * \return True if both the width and height are 0, or false otherwise > + */ > + > /** > * \brief Assemble and return a string describing the size > * \return A string describing the size > diff --git a/test/geometry.cpp b/test/geometry.cpp > index 27e65565d7c6..904ad92c9448 100644 > --- a/test/geometry.cpp > +++ b/test/geometry.cpp > @@ -36,6 +36,16 @@ protected: > > int run() > { > + if (!Size().isNull() || !Size(0, 0).isNull()) { > + cout << "Null size incorrectly reported as not null" << endl; > + return TestFail; > + } > + > + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { > + cout << "Non-null size incorrectly reported as null" << endl; > + return TestFail; > + } > + > /* Test Size equality and inequality. */ > if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) > return TestFail; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Jun 25, 2020 at 11:50:18AM +0100, Kieran Bingham wrote: > Ohhhh a bikeshedding thread :-D > > /me jumps in ... :-) > On 25/06/2020 11:17, Jacopo Mondi wrote: > > On Thu, Jun 25, 2020 at 12:51:15PM +0300, Laurent Pinchart wrote: > >> On Thu, Jun 25, 2020 at 11:26:17AM +0200, Jacopo Mondi wrote: > >>> On Thu, Jun 25, 2020 at 12:07:39PM +0300, Laurent Pinchart wrote: > >>>> On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: > >>>>> On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > >>>>>> On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > >>>>>>> On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > >>>>>>>> It's common for code to check if a size is null. Add a helper function > >>>>>>>> to do so. > >>>>>>>> > >>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>>>> --- > >>>>>>>> Changes since v1: > >>>>>>>> > >>>>>>>> - Add test > >>>>>>>> --- > >>>>>>>> include/libcamera/geometry.h | 1 + > >>>>>>>> src/libcamera/geometry.cpp | 6 ++++++ > >>>>>>>> test/geometry.cpp | 10 ++++++++++ > >>>>>>>> 3 files changed, 17 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > >>>>>>>> index edda42cf34cc..7d4b8bcfe3d8 100644 > >>>>>>>> --- a/include/libcamera/geometry.h > >>>>>>>> +++ b/include/libcamera/geometry.h > >>>>>>>> @@ -41,6 +41,7 @@ struct Size { > >>>>>>>> unsigned int width; > >>>>>>>> unsigned int height; > >>>>>>>> > >>>>>>>> + bool isNull() const { return !width && !height; } > >>>>>>> > >>>>>>> Is isNull() a good name ? We have isValid() for PixelFormat() should > >>>>>>> we stay consistent ? > >>>>>> > >>>>>> I initially had isEmpty(), and I've taken inspiration from QRect the > >>>>>> defines isNull() as !width && !height (or rather width <= 0 && height <= > >>>>>> 0, but our width and height are unsigned), and isEmpty() as !width || > >>>>>> !height. > >>>>> > >>>>> I was more concerned about the fact we have a number of classes using > >>>>> isValid() already, and consistency with the library is important, > >>>>> otherwise it's a constant "go to look at the class definition", much > >>>>> similar to when we had Rectangle::width and Size::w > >>>>> > >>>>> include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } > >>>>> include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } > >>>>> include/libcamera/internal/ipa_module.h: bool isValid() const; > >>>>> include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } > >>>>> include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } > >>>>> include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } > >>>>> include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } > >>>>> > >>>>> For the record we have one isEmpty() > >>>>> nclude/libcamera/internal/formats.h: bool isEmpty() const; > >>>>> > >>>>> but no isNull(). I would avoid introducing another term for a similar, > >>>>> if not the same, concept. > >>>> > >>>> That boils down to the question of is a Size(0, 0) "invalid" ? > >>> > >>> Sorry about this, but how would you define a "valid" size ? To me > >>> whatever size that has -both- width and height > 0 is valid. > >>> Everything else is not. I don't really see a need for a distinction > >>> between a "valid" (w || h) and a "!null" (or zero) one (w && h). > >>> > >>> A size with any of its members set to 0 is not valid. Is this too > >>> harsh ? > >> > >> That wasn't the question :-) My point is that the name "valid" doesn't > >> seem to be a good candidate to me here, I wouldn't call a size that has > >> a width or height (or both) equal to 0 as "invalid". It's a > >> zero/null/empty size, but not intrinsicly invalid. Whether such a size > >> can be valid for a given usage depends on the usage, but isn't a > >> property of the Size class in my opinion. > > > > As you wish. This is going into the philosophical territory. To me a > > Size with any of its member set to 0 is empty, null, invalid or > > whatever as I don't see a need to distinguish between an "empty" a > > "null" or a "valid" size. I'm not sure it's philosophical, but it's clearly a semantical issue. I don't like using "invalid" (or "valid") in this context, as having 0 width or height doesn't make the size intrinsically invalid. A PixelFormat with a 0 fourcc, on the other hand, is invalid, because 0 is not a valid fourcc. > > I just wanted to use the most widespread > > name we have to avoid having to look it up every time, which is just > > annoying. That said, up to you :) That's an interesting point. Based on my (limited) experience with Qt, MFC and .net frameworks (in another life), I believe that applying the same term throughout the API to incrzase consistency isn't a rule that should be blindly followed. Of course consistency is overall encouraged, but I've found it was much more difficult to work with the MFC or .net GUI frameworks than with Qt because the first would pick a set of concepts and apply them to everything, even when they were not the best fit, while the second would try to pick the right concept tailored to each problem at hand. There is a wider range of names in Qt, but when I needed a function in a class, my first guess for its name was usually right. It was a long time ago, so I don't have any specific example in mind, but if I had to make one, let's say you have a widget and you need to set its internal margins. Qt would have a setMargins() function, while other frameworks would use something like setLayoutProperties() that would be present in all GUI classes, and take a list of properties as a map from property ID to property value. The function would be consistently used through the API, but you would have to look up every time which properties a particular widget would accept. This is a completely made up example, but working with Qt really impressed me in how the API felt natural to use. Since that day I've strived to achieve the same level of quality in APIs (and have surely largely failed :-)). This is why, in this case, I don't think that isValid() would be a good choice. > Personally I would say that a size of 0 is 'valid'. Maybe it depends on > context, but I can attest that the size of my cherry-bakewell is now 0 > in both width, height (and depth): > > if (cherry-bakewell.size == Size(0, 0)) { > std::cout << "Was it yummy?" << std::endl; > } else { > std::cout << "C'mon - eat up..." << std::endl; > } > > isNull, isZero, would both be fine with me (I'd actually use isZero) But > for some reason - not isEmpty(). > > I don't think you'd have an empty size, as the 'size' is not a container. isEmpty() is a function that would indeed be more applicable to the Rectangle class for instance.
On 2020-06-25 11:50:18 +0100, Kieran Bingham wrote: > Ohhhh a bikeshedding thread :-D I don't like bikeshedding, but I would like for this functionality to be merged, so here is my suggestions :-) I like isNull(). As Laurent points out isValid() implies we can have an invalid size. isEmpty() makes me think of containers and makes little sens for me in this context. > > /me jumps in ... > > On 25/06/2020 11:17, Jacopo Mondi wrote: > > On Thu, Jun 25, 2020 at 12:51:15PM +0300, Laurent Pinchart wrote: > >> Hi Jacopo, > >> > >> On Thu, Jun 25, 2020 at 11:26:17AM +0200, Jacopo Mondi wrote: > >>> On Thu, Jun 25, 2020 at 12:07:39PM +0300, Laurent Pinchart wrote: > >>>> On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: > >>>>> On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > >>>>>> On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > >>>>>>> On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > >>>>>>>> It's common for code to check if a size is null. Add a helper function > >>>>>>>> to do so. > >>>>>>>> > >>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>>>> --- > >>>>>>>> Changes since v1: > >>>>>>>> > >>>>>>>> - Add test > >>>>>>>> --- > >>>>>>>> include/libcamera/geometry.h | 1 + > >>>>>>>> src/libcamera/geometry.cpp | 6 ++++++ > >>>>>>>> test/geometry.cpp | 10 ++++++++++ > >>>>>>>> 3 files changed, 17 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > >>>>>>>> index edda42cf34cc..7d4b8bcfe3d8 100644 > >>>>>>>> --- a/include/libcamera/geometry.h > >>>>>>>> +++ b/include/libcamera/geometry.h > >>>>>>>> @@ -41,6 +41,7 @@ struct Size { > >>>>>>>> unsigned int width; > >>>>>>>> unsigned int height; > >>>>>>>> > >>>>>>>> + bool isNull() const { return !width && !height; } > >>>>>>> > >>>>>>> Is isNull() a good name ? We have isValid() for PixelFormat() should > >>>>>>> we stay consistent ? > >>>>>> > >>>>>> I initially had isEmpty(), and I've taken inspiration from QRect the > >>>>>> defines isNull() as !width && !height (or rather width <= 0 && height <= > >>>>>> 0, but our width and height are unsigned), and isEmpty() as !width || > >>>>>> !height. > >>>>> > >>>>> I was more concerned about the fact we have a number of classes using > >>>>> isValid() already, and consistency with the library is important, > >>>>> otherwise it's a constant "go to look at the class definition", much > >>>>> similar to when we had Rectangle::width and Size::w > >>>>> > >>>>> include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } > >>>>> include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } > >>>>> include/libcamera/internal/ipa_module.h: bool isValid() const; > >>>>> include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } > >>>>> include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } > >>>>> include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } > >>>>> include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } > >>>>> > >>>>> For the record we have one isEmpty() > >>>>> nclude/libcamera/internal/formats.h: bool isEmpty() const; > >>>>> > >>>>> but no isNull(). I would avoid introducing another term for a similar, > >>>>> if not the same, concept. > >>>> > >>>> That boils down to the question of is a Size(0, 0) "invalid" ? > >>> > >>> Sorry about this, but how would you define a "valid" size ? To me > >>> whatever size that has -both- width and height > 0 is valid. > >>> Everything else is not. I don't really see a need for a distinction > >>> between a "valid" (w || h) and a "!null" (or zero) one (w && h). > >>> > >>> A size with any of its members set to 0 is not valid. Is this too > >>> harsh ? > >> > >> That wasn't the question :-) My point is that the name "valid" doesn't > >> seem to be a good candidate to me here, I wouldn't call a size that has > >> a width or height (or both) equal to 0 as "invalid". It's a > >> zero/null/empty size, but not intrinsicly invalid. Whether such a size > >> can be valid for a given usage depends on the usage, but isn't a > >> property of the Size class in my opinion. > >> > > > > As you wish. This is going into the philosophical territory. To me a > > Size with any of its member set to 0 is empty, null, invalid or > > whatever as I don't see a need to distinguish between an "empty" a > > "null" or a "valid" size. I just wanted to use the most widespread > > name we have to avoid having to look it up every time, which is just > > annoying. That said, up to you :) > > > Personally I would say that a size of 0 is 'valid'. Maybe it depends on > context, but I can attest that the size of my cherry-bakewell is now 0 > in both width, height (and depth): > > if (cherry-bakewell.size == Size(0, 0)) { > std::cout << "Was it yummy?" << std::endl; > } else { > std::cout << "C'mon - eat up..." << std::endl; > } > > isNull, isZero, would both be fine with me (I'd actually use isZero) But > for some reason - not isEmpty(). > > I don't think you'd have an empty size, as the 'size' is not a container. > > -- > Kieran > > > > > > > Thanks > > j > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Fri, Jun 26, 2020 at 03:25:05AM +0200, Niklas Söderlund wrote: > On 2020-06-25 11:50:18 +0100, Kieran Bingham wrote: > > Ohhhh a bikeshedding thread :-D > > I don't like bikeshedding, but I would like for this functionality to be > merged, so here is my suggestions :-) > > I like isNull(). As Laurent points out isValid() implies we can have an > invalid size. isEmpty() makes me think of containers and makes little > sens for me in this context. I'd rather go for isNull() as well (I'm possibly influenced by Qt here). If we later need a function to test width == 0 || height == 0 in addition to width == 0 && height == 0 we'll bikeshed the name for that new function again :-) > > /me jumps in ... > > > > On 25/06/2020 11:17, Jacopo Mondi wrote: > > > On Thu, Jun 25, 2020 at 12:51:15PM +0300, Laurent Pinchart wrote: > > >> Hi Jacopo, > > >> > > >> On Thu, Jun 25, 2020 at 11:26:17AM +0200, Jacopo Mondi wrote: > > >>> On Thu, Jun 25, 2020 at 12:07:39PM +0300, Laurent Pinchart wrote: > > >>>> On Thu, Jun 25, 2020 at 11:07:33AM +0200, Jacopo Mondi wrote: > > >>>>> On Thu, Jun 25, 2020 at 11:57:05AM +0300, Laurent Pinchart wrote: > > >>>>>> On Thu, Jun 25, 2020 at 09:37:39AM +0200, Jacopo Mondi wrote: > > >>>>>>> On Thu, Jun 25, 2020 at 04:23:30AM +0300, Laurent Pinchart wrote: > > >>>>>>>> It's common for code to check if a size is null. Add a helper function > > >>>>>>>> to do so. > > >>>>>>>> > > >>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>>>>>>> --- > > >>>>>>>> Changes since v1: > > >>>>>>>> > > >>>>>>>> - Add test > > >>>>>>>> --- > > >>>>>>>> include/libcamera/geometry.h | 1 + > > >>>>>>>> src/libcamera/geometry.cpp | 6 ++++++ > > >>>>>>>> test/geometry.cpp | 10 ++++++++++ > > >>>>>>>> 3 files changed, 17 insertions(+) > > >>>>>>>> > > >>>>>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > >>>>>>>> index edda42cf34cc..7d4b8bcfe3d8 100644 > > >>>>>>>> --- a/include/libcamera/geometry.h > > >>>>>>>> +++ b/include/libcamera/geometry.h > > >>>>>>>> @@ -41,6 +41,7 @@ struct Size { > > >>>>>>>> unsigned int width; > > >>>>>>>> unsigned int height; > > >>>>>>>> > > >>>>>>>> + bool isNull() const { return !width && !height; } > > >>>>>>> > > >>>>>>> Is isNull() a good name ? We have isValid() for PixelFormat() should > > >>>>>>> we stay consistent ? > > >>>>>> > > >>>>>> I initially had isEmpty(), and I've taken inspiration from QRect the > > >>>>>> defines isNull() as !width && !height (or rather width <= 0 && height <= > > >>>>>> 0, but our width and height are unsigned), and isEmpty() as !width || > > >>>>>> !height. > > >>>>> > > >>>>> I was more concerned about the fact we have a number of classes using > > >>>>> isValid() already, and consistency with the library is important, > > >>>>> otherwise it's a constant "go to look at the class definition", much > > >>>>> similar to when we had Rectangle::width and Size::w > > >>>>> > > >>>>> include/libcamera/file_descriptor.h: bool isValid() const { return fd_ != nullptr; } > > >>>>> include/libcamera/internal/formats.h: bool isValid() const { return format.isValid(); } > > >>>>> include/libcamera/internal/ipa_module.h: bool isValid() const; > > >>>>> include/libcamera/internal/ipa_proxy.h: bool isValid() const { return valid_; } > > >>>>> include/libcamera/internal/pub_key.h: bool isValid() const { return valid_; } > > >>>>> include/libcamera/internal/v4l2_pixelformat.h: bool isValid() const { return fourcc_ != 0; } > > >>>>> include/libcamera/pixel_format.h: constexpr bool isValid() const { return fourcc_ != 0; } > > >>>>> > > >>>>> For the record we have one isEmpty() > > >>>>> nclude/libcamera/internal/formats.h: bool isEmpty() const; > > >>>>> > > >>>>> but no isNull(). I would avoid introducing another term for a similar, > > >>>>> if not the same, concept. > > >>>> > > >>>> That boils down to the question of is a Size(0, 0) "invalid" ? > > >>> > > >>> Sorry about this, but how would you define a "valid" size ? To me > > >>> whatever size that has -both- width and height > 0 is valid. > > >>> Everything else is not. I don't really see a need for a distinction > > >>> between a "valid" (w || h) and a "!null" (or zero) one (w && h). > > >>> > > >>> A size with any of its members set to 0 is not valid. Is this too > > >>> harsh ? > > >> > > >> That wasn't the question :-) My point is that the name "valid" doesn't > > >> seem to be a good candidate to me here, I wouldn't call a size that has > > >> a width or height (or both) equal to 0 as "invalid". It's a > > >> zero/null/empty size, but not intrinsicly invalid. Whether such a size > > >> can be valid for a given usage depends on the usage, but isn't a > > >> property of the Size class in my opinion. > > >> > > > > > > As you wish. This is going into the philosophical territory. To me a > > > Size with any of its member set to 0 is empty, null, invalid or > > > whatever as I don't see a need to distinguish between an "empty" a > > > "null" or a "valid" size. I just wanted to use the most widespread > > > name we have to avoid having to look it up every time, which is just > > > annoying. That said, up to you :) > > > > > > Personally I would say that a size of 0 is 'valid'. Maybe it depends on > > context, but I can attest that the size of my cherry-bakewell is now 0 > > in both width, height (and depth): > > > > if (cherry-bakewell.size == Size(0, 0)) { > > std::cout << "Was it yummy?" << std::endl; > > } else { > > std::cout << "C'mon - eat up..." << std::endl; > > } > > > > isNull, isZero, would both be fine with me (I'd actually use isZero) But > > for some reason - not isEmpty(). > > > > I don't think you'd have an empty size, as the 'size' is not a container.
diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h index edda42cf34cc..7d4b8bcfe3d8 100644 --- a/include/libcamera/geometry.h +++ b/include/libcamera/geometry.h @@ -41,6 +41,7 @@ struct Size { unsigned int width; unsigned int height; + bool isNull() const { return !width && !height; } const std::string toString() const; }; diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp index fd78cf2c0ab7..24c44fb43acf 100644 --- a/src/libcamera/geometry.cpp +++ b/src/libcamera/geometry.cpp @@ -107,6 +107,12 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs) * \brief The Size height */ +/** + * \fn bool Size::isNull() const + * \brief Check if the size is null + * \return True if both the width and height are 0, or false otherwise + */ + /** * \brief Assemble and return a string describing the size * \return A string describing the size diff --git a/test/geometry.cpp b/test/geometry.cpp index 27e65565d7c6..904ad92c9448 100644 --- a/test/geometry.cpp +++ b/test/geometry.cpp @@ -36,6 +36,16 @@ protected: int run() { + if (!Size().isNull() || !Size(0, 0).isNull()) { + cout << "Null size incorrectly reported as not null" << endl; + return TestFail; + } + + if (Size(0, 100).isNull() || Size(100, 0).isNull() || Size(100, 100).isNull()) { + cout << "Non-null size incorrectly reported as null" << endl; + return TestFail; + } + /* Test Size equality and inequality. */ if (!compare(Size(100, 100), Size(100, 100), &operator==, "==", true)) return TestFail;
It's common for code to check if a size is null. Add a helper function to do so. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Add test --- include/libcamera/geometry.h | 1 + src/libcamera/geometry.cpp | 6 ++++++ test/geometry.cpp | 10 ++++++++++ 3 files changed, 17 insertions(+)