[libcamera-devel] [PATCH v2 1/3] libcamera: Add ColorSpace class

[Laurent Pinchart]

Hi David,

(CC'ing Hans, the colour space expert in V4L2)

Thank you for the patch.

Hans, if you have a bit of time to look at this, your feedback would be
appreciated.

On Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:
> This class represents a colour space by defining its YCbCr encoding,
> the transfer (gamma) function is uses, and whether the output is full

s/is/it/

> or limited range.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/color_space.h |  83 +++++++++++++
>  include/libcamera/meson.build   |   1 +
>  src/libcamera/color_space.cpp   | 207 ++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build       |   1 +
>  4 files changed, 292 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..9cd10503
> --- /dev/null
> +++ b/include/libcamera/color_space.h
> @@ -0,0 +1,83 @@
> +/* 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__
> +
> +#include <string>
> +
> +namespace libcamera {
> +
> +class ColorSpace
> +{
> +public:
> +	enum class Encoding : int {
> +		UNDEFINED,

We typically use CamelCase for enumerators.

> +		RAW,
> +		REC601,
> +		REC709,
> +		REC2020,
> +		VIDEO,
> +	};
> +
> +	enum class TransferFunction : int {
> +		UNDEFINED,
> +		IDENTITY,
> +		SRGB,
> +		REC709,
> +	};
> +
> +	enum class Range : int {
> +		UNDEFINED,
> +		FULL,
> +		LIMITED,
> +	};
> +
> +	constexpr ColorSpace(Encoding e, TransferFunction t, Range r)
> +		: encoding(e), transferFunction(t), range(r)
> +	{
> +	}
> +
> +	constexpr ColorSpace()
> +		: ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)

Line wrap ?

> +	{
> +	}

I'd move the default constructor furst.

> +
> +	static const ColorSpace UNDEFINED;
> +	static const ColorSpace RAW;
> +	static const ColorSpace JFIF;
> +	static const ColorSpace SMPTE170M;
> +	static const ColorSpace REC709;
> +	static const ColorSpace REC2020;
> +	static const ColorSpace VIDEO;

Shouldn't these be defined as static constexpr with a value here ?

> +
> +	Encoding encoding;
> +	TransferFunction transferFunction;
> +	Range range;
> +
> +	bool isFullyDefined() const;
> +
> +	const std::string toString() const;
> +};
> +
> +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };
> +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };
> +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };
> +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };
> +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };
> +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };
> +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };

This could then be dropped, or possibly moved (without the initializers)
to color_space.cpp if there's a need to bind a reference or take the
address of these variables.

> +
> +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 5b25ef84..7a8a04e5 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -3,6 +3,7 @@
>  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..d1ccb4cc
> --- /dev/null
> +++ b/src/libcamera/color_space.cpp
> @@ -0,0 +1,207 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.cpp - color spaces.
> + */
> +
> +#include <sstream>
> +
> +#include <libcamera/color_space.h>

This should go first, to ensure that the header file is self-contained.

> +
> +/**
> + * \file color_space.h
> + * \brief Class and enums to represent colour spaces.

No period at the end of briefs. Same below.

> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class ColorSpace
> + * \brief Class to describe a color space.
> + *
> + * The color space class defines the encodings of the color primaries, the

s/color space/ColorSpace/

You use both color and colour. For the class name, ColorSpace seems
right (well, it seems wrong to me, but it seems to be the right choice
:-)). For the text, we'll likely have to go through the documentation
one day to make everything consistent, and will unfortunately likely
have to pick the US spelling. This hasn't been completely decided yet,
so I've fine with either for now, but let's make it consistent within
the file.

> + * 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 "JFIF" or "REC709", though there is flexibility to leave some or all
> + * of them undefined too.
> + */
> +
> +/**
> + * \enum ColorSpace::Encoding
> + * \brief The encoding used for the color primaries.

Can we name this Primaries ? Encoding is strongly tied to the Y'CbCr
encoding from R'G'B' values, I think it would be confusing.

Now that I've written this, I realize (after reading the rest of the
series) that you're using this as the actual Y'CbCr encoding. The name
is thus fine, but it shouldn't mention colour primaries then, it should
be defined as Y'CbCr encoding. I would also move the definition of the
Encoding enum after TransferFunction, as that's the order in which
they're applied.

This leads me to the next question: what should we do with the colour
primaries (and white point) ? Those are part of the colour space
definition.

> + *
> + * \var ColorSpace::Encoding::UNDEFINED
> + * \brief The encoding for the colour primaries is not specified.

I think we need to include more usage information. Can an application
set the encoding to undefined ? What will happen then ? Can a camera
return undefined ? 

> + * \var ColorSpace::Encoding::RAW
> + * \brief These are raw colours from the sensor.

Will this also apply to RGB formats ?

> + * \var ColorSpace::Encoding::REC601
> + * \brief REC601 colour primaries.

s/REC601/Rec. 601/

or possibly "ITU-R Rec. 601" or "ITU-R Rec. BT.601" if we want to be
precise. Same below.

Should we also include the encoding formula explicitly, or would that be
too much details ?

> + * \var ColorSpace::Encoding::REC709
> + * \brief Rec709 colour primaries.
> + * \var ColorSpace::Encoding::REC2020
> + * \brief REC2020 colour primaries.
> + * \var ColorSpace::Encoding::VIDEO
> + * \brief A place-holder for video streams which will be resolved to one
> + * of REC601, REC709 or REC2020 once the video resolution is known.

We also need more information here. What's your expected use case for
this ? I'm concerned it may seem safe for applications to pick this as a
default, but they won't care much about colour spaces then, and will
likely get things wrong.

> + */
> +
> +/**
> + * \enum ColorSpace::TransferFunction
> + * \brief The transfer function used for this colour space.
> + *
> + * \var ColorSpace::TransferFunction::UNDEFINED
> + * \brief The transfer function is not specified.
> + * \var ColorSpace::TransferFunction::IDENTITY
> + * \brief This color space uses an identity transfer function.

Isn't "linear" a more common term than "identity" for the 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.

Technically speaking, the quantisation isn't part of the colour space,
but I fear it would bring lots of complexity and little gain to try and
keep it separate.

> + *
> + * \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.

Should we define these in more details as well, with the exact range ?

> + */
> +
> +/**
> + * \fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)
> + * \brief Construct a ColorSpace from explicit values
> + * \param[in] e The encoding for the color primaries
> + * \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 Return true if all the fields of the color space are defined, otherwise false.

Line wrap. You also need a \return, so I'd write \brief as "Check if
...".

> + */
> +bool ColorSpace::isFullyDefined() const
> +{
> +	return encoding != Encoding::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
> +{
> +	static const char *encodings[] = {
> +		"UNDEFINED",
> +		"RAW",
> +		"REC601",
> +		"REC709",
> +		"REC2020",
> +	};
> +	static const char *transferFunctions[] = {
> +		"UNDEFINED",
> +		"IDENTITY",
> +		"SRGB",
> +		"REC709",
> +	};
> +	static const char *ranges[] = {
> +		"UNDEFINED",
> +		"FULL",
> +		"LIMITED",
> +	};
> +
> +	std::stringstream ss;
> +	ss << std::string(encodings[static_cast<int>(encoding)]) << "+"

"+" seems a bit of a weird separator, I would have expected "/". Is
there a specific reason ?

> +	   << std::string(transferFunctions[static_cast<int>(transferFunction)]) << "+"
> +	   << std::string(ranges[static_cast<int>(range)]);
> +
> +	return ss.str();
> +}
> +
> +/**
> + * \var ColorSpace::encoding
> + * \brief The encoding of the color primaries
> + */
> +
> +/**
> + * \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::JFIF
> + * \brief A constant representing the JFIF color space usually used for
> + * encoding JPEG images.

Should this and the colour spaces below be defined more precisely in the
documentation, or do you think users can just look at the values of the
different members to figure out which is which ?

> + */
> +
> +/**
> + * \var ColorSpace::SMPTE170M
> + * \brief A constant representing the SMPTE170M color space (sometimes also
> + * referred to as "full range BT601").

Even though quantization is limited range ?

> + */
> +
> +/**
> + * \var ColorSpace::REC709
> + * \brief A constant representing the REC709 color space.
> + */
> +
> +/**
> + * \var ColorSpace::REC2020
> + * \brief A constant representing the REC2020 color space.
> + */
> +
> +/**
> + * \var ColorSpace::VIDEO
> + * \brief A constant that video streams can use to indicate the "default"
> + * color space for a video of this resolution, once that is is known. For
> + * exmample, SD streams would interpret this as SMPTE170M, HD streams as

s/exmample/example/

> + * REC709 and ultra HD as REC2020.

As mentioned above, this sounds a bit dangerous to me, but maybe I worry
too much. If we want to keep this, I think we need to make the
definition stricter, by documenting the required behaviour (thus
removing "for example", and defining SD and HD).

> + */
> +
> +/**
> + * \brief Compare color spaces for equality

 *
 * Color spaces are considered identical if they have the same
 * \ref transferFunction, \ref encoding and \ref range.
 *

> + * \return True if the two color spaces are identical, false otherwise
> + */
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> +	return lhs.encoding == rhs.encoding &&
> +	       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 bf82d38b..88dfbf24 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',

-- 
Regards,

Laurent Pinchart