[libcamera-devel] [PATCH v5 1/7] libcamera: Add ColorSpace class
Jacopo Mondi
jacopo at jmondi.org
Sat Nov 6 17:11:19 CET 2021
Hi David,
I'm sorry for getting here late and I praise your persistency in
pushing this forward.
I read all the previous version and I realized how ignorant was I
about color spaces and seeing that you had gone through them with both
Laurent and Hans I will limit my comments to the implementation
seeing that the underlying concepts have been discussed in detail.
On Thu, Nov 04, 2021 at 01:57:59PM +0000, David Plowman wrote:
> This class represents a color space by defining its color primaries,
> YCbCr encoding, the transfer (gamma) function it uses, and whether the
> output is full or limited range.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> include/libcamera/color_space.h | 88 +++++++++++
> include/libcamera/meson.build | 1 +
> src/libcamera/color_space.cpp | 257 ++++++++++++++++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 4 files changed, 347 insertions(+)
> create mode 100644 include/libcamera/color_space.h
> create mode 100644 src/libcamera/color_space.cpp
>
> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> new file mode 100644
> index 00000000..2af9da31
> --- /dev/null
> +++ b/include/libcamera/color_space.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.h - color space definitions
Nit: "Color space" :)
> + */
> +
> +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> +#define __LIBCAMERA_COLOR_SPACE_H__
> +
> +#include <string>
> +
> +namespace libcamera {
> +
> +class ColorSpace
> +{
> +public:
> + enum class Primaries : int {
> + Undefined,
> + Raw,
> + Smpte170m,
> + Rec709,
> + Rec2020,
> + };
> +
> + enum class YcbcrEncoding : int {
> + Undefined,
> + Rec601,
> + Rec709,
> + Rec2020,
> + };
> +
> + enum class TransferFunction : int {
> + Undefined,
> + Linear,
> + Srgb,
> + Rec709,
> + };
> +
> + enum class Range : int {
> + Undefined,
> + Full,
> + Limited,
> + };
> +
> + constexpr ColorSpace()
> + : ColorSpace(Primaries::Undefined, YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined)
> + {
> + }
> +
> + constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> + : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> + {
> + }
> +
> + static const ColorSpace Undefined;
> + static const ColorSpace Raw;
> + static const ColorSpace Jpeg;
> + static const ColorSpace Smpte170m;
> + static const ColorSpace Rec709;
> + static const ColorSpace Rec2020;
> +
> + Primaries primaries;
> + YcbcrEncoding ycbcrEncoding;
> + TransferFunction transferFunction;
> + Range range;
> +
> + bool isFullyDefined() const;
> +
> + const std::string toString() const;
> +};
Contradicting myself after just a few lines, I have a question on the
"concepts" :)
Do you expect applications to define their own ColorSpace by
assembling together the different components that define a color space
(Primaries, YcbcrEncoding, TransferFunction and Range) or do you
expect them to use the below defined ones only ?
> +
> +constexpr ColorSpace ColorSpace::Undefined = { Primaries::Undefined, YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined };
> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
Reading this
https://isocpp.org/blog/2018/05/quick-q-use-of-constexpr-in-header-file
my understanding is that each translation unit including color_space.h
will have its own copy of these instances.
Shouldn't you declare them as 'extern const' and define them in the
.cpp file as we do for controls, or since we're now with C++17 use the
'inline' keyword that allows you to define them in the header with
automatic external linkage ?
https://en.cppreference.com/w/cpp/language/inline
> +
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> + return !(lhs == rhs);
> +}
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 7155ff20..131e1740 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
> libcamera_public_headers = files([
> 'camera.h',
> 'camera_manager.h',
> + 'color_space.h',
> 'compiler.h',
> 'controls.h',
> 'file_descriptor.h',
> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> new file mode 100644
> index 00000000..dfd9fa1d
> --- /dev/null
> +++ b/src/libcamera/color_space.cpp
> @@ -0,0 +1,257 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.cpp - color spaces.
> + */
> +
> +#include <libcamera/color_space.h>
> +
> +#include <algorithm>
> +#include <sstream>
> +#include <vector>
> +
> +/**
> + * \file color_space.h
> + * \brief Class and enums to represent color spaces
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class ColorSpace
> + * \brief Class to describe a color space
> + *
> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> + * the transfer function associated with the color space, and the range
> + * (sometimes also referred to as the quantisation) of the color space.
> + *
> + * Certain combinations of these fields form well-known standard color
> + * spaces such as "JPEG" or "REC709". Applications must not request color
> + * spaces with undefined fields, but the "Undefined" value may be
> + * returned if the camera drivers decide to use a color space that is
> + * not recognised by the ColorSpace class.
> + *
> + * For more information on the specific color spaces described here, please
> + * see:
> + *
> + * <a href="https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-srgb">sRGB</a> and <a href="https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-jpeg">JPEG</a>
> +>* <a href="https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-smpte-170m">SMPTE 170M</a>
> + * <a href="https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-rec709">Rec.709</a>
> + * <a href="https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-bt2020">Rec.2020</a>
> + */
> +
> +/**
> + * \enum ColorSpace::Primaries
> + * \brief The color primaries for this color space
> + *
> + * \var ColorSpace::Primaries::Undefined
> + * \brief The color primaries are undefined
So "Undefined" is meant not to be used by application but only
returned in case libcamera is not capable of interpreting what's
reported by the driver. I'm tempted to say this should be repeated in
each xx::xx::Undefined definition, but I might just be paranoid.
Thinking out loud:
Depending on the expectations of how application should use ColorSpace
instances (create their own ones or just use the 'well known'
combinations) I wonder if we shouldn't restrict the visibility of
"Undefined" (by removing it :)
As you're using un-typed enums (plain enums) we could
just define the "ColorSpace::Undefined" instance with all members
initialized to -1 and in case one of the 4 components (primaries,
encoding, range and quantisation) is not recognized by libcamera
set StreamConfiguration.colorSpace = ColorSpace::Undefined.
> + * \var ColorSpace::Primaries::Raw
> + * \brief These are raw colors directly from a sensor
It surprises me to see this here, as all other standards define how
primaries are obtained from a standard illuminant, but this seems to
suggest "whatever the sensor produces". I assume this has been
discussed already, so I might just be missing something obvious.
> + * \var ColorSpace::Primaries::Smpte170m
> + * \brief SMPTE 170M color primaries
> + * \var ColorSpace::Primaries::Rec709
> + * \brief Rec.709 color primaries
> + * \var ColorSpace::Primaries::Rec2020
> + * \brief Rec.2020 color primaries
> + */
> +
> +/**
> + * \enum ColorSpace::YcbcrEncoding
> + * \brief The Y'CbCr encoding
> + *
> + * \var ColorSpace::YcbcrEncoding::Undefined
> + * \brief The Y'CbCr encoding is undefined
> + * \var ColorSpace::YcbcrEncoding::Rec601
> + * \brief Rec.601 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec709
> + * \brief Rec.709 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec2020
> + * \brief Rec.2020 Y'CbCr encoding
> + */
> +
> +/**
> + * \enum ColorSpace::TransferFunction
> + * \brief The transfer function used for this color space
> + *
> + * \var ColorSpace::TransferFunction::Undefined
> + * \brief The transfer function is not specified
> + * \var ColorSpace::TransferFunction::Linear
> + * \brief This color space uses a linear (identity) transfer function
> + * \var ColorSpace::TransferFunction::Srgb
> + * \brief sRGB transfer function
> + * \var ColorSpace::TransferFunction::Rec709
> + * \brief Rec709 transfer function
> + */
> +
> +/**
> + * \enum ColorSpace::Range
> + * \brief The range (sometimes "quantisation") for this color space
> + *
> + * \var ColorSpace::Range::Undefined
> + * \brief The range is not specified
> + * \var ColorSpace::Range::Full
> + * \brief This color space uses full range pixel values
> + * \var ColorSpace::Range::Limited
> + * \brief This color space uses limited range pixel values, being
> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> + */
> +
> +/**
> + * \fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)
> + * \brief Construct a ColorSpace from explicit values
> + * \param[in] e The Y'CbCr encoding
> + * \param[in] t The transfer function for the color space
> + * \param[in] r The range of the pixel values in this color space
> + */
> +
> +/**
> + * \fn ColorSpace::ColorSpace()
> + * \brief Construct a color space with undefined encoding, transfer function
> + * and range
> + */
> +
> +/**
> + * \brief Check if all the fields of the color space are defined
> + * \return Return true if all the fields of the color space are defined,
> + * otherwise false
> + */
> +bool ColorSpace::isFullyDefined() const
> +{
> + return primaries != Primaries::Undefined &&
> + ycbcrEncoding != YcbcrEncoding::Undefined &&
> + transferFunction != TransferFunction::Undefined &&
> + range != Range::Undefined;
> +}
> +
> +/**
> + * \brief Assemble and return a readable string representation of the
> + * ColorSpace
> + * \return A string describing the ColorSpace
> + */
> +const std::string ColorSpace::toString() const
> +{
> + /* Print out a brief name only for standard color sapces. */
spaces
But I think the comment can be dropped..
Thanks
j
> +
> + static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> + { ColorSpace::Undefined, "Undefined" },
> + { ColorSpace::Raw, "Raw" },
> + { ColorSpace::Jpeg, "Jpeg" },
> + { ColorSpace::Smpte170m, "Smpte170m" },
> + { ColorSpace::Rec709, "Rec709" },
> + { ColorSpace::Rec2020, "Rec2020" },
> + };
> + auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> + [this](const auto &item) {
> + return *this == item.first;
> + });
> + if (it != colorSpaceNames.end())
> + return std::string(it->second);
> +
> + static const char *primariesNames[] = {
> + "Undefined",
> + "Raw",
> + "Smpte170m",
> + "Rec709",
> + "Rec2020",
> + };
> + static const char *encodingNames[] = {
> + "Undefined",
> + "Rec601",
> + "Rec709",
> + "Rec2020",
> + };
> + static const char *transferFunctionNames[] = {
> + "Undefined",
> + "Linear",
> + "Srgb",
> + "Rec709",
> + };
> + static const char *rangeNames[] = {
> + "Undefined",
> + "Full",
> + "Limited",
> + };
> +
> + std::stringstream ss;
> + ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> + << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> + << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> + << std::string(rangeNames[static_cast<int>(range)]);
> +
> + return ss.str();
> +}
> +
> +/**
> + * \var ColorSpace::primaries
> + * \brief The color primaries
> + */
> +
> +/**
> + * \var ColorSpace::ycbcrEncoding
> + * \brief The Y'CbCr encoding
> + */
> +
> +/**
> + * \var ColorSpace::transferFunction
> + * \brief The transfer function for this color space
> + */
> +
> +/**
> + * \var ColorSpace::range
> + * \brief The pixel range used by this color space
> + */
> +
> +/**
> + * \var ColorSpace::Undefined
> + * \brief A constant representing a fully undefined color space
> + */
> +
> +/**
> + * \var ColorSpace::Raw
> + * \brief A constant representing a raw color space (from a sensor)
> + */
> +
> +/**
> + * \var ColorSpace::Jpeg
> + * \brief A constant representing the JPEG color space used for
> + * encoding JPEG images (and regarded as being the same as the sRGB
> + * color space)
> + */
> +
> +/**
> + * \var ColorSpace::Smpte170m
> + * \brief A constant representing the SMPTE170M color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec709
> + * \brief A constant representing the Rec.709 color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec2020
> + * \brief A constant representing the Rec.2020 color space
> + */
> +
> +/**
> + * \brief Compare color spaces for equality
> + * \return True if the two color spaces are identical, false otherwise
> + */
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> + return lhs.primaries == rhs.primaries &&
> + lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> + lhs.transferFunction == rhs.transferFunction &&
> + lhs.range == rhs.range;
> +}
> +
> +/**
> + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> + * \brief Compare color spaces for inequality
> + * \return True if the two color spaces are not identical, false otherwise
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6727a777..e7371d20 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -8,6 +8,7 @@ libcamera_sources = files([
> 'camera_manager.cpp',
> 'camera_sensor.cpp',
> 'camera_sensor_properties.cpp',
> + 'color_space.cpp',
> 'controls.cpp',
> 'control_serializer.cpp',
> 'control_validator.cpp',
> --
> 2.20.1
>
More information about the libcamera-devel
mailing list