[libcamera-devel] [PATCH v2 0/8] libcamera: PixelFormat: Turn into a class

[Laurent Pinchart]

Hi Niklas,

On Tue, Mar 17, 2020 at 04:52:31AM +0100, Niklas Söderlund wrote:
> Hello,
> 
> This series replaces the PixelFormat definition of a unisgned int with a
> class implementation that can hold both a FourCC and a set of modifiers.
> This is important to allow users of libcamera to look at modifiers.
> 
> This series do not make use of the modifiers, a follow up series that
> adds RAW capture to the IPU3 will make use of them to describe the IPU3
> Bayer format memory layout.
> 
> I have looked at Laurent's series for V4L2PixelFormat [1] and I agree we 
> should align the interface between the two. I have however not taken all 
> concepts from it (yet). The two outstanding questions for me are
> 
> 1. operator uint32_t()
> 
>    I agree this looks real nice for V4l2PixelFormat, would it be as nice 
>    for PixelFormot which can have modifiers? I have no strong feelings.  
>    I feel that if we go for it V4L2PixelFormat::value() and 
>    PixelFormat::fourcc() should then be dropped.

We could drop V4L2PixelFormat::value(), but then when you need to access
the numerical value you will need to static_cast<uint32_t>(format),
which isn't nice. For PixelFormat I wouldn't drop fourcc(), as it's the
counterpart to modifiers().

>    Two ways to do the same thing can't be good ;-)

It's use case-dependent :-) If we have to drop one, I would drop the
conversion operator, but then you can't write

	if (format == V4L2_PIX_FMT_YUYV)
		...

but will have to write either

	if (format == V4L2PixelFormat(V4L2_PIX_FMT_YUYV))
		...

or

	if (format.value() == V4L2_PIX_FMT_YUYV)
		...

The goal of the PixelFormat and V4L2PixelFormat isn't to made writing
code more difficult everywhere, but to force the user to check if the
format they're using is actually of the right type. There's value in
forcing replacement of

	PixelFormat format = fourcc;

with

	PixelFormat format = PixelFormat(fourcc);

as you have to think if fourcc is really a DRM fourcc, not a V4L2
fourcc. In the cases above though,

	if (format.value() == V4L2_PIX_FMT_YUYV)
		...

doesn't provide any additional value in my opinion, as there's no
safeguard against format being a PixelFormat (OK, PixelFormat has
.fourcc() and format has .value(), but that's easy to overlook). On the
other hand,

	if (format == V4L2PixelFormat(V4L2_PIX_FMT_YUYV))
		...

would not compile if format was a PixelFormat.

If we decide to drop the conversion operators, and really want maximum
safety, we should thus use the second form of comparison, not the first
one. Note that this wouldn't work for a switch() statement anyway, as
neither PixelFormat nor V4L2PixelFormat have constexpr constructors, so
for that we'll have to use .value() or .fourcc(), which defeats the
point a little bit.

> 2. isValid()
> 
>    Is this good or bad, I can't make up my mind. Yet again the modifiers 
>    plays in. How would PixelFormat::isValid() deal with 
>    DRM_FORMAT_SRGGB10 without any modifiers to describe the memory 
>    layout?

There's a need for a default constructor for the PixelFormat and
V4L2PixelFormat classes. That constructor sets the fourcc to 0, creating
an "invalid" pixel format. I think it's a good idea to add an isValid()
function to check that, to avoid hardcoding a comparison with 0 in the
caller. This effectively makes the fact that an invalid pixel format has
a 0 fourcc an implementation detail, and allows us to change that in the
future if needed. In also makes the code more explicit, as it's clear
from .isValid() that you're checking if the pixel format is valid or
not. We could however rename that to isInitialized() but I don't like it
much. Feel free to propose a better name if you think of one.

> 1. [PATCH 0/2] Add a V4L2PixelFormat class
> 
> Laurent Pinchart (1):
>   libcamera: PixelFormat: Make constructor explicit
> 
> Niklas Söderlund (7):
>   libcamera: Use PixelFormat instead of unsigned int where appropriate
>   test: v4l2_videodevice: buffer_cache: Use DRM pixel format
>   libcamera: pipeline: vimc: Remove internal usage of ImageFormats
>   libcamera: pipeline: uvcvideo: Translate from V4L2 to DRM pixel
>     formats
>   libcamera: v4l2_videodevice: Remove usage of ImageFormats
>   libcamera: PixelFormat: Turn into a class
>   libcamera: PixelFormat: Mark all function arguments of type
>     PixelFormat as const reference
> 
>  include/libcamera/pixelformats.h         | 21 ++++++-
>  include/libcamera/stream.h               |  4 +-
>  src/cam/main.cpp                         |  8 +--
>  src/gstreamer/gstlibcamera-utils.cpp     | 18 +++---
>  src/libcamera/include/v4l2_videodevice.h |  7 ++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  8 +--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 22 +++----
>  src/libcamera/pipeline/uvcvideo.cpp      | 19 ++++--
>  src/libcamera/pipeline/vimc.cpp          | 20 +++---
>  src/libcamera/pixelformats.cpp           | 77 ++++++++++++++++++++++--
>  src/libcamera/stream.cpp                 |  8 +--
>  src/libcamera/v4l2_videodevice.cpp       | 47 +++++++--------
>  src/qcam/format_converter.cpp            |  6 +-
>  src/qcam/format_converter.h              |  6 +-
>  src/qcam/viewfinder.cpp                  |  4 +-
>  src/qcam/viewfinder.h                    |  6 +-
>  src/v4l2/v4l2_camera.cpp                 |  2 +-
>  src/v4l2/v4l2_camera.h                   |  2 +-
>  src/v4l2/v4l2_camera_proxy.cpp           | 34 +++++------
>  src/v4l2/v4l2_camera_proxy.h             |  2 +-
>  test/stream/stream_formats.cpp           | 24 ++++----
>  test/v4l2_videodevice/buffer_cache.cpp   |  4 +-
>  22 files changed, 221 insertions(+), 128 deletions(-)

-- 
Regards,

Laurent Pinchart