[libcamera-devel] [PATCH v2.1 4/6] libcamera: color_space: Move color space adjustment to ColorSpace class
Umang Jain
umang.jain at ideasonboard.com
Fri Aug 26 11:15:47 CEST 2022
Hi Laurent,
Thank you for v2.1
On 8/25/22 9:36 PM, Laurent Pinchart wrote:
> From: Umang Jain <umang.jain at 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 at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at 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 at 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
More information about the libcamera-devel
mailing list