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

[Jacopo Mondi]

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 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" ?
> >
> > 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