[libcamera-devel,v6,1/7] libcamera: Add ColorSpace class

14626 diff mbox series

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

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/color_space.h |  79 +++++++++++
 include/libcamera/meson.build   |   1 +
 src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
 src/libcamera/meson.build       |   1 +
 4 files changed, 324 insertions(+)
 create mode 100644 include/libcamera/color_space.h
 create mode 100644 src/libcamera/color_space.cpp

Hi David,

Thank you for the patch.

On 11/18/21 8:49 PM, David Plowman wrote:
> This class represents a color space by defining its color primaries,
> YCbCr encoding, the transfer (gamma) function it uses, and whether the
> output is full or limited range.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>   include/libcamera/color_space.h |  79 +++++++++++
>   include/libcamera/meson.build   |   1 +
>   src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
>   src/libcamera/meson.build       |   1 +
>   4 files changed, 324 insertions(+)
>   create mode 100644 include/libcamera/color_space.h
>   create mode 100644 src/libcamera/color_space.cpp
>
> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> new file mode 100644
> index 00000000..44ac077a
> --- /dev/null
> +++ b/include/libcamera/color_space.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.h - color space definitions
> + */
> +
> +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> +#define __LIBCAMERA_COLOR_SPACE_H__
> +
> +#include <optional>
> +#include <string>
> +
> +namespace libcamera {
> +
> +class ColorSpace
> +{
> +public:
> +	enum class Primaries : int {
> +		Raw,
> +		Smpte170m,
> +		Rec709,
> +		Rec2020,
> +	};
> +
> +	enum class YcbcrEncoding : int {
> +		Rec601,
> +		Rec709,
> +		Rec2020,
> +	};
> +
> +	enum class TransferFunction : int {
> +		Linear,
> +		Srgb,
> +		Rec709,
> +	};
> +
> +	enum class Range : int {
> +		Full,
> +		Limited,
> +	};
> +
> +	constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> +		: primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> +	{
> +	}
> +
> +	static const ColorSpace Raw;
> +	static const ColorSpace Jpeg;
> +	static const ColorSpace Srgb;
> +	static const ColorSpace Smpte170m;
> +	static const ColorSpace Rec709;
> +	static const ColorSpace Rec2020;
> +
> +	Primaries primaries;
> +	YcbcrEncoding ycbcrEncoding;
> +	TransferFunction transferFunction;
> +	Range range;
> +
> +	const std::string toString() const;
> +	static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> +};
> +
> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
> +
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 7155ff20..131e1740 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
>   libcamera_public_headers = files([
>       'camera.h',
>       'camera_manager.h',
> +    'color_space.h',
>       'compiler.h',
>       'controls.h',
>       'file_descriptor.h',
> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> new file mode 100644
> index 00000000..1a5fc7a2
> --- /dev/null
> +++ b/src/libcamera/color_space.cpp
> @@ -0,0 +1,243 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.cpp - color spaces.
> + */
> +
> +#include <libcamera/color_space.h>
> +
> +#include <algorithm>
> +#include <sstream>
> +#include <vector>
> +
> +/**
> + * \file color_space.h
> + * \brief Class and enums to represent color spaces
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class ColorSpace
> + * \brief Class to describe a color space
> + *
> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> + * the transfer function associated with the color space, and the range
> + * (sometimes also referred to as the quantisation) of the color space.
> + *
> + * Certain combinations of these fields form well-known standard color
> + * spaces such as "JPEG" or "REC709".
> + *
> + * In the strictest sense a "color space" formally only refers to the
> + * color primaries and white point. Here, however, the ColorSpace class
> + * adopts the common broader usage that includes the transfer function,
> + * Y'CbCr encoding method and quantisation.
> + *
> + * For more information on the specific color spaces described here, please
> + * see:
> + *
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
> + */
> +
> +/**
> + * \enum ColorSpace::Primaries
> + * \brief The color primaries for this color space
> + *
> + * \var ColorSpace::Primaries::Raw
> + * \brief These are raw colors directly from a sensor
> + * \var ColorSpace::Primaries::Smpte170m
> + * \brief SMPTE 170M color primaries
> + * \var ColorSpace::Primaries::Rec709
> + * \brief Rec.709 color primaries
> + * \var ColorSpace::Primaries::Rec2020
> + * \brief Rec.2020 color primaries
> + */
> +
> +/**
> + * \enum ColorSpace::YcbcrEncoding
> + * \brief The Y'CbCr encoding
> + *
> + * \var ColorSpace::YcbcrEncoding::Rec601
> + * \brief Rec.601 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec709
> + * \brief Rec.709 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec2020
> + * \brief Rec.2020 Y'CbCr encoding
> + */
> +
> +/**
> + * \enum ColorSpace::TransferFunction
> + * \brief The transfer function used for this color space
> + *
> + * \var ColorSpace::TransferFunction::Linear
> + * \brief This color space uses a linear (identity) transfer function
> + * \var ColorSpace::TransferFunction::Srgb
> + * \brief sRGB transfer function
> + * \var ColorSpace::TransferFunction::Rec709
> + * \brief Rec.709 transfer function
> + */
> +
> +/**
> + * \enum ColorSpace::Range
> + * \brief The range (sometimes "quantisation") for this color space
> + *
> + * \var ColorSpace::Range::Full
> + * \brief This color space uses full range pixel values
> + * \var ColorSpace::Range::Limited
> + * \brief This color space uses limited range pixel values, being
> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> + */
> +
> +/**
> + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)
> + * \brief Construct a ColorSpace from explicit values
> + * \param[in] p The color primaries
> + * \param[in] e The Y'CbCr encoding
> + * \param[in] t The transfer function for the color space
> + * \param[in] r The range of the pixel values in this color space
> + */
> +
> +/**
> + * \brief Assemble and return a readable string representation of the
> + * ColorSpace
> + * \return A string describing the ColorSpace
> + */
> +const std::string ColorSpace::toString() const
> +{
> +	/* Print out a brief name only for standard color sapces. */


s/sapces/spaces

> +
> +	static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> +		{ ColorSpace::Raw, "Raw" },
> +		{ ColorSpace::Jpeg, "Jpeg" },
> +		{ ColorSpace::Srgb, "Srgb" },
> +		{ ColorSpace::Smpte170m, "Smpte170m" },
> +		{ ColorSpace::Rec709, "Rec709" },
> +		{ ColorSpace::Rec2020, "Rec2020" },
> +	};


Should we maintain this class-wide in its definition instead?

> +	auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> +			       [this](const auto &item) {
> +				       return *this == item.first;
> +			       });
> +	if (it != colorSpaceNames.end())
> +		return std::string(it->second);
> +


I am not sure I follow this code-path.

If we cannot find a winner from colorSpaceNames vector, we return a 
bunch of color-spcae specific information below? Can you please cover 
this here (via a comment) and in doxygen documentation above of the 
function?

> +	static const char *primariesNames[] = {
> +		"Raw",
> +		"Smpte170m",
> +		"Rec709",
> +		"Rec2020",
> +	};
> +	static const char *encodingNames[] = {
> +		"Rec601",
> +		"Rec709",
> +		"Rec2020",
> +	};
> +	static const char *transferFunctionNames[] = {
> +		"Linear",
> +		"Srgb",
> +		"Rec709",
> +	};
> +	static const char *rangeNames[] = {
> +		"Full",
> +		"Limited",
> +	};
> +
> +	std::stringstream ss;
> +	ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> +	   << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> +	   << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> +	   << std::string(rangeNames[static_cast<int>(range)]);
> +
> +	return ss.str();
> +}
> +
> +/**
> + * \brief Assemble and return a readable string representation of an
> + * optional ColorSpace
> + * \return A string describing the optional ColorSpace
> + */
> +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> +{
> +	if (!colorSpace)
> +		return "Unknown";
> +
> +	return colorSpace->toString();
> +}
> +
> +/**
> + * \var ColorSpace::primaries
> + * \brief The color primaries
> + */
> +
> +/**
> + * \var ColorSpace::ycbcrEncoding
> + * \brief The Y'CbCr encoding
> + */
> +
> +/**
> + * \var ColorSpace::transferFunction
> + * \brief The transfer function for this color space
> + */
> +
> +/**
> + * \var ColorSpace::range
> + * \brief The pixel range used by this color space
> + */
> +
> +/**
> + * \var ColorSpace::Undefined
> + * \brief A constant representing a fully undefined color space
> + */
> +
> +/**
> + * \var ColorSpace::Raw
> + * \brief A constant representing a raw color space (from a sensor)
> + */
> +
> +/**
> + * \var ColorSpace::Jpeg
> + * \brief A constant representing the JPEG color space used for
> + * encoding JPEG images (and regarded as being the same as the sRGB
> + * color space)
> + */
> +
> +/**
> + * \var ColorSpace::Smpte170m
> + * \brief A constant representing the SMPTE170M color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec709
> + * \brief A constant representing the Rec.709 color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec2020
> + * \brief A constant representing the Rec.2020 color space
> + */
> +
> +/**
> + * \brief Compare color spaces for equality
> + * \return True if the two color spaces are identical, false otherwise
> + */
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> +	return lhs.primaries == rhs.primaries &&
> +	       lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> +	       lhs.transferFunction == rhs.transferFunction &&
> +	       lhs.range == rhs.range;
> +}
> +
> +/**
> + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> + * \brief Compare color spaces for inequality
> + * \return True if the two color spaces are not identical, false otherwise
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6727a777..e7371d20 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -8,6 +8,7 @@ libcamera_sources = files([
>       'camera_manager.cpp',
>       'camera_sensor.cpp',
>       'camera_sensor_properties.cpp',
> +    'color_space.cpp',
>       'controls.cpp',
>       'control_serializer.cpp',
>       'control_validator.cpp',

Hi,

On 11/23/21 12:50 PM, Umang Jain wrote:
> Hi David,
>
> Thank you for the patch.
>
> On 11/18/21 8:49 PM, David Plowman wrote:
>> This class represents a color space by defining its color primaries,
>> YCbCr encoding, the transfer (gamma) function it uses, and whether the
>> output is full or limited range.
>>
>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>> ---
>>   include/libcamera/color_space.h |  79 +++++++++++
>>   include/libcamera/meson.build   |   1 +
>>   src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
>>   src/libcamera/meson.build       |   1 +
>>   4 files changed, 324 insertions(+)
>>   create mode 100644 include/libcamera/color_space.h
>>   create mode 100644 src/libcamera/color_space.cpp
>>
>> diff --git a/include/libcamera/color_space.h 
>> b/include/libcamera/color_space.h
>> new file mode 100644
>> index 00000000..44ac077a
>> --- /dev/null
>> +++ b/include/libcamera/color_space.h
>> @@ -0,0 +1,79 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
>> + *
>> + * color_space.h - color space definitions
>> + */
>> +
>> +#ifndef __LIBCAMERA_COLOR_SPACE_H__
>> +#define __LIBCAMERA_COLOR_SPACE_H__
>> +
>> +#include <optional>
>> +#include <string>
>> +
>> +namespace libcamera {
>> +
>> +class ColorSpace
>> +{
>> +public:
>> +    enum class Primaries : int {
>> +        Raw,
>> +        Smpte170m,
>> +        Rec709,
>> +        Rec2020,
>> +    };
>> +
>> +    enum class YcbcrEncoding : int {
>> +        Rec601,
>> +        Rec709,
>> +        Rec2020,
>> +    };
>> +
>> +    enum class TransferFunction : int {
>> +        Linear,
>> +        Srgb,
>> +        Rec709,
>> +    };
>> +
>> +    enum class Range : int {
>> +        Full,
>> +        Limited,
>> +    };
>> +
>> +    constexpr ColorSpace(Primaries p, YcbcrEncoding e, 
>> TransferFunction t, Range r)
>> +        : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
>> +    {
>> +    }
>> +
>> +    static const ColorSpace Raw;
>> +    static const ColorSpace Jpeg;
>> +    static const ColorSpace Srgb;
>> +    static const ColorSpace Smpte170m;
>> +    static const ColorSpace Rec709;
>> +    static const ColorSpace Rec2020;
>> +
>> +    Primaries primaries;
>> +    YcbcrEncoding ycbcrEncoding;
>> +    TransferFunction transferFunction;
>> +    Range range;
>> +
>> +    const std::string toString() const;
>> +    static const std::string toString(const 
>> std::optional<ColorSpace> &colorSpace);
>> +};
>> +
>> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, 
>> YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
>> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, 
>> YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
>> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, 
>> YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
>> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, 
>> YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
>> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, 
>> YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
>> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, 
>> YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
>> +
>> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
>> +static inline bool operator!=(const ColorSpace &lhs, const 
>> ColorSpace &rhs)
>> +{
>> +    return !(lhs == rhs);
>> +}
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
>> diff --git a/include/libcamera/meson.build 
>> b/include/libcamera/meson.build
>> index 7155ff20..131e1740 100644
>> --- a/include/libcamera/meson.build
>> +++ b/include/libcamera/meson.build
>> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
>>   libcamera_public_headers = files([
>>       'camera.h',
>>       'camera_manager.h',
>> +    'color_space.h',
>>       'compiler.h',
>>       'controls.h',
>>       'file_descriptor.h',
>> diff --git a/src/libcamera/color_space.cpp 
>> b/src/libcamera/color_space.cpp
>> new file mode 100644
>> index 00000000..1a5fc7a2
>> --- /dev/null
>> +++ b/src/libcamera/color_space.cpp
>> @@ -0,0 +1,243 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
>> + *
>> + * color_space.cpp - color spaces.
>> + */
>> +
>> +#include <libcamera/color_space.h>
>> +
>> +#include <algorithm>
>> +#include <sstream>
>> +#include <vector>
>> +
>> +/**
>> + * \file color_space.h
>> + * \brief Class and enums to represent color spaces
>> + */
>> +
>> +namespace libcamera {
>> +
>> +/**
>> + * \class ColorSpace
>> + * \brief Class to describe a color space
>> + *
>> + * The ColorSpace class defines the color primaries, the Y'CbCr 
>> encoding,
>> + * the transfer function associated with the color space, and the range
>> + * (sometimes also referred to as the quantisation) of the color space.
>> + *
>> + * Certain combinations of these fields form well-known standard color
>> + * spaces such as "JPEG" or "REC709".
>> + *
>> + * In the strictest sense a "color space" formally only refers to the
>> + * color primaries and white point. Here, however, the ColorSpace class
>> + * adopts the common broader usage that includes the transfer function,
>> + * Y'CbCr encoding method and quantisation.
>> + *
>> + * For more information on the specific color spaces described here, 
>> please
>> + * see:
>> + *
>> + * <a 
>> href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
>> + * <a 
>> href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
>> + * <a 
>> href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 
>> 170M</a>
>> + * <a 
>> href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
>> + * <a 
>> href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
>> + */
>> +
>> +/**
>> + * \enum ColorSpace::Primaries
>> + * \brief The color primaries for this color space
>> + *
>> + * \var ColorSpace::Primaries::Raw
>> + * \brief These are raw colors directly from a sensor
>> + * \var ColorSpace::Primaries::Smpte170m
>> + * \brief SMPTE 170M color primaries
>> + * \var ColorSpace::Primaries::Rec709
>> + * \brief Rec.709 color primaries
>> + * \var ColorSpace::Primaries::Rec2020
>> + * \brief Rec.2020 color primaries
>> + */
>> +
>> +/**
>> + * \enum ColorSpace::YcbcrEncoding
>> + * \brief The Y'CbCr encoding
>> + *
>> + * \var ColorSpace::YcbcrEncoding::Rec601
>> + * \brief Rec.601 Y'CbCr encoding
>> + * \var ColorSpace::YcbcrEncoding::Rec709
>> + * \brief Rec.709 Y'CbCr encoding
>> + * \var ColorSpace::YcbcrEncoding::Rec2020
>> + * \brief Rec.2020 Y'CbCr encoding
>> + */
>> +
>> +/**
>> + * \enum ColorSpace::TransferFunction
>> + * \brief The transfer function used for this color space
>> + *
>> + * \var ColorSpace::TransferFunction::Linear
>> + * \brief This color space uses a linear (identity) transfer function
>> + * \var ColorSpace::TransferFunction::Srgb
>> + * \brief sRGB transfer function
>> + * \var ColorSpace::TransferFunction::Rec709
>> + * \brief Rec.709 transfer function
>> + */
>> +
>> +/**
>> + * \enum ColorSpace::Range
>> + * \brief The range (sometimes "quantisation") for this color space
>> + *
>> + * \var ColorSpace::Range::Full
>> + * \brief This color space uses full range pixel values
>> + * \var ColorSpace::Range::Limited
>> + * \brief This color space uses limited range pixel values, being
>> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
>> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
>> + */
>> +
>> +/**
>> + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, 
>> TransferFunction t, Range r)
>> + * \brief Construct a ColorSpace from explicit values
>> + * \param[in] p The color primaries
>> + * \param[in] e The Y'CbCr encoding
>> + * \param[in] t The transfer function for the color space
>> + * \param[in] r The range of the pixel values in this color space
>> + */
>> +
>> +/**
>> + * \brief Assemble and return a readable string representation of the
>> + * ColorSpace
>> + * \return A string describing the ColorSpace
>> + */
>> +const std::string ColorSpace::toString() const
>> +{
>> +    /* Print out a brief name only for standard color sapces. */
>
>
> s/sapces/spaces
>
>> +
>> +    static const std::vector<std::pair<ColorSpace, const char *>> 
>> colorSpaceNames = {
>> +        { ColorSpace::Raw, "Raw" },
>> +        { ColorSpace::Jpeg, "Jpeg" },
>> +        { ColorSpace::Srgb, "Srgb" },
>> +        { ColorSpace::Smpte170m, "Smpte170m" },
>> +        { ColorSpace::Rec709, "Rec709" },
>> +        { ColorSpace::Rec2020, "Rec2020" },
>> +    };
>
>
> Should we maintain this class-wide in its definition instead?
>
>> +    auto it = std::find_if(colorSpaceNames.begin(), 
>> colorSpaceNames.end(),
>> +                   [this](const auto &item) {
>> +                       return *this == item.first;
>> +                   });
>> +    if (it != colorSpaceNames.end())
>> +        return std::string(it->second);
>> +
>
>
> I am not sure I follow this code-path.
>
> If we cannot find a winner from colorSpaceNames vector, we return a 
> bunch of color-spcae specific information below? Can you please cover 
> this here (via a comment) and in doxygen documentation above of the 
> function?


Ok, this got cleared up in an conversion on IRC. It will print a valid 
ColorSpace which is not a part of constexpr ColorSpaces defined in header.

>
>> +    static const char *primariesNames[] = {
>> +        "Raw",
>> +        "Smpte170m",
>> +        "Rec709",
>> +        "Rec2020",
>> +    };
>> +    static const char *encodingNames[] = {
>> +        "Rec601",
>> +        "Rec709",
>> +        "Rec2020",
>> +    };
>> +    static const char *transferFunctionNames[] = {
>> +        "Linear",
>> +        "Srgb",
>> +        "Rec709",
>> +    };
>> +    static const char *rangeNames[] = {
>> +        "Full",
>> +        "Limited",
>> +    };


This does match the enums so does it make  sense to collate this under a 
single structure - for easier to extension in future

Overall looks good to me

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>> +
>> +    std::stringstream ss;
>> +    ss << std::string(primariesNames[static_cast<int>(primaries)]) 
>> << "/"
>> +       << 
>> std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
>> +       << 
>> std::string(transferFunctionNames[static_cast<int>(transferFunction)]) 
>> << "/"
>> +       << std::string(rangeNames[static_cast<int>(range)]);
>> +
>> +    return ss.str();
>> +}
>> +
>> +/**
>> + * \brief Assemble and return a readable string representation of an
>> + * optional ColorSpace
>> + * \return A string describing the optional ColorSpace
>> + */
>> +const std::string ColorSpace::toString(const 
>> std::optional<ColorSpace> &colorSpace)
>> +{
>> +    if (!colorSpace)
>> +        return "Unknown";
>> +
>> +    return colorSpace->toString();
>> +}
>> +
>> +/**
>> + * \var ColorSpace::primaries
>> + * \brief The color primaries
>> + */
>> +
>> +/**
>> + * \var ColorSpace::ycbcrEncoding
>> + * \brief The Y'CbCr encoding
>> + */
>> +
>> +/**
>> + * \var ColorSpace::transferFunction
>> + * \brief The transfer function for this color space
>> + */
>> +
>> +/**
>> + * \var ColorSpace::range
>> + * \brief The pixel range used by this color space
>> + */
>> +
>> +/**
>> + * \var ColorSpace::Undefined
>> + * \brief A constant representing a fully undefined color space
>> + */
>> +
>> +/**
>> + * \var ColorSpace::Raw
>> + * \brief A constant representing a raw color space (from a sensor)
>> + */
>> +
>> +/**
>> + * \var ColorSpace::Jpeg
>> + * \brief A constant representing the JPEG color space used for
>> + * encoding JPEG images (and regarded as being the same as the sRGB
>> + * color space)
>> + */
>> +
>> +/**
>> + * \var ColorSpace::Smpte170m
>> + * \brief A constant representing the SMPTE170M color space
>> + */
>> +
>> +/**
>> + * \var ColorSpace::Rec709
>> + * \brief A constant representing the Rec.709 color space
>> + */
>> +
>> +/**
>> + * \var ColorSpace::Rec2020
>> + * \brief A constant representing the Rec.2020 color space
>> + */
>> +
>> +/**
>> + * \brief Compare color spaces for equality
>> + * \return True if the two color spaces are identical, false otherwise
>> + */
>> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
>> +{
>> +    return lhs.primaries == rhs.primaries &&
>> +           lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
>> +           lhs.transferFunction == rhs.transferFunction &&
>> +           lhs.range == rhs.range;
>> +}
>> +
>> +/**
>> + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
>> + * \brief Compare color spaces for inequality
>> + * \return True if the two color spaces are not identical, false 
>> otherwise
>> + */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 6727a777..e7371d20 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -8,6 +8,7 @@ libcamera_sources = files([
>>       'camera_manager.cpp',
>>       'camera_sensor.cpp',
>>       'camera_sensor_properties.cpp',
>> +    'color_space.cpp',
>>       'controls.cpp',
>>       'control_serializer.cpp',
>>       'control_validator.cpp',

Hi David,

On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:
> This class represents a color space by defining its color primaries,
> YCbCr encoding, the transfer (gamma) function it uses, and whether the
> output is full or limited range.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/color_space.h |  79 +++++++++++
>  include/libcamera/meson.build   |   1 +
>  src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build       |   1 +
>  4 files changed, 324 insertions(+)
>  create mode 100644 include/libcamera/color_space.h
>  create mode 100644 src/libcamera/color_space.cpp
>
> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> new file mode 100644
> index 00000000..44ac077a
> --- /dev/null
> +++ b/include/libcamera/color_space.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.h - color space definitions
> + */
> +
> +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> +#define __LIBCAMERA_COLOR_SPACE_H__

You can now use #pragma once.
Kieran has sent a series to move the whole codebase to use it

> +
> +#include <optional>
> +#include <string>
> +
> +namespace libcamera {
> +
> +class ColorSpace
> +{
> +public:
> +	enum class Primaries : int {
> +		Raw,
> +		Smpte170m,
> +		Rec709,
> +		Rec2020,
> +	};
> +
> +	enum class YcbcrEncoding : int {
> +		Rec601,
> +		Rec709,
> +		Rec2020,
> +	};
> +
> +	enum class TransferFunction : int {
> +		Linear,
> +		Srgb,
> +		Rec709,
> +	};
> +
> +	enum class Range : int {
> +		Full,
> +		Limited,
> +	};

int or unsigned int ?

> +
> +	constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> +		: primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> +	{
> +	}
> +
> +	static const ColorSpace Raw;
> +	static const ColorSpace Jpeg;
> +	static const ColorSpace Srgb;
> +	static const ColorSpace Smpte170m;
> +	static const ColorSpace Rec709;
> +	static const ColorSpace Rec2020;
> +
> +	Primaries primaries;
> +	YcbcrEncoding ycbcrEncoding;
> +	TransferFunction transferFunction;
> +	Range range;
> +
> +	const std::string toString() const;
> +	static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> +};
> +
> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
> +
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 7155ff20..131e1740 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
>  libcamera_public_headers = files([
>      'camera.h',
>      'camera_manager.h',
> +    'color_space.h',
>      'compiler.h',
>      'controls.h',
>      'file_descriptor.h',
> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> new file mode 100644
> index 00000000..1a5fc7a2
> --- /dev/null
> +++ b/src/libcamera/color_space.cpp
> @@ -0,0 +1,243 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.cpp - color spaces.
> + */
> +
> +#include <libcamera/color_space.h>
> +
> +#include <algorithm>
> +#include <sstream>
> +#include <vector>

Do you need to include <utility> for std::pair

> +
> +/**
> + * \file color_space.h
> + * \brief Class and enums to represent color spaces
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class ColorSpace
> + * \brief Class to describe a color space
> + *
> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> + * the transfer function associated with the color space, and the range
> + * (sometimes also referred to as the quantisation) of the color space.
> + *
> + * Certain combinations of these fields form well-known standard color
> + * spaces such as "JPEG" or "REC709".
> + *
> + * In the strictest sense a "color space" formally only refers to the
> + * color primaries and white point. Here, however, the ColorSpace class
> + * adopts the common broader usage that includes the transfer function,
> + * Y'CbCr encoding method and quantisation.
> + *
> + * For more information on the specific color spaces described here, please
> + * see:
> + *
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>

Please use links from html/latest/

> + */
> +
> +/**
> + * \enum ColorSpace::Primaries
> + * \brief The color primaries for this color space
> + *

I'm debated if this documentation should provide an overview of what
each component is or it is better to assume knowledges about that and
do not say anything.

On one side, we cannot cover the whole knowledge required, and we'll
end up repeating the existing probably. On the other side,
documentation will feel a bit incomplete for who has not previous
background. But I understand we cannot document the whole world
knowledge so I think it's fair assuming background, even if this is an
application facing API which application developers will have to deal
with.

> + * \var ColorSpace::Primaries::Raw
> + * \brief These are raw colors directly from a sensor
> + * \var ColorSpace::Primaries::Smpte170m
> + * \brief SMPTE 170M color primaries

What about a blank line between these \var+\brief entries ?

> + * \var ColorSpace::Primaries::Rec709
> + * \brief Rec.709 color primaries
> + * \var ColorSpace::Primaries::Rec2020
> + * \brief Rec.2020 color primaries
> + */
> +
> +/**
> + * \enum ColorSpace::YcbcrEncoding
> + * \brief The Y'CbCr encoding

Same feeling I had before regarding documentation...

I suspect application developers might want to know more, as we are
going to 'force' them to care about color spaces..

> + *
> + * \var ColorSpace::YcbcrEncoding::Rec601
> + * \brief Rec.601 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec709
> + * \brief Rec.709 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec2020
> + * \brief Rec.2020 Y'CbCr encoding
> + */
> +
> +/**
> + * \enum ColorSpace::TransferFunction
> + * \brief The transfer function used for this color space
> + *
> + * \var ColorSpace::TransferFunction::Linear
> + * \brief This color space uses a linear (identity) transfer function
> + * \var ColorSpace::TransferFunction::Srgb
> + * \brief sRGB transfer function
> + * \var ColorSpace::TransferFunction::Rec709
> + * \brief Rec.709 transfer function
> + */
> +
> +/**
> + * \enum ColorSpace::Range
> + * \brief The range (sometimes "quantisation") for this color space
> + *
> + * \var ColorSpace::Range::Full
> + * \brief This color space uses full range pixel values

Does the 'full' reange changes with bitdepth as for Limited :)

> + * \var ColorSpace::Range::Limited
> + * \brief This color space uses limited range pixel values, being
> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> + */
> +
> +/**
> + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)
> + * \brief Construct a ColorSpace from explicit values
> + * \param[in] p The color primaries
> + * \param[in] e The Y'CbCr encoding
> + * \param[in] t The transfer function for the color space
> + * \param[in] r The range of the pixel values in this color space
> + */
> +
> +/**
> + * \brief Assemble and return a readable string representation of the
> + * ColorSpace
> + * \return A string describing the ColorSpace
> + */
> +const std::string ColorSpace::toString() const
> +{
> +	/* Print out a brief name only for standard color sapces. */
> +
> +	static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> +		{ ColorSpace::Raw, "Raw" },
> +		{ ColorSpace::Jpeg, "Jpeg" },
> +		{ ColorSpace::Srgb, "Srgb" },
> +		{ ColorSpace::Smpte170m, "Smpte170m" },
> +		{ ColorSpace::Rec709, "Rec709" },
> +		{ ColorSpace::Rec2020, "Rec2020" },
> +	};

This really looks like a map :)

> +	auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> +			       [this](const auto &item) {
> +				       return *this == item.first;
> +			       });
> +	if (it != colorSpaceNames.end())
> +		return std::string(it->second);
> +
> +	static const char *primariesNames[] = {
> +		"Raw",
> +		"Smpte170m",
> +		"Rec709",
> +		"Rec2020",
> +	};

I was expecting a warning caused by "converting a string constant to
char *"

> +	static const char *encodingNames[] = {
> +		"Rec601",
> +		"Rec709",
> +		"Rec2020",
> +	};
> +	static const char *transferFunctionNames[] = {
> +		"Linear",
> +		"Srgb",
> +		"Rec709",
> +	};
> +	static const char *rangeNames[] = {
> +		"Full",
> +		"Limited",
> +	};

This could be made a bit more C++-ish by using std::array and
std::string, but as far as I can tell using a container frowns up
making this a constexpr (soemthing you could do with raw arrays
though)

This is also slightly fragile as it requires care to maintain the vectors
and the enums in sync. Maps are probably more expensive in terms of
memory occupation but forces the association to be explicitely
maintained.

> +
> +	std::stringstream ss;
> +	ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> +	   << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> +	   << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> +	   << std::string(rangeNames[static_cast<int>(range)]);
> +
> +	return ss.str();
> +}
> +
> +/**
> + * \brief Assemble and return a readable string representation of an
> + * optional ColorSpace
> + * \return A string describing the optional ColorSpace
> + */
> +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> +{
> +	if (!colorSpace)
> +		return "Unknown";
> +
> +	return colorSpace->toString();
> +}

How do you immagine this to be used. Why would a caller use this
function on a !colorspace ? I would have ASSERT(colorspace) :)

> +
> +/**
> + * \var ColorSpace::primaries
> + * \brief The color primaries
> + */
> +
> +/**
> + * \var ColorSpace::ycbcrEncoding
> + * \brief The Y'CbCr encoding
> + */
> +
> +/**
> + * \var ColorSpace::transferFunction
> + * \brief The transfer function for this color space
> + */
> +
> +/**
> + * \var ColorSpace::range
> + * \brief The pixel range used by this color space
> + */
> +
> +/**
> + * \var ColorSpace::Undefined
> + * \brief A constant representing a fully undefined color space
> + */
> +
> +/**
> + * \var ColorSpace::Raw
> + * \brief A constant representing a raw color space (from a sensor)
> + */
> +
> +/**
> + * \var ColorSpace::Jpeg
> + * \brief A constant representing the JPEG color space used for
> + * encoding JPEG images (and regarded as being the same as the sRGB
> + * color space)
> + */
> +
> +/**
> + * \var ColorSpace::Smpte170m
> + * \brief A constant representing the SMPTE170M color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec709
> + * \brief A constant representing the Rec.709 color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec2020
> + * \brief A constant representing the Rec.2020 color space
> + */
> +
> +/**
> + * \brief Compare color spaces for equality
> + * \return True if the two color spaces are identical, false otherwise
> + */
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> +	return lhs.primaries == rhs.primaries &&
> +	       lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> +	       lhs.transferFunction == rhs.transferFunction &&
> +	       lhs.range == rhs.range;
> +}
> +
> +/**
> + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> + * \brief Compare color spaces for inequality
> + * \return True if the two color spaces are not identical, false otherwise
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6727a777..e7371d20 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -8,6 +8,7 @@ libcamera_sources = files([
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
>      'camera_sensor_properties.cpp',
> +    'color_space.cpp',
>      'controls.cpp',
>      'control_serializer.cpp',
>      'control_validator.cpp',
> --
> 2.20.1
>

Sorry, forgot one thing

On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:
> This class represents a color space by defining its color primaries,
> YCbCr encoding, the transfer (gamma) function it uses, and whether the
> output is full or limited range.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/color_space.h |  79 +++++++++++
>  include/libcamera/meson.build   |   1 +
>  src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build       |   1 +
>  4 files changed, 324 insertions(+)
>  create mode 100644 include/libcamera/color_space.h
>  create mode 100644 src/libcamera/color_space.cpp
>
> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> new file mode 100644
> index 00000000..44ac077a
> --- /dev/null
> +++ b/include/libcamera/color_space.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.h - color space definitions
> + */
> +
> +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> +#define __LIBCAMERA_COLOR_SPACE_H__
> +
> +#include <optional>
> +#include <string>
> +
> +namespace libcamera {
> +
> +class ColorSpace
> +{
> +public:
> +	enum class Primaries : int {
> +		Raw,
> +		Smpte170m,
> +		Rec709,
> +		Rec2020,
> +	};
> +
> +	enum class YcbcrEncoding : int {
> +		Rec601,
> +		Rec709,
> +		Rec2020,
> +	};
> +
> +	enum class TransferFunction : int {
> +		Linear,
> +		Srgb,
> +		Rec709,
> +	};
> +
> +	enum class Range : int {
> +		Full,
> +		Limited,
> +	};
> +
> +	constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> +		: primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> +	{
> +	}
> +
> +	static const ColorSpace Raw;
> +	static const ColorSpace Jpeg;
> +	static const ColorSpace Srgb;
> +	static const ColorSpace Smpte170m;
> +	static const ColorSpace Rec709;
> +	static const ColorSpace Rec2020;
> +
> +	Primaries primaries;
> +	YcbcrEncoding ycbcrEncoding;
> +	TransferFunction transferFunction;
> +	Range range;
> +
> +	const std::string toString() const;
> +	static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> +};
> +
> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };

As commented on the previous version

Reading this
https://isocpp.org/blog/2018/05/quick-q-use-of-constexpr-in-header-file

my understanding is that each translation unit including color_space.h
will have its own copy of these instances.

Shouldn't you declare them as 'extern const' and define them in the
.cpp file as we do for controls, or since we're now with C++17 use the
'inline' keyword that allows you to define them in the header with
automatic external linkage ?

https://en.cppreference.com/w/cpp/language/inline

> +
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 7155ff20..131e1740 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
>  libcamera_public_headers = files([
>      'camera.h',
>      'camera_manager.h',
> +    'color_space.h',
>      'compiler.h',
>      'controls.h',
>      'file_descriptor.h',
> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> new file mode 100644
> index 00000000..1a5fc7a2
> --- /dev/null
> +++ b/src/libcamera/color_space.cpp
> @@ -0,0 +1,243 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> + *
> + * color_space.cpp - color spaces.
> + */
> +
> +#include <libcamera/color_space.h>
> +
> +#include <algorithm>
> +#include <sstream>
> +#include <vector>
> +
> +/**
> + * \file color_space.h
> + * \brief Class and enums to represent color spaces
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class ColorSpace
> + * \brief Class to describe a color space
> + *
> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> + * the transfer function associated with the color space, and the range
> + * (sometimes also referred to as the quantisation) of the color space.
> + *
> + * Certain combinations of these fields form well-known standard color
> + * spaces such as "JPEG" or "REC709".
> + *
> + * In the strictest sense a "color space" formally only refers to the
> + * color primaries and white point. Here, however, the ColorSpace class
> + * adopts the common broader usage that includes the transfer function,
> + * Y'CbCr encoding method and quantisation.
> + *
> + * For more information on the specific color spaces described here, please
> + * see:
> + *
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
> + */
> +
> +/**
> + * \enum ColorSpace::Primaries
> + * \brief The color primaries for this color space
> + *
> + * \var ColorSpace::Primaries::Raw
> + * \brief These are raw colors directly from a sensor
> + * \var ColorSpace::Primaries::Smpte170m
> + * \brief SMPTE 170M color primaries
> + * \var ColorSpace::Primaries::Rec709
> + * \brief Rec.709 color primaries
> + * \var ColorSpace::Primaries::Rec2020
> + * \brief Rec.2020 color primaries
> + */
> +
> +/**
> + * \enum ColorSpace::YcbcrEncoding
> + * \brief The Y'CbCr encoding
> + *
> + * \var ColorSpace::YcbcrEncoding::Rec601
> + * \brief Rec.601 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec709
> + * \brief Rec.709 Y'CbCr encoding
> + * \var ColorSpace::YcbcrEncoding::Rec2020
> + * \brief Rec.2020 Y'CbCr encoding
> + */
> +
> +/**
> + * \enum ColorSpace::TransferFunction
> + * \brief The transfer function used for this color space
> + *
> + * \var ColorSpace::TransferFunction::Linear
> + * \brief This color space uses a linear (identity) transfer function
> + * \var ColorSpace::TransferFunction::Srgb
> + * \brief sRGB transfer function
> + * \var ColorSpace::TransferFunction::Rec709
> + * \brief Rec.709 transfer function
> + */
> +
> +/**
> + * \enum ColorSpace::Range
> + * \brief The range (sometimes "quantisation") for this color space
> + *
> + * \var ColorSpace::Range::Full
> + * \brief This color space uses full range pixel values
> + * \var ColorSpace::Range::Limited
> + * \brief This color space uses limited range pixel values, being
> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> + */
> +
> +/**
> + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)
> + * \brief Construct a ColorSpace from explicit values
> + * \param[in] p The color primaries
> + * \param[in] e The Y'CbCr encoding
> + * \param[in] t The transfer function for the color space
> + * \param[in] r The range of the pixel values in this color space
> + */
> +
> +/**
> + * \brief Assemble and return a readable string representation of the
> + * ColorSpace
> + * \return A string describing the ColorSpace
> + */
> +const std::string ColorSpace::toString() const
> +{
> +	/* Print out a brief name only for standard color sapces. */
> +
> +	static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> +		{ ColorSpace::Raw, "Raw" },
> +		{ ColorSpace::Jpeg, "Jpeg" },
> +		{ ColorSpace::Srgb, "Srgb" },
> +		{ ColorSpace::Smpte170m, "Smpte170m" },
> +		{ ColorSpace::Rec709, "Rec709" },
> +		{ ColorSpace::Rec2020, "Rec2020" },
> +	};
> +	auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> +			       [this](const auto &item) {
> +				       return *this == item.first;
> +			       });
> +	if (it != colorSpaceNames.end())
> +		return std::string(it->second);
> +
> +	static const char *primariesNames[] = {
> +		"Raw",
> +		"Smpte170m",
> +		"Rec709",
> +		"Rec2020",
> +	};
> +	static const char *encodingNames[] = {
> +		"Rec601",
> +		"Rec709",
> +		"Rec2020",
> +	};
> +	static const char *transferFunctionNames[] = {
> +		"Linear",
> +		"Srgb",
> +		"Rec709",
> +	};
> +	static const char *rangeNames[] = {
> +		"Full",
> +		"Limited",
> +	};
> +
> +	std::stringstream ss;
> +	ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> +	   << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> +	   << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> +	   << std::string(rangeNames[static_cast<int>(range)]);
> +
> +	return ss.str();
> +}
> +
> +/**
> + * \brief Assemble and return a readable string representation of an
> + * optional ColorSpace
> + * \return A string describing the optional ColorSpace
> + */
> +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> +{
> +	if (!colorSpace)
> +		return "Unknown";
> +
> +	return colorSpace->toString();
> +}
> +
> +/**
> + * \var ColorSpace::primaries
> + * \brief The color primaries
> + */
> +
> +/**
> + * \var ColorSpace::ycbcrEncoding
> + * \brief The Y'CbCr encoding
> + */
> +
> +/**
> + * \var ColorSpace::transferFunction
> + * \brief The transfer function for this color space
> + */
> +
> +/**
> + * \var ColorSpace::range
> + * \brief The pixel range used by this color space
> + */
> +
> +/**
> + * \var ColorSpace::Undefined
> + * \brief A constant representing a fully undefined color space
> + */
> +
> +/**
> + * \var ColorSpace::Raw
> + * \brief A constant representing a raw color space (from a sensor)
> + */
> +
> +/**
> + * \var ColorSpace::Jpeg
> + * \brief A constant representing the JPEG color space used for
> + * encoding JPEG images (and regarded as being the same as the sRGB
> + * color space)
> + */
> +
> +/**
> + * \var ColorSpace::Smpte170m
> + * \brief A constant representing the SMPTE170M color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec709
> + * \brief A constant representing the Rec.709 color space
> + */
> +
> +/**
> + * \var ColorSpace::Rec2020
> + * \brief A constant representing the Rec.2020 color space
> + */
> +
> +/**
> + * \brief Compare color spaces for equality
> + * \return True if the two color spaces are identical, false otherwise
> + */
> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> +{
> +	return lhs.primaries == rhs.primaries &&
> +	       lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> +	       lhs.transferFunction == rhs.transferFunction &&
> +	       lhs.range == rhs.range;
> +}
> +
> +/**
> + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> + * \brief Compare color spaces for inequality
> + * \return True if the two color spaces are not identical, false otherwise
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6727a777..e7371d20 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -8,6 +8,7 @@ libcamera_sources = files([
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
>      'camera_sensor_properties.cpp',
> +    'color_space.cpp',
>      'controls.cpp',
>      'control_serializer.cpp',
>      'control_validator.cpp',
> --
> 2.20.1
>

Hi Jacopo

Thanks for the review!

On Wed, 24 Nov 2021 at 23:39, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Sorry, forgot one thing
>
> On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:
> > This class represents a color space by defining its color primaries,
> > YCbCr encoding, the transfer (gamma) function it uses, and whether the
> > output is full or limited range.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/color_space.h |  79 +++++++++++
> >  include/libcamera/meson.build   |   1 +
> >  src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build       |   1 +
> >  4 files changed, 324 insertions(+)
> >  create mode 100644 include/libcamera/color_space.h
> >  create mode 100644 src/libcamera/color_space.cpp
> >
> > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> > new file mode 100644
> > index 00000000..44ac077a
> > --- /dev/null
> > +++ b/include/libcamera/color_space.h
> > @@ -0,0 +1,79 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > + *
> > + * color_space.h - color space definitions
> > + */
> > +
> > +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> > +#define __LIBCAMERA_COLOR_SPACE_H__
> > +
> > +#include <optional>
> > +#include <string>
> > +
> > +namespace libcamera {
> > +
> > +class ColorSpace
> > +{
> > +public:
> > +     enum class Primaries : int {
> > +             Raw,
> > +             Smpte170m,
> > +             Rec709,
> > +             Rec2020,
> > +     };
> > +
> > +     enum class YcbcrEncoding : int {
> > +             Rec601,
> > +             Rec709,
> > +             Rec2020,
> > +     };
> > +
> > +     enum class TransferFunction : int {
> > +             Linear,
> > +             Srgb,
> > +             Rec709,
> > +     };
> > +
> > +     enum class Range : int {
> > +             Full,
> > +             Limited,
> > +     };
> > +
> > +     constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> > +             : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> > +     {
> > +     }
> > +
> > +     static const ColorSpace Raw;
> > +     static const ColorSpace Jpeg;
> > +     static const ColorSpace Srgb;
> > +     static const ColorSpace Smpte170m;
> > +     static const ColorSpace Rec709;
> > +     static const ColorSpace Rec2020;
> > +
> > +     Primaries primaries;
> > +     YcbcrEncoding ycbcrEncoding;
> > +     TransferFunction transferFunction;
> > +     Range range;
> > +
> > +     const std::string toString() const;
> > +     static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> > +};
> > +
> > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
>
> As commented on the previous version
>
> Reading this
> https://isocpp.org/blog/2018/05/quick-q-use-of-constexpr-in-header-file
>
> my understanding is that each translation unit including color_space.h
> will have its own copy of these instances.
>
> Shouldn't you declare them as 'extern const' and define them in the
> .cpp file as we do for controls, or since we're now with C++17 use the
> 'inline' keyword that allows you to define them in the header with
> automatic external linkage ?
>
> https://en.cppreference.com/w/cpp/language/inline

I guess I wanted to leave them as constexpr, but I agree it's all a
bit ugly. Happy to move them into the cpp file.

Thanks!
David

>
> > +
> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > +{
> > +     return !(lhs == rhs);
> > +}
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 7155ff20..131e1740 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
> >  libcamera_public_headers = files([
> >      'camera.h',
> >      'camera_manager.h',
> > +    'color_space.h',
> >      'compiler.h',
> >      'controls.h',
> >      'file_descriptor.h',
> > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> > new file mode 100644
> > index 00000000..1a5fc7a2
> > --- /dev/null
> > +++ b/src/libcamera/color_space.cpp
> > @@ -0,0 +1,243 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > + *
> > + * color_space.cpp - color spaces.
> > + */
> > +
> > +#include <libcamera/color_space.h>
> > +
> > +#include <algorithm>
> > +#include <sstream>
> > +#include <vector>
> > +
> > +/**
> > + * \file color_space.h
> > + * \brief Class and enums to represent color spaces
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class ColorSpace
> > + * \brief Class to describe a color space
> > + *
> > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> > + * the transfer function associated with the color space, and the range
> > + * (sometimes also referred to as the quantisation) of the color space.
> > + *
> > + * Certain combinations of these fields form well-known standard color
> > + * spaces such as "JPEG" or "REC709".
> > + *
> > + * In the strictest sense a "color space" formally only refers to the
> > + * color primaries and white point. Here, however, the ColorSpace class
> > + * adopts the common broader usage that includes the transfer function,
> > + * Y'CbCr encoding method and quantisation.
> > + *
> > + * For more information on the specific color spaces described here, please
> > + * see:
> > + *
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::Primaries
> > + * \brief The color primaries for this color space
> > + *
> > + * \var ColorSpace::Primaries::Raw
> > + * \brief These are raw colors directly from a sensor
> > + * \var ColorSpace::Primaries::Smpte170m
> > + * \brief SMPTE 170M color primaries
> > + * \var ColorSpace::Primaries::Rec709
> > + * \brief Rec.709 color primaries
> > + * \var ColorSpace::Primaries::Rec2020
> > + * \brief Rec.2020 color primaries
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::YcbcrEncoding
> > + * \brief The Y'CbCr encoding
> > + *
> > + * \var ColorSpace::YcbcrEncoding::Rec601
> > + * \brief Rec.601 Y'CbCr encoding
> > + * \var ColorSpace::YcbcrEncoding::Rec709
> > + * \brief Rec.709 Y'CbCr encoding
> > + * \var ColorSpace::YcbcrEncoding::Rec2020
> > + * \brief Rec.2020 Y'CbCr encoding
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::TransferFunction
> > + * \brief The transfer function used for this color space
> > + *
> > + * \var ColorSpace::TransferFunction::Linear
> > + * \brief This color space uses a linear (identity) transfer function
> > + * \var ColorSpace::TransferFunction::Srgb
> > + * \brief sRGB transfer function
> > + * \var ColorSpace::TransferFunction::Rec709
> > + * \brief Rec.709 transfer function
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::Range
> > + * \brief The range (sometimes "quantisation") for this color space
> > + *
> > + * \var ColorSpace::Range::Full
> > + * \brief This color space uses full range pixel values
> > + * \var ColorSpace::Range::Limited
> > + * \brief This color space uses limited range pixel values, being
> > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> > + */
> > +
> > +/**
> > + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)
> > + * \brief Construct a ColorSpace from explicit values
> > + * \param[in] p The color primaries
> > + * \param[in] e The Y'CbCr encoding
> > + * \param[in] t The transfer function for the color space
> > + * \param[in] r The range of the pixel values in this color space
> > + */
> > +
> > +/**
> > + * \brief Assemble and return a readable string representation of the
> > + * ColorSpace
> > + * \return A string describing the ColorSpace
> > + */
> > +const std::string ColorSpace::toString() const
> > +{
> > +     /* Print out a brief name only for standard color sapces. */
> > +
> > +     static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> > +             { ColorSpace::Raw, "Raw" },
> > +             { ColorSpace::Jpeg, "Jpeg" },
> > +             { ColorSpace::Srgb, "Srgb" },
> > +             { ColorSpace::Smpte170m, "Smpte170m" },
> > +             { ColorSpace::Rec709, "Rec709" },
> > +             { ColorSpace::Rec2020, "Rec2020" },
> > +     };
> > +     auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> > +                            [this](const auto &item) {
> > +                                    return *this == item.first;
> > +                            });
> > +     if (it != colorSpaceNames.end())
> > +             return std::string(it->second);
> > +
> > +     static const char *primariesNames[] = {
> > +             "Raw",
> > +             "Smpte170m",
> > +             "Rec709",
> > +             "Rec2020",
> > +     };
> > +     static const char *encodingNames[] = {
> > +             "Rec601",
> > +             "Rec709",
> > +             "Rec2020",
> > +     };
> > +     static const char *transferFunctionNames[] = {
> > +             "Linear",
> > +             "Srgb",
> > +             "Rec709",
> > +     };
> > +     static const char *rangeNames[] = {
> > +             "Full",
> > +             "Limited",
> > +     };
> > +
> > +     std::stringstream ss;
> > +     ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> > +        << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> > +        << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> > +        << std::string(rangeNames[static_cast<int>(range)]);
> > +
> > +     return ss.str();
> > +}
> > +
> > +/**
> > + * \brief Assemble and return a readable string representation of an
> > + * optional ColorSpace
> > + * \return A string describing the optional ColorSpace
> > + */
> > +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> > +{
> > +     if (!colorSpace)
> > +             return "Unknown";
> > +
> > +     return colorSpace->toString();
> > +}
> > +
> > +/**
> > + * \var ColorSpace::primaries
> > + * \brief The color primaries
> > + */
> > +
> > +/**
> > + * \var ColorSpace::ycbcrEncoding
> > + * \brief The Y'CbCr encoding
> > + */
> > +
> > +/**
> > + * \var ColorSpace::transferFunction
> > + * \brief The transfer function for this color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::range
> > + * \brief The pixel range used by this color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Undefined
> > + * \brief A constant representing a fully undefined color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Raw
> > + * \brief A constant representing a raw color space (from a sensor)
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Jpeg
> > + * \brief A constant representing the JPEG color space used for
> > + * encoding JPEG images (and regarded as being the same as the sRGB
> > + * color space)
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Smpte170m
> > + * \brief A constant representing the SMPTE170M color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Rec709
> > + * \brief A constant representing the Rec.709 color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Rec2020
> > + * \brief A constant representing the Rec.2020 color space
> > + */
> > +
> > +/**
> > + * \brief Compare color spaces for equality
> > + * \return True if the two color spaces are identical, false otherwise
> > + */
> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> > +{
> > +     return lhs.primaries == rhs.primaries &&
> > +            lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> > +            lhs.transferFunction == rhs.transferFunction &&
> > +            lhs.range == rhs.range;
> > +}
> > +
> > +/**
> > + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > + * \brief Compare color spaces for inequality
> > + * \return True if the two color spaces are not identical, false otherwise
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 6727a777..e7371d20 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -8,6 +8,7 @@ libcamera_sources = files([
> >      'camera_manager.cpp',
> >      'camera_sensor.cpp',
> >      'camera_sensor_properties.cpp',
> > +    'color_space.cpp',
> >      'controls.cpp',
> >      'control_serializer.cpp',
> >      'control_validator.cpp',
> > --
> > 2.20.1
> >

Hi Jacopo

Thanks for reviewing this!

On Wed, 24 Nov 2021 at 23:36, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:
> > This class represents a color space by defining its color primaries,
> > YCbCr encoding, the transfer (gamma) function it uses, and whether the
> > output is full or limited range.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/color_space.h |  79 +++++++++++
> >  include/libcamera/meson.build   |   1 +
> >  src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build       |   1 +
> >  4 files changed, 324 insertions(+)
> >  create mode 100644 include/libcamera/color_space.h
> >  create mode 100644 src/libcamera/color_space.cpp
> >
> > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> > new file mode 100644
> > index 00000000..44ac077a
> > --- /dev/null
> > +++ b/include/libcamera/color_space.h
> > @@ -0,0 +1,79 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > + *
> > + * color_space.h - color space definitions
> > + */
> > +
> > +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> > +#define __LIBCAMERA_COLOR_SPACE_H__
>
> You can now use #pragma once.
> Kieran has sent a series to move the whole codebase to use it

Ah, very nice.

>
> > +
> > +#include <optional>
> > +#include <string>
> > +
> > +namespace libcamera {
> > +
> > +class ColorSpace
> > +{
> > +public:
> > +     enum class Primaries : int {
> > +             Raw,
> > +             Smpte170m,
> > +             Rec709,
> > +             Rec2020,
> > +     };
> > +
> > +     enum class YcbcrEncoding : int {
> > +             Rec601,
> > +             Rec709,
> > +             Rec2020,
> > +     };
> > +
> > +     enum class TransferFunction : int {
> > +             Linear,
> > +             Srgb,
> > +             Rec709,
> > +     };
> > +
> > +     enum class Range : int {
> > +             Full,
> > +             Limited,
> > +     };
>
> int or unsigned int ?

I think I've always just used int in the past but don't really mind.
Anyone feel strongly?

>
> > +
> > +     constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> > +             : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> > +     {
> > +     }
> > +
> > +     static const ColorSpace Raw;
> > +     static const ColorSpace Jpeg;
> > +     static const ColorSpace Srgb;
> > +     static const ColorSpace Smpte170m;
> > +     static const ColorSpace Rec709;
> > +     static const ColorSpace Rec2020;
> > +
> > +     Primaries primaries;
> > +     YcbcrEncoding ycbcrEncoding;
> > +     TransferFunction transferFunction;
> > +     Range range;
> > +
> > +     const std::string toString() const;
> > +     static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> > +};
> > +
> > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
> > +
> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > +{
> > +     return !(lhs == rhs);
> > +}
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 7155ff20..131e1740 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
> >  libcamera_public_headers = files([
> >      'camera.h',
> >      'camera_manager.h',
> > +    'color_space.h',
> >      'compiler.h',
> >      'controls.h',
> >      'file_descriptor.h',
> > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> > new file mode 100644
> > index 00000000..1a5fc7a2
> > --- /dev/null
> > +++ b/src/libcamera/color_space.cpp
> > @@ -0,0 +1,243 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > + *
> > + * color_space.cpp - color spaces.
> > + */
> > +
> > +#include <libcamera/color_space.h>
> > +
> > +#include <algorithm>
> > +#include <sstream>
> > +#include <vector>
>
> Do you need to include <utility> for std::pair

Probably. Hard to spot when nothing complains...

>
> > +
> > +/**
> > + * \file color_space.h
> > + * \brief Class and enums to represent color spaces
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class ColorSpace
> > + * \brief Class to describe a color space
> > + *
> > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> > + * the transfer function associated with the color space, and the range
> > + * (sometimes also referred to as the quantisation) of the color space.
> > + *
> > + * Certain combinations of these fields form well-known standard color
> > + * spaces such as "JPEG" or "REC709".
> > + *
> > + * In the strictest sense a "color space" formally only refers to the
> > + * color primaries and white point. Here, however, the ColorSpace class
> > + * adopts the common broader usage that includes the transfer function,
> > + * Y'CbCr encoding method and quantisation.
> > + *
> > + * For more information on the specific color spaces described here, please
> > + * see:
> > + *
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
>
> Please use links from html/latest/

What is html/latest/ exactly?

>
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::Primaries
> > + * \brief The color primaries for this color space
> > + *
>
> I'm debated if this documentation should provide an overview of what
> each component is or it is better to assume knowledges about that and
> do not say anything.
>
> On one side, we cannot cover the whole knowledge required, and we'll
> end up repeating the existing probably. On the other side,
> documentation will feel a bit incomplete for who has not previous
> background. But I understand we cannot document the whole world
> knowledge so I think it's fair assuming background, even if this is an
> application facing API which application developers will have to deal
> with.

There has been some discussion of this before. I think I'd like to go
with the idea that we link to other references which explain it all
much better than we are going to, rather than to duplicate everything.

>
> > + * \var ColorSpace::Primaries::Raw
> > + * \brief These are raw colors directly from a sensor
> > + * \var ColorSpace::Primaries::Smpte170m
> > + * \brief SMPTE 170M color primaries
>
> What about a blank line between these \var+\brief entries ?

Hmm, the style checker didn't ask me to do that, but I can add them.

>
> > + * \var ColorSpace::Primaries::Rec709
> > + * \brief Rec.709 color primaries
> > + * \var ColorSpace::Primaries::Rec2020
> > + * \brief Rec.2020 color primaries
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::YcbcrEncoding
> > + * \brief The Y'CbCr encoding
>
> Same feeling I had before regarding documentation...

As above, really. I think trying to explain this stuff here isn't the
way to go, folks should read those links above which explain it pretty
well.

>
> I suspect application developers might want to know more, as we are
> going to 'force' them to care about color spaces..

But we aren't "forcing" them! They can explicitly leave the ColorSpace
unset (and bury their heads in the sand)... :)

>
> > + *
> > + * \var ColorSpace::YcbcrEncoding::Rec601
> > + * \brief Rec.601 Y'CbCr encoding
> > + * \var ColorSpace::YcbcrEncoding::Rec709
> > + * \brief Rec.709 Y'CbCr encoding
> > + * \var ColorSpace::YcbcrEncoding::Rec2020
> > + * \brief Rec.2020 Y'CbCr encoding
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::TransferFunction
> > + * \brief The transfer function used for this color space
> > + *
> > + * \var ColorSpace::TransferFunction::Linear
> > + * \brief This color space uses a linear (identity) transfer function
> > + * \var ColorSpace::TransferFunction::Srgb
> > + * \brief sRGB transfer function
> > + * \var ColorSpace::TransferFunction::Rec709
> > + * \brief Rec.709 transfer function
> > + */
> > +
> > +/**
> > + * \enum ColorSpace::Range
> > + * \brief The range (sometimes "quantisation") for this color space
> > + *
> > + * \var ColorSpace::Range::Full
> > + * \brief This color space uses full range pixel values
>
> Does the 'full' reange changes with bitdepth as for Limited :)
>
> > + * \var ColorSpace::Range::Limited
> > + * \brief This color space uses limited range pixel values, being
> > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> > + */
> > +
> > +/**
> > + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)
> > + * \brief Construct a ColorSpace from explicit values
> > + * \param[in] p The color primaries
> > + * \param[in] e The Y'CbCr encoding
> > + * \param[in] t The transfer function for the color space
> > + * \param[in] r The range of the pixel values in this color space
> > + */
> > +
> > +/**
> > + * \brief Assemble and return a readable string representation of the
> > + * ColorSpace
> > + * \return A string describing the ColorSpace
> > + */
> > +const std::string ColorSpace::toString() const
> > +{
> > +     /* Print out a brief name only for standard color sapces. */
> > +
> > +     static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> > +             { ColorSpace::Raw, "Raw" },
> > +             { ColorSpace::Jpeg, "Jpeg" },
> > +             { ColorSpace::Srgb, "Srgb" },
> > +             { ColorSpace::Smpte170m, "Smpte170m" },
> > +             { ColorSpace::Rec709, "Rec709" },
> > +             { ColorSpace::Rec2020, "Rec2020" },
> > +     };
>
> This really looks like a map :)

I know. I'm slightly loath to add an artificial ordering function just
so that I can use a map, or even add a hashing function so that I can
use an unordered_map, when for smallish arrays it doesn't gain
anything. But there may come a point where adding this extra stuff
becomes easier than explaining why there's no point in adding it!

>
> > +     auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> > +                            [this](const auto &item) {
> > +                                    return *this == item.first;
> > +                            });
> > +     if (it != colorSpaceNames.end())
> > +             return std::string(it->second);
> > +
> > +     static const char *primariesNames[] = {
> > +             "Raw",
> > +             "Smpte170m",
> > +             "Rec709",
> > +             "Rec2020",
> > +     };
>
> I was expecting a warning caused by "converting a string constant to
> char *"
>
> > +     static const char *encodingNames[] = {
> > +             "Rec601",
> > +             "Rec709",
> > +             "Rec2020",
> > +     };
> > +     static const char *transferFunctionNames[] = {
> > +             "Linear",
> > +             "Srgb",
> > +             "Rec709",
> > +     };
> > +     static const char *rangeNames[] = {
> > +             "Full",
> > +             "Limited",
> > +     };
>
> This could be made a bit more C++-ish by using std::array and
> std::string, but as far as I can tell using a container frowns up
> making this a constexpr (soemthing you could do with raw arrays
> though)
>
> This is also slightly fragile as it requires care to maintain the vectors
> and the enums in sync. Maps are probably more expensive in terms of
> memory occupation but forces the association to be explicitely
> maintained.

Honestly I don't know what's best. Perhaps it should check the values
don't run off the end of the tables. Anyone feel strongly about how to
do this?

>
> > +
> > +     std::stringstream ss;
> > +     ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> > +        << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> > +        << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> > +        << std::string(rangeNames[static_cast<int>(range)]);
> > +
> > +     return ss.str();
> > +}
> > +
> > +/**
> > + * \brief Assemble and return a readable string representation of an
> > + * optional ColorSpace
> > + * \return A string describing the optional ColorSpace
> > + */
> > +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> > +{
> > +     if (!colorSpace)
> > +             return "Unknown";
> > +
> > +     return colorSpace->toString();
> > +}
>
> How do you immagine this to be used. Why would a caller use this
> function on a !colorspace ? I would have ASSERT(colorspace) :)

The catch is that our configuration and format classes contain a
std::optional<ColorSpace>, not a simple ColorSpace. So they would
spend all their time doing this:

LOG(XXX, Debug) << (colorSpace ? colorSpace.toString() : "unknown");

whereas I slightly prefer

LOG(XXX, Debug) << ColorSpace::toString(colorSpace);

Does that make sense? Or would it be better somewhere else with some
other kind of syntax?

Thanks!
David

>
> > +
> > +/**
> > + * \var ColorSpace::primaries
> > + * \brief The color primaries
> > + */
> > +
> > +/**
> > + * \var ColorSpace::ycbcrEncoding
> > + * \brief The Y'CbCr encoding
> > + */
> > +
> > +/**
> > + * \var ColorSpace::transferFunction
> > + * \brief The transfer function for this color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::range
> > + * \brief The pixel range used by this color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Undefined
> > + * \brief A constant representing a fully undefined color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Raw
> > + * \brief A constant representing a raw color space (from a sensor)
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Jpeg
> > + * \brief A constant representing the JPEG color space used for
> > + * encoding JPEG images (and regarded as being the same as the sRGB
> > + * color space)
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Smpte170m
> > + * \brief A constant representing the SMPTE170M color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Rec709
> > + * \brief A constant representing the Rec.709 color space
> > + */
> > +
> > +/**
> > + * \var ColorSpace::Rec2020
> > + * \brief A constant representing the Rec.2020 color space
> > + */
> > +
> > +/**
> > + * \brief Compare color spaces for equality
> > + * \return True if the two color spaces are identical, false otherwise
> > + */
> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> > +{
> > +     return lhs.primaries == rhs.primaries &&
> > +            lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> > +            lhs.transferFunction == rhs.transferFunction &&
> > +            lhs.range == rhs.range;
> > +}
> > +
> > +/**
> > + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > + * \brief Compare color spaces for inequality
> > + * \return True if the two color spaces are not identical, false otherwise
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 6727a777..e7371d20 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -8,6 +8,7 @@ libcamera_sources = files([
> >      'camera_manager.cpp',
> >      'camera_sensor.cpp',
> >      'camera_sensor_properties.cpp',
> > +    'color_space.cpp',
> >      'controls.cpp',
> >      'control_serializer.cpp',
> >      'control_validator.cpp',
> > --
> > 2.20.1
> >

Hi David,

On Thu, Nov 25, 2021 at 11:54:04AM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for reviewing this!
>
> On Wed, 24 Nov 2021 at 23:36, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David,
> >
> > On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:
> > > This class represents a color space by defining its color primaries,
> > > YCbCr encoding, the transfer (gamma) function it uses, and whether the
> > > output is full or limited range.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/color_space.h |  79 +++++++++++
> > >  include/libcamera/meson.build   |   1 +
> > >  src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build       |   1 +
> > >  4 files changed, 324 insertions(+)
> > >  create mode 100644 include/libcamera/color_space.h
> > >  create mode 100644 src/libcamera/color_space.cpp
> > >
> > > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> > > new file mode 100644
> > > index 00000000..44ac077a
> > > --- /dev/null
> > > +++ b/include/libcamera/color_space.h
> > > @@ -0,0 +1,79 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > > + *
> > > + * color_space.h - color space definitions
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> > > +#define __LIBCAMERA_COLOR_SPACE_H__
> >
> > You can now use #pragma once.
> > Kieran has sent a series to move the whole codebase to use it
>
> Ah, very nice.
>
> >
> > > +
> > > +#include <optional>
> > > +#include <string>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class ColorSpace
> > > +{
> > > +public:
> > > +     enum class Primaries : int {
> > > +             Raw,
> > > +             Smpte170m,
> > > +             Rec709,
> > > +             Rec2020,
> > > +     };
> > > +
> > > +     enum class YcbcrEncoding : int {
> > > +             Rec601,
> > > +             Rec709,
> > > +             Rec2020,
> > > +     };
> > > +
> > > +     enum class TransferFunction : int {
> > > +             Linear,
> > > +             Srgb,
> > > +             Rec709,
> > > +     };
> > > +
> > > +     enum class Range : int {
> > > +             Full,
> > > +             Limited,
> > > +     };
> >
> > int or unsigned int ?
>
> I think I've always just used int in the past but don't really mind.
> Anyone feel strongly?
>

well, if it doesn't need to be < 0 there's no reason not to use
unsigned

> >
> > > +
> > > +     constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> > > +             : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> > > +     {
> > > +     }
> > > +
> > > +     static const ColorSpace Raw;
> > > +     static const ColorSpace Jpeg;
> > > +     static const ColorSpace Srgb;
> > > +     static const ColorSpace Smpte170m;
> > > +     static const ColorSpace Rec709;
> > > +     static const ColorSpace Rec2020;
> > > +
> > > +     Primaries primaries;
> > > +     YcbcrEncoding ycbcrEncoding;
> > > +     TransferFunction transferFunction;
> > > +     Range range;
> > > +
> > > +     const std::string toString() const;
> > > +     static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> > > +};
> > > +
> > > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> > > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> > > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> > > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> > > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> > > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
> > > +
> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> > > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > > +{
> > > +     return !(lhs == rhs);
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > index 7155ff20..131e1740 100644
> > > --- a/include/libcamera/meson.build
> > > +++ b/include/libcamera/meson.build
> > > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
> > >  libcamera_public_headers = files([
> > >      'camera.h',
> > >      'camera_manager.h',
> > > +    'color_space.h',
> > >      'compiler.h',
> > >      'controls.h',
> > >      'file_descriptor.h',
> > > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> > > new file mode 100644
> > > index 00000000..1a5fc7a2
> > > --- /dev/null
> > > +++ b/src/libcamera/color_space.cpp
> > > @@ -0,0 +1,243 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > > + *
> > > + * color_space.cpp - color spaces.
> > > + */
> > > +
> > > +#include <libcamera/color_space.h>
> > > +
> > > +#include <algorithm>
> > > +#include <sstream>
> > > +#include <vector>
> >
> > Do you need to include <utility> for std::pair
>
> Probably. Hard to spot when nothing complains...
>
> >
> > > +
> > > +/**
> > > + * \file color_space.h
> > > + * \brief Class and enums to represent color spaces
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class ColorSpace
> > > + * \brief Class to describe a color space
> > > + *
> > > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> > > + * the transfer function associated with the color space, and the range
> > > + * (sometimes also referred to as the quantisation) of the color space.
> > > + *
> > > + * Certain combinations of these fields form well-known standard color
> > > + * spaces such as "JPEG" or "REC709".
> > > + *
> > > + * In the strictest sense a "color space" formally only refers to the
> > > + * color primaries and white point. Here, however, the ColorSpace class
> > > + * adopts the common broader usage that includes the transfer function,
> > > + * Y'CbCr encoding method and quantisation.
> > > + *
> > > + * For more information on the specific color spaces described here, please
> > > + * see:
> > > + *
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
> >
> > Please use links from html/latest/
>
> What is html/latest/ exactly?
>

https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-bt2020

> >
> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::Primaries
> > > + * \brief The color primaries for this color space
> > > + *
> >
> > I'm debated if this documentation should provide an overview of what
> > each component is or it is better to assume knowledges about that and
> > do not say anything.
> >
> > On one side, we cannot cover the whole knowledge required, and we'll
> > end up repeating the existing probably. On the other side,
> > documentation will feel a bit incomplete for who has not previous
> > background. But I understand we cannot document the whole world
> > knowledge so I think it's fair assuming background, even if this is an
> > application facing API which application developers will have to deal
> > with.
>
> There has been some discussion of this before. I think I'd like to go
> with the idea that we link to other references which explain it all
> much better than we are going to, rather than to duplicate everything.
>

Ack, to all comments on documentation if agreed with others

> >
> > > + * \var ColorSpace::Primaries::Raw
> > > + * \brief These are raw colors directly from a sensor
> > > + * \var ColorSpace::Primaries::Smpte170m
> > > + * \brief SMPTE 170M color primaries
> >
> > What about a blank line between these \var+\brief entries ?
>
> Hmm, the style checker didn't ask me to do that, but I can add them.
>

Not hard requirement, just more readable imho. Up to you.

> >
> > > + * \var ColorSpace::Primaries::Rec709
> > > + * \brief Rec.709 color primaries
> > > + * \var ColorSpace::Primaries::Rec2020
> > > + * \brief Rec.2020 color primaries
> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::YcbcrEncoding
> > > + * \brief The Y'CbCr encoding
> >
> > Same feeling I had before regarding documentation...
>
> As above, really. I think trying to explain this stuff here isn't the
> way to go, folks should read those links above which explain it pretty
> well.
>
> >
> > I suspect application developers might want to know more, as we are
> > going to 'force' them to care about color spaces..
>
> But we aren't "forcing" them! They can explicitly leave the ColorSpace
> unset (and bury their heads in the sand)... :)
>
> >
> > > + *
> > > + * \var ColorSpace::YcbcrEncoding::Rec601
> > > + * \brief Rec.601 Y'CbCr encoding
> > > + * \var ColorSpace::YcbcrEncoding::Rec709
> > > + * \brief Rec.709 Y'CbCr encoding
> > > + * \var ColorSpace::YcbcrEncoding::Rec2020
> > > + * \brief Rec.2020 Y'CbCr encoding
> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::TransferFunction
> > > + * \brief The transfer function used for this color space
> > > + *
> > > + * \var ColorSpace::TransferFunction::Linear
> > > + * \brief This color space uses a linear (identity) transfer function
> > > + * \var ColorSpace::TransferFunction::Srgb
> > > + * \brief sRGB transfer function
> > > + * \var ColorSpace::TransferFunction::Rec709
> > > + * \brief Rec.709 transfer function
> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::Range
> > > + * \brief The range (sometimes "quantisation") for this color space
> > > + *
> > > + * \var ColorSpace::Range::Full
> > > + * \brief This color space uses full range pixel values
> >
> > Does the 'full' reange changes with bitdepth as for Limited :)
> >
> > > + * \var ColorSpace::Range::Limited
> > > + * \brief This color space uses limited range pixel values, being
> > > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> > > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> > > + */
> > > +
> > > +/**
> > > + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)
> > > + * \brief Construct a ColorSpace from explicit values
> > > + * \param[in] p The color primaries
> > > + * \param[in] e The Y'CbCr encoding
> > > + * \param[in] t The transfer function for the color space
> > > + * \param[in] r The range of the pixel values in this color space
> > > + */
> > > +
> > > +/**
> > > + * \brief Assemble and return a readable string representation of the
> > > + * ColorSpace
> > > + * \return A string describing the ColorSpace
> > > + */
> > > +const std::string ColorSpace::toString() const
> > > +{
> > > +     /* Print out a brief name only for standard color sapces. */
> > > +
> > > +     static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> > > +             { ColorSpace::Raw, "Raw" },
> > > +             { ColorSpace::Jpeg, "Jpeg" },
> > > +             { ColorSpace::Srgb, "Srgb" },
> > > +             { ColorSpace::Smpte170m, "Smpte170m" },
> > > +             { ColorSpace::Rec709, "Rec709" },
> > > +             { ColorSpace::Rec2020, "Rec2020" },
> > > +     };
> >
> > This really looks like a map :)
>
> I know. I'm slightly loath to add an artificial ordering function just
> so that I can use a map, or even add a hashing function so that I can
> use an unordered_map, when for smallish arrays it doesn't gain
> anything. But there may come a point where adding this extra stuff
> becomes easier than explaining why there's no point in adding it!
>

:) Up to you

> >
> > > +     auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> > > +                            [this](const auto &item) {
> > > +                                    return *this == item.first;
> > > +                            });
> > > +     if (it != colorSpaceNames.end())
> > > +             return std::string(it->second);
> > > +
> > > +     static const char *primariesNames[] = {
> > > +             "Raw",
> > > +             "Smpte170m",
> > > +             "Rec709",
> > > +             "Rec2020",
> > > +     };
> >
> > I was expecting a warning caused by "converting a string constant to
> > char *"
> >
> > > +     static const char *encodingNames[] = {
> > > +             "Rec601",
> > > +             "Rec709",
> > > +             "Rec2020",
> > > +     };
> > > +     static const char *transferFunctionNames[] = {
> > > +             "Linear",
> > > +             "Srgb",
> > > +             "Rec709",
> > > +     };
> > > +     static const char *rangeNames[] = {
> > > +             "Full",
> > > +             "Limited",
> > > +     };
> >
> > This could be made a bit more C++-ish by using std::array and
> > std::string, but as far as I can tell using a container frowns up
> > making this a constexpr (soemthing you could do with raw arrays
> > though)
> >
> > This is also slightly fragile as it requires care to maintain the vectors
> > and the enums in sync. Maps are probably more expensive in terms of
> > memory occupation but forces the association to be explicitely
> > maintained.
>
> Honestly I don't know what's best. Perhaps it should check the values
> don't run off the end of the tables. Anyone feel strongly about how to
> do this?

This feels a bit fragile. The alternative is to define for each
colorspace/primary/encoding etc a type which contains a string
description, something I'm not sure it's worth it.

Sometimes I miss the C preprocessor stringify function :)


>
> >
> > > +
> > > +     std::stringstream ss;
> > > +     ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> > > +        << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> > > +        << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> > > +        << std::string(rangeNames[static_cast<int>(range)]);
> > > +
> > > +     return ss.str();
> > > +}
> > > +
> > > +/**
> > > + * \brief Assemble and return a readable string representation of an
> > > + * optional ColorSpace
> > > + * \return A string describing the optional ColorSpace
> > > + */
> > > +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> > > +{
> > > +     if (!colorSpace)
> > > +             return "Unknown";
> > > +
> > > +     return colorSpace->toString();
> > > +}
> >
> > How do you immagine this to be used. Why would a caller use this
> > function on a !colorspace ? I would have ASSERT(colorspace) :)
>
> The catch is that our configuration and format classes contain a
> std::optional<ColorSpace>, not a simple ColorSpace. So they would
> spend all their time doing this:
>
> LOG(XXX, Debug) << (colorSpace ? colorSpace.toString() : "unknown");
>
> whereas I slightly prefer
>
> LOG(XXX, Debug) << ColorSpace::toString(colorSpace);
>
> Does that make sense? Or would it be better somewhere else with some
> other kind of syntax?
>
> Thanks!
> David
>
> >
> > > +
> > > +/**
> > > + * \var ColorSpace::primaries
> > > + * \brief The color primaries
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::ycbcrEncoding
> > > + * \brief The Y'CbCr encoding
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::transferFunction
> > > + * \brief The transfer function for this color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::range
> > > + * \brief The pixel range used by this color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Undefined
> > > + * \brief A constant representing a fully undefined color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Raw
> > > + * \brief A constant representing a raw color space (from a sensor)
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Jpeg
> > > + * \brief A constant representing the JPEG color space used for
> > > + * encoding JPEG images (and regarded as being the same as the sRGB
> > > + * color space)
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Smpte170m
> > > + * \brief A constant representing the SMPTE170M color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Rec709
> > > + * \brief A constant representing the Rec.709 color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Rec2020
> > > + * \brief A constant representing the Rec.2020 color space
> > > + */
> > > +
> > > +/**
> > > + * \brief Compare color spaces for equality
> > > + * \return True if the two color spaces are identical, false otherwise
> > > + */
> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> > > +{
> > > +     return lhs.primaries == rhs.primaries &&
> > > +            lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> > > +            lhs.transferFunction == rhs.transferFunction &&
> > > +            lhs.range == rhs.range;
> > > +}
> > > +
> > > +/**
> > > + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > > + * \brief Compare color spaces for inequality
> > > + * \return True if the two color spaces are not identical, false otherwise
> > > + */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 6727a777..e7371d20 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -8,6 +8,7 @@ libcamera_sources = files([
> > >      'camera_manager.cpp',
> > >      'camera_sensor.cpp',
> > >      'camera_sensor_properties.cpp',
> > > +    'color_space.cpp',
> > >      'controls.cpp',
> > >      'control_serializer.cpp',
> > >      'control_validator.cpp',
> > > --
> > > 2.20.1
> > >

Quoting David Plowman (2021-11-25 11:54:04)
> Hi Jacopo
> 
> Thanks for reviewing this!
> 
> On Wed, 24 Nov 2021 at 23:36, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David,
> >
> > On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:
> > > This class represents a color space by defining its color primaries,
> > > YCbCr encoding, the transfer (gamma) function it uses, and whether the
> > > output is full or limited range.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/color_space.h |  79 +++++++++++
> > >  include/libcamera/meson.build   |   1 +
> > >  src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build       |   1 +
> > >  4 files changed, 324 insertions(+)
> > >  create mode 100644 include/libcamera/color_space.h
> > >  create mode 100644 src/libcamera/color_space.cpp
> > >
> > > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> > > new file mode 100644
> > > index 00000000..44ac077a
> > > --- /dev/null
> > > +++ b/include/libcamera/color_space.h
> > > @@ -0,0 +1,79 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > > + *
> > > + * color_space.h - color space definitions
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> > > +#define __LIBCAMERA_COLOR_SPACE_H__
> >
> > You can now use #pragma once.
> > Kieran has sent a series to move the whole codebase to use it
> 
> Ah, very nice.
> 
> >
> > > +
> > > +#include <optional>
> > > +#include <string>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class ColorSpace
> > > +{
> > > +public:
> > > +     enum class Primaries : int {
> > > +             Raw,
> > > +             Smpte170m,
> > > +             Rec709,
> > > +             Rec2020,
> > > +     };
> > > +
> > > +     enum class YcbcrEncoding : int {
> > > +             Rec601,
> > > +             Rec709,
> > > +             Rec2020,
> > > +     };
> > > +
> > > +     enum class TransferFunction : int {
> > > +             Linear,
> > > +             Srgb,
> > > +             Rec709,
> > > +     };
> > > +
> > > +     enum class Range : int {
> > > +             Full,
> > > +             Limited,
> > > +     };
> >
> > int or unsigned int ?
> 
> I think I've always just used int in the past but don't really mind.
> Anyone feel strongly?

Do we need to specify the types at all? Do we do that on other enums?

> > > +     constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)
> > > +             : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)
> > > +     {
> > > +     }
> > > +
> > > +     static const ColorSpace Raw;
> > > +     static const ColorSpace Jpeg;
> > > +     static const ColorSpace Srgb;
> > > +     static const ColorSpace Smpte170m;
> > > +     static const ColorSpace Rec709;
> > > +     static const ColorSpace Rec2020;
> > > +
> > > +     Primaries primaries;
> > > +     YcbcrEncoding ycbcrEncoding;
> > > +     TransferFunction transferFunction;
> > > +     Range range;
> > > +
> > > +     const std::string toString() const;
> > > +     static const std::string toString(const std::optional<ColorSpace> &colorSpace);
> > > +};
> > > +
> > > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };
> > > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };
> > > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };
> > > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };
> > > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };
> > > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };
> > > +
> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> > > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > > +{
> > > +     return !(lhs == rhs);
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > index 7155ff20..131e1740 100644
> > > --- a/include/libcamera/meson.build
> > > +++ b/include/libcamera/meson.build
> > > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'
> > >  libcamera_public_headers = files([
> > >      'camera.h',
> > >      'camera_manager.h',
> > > +    'color_space.h',
> > >      'compiler.h',
> > >      'controls.h',
> > >      'file_descriptor.h',
> > > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> > > new file mode 100644
> > > index 00000000..1a5fc7a2
> > > --- /dev/null
> > > +++ b/src/libcamera/color_space.cpp
> > > @@ -0,0 +1,243 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > > + *
> > > + * color_space.cpp - color spaces.
> > > + */
> > > +
> > > +#include <libcamera/color_space.h>
> > > +
> > > +#include <algorithm>
> > > +#include <sstream>
> > > +#include <vector>
> >
> > Do you need to include <utility> for std::pair
> 
> Probably. Hard to spot when nothing complains...

There's a tool called iwyu

https://include-what-you-use.org/

I've tried to figure out how we can incorporate that into checkstyle.
But it requires a build tree, so it might have to be run as an
integration type check :-(

I use it like this:
	iwyu_tool -p build/gcc/ -j32 > iwyu.report


> > > +
> > > +/**
> > > + * \file color_space.h
> > > + * \brief Class and enums to represent color spaces
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class ColorSpace
> > > + * \brief Class to describe a color space
> > > + *
> > > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,
> > > + * the transfer function associated with the color space, and the range
> > > + * (sometimes also referred to as the quantisation) of the color space.
> > > + *
> > > + * Certain combinations of these fields form well-known standard color
> > > + * spaces such as "JPEG" or "REC709".
> > > + *
> > > + * In the strictest sense a "color space" formally only refers to the
> > > + * color primaries and white point. Here, however, the ColorSpace class
> > > + * adopts the common broader usage that includes the transfer function,
> > > + * Y'CbCr encoding method and quantisation.
> > > + *
> > > + * For more information on the specific color spaces described here, please
> > > + * see:
> > > + *
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb">sRGB</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg">JPEG</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m">SMPTE 170M</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709">Rec.709</a>
> > > + * <a href="https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020">Rec.2020</a>
> >
> > Please use links from html/latest/
> 
> What is html/latest/ exactly?

It will always generate from the latest kernel release.

	https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-bt2020

My worry on that is that it can lead to stale/broken links if the
documentation paths change.

But we should at least be using the latest released kernel: 5.15...
	https://www.kernel.org/doc/html/v5.15/userspace-api/media/v4l/colorspaces-details.html#col-bt2020

We can catch broken links with tooling in doxygen though - so I think
using html/latest makes more sense.

	ninja Documentation/linkcheck

Will run the check, so this is automatable too.

> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::Primaries
> > > + * \brief The color primaries for this color space
> > > + *
> >
> > I'm debated if this documentation should provide an overview of what
> > each component is or it is better to assume knowledges about that and
> > do not say anything.
> >
> > On one side, we cannot cover the whole knowledge required, and we'll
> > end up repeating the existing probably. On the other side,
> > documentation will feel a bit incomplete for who has not previous
> > background. But I understand we cannot document the whole world
> > knowledge so I think it's fair assuming background, even if this is an
> > application facing API which application developers will have to deal
> > with.
> 
> There has been some discussion of this before. I think I'd like to go
> with the idea that we link to other references which explain it all
> much better than we are going to, rather than to duplicate everything.
> 
> >
> > > + * \var ColorSpace::Primaries::Raw
> > > + * \brief These are raw colors directly from a sensor
> > > + * \var ColorSpace::Primaries::Smpte170m
> > > + * \brief SMPTE 170M color primaries
> >
> > What about a blank line between these \var+\brief entries ?
> 
> Hmm, the style checker didn't ask me to do that, but I can add them.

Unfortunately the style checker can't check the doxygen comments. (yet?)

> > > + * \var ColorSpace::Primaries::Rec709
> > > + * \brief Rec.709 color primaries
> > > + * \var ColorSpace::Primaries::Rec2020
> > > + * \brief Rec.2020 color primaries
> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::YcbcrEncoding
> > > + * \brief The Y'CbCr encoding
> >
> > Same feeling I had before regarding documentation...
> 
> As above, really. I think trying to explain this stuff here isn't the
> way to go, folks should read those links above which explain it pretty
> well.
> 
> >
> > I suspect application developers might want to know more, as we are
> > going to 'force' them to care about color spaces..
> 
> But we aren't "forcing" them! They can explicitly leave the ColorSpace
> unset (and bury their heads in the sand)... :)

I think that's fine. They will leave it unset when configuring, but be
/told/ what colourspace it is when the configuration is validated.

If they don't choose to care about it from there, then that's up to them
;-) Perhaps they really didn't need it..

> > > + *
> > > + * \var ColorSpace::YcbcrEncoding::Rec601
> > > + * \brief Rec.601 Y'CbCr encoding
> > > + * \var ColorSpace::YcbcrEncoding::Rec709
> > > + * \brief Rec.709 Y'CbCr encoding
> > > + * \var ColorSpace::YcbcrEncoding::Rec2020
> > > + * \brief Rec.2020 Y'CbCr encoding
> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::TransferFunction
> > > + * \brief The transfer function used for this color space
> > > + *
> > > + * \var ColorSpace::TransferFunction::Linear
> > > + * \brief This color space uses a linear (identity) transfer function
> > > + * \var ColorSpace::TransferFunction::Srgb
> > > + * \brief sRGB transfer function
> > > + * \var ColorSpace::TransferFunction::Rec709
> > > + * \brief Rec.709 transfer function
> > > + */
> > > +
> > > +/**
> > > + * \enum ColorSpace::Range
> > > + * \brief The range (sometimes "quantisation") for this color space
> > > + *
> > > + * \var ColorSpace::Range::Full
> > > + * \brief This color space uses full range pixel values
> >
> > Does the 'full' reange changes with bitdepth as for Limited :)
> >
> > > + * \var ColorSpace::Range::Limited
> > > + * \brief This color space uses limited range pixel values, being
> > > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)
> > > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)
> > > + */
> > > +
> > > +/**
> > > + * \fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)
> > > + * \brief Construct a ColorSpace from explicit values
> > > + * \param[in] p The color primaries
> > > + * \param[in] e The Y'CbCr encoding
> > > + * \param[in] t The transfer function for the color space
> > > + * \param[in] r The range of the pixel values in this color space
> > > + */
> > > +
> > > +/**
> > > + * \brief Assemble and return a readable string representation of the
> > > + * ColorSpace
> > > + * \return A string describing the ColorSpace
> > > + */
> > > +const std::string ColorSpace::toString() const
> > > +{
> > > +     /* Print out a brief name only for standard color sapces. */
> > > +
> > > +     static const std::vector<std::pair<ColorSpace, const char *>> colorSpaceNames = {
> > > +             { ColorSpace::Raw, "Raw" },
> > > +             { ColorSpace::Jpeg, "Jpeg" },
> > > +             { ColorSpace::Srgb, "Srgb" },
> > > +             { ColorSpace::Smpte170m, "Smpte170m" },
> > > +             { ColorSpace::Rec709, "Rec709" },
> > > +             { ColorSpace::Rec2020, "Rec2020" },
> > > +     };
> >
> > This really looks like a map :)
> 
> I know. I'm slightly loath to add an artificial ordering function just
> so that I can use a map, or even add a hashing function so that I can
> use an unordered_map, when for smallish arrays it doesn't gain
> anything. But there may come a point where adding this extra stuff
> becomes easier than explaining why there's no point in adding it!

Ah - yes the ColorSpace is not something that's easily turned into an
index...

> > > +     auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),
> > > +                            [this](const auto &item) {
> > > +                                    return *this == item.first;
> > > +                            });
> > > +     if (it != colorSpaceNames.end())
> > > +             return std::string(it->second);
> > > +
> > > +     static const char *primariesNames[] = {
> > > +             "Raw",
> > > +             "Smpte170m",
> > > +             "Rec709",
> > > +             "Rec2020",
> > > +     };
> >
> > I was expecting a warning caused by "converting a string constant to
> > char *"
> >
> > > +     static const char *encodingNames[] = {
> > > +             "Rec601",
> > > +             "Rec709",
> > > +             "Rec2020",
> > > +     };
> > > +     static const char *transferFunctionNames[] = {
> > > +             "Linear",
> > > +             "Srgb",
> > > +             "Rec709",
> > > +     };
> > > +     static const char *rangeNames[] = {
> > > +             "Full",
> > > +             "Limited",
> > > +     };
> >
> > This could be made a bit more C++-ish by using std::array and
> > std::string, but as far as I can tell using a container frowns up
> > making this a constexpr (soemthing you could do with raw arrays
> > though)
> >
> > This is also slightly fragile as it requires care to maintain the vectors
> > and the enums in sync. Maps are probably more expensive in terms of
> > memory occupation but forces the association to be explicitely
> > maintained.
> 
> Honestly I don't know what's best. Perhaps it should check the values
> don't run off the end of the tables. Anyone feel strongly about how to
> do this?

I'd be a little bit worried ...

Can an appliction now construct a ColorSpace directly, with an invalid
range of '4', and cause this function to access memory beyond the
definition?

If so - we certainly need to prevent that, otherwise we'll start seeing
people filling vulnerabilities against us...


> > > +
> > > +     std::stringstream ss;
> > > +     ss << std::string(primariesNames[static_cast<int>(primaries)]) << "/"
> > > +        << std::string(encodingNames[static_cast<int>(ycbcrEncoding)]) << "/"
> > > +        << std::string(transferFunctionNames[static_cast<int>(transferFunction)]) << "/"
> > > +        << std::string(rangeNames[static_cast<int>(range)]);
> > > +
> > > +     return ss.str();
> > > +}
> > > +
> > > +/**
> > > + * \brief Assemble and return a readable string representation of an
> > > + * optional ColorSpace
> > > + * \return A string describing the optional ColorSpace
> > > + */
> > > +const std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> > > +{
> > > +     if (!colorSpace)
> > > +             return "Unknown";
> > > +
> > > +     return colorSpace->toString();
> > > +}
> >
> > How do you immagine this to be used. Why would a caller use this
> > function on a !colorspace ? I would have ASSERT(colorspace) :)
> 
> The catch is that our configuration and format classes contain a
> std::optional<ColorSpace>, not a simple ColorSpace. So they would
> spend all their time doing this:
> 
> LOG(XXX, Debug) << (colorSpace ? colorSpace.toString() : "unknown");
> 
> whereas I slightly prefer
> 
> LOG(XXX, Debug) << ColorSpace::toString(colorSpace);

^^ This is better IMO.

> 
> Does that make sense? Or would it be better somewhere else with some
> other kind of syntax?
> 
> Thanks!
> David
> 
> >
> > > +
> > > +/**
> > > + * \var ColorSpace::primaries
> > > + * \brief The color primaries
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::ycbcrEncoding
> > > + * \brief The Y'CbCr encoding
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::transferFunction
> > > + * \brief The transfer function for this color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::range
> > > + * \brief The pixel range used by this color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Undefined
> > > + * \brief A constant representing a fully undefined color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Raw
> > > + * \brief A constant representing a raw color space (from a sensor)
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Jpeg
> > > + * \brief A constant representing the JPEG color space used for
> > > + * encoding JPEG images (and regarded as being the same as the sRGB
> > > + * color space)
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Smpte170m
> > > + * \brief A constant representing the SMPTE170M color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Rec709
> > > + * \brief A constant representing the Rec.709 color space
> > > + */
> > > +
> > > +/**
> > > + * \var ColorSpace::Rec2020
> > > + * \brief A constant representing the Rec.2020 color space
> > > + */
> > > +
> > > +/**
> > > + * \brief Compare color spaces for equality
> > > + * \return True if the two color spaces are identical, false otherwise
> > > + */
> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> > > +{
> > > +     return lhs.primaries == rhs.primaries &&
> > > +            lhs.ycbcrEncoding == rhs.ycbcrEncoding &&
> > > +            lhs.transferFunction == rhs.transferFunction &&
> > > +            lhs.range == rhs.range;
> > > +}
> > > +
> > > +/**
> > > + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > > + * \brief Compare color spaces for inequality
> > > + * \return True if the two color spaces are not identical, false otherwise
> > > + */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 6727a777..e7371d20 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -8,6 +8,7 @@ libcamera_sources = files([
> > >      'camera_manager.cpp',
> > >      'camera_sensor.cpp',
> > >      'camera_sensor_properties.cpp',
> > > +    'color_space.cpp',
> > >      'controls.cpp',
> > >      'control_serializer.cpp',
> > >      'control_validator.cpp',
> > > --
> > > 2.20.1
> > >