[libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Fix handling of colour spaces

[David Plowman]

Hi Naush

Thanks for the suggestions.

On Fri, 6 Jan 2023 at 10:09, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for your work.
>
> On Tue, 3 Jan 2023 at 11:33, David Plowman via libcamera-devel <libcamera-devel at lists.libcamera.org> wrote:
>>
>> We implement a custom validateColorSpaces method that forces all
>> (non-raw) streams to same colour space, whilst distinguishing RGB
>> streams from YUV ones, as the former must have the YCbCr encoding and
>> range over-written.
>>
>> When we apply the colour space, we always send the full YUV version as
>> that gets converted correctly to what our hardware drivers expect. It
>> is also careful to check what comes back as the YCbCr information gets
>> overwritten again.
>>
>> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
>> ---
>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 99 ++++++++++++++++++-
>>  1 file changed, 98 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index 8569df17..bb574afc 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public CameraConfiguration
>>  public:
>>         RPiCameraConfiguration(const RPiCameraData *data);
>>
>> +       CameraConfiguration::Status validateColorSpaces(ColorSpaceFlags flags);
>>         Status validate() override;
>>
>>         /* Cache the combinedTransform_ that will be applied to the sensor */
>> @@ -317,6 +318,13 @@ public:
>>
>>  private:
>>         const RPiCameraData *data_;
>> +
>> +       /*
>> +        * Store the colour spaces that all our streams will have. RGB format streams
>> +        * will be the same but need the YCbCr fields cleared.
>> +        */
>> +       std::optional<ColorSpace> yuvColorSpace_;
>> +       std::optional<ColorSpace> rgbColorSpace_;
>>  };
>
>
> Should these fields live in RPiCameraData for the multi-camera cases?

Having looked again at this, I think we've decided it's OK.

>
>>
>>
>>  class PipelineHandlerRPi : public PipelineHandler
>> @@ -357,6 +365,89 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
>>  {
>>  }
>>
>> +static const std::vector<ColorSpace> validColorSpaces = {
>> +       ColorSpace::Sycc,
>> +       ColorSpace::Smpte170m,
>> +       ColorSpace::Rec709
>> +};
>> +
>> +static std::optional<ColorSpace> findValidColorSpace(const ColorSpace &colourSpace)
>> +{
>> +       for (auto cs : validColorSpaces) {
>> +               if (colourSpace.primaries == cs.primaries &&
>> +                   colourSpace.transferFunction == cs.transferFunction)
>> +                       return cs;
>> +       }
>> +
>> +       return std::nullopt;
>> +}
>> +
>> +static bool isRgb(const PixelFormat &pixFmt)
>> +{
>> +       /*
>> +        * Be careful not to use this when a format might be raw because it returns
>> +        * the wrong result for mono raw streams.
>> +        */
>
>
> Could we add an assert that tests this?

True enough. In fact, why don't I just handle that case correctly?
I'll submit a v3 with that little change!

Thanks
David

>
> With those addressed (if appropriate):
>
> Reviewed-by: Naushir Patuck <naushir at raspberrypi.com>
>
>>
>> +       const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
>> +       return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;
>> +}
>> +
>> +static bool isYuv(const PixelFormat &pixFmt)
>> +{
>> +       const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
>> +       return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;
>> +}
>> +
>> +CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_unused]] ColorSpaceFlags flags)
>> +{
>> +       Status status = Valid;
>> +       yuvColorSpace_.reset();
>> +
>> +       for (auto cfg : config_) {
>> +               /* First fix up raw streams to have the "raw" colour space. */
>> +               if (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) {
>> +                       /* If there was no value here, that doesn't count as "adjusted". */
>> +                       if (cfg.colorSpace)
>> +                               status = Adjusted;
>> +                       cfg.colorSpace = ColorSpace::Raw;
>> +               }
>> +
>> +               /* Next we need to find our shared colour space. The first valid one will do. */
>> +               if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw && !yuvColorSpace_)
>> +                       yuvColorSpace_ = findValidColorSpace(cfg.colorSpace.value());
>> +       }
>> +
>> +       /* If no colour space was given anywhere, choose sYCC. */
>> +       if (!yuvColorSpace_)
>> +               yuvColorSpace_ = ColorSpace::Sycc;
>> +
>> +       /* Note the version of this that any RGB streams will have to use. */
>> +       rgbColorSpace_ = yuvColorSpace_;
>> +       rgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;
>> +       rgbColorSpace_->range = ColorSpace::Range::Full;
>> +
>> +       /* Go through the streams again and force everyone to the same colour space. */
>> +       for (auto cfg : config_) {
>> +               if (cfg.colorSpace == ColorSpace::Raw)
>> +                       continue;
>> +
>> +               if (isYuv(cfg.pixelFormat) && cfg.colorSpace != yuvColorSpace_) {
>> +                       /* Again, no value means "not adjusted". */
>> +                       if (cfg.colorSpace)
>> +                               status = Adjusted;
>> +                       cfg.colorSpace = yuvColorSpace_;
>> +               }
>> +               if (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) {
>> +                       /* Be nice, and let the YUV version count as non-adjusted too. */
>> +                       if (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_)
>> +                               status = Adjusted;
>> +                       cfg.colorSpace = rgbColorSpace_;
>> +               }
>> +       }
>> +
>> +       return status;
>> +}
>> +
>>  CameraConfiguration::Status RPiCameraConfiguration::validate()
>>  {
>>         Status status = Valid;
>> @@ -533,7 +624,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>>                 V4L2DeviceFormat format;
>>                 format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
>>                 format.size = cfg.size;
>> -               format.colorSpace = cfg.colorSpace;
>> +               /* We want to send the associated YCbCr info through to the driver. */
>> +               format.colorSpace = yuvColorSpace_;
>>
>>                 LOG(RPI, Debug)
>>                         << "Try color space " << ColorSpace::toString(cfg.colorSpace);
>> @@ -542,6 +634,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>>                 if (ret)
>>                         return Invalid;
>>
>> +               /*
>> +                * But for RGB streams, the YCbCr info gets overwritten on the way back
>> +                * so we must check against what the stream cfg says, not what we actually
>> +                * requested (which carefully included the YCbCr info)!
>> +                */
>>                 if (cfg.colorSpace != format.colorSpace) {
>>                         status = Adjusted;
>>                         LOG(RPI, Debug)
>> --
>> 2.30.2
>>