[libcamera-devel,RFC,2/4] libcamera: colorspace: Adjust colorspace of YUV streams

16919 diff mbox series

If a YUV stream is requested with no Y'Cbcr encoding, we need to adjust
it to provide a Y'Cbcr encoding. Depending on the transfer function and
primaries specified, a Y'Cbcr encoding is picked up.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 include/libcamera/color_space.h |  6 ++++
 src/libcamera/camera.cpp        | 11 ++++++
 src/libcamera/color_space.cpp   | 61 +++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

Hi Umang,

Thank you for the patch.

On Wed, Aug 03, 2022 at 12:27:17AM +0530, Umang Jain via libcamera-devel wrote:
> If a YUV stream is requested with no Y'Cbcr encoding, we need to adjust
> it to provide a Y'Cbcr encoding. Depending on the transfer function and
> primaries specified, a Y'Cbcr encoding is picked up.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  include/libcamera/color_space.h |  6 ++++
>  src/libcamera/camera.cpp        | 11 ++++++
>  src/libcamera/color_space.cpp   | 61 +++++++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+)
> 
> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> index 0d39fbc0..113e2715 100644
> --- a/include/libcamera/color_space.h
> +++ b/include/libcamera/color_space.h
> @@ -12,6 +12,8 @@
>  
>  namespace libcamera {
>  
> +struct StreamConfiguration;
> +
>  class ColorSpace
>  {
>  public:
> @@ -59,6 +61,10 @@ public:
>  
>  	std::string toString() const;
>  	static std::string toString(const std::optional<ColorSpace> &colorSpace);
> +	ColorSpace adjust(const StreamConfiguration &cfg);

This function is meant to be used internally inside libcamera, I would
prefer to avoid exposing it in the public API.

> +
> +private:
> +	ColorSpace adjustYuv(const StreamConfiguration &cfg);
>  };
>  
>  bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 3910915c..05135d16 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -386,6 +386,17 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>  	if (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
>  		return status;
>  
> +	/*
> +	 * \todo Question StreamsShareColorSpace <> an adjusted colorspace
> +	 * interation.
> +	 */
> +	if (config_[index].colorSpace) {
> +		ColorSpace oldColorspace = config_[index].colorSpace.value();
> +		ColorSpace newColorspace = oldColorspace.adjust(config_[index]);
> +		if (oldColorspace != newColorspace)
> +			config_[index].colorSpace = newColorspace;

Don't you need to set status to Adjusted here ?

> +	}

The color space needs to be adjusted for every stream, even in the
non-shared color space case.

> +
>  	/* Make all output color spaces the same, if requested. */

I think we'll need to extend this in the future, with additional flags
to tell if the full color space is shared, or only part of it. The color
primaries and transfer functions are typically configured in the ISP and
shared between streams, but the quantization range (and perhaps even the
YCbCr encoding) can possibly differ between ISP outputs. Let's address
that later if the need arises.

>  	for (auto &cfg : config_) {
>  		if (!isRaw(cfg.pixelFormat) &&

I would split the change to this file to a separate patch as it will
grow bigger.

> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> index 73148228..ff3f76d9 100644
> --- a/src/libcamera/color_space.cpp
> +++ b/src/libcamera/color_space.cpp
> @@ -13,6 +13,10 @@
>  #include <sstream>
>  #include <utility>
>  
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/formats.h"
> +
>  /**
>   * \file color_space.h
>   * \brief Class and enums to represent color spaces
> @@ -193,6 +197,63 @@ std::string ColorSpace::toString() const
>  	return ss.str();
>  }
>  
> +/**
> + * \brief Adjust colospace when stream configuration contains YUV stream
> + * \param[in] config Stream configuration
> + *
> + * This function adjust the stream's colorspace if it consists a YUV stream
> + * and has no Y'Cbcr encoding specified. The function shall update the
> + * Y'Cbcr encoding based on the transfer function and primaries fields.
> + *
> + * \return The adjusted colorspace
> + */
> +ColorSpace
> +ColorSpace::adjustYuv(const StreamConfiguration &cfg)
> +{
> +	ColorSpace cs = *this;
> +	bool isYUV = (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
> +		      PixelFormatInfo::ColourEncodingYUV);
> +
> +	if (isYUV && cs.ycbcrEncoding == YcbcrEncoding::None) {
> +		if (cs.transferFunction == TransferFunction::Rec709) {
> +			switch (cs.primaries) {
> +			/* Raw should never happen */
> +			case Primaries::Raw:
> +			case Primaries::Smpte170m:
> +				cs.ycbcrEncoding = YcbcrEncoding::Rec601;
> +				break;
> +			case Primaries::Rec709:
> +				cs.ycbcrEncoding = YcbcrEncoding::Rec709;
> +				break;
> +			case Primaries::Rec2020:
> +				cs.ycbcrEncoding = YcbcrEncoding::Rec2020;
> +				break;
> +			}
> +		} else if (cs.transferFunction == TransferFunction::Srgb) {
> +			cs.ycbcrEncoding = YcbcrEncoding::Rec601;
> +		}
> +
> +		/* \todo: Determine if range needs to be adjusted in some cases? */

Any opinion ?

> +	}
> +
> +	return cs;
> +}
> +
> +/**
> + * \brief Adjust the colorspace depending on the stream configuration
> + * \param[in] config Stream configuration
> + *
> + * This function adjust the stream's colorspace depending on various factors
> + * as reflected by the \a config.
> + *
> + * \return The adjusted colorspace according to the stream configuration

Given the function name, and the fact the function isn't const, I would
have guessed that it adjusts the color space in-place. This is
error-prone, someone could call the function and ignore the return
value. One option is to make this a const function and mark the return
value as [[nodiscard]], but I think it would be better, given the
function name, to adjust the color space in-place.

> + */
> +ColorSpace
> +ColorSpace::adjust(const StreamConfiguration &config)
> +{
> +	return adjustYuv(config);
> +}

Why is this split in two functions ?

> +
>  /**
>   * \brief Assemble and return a readable string representation of an
>   * optional ColorSpace