[libcamera-devel] [PATCH v2.1 4/6] libcamera: color_space: Move color space adjustment to ColorSpace class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 26 12:32:24 CEST 2022
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 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
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 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list