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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart