[libcamera-devel,v2.1,4/6] libcamera: color_space: Move color space adjustment to ColorSpace class

17202 diff mbox series

From: Umang Jain <umang.jain@ideasonboard.com>

The CameraConfiguration::validateColorSpaces() function performs color
space validation on a camera configuration, by validating the color
space of each stream individually, and optionally ensuring that all
streams share the same color space. The individual validation is very
basic, limited to ensuring that raw formats use a raw color space.

Color spaces are more constrained than that:

- The Y'CbCr encoding and quantization range for RGB formats must be
  YcbcrEncoding::None and Range::Full respectively.
- The Y'CbCr encoding for YUV formats must not be YcbcrEncoding::None.

Instead of open-coding these constraints in the validateColorSpaces()
function, create a new ColorSpace::adjust() function to centralize color
space validation and adjustment, and use it in validateColorSpaces().

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Hi Umang,

I thought it would be easier to send a patch than explaining exactly
what I was envisioning in the review.
---
 include/libcamera/color_space.h |   4 ++
 src/libcamera/camera.cpp        |  43 ++++++--------
 src/libcamera/color_space.cpp   | 101 ++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+), 26 deletions(-)

Hi Laurent,

Thank you for v2.1

On 8/25/22 9:36 PM, Laurent Pinchart wrote:
> From: Umang Jain <umang.jain@ideasonboard.com>
>
> The CameraConfiguration::validateColorSpaces() function performs color
> space validation on a camera configuration, by validating the color
> space of each stream individually, and optionally ensuring that all
> streams share the same color space. The individual validation is very
> basic, limited to ensuring that raw formats use a raw color space.
>
> Color spaces are more constrained than that:
>
> - The Y'CbCr encoding and quantization range for RGB formats must be
>    YcbcrEncoding::None and Range::Full respectively.
> - The Y'CbCr encoding for YUV formats must not be YcbcrEncoding::None.
>
> Instead of open-coding these constraints in the validateColorSpaces()
> function, create a new ColorSpace::adjust() function to centralize color
> space validation and adjustment, and use it in validateColorSpaces().
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Hi Umang,
>
> I thought it would be easier to send a patch than explaining exactly
> what I was envisioning in the review.

Nice :-)
> ---
>   include/libcamera/color_space.h |   4 ++
>   src/libcamera/camera.cpp        |  43 ++++++--------
>   src/libcamera/color_space.cpp   | 101 ++++++++++++++++++++++++++++++++
>   3 files changed, 122 insertions(+), 26 deletions(-)
>
> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> index 8030a264c66f..33e1e6999de1 100644
> --- a/include/libcamera/color_space.h
> +++ b/include/libcamera/color_space.h
> @@ -12,6 +12,8 @@
>   
>   namespace libcamera {
>   
> +class PixelFormat;
> +
>   class ColorSpace
>   {
>   public:
> @@ -59,6 +61,8 @@ public:
>   
>   	std::string toString() const;
>   	static std::string toString(const std::optional<ColorSpace> &colorSpace);
> +
> +	bool adjust(PixelFormat format);
>   };
>   
>   bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index a5c3aabeddbb..9fe29ca9dca5 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -317,17 +317,6 @@ std::size_t CameraConfiguration::size() const
>   	return config_.size();
>   }
>   
> -namespace {
> -
> -bool isRaw(const PixelFormat &pixFmt)
> -{
> -	const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> -	return info.isValid() &&
> -	       info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> -}
> -
> -} /* namespace */
> -
>   /**
>    * \enum CameraConfiguration::ColorSpaceFlag
>    * \brief Specify the behaviour of validateColorSpaces
> @@ -368,29 +357,31 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>   	Status status = Valid;
>   
>   	/*
> -	 * Set all raw streams to the Raw color space, and make a note of the largest
> -	 * non-raw stream with a defined color space (if there is one).
> +	 * Set all raw streams to the Raw color space, and make a note of the
> +	 * largest non-raw stream with a defined color space (if there is one).
>   	 */
> -	int index = -1;
> +	std::optional<ColorSpace> colorSpace;
> +
>   	for (auto [i, cfg] : utils::enumerate(config_)) {
> -		if (isRaw(cfg.pixelFormat)) {
> -			if (cfg.colorSpace != ColorSpace::Raw) {
> -				cfg.colorSpace = ColorSpace::Raw;
> -				status = Adjusted;
> -			}
> -		} else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {
> -			index = i;
> -		}
> +		if (!cfg.colorSpace)
> +			continue;
> +
> +		if (cfg.colorSpace->adjust(cfg.pixelFormat))
> +			status = Adjusted;
> +
> +		if (cfg.colorSpace != ColorSpace::Raw &&
> +		    (!colorSpace || cfg.size > config_[i].size))
> +			colorSpace = cfg.colorSpace;
>   	}
>   
> -	if (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
> +	if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
>   		return status;
>   
>   	/* Make all output color spaces the same, if requested. */
>   	for (auto &cfg : config_) {
> -		if (!isRaw(cfg.pixelFormat) &&
> -		    cfg.colorSpace != config_[index].colorSpace) {
> -			cfg.colorSpace = config_[index].colorSpace;
> +		if (cfg.colorSpace != ColorSpace::Raw &&
> +		    cfg.colorSpace != colorSpace) {
> +			cfg.colorSpace = colorSpace;
>   			status = Adjusted;
>   		}
>   	}

This is already looking so much cleaner :-)
> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> index cb47acf9727b..eefcf7616766 100644
> --- a/src/libcamera/color_space.cpp
> +++ b/src/libcamera/color_space.cpp
> @@ -13,6 +13,8 @@
>   #include <sstream>
>   #include <utility>
>   
> +#include "libcamera/internal/formats.h"
> +
>   /**
>    * \file color_space.h
>    * \brief Class and enums to represent color spaces
> @@ -203,6 +205,105 @@ std::string ColorSpace::toString() const
>   	return ss.str();
>   }
>   
> +/**
> + * \brief Adjust the color space to match a pixel format
> + * \param[in] format The pixel format
> + *
> + * Not all combinations of pixel formats and color spaces make sense. For
> + * instance, nobody uses a limited quantization range with raw Bayer formats,
> + * and the YcbcrEncoding::None encoding isn't valid for YUV formats. This
> + * function adjusts the ColorSpace to make it compatible with the given \a
> + * format, by applying the following rules:
> + *
> + * - The color space for RAW formats must be Raw.
> + * - The Y'CbCr encoding and quantization range for RGB formats must be
> + *   YcbcrEncoding::None and Range::Full respectively.
> + * - The Y'CbCr encoding for YUV formats must not be YcbcrEncoding::None. The
> + *   best encoding is in that case guessed based on the primaries and transfer
> + *   function.
> + *
> + * \return True if the color space has been adjusted, or false if it was
> + * already compatible with the format and hasn't been changed
> + */
> +bool ColorSpace::adjust(PixelFormat format)

Ah, ,my version of Colorspace::adjust API was terrible :S

v2.1 looks very good!

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +{
> +	const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +	bool adjusted = false;
> +
> +	switch (info.colourEncoding) {
> +	case PixelFormatInfo::ColourEncodingRAW:
> +		/* Raw formats must use the raw color space. */
> +		if (*this != ColorSpace::Raw) {
> +			*this = ColorSpace::Raw;
> +			adjusted = true;
> +		}
> +		break;
> +
> +	case PixelFormatInfo::ColourEncodingRGB:
> +		/*
> +		 * RGB formats can't have a Y'CbCr encoding, and must use full
> +		 * range quantization.
> +		 */
> +		if (ycbcrEncoding != YcbcrEncoding::None) {
> +			ycbcrEncoding = YcbcrEncoding::None;
> +			adjusted = true;
> +		}
> +
> +		if (range != Range::Full) {
> +			range = Range::Full;
> +			adjusted = true;
> +		}
> +		break;
> +
> +	case PixelFormatInfo::ColourEncodingYUV:
> +		if (ycbcrEncoding != YcbcrEncoding::None)
> +			break;
> +
> +		/*
> +		 * YUV formats must have a Y'CbCr encoding. Infer the most
> +		 * probable option from the transfer function and primaries.
> +		 */
> +		switch (transferFunction) {
> +		case TransferFunction::Linear:
> +			/*
> +			 * Linear YUV is not used in any standard color space,
> +			 * pick the widely supported and used Rec601 as default.
> +			 */
> +			ycbcrEncoding = YcbcrEncoding::Rec601;
> +			break;
> +
> +		case TransferFunction::Rec709:
> +			switch (primaries) {
> +			/* Raw should never happen. */
> +			case Primaries::Raw:
> +			case Primaries::Smpte170m:
> +				ycbcrEncoding = YcbcrEncoding::Rec601;
> +				break;
> +			case Primaries::Rec709:
> +				ycbcrEncoding = YcbcrEncoding::Rec709;
> +				break;
> +			case Primaries::Rec2020:
> +				ycbcrEncoding = YcbcrEncoding::Rec2020;
> +				break;
> +			}
> +			break;
> +
> +		case TransferFunction::Srgb:
> +			/*
> +			 * Only the sYCC color space uses the sRGB transfer
> +			 * function, the corresponding encoding is Rec601.
> +			 */
> +			ycbcrEncoding = YcbcrEncoding::Rec601;
> +			break;
> +		}
> +
> +		adjusted = true;
> +		break;
> +	}
> +
> +	return adjusted;
> +}
> +
>   /**
>    * \brief Assemble and return a readable string representation of an
>    * optional ColorSpace

Hi Umang,

On Fri, Aug 26, 2022 at 02:45:47PM +0530, Umang Jain wrote:
> On 8/25/22 9:36 PM, Laurent Pinchart wrote:
> > From: Umang Jain <umang.jain@ideasonboard.com>
> >
> > The CameraConfiguration::validateColorSpaces() function performs color
> > space validation on a camera configuration, by validating the color
> > space of each stream individually, and optionally ensuring that all
> > streams share the same color space. The individual validation is very
> > basic, limited to ensuring that raw formats use a raw color space.
> >
> > Color spaces are more constrained than that:
> >
> > - The Y'CbCr encoding and quantization range for RGB formats must be
> >    YcbcrEncoding::None and Range::Full respectively.
> > - The Y'CbCr encoding for YUV formats must not be YcbcrEncoding::None.
> >
> > Instead of open-coding these constraints in the validateColorSpaces()
> > function, create a new ColorSpace::adjust() function to centralize color
> > space validation and adjustment, and use it in validateColorSpaces().
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Hi Umang,
> >
> > I thought it would be easier to send a patch than explaining exactly
> > what I was envisioning in the review.
> 
> Nice :-)
> 
> > ---
> >   include/libcamera/color_space.h |   4 ++
> >   src/libcamera/camera.cpp        |  43 ++++++--------
> >   src/libcamera/color_space.cpp   | 101 ++++++++++++++++++++++++++++++++
> >   3 files changed, 122 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> > index 8030a264c66f..33e1e6999de1 100644
> > --- a/include/libcamera/color_space.h
> > +++ b/include/libcamera/color_space.h
> > @@ -12,6 +12,8 @@
> >   
> >   namespace libcamera {
> >   
> > +class PixelFormat;
> > +
> >   class ColorSpace
> >   {
> >   public:
> > @@ -59,6 +61,8 @@ public:
> >   
> >   	std::string toString() const;
> >   	static std::string toString(const std::optional<ColorSpace> &colorSpace);
> > +
> > +	bool adjust(PixelFormat format);
> >   };
> >   
> >   bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index a5c3aabeddbb..9fe29ca9dca5 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -317,17 +317,6 @@ std::size_t CameraConfiguration::size() const
> >   	return config_.size();
> >   }
> >   
> > -namespace {
> > -
> > -bool isRaw(const PixelFormat &pixFmt)
> > -{
> > -	const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > -	return info.isValid() &&
> > -	       info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> > -}
> > -
> > -} /* namespace */
> > -
> >   /**
> >    * \enum CameraConfiguration::ColorSpaceFlag
> >    * \brief Specify the behaviour of validateColorSpaces
> > @@ -368,29 +357,31 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >   	Status status = Valid;
> >   
> >   	/*
> > -	 * Set all raw streams to the Raw color space, and make a note of the largest
> > -	 * non-raw stream with a defined color space (if there is one).
> > +	 * Set all raw streams to the Raw color space, and make a note of the
> > +	 * largest non-raw stream with a defined color space (if there is one).
> >   	 */
> > -	int index = -1;
> > +	std::optional<ColorSpace> colorSpace;
> > +
> >   	for (auto [i, cfg] : utils::enumerate(config_)) {
> > -		if (isRaw(cfg.pixelFormat)) {
> > -			if (cfg.colorSpace != ColorSpace::Raw) {
> > -				cfg.colorSpace = ColorSpace::Raw;
> > -				status = Adjusted;
> > -			}
> > -		} else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {
> > -			index = i;
> > -		}
> > +		if (!cfg.colorSpace)
> > +			continue;
> > +
> > +		if (cfg.colorSpace->adjust(cfg.pixelFormat))
> > +			status = Adjusted;
> > +
> > +		if (cfg.colorSpace != ColorSpace::Raw &&
> > +		    (!colorSpace || cfg.size > config_[i].size))
> > +			colorSpace = cfg.colorSpace;
> >   	}
> >   
> > -	if (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
> > +	if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
> >   		return status;
> >   
> >   	/* Make all output color spaces the same, if requested. */
> >   	for (auto &cfg : config_) {
> > -		if (!isRaw(cfg.pixelFormat) &&
> > -		    cfg.colorSpace != config_[index].colorSpace) {
> > -			cfg.colorSpace = config_[index].colorSpace;
> > +		if (cfg.colorSpace != ColorSpace::Raw &&
> > +		    cfg.colorSpace != colorSpace) {
> > +			cfg.colorSpace = colorSpace;
> >   			status = Adjusted;
> >   		}
> >   	}
> 
> This is already looking so much cleaner :-)
> 
> > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> > index cb47acf9727b..eefcf7616766 100644
> > --- a/src/libcamera/color_space.cpp
> > +++ b/src/libcamera/color_space.cpp
> > @@ -13,6 +13,8 @@
> >   #include <sstream>
> >   #include <utility>
> >   
> > +#include "libcamera/internal/formats.h"
> > +
> >   /**
> >    * \file color_space.h
> >    * \brief Class and enums to represent color spaces
> > @@ -203,6 +205,105 @@ std::string ColorSpace::toString() const
> >   	return ss.str();
> >   }
> >   
> > +/**
> > + * \brief Adjust the color space to match a pixel format
> > + * \param[in] format The pixel format
> > + *
> > + * Not all combinations of pixel formats and color spaces make sense. For
> > + * instance, nobody uses a limited quantization range with raw Bayer formats,
> > + * and the YcbcrEncoding::None encoding isn't valid for YUV formats. This
> > + * function adjusts the ColorSpace to make it compatible with the given \a
> > + * format, by applying the following rules:
> > + *
> > + * - The color space for RAW formats must be Raw.
> > + * - The Y'CbCr encoding and quantization range for RGB formats must be
> > + *   YcbcrEncoding::None and Range::Full respectively.
> > + * - The Y'CbCr encoding for YUV formats must not be YcbcrEncoding::None. The
> > + *   best encoding is in that case guessed based on the primaries and transfer
> > + *   function.
> > + *
> > + * \return True if the color space has been adjusted, or false if it was
> > + * already compatible with the format and hasn't been changed
> > + */
> > +bool ColorSpace::adjust(PixelFormat format)
> 
> Ah, ,my version of Colorspace::adjust API was terrible :S

Not at all. I've based v2.1 on your work, the good outcome results from
collective work :-)

> v2.1 looks very good!
> 
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> > +{
> > +	const PixelFormatInfo &info = PixelFormatInfo::info(format);
> > +	bool adjusted = false;
> > +
> > +	switch (info.colourEncoding) {
> > +	case PixelFormatInfo::ColourEncodingRAW:
> > +		/* Raw formats must use the raw color space. */
> > +		if (*this != ColorSpace::Raw) {
> > +			*this = ColorSpace::Raw;
> > +			adjusted = true;
> > +		}
> > +		break;
> > +
> > +	case PixelFormatInfo::ColourEncodingRGB:
> > +		/*
> > +		 * RGB formats can't have a Y'CbCr encoding, and must use full
> > +		 * range quantization.
> > +		 */
> > +		if (ycbcrEncoding != YcbcrEncoding::None) {
> > +			ycbcrEncoding = YcbcrEncoding::None;
> > +			adjusted = true;
> > +		}
> > +
> > +		if (range != Range::Full) {
> > +			range = Range::Full;
> > +			adjusted = true;
> > +		}
> > +		break;
> > +
> > +	case PixelFormatInfo::ColourEncodingYUV:
> > +		if (ycbcrEncoding != YcbcrEncoding::None)
> > +			break;
> > +
> > +		/*
> > +		 * YUV formats must have a Y'CbCr encoding. Infer the most
> > +		 * probable option from the transfer function and primaries.
> > +		 */
> > +		switch (transferFunction) {
> > +		case TransferFunction::Linear:
> > +			/*
> > +			 * Linear YUV is not used in any standard color space,
> > +			 * pick the widely supported and used Rec601 as default.
> > +			 */
> > +			ycbcrEncoding = YcbcrEncoding::Rec601;
> > +			break;
> > +
> > +		case TransferFunction::Rec709:
> > +			switch (primaries) {
> > +			/* Raw should never happen. */
> > +			case Primaries::Raw:
> > +			case Primaries::Smpte170m:
> > +				ycbcrEncoding = YcbcrEncoding::Rec601;
> > +				break;
> > +			case Primaries::Rec709:
> > +				ycbcrEncoding = YcbcrEncoding::Rec709;
> > +				break;
> > +			case Primaries::Rec2020:
> > +				ycbcrEncoding = YcbcrEncoding::Rec2020;
> > +				break;
> > +			}
> > +			break;
> > +
> > +		case TransferFunction::Srgb:
> > +			/*
> > +			 * Only the sYCC color space uses the sRGB transfer
> > +			 * function, the corresponding encoding is Rec601.
> > +			 */
> > +			ycbcrEncoding = YcbcrEncoding::Rec601;
> > +			break;
> > +		}
> > +
> > +		adjusted = true;
> > +		break;
> > +	}
> > +
> > +	return adjusted;
> > +}
> > +
> >   /**
> >    * \brief Assemble and return a readable string representation of an
> >    * optional ColorSpace