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

18105 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 on the way back.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 99 ++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

Hi David,

Thanks for the patch.

On 1/12/23 5:40 PM, David Plowman via libcamera-devel 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

If I may, can you point to sources what the hardware / firmware expects 
with respect to colorspaces /exactly/ ?

The larger issue I see this is as a fragile piece, that has been 
regressed. Moving forward, I believe there is a high chance that the 
regression might happen again (provided the fragility) hence a solid 
document on the Pi's hardware / firmware should be included in the code 
base I believe.  If it has been summarised in any other colorspace 
threads, I would be happy to take a look at that as well!
> is also careful to check what comes back as the YCbCr information gets
> overwritten again on the way back.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@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..135024e7 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_;
>   };
>   
>   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)
> +{
> +	const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> +	return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;
> +}
> +
> +static bool isYuv(const PixelFormat &pixFmt)
> +{
> +	/* The code below would return true for raw mono streams, so weed those out first. */
> +	if (isRaw(pixFmt))
> +		return false;
> +
> +	const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> +	return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;
> +}

Both of these helpers can be utils I believe - but can be done on top as 
well...
> +
> +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;

Can be slightly optimised by having a    :
               continue;
> +		}
> +
> +		/* 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;

I believe there is some degree of logic duplication coming primarily 
from CameraConfiguration::validateColorSpaces() which calls 
ColorSpace::adjust()

I am not sure (yet) if we want to merge the two - i.e. a overall 
CameraConfiguration::validateColorspace() with a pipeline-handler 
specific PHValidateColorSpace() working / adjusting on top...  and have 
a more clear layers of functions operating on user-input-ed colorspace
or
Have a single function only for adjusting colorspace (either use 
CameraConfiguration::validateColorspace() OR a pipeline-handler specific 
one like you've done here).

But .... nonetheless  I believe:

- Assigning ColorSpace::Raw  for the raw streams above
- For clearing Y'CbCrEncoding and range on RGB stream

One can use the ColorSpace::adjust() helper and avoid the code 
duplication here... no?
> +
> +	/* 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;

I am not sure what's happening here.
> +			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)

Hi Umang

Thanks for the comments!

On Tue, 17 Jan 2023 at 08:47, Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Hi David,
>
> Thanks for the patch.
>
> On 1/12/23 5:40 PM, David Plowman via libcamera-devel 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
>
> If I may, can you point to sources what the hardware / firmware expects
> with respect to colorspaces /exactly/ ?

The Pi is very simple. When a raw stream is being used, the V4L2
drivers expect the colour space to be V4L2_COLORSPACE_RAW.

For non-raw streams, the colour space should be one of
V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M or
V4L2_COLORSPACE_REC709. That's it. All the other colour space modifier
fields are ignored.

Correspondingly, the only colour spaces I want to use in libcamera are
ColorSpace::Raw (raw streams only), otherwise ColorSpace::Sycc,
ColorSpace::Smpte170m or ColorSpace::Rec709. It should be very easy!

>
> The larger issue I see this is as a fragile piece, that has been
> regressed. Moving forward, I believe there is a high chance that the
> regression might happen again (provided the fragility) hence a solid
> document on the Pi's hardware / firmware should be included in the code
> base I believe.  If it has been summarised in any other colorspace
> threads, I would be happy to take a look at that as well!

Where would be the best place to put this, do you think? I'm very
happy to add something - as I explained above, the situation on the Pi
is very straightforward.

We have very comprehensive tests for all our stream/colour space
combinations so we should notice straight away when things are broken.
Hopefully Kieran will be able to start running these once this patch
is merged and colour spaces on the Pi are working again.

> > is also careful to check what comes back as the YCbCr information gets
> > overwritten again on the way back.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@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..135024e7 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_;
> >   };
> >
> >   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)
> > +{
> > +     const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > +     return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;
> > +}
> > +
> > +static bool isYuv(const PixelFormat &pixFmt)
> > +{
> > +     /* The code below would return true for raw mono streams, so weed those out first. */
> > +     if (isRaw(pixFmt))
> > +             return false;
> > +
> > +     const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > +     return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;
> > +}
>
> Both of these helpers can be utils I believe - but can be done on top as
> well...

Possibly... though some other PHs don't agree with my definition of
isYuv (for raw mono streams, as noted in the comments), and
consequently, our definition of isRaw too. So I think I may be better
off with local versions here.

> > +
> > +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;
>
> Can be slightly optimised by having a    :
>                continue;

Sure, I can do that!

> > +             }
> > +
> > +             /* 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;
>
> I believe there is some degree of logic duplication coming primarily
> from CameraConfiguration::validateColorSpaces() which calls
> ColorSpace::adjust()
>
> I am not sure (yet) if we want to merge the two - i.e. a overall
> CameraConfiguration::validateColorspace() with a pipeline-handler
> specific PHValidateColorSpace() working / adjusting on top...  and have
> a more clear layers of functions operating on user-input-ed colorspace
> or
> Have a single function only for adjusting colorspace (either use
> CameraConfiguration::validateColorspace() OR a pipeline-handler specific
> one like you've done here).
>
> But .... nonetheless  I believe:
>
> - Assigning ColorSpace::Raw  for the raw streams above
> - For clearing Y'CbCrEncoding and range on RGB stream
>
> One can use the ColorSpace::adjust() helper and avoid the code
> duplication here... no?

I'm not terribly keen on the automatic colour space changes that are
applied in ColorSpace::adjust() so I'd rather be in control of them
myself. Really, I don't want any general-purpose functions that change
the colour space, either when I send stuff to the drivers or look at
what comes back, without me knowing what has happened.

> > +
> > +     /* 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;
>
> I am not sure what's happening here.

The current libcamera behaviour is that you can't ask for (for
example) ColorSpace::Rec709 when the output format is RGB. Instead,
the colour space must be changed to Rec709/Srgb/None/Full.

In the code above I leave Rec709 alone for YUV streams, but "correct"
it to Rec709/Srgb/None/Full for RGB streams. But I've decided not to
flag the config as "adjusted" if the colour space was Rec709 initially
(if it was something else then sure, that's "adjusted"). The reason is
that nothing has really changed - the output pixels will be no
different - so forcing the user to figure out what got changed and
whether it matters, seems quite unfriendly. But we do have a choice
whether to be nice to our users or not!

It's worth noting that the base class validateColorSpaces method
doesn't handle "shared" colour spaces correctly any more, it even
seems like an awkward concept now. Personally, I'm tending to think we
should remove it, at which point the base class method doesn't provide
a huge amount of value to the Pi.

You know how it goes - I thought I was doing the world a favour by
adding a base class method but now, well, I wish I hadn't.  :(

Thanks!
David

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