[{"id":21110,"web_url":"https://patchwork.libcamera.org/comment/21110/","msgid":"<95df5e7c-afd6-a718-e1e7-ca07f1cce50e@ideasonboard.com>","date":"2021-11-23T07:20:35","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn 11/18/21 8:49 PM, David Plowman wrote:\n> This class represents a color space by defining its color primaries,\n> YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> output is full or limited range.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> include/libcamera/color_space.h | 79 +++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 324 insertions(+)\n> create mode 100644 include/libcamera/color_space.h\n> create mode 100644 src/libcamera/color_space.cpp\n>\n> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> new file mode 100644\n> index 00000000..44ac077a\n> --- /dev/null\n> +++ b/include/libcamera/color_space.h\n> @@ -0,0 +1,79 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.h - color space definitions\n> + */\n> +\n> +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n> +#define __LIBCAMERA_COLOR_SPACE_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class ColorSpace\n> +{\n> +public:\n> +\tenum class Primaries : int {\n> +\t\tRaw,\n> +\t\tSmpte170m,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class YcbcrEncoding : int {\n> +\t\tRec601,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class TransferFunction : int {\n> +\t\tLinear,\n> +\t\tSrgb,\n> +\t\tRec709,\n> +\t};\n> +\n> +\tenum class Range : int {\n> +\t\tFull,\n> +\t\tLimited,\n> +\t};\n> +\n> +\tconstexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> +\t\t: primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> +\t{\n> +\t}\n> +\n> +\tstatic const ColorSpace Raw;\n> +\tstatic const ColorSpace Jpeg;\n> +\tstatic const ColorSpace Srgb;\n> +\tstatic const ColorSpace Smpte170m;\n> +\tstatic const ColorSpace Rec709;\n> +\tstatic const ColorSpace Rec2020;\n> +\n> +\tPrimaries primaries;\n> +\tYcbcrEncoding ycbcrEncoding;\n> +\tTransferFunction transferFunction;\n> +\tRange range;\n> +\n> +\tconst std::string toString() const;\n> +\tstatic const std::string toString(const std::optional &colorSpace);\n> +};\n> +\n> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n> +\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn !(lhs == rhs);\n> +}\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 7155ff20..131e1740 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> libcamera_public_headers = files([\n> 'camera.h',\n> 'camera_manager.h',\n> + 'color_space.h',\n> 'compiler.h',\n> 'controls.h',\n> 'file_descriptor.h',\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> new file mode 100644\n> index 00000000..1a5fc7a2\n> --- /dev/null\n> +++ b/src/libcamera/color_space.cpp\n> @@ -0,0 +1,243 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.cpp - color spaces.\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +/**\n> + * \\file color_space.h\n> + * \\brief Class and enums to represent color spaces\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class ColorSpace\n> + * \\brief Class to describe a color space\n> + *\n> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> + * the transfer function associated with the color space, and the range\n> + * (sometimes also referred to as the quantisation) of the color space.\n> + *\n> + * Certain combinations of these fields form well-known standard color\n> + * spaces such as \"JPEG\" or \"REC709\".\n> + *\n> + * In the strictest sense a \"color space\" formally only refers to the\n> + * color primaries and white point. Here, however, the ColorSpace class\n> + * adopts the common broader usage that includes the transfer function,\n> + * Y'CbCr encoding method and quantisation.\n> + *\n> + * For more information on the specific color spaces described here, please\n> + * see:\n> + *\n> + * sRGB\n> + * JPEG\n> + * SMPTE 170M\n> + * Rec.709\n> + * Rec.2020\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Primaries\n> + * \\brief The color primaries for this color space\n> + *\n> + * \\var ColorSpace::Primaries::Raw\n> + * \\brief These are raw colors directly from a sensor\n> + * \\var ColorSpace::Primaries::Smpte170m\n> + * \\brief SMPTE 170M color primaries\n> + * \\var ColorSpace::Primaries::Rec709\n> + * \\brief Rec.709 color primaries\n> + * \\var ColorSpace::Primaries::Rec2020\n> + * \\brief Rec.2020 color primaries\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::YcbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + *\n> + * \\var ColorSpace::YcbcrEncoding::Rec601\n> + * \\brief Rec.601 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec709\n> + * \\brief Rec.709 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> + * \\brief Rec.2020 Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::TransferFunction\n> + * \\brief The transfer function used for this color space\n> + *\n> + * \\var ColorSpace::TransferFunction::Linear\n> + * \\brief This color space uses a linear (identity) transfer function\n> + * \\var ColorSpace::TransferFunction::Srgb\n> + * \\brief sRGB transfer function\n> + * \\var ColorSpace::TransferFunction::Rec709\n> + * \\brief Rec.709 transfer function\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Range\n> + * \\brief The range (sometimes \"quantisation\") for this color space\n> + *\n> + * \\var ColorSpace::Range::Full\n> + * \\brief This color space uses full range pixel values\n> + * \\var ColorSpace::Range::Limited\n> + * \\brief This color space uses limited range pixel values, being\n> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)\n> + * \\brief Construct a ColorSpace from explicit values\n> + * \\param[in] p The color primaries\n> + * \\param[in] e The Y'CbCr encoding\n> + * \\param[in] t The transfer function for the color space\n> + * \\param[in] r The range of the pixel values in this color space\n> + */\n> +\n> +/**\n> + * \\brief Assemble and return a readable string representation of the\n> + * ColorSpace\n> + * \\return A string describing the ColorSpace\n> + */\n> +const std::string ColorSpace::toString() const\n> +{\n> +\t/* Print out a brief name only for standard color sapces. */\n\n\ns/sapces/spaces\n\n> +\n> +\tstatic const std::vector> colorSpaceNames = {\n> +\t\t{ ColorSpace::Raw, \"Raw\" },\n> +\t\t{ ColorSpace::Jpeg, \"Jpeg\" },\n> +\t\t{ ColorSpace::Srgb, \"Srgb\" },\n> +\t\t{ ColorSpace::Smpte170m, \"Smpte170m\" },\n> +\t\t{ ColorSpace::Rec709, \"Rec709\" },\n> +\t\t{ ColorSpace::Rec2020, \"Rec2020\" },\n> +\t};\n\n\nShould we maintain this class-wide in its definition instead?\n\n> +\tauto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> +\t\t\t [this](const auto &item) {\n> +\t\t\t\t return *this == item.first;\n> +\t\t\t });\n> +\tif (it != colorSpaceNames.end())\n> +\t\treturn std::string(it->second);\n> +\n\n\nI am not sure I follow this code-path.\n\nIf we cannot find a winner from colorSpaceNames vector, we return a \nbunch of color-spcae specific information below? Can you please cover \nthis here (via a comment) and in doxygen documentation above of the \nfunction?\n\n> +\tstatic const char *primariesNames[] = {\n> +\t\t\"Raw\",\n> +\t\t\"Smpte170m\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n> +\tstatic const char *encodingNames[] = {\n> +\t\t\"Rec601\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n> +\tstatic const char *transferFunctionNames[] = {\n> +\t\t\"Linear\",\n> +\t\t\"Srgb\",\n> +\t\t\"Rec709\",\n> +\t};\n> +\tstatic const char *rangeNames[] = {\n> +\t\t\"Full\",\n> +\t\t\"Limited\",\n> +\t};\n> +\n> +\tstd::stringstream ss;\n> +\tss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> +\t << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> +\t << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> +\t << std::string(rangeNames[static_cast(range)]);\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +/**\n> + * \\brief Assemble and return a readable string representation of an\n> + * optional ColorSpace\n> + * \\return A string describing the optional ColorSpace\n> + */\n> +const std::string ColorSpace::toString(const std::optional &colorSpace)\n> +{\n> +\tif (!colorSpace)\n> +\t\treturn \"Unknown\";\n> +\n> +\treturn colorSpace->toString();\n> +}\n> +\n> +/**\n> + * \\var ColorSpace::primaries\n> + * \\brief The color primaries\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::ycbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::transferFunction\n> + * \\brief The transfer function for this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::range\n> + * \\brief The pixel range used by this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Undefined\n> + * \\brief A constant representing a fully undefined color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Raw\n> + * \\brief A constant representing a raw color space (from a sensor)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Jpeg\n> + * \\brief A constant representing the JPEG color space used for\n> + * encoding JPEG images (and regarded as being the same as the sRGB\n> + * color space)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Smpte170m\n> + * \\brief A constant representing the SMPTE170M color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec709\n> + * \\brief A constant representing the Rec.709 color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec2020\n> + * \\brief A constant representing the Rec.2020 color space\n> + */\n> +\n> +/**\n> + * \\brief Compare color spaces for equality\n> + * \\return True if the two color spaces are identical, false otherwise\n> + */\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn lhs.primaries == rhs.primaries &&\n> +\t lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> +\t lhs.transferFunction == rhs.transferFunction &&\n> +\t lhs.range == rhs.range;\n> +}\n> +\n> +/**\n> + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> + * \\brief Compare color spaces for inequality\n> + * \\return True if the two color spaces are not identical, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777..e7371d20 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -8,6 +8,7 @@ libcamera_sources = files([\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> 'camera_sensor_properties.cpp',\n> + 'color_space.cpp',\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7640CBF415\n\tfor ;\n\tTue, 23 Nov 2021 07:20:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0C856036F;\n\tTue, 23 Nov 2021 08:20:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E973B60227\n\tfor ;\n\tTue, 23 Nov 2021 08:20:42 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86057A1B;\n\tTue, 23 Nov 2021 08:20:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Z1/gLC5Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637652042;\n\tbh=lc5JAeVUTyCTZZHv6O5HFtKDRMeo0PwdgUrPIYiqM6o=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Z1/gLC5QS/TR5RSvUIZmtm/ljMG9ZT1NLNZWvBTCcahXY+j9dh51uS8OXlwl+3Wm3\n\tvzcIcINapKA8gSDo9MWJYvxr8WyUgWBI5M03OxDZyH8fEdtVxlgfRPaMKgXeKfMIMw\n\tbOozJPL/lXtJe5GNIy2Owfuu4bYoPvAIhgPLOnLI=","To":"David Plowman ,\n\tlaurent.pinchart@ideasonboard.com, kieran.bingham@ideasonboard.com,\n\thverkuil-cisco@xs4all.nl, tfiga@google.com, jacopo@jmondi.org,\n\tnaush@raspberrypi.com, libcamera-devel@lists.libcamera.org","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>","From":"Umang Jain ","Message-ID":"<95df5e7c-afd6-a718-e1e7-ca07f1cce50e@ideasonboard.com>","Date":"Tue, 23 Nov 2021 12:50:35 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211118151933.15627-2-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21137,"web_url":"https://patchwork.libcamera.org/comment/21137/","msgid":"","date":"2021-11-23T14:41:41","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 11/23/21 12:50 PM, Umang Jain wrote:\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On 11/18/21 8:49 PM, David Plowman wrote:\n>> This class represents a color space by defining its color primaries,\n>> YCbCr encoding, the transfer (gamma) function it uses, and whether the\n>> output is full or limited range.\n>>\n>> Signed-off-by: David Plowman \n>> Reviewed-by: Naushir Patuck \n>> ---\n>>   include/libcamera/color_space.h |  79 +++++++++++\n>>   include/libcamera/meson.build   |   1 +\n>>   src/libcamera/color_space.cpp   | 243 ++++++++++++++++++++++++++++++++\n>>   src/libcamera/meson.build       |   1 +\n>>   4 files changed, 324 insertions(+)\n>>   create mode 100644 include/libcamera/color_space.h\n>>   create mode 100644 src/libcamera/color_space.cpp\n>>\n>> diff --git a/include/libcamera/color_space.h \n>> b/include/libcamera/color_space.h\n>> new file mode 100644\n>> index 00000000..44ac077a\n>> --- /dev/null\n>> +++ b/include/libcamera/color_space.h\n>> @@ -0,0 +1,79 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n>> + *\n>> + * color_space.h - color space definitions\n>> + */\n>> +\n>> +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n>> +#define __LIBCAMERA_COLOR_SPACE_H__\n>> +\n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +class ColorSpace\n>> +{\n>> +public:\n>> +    enum class Primaries : int {\n>> +        Raw,\n>> +        Smpte170m,\n>> +        Rec709,\n>> +        Rec2020,\n>> +    };\n>> +\n>> +    enum class YcbcrEncoding : int {\n>> +        Rec601,\n>> +        Rec709,\n>> +        Rec2020,\n>> +    };\n>> +\n>> +    enum class TransferFunction : int {\n>> +        Linear,\n>> +        Srgb,\n>> +        Rec709,\n>> +    };\n>> +\n>> +    enum class Range : int {\n>> +        Full,\n>> +        Limited,\n>> +    };\n>> +\n>> +    constexpr ColorSpace(Primaries p, YcbcrEncoding e, \n>> TransferFunction t, Range r)\n>> +        : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n>> +    {\n>> +    }\n>> +\n>> +    static const ColorSpace Raw;\n>> +    static const ColorSpace Jpeg;\n>> +    static const ColorSpace Srgb;\n>> +    static const ColorSpace Smpte170m;\n>> +    static const ColorSpace Rec709;\n>> +    static const ColorSpace Rec2020;\n>> +\n>> +    Primaries primaries;\n>> +    YcbcrEncoding ycbcrEncoding;\n>> +    TransferFunction transferFunction;\n>> +    Range range;\n>> +\n>> +    const std::string toString() const;\n>> +    static const std::string toString(const \n>> std::optional &colorSpace);\n>> +};\n>> +\n>> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, \n>> YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n>> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, \n>> YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n>> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, \n>> YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n>> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, \n>> YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n>> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, \n>> YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n>> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, \n>> YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n>> +\n>> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n>> +static inline bool operator!=(const ColorSpace &lhs, const \n>> ColorSpace &rhs)\n>> +{\n>> +    return !(lhs == rhs);\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n>> diff --git a/include/libcamera/meson.build \n>> b/include/libcamera/meson.build\n>> index 7155ff20..131e1740 100644\n>> --- a/include/libcamera/meson.build\n>> +++ b/include/libcamera/meson.build\n>> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n>>   libcamera_public_headers = files([\n>>       'camera.h',\n>>       'camera_manager.h',\n>> +    'color_space.h',\n>>       'compiler.h',\n>>       'controls.h',\n>>       'file_descriptor.h',\n>> diff --git a/src/libcamera/color_space.cpp \n>> b/src/libcamera/color_space.cpp\n>> new file mode 100644\n>> index 00000000..1a5fc7a2\n>> --- /dev/null\n>> +++ b/src/libcamera/color_space.cpp\n>> @@ -0,0 +1,243 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n>> + *\n>> + * color_space.cpp - color spaces.\n>> + */\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +/**\n>> + * \\file color_space.h\n>> + * \\brief Class and enums to represent color spaces\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +/**\n>> + * \\class ColorSpace\n>> + * \\brief Class to describe a color space\n>> + *\n>> + * The ColorSpace class defines the color primaries, the Y'CbCr \n>> encoding,\n>> + * the transfer function associated with the color space, and the range\n>> + * (sometimes also referred to as the quantisation) of the color space.\n>> + *\n>> + * Certain combinations of these fields form well-known standard color\n>> + * spaces such as \"JPEG\" or \"REC709\".\n>> + *\n>> + * In the strictest sense a \"color space\" formally only refers to the\n>> + * color primaries and white point. Here, however, the ColorSpace class\n>> + * adopts the common broader usage that includes the transfer function,\n>> + * Y'CbCr encoding method and quantisation.\n>> + *\n>> + * For more information on the specific color spaces described here, \n>> please\n>> + * see:\n>> + *\n>> + * > href=\"https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-srgb\">sRGB\n>> + * > href=\"https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-jpeg\">JPEG\n>> + * > href=\"https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m\">SMPTE \n>> 170M\n>> + * > href=\"https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-rec709\">Rec.709\n>> + * > href=\"https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/colorspaces-details.html#col-bt2020\">Rec.2020\n>> + */\n>> +\n>> +/**\n>> + * \\enum ColorSpace::Primaries\n>> + * \\brief The color primaries for this color space\n>> + *\n>> + * \\var ColorSpace::Primaries::Raw\n>> + * \\brief These are raw colors directly from a sensor\n>> + * \\var ColorSpace::Primaries::Smpte170m\n>> + * \\brief SMPTE 170M color primaries\n>> + * \\var ColorSpace::Primaries::Rec709\n>> + * \\brief Rec.709 color primaries\n>> + * \\var ColorSpace::Primaries::Rec2020\n>> + * \\brief Rec.2020 color primaries\n>> + */\n>> +\n>> +/**\n>> + * \\enum ColorSpace::YcbcrEncoding\n>> + * \\brief The Y'CbCr encoding\n>> + *\n>> + * \\var ColorSpace::YcbcrEncoding::Rec601\n>> + * \\brief Rec.601 Y'CbCr encoding\n>> + * \\var ColorSpace::YcbcrEncoding::Rec709\n>> + * \\brief Rec.709 Y'CbCr encoding\n>> + * \\var ColorSpace::YcbcrEncoding::Rec2020\n>> + * \\brief Rec.2020 Y'CbCr encoding\n>> + */\n>> +\n>> +/**\n>> + * \\enum ColorSpace::TransferFunction\n>> + * \\brief The transfer function used for this color space\n>> + *\n>> + * \\var ColorSpace::TransferFunction::Linear\n>> + * \\brief This color space uses a linear (identity) transfer function\n>> + * \\var ColorSpace::TransferFunction::Srgb\n>> + * \\brief sRGB transfer function\n>> + * \\var ColorSpace::TransferFunction::Rec709\n>> + * \\brief Rec.709 transfer function\n>> + */\n>> +\n>> +/**\n>> + * \\enum ColorSpace::Range\n>> + * \\brief The range (sometimes \"quantisation\") for this color space\n>> + *\n>> + * \\var ColorSpace::Range::Full\n>> + * \\brief This color space uses full range pixel values\n>> + * \\var ColorSpace::Range::Limited\n>> + * \\brief This color space uses limited range pixel values, being\n>> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n>> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n>> + */\n>> +\n>> +/**\n>> + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, \n>> TransferFunction t, Range r)\n>> + * \\brief Construct a ColorSpace from explicit values\n>> + * \\param[in] p The color primaries\n>> + * \\param[in] e The Y'CbCr encoding\n>> + * \\param[in] t The transfer function for the color space\n>> + * \\param[in] r The range of the pixel values in this color space\n>> + */\n>> +\n>> +/**\n>> + * \\brief Assemble and return a readable string representation of the\n>> + * ColorSpace\n>> + * \\return A string describing the ColorSpace\n>> + */\n>> +const std::string ColorSpace::toString() const\n>> +{\n>> +    /* Print out a brief name only for standard color sapces. */\n>\n>\n> s/sapces/spaces\n>\n>> +\n>> +    static const std::vector> \n>> colorSpaceNames = {\n>> +        { ColorSpace::Raw, \"Raw\" },\n>> +        { ColorSpace::Jpeg, \"Jpeg\" },\n>> +        { ColorSpace::Srgb, \"Srgb\" },\n>> +        { ColorSpace::Smpte170m, \"Smpte170m\" },\n>> +        { ColorSpace::Rec709, \"Rec709\" },\n>> +        { ColorSpace::Rec2020, \"Rec2020\" },\n>> +    };\n>\n>\n> Should we maintain this class-wide in its definition instead?\n>\n>> +    auto it = std::find_if(colorSpaceNames.begin(), \n>> colorSpaceNames.end(),\n>> +                   [this](const auto &item) {\n>> +                       return *this == item.first;\n>> +                   });\n>> +    if (it != colorSpaceNames.end())\n>> +        return std::string(it->second);\n>> +\n>\n>\n> I am not sure I follow this code-path.\n>\n> If we cannot find a winner from colorSpaceNames vector, we return a \n> bunch of color-spcae specific information below? Can you please cover \n> this here (via a comment) and in doxygen documentation above of the \n> function?\n\n\nOk, this got cleared up in an conversion on IRC. It will print a valid \nColorSpace which is not a part of constexpr ColorSpaces defined in header.\n\n>\n>> +    static const char *primariesNames[] = {\n>> +        \"Raw\",\n>> +        \"Smpte170m\",\n>> +        \"Rec709\",\n>> +        \"Rec2020\",\n>> +    };\n>> +    static const char *encodingNames[] = {\n>> +        \"Rec601\",\n>> +        \"Rec709\",\n>> +        \"Rec2020\",\n>> +    };\n>> +    static const char *transferFunctionNames[] = {\n>> +        \"Linear\",\n>> +        \"Srgb\",\n>> +        \"Rec709\",\n>> +    };\n>> +    static const char *rangeNames[] = {\n>> +        \"Full\",\n>> +        \"Limited\",\n>> +    };\n\n\nThis does match the enums so does it make  sense to collate this under a \nsingle structure - for easier to extension in future\n\nOverall looks good to me\n\nReviewed-by: Umang Jain \n\n>> +\n>> +    std::stringstream ss;\n>> +    ss << std::string(primariesNames[static_cast(primaries)]) \n>> << \"/\"\n>> +       << \n>> std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n>> +       << \n>> std::string(transferFunctionNames[static_cast(transferFunction)]) \n>> << \"/\"\n>> +       << std::string(rangeNames[static_cast(range)]);\n>> +\n>> +    return ss.str();\n>> +}\n>> +\n>> +/**\n>> + * \\brief Assemble and return a readable string representation of an\n>> + * optional ColorSpace\n>> + * \\return A string describing the optional ColorSpace\n>> + */\n>> +const std::string ColorSpace::toString(const \n>> std::optional &colorSpace)\n>> +{\n>> +    if (!colorSpace)\n>> +        return \"Unknown\";\n>> +\n>> +    return colorSpace->toString();\n>> +}\n>> +\n>> +/**\n>> + * \\var ColorSpace::primaries\n>> + * \\brief The color primaries\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::ycbcrEncoding\n>> + * \\brief The Y'CbCr encoding\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::transferFunction\n>> + * \\brief The transfer function for this color space\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::range\n>> + * \\brief The pixel range used by this color space\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::Undefined\n>> + * \\brief A constant representing a fully undefined color space\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::Raw\n>> + * \\brief A constant representing a raw color space (from a sensor)\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::Jpeg\n>> + * \\brief A constant representing the JPEG color space used for\n>> + * encoding JPEG images (and regarded as being the same as the sRGB\n>> + * color space)\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::Smpte170m\n>> + * \\brief A constant representing the SMPTE170M color space\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::Rec709\n>> + * \\brief A constant representing the Rec.709 color space\n>> + */\n>> +\n>> +/**\n>> + * \\var ColorSpace::Rec2020\n>> + * \\brief A constant representing the Rec.2020 color space\n>> + */\n>> +\n>> +/**\n>> + * \\brief Compare color spaces for equality\n>> + * \\return True if the two color spaces are identical, false otherwise\n>> + */\n>> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n>> +{\n>> +    return lhs.primaries == rhs.primaries &&\n>> +           lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n>> +           lhs.transferFunction == rhs.transferFunction &&\n>> +           lhs.range == rhs.range;\n>> +}\n>> +\n>> +/**\n>> + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n>> + * \\brief Compare color spaces for inequality\n>> + * \\return True if the two color spaces are not identical, false \n>> otherwise\n>> + */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 6727a777..e7371d20 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -8,6 +8,7 @@ libcamera_sources = files([\n>>       'camera_manager.cpp',\n>>       'camera_sensor.cpp',\n>>       'camera_sensor_properties.cpp',\n>> +    'color_space.cpp',\n>>       'controls.cpp',\n>>       'control_serializer.cpp',\n>>       'control_validator.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BE52ABDB13\n\tfor ;\n\tTue, 23 Nov 2021 14:41:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC5FE6036F;\n\tTue, 23 Nov 2021 15:41:47 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A96B160121\n\tfor ;\n\tTue, 23 Nov 2021 15:41:46 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 82621F95\n\tfor ;\n\tTue, 23 Nov 2021 15:41:45 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gUKpGFCd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637678506;\n\tbh=BIhpl6XAr9QL+Ot+o9XHWq2PzRE9SLX4x3rgC0h0t70=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=gUKpGFCdAU9ZbZtOsqEvipClmgag1DrCy6UyEMZVpb/nfqd9NwyqP0MZNXtUshnZy\n\t8Unh5vgRY0PQ7t5gjmMJwSm2oxWtTKpyQsauMZmF4wonwYDUQqt+jSA2bjGrrZp1G+\n\tUzlx7YuOFacE2QlvN+4fDwhZj9nU1Nvemrtb5o30=","To":"libcamera-devel@lists.libcamera.org","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>\n\t<95df5e7c-afd6-a718-e1e7-ca07f1cce50e@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 23 Nov 2021 20:11:41 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<95df5e7c-afd6-a718-e1e7-ca07f1cce50e@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21206,"web_url":"https://patchwork.libcamera.org/comment/21206/","msgid":"<20211124233705.abuojhhbclc5aynb@uno.localdomain>","date":"2021-11-24T23:37:05","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:\n> This class represents a color space by defining its color primaries,\n> YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> output is full or limited range.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> include/libcamera/color_space.h | 79 +++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 324 insertions(+)\n> create mode 100644 include/libcamera/color_space.h\n> create mode 100644 src/libcamera/color_space.cpp\n>\n> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> new file mode 100644\n> index 00000000..44ac077a\n> --- /dev/null\n> +++ b/include/libcamera/color_space.h\n> @@ -0,0 +1,79 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.h - color space definitions\n> + */\n> +\n> +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n> +#define __LIBCAMERA_COLOR_SPACE_H__\n\nYou can now use #pragma once.\nKieran has sent a series to move the whole codebase to use it\n\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class ColorSpace\n> +{\n> +public:\n> +\tenum class Primaries : int {\n> +\t\tRaw,\n> +\t\tSmpte170m,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class YcbcrEncoding : int {\n> +\t\tRec601,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class TransferFunction : int {\n> +\t\tLinear,\n> +\t\tSrgb,\n> +\t\tRec709,\n> +\t};\n> +\n> +\tenum class Range : int {\n> +\t\tFull,\n> +\t\tLimited,\n> +\t};\n\nint or unsigned int ?\n\n> +\n> +\tconstexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> +\t\t: primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> +\t{\n> +\t}\n> +\n> +\tstatic const ColorSpace Raw;\n> +\tstatic const ColorSpace Jpeg;\n> +\tstatic const ColorSpace Srgb;\n> +\tstatic const ColorSpace Smpte170m;\n> +\tstatic const ColorSpace Rec709;\n> +\tstatic const ColorSpace Rec2020;\n> +\n> +\tPrimaries primaries;\n> +\tYcbcrEncoding ycbcrEncoding;\n> +\tTransferFunction transferFunction;\n> +\tRange range;\n> +\n> +\tconst std::string toString() const;\n> +\tstatic const std::string toString(const std::optional &colorSpace);\n> +};\n> +\n> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n> +\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn !(lhs == rhs);\n> +}\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 7155ff20..131e1740 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> libcamera_public_headers = files([\n> 'camera.h',\n> 'camera_manager.h',\n> + 'color_space.h',\n> 'compiler.h',\n> 'controls.h',\n> 'file_descriptor.h',\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> new file mode 100644\n> index 00000000..1a5fc7a2\n> --- /dev/null\n> +++ b/src/libcamera/color_space.cpp\n> @@ -0,0 +1,243 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.cpp - color spaces.\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n\nDo you need to include for std::pair\n\n> +\n> +/**\n> + * \\file color_space.h\n> + * \\brief Class and enums to represent color spaces\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class ColorSpace\n> + * \\brief Class to describe a color space\n> + *\n> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> + * the transfer function associated with the color space, and the range\n> + * (sometimes also referred to as the quantisation) of the color space.\n> + *\n> + * Certain combinations of these fields form well-known standard color\n> + * spaces such as \"JPEG\" or \"REC709\".\n> + *\n> + * In the strictest sense a \"color space\" formally only refers to the\n> + * color primaries and white point. Here, however, the ColorSpace class\n> + * adopts the common broader usage that includes the transfer function,\n> + * Y'CbCr encoding method and quantisation.\n> + *\n> + * For more information on the specific color spaces described here, please\n> + * see:\n> + *\n> + * sRGB\n> + * JPEG\n> + * SMPTE 170M\n> + * Rec.709\n> + * Rec.2020\n\nPlease use links from html/latest/\n\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Primaries\n> + * \\brief The color primaries for this color space\n> + *\n\nI'm debated if this documentation should provide an overview of what\neach component is or it is better to assume knowledges about that and\ndo not say anything.\n\nOn one side, we cannot cover the whole knowledge required, and we'll\nend up repeating the existing probably. On the other side,\ndocumentation will feel a bit incomplete for who has not previous\nbackground. But I understand we cannot document the whole world\nknowledge so I think it's fair assuming background, even if this is an\napplication facing API which application developers will have to deal\nwith.\n\n> + * \\var ColorSpace::Primaries::Raw\n> + * \\brief These are raw colors directly from a sensor\n> + * \\var ColorSpace::Primaries::Smpte170m\n> + * \\brief SMPTE 170M color primaries\n\nWhat about a blank line between these \\var+\\brief entries ?\n\n> + * \\var ColorSpace::Primaries::Rec709\n> + * \\brief Rec.709 color primaries\n> + * \\var ColorSpace::Primaries::Rec2020\n> + * \\brief Rec.2020 color primaries\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::YcbcrEncoding\n> + * \\brief The Y'CbCr encoding\n\nSame feeling I had before regarding documentation...\n\nI suspect application developers might want to know more, as we are\ngoing to 'force' them to care about color spaces..\n\n> + *\n> + * \\var ColorSpace::YcbcrEncoding::Rec601\n> + * \\brief Rec.601 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec709\n> + * \\brief Rec.709 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> + * \\brief Rec.2020 Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::TransferFunction\n> + * \\brief The transfer function used for this color space\n> + *\n> + * \\var ColorSpace::TransferFunction::Linear\n> + * \\brief This color space uses a linear (identity) transfer function\n> + * \\var ColorSpace::TransferFunction::Srgb\n> + * \\brief sRGB transfer function\n> + * \\var ColorSpace::TransferFunction::Rec709\n> + * \\brief Rec.709 transfer function\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Range\n> + * \\brief The range (sometimes \"quantisation\") for this color space\n> + *\n> + * \\var ColorSpace::Range::Full\n> + * \\brief This color space uses full range pixel values\n\nDoes the 'full' reange changes with bitdepth as for Limited :)\n\n> + * \\var ColorSpace::Range::Limited\n> + * \\brief This color space uses limited range pixel values, being\n> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)\n> + * \\brief Construct a ColorSpace from explicit values\n> + * \\param[in] p The color primaries\n> + * \\param[in] e The Y'CbCr encoding\n> + * \\param[in] t The transfer function for the color space\n> + * \\param[in] r The range of the pixel values in this color space\n> + */\n> +\n> +/**\n> + * \\brief Assemble and return a readable string representation of the\n> + * ColorSpace\n> + * \\return A string describing the ColorSpace\n> + */\n> +const std::string ColorSpace::toString() const\n> +{\n> +\t/* Print out a brief name only for standard color sapces. */\n> +\n> +\tstatic const std::vector> colorSpaceNames = {\n> +\t\t{ ColorSpace::Raw, \"Raw\" },\n> +\t\t{ ColorSpace::Jpeg, \"Jpeg\" },\n> +\t\t{ ColorSpace::Srgb, \"Srgb\" },\n> +\t\t{ ColorSpace::Smpte170m, \"Smpte170m\" },\n> +\t\t{ ColorSpace::Rec709, \"Rec709\" },\n> +\t\t{ ColorSpace::Rec2020, \"Rec2020\" },\n> +\t};\n\nThis really looks like a map :)\n\n> +\tauto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> +\t\t\t [this](const auto &item) {\n> +\t\t\t\t return *this == item.first;\n> +\t\t\t });\n> +\tif (it != colorSpaceNames.end())\n> +\t\treturn std::string(it->second);\n> +\n> +\tstatic const char *primariesNames[] = {\n> +\t\t\"Raw\",\n> +\t\t\"Smpte170m\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n\nI was expecting a warning caused by \"converting a string constant to\nchar *\"\n\n> +\tstatic const char *encodingNames[] = {\n> +\t\t\"Rec601\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n> +\tstatic const char *transferFunctionNames[] = {\n> +\t\t\"Linear\",\n> +\t\t\"Srgb\",\n> +\t\t\"Rec709\",\n> +\t};\n> +\tstatic const char *rangeNames[] = {\n> +\t\t\"Full\",\n> +\t\t\"Limited\",\n> +\t};\n\nThis could be made a bit more C++-ish by using std::array and\nstd::string, but as far as I can tell using a container frowns up\nmaking this a constexpr (soemthing you could do with raw arrays\nthough)\n\nThis is also slightly fragile as it requires care to maintain the vectors\nand the enums in sync. Maps are probably more expensive in terms of\nmemory occupation but forces the association to be explicitely\nmaintained.\n\n> +\n> +\tstd::stringstream ss;\n> +\tss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> +\t << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> +\t << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> +\t << std::string(rangeNames[static_cast(range)]);\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +/**\n> + * \\brief Assemble and return a readable string representation of an\n> + * optional ColorSpace\n> + * \\return A string describing the optional ColorSpace\n> + */\n> +const std::string ColorSpace::toString(const std::optional &colorSpace)\n> +{\n> +\tif (!colorSpace)\n> +\t\treturn \"Unknown\";\n> +\n> +\treturn colorSpace->toString();\n> +}\n\nHow do you immagine this to be used. Why would a caller use this\nfunction on a !colorspace ? I would have ASSERT(colorspace) :)\n\n> +\n> +/**\n> + * \\var ColorSpace::primaries\n> + * \\brief The color primaries\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::ycbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::transferFunction\n> + * \\brief The transfer function for this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::range\n> + * \\brief The pixel range used by this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Undefined\n> + * \\brief A constant representing a fully undefined color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Raw\n> + * \\brief A constant representing a raw color space (from a sensor)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Jpeg\n> + * \\brief A constant representing the JPEG color space used for\n> + * encoding JPEG images (and regarded as being the same as the sRGB\n> + * color space)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Smpte170m\n> + * \\brief A constant representing the SMPTE170M color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec709\n> + * \\brief A constant representing the Rec.709 color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec2020\n> + * \\brief A constant representing the Rec.2020 color space\n> + */\n> +\n> +/**\n> + * \\brief Compare color spaces for equality\n> + * \\return True if the two color spaces are identical, false otherwise\n> + */\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn lhs.primaries == rhs.primaries &&\n> +\t lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> +\t lhs.transferFunction == rhs.transferFunction &&\n> +\t lhs.range == rhs.range;\n> +}\n> +\n> +/**\n> + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> + * \\brief Compare color spaces for inequality\n> + * \\return True if the two color spaces are not identical, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777..e7371d20 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -8,6 +8,7 @@ libcamera_sources = files([\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> 'camera_sensor_properties.cpp',\n> + 'color_space.cpp',\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> --\n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BFD58BDB13\n\tfor ;\n\tWed, 24 Nov 2021 23:36:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26EE26038A;\n\tThu, 25 Nov 2021 00:36:15 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7913060228\n\tfor ;\n\tThu, 25 Nov 2021 00:36:14 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id EC9401BF203;\n\tWed, 24 Nov 2021 23:36:12 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 00:37:05 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211124233705.abuojhhbclc5aynb@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211118151933.15627-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\thverkuil-cisco@xs4all.nl","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21208,"web_url":"https://patchwork.libcamera.org/comment/21208/","msgid":"<20211124234033.q3mednt6eten2jcj@uno.localdomain>","date":"2021-11-24T23:40:33","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Sorry, forgot one thing\n\nOn Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:\n> This class represents a color space by defining its color primaries,\n> YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> output is full or limited range.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> include/libcamera/color_space.h | 79 +++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 324 insertions(+)\n> create mode 100644 include/libcamera/color_space.h\n> create mode 100644 src/libcamera/color_space.cpp\n>\n> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> new file mode 100644\n> index 00000000..44ac077a\n> --- /dev/null\n> +++ b/include/libcamera/color_space.h\n> @@ -0,0 +1,79 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.h - color space definitions\n> + */\n> +\n> +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n> +#define __LIBCAMERA_COLOR_SPACE_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class ColorSpace\n> +{\n> +public:\n> +\tenum class Primaries : int {\n> +\t\tRaw,\n> +\t\tSmpte170m,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class YcbcrEncoding : int {\n> +\t\tRec601,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class TransferFunction : int {\n> +\t\tLinear,\n> +\t\tSrgb,\n> +\t\tRec709,\n> +\t};\n> +\n> +\tenum class Range : int {\n> +\t\tFull,\n> +\t\tLimited,\n> +\t};\n> +\n> +\tconstexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> +\t\t: primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> +\t{\n> +\t}\n> +\n> +\tstatic const ColorSpace Raw;\n> +\tstatic const ColorSpace Jpeg;\n> +\tstatic const ColorSpace Srgb;\n> +\tstatic const ColorSpace Smpte170m;\n> +\tstatic const ColorSpace Rec709;\n> +\tstatic const ColorSpace Rec2020;\n> +\n> +\tPrimaries primaries;\n> +\tYcbcrEncoding ycbcrEncoding;\n> +\tTransferFunction transferFunction;\n> +\tRange range;\n> +\n> +\tconst std::string toString() const;\n> +\tstatic const std::string toString(const std::optional &colorSpace);\n> +};\n> +\n> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n\nAs commented on the previous version\n\nReading this\nhttps://isocpp.org/blog/2018/05/quick-q-use-of-constexpr-in-header-file\n\nmy understanding is that each translation unit including color_space.h\nwill have its own copy of these instances.\n\nShouldn't you declare them as 'extern const' and define them in the\n.cpp file as we do for controls, or since we're now with C++17 use the\n'inline' keyword that allows you to define them in the header with\nautomatic external linkage ?\n\nhttps://en.cppreference.com/w/cpp/language/inline\n\n> +\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn !(lhs == rhs);\n> +}\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 7155ff20..131e1740 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> libcamera_public_headers = files([\n> 'camera.h',\n> 'camera_manager.h',\n> + 'color_space.h',\n> 'compiler.h',\n> 'controls.h',\n> 'file_descriptor.h',\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> new file mode 100644\n> index 00000000..1a5fc7a2\n> --- /dev/null\n> +++ b/src/libcamera/color_space.cpp\n> @@ -0,0 +1,243 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.cpp - color spaces.\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +/**\n> + * \\file color_space.h\n> + * \\brief Class and enums to represent color spaces\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class ColorSpace\n> + * \\brief Class to describe a color space\n> + *\n> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> + * the transfer function associated with the color space, and the range\n> + * (sometimes also referred to as the quantisation) of the color space.\n> + *\n> + * Certain combinations of these fields form well-known standard color\n> + * spaces such as \"JPEG\" or \"REC709\".\n> + *\n> + * In the strictest sense a \"color space\" formally only refers to the\n> + * color primaries and white point. Here, however, the ColorSpace class\n> + * adopts the common broader usage that includes the transfer function,\n> + * Y'CbCr encoding method and quantisation.\n> + *\n> + * For more information on the specific color spaces described here, please\n> + * see:\n> + *\n> + * sRGB\n> + * JPEG\n> + * SMPTE 170M\n> + * Rec.709\n> + * Rec.2020\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Primaries\n> + * \\brief The color primaries for this color space\n> + *\n> + * \\var ColorSpace::Primaries::Raw\n> + * \\brief These are raw colors directly from a sensor\n> + * \\var ColorSpace::Primaries::Smpte170m\n> + * \\brief SMPTE 170M color primaries\n> + * \\var ColorSpace::Primaries::Rec709\n> + * \\brief Rec.709 color primaries\n> + * \\var ColorSpace::Primaries::Rec2020\n> + * \\brief Rec.2020 color primaries\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::YcbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + *\n> + * \\var ColorSpace::YcbcrEncoding::Rec601\n> + * \\brief Rec.601 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec709\n> + * \\brief Rec.709 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> + * \\brief Rec.2020 Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::TransferFunction\n> + * \\brief The transfer function used for this color space\n> + *\n> + * \\var ColorSpace::TransferFunction::Linear\n> + * \\brief This color space uses a linear (identity) transfer function\n> + * \\var ColorSpace::TransferFunction::Srgb\n> + * \\brief sRGB transfer function\n> + * \\var ColorSpace::TransferFunction::Rec709\n> + * \\brief Rec.709 transfer function\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Range\n> + * \\brief The range (sometimes \"quantisation\") for this color space\n> + *\n> + * \\var ColorSpace::Range::Full\n> + * \\brief This color space uses full range pixel values\n> + * \\var ColorSpace::Range::Limited\n> + * \\brief This color space uses limited range pixel values, being\n> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)\n> + * \\brief Construct a ColorSpace from explicit values\n> + * \\param[in] p The color primaries\n> + * \\param[in] e The Y'CbCr encoding\n> + * \\param[in] t The transfer function for the color space\n> + * \\param[in] r The range of the pixel values in this color space\n> + */\n> +\n> +/**\n> + * \\brief Assemble and return a readable string representation of the\n> + * ColorSpace\n> + * \\return A string describing the ColorSpace\n> + */\n> +const std::string ColorSpace::toString() const\n> +{\n> +\t/* Print out a brief name only for standard color sapces. */\n> +\n> +\tstatic const std::vector> colorSpaceNames = {\n> +\t\t{ ColorSpace::Raw, \"Raw\" },\n> +\t\t{ ColorSpace::Jpeg, \"Jpeg\" },\n> +\t\t{ ColorSpace::Srgb, \"Srgb\" },\n> +\t\t{ ColorSpace::Smpte170m, \"Smpte170m\" },\n> +\t\t{ ColorSpace::Rec709, \"Rec709\" },\n> +\t\t{ ColorSpace::Rec2020, \"Rec2020\" },\n> +\t};\n> +\tauto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> +\t\t\t [this](const auto &item) {\n> +\t\t\t\t return *this == item.first;\n> +\t\t\t });\n> +\tif (it != colorSpaceNames.end())\n> +\t\treturn std::string(it->second);\n> +\n> +\tstatic const char *primariesNames[] = {\n> +\t\t\"Raw\",\n> +\t\t\"Smpte170m\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n> +\tstatic const char *encodingNames[] = {\n> +\t\t\"Rec601\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n> +\tstatic const char *transferFunctionNames[] = {\n> +\t\t\"Linear\",\n> +\t\t\"Srgb\",\n> +\t\t\"Rec709\",\n> +\t};\n> +\tstatic const char *rangeNames[] = {\n> +\t\t\"Full\",\n> +\t\t\"Limited\",\n> +\t};\n> +\n> +\tstd::stringstream ss;\n> +\tss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> +\t << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> +\t << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> +\t << std::string(rangeNames[static_cast(range)]);\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +/**\n> + * \\brief Assemble and return a readable string representation of an\n> + * optional ColorSpace\n> + * \\return A string describing the optional ColorSpace\n> + */\n> +const std::string ColorSpace::toString(const std::optional &colorSpace)\n> +{\n> +\tif (!colorSpace)\n> +\t\treturn \"Unknown\";\n> +\n> +\treturn colorSpace->toString();\n> +}\n> +\n> +/**\n> + * \\var ColorSpace::primaries\n> + * \\brief The color primaries\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::ycbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::transferFunction\n> + * \\brief The transfer function for this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::range\n> + * \\brief The pixel range used by this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Undefined\n> + * \\brief A constant representing a fully undefined color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Raw\n> + * \\brief A constant representing a raw color space (from a sensor)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Jpeg\n> + * \\brief A constant representing the JPEG color space used for\n> + * encoding JPEG images (and regarded as being the same as the sRGB\n> + * color space)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Smpte170m\n> + * \\brief A constant representing the SMPTE170M color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec709\n> + * \\brief A constant representing the Rec.709 color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec2020\n> + * \\brief A constant representing the Rec.2020 color space\n> + */\n> +\n> +/**\n> + * \\brief Compare color spaces for equality\n> + * \\return True if the two color spaces are identical, false otherwise\n> + */\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn lhs.primaries == rhs.primaries &&\n> +\t lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> +\t lhs.transferFunction == rhs.transferFunction &&\n> +\t lhs.range == rhs.range;\n> +}\n> +\n> +/**\n> + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> + * \\brief Compare color spaces for inequality\n> + * \\return True if the two color spaces are not identical, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777..e7371d20 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -8,6 +8,7 @@ libcamera_sources = files([\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> 'camera_sensor_properties.cpp',\n> + 'color_space.cpp',\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> --\n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CF1B3BDB13\n\tfor ;\n\tWed, 24 Nov 2021 23:39:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9107A60371;\n\tThu, 25 Nov 2021 00:39:43 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E55060228\n\tfor ;\n\tThu, 25 Nov 2021 00:39:42 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 211A41BF206;\n\tWed, 24 Nov 2021 23:39:40 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 00:40:33 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211124234033.q3mednt6eten2jcj@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211118151933.15627-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\thverkuil-cisco@xs4all.nl","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21233,"web_url":"https://patchwork.libcamera.org/comment/21233/","msgid":"","date":"2021-11-25T11:26:27","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the review!\n\nOn Wed, 24 Nov 2021 at 23:39, Jacopo Mondi wrote:\n>\n> Sorry, forgot one thing\n>\n> On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:\n> > This class represents a color space by defining its color primaries,\n> > YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> > output is full or limited range.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > include/libcamera/color_space.h | 79 +++++++++++\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 324 insertions(+)\n> > create mode 100644 include/libcamera/color_space.h\n> > create mode 100644 src/libcamera/color_space.cpp\n> >\n> > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> > new file mode 100644\n> > index 00000000..44ac077a\n> > --- /dev/null\n> > +++ b/include/libcamera/color_space.h\n> > @@ -0,0 +1,79 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > + *\n> > + * color_space.h - color space definitions\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n> > +#define __LIBCAMERA_COLOR_SPACE_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class ColorSpace\n> > +{\n> > +public:\n> > + enum class Primaries : int {\n> > + Raw,\n> > + Smpte170m,\n> > + Rec709,\n> > + Rec2020,\n> > + };\n> > +\n> > + enum class YcbcrEncoding : int {\n> > + Rec601,\n> > + Rec709,\n> > + Rec2020,\n> > + };\n> > +\n> > + enum class TransferFunction : int {\n> > + Linear,\n> > + Srgb,\n> > + Rec709,\n> > + };\n> > +\n> > + enum class Range : int {\n> > + Full,\n> > + Limited,\n> > + };\n> > +\n> > + constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> > + : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> > + {\n> > + }\n> > +\n> > + static const ColorSpace Raw;\n> > + static const ColorSpace Jpeg;\n> > + static const ColorSpace Srgb;\n> > + static const ColorSpace Smpte170m;\n> > + static const ColorSpace Rec709;\n> > + static const ColorSpace Rec2020;\n> > +\n> > + Primaries primaries;\n> > + YcbcrEncoding ycbcrEncoding;\n> > + TransferFunction transferFunction;\n> > + Range range;\n> > +\n> > + const std::string toString() const;\n> > + static const std::string toString(const std::optional &colorSpace);\n> > +};\n> > +\n> > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n> > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n>\n> As commented on the previous version\n>\n> Reading this\n> https://isocpp.org/blog/2018/05/quick-q-use-of-constexpr-in-header-file\n>\n> my understanding is that each translation unit including color_space.h\n> will have its own copy of these instances.\n>\n> Shouldn't you declare them as 'extern const' and define them in the\n> .cpp file as we do for controls, or since we're now with C++17 use the\n> 'inline' keyword that allows you to define them in the header with\n> automatic external linkage ?\n>\n> https://en.cppreference.com/w/cpp/language/inline\n\nI guess I wanted to leave them as constexpr, but I agree it's all a\nbit ugly. Happy to move them into the cpp file.\n\nThanks!\nDavid\n\n>\n> > +\n> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > +{\n> > + return !(lhs == rhs);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 7155ff20..131e1740 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> > libcamera_public_headers = files([\n> > 'camera.h',\n> > 'camera_manager.h',\n> > + 'color_space.h',\n> > 'compiler.h',\n> > 'controls.h',\n> > 'file_descriptor.h',\n> > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> > new file mode 100644\n> > index 00000000..1a5fc7a2\n> > --- /dev/null\n> > +++ b/src/libcamera/color_space.cpp\n> > @@ -0,0 +1,243 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > + *\n> > + * color_space.cpp - color spaces.\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +/**\n> > + * \\file color_space.h\n> > + * \\brief Class and enums to represent color spaces\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class ColorSpace\n> > + * \\brief Class to describe a color space\n> > + *\n> > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> > + * the transfer function associated with the color space, and the range\n> > + * (sometimes also referred to as the quantisation) of the color space.\n> > + *\n> > + * Certain combinations of these fields form well-known standard color\n> > + * spaces such as \"JPEG\" or \"REC709\".\n> > + *\n> > + * In the strictest sense a \"color space\" formally only refers to the\n> > + * color primaries and white point. Here, however, the ColorSpace class\n> > + * adopts the common broader usage that includes the transfer function,\n> > + * Y'CbCr encoding method and quantisation.\n> > + *\n> > + * For more information on the specific color spaces described here, please\n> > + * see:\n> > + *\n> > + * sRGB\n> > + * JPEG\n> > + * SMPTE 170M\n> > + * Rec.709\n> > + * Rec.2020\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::Primaries\n> > + * \\brief The color primaries for this color space\n> > + *\n> > + * \\var ColorSpace::Primaries::Raw\n> > + * \\brief These are raw colors directly from a sensor\n> > + * \\var ColorSpace::Primaries::Smpte170m\n> > + * \\brief SMPTE 170M color primaries\n> > + * \\var ColorSpace::Primaries::Rec709\n> > + * \\brief Rec.709 color primaries\n> > + * \\var ColorSpace::Primaries::Rec2020\n> > + * \\brief Rec.2020 color primaries\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::YcbcrEncoding\n> > + * \\brief The Y'CbCr encoding\n> > + *\n> > + * \\var ColorSpace::YcbcrEncoding::Rec601\n> > + * \\brief Rec.601 Y'CbCr encoding\n> > + * \\var ColorSpace::YcbcrEncoding::Rec709\n> > + * \\brief Rec.709 Y'CbCr encoding\n> > + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> > + * \\brief Rec.2020 Y'CbCr encoding\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::TransferFunction\n> > + * \\brief The transfer function used for this color space\n> > + *\n> > + * \\var ColorSpace::TransferFunction::Linear\n> > + * \\brief This color space uses a linear (identity) transfer function\n> > + * \\var ColorSpace::TransferFunction::Srgb\n> > + * \\brief sRGB transfer function\n> > + * \\var ColorSpace::TransferFunction::Rec709\n> > + * \\brief Rec.709 transfer function\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::Range\n> > + * \\brief The range (sometimes \"quantisation\") for this color space\n> > + *\n> > + * \\var ColorSpace::Range::Full\n> > + * \\brief This color space uses full range pixel values\n> > + * \\var ColorSpace::Range::Limited\n> > + * \\brief This color space uses limited range pixel values, being\n> > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> > + */\n> > +\n> > +/**\n> > + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)\n> > + * \\brief Construct a ColorSpace from explicit values\n> > + * \\param[in] p The color primaries\n> > + * \\param[in] e The Y'CbCr encoding\n> > + * \\param[in] t The transfer function for the color space\n> > + * \\param[in] r The range of the pixel values in this color space\n> > + */\n> > +\n> > +/**\n> > + * \\brief Assemble and return a readable string representation of the\n> > + * ColorSpace\n> > + * \\return A string describing the ColorSpace\n> > + */\n> > +const std::string ColorSpace::toString() const\n> > +{\n> > + /* Print out a brief name only for standard color sapces. */\n> > +\n> > + static const std::vector> colorSpaceNames = {\n> > + { ColorSpace::Raw, \"Raw\" },\n> > + { ColorSpace::Jpeg, \"Jpeg\" },\n> > + { ColorSpace::Srgb, \"Srgb\" },\n> > + { ColorSpace::Smpte170m, \"Smpte170m\" },\n> > + { ColorSpace::Rec709, \"Rec709\" },\n> > + { ColorSpace::Rec2020, \"Rec2020\" },\n> > + };\n> > + auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> > + [this](const auto &item) {\n> > + return *this == item.first;\n> > + });\n> > + if (it != colorSpaceNames.end())\n> > + return std::string(it->second);\n> > +\n> > + static const char *primariesNames[] = {\n> > + \"Raw\",\n> > + \"Smpte170m\",\n> > + \"Rec709\",\n> > + \"Rec2020\",\n> > + };\n> > + static const char *encodingNames[] = {\n> > + \"Rec601\",\n> > + \"Rec709\",\n> > + \"Rec2020\",\n> > + };\n> > + static const char *transferFunctionNames[] = {\n> > + \"Linear\",\n> > + \"Srgb\",\n> > + \"Rec709\",\n> > + };\n> > + static const char *rangeNames[] = {\n> > + \"Full\",\n> > + \"Limited\",\n> > + };\n> > +\n> > + std::stringstream ss;\n> > + ss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> > + << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> > + << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> > + << std::string(rangeNames[static_cast(range)]);\n> > +\n> > + return ss.str();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Assemble and return a readable string representation of an\n> > + * optional ColorSpace\n> > + * \\return A string describing the optional ColorSpace\n> > + */\n> > +const std::string ColorSpace::toString(const std::optional &colorSpace)\n> > +{\n> > + if (!colorSpace)\n> > + return \"Unknown\";\n> > +\n> > + return colorSpace->toString();\n> > +}\n> > +\n> > +/**\n> > + * \\var ColorSpace::primaries\n> > + * \\brief The color primaries\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::ycbcrEncoding\n> > + * \\brief The Y'CbCr encoding\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::transferFunction\n> > + * \\brief The transfer function for this color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::range\n> > + * \\brief The pixel range used by this color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Undefined\n> > + * \\brief A constant representing a fully undefined color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Raw\n> > + * \\brief A constant representing a raw color space (from a sensor)\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Jpeg\n> > + * \\brief A constant representing the JPEG color space used for\n> > + * encoding JPEG images (and regarded as being the same as the sRGB\n> > + * color space)\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Smpte170m\n> > + * \\brief A constant representing the SMPTE170M color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Rec709\n> > + * \\brief A constant representing the Rec.709 color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Rec2020\n> > + * \\brief A constant representing the Rec.2020 color space\n> > + */\n> > +\n> > +/**\n> > + * \\brief Compare color spaces for equality\n> > + * \\return True if the two color spaces are identical, false otherwise\n> > + */\n> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> > +{\n> > + return lhs.primaries == rhs.primaries &&\n> > + lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> > + lhs.transferFunction == rhs.transferFunction &&\n> > + lhs.range == rhs.range;\n> > +}\n> > +\n> > +/**\n> > + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > + * \\brief Compare color spaces for inequality\n> > + * \\return True if the two color spaces are not identical, false otherwise\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 6727a777..e7371d20 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -8,6 +8,7 @@ libcamera_sources = files([\n> > 'camera_manager.cpp',\n> > 'camera_sensor.cpp',\n> > 'camera_sensor_properties.cpp',\n> > + 'color_space.cpp',\n> > 'controls.cpp',\n> > 'control_serializer.cpp',\n> > 'control_validator.cpp',\n> > --\n> > 2.20.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 74444BF415\n\tfor ;\n\tThu, 25 Nov 2021 11:26:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE3DE60376;\n\tThu, 25 Nov 2021 12:26:40 +0100 (CET)","from mail-wm1-x332.google.com (mail-wm1-x332.google.com\n\t[IPv6:2a00:1450:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BAB760231\n\tfor ;\n\tThu, 25 Nov 2021 12:26:39 +0100 (CET)","by mail-wm1-x332.google.com with SMTP id\n\tj140-20020a1c2392000000b003399ae48f58so8027158wmj.5\n\tfor ;\n\tThu, 25 Nov 2021 03:26:39 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"eb4dDYn4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=vuiAHBaEGnxd4gNFhmGRy8FMSRLbWkRjNyg5z0gLSBg=;\n\tb=eb4dDYn4v8BONT2oqNurujcgp34maZ9s48ebJk1OUzBp8vAlXp8MvdRxrdgKj2x40o\n\tm64q6Agaz0Zh9VP6sGmF/qteCDC/AOH3Vs7HR5+gBO0ZssrllTXs9iNJdL79+uNEOsZ4\n\t6YiqX/tihkVdqwBYnFYo4kwev7fJ8CQpK8YumtZE1AWasWo7Nh3imdRY7SMoOrfddXyM\n\tAmvMu4DjQFdOU4JTHqxTuL/eUrrIagwLNPRcLIydb84XaBnLoq1l3zZbXoKczTqqxRhV\n\tbi5w3VsPUqLMNruZ1XDw5ZnNPaP50PuM7spo5zWkoYo9hay7KVp+L6+8xrrH9IseHjPW\n\tCLIg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=vuiAHBaEGnxd4gNFhmGRy8FMSRLbWkRjNyg5z0gLSBg=;\n\tb=NIBCa7C5BoSotjYgrvAE/xQMwyLICUFgb0ehalAloNPlLcnpM07RDVikx54zzcshd2\n\thiBbR6HPgdAa31uhGrX5XS4K8MrnrTXYzR1uJVhqPzM5OprORCfwkx6XNRx3FPb7VES+\n\tj/44rh7cTQWs+xKg3qfllzb48hjRvWX5HOF7sI3ZKQZmtDvh9I/2NApLRywPssyonTpx\n\tfsHFvR/CBA4MXZp/vetq6Gi7EGq3PlcnqK53mywTiAR3okR8lGQFrjv9GamyrLG/04st\n\tthKDEH87TgJGdWl4SkzzcetAPqrm78RPTgj5ns5UmqpicMrwoV+j8Sn4iMC4Wpq/zvw+\n\t9Odw==","X-Gm-Message-State":"AOAM530zxAS2i7K+KxZkJd2oEXS97m0MaaAw8s8I2B7SWe/AWkUsfe6h\n\tKSnr1VxUP7l8EcX8MtrBsbFFvwS/FgZe0wzrD4cbwg==","X-Google-Smtp-Source":"ABdhPJxkv48eNAj92Vb0WwT8WuHvmeifdIodzF7DfrgufAsqUfxhDuA7KxZpAz32OY5t4YVPL5VprCQiKQJLs9bMMUM=","X-Received":"by 2002:a7b:ca54:: with SMTP id m20mr6191663wml.21.1637839598993;\n\tThu, 25 Nov 2021 03:26:38 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>\n\t<20211124234033.q3mednt6eten2jcj@uno.localdomain>","In-Reply-To":"<20211124234033.q3mednt6eten2jcj@uno.localdomain>","From":"David Plowman ","Date":"Thu, 25 Nov 2021 11:26:27 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21237,"web_url":"https://patchwork.libcamera.org/comment/21237/","msgid":"","date":"2021-11-25T11:54:04","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for reviewing this!\n\nOn Wed, 24 Nov 2021 at 23:36, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:\n> > This class represents a color space by defining its color primaries,\n> > YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> > output is full or limited range.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > include/libcamera/color_space.h | 79 +++++++++++\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 324 insertions(+)\n> > create mode 100644 include/libcamera/color_space.h\n> > create mode 100644 src/libcamera/color_space.cpp\n> >\n> > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> > new file mode 100644\n> > index 00000000..44ac077a\n> > --- /dev/null\n> > +++ b/include/libcamera/color_space.h\n> > @@ -0,0 +1,79 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > + *\n> > + * color_space.h - color space definitions\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n> > +#define __LIBCAMERA_COLOR_SPACE_H__\n>\n> You can now use #pragma once.\n> Kieran has sent a series to move the whole codebase to use it\n\nAh, very nice.\n\n>\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class ColorSpace\n> > +{\n> > +public:\n> > + enum class Primaries : int {\n> > + Raw,\n> > + Smpte170m,\n> > + Rec709,\n> > + Rec2020,\n> > + };\n> > +\n> > + enum class YcbcrEncoding : int {\n> > + Rec601,\n> > + Rec709,\n> > + Rec2020,\n> > + };\n> > +\n> > + enum class TransferFunction : int {\n> > + Linear,\n> > + Srgb,\n> > + Rec709,\n> > + };\n> > +\n> > + enum class Range : int {\n> > + Full,\n> > + Limited,\n> > + };\n>\n> int or unsigned int ?\n\nI think I've always just used int in the past but don't really mind.\nAnyone feel strongly?\n\n>\n> > +\n> > + constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> > + : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> > + {\n> > + }\n> > +\n> > + static const ColorSpace Raw;\n> > + static const ColorSpace Jpeg;\n> > + static const ColorSpace Srgb;\n> > + static const ColorSpace Smpte170m;\n> > + static const ColorSpace Rec709;\n> > + static const ColorSpace Rec2020;\n> > +\n> > + Primaries primaries;\n> > + YcbcrEncoding ycbcrEncoding;\n> > + TransferFunction transferFunction;\n> > + Range range;\n> > +\n> > + const std::string toString() const;\n> > + static const std::string toString(const std::optional &colorSpace);\n> > +};\n> > +\n> > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n> > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n> > +\n> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > +{\n> > + return !(lhs == rhs);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 7155ff20..131e1740 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> > libcamera_public_headers = files([\n> > 'camera.h',\n> > 'camera_manager.h',\n> > + 'color_space.h',\n> > 'compiler.h',\n> > 'controls.h',\n> > 'file_descriptor.h',\n> > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> > new file mode 100644\n> > index 00000000..1a5fc7a2\n> > --- /dev/null\n> > +++ b/src/libcamera/color_space.cpp\n> > @@ -0,0 +1,243 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > + *\n> > + * color_space.cpp - color spaces.\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n>\n> Do you need to include for std::pair\n\nProbably. Hard to spot when nothing complains...\n\n>\n> > +\n> > +/**\n> > + * \\file color_space.h\n> > + * \\brief Class and enums to represent color spaces\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class ColorSpace\n> > + * \\brief Class to describe a color space\n> > + *\n> > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> > + * the transfer function associated with the color space, and the range\n> > + * (sometimes also referred to as the quantisation) of the color space.\n> > + *\n> > + * Certain combinations of these fields form well-known standard color\n> > + * spaces such as \"JPEG\" or \"REC709\".\n> > + *\n> > + * In the strictest sense a \"color space\" formally only refers to the\n> > + * color primaries and white point. Here, however, the ColorSpace class\n> > + * adopts the common broader usage that includes the transfer function,\n> > + * Y'CbCr encoding method and quantisation.\n> > + *\n> > + * For more information on the specific color spaces described here, please\n> > + * see:\n> > + *\n> > + * sRGB\n> > + * JPEG\n> > + * SMPTE 170M\n> > + * Rec.709\n> > + * Rec.2020\n>\n> Please use links from html/latest/\n\nWhat is html/latest/ exactly?\n\n>\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::Primaries\n> > + * \\brief The color primaries for this color space\n> > + *\n>\n> I'm debated if this documentation should provide an overview of what\n> each component is or it is better to assume knowledges about that and\n> do not say anything.\n>\n> On one side, we cannot cover the whole knowledge required, and we'll\n> end up repeating the existing probably. On the other side,\n> documentation will feel a bit incomplete for who has not previous\n> background. But I understand we cannot document the whole world\n> knowledge so I think it's fair assuming background, even if this is an\n> application facing API which application developers will have to deal\n> with.\n\nThere has been some discussion of this before. I think I'd like to go\nwith the idea that we link to other references which explain it all\nmuch better than we are going to, rather than to duplicate everything.\n\n>\n> > + * \\var ColorSpace::Primaries::Raw\n> > + * \\brief These are raw colors directly from a sensor\n> > + * \\var ColorSpace::Primaries::Smpte170m\n> > + * \\brief SMPTE 170M color primaries\n>\n> What about a blank line between these \\var+\\brief entries ?\n\nHmm, the style checker didn't ask me to do that, but I can add them.\n\n>\n> > + * \\var ColorSpace::Primaries::Rec709\n> > + * \\brief Rec.709 color primaries\n> > + * \\var ColorSpace::Primaries::Rec2020\n> > + * \\brief Rec.2020 color primaries\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::YcbcrEncoding\n> > + * \\brief The Y'CbCr encoding\n>\n> Same feeling I had before regarding documentation...\n\nAs above, really. I think trying to explain this stuff here isn't the\nway to go, folks should read those links above which explain it pretty\nwell.\n\n>\n> I suspect application developers might want to know more, as we are\n> going to 'force' them to care about color spaces..\n\nBut we aren't \"forcing\" them! They can explicitly leave the ColorSpace\nunset (and bury their heads in the sand)... :)\n\n>\n> > + *\n> > + * \\var ColorSpace::YcbcrEncoding::Rec601\n> > + * \\brief Rec.601 Y'CbCr encoding\n> > + * \\var ColorSpace::YcbcrEncoding::Rec709\n> > + * \\brief Rec.709 Y'CbCr encoding\n> > + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> > + * \\brief Rec.2020 Y'CbCr encoding\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::TransferFunction\n> > + * \\brief The transfer function used for this color space\n> > + *\n> > + * \\var ColorSpace::TransferFunction::Linear\n> > + * \\brief This color space uses a linear (identity) transfer function\n> > + * \\var ColorSpace::TransferFunction::Srgb\n> > + * \\brief sRGB transfer function\n> > + * \\var ColorSpace::TransferFunction::Rec709\n> > + * \\brief Rec.709 transfer function\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::Range\n> > + * \\brief The range (sometimes \"quantisation\") for this color space\n> > + *\n> > + * \\var ColorSpace::Range::Full\n> > + * \\brief This color space uses full range pixel values\n>\n> Does the 'full' reange changes with bitdepth as for Limited :)\n>\n> > + * \\var ColorSpace::Range::Limited\n> > + * \\brief This color space uses limited range pixel values, being\n> > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> > + */\n> > +\n> > +/**\n> > + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)\n> > + * \\brief Construct a ColorSpace from explicit values\n> > + * \\param[in] p The color primaries\n> > + * \\param[in] e The Y'CbCr encoding\n> > + * \\param[in] t The transfer function for the color space\n> > + * \\param[in] r The range of the pixel values in this color space\n> > + */\n> > +\n> > +/**\n> > + * \\brief Assemble and return a readable string representation of the\n> > + * ColorSpace\n> > + * \\return A string describing the ColorSpace\n> > + */\n> > +const std::string ColorSpace::toString() const\n> > +{\n> > + /* Print out a brief name only for standard color sapces. */\n> > +\n> > + static const std::vector> colorSpaceNames = {\n> > + { ColorSpace::Raw, \"Raw\" },\n> > + { ColorSpace::Jpeg, \"Jpeg\" },\n> > + { ColorSpace::Srgb, \"Srgb\" },\n> > + { ColorSpace::Smpte170m, \"Smpte170m\" },\n> > + { ColorSpace::Rec709, \"Rec709\" },\n> > + { ColorSpace::Rec2020, \"Rec2020\" },\n> > + };\n>\n> This really looks like a map :)\n\nI know. I'm slightly loath to add an artificial ordering function just\nso that I can use a map, or even add a hashing function so that I can\nuse an unordered_map, when for smallish arrays it doesn't gain\nanything. But there may come a point where adding this extra stuff\nbecomes easier than explaining why there's no point in adding it!\n\n>\n> > + auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> > + [this](const auto &item) {\n> > + return *this == item.first;\n> > + });\n> > + if (it != colorSpaceNames.end())\n> > + return std::string(it->second);\n> > +\n> > + static const char *primariesNames[] = {\n> > + \"Raw\",\n> > + \"Smpte170m\",\n> > + \"Rec709\",\n> > + \"Rec2020\",\n> > + };\n>\n> I was expecting a warning caused by \"converting a string constant to\n> char *\"\n>\n> > + static const char *encodingNames[] = {\n> > + \"Rec601\",\n> > + \"Rec709\",\n> > + \"Rec2020\",\n> > + };\n> > + static const char *transferFunctionNames[] = {\n> > + \"Linear\",\n> > + \"Srgb\",\n> > + \"Rec709\",\n> > + };\n> > + static const char *rangeNames[] = {\n> > + \"Full\",\n> > + \"Limited\",\n> > + };\n>\n> This could be made a bit more C++-ish by using std::array and\n> std::string, but as far as I can tell using a container frowns up\n> making this a constexpr (soemthing you could do with raw arrays\n> though)\n>\n> This is also slightly fragile as it requires care to maintain the vectors\n> and the enums in sync. Maps are probably more expensive in terms of\n> memory occupation but forces the association to be explicitely\n> maintained.\n\nHonestly I don't know what's best. Perhaps it should check the values\ndon't run off the end of the tables. Anyone feel strongly about how to\ndo this?\n\n>\n> > +\n> > + std::stringstream ss;\n> > + ss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> > + << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> > + << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> > + << std::string(rangeNames[static_cast(range)]);\n> > +\n> > + return ss.str();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Assemble and return a readable string representation of an\n> > + * optional ColorSpace\n> > + * \\return A string describing the optional ColorSpace\n> > + */\n> > +const std::string ColorSpace::toString(const std::optional &colorSpace)\n> > +{\n> > + if (!colorSpace)\n> > + return \"Unknown\";\n> > +\n> > + return colorSpace->toString();\n> > +}\n>\n> How do you immagine this to be used. Why would a caller use this\n> function on a !colorspace ? I would have ASSERT(colorspace) :)\n\nThe catch is that our configuration and format classes contain a\nstd::optional, not a simple ColorSpace. So they would\nspend all their time doing this:\n\nLOG(XXX, Debug) << (colorSpace ? colorSpace.toString() : \"unknown\");\n\nwhereas I slightly prefer\n\nLOG(XXX, Debug) << ColorSpace::toString(colorSpace);\n\nDoes that make sense? Or would it be better somewhere else with some\nother kind of syntax?\n\nThanks!\nDavid\n\n>\n> > +\n> > +/**\n> > + * \\var ColorSpace::primaries\n> > + * \\brief The color primaries\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::ycbcrEncoding\n> > + * \\brief The Y'CbCr encoding\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::transferFunction\n> > + * \\brief The transfer function for this color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::range\n> > + * \\brief The pixel range used by this color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Undefined\n> > + * \\brief A constant representing a fully undefined color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Raw\n> > + * \\brief A constant representing a raw color space (from a sensor)\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Jpeg\n> > + * \\brief A constant representing the JPEG color space used for\n> > + * encoding JPEG images (and regarded as being the same as the sRGB\n> > + * color space)\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Smpte170m\n> > + * \\brief A constant representing the SMPTE170M color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Rec709\n> > + * \\brief A constant representing the Rec.709 color space\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::Rec2020\n> > + * \\brief A constant representing the Rec.2020 color space\n> > + */\n> > +\n> > +/**\n> > + * \\brief Compare color spaces for equality\n> > + * \\return True if the two color spaces are identical, false otherwise\n> > + */\n> > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> > +{\n> > + return lhs.primaries == rhs.primaries &&\n> > + lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> > + lhs.transferFunction == rhs.transferFunction &&\n> > + lhs.range == rhs.range;\n> > +}\n> > +\n> > +/**\n> > + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > + * \\brief Compare color spaces for inequality\n> > + * \\return True if the two color spaces are not identical, false otherwise\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 6727a777..e7371d20 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -8,6 +8,7 @@ libcamera_sources = files([\n> > 'camera_manager.cpp',\n> > 'camera_sensor.cpp',\n> > 'camera_sensor_properties.cpp',\n> > + 'color_space.cpp',\n> > 'controls.cpp',\n> > 'control_serializer.cpp',\n> > 'control_validator.cpp',\n> > --\n> > 2.20.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 89E0ABDB13\n\tfor ;\n\tThu, 25 Nov 2021 11:54:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 35F496036F;\n\tThu, 25 Nov 2021 12:54:17 +0100 (CET)","from mail-wr1-x430.google.com (mail-wr1-x430.google.com\n\t[IPv6:2a00:1450:4864:20::430])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17F2F60231\n\tfor ;\n\tThu, 25 Nov 2021 12:54:15 +0100 (CET)","by mail-wr1-x430.google.com with SMTP id o13so11005034wrs.12\n\tfor ;\n\tThu, 25 Nov 2021 03:54:15 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ANLTQRpl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=7+o9XEZeEzRqslBGGahtOj3rdS6an6WveVosZCqaCFk=;\n\tb=ANLTQRplf7h13W3Xhxt6rp7QnH030JVUX52SR9oMY3aSFDXUSc4w5XgiJHrpOPLu1I\n\tZ2PyaTwO5V89HUAPEpCZQZnFXuxqA71jwvoOhpd7h1h+3B/qmcJjmZ5NNMo+G/pHlU5W\n\tJTnrw6JCIgSJV24RxQYDMUz4sIHvwdBe0xvA1NRNQKrcn8G00fYvhU4y6xN21QHUJth2\n\t49CtrYFQovxtJqy64oC1F6NuvVn5C2funG6HXLNJkY/ntxaPzk/FOuBq7YH2XYRhi9y5\n\tEMD0zaA+Pv5OoiNZisAU0IErgT2Ns+S0WwBclnMM4H0QL6DpICQJJ0xZlwSjTsZXejwZ\n\tvrag==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=7+o9XEZeEzRqslBGGahtOj3rdS6an6WveVosZCqaCFk=;\n\tb=Nw1wTyINns7ZT8UTqKbFtU5rTZXBp0JyCXSFKWx4wt8QzAfhGEM1k/U8W3RItvKKs8\n\t8Dn+UITU8iXBNaTedG+yrAjHo14NEFc+QcHTf5dL2aGj5XsawnQfT9F5SYg6I6C1ifxM\n\tu8Y/bA7Z5LHe2a6PgU3o3sDpxg3j7qbP8IFlyGqqhk9TsOf6J5Y6QLlF/pmIwnDi0RwA\n\tFgxsPCPeDqGEH6z8L3ctRmsttPs+M3taW+MhrhlgTG4PIBwsuzXcuKmYA6dkTg8kd9y+\n\tYjva6uktWw7U3Y3+slLUDqm58ulja3Yzr7C3wb96Q2FFUwwjl7ksKYgnOXiUU/AXc64P\n\t/ZiA==","X-Gm-Message-State":"AOAM530UP4TY9p+ClDhaFTcWlPlidRChf+ZEO3rdwtZwwV6a4uCTSvDA\n\tQCbmRdA9XlE5yHPrHdELSfBTVP1tTJCKae3wQ9gpuA==","X-Google-Smtp-Source":"ABdhPJxBylwY/O/KG7PhkPcL4lBh6qrzVd0nlbnCdASuHiLCBTbe9OzCzSKpX+ATs7aqLe6Bkq0C/Bmefy10HMQUQrM=","X-Received":"by 2002:adf:f44e:: with SMTP id f14mr5838948wrp.37.1637841255499;\n\tThu, 25 Nov 2021 03:54:15 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>\n\t<20211124233705.abuojhhbclc5aynb@uno.localdomain>","In-Reply-To":"<20211124233705.abuojhhbclc5aynb@uno.localdomain>","From":"David Plowman ","Date":"Thu, 25 Nov 2021 11:54:04 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21245,"web_url":"https://patchwork.libcamera.org/comment/21245/","msgid":"<20211125121639.tc6ysgwcapmvsnyt@uno.localdomain>","date":"2021-11-25T12:16:39","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Thu, Nov 25, 2021 at 11:54:04AM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for reviewing this!\n>\n> On Wed, 24 Nov 2021 at 23:36, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:\n> > > This class represents a color space by defining its color primaries,\n> > > YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> > > output is full or limited range.\n> > >\n> > > Signed-off-by: David Plowman \n> > > Reviewed-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/color_space.h | 79 +++++++++++\n> > > include/libcamera/meson.build | 1 +\n> > > src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 324 insertions(+)\n> > > create mode 100644 include/libcamera/color_space.h\n> > > create mode 100644 src/libcamera/color_space.cpp\n> > >\n> > > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> > > new file mode 100644\n> > > index 00000000..44ac077a\n> > > --- /dev/null\n> > > +++ b/include/libcamera/color_space.h\n> > > @@ -0,0 +1,79 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > > + *\n> > > + * color_space.h - color space definitions\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n> > > +#define __LIBCAMERA_COLOR_SPACE_H__\n> >\n> > You can now use #pragma once.\n> > Kieran has sent a series to move the whole codebase to use it\n>\n> Ah, very nice.\n>\n> >\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class ColorSpace\n> > > +{\n> > > +public:\n> > > + enum class Primaries : int {\n> > > + Raw,\n> > > + Smpte170m,\n> > > + Rec709,\n> > > + Rec2020,\n> > > + };\n> > > +\n> > > + enum class YcbcrEncoding : int {\n> > > + Rec601,\n> > > + Rec709,\n> > > + Rec2020,\n> > > + };\n> > > +\n> > > + enum class TransferFunction : int {\n> > > + Linear,\n> > > + Srgb,\n> > > + Rec709,\n> > > + };\n> > > +\n> > > + enum class Range : int {\n> > > + Full,\n> > > + Limited,\n> > > + };\n> >\n> > int or unsigned int ?\n>\n> I think I've always just used int in the past but don't really mind.\n> Anyone feel strongly?\n>\n\nwell, if it doesn't need to be < 0 there's no reason not to use\nunsigned\n\n> >\n> > > +\n> > > + constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> > > + : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> > > + {\n> > > + }\n> > > +\n> > > + static const ColorSpace Raw;\n> > > + static const ColorSpace Jpeg;\n> > > + static const ColorSpace Srgb;\n> > > + static const ColorSpace Smpte170m;\n> > > + static const ColorSpace Rec709;\n> > > + static const ColorSpace Rec2020;\n> > > +\n> > > + Primaries primaries;\n> > > + YcbcrEncoding ycbcrEncoding;\n> > > + TransferFunction transferFunction;\n> > > + Range range;\n> > > +\n> > > + const std::string toString() const;\n> > > + static const std::string toString(const std::optional &colorSpace);\n> > > +};\n> > > +\n> > > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> > > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> > > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n> > > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> > > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> > > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n> > > +\n> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> > > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > > +{\n> > > + return !(lhs == rhs);\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index 7155ff20..131e1740 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> > > libcamera_public_headers = files([\n> > > 'camera.h',\n> > > 'camera_manager.h',\n> > > + 'color_space.h',\n> > > 'compiler.h',\n> > > 'controls.h',\n> > > 'file_descriptor.h',\n> > > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> > > new file mode 100644\n> > > index 00000000..1a5fc7a2\n> > > --- /dev/null\n> > > +++ b/src/libcamera/color_space.cpp\n> > > @@ -0,0 +1,243 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > > + *\n> > > + * color_space.cpp - color spaces.\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> >\n> > Do you need to include for std::pair\n>\n> Probably. Hard to spot when nothing complains...\n>\n> >\n> > > +\n> > > +/**\n> > > + * \\file color_space.h\n> > > + * \\brief Class and enums to represent color spaces\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class ColorSpace\n> > > + * \\brief Class to describe a color space\n> > > + *\n> > > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> > > + * the transfer function associated with the color space, and the range\n> > > + * (sometimes also referred to as the quantisation) of the color space.\n> > > + *\n> > > + * Certain combinations of these fields form well-known standard color\n> > > + * spaces such as \"JPEG\" or \"REC709\".\n> > > + *\n> > > + * In the strictest sense a \"color space\" formally only refers to the\n> > > + * color primaries and white point. Here, however, the ColorSpace class\n> > > + * adopts the common broader usage that includes the transfer function,\n> > > + * Y'CbCr encoding method and quantisation.\n> > > + *\n> > > + * For more information on the specific color spaces described here, please\n> > > + * see:\n> > > + *\n> > > + * sRGB\n> > > + * JPEG\n> > > + * SMPTE 170M\n> > > + * Rec.709\n> > > + * Rec.2020\n> >\n> > Please use links from html/latest/\n>\n> What is html/latest/ exactly?\n>\n\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-bt2020\n\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::Primaries\n> > > + * \\brief The color primaries for this color space\n> > > + *\n> >\n> > I'm debated if this documentation should provide an overview of what\n> > each component is or it is better to assume knowledges about that and\n> > do not say anything.\n> >\n> > On one side, we cannot cover the whole knowledge required, and we'll\n> > end up repeating the existing probably. On the other side,\n> > documentation will feel a bit incomplete for who has not previous\n> > background. But I understand we cannot document the whole world\n> > knowledge so I think it's fair assuming background, even if this is an\n> > application facing API which application developers will have to deal\n> > with.\n>\n> There has been some discussion of this before. I think I'd like to go\n> with the idea that we link to other references which explain it all\n> much better than we are going to, rather than to duplicate everything.\n>\n\nAck, to all comments on documentation if agreed with others\n\n> >\n> > > + * \\var ColorSpace::Primaries::Raw\n> > > + * \\brief These are raw colors directly from a sensor\n> > > + * \\var ColorSpace::Primaries::Smpte170m\n> > > + * \\brief SMPTE 170M color primaries\n> >\n> > What about a blank line between these \\var+\\brief entries ?\n>\n> Hmm, the style checker didn't ask me to do that, but I can add them.\n>\n\nNot hard requirement, just more readable imho. Up to you.\n\n> >\n> > > + * \\var ColorSpace::Primaries::Rec709\n> > > + * \\brief Rec.709 color primaries\n> > > + * \\var ColorSpace::Primaries::Rec2020\n> > > + * \\brief Rec.2020 color primaries\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::YcbcrEncoding\n> > > + * \\brief The Y'CbCr encoding\n> >\n> > Same feeling I had before regarding documentation...\n>\n> As above, really. I think trying to explain this stuff here isn't the\n> way to go, folks should read those links above which explain it pretty\n> well.\n>\n> >\n> > I suspect application developers might want to know more, as we are\n> > going to 'force' them to care about color spaces..\n>\n> But we aren't \"forcing\" them! They can explicitly leave the ColorSpace\n> unset (and bury their heads in the sand)... :)\n>\n> >\n> > > + *\n> > > + * \\var ColorSpace::YcbcrEncoding::Rec601\n> > > + * \\brief Rec.601 Y'CbCr encoding\n> > > + * \\var ColorSpace::YcbcrEncoding::Rec709\n> > > + * \\brief Rec.709 Y'CbCr encoding\n> > > + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> > > + * \\brief Rec.2020 Y'CbCr encoding\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::TransferFunction\n> > > + * \\brief The transfer function used for this color space\n> > > + *\n> > > + * \\var ColorSpace::TransferFunction::Linear\n> > > + * \\brief This color space uses a linear (identity) transfer function\n> > > + * \\var ColorSpace::TransferFunction::Srgb\n> > > + * \\brief sRGB transfer function\n> > > + * \\var ColorSpace::TransferFunction::Rec709\n> > > + * \\brief Rec.709 transfer function\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::Range\n> > > + * \\brief The range (sometimes \"quantisation\") for this color space\n> > > + *\n> > > + * \\var ColorSpace::Range::Full\n> > > + * \\brief This color space uses full range pixel values\n> >\n> > Does the 'full' reange changes with bitdepth as for Limited :)\n> >\n> > > + * \\var ColorSpace::Range::Limited\n> > > + * \\brief This color space uses limited range pixel values, being\n> > > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> > > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)\n> > > + * \\brief Construct a ColorSpace from explicit values\n> > > + * \\param[in] p The color primaries\n> > > + * \\param[in] e The Y'CbCr encoding\n> > > + * \\param[in] t The transfer function for the color space\n> > > + * \\param[in] r The range of the pixel values in this color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Assemble and return a readable string representation of the\n> > > + * ColorSpace\n> > > + * \\return A string describing the ColorSpace\n> > > + */\n> > > +const std::string ColorSpace::toString() const\n> > > +{\n> > > + /* Print out a brief name only for standard color sapces. */\n> > > +\n> > > + static const std::vector> colorSpaceNames = {\n> > > + { ColorSpace::Raw, \"Raw\" },\n> > > + { ColorSpace::Jpeg, \"Jpeg\" },\n> > > + { ColorSpace::Srgb, \"Srgb\" },\n> > > + { ColorSpace::Smpte170m, \"Smpte170m\" },\n> > > + { ColorSpace::Rec709, \"Rec709\" },\n> > > + { ColorSpace::Rec2020, \"Rec2020\" },\n> > > + };\n> >\n> > This really looks like a map :)\n>\n> I know. I'm slightly loath to add an artificial ordering function just\n> so that I can use a map, or even add a hashing function so that I can\n> use an unordered_map, when for smallish arrays it doesn't gain\n> anything. But there may come a point where adding this extra stuff\n> becomes easier than explaining why there's no point in adding it!\n>\n\n:) Up to you\n\n> >\n> > > + auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> > > + [this](const auto &item) {\n> > > + return *this == item.first;\n> > > + });\n> > > + if (it != colorSpaceNames.end())\n> > > + return std::string(it->second);\n> > > +\n> > > + static const char *primariesNames[] = {\n> > > + \"Raw\",\n> > > + \"Smpte170m\",\n> > > + \"Rec709\",\n> > > + \"Rec2020\",\n> > > + };\n> >\n> > I was expecting a warning caused by \"converting a string constant to\n> > char *\"\n> >\n> > > + static const char *encodingNames[] = {\n> > > + \"Rec601\",\n> > > + \"Rec709\",\n> > > + \"Rec2020\",\n> > > + };\n> > > + static const char *transferFunctionNames[] = {\n> > > + \"Linear\",\n> > > + \"Srgb\",\n> > > + \"Rec709\",\n> > > + };\n> > > + static const char *rangeNames[] = {\n> > > + \"Full\",\n> > > + \"Limited\",\n> > > + };\n> >\n> > This could be made a bit more C++-ish by using std::array and\n> > std::string, but as far as I can tell using a container frowns up\n> > making this a constexpr (soemthing you could do with raw arrays\n> > though)\n> >\n> > This is also slightly fragile as it requires care to maintain the vectors\n> > and the enums in sync. Maps are probably more expensive in terms of\n> > memory occupation but forces the association to be explicitely\n> > maintained.\n>\n> Honestly I don't know what's best. Perhaps it should check the values\n> don't run off the end of the tables. Anyone feel strongly about how to\n> do this?\n\nThis feels a bit fragile. The alternative is to define for each\ncolorspace/primary/encoding etc a type which contains a string\ndescription, something I'm not sure it's worth it.\n\nSometimes I miss the C preprocessor stringify function :)\n\n\n>\n> >\n> > > +\n> > > + std::stringstream ss;\n> > > + ss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> > > + << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> > > + << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> > > + << std::string(rangeNames[static_cast(range)]);\n> > > +\n> > > + return ss.str();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Assemble and return a readable string representation of an\n> > > + * optional ColorSpace\n> > > + * \\return A string describing the optional ColorSpace\n> > > + */\n> > > +const std::string ColorSpace::toString(const std::optional &colorSpace)\n> > > +{\n> > > + if (!colorSpace)\n> > > + return \"Unknown\";\n> > > +\n> > > + return colorSpace->toString();\n> > > +}\n> >\n> > How do you immagine this to be used. Why would a caller use this\n> > function on a !colorspace ? I would have ASSERT(colorspace) :)\n>\n> The catch is that our configuration and format classes contain a\n> std::optional, not a simple ColorSpace. So they would\n> spend all their time doing this:\n>\n> LOG(XXX, Debug) << (colorSpace ? colorSpace.toString() : \"unknown\");\n>\n> whereas I slightly prefer\n>\n> LOG(XXX, Debug) << ColorSpace::toString(colorSpace);\n>\n> Does that make sense? Or would it be better somewhere else with some\n> other kind of syntax?\n>\n> Thanks!\n> David\n>\n> >\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::primaries\n> > > + * \\brief The color primaries\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::ycbcrEncoding\n> > > + * \\brief The Y'CbCr encoding\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::transferFunction\n> > > + * \\brief The transfer function for this color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::range\n> > > + * \\brief The pixel range used by this color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Undefined\n> > > + * \\brief A constant representing a fully undefined color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Raw\n> > > + * \\brief A constant representing a raw color space (from a sensor)\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Jpeg\n> > > + * \\brief A constant representing the JPEG color space used for\n> > > + * encoding JPEG images (and regarded as being the same as the sRGB\n> > > + * color space)\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Smpte170m\n> > > + * \\brief A constant representing the SMPTE170M color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Rec709\n> > > + * \\brief A constant representing the Rec.709 color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Rec2020\n> > > + * \\brief A constant representing the Rec.2020 color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Compare color spaces for equality\n> > > + * \\return True if the two color spaces are identical, false otherwise\n> > > + */\n> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> > > +{\n> > > + return lhs.primaries == rhs.primaries &&\n> > > + lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> > > + lhs.transferFunction == rhs.transferFunction &&\n> > > + lhs.range == rhs.range;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > > + * \\brief Compare color spaces for inequality\n> > > + * \\return True if the two color spaces are not identical, false otherwise\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 6727a777..e7371d20 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -8,6 +8,7 @@ libcamera_sources = files([\n> > > 'camera_manager.cpp',\n> > > 'camera_sensor.cpp',\n> > > 'camera_sensor_properties.cpp',\n> > > + 'color_space.cpp',\n> > > 'controls.cpp',\n> > > 'control_serializer.cpp',\n> > > 'control_validator.cpp',\n> > > --\n> > > 2.20.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4FB9BBDB13\n\tfor ;\n\tThu, 25 Nov 2021 12:15:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 164F76036F;\n\tThu, 25 Nov 2021 13:15:51 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB36A60231\n\tfor ;\n\tThu, 25 Nov 2021 13:15:49 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6B76E1BF208;\n\tThu, 25 Nov 2021 12:15:48 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 13:16:39 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211125121639.tc6ysgwcapmvsnyt@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>\n\t<20211124233705.abuojhhbclc5aynb@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21246,"web_url":"https://patchwork.libcamera.org/comment/21246/","msgid":"<163784307448.3059017.14646830127793316302@Monstersaurus>","date":"2021-11-25T12:24:34","subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-25 11:54:04)\n> Hi Jacopo\n> \n> Thanks for reviewing this!\n> \n> On Wed, 24 Nov 2021 at 23:36, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Thu, Nov 18, 2021 at 03:19:27PM +0000, David Plowman wrote:\n> > > This class represents a color space by defining its color primaries,\n> > > YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> > > output is full or limited range.\n> > >\n> > > Signed-off-by: David Plowman \n> > > Reviewed-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/color_space.h | 79 +++++++++++\n> > > include/libcamera/meson.build | 1 +\n> > > src/libcamera/color_space.cpp | 243 ++++++++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 324 insertions(+)\n> > > create mode 100644 include/libcamera/color_space.h\n> > > create mode 100644 src/libcamera/color_space.cpp\n> > >\n> > > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> > > new file mode 100644\n> > > index 00000000..44ac077a\n> > > --- /dev/null\n> > > +++ b/include/libcamera/color_space.h\n> > > @@ -0,0 +1,79 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > > + *\n> > > + * color_space.h - color space definitions\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_COLOR_SPACE_H__\n> > > +#define __LIBCAMERA_COLOR_SPACE_H__\n> >\n> > You can now use #pragma once.\n> > Kieran has sent a series to move the whole codebase to use it\n> \n> Ah, very nice.\n> \n> >\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class ColorSpace\n> > > +{\n> > > +public:\n> > > + enum class Primaries : int {\n> > > + Raw,\n> > > + Smpte170m,\n> > > + Rec709,\n> > > + Rec2020,\n> > > + };\n> > > +\n> > > + enum class YcbcrEncoding : int {\n> > > + Rec601,\n> > > + Rec709,\n> > > + Rec2020,\n> > > + };\n> > > +\n> > > + enum class TransferFunction : int {\n> > > + Linear,\n> > > + Srgb,\n> > > + Rec709,\n> > > + };\n> > > +\n> > > + enum class Range : int {\n> > > + Full,\n> > > + Limited,\n> > > + };\n> >\n> > int or unsigned int ?\n> \n> I think I've always just used int in the past but don't really mind.\n> Anyone feel strongly?\n\nDo we need to specify the types at all? Do we do that on other enums?\n\n> > > + constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> > > + : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> > > + {\n> > > + }\n> > > +\n> > > + static const ColorSpace Raw;\n> > > + static const ColorSpace Jpeg;\n> > > + static const ColorSpace Srgb;\n> > > + static const ColorSpace Smpte170m;\n> > > + static const ColorSpace Rec709;\n> > > + static const ColorSpace Rec2020;\n> > > +\n> > > + Primaries primaries;\n> > > + YcbcrEncoding ycbcrEncoding;\n> > > + TransferFunction transferFunction;\n> > > + Range range;\n> > > +\n> > > + const std::string toString() const;\n> > > + static const std::string toString(const std::optional &colorSpace);\n> > > +};\n> > > +\n> > > +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> > > +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> > > +constexpr ColorSpace ColorSpace::Srgb = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Limited };\n> > > +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> > > +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> > > +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n> > > +\n> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> > > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > > +{\n> > > + return !(lhs == rhs);\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index 7155ff20..131e1740 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> > > libcamera_public_headers = files([\n> > > 'camera.h',\n> > > 'camera_manager.h',\n> > > + 'color_space.h',\n> > > 'compiler.h',\n> > > 'controls.h',\n> > > 'file_descriptor.h',\n> > > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> > > new file mode 100644\n> > > index 00000000..1a5fc7a2\n> > > --- /dev/null\n> > > +++ b/src/libcamera/color_space.cpp\n> > > @@ -0,0 +1,243 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > > + *\n> > > + * color_space.cpp - color spaces.\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> >\n> > Do you need to include for std::pair\n> \n> Probably. Hard to spot when nothing complains...\n\nThere's a tool called iwyu\n\nhttps://include-what-you-use.org/\n\nI've tried to figure out how we can incorporate that into checkstyle.\nBut it requires a build tree, so it might have to be run as an\nintegration type check :-(\n\nI use it like this:\n\tiwyu_tool -p build/gcc/ -j32 > iwyu.report\n\n\n> > > +\n> > > +/**\n> > > + * \\file color_space.h\n> > > + * \\brief Class and enums to represent color spaces\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class ColorSpace\n> > > + * \\brief Class to describe a color space\n> > > + *\n> > > + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> > > + * the transfer function associated with the color space, and the range\n> > > + * (sometimes also referred to as the quantisation) of the color space.\n> > > + *\n> > > + * Certain combinations of these fields form well-known standard color\n> > > + * spaces such as \"JPEG\" or \"REC709\".\n> > > + *\n> > > + * In the strictest sense a \"color space\" formally only refers to the\n> > > + * color primaries and white point. Here, however, the ColorSpace class\n> > > + * adopts the common broader usage that includes the transfer function,\n> > > + * Y'CbCr encoding method and quantisation.\n> > > + *\n> > > + * For more information on the specific color spaces described here, please\n> > > + * see:\n> > > + *\n> > > + * sRGB\n> > > + * JPEG\n> > > + * SMPTE 170M\n> > > + * Rec.709\n> > > + * Rec.2020\n> >\n> > Please use links from html/latest/\n> \n> What is html/latest/ exactly?\n\nIt will always generate from the latest kernel release.\n\n\thttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-bt2020\n\nMy worry on that is that it can lead to stale/broken links if the\ndocumentation paths change.\n\nBut we should at least be using the latest released kernel: 5.15...\n\thttps://www.kernel.org/doc/html/v5.15/userspace-api/media/v4l/colorspaces-details.html#col-bt2020\n\nWe can catch broken links with tooling in doxygen though - so I think\nusing html/latest makes more sense.\n\n\tninja Documentation/linkcheck\n\nWill run the check, so this is automatable too.\n\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::Primaries\n> > > + * \\brief The color primaries for this color space\n> > > + *\n> >\n> > I'm debated if this documentation should provide an overview of what\n> > each component is or it is better to assume knowledges about that and\n> > do not say anything.\n> >\n> > On one side, we cannot cover the whole knowledge required, and we'll\n> > end up repeating the existing probably. On the other side,\n> > documentation will feel a bit incomplete for who has not previous\n> > background. But I understand we cannot document the whole world\n> > knowledge so I think it's fair assuming background, even if this is an\n> > application facing API which application developers will have to deal\n> > with.\n> \n> There has been some discussion of this before. I think I'd like to go\n> with the idea that we link to other references which explain it all\n> much better than we are going to, rather than to duplicate everything.\n> \n> >\n> > > + * \\var ColorSpace::Primaries::Raw\n> > > + * \\brief These are raw colors directly from a sensor\n> > > + * \\var ColorSpace::Primaries::Smpte170m\n> > > + * \\brief SMPTE 170M color primaries\n> >\n> > What about a blank line between these \\var+\\brief entries ?\n> \n> Hmm, the style checker didn't ask me to do that, but I can add them.\n\nUnfortunately the style checker can't check the doxygen comments. (yet?)\n\n> > > + * \\var ColorSpace::Primaries::Rec709\n> > > + * \\brief Rec.709 color primaries\n> > > + * \\var ColorSpace::Primaries::Rec2020\n> > > + * \\brief Rec.2020 color primaries\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::YcbcrEncoding\n> > > + * \\brief The Y'CbCr encoding\n> >\n> > Same feeling I had before regarding documentation...\n> \n> As above, really. I think trying to explain this stuff here isn't the\n> way to go, folks should read those links above which explain it pretty\n> well.\n> \n> >\n> > I suspect application developers might want to know more, as we are\n> > going to 'force' them to care about color spaces..\n> \n> But we aren't \"forcing\" them! They can explicitly leave the ColorSpace\n> unset (and bury their heads in the sand)... :)\n\nI think that's fine. They will leave it unset when configuring, but be\n/told/ what colourspace it is when the configuration is validated.\n\nIf they don't choose to care about it from there, then that's up to them\n;-) Perhaps they really didn't need it..\n\n> > > + *\n> > > + * \\var ColorSpace::YcbcrEncoding::Rec601\n> > > + * \\brief Rec.601 Y'CbCr encoding\n> > > + * \\var ColorSpace::YcbcrEncoding::Rec709\n> > > + * \\brief Rec.709 Y'CbCr encoding\n> > > + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> > > + * \\brief Rec.2020 Y'CbCr encoding\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::TransferFunction\n> > > + * \\brief The transfer function used for this color space\n> > > + *\n> > > + * \\var ColorSpace::TransferFunction::Linear\n> > > + * \\brief This color space uses a linear (identity) transfer function\n> > > + * \\var ColorSpace::TransferFunction::Srgb\n> > > + * \\brief sRGB transfer function\n> > > + * \\var ColorSpace::TransferFunction::Rec709\n> > > + * \\brief Rec.709 transfer function\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::Range\n> > > + * \\brief The range (sometimes \"quantisation\") for this color space\n> > > + *\n> > > + * \\var ColorSpace::Range::Full\n> > > + * \\brief This color space uses full range pixel values\n> >\n> > Does the 'full' reange changes with bitdepth as for Limited :)\n> >\n> > > + * \\var ColorSpace::Range::Limited\n> > > + * \\brief This color space uses limited range pixel values, being\n> > > + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> > > + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ColorSpace::ColorSpace(Primaries p, Encoding e, TransferFunction t, Range r)\n> > > + * \\brief Construct a ColorSpace from explicit values\n> > > + * \\param[in] p The color primaries\n> > > + * \\param[in] e The Y'CbCr encoding\n> > > + * \\param[in] t The transfer function for the color space\n> > > + * \\param[in] r The range of the pixel values in this color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Assemble and return a readable string representation of the\n> > > + * ColorSpace\n> > > + * \\return A string describing the ColorSpace\n> > > + */\n> > > +const std::string ColorSpace::toString() const\n> > > +{\n> > > + /* Print out a brief name only for standard color sapces. */\n> > > +\n> > > + static const std::vector> colorSpaceNames = {\n> > > + { ColorSpace::Raw, \"Raw\" },\n> > > + { ColorSpace::Jpeg, \"Jpeg\" },\n> > > + { ColorSpace::Srgb, \"Srgb\" },\n> > > + { ColorSpace::Smpte170m, \"Smpte170m\" },\n> > > + { ColorSpace::Rec709, \"Rec709\" },\n> > > + { ColorSpace::Rec2020, \"Rec2020\" },\n> > > + };\n> >\n> > This really looks like a map :)\n> \n> I know. I'm slightly loath to add an artificial ordering function just\n> so that I can use a map, or even add a hashing function so that I can\n> use an unordered_map, when for smallish arrays it doesn't gain\n> anything. But there may come a point where adding this extra stuff\n> becomes easier than explaining why there's no point in adding it!\n\nAh - yes the ColorSpace is not something that's easily turned into an\nindex...\n\n> > > + auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> > > + [this](const auto &item) {\n> > > + return *this == item.first;\n> > > + });\n> > > + if (it != colorSpaceNames.end())\n> > > + return std::string(it->second);\n> > > +\n> > > + static const char *primariesNames[] = {\n> > > + \"Raw\",\n> > > + \"Smpte170m\",\n> > > + \"Rec709\",\n> > > + \"Rec2020\",\n> > > + };\n> >\n> > I was expecting a warning caused by \"converting a string constant to\n> > char *\"\n> >\n> > > + static const char *encodingNames[] = {\n> > > + \"Rec601\",\n> > > + \"Rec709\",\n> > > + \"Rec2020\",\n> > > + };\n> > > + static const char *transferFunctionNames[] = {\n> > > + \"Linear\",\n> > > + \"Srgb\",\n> > > + \"Rec709\",\n> > > + };\n> > > + static const char *rangeNames[] = {\n> > > + \"Full\",\n> > > + \"Limited\",\n> > > + };\n> >\n> > This could be made a bit more C++-ish by using std::array and\n> > std::string, but as far as I can tell using a container frowns up\n> > making this a constexpr (soemthing you could do with raw arrays\n> > though)\n> >\n> > This is also slightly fragile as it requires care to maintain the vectors\n> > and the enums in sync. Maps are probably more expensive in terms of\n> > memory occupation but forces the association to be explicitely\n> > maintained.\n> \n> Honestly I don't know what's best. Perhaps it should check the values\n> don't run off the end of the tables. Anyone feel strongly about how to\n> do this?\n\nI'd be a little bit worried ...\n\nCan an appliction now construct a ColorSpace directly, with an invalid\nrange of '4', and cause this function to access memory beyond the\ndefinition?\n\nIf so - we certainly need to prevent that, otherwise we'll start seeing\npeople filling vulnerabilities against us...\n\n\n> > > +\n> > > + std::stringstream ss;\n> > > + ss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> > > + << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> > > + << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> > > + << std::string(rangeNames[static_cast(range)]);\n> > > +\n> > > + return ss.str();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Assemble and return a readable string representation of an\n> > > + * optional ColorSpace\n> > > + * \\return A string describing the optional ColorSpace\n> > > + */\n> > > +const std::string ColorSpace::toString(const std::optional &colorSpace)\n> > > +{\n> > > + if (!colorSpace)\n> > > + return \"Unknown\";\n> > > +\n> > > + return colorSpace->toString();\n> > > +}\n> >\n> > How do you immagine this to be used. Why would a caller use this\n> > function on a !colorspace ? I would have ASSERT(colorspace) :)\n> \n> The catch is that our configuration and format classes contain a\n> std::optional, not a simple ColorSpace. So they would\n> spend all their time doing this:\n> \n> LOG(XXX, Debug) << (colorSpace ? colorSpace.toString() : \"unknown\");\n> \n> whereas I slightly prefer\n> \n> LOG(XXX, Debug) << ColorSpace::toString(colorSpace);\n\n^^ This is better IMO.\n\n> \n> Does that make sense? Or would it be better somewhere else with some\n> other kind of syntax?\n> \n> Thanks!\n> David\n> \n> >\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::primaries\n> > > + * \\brief The color primaries\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::ycbcrEncoding\n> > > + * \\brief The Y'CbCr encoding\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::transferFunction\n> > > + * \\brief The transfer function for this color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::range\n> > > + * \\brief The pixel range used by this color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Undefined\n> > > + * \\brief A constant representing a fully undefined color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Raw\n> > > + * \\brief A constant representing a raw color space (from a sensor)\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Jpeg\n> > > + * \\brief A constant representing the JPEG color space used for\n> > > + * encoding JPEG images (and regarded as being the same as the sRGB\n> > > + * color space)\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Smpte170m\n> > > + * \\brief A constant representing the SMPTE170M color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Rec709\n> > > + * \\brief A constant representing the Rec.709 color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::Rec2020\n> > > + * \\brief A constant representing the Rec.2020 color space\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Compare color spaces for equality\n> > > + * \\return True if the two color spaces are identical, false otherwise\n> > > + */\n> > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> > > +{\n> > > + return lhs.primaries == rhs.primaries &&\n> > > + lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> > > + lhs.transferFunction == rhs.transferFunction &&\n> > > + lhs.range == rhs.range;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> > > + * \\brief Compare color spaces for inequality\n> > > + * \\return True if the two color spaces are not identical, false otherwise\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 6727a777..e7371d20 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -8,6 +8,7 @@ libcamera_sources = files([\n> > > 'camera_manager.cpp',\n> > > 'camera_sensor.cpp',\n> > > 'camera_sensor_properties.cpp',\n> > > + 'color_space.cpp',\n> > > 'controls.cpp',\n> > > 'control_serializer.cpp',\n> > > 'control_validator.cpp',\n> > > --\n> > > 2.20.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D82F6BF415\n\tfor ;\n\tThu, 25 Nov 2021 12:24:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 326D360376;\n\tThu, 25 Nov 2021 13:24:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B36A060231\n\tfor ;\n\tThu, 25 Nov 2021 13:24:37 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3605B90E;\n\tThu, 25 Nov 2021 13:24:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UTuwZ5gD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637843077;\n\tbh=KHUX1zIp7etzAdPwuZ2331kPMsjGEiDJG1IX/rIVIRQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=UTuwZ5gDEuXtol1m5Jl+89zkrllSDZ2yYoj0Nzhcp8s3/jM+C40wK0gxc/HoZQMqy\n\t0VCsxJjOdoumw5G/nJ2c9EElxC3P8moorlQ4DG+1DGEc04RxSCHqR8JC2t4adLUxgh\n\tlGEa/aSHfpEfluIuALKd1uaUb+Pb9C6xAoR/o6is=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-2-david.plowman@raspberrypi.com>\n\t<20211124233705.abuojhhbclc5aynb@uno.localdomain>\n\t","From":"Kieran Bingham ","To":"David Plowman ,\n\tJacopo Mondi ","Date":"Thu, 25 Nov 2021 12:24:34 +0000","Message-ID":"<163784307448.3059017.14646830127793316302@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Hans Verkuil , Tomasz Figa , \n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]