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

Laurent Pinchart laurent.pinchart at ideasonboard.com
Fri Jun 26 03:50:47 CEST 2020


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

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list