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

15090 mbox series

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

Hi David,

On Thu, Dec 09, 2021 at 10:12:37AM +0000, David Plowman wrote:
> Hi everyone
> 
> We're on to version 10 now!

8 more versions to come of age :-D

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

I should have read this before reviewing, looks like I've repeated
questions in 8/8 that you're answering below.

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

Probably that the driver author didn't know what to do.

> Should we turn it into ColorSpace::Raw regardless, or take it at
> face value and let a pipeline handler complain (if it cares)?

Maybe we could handle it when the situation arises.

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

OK, no worries.

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

Marginally, yes, I think.

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

I agree that we shouldn't have warning or error messages for situations
that can arise under normal circumstances.

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

Maybe we could do so to avoid duplication of messages, and change it
later if needed ? The toColorSpace() and fromColorSpace() functions
should be made protected in that case, to make sure we noticed attempts
to use them directly from pipeline handlers.

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

If you could capture the rationale in a sentence or two in the patch or
commit message, that would be nice.

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

There's just one warning message left, that tells if colour spaces have
been adjusted. Isn't that allowed under normal circumstances ?

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

OK, sounds good to me.

> Anyway, thanks again for all the effort that has gone into the many
> reviews!
> 
> 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