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

[Umang Jain]

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 at raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush 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..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)