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

13951 diff mbox series

This class represents a colour space by defining its YCbCr encoding,
the transfer (gamma) function is uses, and whether the output is full
or limited range.

Signed-off-by: David Plowman <david.plowman@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

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@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',

Hi Laurent

Thanks for the detailed review of all this. I probably won't comment
on some of the more "trivial" items (I'll just do a v3!) but some of
the points do indeed want some more discussion.

On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@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.

I tend to go American in the code and then UK in the text, but I can
go all-American, it's fine!

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

Yes, we do need a field for primaries, my bad for not including it. I
guess I'd imagined wrapping it up into the encoding as they often go
together, but that's not right.

Looking at the colour spaces we have in mind, I think we'll need
values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or "SMPTE-C"
for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.

I don't think we need to do anything about the white point, that's
determined for you once you have your primaries I believe.

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

Maybe some web links instead, to avoid duplication?

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

So the idea was that generateConfiguration would use VIDEO when the
stream role is "video", so that it could be resolved later once the
video resolution is known. That way, application code would almost
never have to deal with colour spaces because you'd do

configuration = camera->generateConfiguration(roles);
/* set up formats and resolutions *./
camera->configure(configuration);

and for JPEG, mpeg/h26x video it would all work out correctly. The
only case you'd have to worry about would be something like mjpeg,
where you'd have to set the colour space explicitly to JPEG/JFIF.

Note that we would have been able to leave it as UNDEFINED (rather
than inventing VIDEO) but for the fact that the configuration
generated doesn't record that it was intended for video.

Slightly unhappily, we will need to do the same with the (proposed)
Primaries field, as that too can change depending on the video
resolution. Or perhaps it would be better for a stream configuration
to remember its "role"? I could go with that.

>
> > + */
> > +
> > +/**
> > + * \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 ?

Yes, makes sense. Or possibly web links, will have a look!

>
> > + */
> > +
> > +/**
> > + * \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 ?

I shouldn't refer to SMPTE170M as a kind of "full range BT601", even
though rather lazily I sometimes think that. BT601 doesn't define
primaries, so it's misleading.

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

Or maybe store the stream role in the configuration...?

Let's continue the discussion, but in the meantime I'll put together a
v3 with most of the other points addressed.

Thanks!

David

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

Hi David,

On Mon, Oct 11, 2021 at 03:16:35PM +0100, David Plowman wrote:
> Hi Laurent
> 
> Thanks for the detailed review of all this. I probably won't comment
> on some of the more "trivial" items (I'll just do a v3!) but some of
> the points do indeed want some more discussion.
> 
> On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart wrote:
> >
> > 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@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.
> 
> I tend to go American in the code and then UK in the text, but I can
> go all-American, it's fine!

My point was that the text mixes color and colour. I'm fine if you use
the US spelling in code and UK in documentation for now (even though we
will likely move everything to the US spelling at some point, to my
dismay), as long as the documentation is consistent with itself.

> > > + * 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.
> 
> Yes, we do need a field for primaries, my bad for not including it. I
> guess I'd imagined wrapping it up into the encoding as they often go
> together, but that's not right.
> 
> Looking at the colour spaces we have in mind, I think we'll need
> values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or "SMPTE-C"
> for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.
> 
> I don't think we need to do anything about the white point, that's
> determined for you once you have your primaries I believe.

I'm not sure if the white point is a direct result of the chromaticity
coordinates of the primaries, but it's defined in the above
specifications, so if we go for presets it doesn't need to be handled
separately.

> > > + *
> > > + * \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 ?
> 
> Maybe some web links instead, to avoid duplication?

If you can find web links that are standard and stable enough, that's
fine with me.

> > > + * \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.
> 
> So the idea was that generateConfiguration would use VIDEO when the
> stream role is "video", so that it could be resolved later once the
> video resolution is known. That way, application code would almost
> never have to deal with colour spaces because you'd do
> 
> configuration = camera->generateConfiguration(roles);
> /* set up formats and resolutions *./
> camera->configure(configuration);
> 
> and for JPEG, mpeg/h26x video it would all work out correctly. The
> only case you'd have to worry about would be something like mjpeg,
> where you'd have to set the colour space explicitly to JPEG/JFIF.
> 
> Note that we would have been able to leave it as UNDEFINED (rather
> than inventing VIDEO) but for the fact that the configuration
> generated doesn't record that it was intended for video.
> 
> Slightly unhappily, we will need to do the same with the (proposed)
> Primaries field, as that too can change depending on the video
> resolution. Or perhaps it would be better for a stream configuration
> to remember its "role"? I could go with that.

Roles were not meant to be remembered, and may actually go away in their
current form, so I'd rather not rely on them.

One part that bothers me a bit is that the VIDEO encoding needs to be
resolved by pipeline handlers, which will end up copying each other,
with their own little bugs of course. Could we try to at least
centralize that ?

> > > + */
> > > +
> > > +/**
> > > + * \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 ?
> 
> Yes, makes sense. Or possibly web links, will have a look!
> 
> > > + */
> > > +
> > > +/**
> > > + * \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 ?
> 
> I shouldn't refer to SMPTE170M as a kind of "full range BT601", even
> though rather lazily I sometimes think that. BT601 doesn't define
> primaries, so it's misleading.
> 
> > > + */
> > > +
> > > +/**
> > > + * \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).
> 
> Or maybe store the stream role in the configuration...?
> 
> Let's continue the discussion, but in the meantime I'll put together a
> v3 with most of the other points addressed.
> 
> > > + */
> > > +
> > > +/**
> > > + * \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',

Hi again Laurent

On Mon, 11 Oct 2021 at 15:31, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> On Mon, Oct 11, 2021 at 03:16:35PM +0100, David Plowman wrote:
> > Hi Laurent
> >
> > Thanks for the detailed review of all this. I probably won't comment
> > on some of the more "trivial" items (I'll just do a v3!) but some of
> > the points do indeed want some more discussion.
> >
> > On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart wrote:
> > >
> > > 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@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.
> >
> > I tend to go American in the code and then UK in the text, but I can
> > go all-American, it's fine!
>
> My point was that the text mixes color and colour. I'm fine if you use
> the US spelling in code and UK in documentation for now (even though we
> will likely move everything to the US spelling at some point, to my
> dismay), as long as the documentation is consistent with itself.
>
> > > > + * 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.
> >
> > Yes, we do need a field for primaries, my bad for not including it. I
> > guess I'd imagined wrapping it up into the encoding as they often go
> > together, but that's not right.
> >
> > Looking at the colour spaces we have in mind, I think we'll need
> > values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or "SMPTE-C"
> > for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.
> >
> > I don't think we need to do anything about the white point, that's
> > determined for you once you have your primaries I believe.
>
> I'm not sure if the white point is a direct result of the chromaticity
> coordinates of the primaries, but it's defined in the above
> specifications, so if we go for presets it doesn't need to be handled
> separately.

In general we have a 3x3 RGB to XYZ matrix (for example
http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html). The
primaries are what we get when we apply that matrix to red (1,0,0),
green (0,1,0) and blue (0,0,1). So the result of applying the matrix
to (1,1,1) is now pre-determined, and is the white point. Someone
please correct me if I understood that less well than I thought! :)

But I think going with presets is the thing to do, trying to do
on-the-fly conversions of CCMs to arbitrary colour spaces sounds quite
fraught... (and we certainly would want to opt out of implementing
anything like that)

>
> > > > + *
> > > > + * \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 ?
> >
> > Maybe some web links instead, to avoid duplication?
>
> If you can find web links that are standard and stable enough, that's
> fine with me.
>
> > > > + * \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.
> >
> > So the idea was that generateConfiguration would use VIDEO when the
> > stream role is "video", so that it could be resolved later once the
> > video resolution is known. That way, application code would almost
> > never have to deal with colour spaces because you'd do
> >
> > configuration = camera->generateConfiguration(roles);
> > /* set up formats and resolutions *./
> > camera->configure(configuration);
> >
> > and for JPEG, mpeg/h26x video it would all work out correctly. The
> > only case you'd have to worry about would be something like mjpeg,
> > where you'd have to set the colour space explicitly to JPEG/JFIF.
> >
> > Note that we would have been able to leave it as UNDEFINED (rather
> > than inventing VIDEO) but for the fact that the configuration
> > generated doesn't record that it was intended for video.
> >
> > Slightly unhappily, we will need to do the same with the (proposed)
> > Primaries field, as that too can change depending on the video
> > resolution. Or perhaps it would be better for a stream configuration
> > to remember its "role"? I could go with that.
>
> Roles were not meant to be remembered, and may actually go away in their
> current form, so I'd rather not rely on them.
>
> One part that bothers me a bit is that the VIDEO encoding needs to be
> resolved by pipeline handlers, which will end up copying each other,
> with their own little bugs of course. Could we try to at least
> centralize that ?

Ah OK, didn't realise that roles might be changing. So what did you
have in mind here, maybe a PipelineHandler base class method that you
can run at the start of validate() which patches up any colour space
fields according to the stream's output resolution (if marked as
"VIDEO")?

David

>
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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 ?
> >
> > Yes, makes sense. Or possibly web links, will have a look!
> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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 ?
> >
> > I shouldn't refer to SMPTE170M as a kind of "full range BT601", even
> > though rather lazily I sometimes think that. BT601 doesn't define
> > primaries, so it's misleading.
> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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).
> >
> > Or maybe store the stream role in the configuration...?
> >
> > Let's continue the discussion, but in the meantime I'll put together a
> > v3 with most of the other points addressed.
> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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

Hi Daniel, Laurent,

Some comments:

On 11/10/2021 16:16, David Plowman wrote:
> Hi Laurent
> 
> Thanks for the detailed review of all this. I probably won't comment
> on some of the more "trivial" items (I'll just do a v3!) but some of
> the points do indeed want some more discussion.
> 
> On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> 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@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 {

You probably want to be explicit and call this YCbCrEncoding, since this only applies to YCbCr
pixel encodings.

>>> +             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;

JFIF (aka JPEG) is not a colorspace, it is a container. Effectively this is equivalent to sRGB, storing the pixels as YCbCr using Bt.601
encoding and full range. I would not set it as a separate colorspace.

>>> +     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.
> 
> I tend to go American in the code and then UK in the text, but I can
> go all-American, it's fine!
> 
>>
>>> + * 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.
> 
> Yes, we do need a field for primaries, my bad for not including it. I
> guess I'd imagined wrapping it up into the encoding as they often go
> together, but that's not right.
> 
> Looking at the colour spaces we have in mind, I think we'll need
> values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or "SMPTE-C"
> for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.
> 
> I don't think we need to do anything about the white point, that's
> determined for you once you have your primaries I believe.

The white point is part of these standards. So Rec.709 defines both the
primaries and the white point. It's true for all of these.

BTW, sections 2.15-2.18 are very useful material to read:

https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt.html

There are four 'parameters' that define how to interpret color values:

- the colorspace, aka chromaticities (RGB primaries + whitepoint)
- the transfer function
- the quantization range
- Y'CbCr encoding (only for Y'CbCr, of course). There is also a HSV encoding
  if the pixels are stored using HSV instead of Y'CbCr, but that's not relevant
  here, I think.

Most colorspace standards define these four 'parameters', but APIs should
really treat them as independent parameters.

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

Since Encoding is what I would call the Y'CbCr encoding, 'UNDEFINED' can
be used for RGB, but not for Y'CbCr: there you always have to know the
encoding used.

>>
>>> + * \var ColorSpace::Encoding::RAW
>>> + * \brief These are raw colours from the sensor.
>>
>> Will this also apply to RGB formats ?

This makes no sense for Y'CbCr encoding.

>>
>>> + * \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 ?
> 
> Maybe some web links instead, to avoid duplication?

You can refer to the V4L2 colorspace sections.

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

Don't do this. If you don't know the Y'CbCr encoding yet, then just pick
something and update it once you do know it.

The Y'CbCr encoding is in principle independent of the resolution, although for
some interfaces (HDMI) it can be tied to the resolution, but it doesn't have to.

>>
>> 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.
> 
> So the idea was that generateConfiguration would use VIDEO when the
> stream role is "video", so that it could be resolved later once the
> video resolution is known. That way, application code would almost
> never have to deal with colour spaces because you'd do
> 
> configuration = camera->generateConfiguration(roles);
> /* set up formats and resolutions *./
> camera->configure(configuration);
> 
> and for JPEG, mpeg/h26x video it would all work out correctly. The
> only case you'd have to worry about would be something like mjpeg,
> where you'd have to set the colour space explicitly to JPEG/JFIF.
> 
> Note that we would have been able to leave it as UNDEFINED (rather
> than inventing VIDEO) but for the fact that the configuration
> generated doesn't record that it was intended for video.
> 
> Slightly unhappily, we will need to do the same with the (proposed)
> Primaries field, as that too can change depending on the video
> resolution. Or perhaps it would be better for a stream configuration
> to remember its "role"? I could go with that.

You can do something similar to what V4L2 does, add 'DEFAULT' and
deduce the corresponding parameter based on the other information.
See e.g. V4L2_MAP_COLORSPACE_DEFAULT et al in videodev2.h.

> 
>>
>>> + */
>>> +
>>> +/**
>>> + * \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 ?

Yes. The transfer function is not a matrix, but a non-linear function.
So 'identity' is not really a correct term. V4L2 just calls it V4L2_XFER_FUNC_NONE,
i.e. there is no 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.

'UNDEFINED' is meaningless. 'DEFAULT' can make sense (see V4L2_MAP_QUANTIZATION_DEFAULT),
but if you don't know the range, then you can't interpret the data.

Using the wrong quantization range is very visible to the end user, so this must
be right.

>>> + * \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 ?
> 
> Yes, makes sense. Or possibly web links, will have a look!

Again, refer to the V4L2 spec. Easiest resource for this.

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

'full range BT601' makes no sense whatsoever in this context. This is simply wrong.
It *is* sometimes (incorrectly) referred to as BT601. I'd drop this bit completely,
though.

>>
>> Even though quantization is limited range ?
> 
> I shouldn't refer to SMPTE170M as a kind of "full range BT601", even
> though rather lazily I sometimes think that. BT601 doesn't define
> primaries, so it's misleading.
> 
>>
>>> + */
>>> +
>>> +/**
>>> + * \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.

UHD can be either REC709 or REC2020. And if it is HDR then UHD would use the SMPTE
2084 transfer function instead of REC709.

Regards,

	Hans

>>
>> 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).
> 
> Or maybe store the stream role in the configuration...?

I would really drop both UNDEFINED and VIDEO, and instead use a DEFAULT enum.
That will make an educated guess.

Regards,

	Hans

> 
> Let's continue the discussion, but in the meantime I'll put together a
> v3 with most of the other points addressed.
> 
> Thanks!
> 
> David
> 
>>
>>> + */
>>> +
>>> +/**
>>> + * \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

Hi Hans, everyone

Thank you for the detailed review, I think that's very helpful.
Obviously I'm working on a v3 at the moment where I can adopt most of
these suggestions.

I like the idea of DEFAULT, rather than VIDEO. UNDEFINED is indeed
undesirable, though I wonder if it lives on purely as a return value
in case V4L2 changes the colour space to one that we haven't defined.
(Obviously we could define them all, but I'm not sure about that, and
of course there might in future one day be things other than V4L2 as
well. Opinions always welcome!)

Laurent -

A few little points about what else I'm planning for v3:

* I need to add colour spaces to the subdev format. I think that means
v4l2ToColorSpace and colorSpaceToV4l2 methods in the base V4L2Device
class.

* All pipeline handlers will change any "DEFAULT" non-raw streams to
the "best choice" for video, according to the resolution. Raw streams
will be changed to ColorSpace::Raw. (Changing a DEFAULT value will not
count as an adjustment.)

* I'll write a CameraConfiguration::updateColorSpaces function to do
this, and make all pipeline handlers call it at the start of
validate(). Optionally it will also force non-raw streams to the same
colour space.

* The rule will be that the V4L2Device functions will never get passed
anything "DEFAULT", and *absolutely never* anything "UNDEFINED". I
think both should be regarded as errors. (But UNDEFINED can be
returned, as above.) Generally I take the view that colour space
problems really need to hit you in the face, so to speak, otherwise
it's terribly easy to miss them... until after your customer is
shipping a product :)

Does that make sense?

Thanks everyone!
David

On Tue, 12 Oct 2021 at 12:44, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Hi Daniel, Laurent,
>
> Some comments:
>
> On 11/10/2021 16:16, David Plowman wrote:
> > Hi Laurent
> >
> > Thanks for the detailed review of all this. I probably won't comment
> > on some of the more "trivial" items (I'll just do a v3!) but some of
> > the points do indeed want some more discussion.
> >
> > On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> 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@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 {
>
> You probably want to be explicit and call this YCbCrEncoding, since this only applies to YCbCr
> pixel encodings.
>
> >>> +             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;
>
> JFIF (aka JPEG) is not a colorspace, it is a container. Effectively this is equivalent to sRGB, storing the pixels as YCbCr using Bt.601
> encoding and full range. I would not set it as a separate colorspace.
>
> >>> +     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.
> >
> > I tend to go American in the code and then UK in the text, but I can
> > go all-American, it's fine!
> >
> >>
> >>> + * 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.
> >
> > Yes, we do need a field for primaries, my bad for not including it. I
> > guess I'd imagined wrapping it up into the encoding as they often go
> > together, but that's not right.
> >
> > Looking at the colour spaces we have in mind, I think we'll need
> > values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or "SMPTE-C"
> > for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.
> >
> > I don't think we need to do anything about the white point, that's
> > determined for you once you have your primaries I believe.
>
> The white point is part of these standards. So Rec.709 defines both the
> primaries and the white point. It's true for all of these.
>
> BTW, sections 2.15-2.18 are very useful material to read:
>
> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt.html
>
> There are four 'parameters' that define how to interpret color values:
>
> - the colorspace, aka chromaticities (RGB primaries + whitepoint)
> - the transfer function
> - the quantization range
> - Y'CbCr encoding (only for Y'CbCr, of course). There is also a HSV encoding
>   if the pixels are stored using HSV instead of Y'CbCr, but that's not relevant
>   here, I think.
>
> Most colorspace standards define these four 'parameters', but APIs should
> really treat them as independent parameters.
>
> >
> >>
> >>> + *
> >>> + * \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 ?
>
> Since Encoding is what I would call the Y'CbCr encoding, 'UNDEFINED' can
> be used for RGB, but not for Y'CbCr: there you always have to know the
> encoding used.
>
> >>
> >>> + * \var ColorSpace::Encoding::RAW
> >>> + * \brief These are raw colours from the sensor.
> >>
> >> Will this also apply to RGB formats ?
>
> This makes no sense for Y'CbCr encoding.
>
> >>
> >>> + * \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 ?
> >
> > Maybe some web links instead, to avoid duplication?
>
> You can refer to the V4L2 colorspace sections.
>
> >
> >>
> >>> + * \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.
>
> Don't do this. If you don't know the Y'CbCr encoding yet, then just pick
> something and update it once you do know it.
>
> The Y'CbCr encoding is in principle independent of the resolution, although for
> some interfaces (HDMI) it can be tied to the resolution, but it doesn't have to.
>
> >>
> >> 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.
> >
> > So the idea was that generateConfiguration would use VIDEO when the
> > stream role is "video", so that it could be resolved later once the
> > video resolution is known. That way, application code would almost
> > never have to deal with colour spaces because you'd do
> >
> > configuration = camera->generateConfiguration(roles);
> > /* set up formats and resolutions *./
> > camera->configure(configuration);
> >
> > and for JPEG, mpeg/h26x video it would all work out correctly. The
> > only case you'd have to worry about would be something like mjpeg,
> > where you'd have to set the colour space explicitly to JPEG/JFIF.
> >
> > Note that we would have been able to leave it as UNDEFINED (rather
> > than inventing VIDEO) but for the fact that the configuration
> > generated doesn't record that it was intended for video.
> >
> > Slightly unhappily, we will need to do the same with the (proposed)
> > Primaries field, as that too can change depending on the video
> > resolution. Or perhaps it would be better for a stream configuration
> > to remember its "role"? I could go with that.
>
> You can do something similar to what V4L2 does, add 'DEFAULT' and
> deduce the corresponding parameter based on the other information.
> See e.g. V4L2_MAP_COLORSPACE_DEFAULT et al in videodev2.h.
>
> >
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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 ?
>
> Yes. The transfer function is not a matrix, but a non-linear function.
> So 'identity' is not really a correct term. V4L2 just calls it V4L2_XFER_FUNC_NONE,
> i.e. there is no 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.
>
> 'UNDEFINED' is meaningless. 'DEFAULT' can make sense (see V4L2_MAP_QUANTIZATION_DEFAULT),
> but if you don't know the range, then you can't interpret the data.
>
> Using the wrong quantization range is very visible to the end user, so this must
> be right.
>
> >>> + * \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 ?
> >
> > Yes, makes sense. Or possibly web links, will have a look!
>
> Again, refer to the V4L2 spec. Easiest resource for this.
>
> >
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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").
>
> 'full range BT601' makes no sense whatsoever in this context. This is simply wrong.
> It *is* sometimes (incorrectly) referred to as BT601. I'd drop this bit completely,
> though.
>
> >>
> >> Even though quantization is limited range ?
> >
> > I shouldn't refer to SMPTE170M as a kind of "full range BT601", even
> > though rather lazily I sometimes think that. BT601 doesn't define
> > primaries, so it's misleading.
> >
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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.
>
> UHD can be either REC709 or REC2020. And if it is HDR then UHD would use the SMPTE
> 2084 transfer function instead of REC709.
>
> Regards,
>
>         Hans
>
> >>
> >> 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).
> >
> > Or maybe store the stream role in the configuration...?
>
> I would really drop both UNDEFINED and VIDEO, and instead use a DEFAULT enum.
> That will make an educated guess.
>
> Regards,
>
>         Hans
>
> >
> > Let's continue the discussion, but in the meantime I'll put together a
> > v3 with most of the other points addressed.
> >
> > Thanks!
> >
> > David
> >
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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
>