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

[Kieran Bingham]

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