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

[Niklas Söderlund]

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 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 :)
> 
> 
> 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> > 
> 
> -- 
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund