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

18084 diff mbox series

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@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 99 ++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

Hi David,

Thank you for your work.

On Tue, 3 Jan 2023 at 11:33, David Plowman via libcamera-devel <
libcamera-devel@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@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?


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

With those addressed (if appropriate):

Reviewed-by: Naushir Patuck <naushir@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
>
>

Hi Naush

Thanks for the suggestions.

On Fri, 6 Jan 2023 at 10:09, Naushir Patuck <naush@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@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@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@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
>>