[libcamera-devel] [PATCH v10 0/8] Colour spaces

[David Plowman]

Hi everyone

We're on to version 10 now! Thanks for all the latest round of
reviews, that's much appreciated. I've adopted pretty much all of
those suggestions, but I'll try and round up the handful of discussion
points into this cover note.

1. Should we have a "None" Y'CbCr encoding?

I have added one as it bothered me to have "Rec601" in ColorSpace::Raw
otherwise. I wonder slightly what you should do if a device returns
V4L2_COLORSPACE_RAW and a non-default YCbCr encoding. What would that
mean? Should we turn it into ColorSpace::Raw regardless, or take it at
face value and let a pipeline handler complain (if it cares)?

For now I've taken the second approach, I suspect that in reality no
one is really going to care, but we could change that.

2. std::array in place of std::vector in color_space.cpp

Unfortunately I still can't make the std::array constexpr, because the
named ColorSpaces within them aren't. And they can't be because
they're static class members which would have to be initialised within
the class, only you can't initialise the objects of the class from
within the declaration...

But I have changed it to std::array anyway (just a const one), aren't
they in theory marginally cheaper than std::vectors?

3. Error messages in v4l2_videodevice.cpp etc.

Should these be in to/fromColorSpace instead? I've left them where
they are because I can still imagine a pipeline handler calling these
functions to check the results for themselves, at which point
functions that spit out messages are just annoying.

But I don't feel very strongly about this given that currently no one
else is calling them, so would be happy enough to change it!

4. Why should the largest output stream determine the color space?

Because it's the biggest, therefore it's the most important. :)

Though to be fair sometimes we use our low resolution output stream
for image analysis that users never see, so the colour space there
probably is less important. But in the end we have to pick one of
them and it doesn't really matter that much which.

5. Warnings in the RPi pipeline handler...

I've removed one bunch of warnings on the assurance that validate()
has been called... Some others I've left in because they really
shouldn't happen and I am completely paranoid about colour spaces. If
I could be less anxious about them that would be nice...

6. Where to call validateColorSpaces?

I've left this where it is. The one thing we're never going to do is
change raw pixel formats to non-raw ones, and vice versa. And mostly I
think pixel formats have little to do with colour spaces. There are
some exceptions (e.g. raw formats imply "raw" colour spaces), and YUV
type outputs imply using the Y'CbCr encoding matrix which is sort of
related, but apart from that I think they're mostly distinct.
(Everyone will now list more exceptions...)

Anyway, thanks again for all the effort that has gone into the many
reviews!

Best regards

David

David Plowman (8):
  libcamera: Add ColorSpace class
  libcamera: stream: Add ColorSpace fields to StreamConfiguration
  libcamera: video_device: Convert between ColorSpace class and V4L2
    formats
  libcamera: video_device: Support passing ColorSpaces to V4L2 video
    devices
  libcamera: v4l2_subdevice: Add colorSpace field to V4L2SubdeviceFormat
  libcamera: v4l2_subdevice: Support passing ColorSpaces to V4L2
    subdevices
  libcamera: Add validateColorSpaces to CameraConfiguration class
  libcamera: pipeline: raspberrypi: Support color spaces

 include/libcamera/camera.h                    |  10 +
 include/libcamera/color_space.h               |  70 ++++
 include/libcamera/internal/v4l2_device.h      |   8 +
 include/libcamera/internal/v4l2_subdevice.h   |   3 +
 include/libcamera/internal/v4l2_videodevice.h |   3 +
 include/libcamera/meson.build                 |   1 +
 include/libcamera/stream.h                    |   3 +
 src/libcamera/camera.cpp                      |  84 +++++
 src/libcamera/camera_sensor.cpp               |   1 +
 src/libcamera/color_space.cpp                 | 318 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 src/libcamera/pipeline/ipu3/cio2.cpp          |   7 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      |  40 +++
 src/libcamera/pipeline/simple/simple.cpp      |   8 +-
 src/libcamera/stream.cpp                      |  20 ++
 src/libcamera/v4l2_device.cpp                 | 194 +++++++++++
 src/libcamera/v4l2_subdevice.cpp              |  25 +-
 src/libcamera/v4l2_videodevice.cpp            |  32 ++
 18 files changed, 821 insertions(+), 7 deletions(-)
 create mode 100644 include/libcamera/color_space.h
 create mode 100644 src/libcamera/color_space.cpp

-- 
2.30.2