[libcamera-devel] [PATCH v2] libcamera: geometry: Add isNull() function to Size class

[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 at 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 at 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