[libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 25 00:37:05 CET 2021
Hi David,
On Thu, Nov 18, 2021 at 03:19:27PM +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 | 79 +++++++++++
> include/libcamera/meson.build | 1 +
> src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 4 files changed, 324 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..44ac077a
> --- /dev/null
> +++ b/include/libcamera/color_space.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.h - color space definitions
> + */
> +
> +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> +#define __LIBCAMERA_COLOR_SPACE_H__
You can now use #pragma once.
Kieran has sent a series to move the whole codebase to use it
> +
> +#include <optional>
> +#include <string>
> +
> +namespace libcamera {
> +
> +class ColorSpace
> +{
> +public:
> + enum class Primaries : int {
> + Raw,
> + Smpte170m,
> + Rec709,
> + Rec2020,
> + };
> +
> + enum class YcbcrEncoding : int {
> + Rec601,
> + Rec709,
> + Rec2020,
> + };
> +
> + enum class TransferFunction : int {
> + Linear,
> + Srgb,
> + Rec709,
> + };
> +
> + enum class Range : int {
> + Full,
> + Limited,
> + };
int or unsigned int ?
> +
> + constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> + : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> + {
> + }
> +
> + static const ColorSpace Raw;
> + static const ColorSpace Jpeg;
> + static const ColorSpace Srgb;
> + static const ColorSpace Smpte170m;
> + static const ColorSpace Rec709;
> + static const ColorSpace Rec2020;
> +
> + Primaries primaries;
> + YcbcrEncoding ycbcrEncoding;
> + TransferFunction transferFunction;
> + Range range;
> +
> + const std::string toString() const;
> + static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> +};
> +
> +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::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> +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 };
> +
> +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..1a5fc7a2
> --- /dev/null
> +++ b/src/libcamera/color_space.cpp
> @@ -0,0 +1,243 @@
> +/* 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>
Do you need to include <utility> for std::pair
> +
> +/**
> + * \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".
> + *
> + * In the strictest sense a "color space" formally only refers to the
> + * color primaries and white point. Here, however, the ColorSpace class
> + * adopts the common broader usage that includes the transfer function,
> + * Y'CbCr encoding method and quantisation.
> + *
> + * For more information on the specific color spaces described here, please
> + * see:
> + *
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
Please use links from html/latest/
> + */
> +
> +/**
> + * \enum ColorSpace::Primaries
> + * \brief The color primaries for this color space
> + *
I'm debated if this documentation should provide an overview of what
each component is or it is better to assume knowledges about that and
do not say anything.
On one side, we cannot cover the whole knowledge required, and we'll
end up repeating the existing probably. On the other side,
documentation will feel a bit incomplete for who has not previous
background. But I understand we cannot document the whole world
knowledge so I think it's fair assuming background, even if this is an
application facing API which application developers will have to deal
with.
> + * \var ColorSpace::Primaries::Raw
> + * \brief These are raw colors directly from a sensor
> + * \var ColorSpace::Primaries::Smpte170m
> + * \brief SMPTE 170M color primaries
What about a blank line between these \var+\brief entries ?
> + * \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
Same feeling I had before regarding documentation...
I suspect application developers might want to know more, as we are
going to 'force' them to care about color spaces..
> + *
> + * \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::Linear
> + * \brief This color space uses a linear (identity) transfer function
> + * \var ColorSpace::TransferFunction::Srgb
> + * \brief sRGB transfer function
> + * \var ColorSpace::TransferFunction::Rec709
> + * \brief Rec.709 transfer function
> + */
> +
> +/**
> + * \enum ColorSpace::Range
> + * \brief The range (sometimes "quantisation") for this color space
> + *
> + * \var ColorSpace::Range::Full
> + * \brief This color space uses full range pixel values
Does the 'full' reange changes with bitdepth as for Limited :)
> + * \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(Primaries p, Encoding e, TransferFunction t, Range r)
> + * \brief Construct a ColorSpace from explicit values
> + * \param[in] p The color primaries
> + * \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
> + */
> +
> +/**
> + * \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. */
> +
> + static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> + { ColorSpace::Raw, "Raw" },
> + { ColorSpace::Jpeg, "Jpeg" },
> + { ColorSpace::Srgb, "Srgb" },
> + { ColorSpace::Smpte170m, "Smpte170m" },
> + { ColorSpace::Rec709, "Rec709" },
> + { ColorSpace::Rec2020, "Rec2020" },
> + };
This really looks like a map :)
> + 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[] = {
> + "Raw",
> + "Smpte170m",
> + "Rec709",
> + "Rec2020",
> + };
I was expecting a warning caused by "converting a string constant to
char *"
> + static const char *encodingNames[] = {
> + "Rec601",
> + "Rec709",
> + "Rec2020",
> + };
> + static const char *transferFunctionNames[] = {
> + "Linear",
> + "Srgb",
> + "Rec709",
> + };
> + static const char *rangeNames[] = {
> + "Full",
> + "Limited",
> + };
This could be made a bit more C++-ish by using std::array and
std::string, but as far as I can tell using a container frowns up
making this a constexpr (soemthing you could do with raw arrays
though)
This is also slightly fragile as it requires care to maintain the vectors
and the enums in sync. Maps are probably more expensive in terms of
memory occupation but forces the association to be explicitely
maintained.
> +
> + 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();
> +}
> +
> +/**
> + * \brief Assemble and return a readable string representation of an
> + * optional ColorSpace
> + * \return A string describing the optional ColorSpace
> + */
> +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> +{
> + if (!colorSpace)
> + return "Unknown";
> +
> + return colorSpace->toString();
> +}
How do you immagine this to be used. Why would a caller use this
function on a !colorspace ? I would have ASSERT(colorspace) :)
> +
> +/**
> + * \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