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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart