[{"id":20073,"web_url":"https://patchwork.libcamera.org/comment/20073/","msgid":"","date":"2021-10-05T21:03:47","subject":"Re: [libcamera-devel] [PATCH v2 1/3] libcamera: Add ColorSpace class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\n(CC'ing Hans, the colour space expert in V4L2)\n\nThank you for the patch.\n\nHans, if you have a bit of time to look at this, your feedback would be\nappreciated.\n\nOn Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:\n> This class represents a colour space by defining its YCbCr encoding,\n> the transfer (gamma) function is uses, and whether the output is full\n\ns/is/it/\n\n> or limited range.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/color_space.h | 83 +++++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/color_space.cpp | 207 ++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 292 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..9cd10503\n> --- /dev/null\n> +++ b/include/libcamera/color_space.h\n> @@ -0,0 +1,83 @@\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> +\n> +namespace libcamera {\n> +\n> +class ColorSpace\n> +{\n> +public:\n> +\tenum class Encoding : int {\n> +\t\tUNDEFINED,\n\nWe typically use CamelCase for enumerators.\n\n> +\t\tRAW,\n> +\t\tREC601,\n> +\t\tREC709,\n> +\t\tREC2020,\n> +\t\tVIDEO,\n> +\t};\n> +\n> +\tenum class TransferFunction : int {\n> +\t\tUNDEFINED,\n> +\t\tIDENTITY,\n> +\t\tSRGB,\n> +\t\tREC709,\n> +\t};\n> +\n> +\tenum class Range : int {\n> +\t\tUNDEFINED,\n> +\t\tFULL,\n> +\t\tLIMITED,\n> +\t};\n> +\n> +\tconstexpr ColorSpace(Encoding e, TransferFunction t, Range r)\n> +\t\t: encoding(e), transferFunction(t), range(r)\n> +\t{\n> +\t}\n> +\n> +\tconstexpr ColorSpace()\n> +\t\t: ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)\n\nLine wrap ?\n\n> +\t{\n> +\t}\n\nI'd move the default constructor furst.\n\n> +\n> +\tstatic const ColorSpace UNDEFINED;\n> +\tstatic const ColorSpace RAW;\n> +\tstatic const ColorSpace JFIF;\n> +\tstatic const ColorSpace SMPTE170M;\n> +\tstatic const ColorSpace REC709;\n> +\tstatic const ColorSpace REC2020;\n> +\tstatic const ColorSpace VIDEO;\n\nShouldn't these be defined as static constexpr with a value here ?\n\n> +\n> +\tEncoding encoding;\n> +\tTransferFunction transferFunction;\n> +\tRange range;\n> +\n> +\tbool isFullyDefined() const;\n> +\n> +\tconst std::string toString() const;\n> +};\n> +\n> +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };\n> +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };\n> +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };\n> +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };\n> +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };\n> +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };\n> +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };\n\nThis could then be dropped, or possibly moved (without the initializers)\nto color_space.cpp if there's a need to bind a reference or take the\naddress of these variables.\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 5b25ef84..7a8a04e5 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -3,6 +3,7 @@\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..d1ccb4cc\n> --- /dev/null\n> +++ b/src/libcamera/color_space.cpp\n> @@ -0,0 +1,207 @@\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\nThis should go first, to ensure that the header file is self-contained.\n\n> +\n> +/**\n> + * \\file color_space.h\n> + * \\brief Class and enums to represent colour spaces.\n\nNo period at the end of briefs. Same below.\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class ColorSpace\n> + * \\brief Class to describe a color space.\n> + *\n> + * The color space class defines the encodings of the color primaries, the\n\ns/color space/ColorSpace/\n\nYou use both color and colour. For the class name, ColorSpace seems\nright (well, it seems wrong to me, but it seems to be the right choice\n:-)). For the text, we'll likely have to go through the documentation\none day to make everything consistent, and will unfortunately likely\nhave to pick the US spelling. This hasn't been completely decided yet,\nso I've fine with either for now, but let's make it consistent within\nthe file.\n\n> + * transfer function associated with the color space, and the range (sometimes\n> + * also referred to as the quantisation) of the color space.\n> + *\n> + * Certain combinations of these fields form well-known standard color spaces,\n> + * such as \"JFIF\" or \"REC709\", though there is flexibility to leave some or all\n> + * of them undefined too.\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Encoding\n> + * \\brief The encoding used for the color primaries.\n\nCan we name this Primaries ? Encoding is strongly tied to the Y'CbCr\nencoding from R'G'B' values, I think it would be confusing.\n\nNow that I've written this, I realize (after reading the rest of the\nseries) that you're using this as the actual Y'CbCr encoding. The name\nis thus fine, but it shouldn't mention colour primaries then, it should\nbe defined as Y'CbCr encoding. I would also move the definition of the\nEncoding enum after TransferFunction, as that's the order in which\nthey're applied.\n\nThis leads me to the next question: what should we do with the colour\nprimaries (and white point) ? Those are part of the colour space\ndefinition.\n\n> + *\n> + * \\var ColorSpace::Encoding::UNDEFINED\n> + * \\brief The encoding for the colour primaries is not specified.\n\nI think we need to include more usage information. Can an application\nset the encoding to undefined ? What will happen then ? Can a camera\nreturn undefined ? \n\n> + * \\var ColorSpace::Encoding::RAW\n> + * \\brief These are raw colours from the sensor.\n\nWill this also apply to RGB formats ?\n\n> + * \\var ColorSpace::Encoding::REC601\n> + * \\brief REC601 colour primaries.\n\ns/REC601/Rec. 601/\n\nor possibly \"ITU-R Rec. 601\" or \"ITU-R Rec. BT.601\" if we want to be\nprecise. Same below.\n\nShould we also include the encoding formula explicitly, or would that be\ntoo much details ?\n\n> + * \\var ColorSpace::Encoding::REC709\n> + * \\brief Rec709 colour primaries.\n> + * \\var ColorSpace::Encoding::REC2020\n> + * \\brief REC2020 colour primaries.\n> + * \\var ColorSpace::Encoding::VIDEO\n> + * \\brief A place-holder for video streams which will be resolved to one\n> + * of REC601, REC709 or REC2020 once the video resolution is known.\n\nWe also need more information here. What's your expected use case for\nthis ? I'm concerned it may seem safe for applications to pick this as a\ndefault, but they won't care much about colour spaces then, and will\nlikely get things wrong.\n\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::TransferFunction\n> + * \\brief The transfer function used for this colour space.\n> + *\n> + * \\var ColorSpace::TransferFunction::UNDEFINED\n> + * \\brief The transfer function is not specified.\n> + * \\var ColorSpace::TransferFunction::IDENTITY\n> + * \\brief This color space uses an identity transfer function.\n\nIsn't \"linear\" a more common term than \"identity\" for the transfer\nfunction ?\n\n> + * \\var ColorSpace::TransferFunction::SRGB\n> + * \\brief sRGB transfer function.\n> + * \\var ColorSpace::TransferFunction::REC709\n> + * \\brief Rec709 transfer function.\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Range\n> + * \\brief The range (sometimes \"quantisation\") for this color space.\n\nTechnically speaking, the quantisation isn't part of the colour space,\nbut I fear it would bring lots of complexity and little gain to try and\nkeep it separate.\n\n> + *\n> + * \\var ColorSpace::Range::UNDEFINED\n> + * \\brief The range is not specified.\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.\n\nShould we define these in more details as well, with the exact range ?\n\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n> + * \\brief Construct a ColorSpace from explicit values\n> + * \\param[in] e The encoding for the color primaries\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> + * \\fn ColorSpace::ColorSpace()\n> + * \\brief Construct a color space with undefined encoding, transfer function\n> + * and range\n> + */\n> +\n> +/**\n> + * \\brief Return true if all the fields of the color space are defined, otherwise false.\n\nLine wrap. You also need a \\return, so I'd write \\brief as \"Check if\n...\".\n\n> + */\n> +bool ColorSpace::isFullyDefined() const\n> +{\n> +\treturn encoding != Encoding::UNDEFINED &&\n> +\t transferFunction != TransferFunction::UNDEFINED &&\n> +\t range != Range::UNDEFINED;\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> +\tstatic const char *encodings[] = {\n> +\t\t\"UNDEFINED\",\n> +\t\t\"RAW\",\n> +\t\t\"REC601\",\n> +\t\t\"REC709\",\n> +\t\t\"REC2020\",\n> +\t};\n> +\tstatic const char *transferFunctions[] = {\n> +\t\t\"UNDEFINED\",\n> +\t\t\"IDENTITY\",\n> +\t\t\"SRGB\",\n> +\t\t\"REC709\",\n> +\t};\n> +\tstatic const char *ranges[] = {\n> +\t\t\"UNDEFINED\",\n> +\t\t\"FULL\",\n> +\t\t\"LIMITED\",\n> +\t};\n> +\n> +\tstd::stringstream ss;\n> +\tss << std::string(encodings[static_cast(encoding)]) << \"+\"\n\n\"+\" seems a bit of a weird separator, I would have expected \"/\". Is\nthere a specific reason ?\n\n> +\t << std::string(transferFunctions[static_cast(transferFunction)]) << \"+\"\n> +\t << std::string(ranges[static_cast(range)]);\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +/**\n> + * \\var ColorSpace::encoding\n> + * \\brief The encoding of the color primaries\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::JFIF\n> + * \\brief A constant representing the JFIF color space usually used for\n> + * encoding JPEG images.\n\nShould this and the colour spaces below be defined more precisely in the\ndocumentation, or do you think users can just look at the values of the\ndifferent members to figure out which is which ?\n\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::SMPTE170M\n> + * \\brief A constant representing the SMPTE170M color space (sometimes also\n> + * referred to as \"full range BT601\").\n\nEven though quantization is limited range ?\n\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::REC709\n> + * \\brief A constant representing the REC709 color space.\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::REC2020\n> + * \\brief A constant representing the REC2020 color space.\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::VIDEO\n> + * \\brief A constant that video streams can use to indicate the \"default\"\n> + * color space for a video of this resolution, once that is is known. For\n> + * exmample, SD streams would interpret this as SMPTE170M, HD streams as\n\ns/exmample/example/\n\n> + * REC709 and ultra HD as REC2020.\n\nAs mentioned above, this sounds a bit dangerous to me, but maybe I worry\ntoo much. If we want to keep this, I think we need to make the\ndefinition stricter, by documenting the required behaviour (thus\nremoving \"for example\", and defining SD and HD).\n\n> + */\n> +\n> +/**\n> + * \\brief Compare color spaces for equality\n\n *\n * Color spaces are considered identical if they have the same\n * \\ref transferFunction, \\ref encoding and \\ref range.\n *\n\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.encoding == rhs.encoding &&\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 bf82d38b..88dfbf24 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 C7CF8BDC71\n\tfor ;\n\tTue, 5 Oct 2021 21:03:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40861691BB;\n\tTue, 5 Oct 2021 23:03:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BCCC4684C6\n\tfor ;\n\tTue, 5 Oct 2021 23:03:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 27A5C25B;\n\tTue, 5 Oct 2021 23:03:55 +0200 (CEST)"],"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=\"AtgBM1vw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633467835;\n\tbh=7uTCUZN5MbL7THp8Xg0+DtJMt4UHYXksd/ltsNwGejI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AtgBM1vw4FhECzRAjxFQQoeJ71UKJC1aW1uMqjAyGULfmvfWDyGDeKaGCz6IsJ5cd\n\t7QbKCM8ZXsqsv3srhcIDFW9wXy6zyY2riujBzwRSq119JSCj5j9RzwVbYZ7Itu1DpH\n\tlf8Re8U9pDdGktbGUW93Q7xmPnpLp/vyU/h3zNBg=","Date":"Wed, 6 Oct 2021 00:03:47 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210927123327.14554-1-david.plowman@raspberrypi.com>\n\t<20210927123327.14554-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927123327.14554-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20115,"web_url":"https://patchwork.libcamera.org/comment/20115/","msgid":"","date":"2021-10-11T14:16:35","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 Laurent\n\nThanks for the detailed review of all this. I probably won't comment\non some of the more \"trivial\" items (I'll just do a v3!) but some of\nthe points do indeed want some more discussion.\n\nOn Tue, 5 Oct 2021 at 22:03, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> (CC'ing Hans, the colour space expert in V4L2)\n>\n> Thank you for the patch.\n>\n> Hans, if you have a bit of time to look at this, your feedback would be\n> appreciated.\n>\n> On Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:\n> > This class represents a colour space by defining its YCbCr encoding,\n> > the transfer (gamma) function is uses, and whether the output is full\n>\n> s/is/it/\n>\n> > or limited range.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/color_space.h | 83 +++++++++++++\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/color_space.cpp | 207 ++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 292 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..9cd10503\n> > --- /dev/null\n> > +++ b/include/libcamera/color_space.h\n> > @@ -0,0 +1,83 @@\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> > +\n> > +namespace libcamera {\n> > +\n> > +class ColorSpace\n> > +{\n> > +public:\n> > + enum class Encoding : int {\n> > + UNDEFINED,\n>\n> We typically use CamelCase for enumerators.\n>\n> > + RAW,\n> > + REC601,\n> > + REC709,\n> > + REC2020,\n> > + VIDEO,\n> > + };\n> > +\n> > + enum class TransferFunction : int {\n> > + UNDEFINED,\n> > + IDENTITY,\n> > + SRGB,\n> > + REC709,\n> > + };\n> > +\n> > + enum class Range : int {\n> > + UNDEFINED,\n> > + FULL,\n> > + LIMITED,\n> > + };\n> > +\n> > + constexpr ColorSpace(Encoding e, TransferFunction t, Range r)\n> > + : encoding(e), transferFunction(t), range(r)\n> > + {\n> > + }\n> > +\n> > + constexpr ColorSpace()\n> > + : ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)\n>\n> Line wrap ?\n>\n> > + {\n> > + }\n>\n> I'd move the default constructor furst.\n>\n> > +\n> > + static const ColorSpace UNDEFINED;\n> > + static const ColorSpace RAW;\n> > + static const ColorSpace JFIF;\n> > + static const ColorSpace SMPTE170M;\n> > + static const ColorSpace REC709;\n> > + static const ColorSpace REC2020;\n> > + static const ColorSpace VIDEO;\n>\n> Shouldn't these be defined as static constexpr with a value here ?\n>\n> > +\n> > + Encoding encoding;\n> > + TransferFunction transferFunction;\n> > + Range range;\n> > +\n> > + bool isFullyDefined() const;\n> > +\n> > + const std::string toString() const;\n> > +};\n> > +\n> > +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };\n> > +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };\n> > +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };\n> > +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };\n> > +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };\n> > +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };\n> > +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };\n>\n> This could then be dropped, or possibly moved (without the initializers)\n> to color_space.cpp if there's a need to bind a reference or take the\n> address of these variables.\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 5b25ef84..7a8a04e5 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -3,6 +3,7 @@\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..d1ccb4cc\n> > --- /dev/null\n> > +++ b/src/libcamera/color_space.cpp\n> > @@ -0,0 +1,207 @@\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>\n> This should go first, to ensure that the header file is self-contained.\n>\n> > +\n> > +/**\n> > + * \\file color_space.h\n> > + * \\brief Class and enums to represent colour spaces.\n>\n> No period at the end of briefs. Same below.\n>\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class ColorSpace\n> > + * \\brief Class to describe a color space.\n> > + *\n> > + * The color space class defines the encodings of the color primaries, the\n>\n> s/color space/ColorSpace/\n>\n> You use both color and colour. For the class name, ColorSpace seems\n> right (well, it seems wrong to me, but it seems to be the right choice\n> :-)). For the text, we'll likely have to go through the documentation\n> one day to make everything consistent, and will unfortunately likely\n> have to pick the US spelling. This hasn't been completely decided yet,\n> so I've fine with either for now, but let's make it consistent within\n> the file.\n\nI tend to go American in the code and then UK in the text, but I can\ngo all-American, it's fine!\n\n>\n> > + * transfer function associated with the color space, and the range (sometimes\n> > + * also referred to as the quantisation) of the color space.\n> > + *\n> > + * Certain combinations of these fields form well-known standard color spaces,\n> > + * such as \"JFIF\" or \"REC709\", though there is flexibility to leave some or all\n> > + * of them undefined too.\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::Encoding\n> > + * \\brief The encoding used for the color primaries.\n>\n> Can we name this Primaries ? Encoding is strongly tied to the Y'CbCr\n> encoding from R'G'B' values, I think it would be confusing.\n>\n> Now that I've written this, I realize (after reading the rest of the\n> series) that you're using this as the actual Y'CbCr encoding. The name\n> is thus fine, but it shouldn't mention colour primaries then, it should\n> be defined as Y'CbCr encoding. I would also move the definition of the\n> Encoding enum after TransferFunction, as that's the order in which\n> they're applied.\n>\n> This leads me to the next question: what should we do with the colour\n> primaries (and white point) ? Those are part of the colour space\n> definition.\n\nYes, we do need a field for primaries, my bad for not including it. I\nguess I'd imagined wrapping it up into the encoding as they often go\ntogether, but that's not right.\n\nLooking at the colour spaces we have in mind, I think we'll need\nvalues for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or \"SMPTE-C\"\nfor SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.\n\nI don't think we need to do anything about the white point, that's\ndetermined for you once you have your primaries I believe.\n\n>\n> > + *\n> > + * \\var ColorSpace::Encoding::UNDEFINED\n> > + * \\brief The encoding for the colour primaries is not specified.\n>\n> I think we need to include more usage information. Can an application\n> set the encoding to undefined ? What will happen then ? Can a camera\n> return undefined ?\n>\n> > + * \\var ColorSpace::Encoding::RAW\n> > + * \\brief These are raw colours from the sensor.\n>\n> Will this also apply to RGB formats ?\n>\n> > + * \\var ColorSpace::Encoding::REC601\n> > + * \\brief REC601 colour primaries.\n>\n> s/REC601/Rec. 601/\n>\n> or possibly \"ITU-R Rec. 601\" or \"ITU-R Rec. BT.601\" if we want to be\n> precise. Same below.\n>\n> Should we also include the encoding formula explicitly, or would that be\n> too much details ?\n\nMaybe some web links instead, to avoid duplication?\n\n>\n> > + * \\var ColorSpace::Encoding::REC709\n> > + * \\brief Rec709 colour primaries.\n> > + * \\var ColorSpace::Encoding::REC2020\n> > + * \\brief REC2020 colour primaries.\n> > + * \\var ColorSpace::Encoding::VIDEO\n> > + * \\brief A place-holder for video streams which will be resolved to one\n> > + * of REC601, REC709 or REC2020 once the video resolution is known.\n>\n> We also need more information here. What's your expected use case for\n> this ? I'm concerned it may seem safe for applications to pick this as a\n> default, but they won't care much about colour spaces then, and will\n> likely get things wrong.\n\nSo the idea was that generateConfiguration would use VIDEO when the\nstream role is \"video\", so that it could be resolved later once the\nvideo resolution is known. That way, application code would almost\nnever have to deal with colour spaces because you'd do\n\nconfiguration = camera->generateConfiguration(roles);\n/* set up formats and resolutions *./\ncamera->configure(configuration);\n\nand for JPEG, mpeg/h26x video it would all work out correctly. The\nonly case you'd have to worry about would be something like mjpeg,\nwhere you'd have to set the colour space explicitly to JPEG/JFIF.\n\nNote that we would have been able to leave it as UNDEFINED (rather\nthan inventing VIDEO) but for the fact that the configuration\ngenerated doesn't record that it was intended for video.\n\nSlightly unhappily, we will need to do the same with the (proposed)\nPrimaries field, as that too can change depending on the video\nresolution. Or perhaps it would be better for a stream configuration\nto remember its \"role\"? I could go with that.\n\n>\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::TransferFunction\n> > + * \\brief The transfer function used for this colour space.\n> > + *\n> > + * \\var ColorSpace::TransferFunction::UNDEFINED\n> > + * \\brief The transfer function is not specified.\n> > + * \\var ColorSpace::TransferFunction::IDENTITY\n> > + * \\brief This color space uses an identity transfer function.\n>\n> Isn't \"linear\" a more common term than \"identity\" for the transfer\n> function ?\n>\n> > + * \\var ColorSpace::TransferFunction::SRGB\n> > + * \\brief sRGB transfer function.\n> > + * \\var ColorSpace::TransferFunction::REC709\n> > + * \\brief Rec709 transfer function.\n> > + */\n> > +\n> > +/**\n> > + * \\enum ColorSpace::Range\n> > + * \\brief The range (sometimes \"quantisation\") for this color space.\n>\n> Technically speaking, the quantisation isn't part of the colour space,\n> but I fear it would bring lots of complexity and little gain to try and\n> keep it separate.\n>\n> > + *\n> > + * \\var ColorSpace::Range::UNDEFINED\n> > + * \\brief The range is not specified.\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.\n>\n> Should we define these in more details as well, with the exact range ?\n\nYes, makes sense. Or possibly web links, will have a look!\n\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n> > + * \\brief Construct a ColorSpace from explicit values\n> > + * \\param[in] e The encoding for the color primaries\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> > + * \\fn ColorSpace::ColorSpace()\n> > + * \\brief Construct a color space with undefined encoding, transfer function\n> > + * and range\n> > + */\n> > +\n> > +/**\n> > + * \\brief Return true if all the fields of the color space are defined, otherwise false.\n>\n> Line wrap. You also need a \\return, so I'd write \\brief as \"Check if\n> ...\".\n>\n> > + */\n> > +bool ColorSpace::isFullyDefined() const\n> > +{\n> > + return encoding != Encoding::UNDEFINED &&\n> > + transferFunction != TransferFunction::UNDEFINED &&\n> > + range != Range::UNDEFINED;\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> > + static const char *encodings[] = {\n> > + \"UNDEFINED\",\n> > + \"RAW\",\n> > + \"REC601\",\n> > + \"REC709\",\n> > + \"REC2020\",\n> > + };\n> > + static const char *transferFunctions[] = {\n> > + \"UNDEFINED\",\n> > + \"IDENTITY\",\n> > + \"SRGB\",\n> > + \"REC709\",\n> > + };\n> > + static const char *ranges[] = {\n> > + \"UNDEFINED\",\n> > + \"FULL\",\n> > + \"LIMITED\",\n> > + };\n> > +\n> > + std::stringstream ss;\n> > + ss << std::string(encodings[static_cast(encoding)]) << \"+\"\n>\n> \"+\" seems a bit of a weird separator, I would have expected \"/\". Is\n> there a specific reason ?\n>\n> > + << std::string(transferFunctions[static_cast(transferFunction)]) << \"+\"\n> > + << std::string(ranges[static_cast(range)]);\n> > +\n> > + return ss.str();\n> > +}\n> > +\n> > +/**\n> > + * \\var ColorSpace::encoding\n> > + * \\brief The encoding of the color primaries\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::JFIF\n> > + * \\brief A constant representing the JFIF color space usually used for\n> > + * encoding JPEG images.\n>\n> Should this and the colour spaces below be defined more precisely in the\n> documentation, or do you think users can just look at the values of the\n> different members to figure out which is which ?\n>\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::SMPTE170M\n> > + * \\brief A constant representing the SMPTE170M color space (sometimes also\n> > + * referred to as \"full range BT601\").\n>\n> Even though quantization is limited range ?\n\nI shouldn't refer to SMPTE170M as a kind of \"full range BT601\", even\nthough rather lazily I sometimes think that. BT601 doesn't define\nprimaries, so it's misleading.\n\n>\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::REC709\n> > + * \\brief A constant representing the REC709 color space.\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::REC2020\n> > + * \\brief A constant representing the REC2020 color space.\n> > + */\n> > +\n> > +/**\n> > + * \\var ColorSpace::VIDEO\n> > + * \\brief A constant that video streams can use to indicate the \"default\"\n> > + * color space for a video of this resolution, once that is is known. For\n> > + * exmample, SD streams would interpret this as SMPTE170M, HD streams as\n>\n> s/exmample/example/\n>\n> > + * REC709 and ultra HD as REC2020.\n>\n> As mentioned above, this sounds a bit dangerous to me, but maybe I worry\n> too much. If we want to keep this, I think we need to make the\n> definition stricter, by documenting the required behaviour (thus\n> removing \"for example\", and defining SD and HD).\n\nOr maybe store the stream role in the configuration...?\n\nLet's continue the discussion, but in the meantime I'll put together a\nv3 with most of the other points addressed.\n\nThanks!\n\nDavid\n\n>\n> > + */\n> > +\n> > +/**\n> > + * \\brief Compare color spaces for equality\n>\n> *\n> * Color spaces are considered identical if they have the same\n> * \\ref transferFunction, \\ref encoding and \\ref range.\n> *\n>\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.encoding == rhs.encoding &&\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 bf82d38b..88dfbf24 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> --\n> Regards,\n>\n> Laurent Pinchart","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 6761AC323E\n\tfor ;\n\tMon, 11 Oct 2021 14:16:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6F6D68F50;\n\tMon, 11 Oct 2021 16:16:48 +0200 (CEST)","from mail-wr1-x429.google.com (mail-wr1-x429.google.com\n\t[IPv6:2a00:1450:4864:20::429])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F8A56012B\n\tfor ;\n\tMon, 11 Oct 2021 16:16:47 +0200 (CEST)","by mail-wr1-x429.google.com with SMTP id k7so56604715wrd.13\n\tfor ;\n\tMon, 11 Oct 2021 07:16:47 -0700 (PDT)"],"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=\"YwzzbvGU\"; 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=jjfdJEQngR/0IOBo2aX0QTj8G6TB3DHcBfeqwJnrdUI=;\n\tb=YwzzbvGUCcgbwO3f2e7oR5jDmyJH7cetBBfjm8GWJbLVOL8sTszcU8pK4rmned4kRY\n\tRupwPpvDwup59YjUYqRZWsJ583LZYBxGarfSVrCqosXhymcHefmmsdnx5F+m55/FZmgY\n\tShz83TZF6Sgt8dl+TB8/Eq2SDJqFcadC+TDEVbeJVArAT0i1iU+c5FCN4kubX/MmV2Ff\n\tVcRdA3PY240XFEJMXHZviitveFTCTzI7QTsSMQY0Rt1kM62HdWVV+Z6p6a+0Njs5b4Pd\n\tNGsigQJvcpfPrwI+Umw+Lr6QB6olvG/6i7B6JLk6Z8EkzDjnwiMUwafdlzZneAGgpiKX\n\t3s6w==","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=jjfdJEQngR/0IOBo2aX0QTj8G6TB3DHcBfeqwJnrdUI=;\n\tb=s9a59T8rJh4hcba2m9xzqTVMcCk3Rt7/7GLl+i+kPlPAhAZSYpl2AltLACKdUCSNTE\n\tS5DWP9pxHPY8yjPyu7FgIy1V+qNbnkGxNSAn/EF2T/16P6yF2vPI3uEHu5rzzlA9RQn5\n\t86j+jCtVHRID7Bgh1Hmgmxu+Iy0BzNp7hAm1EBuugZo0ct3uBe+GqkREImvlPC6z6hEs\n\tH8A34ymSMbJYshMsz8oAiakexuGWeLvDwlkK6+O2qrGJfDZZhlEBTQ4xIphpAlB/4ymU\n\tyQW1tJ4LKfXcS/lc8STc9ozPDaj2MHVS5LSt1NQl7ymRnYcuxboOiK15Ys4RZOc04hbd\n\t0I+A==","X-Gm-Message-State":"AOAM532RZyjbOii7MZYmyJ1jVY3lBPPBb4eWQ0BoykVMktftNRBMGRF4\n\thmZopBAxeZ7+8HI/qmVDiv2FCYikbqVB0dtcGQ3Q1w==","X-Google-Smtp-Source":"ABdhPJwmaHF6mHOSCf8rDUdaoYrdharxTLZmuRMtas7TFyDSMcslHDqPxh9jr6tEdk+mNuFe9Uv+85U0Qwn4wMye6No=","X-Received":"by 2002:a05:600c:3b1a:: with SMTP id\n\tm26mr8876947wms.130.1633961806428; \n\tMon, 11 Oct 2021 07:16:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927123327.14554-1-david.plowman@raspberrypi.com>\n\t<20210927123327.14554-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 11 Oct 2021 15:16:35 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20116,"web_url":"https://patchwork.libcamera.org/comment/20116/","msgid":"","date":"2021-10-11T14:31:27","subject":"Re: [libcamera-devel] [PATCH v2 1/3] libcamera: Add ColorSpace class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Mon, Oct 11, 2021 at 03:16:35PM +0100, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for the detailed review of all this. I probably won't comment\n> on some of the more \"trivial\" items (I'll just do a v3!) but some of\n> the points do indeed want some more discussion.\n> \n> On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart wrote:\n> >\n> > Hi David,\n> >\n> > (CC'ing Hans, the colour space expert in V4L2)\n> >\n> > Thank you for the patch.\n> >\n> > Hans, if you have a bit of time to look at this, your feedback would be\n> > appreciated.\n> >\n> > On Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:\n> > > This class represents a colour space by defining its YCbCr encoding,\n> > > the transfer (gamma) function is uses, and whether the output is full\n> >\n> > s/is/it/\n> >\n> > > or limited range.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/color_space.h | 83 +++++++++++++\n> > > include/libcamera/meson.build | 1 +\n> > > src/libcamera/color_space.cpp | 207 ++++++++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 292 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..9cd10503\n> > > --- /dev/null\n> > > +++ b/include/libcamera/color_space.h\n> > > @@ -0,0 +1,83 @@\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> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class ColorSpace\n> > > +{\n> > > +public:\n> > > + enum class Encoding : int {\n> > > + UNDEFINED,\n> >\n> > We typically use CamelCase for enumerators.\n> >\n> > > + RAW,\n> > > + REC601,\n> > > + REC709,\n> > > + REC2020,\n> > > + VIDEO,\n> > > + };\n> > > +\n> > > + enum class TransferFunction : int {\n> > > + UNDEFINED,\n> > > + IDENTITY,\n> > > + SRGB,\n> > > + REC709,\n> > > + };\n> > > +\n> > > + enum class Range : int {\n> > > + UNDEFINED,\n> > > + FULL,\n> > > + LIMITED,\n> > > + };\n> > > +\n> > > + constexpr ColorSpace(Encoding e, TransferFunction t, Range r)\n> > > + : encoding(e), transferFunction(t), range(r)\n> > > + {\n> > > + }\n> > > +\n> > > + constexpr ColorSpace()\n> > > + : ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)\n> >\n> > Line wrap ?\n> >\n> > > + {\n> > > + }\n> >\n> > I'd move the default constructor furst.\n> >\n> > > +\n> > > + static const ColorSpace UNDEFINED;\n> > > + static const ColorSpace RAW;\n> > > + static const ColorSpace JFIF;\n> > > + static const ColorSpace SMPTE170M;\n> > > + static const ColorSpace REC709;\n> > > + static const ColorSpace REC2020;\n> > > + static const ColorSpace VIDEO;\n> >\n> > Shouldn't these be defined as static constexpr with a value here ?\n> >\n> > > +\n> > > + Encoding encoding;\n> > > + TransferFunction transferFunction;\n> > > + Range range;\n> > > +\n> > > + bool isFullyDefined() const;\n> > > +\n> > > + const std::string toString() const;\n> > > +};\n> > > +\n> > > +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };\n> > > +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };\n> > > +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };\n> > > +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };\n> > > +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };\n> > > +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };\n> > > +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };\n> >\n> > This could then be dropped, or possibly moved (without the initializers)\n> > to color_space.cpp if there's a need to bind a reference or take the\n> > address of these variables.\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 5b25ef84..7a8a04e5 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -3,6 +3,7 @@\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..d1ccb4cc\n> > > --- /dev/null\n> > > +++ b/src/libcamera/color_space.cpp\n> > > @@ -0,0 +1,207 @@\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> >\n> > This should go first, to ensure that the header file is self-contained.\n> >\n> > > +\n> > > +/**\n> > > + * \\file color_space.h\n> > > + * \\brief Class and enums to represent colour spaces.\n> >\n> > No period at the end of briefs. Same below.\n> >\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class ColorSpace\n> > > + * \\brief Class to describe a color space.\n> > > + *\n> > > + * The color space class defines the encodings of the color primaries, the\n> >\n> > s/color space/ColorSpace/\n> >\n> > You use both color and colour. For the class name, ColorSpace seems\n> > right (well, it seems wrong to me, but it seems to be the right choice\n> > :-)). For the text, we'll likely have to go through the documentation\n> > one day to make everything consistent, and will unfortunately likely\n> > have to pick the US spelling. This hasn't been completely decided yet,\n> > so I've fine with either for now, but let's make it consistent within\n> > the file.\n> \n> I tend to go American in the code and then UK in the text, but I can\n> go all-American, it's fine!\n\nMy point was that the text mixes color and colour. I'm fine if you use\nthe US spelling in code and UK in documentation for now (even though we\nwill likely move everything to the US spelling at some point, to my\ndismay), as long as the documentation is consistent with itself.\n\n> > > + * transfer function associated with the color space, and the range (sometimes\n> > > + * also referred to as the quantisation) of the color space.\n> > > + *\n> > > + * Certain combinations of these fields form well-known standard color spaces,\n> > > + * such as \"JFIF\" or \"REC709\", though there is flexibility to leave some or all\n> > > + * of them undefined too.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::Encoding\n> > > + * \\brief The encoding used for the color primaries.\n> >\n> > Can we name this Primaries ? Encoding is strongly tied to the Y'CbCr\n> > encoding from R'G'B' values, I think it would be confusing.\n> >\n> > Now that I've written this, I realize (after reading the rest of the\n> > series) that you're using this as the actual Y'CbCr encoding. The name\n> > is thus fine, but it shouldn't mention colour primaries then, it should\n> > be defined as Y'CbCr encoding. I would also move the definition of the\n> > Encoding enum after TransferFunction, as that's the order in which\n> > they're applied.\n> >\n> > This leads me to the next question: what should we do with the colour\n> > primaries (and white point) ? Those are part of the colour space\n> > definition.\n> \n> Yes, we do need a field for primaries, my bad for not including it. I\n> guess I'd imagined wrapping it up into the encoding as they often go\n> together, but that's not right.\n> \n> Looking at the colour spaces we have in mind, I think we'll need\n> values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or \"SMPTE-C\"\n> for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.\n> \n> I don't think we need to do anything about the white point, that's\n> determined for you once you have your primaries I believe.\n\nI'm not sure if the white point is a direct result of the chromaticity\ncoordinates of the primaries, but it's defined in the above\nspecifications, so if we go for presets it doesn't need to be handled\nseparately.\n\n> > > + *\n> > > + * \\var ColorSpace::Encoding::UNDEFINED\n> > > + * \\brief The encoding for the colour primaries is not specified.\n> >\n> > I think we need to include more usage information. Can an application\n> > set the encoding to undefined ? What will happen then ? Can a camera\n> > return undefined ?\n> >\n> > > + * \\var ColorSpace::Encoding::RAW\n> > > + * \\brief These are raw colours from the sensor.\n> >\n> > Will this also apply to RGB formats ?\n> >\n> > > + * \\var ColorSpace::Encoding::REC601\n> > > + * \\brief REC601 colour primaries.\n> >\n> > s/REC601/Rec. 601/\n> >\n> > or possibly \"ITU-R Rec. 601\" or \"ITU-R Rec. BT.601\" if we want to be\n> > precise. Same below.\n> >\n> > Should we also include the encoding formula explicitly, or would that be\n> > too much details ?\n> \n> Maybe some web links instead, to avoid duplication?\n\nIf you can find web links that are standard and stable enough, that's\nfine with me.\n\n> > > + * \\var ColorSpace::Encoding::REC709\n> > > + * \\brief Rec709 colour primaries.\n> > > + * \\var ColorSpace::Encoding::REC2020\n> > > + * \\brief REC2020 colour primaries.\n> > > + * \\var ColorSpace::Encoding::VIDEO\n> > > + * \\brief A place-holder for video streams which will be resolved to one\n> > > + * of REC601, REC709 or REC2020 once the video resolution is known.\n> >\n> > We also need more information here. What's your expected use case for\n> > this ? I'm concerned it may seem safe for applications to pick this as a\n> > default, but they won't care much about colour spaces then, and will\n> > likely get things wrong.\n> \n> So the idea was that generateConfiguration would use VIDEO when the\n> stream role is \"video\", so that it could be resolved later once the\n> video resolution is known. That way, application code would almost\n> never have to deal with colour spaces because you'd do\n> \n> configuration = camera->generateConfiguration(roles);\n> /* set up formats and resolutions *./\n> camera->configure(configuration);\n> \n> and for JPEG, mpeg/h26x video it would all work out correctly. The\n> only case you'd have to worry about would be something like mjpeg,\n> where you'd have to set the colour space explicitly to JPEG/JFIF.\n> \n> Note that we would have been able to leave it as UNDEFINED (rather\n> than inventing VIDEO) but for the fact that the configuration\n> generated doesn't record that it was intended for video.\n> \n> Slightly unhappily, we will need to do the same with the (proposed)\n> Primaries field, as that too can change depending on the video\n> resolution. Or perhaps it would be better for a stream configuration\n> to remember its \"role\"? I could go with that.\n\nRoles were not meant to be remembered, and may actually go away in their\ncurrent form, so I'd rather not rely on them.\n\nOne part that bothers me a bit is that the VIDEO encoding needs to be\nresolved by pipeline handlers, which will end up copying each other,\nwith their own little bugs of course. Could we try to at least\ncentralize that ?\n\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::TransferFunction\n> > > + * \\brief The transfer function used for this colour space.\n> > > + *\n> > > + * \\var ColorSpace::TransferFunction::UNDEFINED\n> > > + * \\brief The transfer function is not specified.\n> > > + * \\var ColorSpace::TransferFunction::IDENTITY\n> > > + * \\brief This color space uses an identity transfer function.\n> >\n> > Isn't \"linear\" a more common term than \"identity\" for the transfer\n> > function ?\n> >\n> > > + * \\var ColorSpace::TransferFunction::SRGB\n> > > + * \\brief sRGB transfer function.\n> > > + * \\var ColorSpace::TransferFunction::REC709\n> > > + * \\brief Rec709 transfer function.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\enum ColorSpace::Range\n> > > + * \\brief The range (sometimes \"quantisation\") for this color space.\n> >\n> > Technically speaking, the quantisation isn't part of the colour space,\n> > but I fear it would bring lots of complexity and little gain to try and\n> > keep it separate.\n> >\n> > > + *\n> > > + * \\var ColorSpace::Range::UNDEFINED\n> > > + * \\brief The range is not specified.\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.\n> >\n> > Should we define these in more details as well, with the exact range ?\n> \n> Yes, makes sense. Or possibly web links, will have a look!\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n> > > + * \\brief Construct a ColorSpace from explicit values\n> > > + * \\param[in] e The encoding for the color primaries\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> > > + * \\fn ColorSpace::ColorSpace()\n> > > + * \\brief Construct a color space with undefined encoding, transfer function\n> > > + * and range\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Return true if all the fields of the color space are defined, otherwise false.\n> >\n> > Line wrap. You also need a \\return, so I'd write \\brief as \"Check if\n> > ...\".\n> >\n> > > + */\n> > > +bool ColorSpace::isFullyDefined() const\n> > > +{\n> > > + return encoding != Encoding::UNDEFINED &&\n> > > + transferFunction != TransferFunction::UNDEFINED &&\n> > > + range != Range::UNDEFINED;\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> > > + static const char *encodings[] = {\n> > > + \"UNDEFINED\",\n> > > + \"RAW\",\n> > > + \"REC601\",\n> > > + \"REC709\",\n> > > + \"REC2020\",\n> > > + };\n> > > + static const char *transferFunctions[] = {\n> > > + \"UNDEFINED\",\n> > > + \"IDENTITY\",\n> > > + \"SRGB\",\n> > > + \"REC709\",\n> > > + };\n> > > + static const char *ranges[] = {\n> > > + \"UNDEFINED\",\n> > > + \"FULL\",\n> > > + \"LIMITED\",\n> > > + };\n> > > +\n> > > + std::stringstream ss;\n> > > + ss << std::string(encodings[static_cast(encoding)]) << \"+\"\n> >\n> > \"+\" seems a bit of a weird separator, I would have expected \"/\". Is\n> > there a specific reason ?\n> >\n> > > + << std::string(transferFunctions[static_cast(transferFunction)]) << \"+\"\n> > > + << std::string(ranges[static_cast(range)]);\n> > > +\n> > > + return ss.str();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::encoding\n> > > + * \\brief The encoding of the color primaries\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::JFIF\n> > > + * \\brief A constant representing the JFIF color space usually used for\n> > > + * encoding JPEG images.\n> >\n> > Should this and the colour spaces below be defined more precisely in the\n> > documentation, or do you think users can just look at the values of the\n> > different members to figure out which is which ?\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::SMPTE170M\n> > > + * \\brief A constant representing the SMPTE170M color space (sometimes also\n> > > + * referred to as \"full range BT601\").\n> >\n> > Even though quantization is limited range ?\n> \n> I shouldn't refer to SMPTE170M as a kind of \"full range BT601\", even\n> though rather lazily I sometimes think that. BT601 doesn't define\n> primaries, so it's misleading.\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::REC709\n> > > + * \\brief A constant representing the REC709 color space.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::REC2020\n> > > + * \\brief A constant representing the REC2020 color space.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var ColorSpace::VIDEO\n> > > + * \\brief A constant that video streams can use to indicate the \"default\"\n> > > + * color space for a video of this resolution, once that is is known. For\n> > > + * exmample, SD streams would interpret this as SMPTE170M, HD streams as\n> >\n> > s/exmample/example/\n> >\n> > > + * REC709 and ultra HD as REC2020.\n> >\n> > As mentioned above, this sounds a bit dangerous to me, but maybe I worry\n> > too much. If we want to keep this, I think we need to make the\n> > definition stricter, by documenting the required behaviour (thus\n> > removing \"for example\", and defining SD and HD).\n> \n> Or maybe store the stream role in the configuration...?\n> \n> Let's continue the discussion, but in the meantime I'll put together a\n> v3 with most of the other points addressed.\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Compare color spaces for equality\n> >\n> > *\n> > * Color spaces are considered identical if they have the same\n> > * \\ref transferFunction, \\ref encoding and \\ref range.\n> > *\n> >\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.encoding == rhs.encoding &&\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 bf82d38b..88dfbf24 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 35E47BDC71\n\tfor ;\n\tMon, 11 Oct 2021 14:31:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AED1D68F4C;\n\tMon, 11 Oct 2021 16:31:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 554656012B\n\tfor ;\n\tMon, 11 Oct 2021 16:31:40 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C93772A8;\n\tMon, 11 Oct 2021 16:31:39 +0200 (CEST)"],"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=\"DChRTFwA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633962700;\n\tbh=KRaSxxKd4o8IKZ7sB4a5lCCXdJ2t3xCkPnbqQ1A6Rmo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DChRTFwAcEVma/YSr4DTfOH7o5DuqfHGTzjYc9kJwdMd+nm4mn2V8vLeFA4WITYvt\n\tp03rpPC4tFMS8ENWuClk6iQo2Jw0XhPfSTvHwHfZ+8F33a5nZRMo86zkm6YZxz/+nB\n\tiHVyb2vOyFZKD5kHFexP761Upbk+h562/fqTsGVE=","Date":"Mon, 11 Oct 2021 17:31:27 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210927123327.14554-1-david.plowman@raspberrypi.com>\n\t<20210927123327.14554-2-david.plowman@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20118,"web_url":"https://patchwork.libcamera.org/comment/20118/","msgid":"","date":"2021-10-11T15:05:13","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 again Laurent\n\nOn Mon, 11 Oct 2021 at 15:31, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Mon, Oct 11, 2021 at 03:16:35PM +0100, David Plowman wrote:\n> > Hi Laurent\n> >\n> > Thanks for the detailed review of all this. I probably won't comment\n> > on some of the more \"trivial\" items (I'll just do a v3!) but some of\n> > the points do indeed want some more discussion.\n> >\n> > On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart wrote:\n> > >\n> > > Hi David,\n> > >\n> > > (CC'ing Hans, the colour space expert in V4L2)\n> > >\n> > > Thank you for the patch.\n> > >\n> > > Hans, if you have a bit of time to look at this, your feedback would be\n> > > appreciated.\n> > >\n> > > On Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:\n> > > > This class represents a colour space by defining its YCbCr encoding,\n> > > > the transfer (gamma) function is uses, and whether the output is full\n> > >\n> > > s/is/it/\n> > >\n> > > > or limited range.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > include/libcamera/color_space.h | 83 +++++++++++++\n> > > > include/libcamera/meson.build | 1 +\n> > > > src/libcamera/color_space.cpp | 207 ++++++++++++++++++++++++++++++++\n> > > > src/libcamera/meson.build | 1 +\n> > > > 4 files changed, 292 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..9cd10503\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/color_space.h\n> > > > @@ -0,0 +1,83 @@\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> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class ColorSpace\n> > > > +{\n> > > > +public:\n> > > > + enum class Encoding : int {\n> > > > + UNDEFINED,\n> > >\n> > > We typically use CamelCase for enumerators.\n> > >\n> > > > + RAW,\n> > > > + REC601,\n> > > > + REC709,\n> > > > + REC2020,\n> > > > + VIDEO,\n> > > > + };\n> > > > +\n> > > > + enum class TransferFunction : int {\n> > > > + UNDEFINED,\n> > > > + IDENTITY,\n> > > > + SRGB,\n> > > > + REC709,\n> > > > + };\n> > > > +\n> > > > + enum class Range : int {\n> > > > + UNDEFINED,\n> > > > + FULL,\n> > > > + LIMITED,\n> > > > + };\n> > > > +\n> > > > + constexpr ColorSpace(Encoding e, TransferFunction t, Range r)\n> > > > + : encoding(e), transferFunction(t), range(r)\n> > > > + {\n> > > > + }\n> > > > +\n> > > > + constexpr ColorSpace()\n> > > > + : ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)\n> > >\n> > > Line wrap ?\n> > >\n> > > > + {\n> > > > + }\n> > >\n> > > I'd move the default constructor furst.\n> > >\n> > > > +\n> > > > + static const ColorSpace UNDEFINED;\n> > > > + static const ColorSpace RAW;\n> > > > + static const ColorSpace JFIF;\n> > > > + static const ColorSpace SMPTE170M;\n> > > > + static const ColorSpace REC709;\n> > > > + static const ColorSpace REC2020;\n> > > > + static const ColorSpace VIDEO;\n> > >\n> > > Shouldn't these be defined as static constexpr with a value here ?\n> > >\n> > > > +\n> > > > + Encoding encoding;\n> > > > + TransferFunction transferFunction;\n> > > > + Range range;\n> > > > +\n> > > > + bool isFullyDefined() const;\n> > > > +\n> > > > + const std::string toString() const;\n> > > > +};\n> > > > +\n> > > > +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };\n> > > > +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };\n> > > > +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };\n> > > > +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };\n> > > > +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };\n> > > > +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };\n> > > > +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };\n> > >\n> > > This could then be dropped, or possibly moved (without the initializers)\n> > > to color_space.cpp if there's a need to bind a reference or take the\n> > > address of these variables.\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 5b25ef84..7a8a04e5 100644\n> > > > --- a/include/libcamera/meson.build\n> > > > +++ b/include/libcamera/meson.build\n> > > > @@ -3,6 +3,7 @@\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..d1ccb4cc\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/color_space.cpp\n> > > > @@ -0,0 +1,207 @@\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> > >\n> > > This should go first, to ensure that the header file is self-contained.\n> > >\n> > > > +\n> > > > +/**\n> > > > + * \\file color_space.h\n> > > > + * \\brief Class and enums to represent colour spaces.\n> > >\n> > > No period at the end of briefs. Same below.\n> > >\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +/**\n> > > > + * \\class ColorSpace\n> > > > + * \\brief Class to describe a color space.\n> > > > + *\n> > > > + * The color space class defines the encodings of the color primaries, the\n> > >\n> > > s/color space/ColorSpace/\n> > >\n> > > You use both color and colour. For the class name, ColorSpace seems\n> > > right (well, it seems wrong to me, but it seems to be the right choice\n> > > :-)). For the text, we'll likely have to go through the documentation\n> > > one day to make everything consistent, and will unfortunately likely\n> > > have to pick the US spelling. This hasn't been completely decided yet,\n> > > so I've fine with either for now, but let's make it consistent within\n> > > the file.\n> >\n> > I tend to go American in the code and then UK in the text, but I can\n> > go all-American, it's fine!\n>\n> My point was that the text mixes color and colour. I'm fine if you use\n> the US spelling in code and UK in documentation for now (even though we\n> will likely move everything to the US spelling at some point, to my\n> dismay), as long as the documentation is consistent with itself.\n>\n> > > > + * transfer function associated with the color space, and the range (sometimes\n> > > > + * also referred to as the quantisation) of the color space.\n> > > > + *\n> > > > + * Certain combinations of these fields form well-known standard color spaces,\n> > > > + * such as \"JFIF\" or \"REC709\", though there is flexibility to leave some or all\n> > > > + * of them undefined too.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\enum ColorSpace::Encoding\n> > > > + * \\brief The encoding used for the color primaries.\n> > >\n> > > Can we name this Primaries ? Encoding is strongly tied to the Y'CbCr\n> > > encoding from R'G'B' values, I think it would be confusing.\n> > >\n> > > Now that I've written this, I realize (after reading the rest of the\n> > > series) that you're using this as the actual Y'CbCr encoding. The name\n> > > is thus fine, but it shouldn't mention colour primaries then, it should\n> > > be defined as Y'CbCr encoding. I would also move the definition of the\n> > > Encoding enum after TransferFunction, as that's the order in which\n> > > they're applied.\n> > >\n> > > This leads me to the next question: what should we do with the colour\n> > > primaries (and white point) ? Those are part of the colour space\n> > > definition.\n> >\n> > Yes, we do need a field for primaries, my bad for not including it. I\n> > guess I'd imagined wrapping it up into the encoding as they often go\n> > together, but that's not right.\n> >\n> > Looking at the colour spaces we have in mind, I think we'll need\n> > values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or \"SMPTE-C\"\n> > for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.\n> >\n> > I don't think we need to do anything about the white point, that's\n> > determined for you once you have your primaries I believe.\n>\n> I'm not sure if the white point is a direct result of the chromaticity\n> coordinates of the primaries, but it's defined in the above\n> specifications, so if we go for presets it doesn't need to be handled\n> separately.\n\nIn general we have a 3x3 RGB to XYZ matrix (for example\nhttp://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html). The\nprimaries are what we get when we apply that matrix to red (1,0,0),\ngreen (0,1,0) and blue (0,0,1). So the result of applying the matrix\nto (1,1,1) is now pre-determined, and is the white point. Someone\nplease correct me if I understood that less well than I thought! :)\n\nBut I think going with presets is the thing to do, trying to do\non-the-fly conversions of CCMs to arbitrary colour spaces sounds quite\nfraught... (and we certainly would want to opt out of implementing\nanything like that)\n\n>\n> > > > + *\n> > > > + * \\var ColorSpace::Encoding::UNDEFINED\n> > > > + * \\brief The encoding for the colour primaries is not specified.\n> > >\n> > > I think we need to include more usage information. Can an application\n> > > set the encoding to undefined ? What will happen then ? Can a camera\n> > > return undefined ?\n> > >\n> > > > + * \\var ColorSpace::Encoding::RAW\n> > > > + * \\brief These are raw colours from the sensor.\n> > >\n> > > Will this also apply to RGB formats ?\n> > >\n> > > > + * \\var ColorSpace::Encoding::REC601\n> > > > + * \\brief REC601 colour primaries.\n> > >\n> > > s/REC601/Rec. 601/\n> > >\n> > > or possibly \"ITU-R Rec. 601\" or \"ITU-R Rec. BT.601\" if we want to be\n> > > precise. Same below.\n> > >\n> > > Should we also include the encoding formula explicitly, or would that be\n> > > too much details ?\n> >\n> > Maybe some web links instead, to avoid duplication?\n>\n> If you can find web links that are standard and stable enough, that's\n> fine with me.\n>\n> > > > + * \\var ColorSpace::Encoding::REC709\n> > > > + * \\brief Rec709 colour primaries.\n> > > > + * \\var ColorSpace::Encoding::REC2020\n> > > > + * \\brief REC2020 colour primaries.\n> > > > + * \\var ColorSpace::Encoding::VIDEO\n> > > > + * \\brief A place-holder for video streams which will be resolved to one\n> > > > + * of REC601, REC709 or REC2020 once the video resolution is known.\n> > >\n> > > We also need more information here. What's your expected use case for\n> > > this ? I'm concerned it may seem safe for applications to pick this as a\n> > > default, but they won't care much about colour spaces then, and will\n> > > likely get things wrong.\n> >\n> > So the idea was that generateConfiguration would use VIDEO when the\n> > stream role is \"video\", so that it could be resolved later once the\n> > video resolution is known. That way, application code would almost\n> > never have to deal with colour spaces because you'd do\n> >\n> > configuration = camera->generateConfiguration(roles);\n> > /* set up formats and resolutions *./\n> > camera->configure(configuration);\n> >\n> > and for JPEG, mpeg/h26x video it would all work out correctly. The\n> > only case you'd have to worry about would be something like mjpeg,\n> > where you'd have to set the colour space explicitly to JPEG/JFIF.\n> >\n> > Note that we would have been able to leave it as UNDEFINED (rather\n> > than inventing VIDEO) but for the fact that the configuration\n> > generated doesn't record that it was intended for video.\n> >\n> > Slightly unhappily, we will need to do the same with the (proposed)\n> > Primaries field, as that too can change depending on the video\n> > resolution. Or perhaps it would be better for a stream configuration\n> > to remember its \"role\"? I could go with that.\n>\n> Roles were not meant to be remembered, and may actually go away in their\n> current form, so I'd rather not rely on them.\n>\n> One part that bothers me a bit is that the VIDEO encoding needs to be\n> resolved by pipeline handlers, which will end up copying each other,\n> with their own little bugs of course. Could we try to at least\n> centralize that ?\n\nAh OK, didn't realise that roles might be changing. So what did you\nhave in mind here, maybe a PipelineHandler base class method that you\ncan run at the start of validate() which patches up any colour space\nfields according to the stream's output resolution (if marked as\n\"VIDEO\")?\n\nDavid\n\n>\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\enum ColorSpace::TransferFunction\n> > > > + * \\brief The transfer function used for this colour space.\n> > > > + *\n> > > > + * \\var ColorSpace::TransferFunction::UNDEFINED\n> > > > + * \\brief The transfer function is not specified.\n> > > > + * \\var ColorSpace::TransferFunction::IDENTITY\n> > > > + * \\brief This color space uses an identity transfer function.\n> > >\n> > > Isn't \"linear\" a more common term than \"identity\" for the transfer\n> > > function ?\n> > >\n> > > > + * \\var ColorSpace::TransferFunction::SRGB\n> > > > + * \\brief sRGB transfer function.\n> > > > + * \\var ColorSpace::TransferFunction::REC709\n> > > > + * \\brief Rec709 transfer function.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\enum ColorSpace::Range\n> > > > + * \\brief The range (sometimes \"quantisation\") for this color space.\n> > >\n> > > Technically speaking, the quantisation isn't part of the colour space,\n> > > but I fear it would bring lots of complexity and little gain to try and\n> > > keep it separate.\n> > >\n> > > > + *\n> > > > + * \\var ColorSpace::Range::UNDEFINED\n> > > > + * \\brief The range is not specified.\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.\n> > >\n> > > Should we define these in more details as well, with the exact range ?\n> >\n> > Yes, makes sense. Or possibly web links, will have a look!\n> >\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n> > > > + * \\brief Construct a ColorSpace from explicit values\n> > > > + * \\param[in] e The encoding for the color primaries\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> > > > + * \\fn ColorSpace::ColorSpace()\n> > > > + * \\brief Construct a color space with undefined encoding, transfer function\n> > > > + * and range\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Return true if all the fields of the color space are defined, otherwise false.\n> > >\n> > > Line wrap. You also need a \\return, so I'd write \\brief as \"Check if\n> > > ...\".\n> > >\n> > > > + */\n> > > > +bool ColorSpace::isFullyDefined() const\n> > > > +{\n> > > > + return encoding != Encoding::UNDEFINED &&\n> > > > + transferFunction != TransferFunction::UNDEFINED &&\n> > > > + range != Range::UNDEFINED;\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> > > > + static const char *encodings[] = {\n> > > > + \"UNDEFINED\",\n> > > > + \"RAW\",\n> > > > + \"REC601\",\n> > > > + \"REC709\",\n> > > > + \"REC2020\",\n> > > > + };\n> > > > + static const char *transferFunctions[] = {\n> > > > + \"UNDEFINED\",\n> > > > + \"IDENTITY\",\n> > > > + \"SRGB\",\n> > > > + \"REC709\",\n> > > > + };\n> > > > + static const char *ranges[] = {\n> > > > + \"UNDEFINED\",\n> > > > + \"FULL\",\n> > > > + \"LIMITED\",\n> > > > + };\n> > > > +\n> > > > + std::stringstream ss;\n> > > > + ss << std::string(encodings[static_cast(encoding)]) << \"+\"\n> > >\n> > > \"+\" seems a bit of a weird separator, I would have expected \"/\". Is\n> > > there a specific reason ?\n> > >\n> > > > + << std::string(transferFunctions[static_cast(transferFunction)]) << \"+\"\n> > > > + << std::string(ranges[static_cast(range)]);\n> > > > +\n> > > > + return ss.str();\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\var ColorSpace::encoding\n> > > > + * \\brief The encoding of the color primaries\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::JFIF\n> > > > + * \\brief A constant representing the JFIF color space usually used for\n> > > > + * encoding JPEG images.\n> > >\n> > > Should this and the colour spaces below be defined more precisely in the\n> > > documentation, or do you think users can just look at the values of the\n> > > different members to figure out which is which ?\n> > >\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var ColorSpace::SMPTE170M\n> > > > + * \\brief A constant representing the SMPTE170M color space (sometimes also\n> > > > + * referred to as \"full range BT601\").\n> > >\n> > > Even though quantization is limited range ?\n> >\n> > I shouldn't refer to SMPTE170M as a kind of \"full range BT601\", even\n> > though rather lazily I sometimes think that. BT601 doesn't define\n> > primaries, so it's misleading.\n> >\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var ColorSpace::REC709\n> > > > + * \\brief A constant representing the REC709 color space.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var ColorSpace::REC2020\n> > > > + * \\brief A constant representing the REC2020 color space.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var ColorSpace::VIDEO\n> > > > + * \\brief A constant that video streams can use to indicate the \"default\"\n> > > > + * color space for a video of this resolution, once that is is known. For\n> > > > + * exmample, SD streams would interpret this as SMPTE170M, HD streams as\n> > >\n> > > s/exmample/example/\n> > >\n> > > > + * REC709 and ultra HD as REC2020.\n> > >\n> > > As mentioned above, this sounds a bit dangerous to me, but maybe I worry\n> > > too much. If we want to keep this, I think we need to make the\n> > > definition stricter, by documenting the required behaviour (thus\n> > > removing \"for example\", and defining SD and HD).\n> >\n> > Or maybe store the stream role in the configuration...?\n> >\n> > Let's continue the discussion, but in the meantime I'll put together a\n> > v3 with most of the other points addressed.\n> >\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Compare color spaces for equality\n> > >\n> > > *\n> > > * Color spaces are considered identical if they have the same\n> > > * \\ref transferFunction, \\ref encoding and \\ref range.\n> > > *\n> > >\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.encoding == rhs.encoding &&\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 bf82d38b..88dfbf24 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> --\n> Regards,\n>\n> Laurent Pinchart","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 989E7BDC71\n\tfor ;\n\tMon, 11 Oct 2021 15:05:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED41568F50;\n\tMon, 11 Oct 2021 17:05:28 +0200 (CEST)","from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com\n\t[IPv6:2a00:1450:4864:20::42a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE7A36012B\n\tfor ;\n\tMon, 11 Oct 2021 17:05:26 +0200 (CEST)","by mail-wr1-x42a.google.com with SMTP id g25so10725327wrb.2\n\tfor ;\n\tMon, 11 Oct 2021 08:05:26 -0700 (PDT)"],"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=\"lzoAwPa3\"; 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=akAOaXlfRNCFGpTQqcT8XeXnnC1ABkrh2yCU2KqSeBY=;\n\tb=lzoAwPa3uWCLWkJ3zvbgWuHV2FGAkXiuR8fuc5Um4fc1al+vFeZ87+0I6LDmObvrcG\n\tC73mhDg2ALmYmQQaANGaB2d76Tgn2qXI/OQFeUzUY4vTFrSkFHDo4NXOrUGwR055hKpA\n\tHsedbkatcmZaoX3+HAEDHtEczFsLZYqHe688rUdKyNiTiKXHa5NumOxtDQZLmUQfnxIA\n\tmK+KtmpFzqhk+1wqBOJHW5PfbOKzJY5nnLCd7RLUw9Zx+gYXJmACic4zINdnV0HzFy68\n\t4lW78vNqQORZ/8JqcDHdVGwKq2mFLKVjoyWe0NcwmFMVv7tUCcbCvOwMxJ9y/bMl+u+v\n\tieug==","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=akAOaXlfRNCFGpTQqcT8XeXnnC1ABkrh2yCU2KqSeBY=;\n\tb=osXlRAci98Icwp2SE547UsYLU4O9muepSMptoOG7SMecVXtoaYpSJh78ONWnVjE3nO\n\tdf0kYGgUjarRy9t9XRZxBtAjVjAPcgwl+VuvMD88uLlmmqX6RE4JSn1EvEBN/+4MQ0kW\n\t989UwX7cUMHscThKg+ULdQIQmqTH0RDp4aZtjP2RJwmCxxol+m8qiLu6ZVlUIB+UU16W\n\t4vxCChpn6LQswBA1fbryT0Y8vbngfE3QLj1TyGXYy3sfiXIuHjVLG+PUfrFWoQEgiuVH\n\tcbG0x4qy2ofwW+r1CDQtx8U7Bg0D6DH69FskeD/GmQD+ReGiUvhVNzi7h/S8sGLX+WG4\n\tD2YA==","X-Gm-Message-State":"AOAM533Cq0Z9QiA9V+H5vUla4CjTKhMxPc1L72BlQ2yz2BZ1nbBcUbRI\n\t9iJsWIwxbPii/mD4dxSMgeyiozh3MbPpfaZsA1Hxt6ircceg6tUx","X-Google-Smtp-Source":"ABdhPJyW++7g6tntLCOV4klszhq2OhSTvakU92zG/sRrIaBnnnYJ4tgDMEObniqTeU3OVTJ9kt4dlkFdxo7SizK33ls=","X-Received":"by 2002:a05:6000:1acc:: with SMTP id\n\ti12mr24880146wry.249.1633964726117; \n\tMon, 11 Oct 2021 08:05:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927123327.14554-1-david.plowman@raspberrypi.com>\n\t<20210927123327.14554-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 11 Oct 2021 16:05:13 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20141,"web_url":"https://patchwork.libcamera.org/comment/20141/","msgid":"<8490ceb9-081d-d041-bee9-6c126f814523@xs4all.nl>","date":"2021-10-12T11:43:59","subject":"Re: [libcamera-devel] [PATCH v2 1/3] libcamera: Add ColorSpace class","submitter":{"id":43,"url":"https://patchwork.libcamera.org/api/people/43/","name":"Hans Verkuil","email":"hverkuil-cisco@xs4all.nl"},"content":"Hi Daniel, Laurent,\n\nSome comments:\n\nOn 11/10/2021 16:16, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for the detailed review of all this. I probably won't comment\n> on some of the more \"trivial\" items (I'll just do a v3!) but some of\n> the points do indeed want some more discussion.\n> \n> On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart\n> wrote:\n>>\n>> Hi David,\n>>\n>> (CC'ing Hans, the colour space expert in V4L2)\n>>\n>> Thank you for the patch.\n>>\n>> Hans, if you have a bit of time to look at this, your feedback would be\n>> appreciated.\n>>\n>> On Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:\n>>> This class represents a colour space by defining its YCbCr encoding,\n>>> the transfer (gamma) function is uses, and whether the output is full\n>>\n>> s/is/it/\n>>\n>>> or limited range.\n>>>\n>>> Signed-off-by: David Plowman \n>>> ---\n>>> include/libcamera/color_space.h | 83 +++++++++++++\n>>> include/libcamera/meson.build | 1 +\n>>> src/libcamera/color_space.cpp | 207 ++++++++++++++++++++++++++++++++\n>>> src/libcamera/meson.build | 1 +\n>>> 4 files changed, 292 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..9cd10503\n>>> --- /dev/null\n>>> +++ b/include/libcamera/color_space.h\n>>> @@ -0,0 +1,83 @@\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>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +class ColorSpace\n>>> +{\n>>> +public:\n>>> + enum class Encoding : int {\n\nYou probably want to be explicit and call this YCbCrEncoding, since this only applies to YCbCr\npixel encodings.\n\n>>> + UNDEFINED,\n>>\n>> We typically use CamelCase for enumerators.\n>>\n>>> + RAW,\n>>> + REC601,\n>>> + REC709,\n>>> + REC2020,\n>>> + VIDEO,\n>>> + };\n>>> +\n>>> + enum class TransferFunction : int {\n>>> + UNDEFINED,\n>>> + IDENTITY,\n>>> + SRGB,\n>>> + REC709,\n>>> + };\n>>> +\n>>> + enum class Range : int {\n>>> + UNDEFINED,\n>>> + FULL,\n>>> + LIMITED,\n>>> + };\n>>> +\n>>> + constexpr ColorSpace(Encoding e, TransferFunction t, Range r)\n>>> + : encoding(e), transferFunction(t), range(r)\n>>> + {\n>>> + }\n>>> +\n>>> + constexpr ColorSpace()\n>>> + : ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)\n>>\n>> Line wrap ?\n>>\n>>> + {\n>>> + }\n>>\n>> I'd move the default constructor furst.\n>>\n>>> +\n>>> + static const ColorSpace UNDEFINED;\n>>> + static const ColorSpace RAW;\n>>> + static const ColorSpace JFIF;\n\nJFIF (aka JPEG) is not a colorspace, it is a container. Effectively this is equivalent to sRGB, storing the pixels as YCbCr using Bt.601\nencoding and full range. I would not set it as a separate colorspace.\n\n>>> + static const ColorSpace SMPTE170M;\n>>> + static const ColorSpace REC709;\n>>> + static const ColorSpace REC2020;\n>>> + static const ColorSpace VIDEO;\n>>\n>> Shouldn't these be defined as static constexpr with a value here ?\n>>\n>>> +\n>>> + Encoding encoding;\n>>> + TransferFunction transferFunction;\n>>> + Range range;\n>>> +\n>>> + bool isFullyDefined() const;\n>>> +\n>>> + const std::string toString() const;\n>>> +};\n>>> +\n>>> +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };\n>>> +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };\n>>> +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };\n>>> +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };\n>>> +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };\n>>> +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };\n>>> +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };\n>>\n>> This could then be dropped, or possibly moved (without the initializers)\n>> to color_space.cpp if there's a need to bind a reference or take the\n>> address of these variables.\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 5b25ef84..7a8a04e5 100644\n>>> --- a/include/libcamera/meson.build\n>>> +++ b/include/libcamera/meson.build\n>>> @@ -3,6 +3,7 @@\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..d1ccb4cc\n>>> --- /dev/null\n>>> +++ b/src/libcamera/color_space.cpp\n>>> @@ -0,0 +1,207 @@\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>>\n>> This should go first, to ensure that the header file is self-contained.\n>>\n>>> +\n>>> +/**\n>>> + * \\file color_space.h\n>>> + * \\brief Class and enums to represent colour spaces.\n>>\n>> No period at the end of briefs. Same below.\n>>\n>>> + */\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +/**\n>>> + * \\class ColorSpace\n>>> + * \\brief Class to describe a color space.\n>>> + *\n>>> + * The color space class defines the encodings of the color primaries, the\n>>\n>> s/color space/ColorSpace/\n>>\n>> You use both color and colour. For the class name, ColorSpace seems\n>> right (well, it seems wrong to me, but it seems to be the right choice\n>> :-)). For the text, we'll likely have to go through the documentation\n>> one day to make everything consistent, and will unfortunately likely\n>> have to pick the US spelling. This hasn't been completely decided yet,\n>> so I've fine with either for now, but let's make it consistent within\n>> the file.\n> \n> I tend to go American in the code and then UK in the text, but I can\n> go all-American, it's fine!\n> \n>>\n>>> + * transfer function associated with the color space, and the range (sometimes\n>>> + * also referred to as the quantisation) of the color space.\n>>> + *\n>>> + * Certain combinations of these fields form well-known standard color spaces,\n>>> + * such as \"JFIF\" or \"REC709\", though there is flexibility to leave some or all\n>>> + * of them undefined too.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\enum ColorSpace::Encoding\n>>> + * \\brief The encoding used for the color primaries.\n>>\n>> Can we name this Primaries ? Encoding is strongly tied to the Y'CbCr\n>> encoding from R'G'B' values, I think it would be confusing.\n>>\n>> Now that I've written this, I realize (after reading the rest of the\n>> series) that you're using this as the actual Y'CbCr encoding. The name\n>> is thus fine, but it shouldn't mention colour primaries then, it should\n>> be defined as Y'CbCr encoding. I would also move the definition of the\n>> Encoding enum after TransferFunction, as that's the order in which\n>> they're applied.\n>>\n>> This leads me to the next question: what should we do with the colour\n>> primaries (and white point) ? Those are part of the colour space\n>> definition.\n> \n> Yes, we do need a field for primaries, my bad for not including it. I\n> guess I'd imagined wrapping it up into the encoding as they often go\n> together, but that's not right.\n> \n> Looking at the colour spaces we have in mind, I think we'll need\n> values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or \"SMPTE-C\"\n> for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.\n> \n> I don't think we need to do anything about the white point, that's\n> determined for you once you have your primaries I believe.\n\nThe white point is part of these standards. So Rec.709 defines both the\nprimaries and the white point. It's true for all of these.\n\nBTW, sections 2.15-2.18 are very useful material to read:\n\nhttps://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt.html\n\nThere are four 'parameters' that define how to interpret color values:\n\n- the colorspace, aka chromaticities (RGB primaries + whitepoint)\n- the transfer function\n- the quantization range\n- Y'CbCr encoding (only for Y'CbCr, of course). There is also a HSV encoding\n if the pixels are stored using HSV instead of Y'CbCr, but that's not relevant\n here, I think.\n\nMost colorspace standards define these four 'parameters', but APIs should\nreally treat them as independent parameters.\n\n> \n>>\n>>> + *\n>>> + * \\var ColorSpace::Encoding::UNDEFINED\n>>> + * \\brief The encoding for the colour primaries is not specified.\n>>\n>> I think we need to include more usage information. Can an application\n>> set the encoding to undefined ? What will happen then ? Can a camera\n>> return undefined ?\n\nSince Encoding is what I would call the Y'CbCr encoding, 'UNDEFINED' can\nbe used for RGB, but not for Y'CbCr: there you always have to know the\nencoding used.\n\n>>\n>>> + * \\var ColorSpace::Encoding::RAW\n>>> + * \\brief These are raw colours from the sensor.\n>>\n>> Will this also apply to RGB formats ?\n\nThis makes no sense for Y'CbCr encoding.\n\n>>\n>>> + * \\var ColorSpace::Encoding::REC601\n>>> + * \\brief REC601 colour primaries.\n>>\n>> s/REC601/Rec. 601/\n>>\n>> or possibly \"ITU-R Rec. 601\" or \"ITU-R Rec. BT.601\" if we want to be\n>> precise. Same below.\n>>\n>> Should we also include the encoding formula explicitly, or would that be\n>> too much details ?\n> \n> Maybe some web links instead, to avoid duplication?\n\nYou can refer to the V4L2 colorspace sections.\n\n> \n>>\n>>> + * \\var ColorSpace::Encoding::REC709\n>>> + * \\brief Rec709 colour primaries.\n>>> + * \\var ColorSpace::Encoding::REC2020\n>>> + * \\brief REC2020 colour primaries.\n>>> + * \\var ColorSpace::Encoding::VIDEO\n>>> + * \\brief A place-holder for video streams which will be resolved to one\n>>> + * of REC601, REC709 or REC2020 once the video resolution is known.\n\nDon't do this. If you don't know the Y'CbCr encoding yet, then just pick\nsomething and update it once you do know it.\n\nThe Y'CbCr encoding is in principle independent of the resolution, although for\nsome interfaces (HDMI) it can be tied to the resolution, but it doesn't have to.\n\n>>\n>> We also need more information here. What's your expected use case for\n>> this ? I'm concerned it may seem safe for applications to pick this as a\n>> default, but they won't care much about colour spaces then, and will\n>> likely get things wrong.\n> \n> So the idea was that generateConfiguration would use VIDEO when the\n> stream role is \"video\", so that it could be resolved later once the\n> video resolution is known. That way, application code would almost\n> never have to deal with colour spaces because you'd do\n> \n> configuration = camera->generateConfiguration(roles);\n> /* set up formats and resolutions *./\n> camera->configure(configuration);\n> \n> and for JPEG, mpeg/h26x video it would all work out correctly. The\n> only case you'd have to worry about would be something like mjpeg,\n> where you'd have to set the colour space explicitly to JPEG/JFIF.\n> \n> Note that we would have been able to leave it as UNDEFINED (rather\n> than inventing VIDEO) but for the fact that the configuration\n> generated doesn't record that it was intended for video.\n> \n> Slightly unhappily, we will need to do the same with the (proposed)\n> Primaries field, as that too can change depending on the video\n> resolution. Or perhaps it would be better for a stream configuration\n> to remember its \"role\"? I could go with that.\n\nYou can do something similar to what V4L2 does, add 'DEFAULT' and\ndeduce the corresponding parameter based on the other information.\nSee e.g. V4L2_MAP_COLORSPACE_DEFAULT et al in videodev2.h.\n\n> \n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\enum ColorSpace::TransferFunction\n>>> + * \\brief The transfer function used for this colour space.\n>>> + *\n>>> + * \\var ColorSpace::TransferFunction::UNDEFINED\n>>> + * \\brief The transfer function is not specified.\n>>> + * \\var ColorSpace::TransferFunction::IDENTITY\n>>> + * \\brief This color space uses an identity transfer function.\n>>\n>> Isn't \"linear\" a more common term than \"identity\" for the transfer\n>> function ?\n\nYes. The transfer function is not a matrix, but a non-linear function.\nSo 'identity' is not really a correct term. V4L2 just calls it V4L2_XFER_FUNC_NONE,\ni.e. there is no transfer function.\n\n>>\n>>> + * \\var ColorSpace::TransferFunction::SRGB\n>>> + * \\brief sRGB transfer function.\n>>> + * \\var ColorSpace::TransferFunction::REC709\n>>> + * \\brief Rec709 transfer function.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\enum ColorSpace::Range\n>>> + * \\brief The range (sometimes \"quantisation\") for this color space.\n>>\n>> Technically speaking, the quantisation isn't part of the colour space,\n>> but I fear it would bring lots of complexity and little gain to try and\n>> keep it separate.\n>>\n>>> + *\n>>> + * \\var ColorSpace::Range::UNDEFINED\n>>> + * \\brief The range is not specified.\n\n'UNDEFINED' is meaningless. 'DEFAULT' can make sense (see V4L2_MAP_QUANTIZATION_DEFAULT),\nbut if you don't know the range, then you can't interpret the data.\n\nUsing the wrong quantization range is very visible to the end user, so this must\nbe right.\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.\n>>\n>> Should we define these in more details as well, with the exact range ?\n> \n> Yes, makes sense. Or possibly web links, will have a look!\n\nAgain, refer to the V4L2 spec. Easiest resource for this.\n\n> \n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n>>> + * \\brief Construct a ColorSpace from explicit values\n>>> + * \\param[in] e The encoding for the color primaries\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>>> + * \\fn ColorSpace::ColorSpace()\n>>> + * \\brief Construct a color space with undefined encoding, transfer function\n>>> + * and range\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Return true if all the fields of the color space are defined, otherwise false.\n>>\n>> Line wrap. You also need a \\return, so I'd write \\brief as \"Check if\n>> ...\".\n>>\n>>> + */\n>>> +bool ColorSpace::isFullyDefined() const\n>>> +{\n>>> + return encoding != Encoding::UNDEFINED &&\n>>> + transferFunction != TransferFunction::UNDEFINED &&\n>>> + range != Range::UNDEFINED;\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>>> + static const char *encodings[] = {\n>>> + \"UNDEFINED\",\n>>> + \"RAW\",\n>>> + \"REC601\",\n>>> + \"REC709\",\n>>> + \"REC2020\",\n>>> + };\n>>> + static const char *transferFunctions[] = {\n>>> + \"UNDEFINED\",\n>>> + \"IDENTITY\",\n>>> + \"SRGB\",\n>>> + \"REC709\",\n>>> + };\n>>> + static const char *ranges[] = {\n>>> + \"UNDEFINED\",\n>>> + \"FULL\",\n>>> + \"LIMITED\",\n>>> + };\n>>> +\n>>> + std::stringstream ss;\n>>> + ss << std::string(encodings[static_cast(encoding)]) << \"+\"\n>>\n>> \"+\" seems a bit of a weird separator, I would have expected \"/\". Is\n>> there a specific reason ?\n>>\n>>> + << std::string(transferFunctions[static_cast(transferFunction)]) << \"+\"\n>>> + << std::string(ranges[static_cast(range)]);\n>>> +\n>>> + return ss.str();\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\var ColorSpace::encoding\n>>> + * \\brief The encoding of the color primaries\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::JFIF\n>>> + * \\brief A constant representing the JFIF color space usually used for\n>>> + * encoding JPEG images.\n>>\n>> Should this and the colour spaces below be defined more precisely in the\n>> documentation, or do you think users can just look at the values of the\n>> different members to figure out which is which ?\n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var ColorSpace::SMPTE170M\n>>> + * \\brief A constant representing the SMPTE170M color space (sometimes also\n>>> + * referred to as \"full range BT601\").\n\n'full range BT601' makes no sense whatsoever in this context. This is simply wrong.\nIt *is* sometimes (incorrectly) referred to as BT601. I'd drop this bit completely,\nthough.\n\n>>\n>> Even though quantization is limited range ?\n> \n> I shouldn't refer to SMPTE170M as a kind of \"full range BT601\", even\n> though rather lazily I sometimes think that. BT601 doesn't define\n> primaries, so it's misleading.\n> \n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var ColorSpace::REC709\n>>> + * \\brief A constant representing the REC709 color space.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var ColorSpace::REC2020\n>>> + * \\brief A constant representing the REC2020 color space.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var ColorSpace::VIDEO\n>>> + * \\brief A constant that video streams can use to indicate the \"default\"\n>>> + * color space for a video of this resolution, once that is is known. For\n>>> + * exmample, SD streams would interpret this as SMPTE170M, HD streams as\n>>\n>> s/exmample/example/\n>>\n>>> + * REC709 and ultra HD as REC2020.\n\nUHD can be either REC709 or REC2020. And if it is HDR then UHD would use the SMPTE\n2084 transfer function instead of REC709.\n\nRegards,\n\n\tHans\n\n>>\n>> As mentioned above, this sounds a bit dangerous to me, but maybe I worry\n>> too much. If we want to keep this, I think we need to make the\n>> definition stricter, by documenting the required behaviour (thus\n>> removing \"for example\", and defining SD and HD).\n> \n> Or maybe store the stream role in the configuration...?\n\nI would really drop both UNDEFINED and VIDEO, and instead use a DEFAULT enum.\nThat will make an educated guess.\n\nRegards,\n\n\tHans\n\n> \n> Let's continue the discussion, but in the meantime I'll put together a\n> v3 with most of the other points addressed.\n> \n> Thanks!\n> \n> David\n> \n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Compare color spaces for equality\n>>\n>> *\n>> * Color spaces are considered identical if they have the same\n>> * \\ref transferFunction, \\ref encoding and \\ref range.\n>> *\n>>\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.encoding == rhs.encoding &&\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 bf82d38b..88dfbf24 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>> --\n>> Regards,\n>>\n>> Laurent Pinchart","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 79DB1BDC71\n\tfor ;\n\tTue, 12 Oct 2021 11:50:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E664268F51;\n\tTue, 12 Oct 2021 13:50:15 +0200 (CEST)","from lb1-smtp-cloud8.xs4all.net (lb1-smtp-cloud8.xs4all.net\n\t[IPv6:2001:888:0:108::1b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2D3868F4C\n\tfor ;\n\tTue, 12 Oct 2021 13:44:03 +0200 (CEST)","from cust-b5b5937f ([IPv6:fc0c:c16d:66b8:757f:c639:739b:9d66:799d])\n\tby smtp-cloud8.xs4all.net with ESMTPA\n\tid aGCNmrZqyx7rIaGCRmLsYV; Tue, 12 Oct 2021 13:44:03 +0200"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=xs4all.nl header.i=@xs4all.nl\n\theader.b=\"QiPqYum7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s2;\n\tt=1634039043; bh=tB4h69O7khf9sDCTY14uJUZoLs/g6Di9pomqmIiJNNU=;\n\th=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type:From:\n\tSubject;\n\tb=QiPqYum7vDch7F/x+gE8uZ1Yq6UBQlbWI8Te1itAUJvRMVpQ8fQ3WcPLHuGHDW37Z\n\tJo+26Ff4gHwfPWR8JN0Y1i5S+hAFgxyWF4IsGV9wZoiBqDrlyoiU4jVnonzlViEhMC\n\tZue4clSqaGnLwKboeZ1cexSl/Au4SoYqELLJdZYmB/VkNZstwU5YOznBSwP87o8M0c\n\tQJwVs4hqBaowiCG5o57jzPrpCYVY8DlrsAo2N3iv17pObykimI/DkmqZzD/0t1QD6/\n\ttiNXx7DyiEg9ohdWMoatRoWXKmVOomN9dg+N3uAowVNflv1uLijXKNbkI4mquaLHLW\n\tubFikq19y67gw==","To":"David Plowman ,\n\tLaurent Pinchart ","References":"<20210927123327.14554-1-david.plowman@raspberrypi.com>\n\t<20210927123327.14554-2-david.plowman@raspberrypi.com>\n\t\n\t","From":"Hans Verkuil ","Message-ID":"<8490ceb9-081d-d041-bee9-6c126f814523@xs4all.nl>","Date":"Tue, 12 Oct 2021 13:43:59 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tFirefox/78.0 Thunderbird/78.14.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4xfC6gHUiuATYxU/nxw/kCdTGc14pGhxiXUvBFsGgU4hwel4FYMzTJ9wmf830fNssvCN9cs7OKCXXYtZZR3ljdkF0Hm/6x8srBNAD5J7+TMHf2vONzigEA\n\tTYySbTm9l+UFR2YPBoVSqI9pHrTEk0v9S+7NiKb9QXiUjChzea8h2p8JcxVECcpez2K20R2S8lwSNRsb9wHRZUNe+mP4ba8RYUJqB236NTbfspNflzYdpywL\n\tbhgJCAw6hiaHU0wmT3DvZxtBz9pC8eAtqCkF8cAfRjt1CVEz6kWBb9J2ePk++oqmz6rssgRpkwLTVy2J8w3C7g==","X-Mailman-Approved-At":"Tue, 12 Oct 2021 13:50:14 +0200","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20146,"web_url":"https://patchwork.libcamera.org/comment/20146/","msgid":"","date":"2021-10-12T14:42:59","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 Hans, everyone\n\nThank you for the detailed review, I think that's very helpful.\nObviously I'm working on a v3 at the moment where I can adopt most of\nthese suggestions.\n\nI like the idea of DEFAULT, rather than VIDEO. UNDEFINED is indeed\nundesirable, though I wonder if it lives on purely as a return value\nin case V4L2 changes the colour space to one that we haven't defined.\n(Obviously we could define them all, but I'm not sure about that, and\nof course there might in future one day be things other than V4L2 as\nwell. Opinions always welcome!)\n\nLaurent -\n\nA few little points about what else I'm planning for v3:\n\n* I need to add colour spaces to the subdev format. I think that means\nv4l2ToColorSpace and colorSpaceToV4l2 methods in the base V4L2Device\nclass.\n\n* All pipeline handlers will change any \"DEFAULT\" non-raw streams to\nthe \"best choice\" for video, according to the resolution. Raw streams\nwill be changed to ColorSpace::Raw. (Changing a DEFAULT value will not\ncount as an adjustment.)\n\n* I'll write a CameraConfiguration::updateColorSpaces function to do\nthis, and make all pipeline handlers call it at the start of\nvalidate(). Optionally it will also force non-raw streams to the same\ncolour space.\n\n* The rule will be that the V4L2Device functions will never get passed\nanything \"DEFAULT\", and *absolutely never* anything \"UNDEFINED\". I\nthink both should be regarded as errors. (But UNDEFINED can be\nreturned, as above.) Generally I take the view that colour space\nproblems really need to hit you in the face, so to speak, otherwise\nit's terribly easy to miss them... until after your customer is\nshipping a product :)\n\nDoes that make sense?\n\nThanks everyone!\nDavid\n\nOn Tue, 12 Oct 2021 at 12:44, Hans Verkuil wrote:\n>\n> Hi Daniel, Laurent,\n>\n> Some comments:\n>\n> On 11/10/2021 16:16, David Plowman wrote:\n> > Hi Laurent\n> >\n> > Thanks for the detailed review of all this. I probably won't comment\n> > on some of the more \"trivial\" items (I'll just do a v3!) but some of\n> > the points do indeed want some more discussion.\n> >\n> > On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart\n> > wrote:\n> >>\n> >> Hi David,\n> >>\n> >> (CC'ing Hans, the colour space expert in V4L2)\n> >>\n> >> Thank you for the patch.\n> >>\n> >> Hans, if you have a bit of time to look at this, your feedback would be\n> >> appreciated.\n> >>\n> >> On Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:\n> >>> This class represents a colour space by defining its YCbCr encoding,\n> >>> the transfer (gamma) function is uses, and whether the output is full\n> >>\n> >> s/is/it/\n> >>\n> >>> or limited range.\n> >>>\n> >>> Signed-off-by: David Plowman \n> >>> ---\n> >>> include/libcamera/color_space.h | 83 +++++++++++++\n> >>> include/libcamera/meson.build | 1 +\n> >>> src/libcamera/color_space.cpp | 207 ++++++++++++++++++++++++++++++++\n> >>> src/libcamera/meson.build | 1 +\n> >>> 4 files changed, 292 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..9cd10503\n> >>> --- /dev/null\n> >>> +++ b/include/libcamera/color_space.h\n> >>> @@ -0,0 +1,83 @@\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> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +class ColorSpace\n> >>> +{\n> >>> +public:\n> >>> + enum class Encoding : int {\n>\n> You probably want to be explicit and call this YCbCrEncoding, since this only applies to YCbCr\n> pixel encodings.\n>\n> >>> + UNDEFINED,\n> >>\n> >> We typically use CamelCase for enumerators.\n> >>\n> >>> + RAW,\n> >>> + REC601,\n> >>> + REC709,\n> >>> + REC2020,\n> >>> + VIDEO,\n> >>> + };\n> >>> +\n> >>> + enum class TransferFunction : int {\n> >>> + UNDEFINED,\n> >>> + IDENTITY,\n> >>> + SRGB,\n> >>> + REC709,\n> >>> + };\n> >>> +\n> >>> + enum class Range : int {\n> >>> + UNDEFINED,\n> >>> + FULL,\n> >>> + LIMITED,\n> >>> + };\n> >>> +\n> >>> + constexpr ColorSpace(Encoding e, TransferFunction t, Range r)\n> >>> + : encoding(e), transferFunction(t), range(r)\n> >>> + {\n> >>> + }\n> >>> +\n> >>> + constexpr ColorSpace()\n> >>> + : ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)\n> >>\n> >> Line wrap ?\n> >>\n> >>> + {\n> >>> + }\n> >>\n> >> I'd move the default constructor furst.\n> >>\n> >>> +\n> >>> + static const ColorSpace UNDEFINED;\n> >>> + static const ColorSpace RAW;\n> >>> + static const ColorSpace JFIF;\n>\n> JFIF (aka JPEG) is not a colorspace, it is a container. Effectively this is equivalent to sRGB, storing the pixels as YCbCr using Bt.601\n> encoding and full range. I would not set it as a separate colorspace.\n>\n> >>> + static const ColorSpace SMPTE170M;\n> >>> + static const ColorSpace REC709;\n> >>> + static const ColorSpace REC2020;\n> >>> + static const ColorSpace VIDEO;\n> >>\n> >> Shouldn't these be defined as static constexpr with a value here ?\n> >>\n> >>> +\n> >>> + Encoding encoding;\n> >>> + TransferFunction transferFunction;\n> >>> + Range range;\n> >>> +\n> >>> + bool isFullyDefined() const;\n> >>> +\n> >>> + const std::string toString() const;\n> >>> +};\n> >>> +\n> >>> +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };\n> >>> +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };\n> >>> +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };\n> >>> +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };\n> >>> +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };\n> >>> +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };\n> >>> +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };\n> >>\n> >> This could then be dropped, or possibly moved (without the initializers)\n> >> to color_space.cpp if there's a need to bind a reference or take the\n> >> address of these variables.\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 5b25ef84..7a8a04e5 100644\n> >>> --- a/include/libcamera/meson.build\n> >>> +++ b/include/libcamera/meson.build\n> >>> @@ -3,6 +3,7 @@\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..d1ccb4cc\n> >>> --- /dev/null\n> >>> +++ b/src/libcamera/color_space.cpp\n> >>> @@ -0,0 +1,207 @@\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> >>\n> >> This should go first, to ensure that the header file is self-contained.\n> >>\n> >>> +\n> >>> +/**\n> >>> + * \\file color_space.h\n> >>> + * \\brief Class and enums to represent colour spaces.\n> >>\n> >> No period at the end of briefs. Same below.\n> >>\n> >>> + */\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +/**\n> >>> + * \\class ColorSpace\n> >>> + * \\brief Class to describe a color space.\n> >>> + *\n> >>> + * The color space class defines the encodings of the color primaries, the\n> >>\n> >> s/color space/ColorSpace/\n> >>\n> >> You use both color and colour. For the class name, ColorSpace seems\n> >> right (well, it seems wrong to me, but it seems to be the right choice\n> >> :-)). For the text, we'll likely have to go through the documentation\n> >> one day to make everything consistent, and will unfortunately likely\n> >> have to pick the US spelling. This hasn't been completely decided yet,\n> >> so I've fine with either for now, but let's make it consistent within\n> >> the file.\n> >\n> > I tend to go American in the code and then UK in the text, but I can\n> > go all-American, it's fine!\n> >\n> >>\n> >>> + * transfer function associated with the color space, and the range (sometimes\n> >>> + * also referred to as the quantisation) of the color space.\n> >>> + *\n> >>> + * Certain combinations of these fields form well-known standard color spaces,\n> >>> + * such as \"JFIF\" or \"REC709\", though there is flexibility to leave some or all\n> >>> + * of them undefined too.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\enum ColorSpace::Encoding\n> >>> + * \\brief The encoding used for the color primaries.\n> >>\n> >> Can we name this Primaries ? Encoding is strongly tied to the Y'CbCr\n> >> encoding from R'G'B' values, I think it would be confusing.\n> >>\n> >> Now that I've written this, I realize (after reading the rest of the\n> >> series) that you're using this as the actual Y'CbCr encoding. The name\n> >> is thus fine, but it shouldn't mention colour primaries then, it should\n> >> be defined as Y'CbCr encoding. I would also move the definition of the\n> >> Encoding enum after TransferFunction, as that's the order in which\n> >> they're applied.\n> >>\n> >> This leads me to the next question: what should we do with the colour\n> >> primaries (and white point) ? Those are part of the colour space\n> >> definition.\n> >\n> > Yes, we do need a field for primaries, my bad for not including it. I\n> > guess I'd imagined wrapping it up into the encoding as they often go\n> > together, but that's not right.\n> >\n> > Looking at the colour spaces we have in mind, I think we'll need\n> > values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or \"SMPTE-C\"\n> > for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.\n> >\n> > I don't think we need to do anything about the white point, that's\n> > determined for you once you have your primaries I believe.\n>\n> The white point is part of these standards. So Rec.709 defines both the\n> primaries and the white point. It's true for all of these.\n>\n> BTW, sections 2.15-2.18 are very useful material to read:\n>\n> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt.html\n>\n> There are four 'parameters' that define how to interpret color values:\n>\n> - the colorspace, aka chromaticities (RGB primaries + whitepoint)\n> - the transfer function\n> - the quantization range\n> - Y'CbCr encoding (only for Y'CbCr, of course). There is also a HSV encoding\n> if the pixels are stored using HSV instead of Y'CbCr, but that's not relevant\n> here, I think.\n>\n> Most colorspace standards define these four 'parameters', but APIs should\n> really treat them as independent parameters.\n>\n> >\n> >>\n> >>> + *\n> >>> + * \\var ColorSpace::Encoding::UNDEFINED\n> >>> + * \\brief The encoding for the colour primaries is not specified.\n> >>\n> >> I think we need to include more usage information. Can an application\n> >> set the encoding to undefined ? What will happen then ? Can a camera\n> >> return undefined ?\n>\n> Since Encoding is what I would call the Y'CbCr encoding, 'UNDEFINED' can\n> be used for RGB, but not for Y'CbCr: there you always have to know the\n> encoding used.\n>\n> >>\n> >>> + * \\var ColorSpace::Encoding::RAW\n> >>> + * \\brief These are raw colours from the sensor.\n> >>\n> >> Will this also apply to RGB formats ?\n>\n> This makes no sense for Y'CbCr encoding.\n>\n> >>\n> >>> + * \\var ColorSpace::Encoding::REC601\n> >>> + * \\brief REC601 colour primaries.\n> >>\n> >> s/REC601/Rec. 601/\n> >>\n> >> or possibly \"ITU-R Rec. 601\" or \"ITU-R Rec. BT.601\" if we want to be\n> >> precise. Same below.\n> >>\n> >> Should we also include the encoding formula explicitly, or would that be\n> >> too much details ?\n> >\n> > Maybe some web links instead, to avoid duplication?\n>\n> You can refer to the V4L2 colorspace sections.\n>\n> >\n> >>\n> >>> + * \\var ColorSpace::Encoding::REC709\n> >>> + * \\brief Rec709 colour primaries.\n> >>> + * \\var ColorSpace::Encoding::REC2020\n> >>> + * \\brief REC2020 colour primaries.\n> >>> + * \\var ColorSpace::Encoding::VIDEO\n> >>> + * \\brief A place-holder for video streams which will be resolved to one\n> >>> + * of REC601, REC709 or REC2020 once the video resolution is known.\n>\n> Don't do this. If you don't know the Y'CbCr encoding yet, then just pick\n> something and update it once you do know it.\n>\n> The Y'CbCr encoding is in principle independent of the resolution, although for\n> some interfaces (HDMI) it can be tied to the resolution, but it doesn't have to.\n>\n> >>\n> >> We also need more information here. What's your expected use case for\n> >> this ? I'm concerned it may seem safe for applications to pick this as a\n> >> default, but they won't care much about colour spaces then, and will\n> >> likely get things wrong.\n> >\n> > So the idea was that generateConfiguration would use VIDEO when the\n> > stream role is \"video\", so that it could be resolved later once the\n> > video resolution is known. That way, application code would almost\n> > never have to deal with colour spaces because you'd do\n> >\n> > configuration = camera->generateConfiguration(roles);\n> > /* set up formats and resolutions *./\n> > camera->configure(configuration);\n> >\n> > and for JPEG, mpeg/h26x video it would all work out correctly. The\n> > only case you'd have to worry about would be something like mjpeg,\n> > where you'd have to set the colour space explicitly to JPEG/JFIF.\n> >\n> > Note that we would have been able to leave it as UNDEFINED (rather\n> > than inventing VIDEO) but for the fact that the configuration\n> > generated doesn't record that it was intended for video.\n> >\n> > Slightly unhappily, we will need to do the same with the (proposed)\n> > Primaries field, as that too can change depending on the video\n> > resolution. Or perhaps it would be better for a stream configuration\n> > to remember its \"role\"? I could go with that.\n>\n> You can do something similar to what V4L2 does, add 'DEFAULT' and\n> deduce the corresponding parameter based on the other information.\n> See e.g. V4L2_MAP_COLORSPACE_DEFAULT et al in videodev2.h.\n>\n> >\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\enum ColorSpace::TransferFunction\n> >>> + * \\brief The transfer function used for this colour space.\n> >>> + *\n> >>> + * \\var ColorSpace::TransferFunction::UNDEFINED\n> >>> + * \\brief The transfer function is not specified.\n> >>> + * \\var ColorSpace::TransferFunction::IDENTITY\n> >>> + * \\brief This color space uses an identity transfer function.\n> >>\n> >> Isn't \"linear\" a more common term than \"identity\" for the transfer\n> >> function ?\n>\n> Yes. The transfer function is not a matrix, but a non-linear function.\n> So 'identity' is not really a correct term. V4L2 just calls it V4L2_XFER_FUNC_NONE,\n> i.e. there is no transfer function.\n>\n> >>\n> >>> + * \\var ColorSpace::TransferFunction::SRGB\n> >>> + * \\brief sRGB transfer function.\n> >>> + * \\var ColorSpace::TransferFunction::REC709\n> >>> + * \\brief Rec709 transfer function.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\enum ColorSpace::Range\n> >>> + * \\brief The range (sometimes \"quantisation\") for this color space.\n> >>\n> >> Technically speaking, the quantisation isn't part of the colour space,\n> >> but I fear it would bring lots of complexity and little gain to try and\n> >> keep it separate.\n> >>\n> >>> + *\n> >>> + * \\var ColorSpace::Range::UNDEFINED\n> >>> + * \\brief The range is not specified.\n>\n> 'UNDEFINED' is meaningless. 'DEFAULT' can make sense (see V4L2_MAP_QUANTIZATION_DEFAULT),\n> but if you don't know the range, then you can't interpret the data.\n>\n> Using the wrong quantization range is very visible to the end user, so this must\n> be right.\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.\n> >>\n> >> Should we define these in more details as well, with the exact range ?\n> >\n> > Yes, makes sense. Or possibly web links, will have a look!\n>\n> Again, refer to the V4L2 spec. Easiest resource for this.\n>\n> >\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n> >>> + * \\brief Construct a ColorSpace from explicit values\n> >>> + * \\param[in] e The encoding for the color primaries\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> >>> + * \\fn ColorSpace::ColorSpace()\n> >>> + * \\brief Construct a color space with undefined encoding, transfer function\n> >>> + * and range\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Return true if all the fields of the color space are defined, otherwise false.\n> >>\n> >> Line wrap. You also need a \\return, so I'd write \\brief as \"Check if\n> >> ...\".\n> >>\n> >>> + */\n> >>> +bool ColorSpace::isFullyDefined() const\n> >>> +{\n> >>> + return encoding != Encoding::UNDEFINED &&\n> >>> + transferFunction != TransferFunction::UNDEFINED &&\n> >>> + range != Range::UNDEFINED;\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> >>> + static const char *encodings[] = {\n> >>> + \"UNDEFINED\",\n> >>> + \"RAW\",\n> >>> + \"REC601\",\n> >>> + \"REC709\",\n> >>> + \"REC2020\",\n> >>> + };\n> >>> + static const char *transferFunctions[] = {\n> >>> + \"UNDEFINED\",\n> >>> + \"IDENTITY\",\n> >>> + \"SRGB\",\n> >>> + \"REC709\",\n> >>> + };\n> >>> + static const char *ranges[] = {\n> >>> + \"UNDEFINED\",\n> >>> + \"FULL\",\n> >>> + \"LIMITED\",\n> >>> + };\n> >>> +\n> >>> + std::stringstream ss;\n> >>> + ss << std::string(encodings[static_cast(encoding)]) << \"+\"\n> >>\n> >> \"+\" seems a bit of a weird separator, I would have expected \"/\". Is\n> >> there a specific reason ?\n> >>\n> >>> + << std::string(transferFunctions[static_cast(transferFunction)]) << \"+\"\n> >>> + << std::string(ranges[static_cast(range)]);\n> >>> +\n> >>> + return ss.str();\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\var ColorSpace::encoding\n> >>> + * \\brief The encoding of the color primaries\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::JFIF\n> >>> + * \\brief A constant representing the JFIF color space usually used for\n> >>> + * encoding JPEG images.\n> >>\n> >> Should this and the colour spaces below be defined more precisely in the\n> >> documentation, or do you think users can just look at the values of the\n> >> different members to figure out which is which ?\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var ColorSpace::SMPTE170M\n> >>> + * \\brief A constant representing the SMPTE170M color space (sometimes also\n> >>> + * referred to as \"full range BT601\").\n>\n> 'full range BT601' makes no sense whatsoever in this context. This is simply wrong.\n> It *is* sometimes (incorrectly) referred to as BT601. I'd drop this bit completely,\n> though.\n>\n> >>\n> >> Even though quantization is limited range ?\n> >\n> > I shouldn't refer to SMPTE170M as a kind of \"full range BT601\", even\n> > though rather lazily I sometimes think that. BT601 doesn't define\n> > primaries, so it's misleading.\n> >\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var ColorSpace::REC709\n> >>> + * \\brief A constant representing the REC709 color space.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var ColorSpace::REC2020\n> >>> + * \\brief A constant representing the REC2020 color space.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var ColorSpace::VIDEO\n> >>> + * \\brief A constant that video streams can use to indicate the \"default\"\n> >>> + * color space for a video of this resolution, once that is is known. For\n> >>> + * exmample, SD streams would interpret this as SMPTE170M, HD streams as\n> >>\n> >> s/exmample/example/\n> >>\n> >>> + * REC709 and ultra HD as REC2020.\n>\n> UHD can be either REC709 or REC2020. And if it is HDR then UHD would use the SMPTE\n> 2084 transfer function instead of REC709.\n>\n> Regards,\n>\n> Hans\n>\n> >>\n> >> As mentioned above, this sounds a bit dangerous to me, but maybe I worry\n> >> too much. If we want to keep this, I think we need to make the\n> >> definition stricter, by documenting the required behaviour (thus\n> >> removing \"for example\", and defining SD and HD).\n> >\n> > Or maybe store the stream role in the configuration...?\n>\n> I would really drop both UNDEFINED and VIDEO, and instead use a DEFAULT enum.\n> That will make an educated guess.\n>\n> Regards,\n>\n> Hans\n>\n> >\n> > Let's continue the discussion, but in the meantime I'll put together a\n> > v3 with most of the other points addressed.\n> >\n> > Thanks!\n> >\n> > David\n> >\n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Compare color spaces for equality\n> >>\n> >> *\n> >> * Color spaces are considered identical if they have the same\n> >> * \\ref transferFunction, \\ref encoding and \\ref range.\n> >> *\n> >>\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.encoding == rhs.encoding &&\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 bf82d38b..88dfbf24 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> >> --\n> >> Regards,\n> >>\n> >> Laurent Pinchart\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 8ACC6C323E\n\tfor ;\n\tTue, 12 Oct 2021 14:43:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CEF5668F4F;\n\tTue, 12 Oct 2021 16:43:11 +0200 (CEST)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECA6468F4C\n\tfor ;\n\tTue, 12 Oct 2021 16:43:10 +0200 (CEST)","by mail-wr1-x42f.google.com with SMTP id i12so54976907wrb.7\n\tfor ;\n\tTue, 12 Oct 2021 07:43:10 -0700 (PDT)"],"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=\"UUdYeSBE\"; 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=wtTdTmiAxad4t/CIbFM0sLqWmvxvF1qIBdcQyFQajqQ=;\n\tb=UUdYeSBEUElNd/xG9j6/pW7e1Q+dz4RkBWX5hb8WwQImoBgHYjvCiCYY3XZKQp+bjt\n\t01t8CTW+6YGJqE6IpKTe4HkF2stI8d4Vd+K6uLwM/GR3ypJpKd1V2IbDZCTYCBO6Sl3h\n\tu2W+aSBBXsKqQOBNmQQcVZU1x7hv0I4Q5ZR4+dVip7v+znPRK9gsKb6I/dTr99J8c9sF\n\tv558y2H2Ji1imOFQTJzWaEy9r8TFJLiuz4s8Q+uIib1i1GqFJYvBA4mW6YjiVUf2PtIE\n\tCcTpcIm6FlsjMWQC7GI6HFO2z00/fsdmZskYpgNOTGJ2/EsOIREP5SFr85t4jcKJUL7M\n\tbJ8w==","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=wtTdTmiAxad4t/CIbFM0sLqWmvxvF1qIBdcQyFQajqQ=;\n\tb=Jx5R4PA4rpj9bMpLqjsLGoqdVy+eLSm4cxqqn6v5vAn8pRNo7Wj+GqB0Lf3x/BIB8o\n\tOzXvc6KIW1UQQPc+j54F/PjxlgDxhW4Y84jYPXYUr1nC9UFlhFFk+UWdI/hEpBYjmo9p\n\tjU+yvW0RFrhQcqKYDfSR07Ur7QzFuzBN5kJL/CyF2AbszyxAKwbPnltyKn5eirSCmnpv\n\tDP6ZoU89p9Q1U0bA959zvJHxOikHAZtQ118Ms/9rOJ7EfsdodSka+fTmV/gBweTXOL69\n\tBeFYSvC0gNy/C/WZUV5Bw82ZG7KWDixIgDp07uTmSMJkN/IHw1gX2G6D8hUqryggYBII\n\tCHew==","X-Gm-Message-State":"AOAM531SEoZPWK1AOnp4mgm2eEZUxMiG5g+qtDNT++GArT76r0/s8dKr\n\tKNeKfmTdj51LWXM5Ky/nlt7YyJ9vvwyGCB/7SnVLaA==","X-Google-Smtp-Source":"ABdhPJxB3B4B6vCGyQ45Ub5fKhtUvhJBjXBqTN10foXFGxCttJaCT/sE9YbSB9M7pIMxQCkzxCpBTkpKj+vs0e1qRaY=","X-Received":"by 2002:a05:6000:1acc:: with SMTP id\n\ti12mr31958788wry.249.1634049790191; \n\tTue, 12 Oct 2021 07:43:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927123327.14554-1-david.plowman@raspberrypi.com>\n\t<20210927123327.14554-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t<8490ceb9-081d-d041-bee9-6c126f814523@xs4all.nl>","In-Reply-To":"<8490ceb9-081d-d041-bee9-6c126f814523@xs4all.nl>","From":"David Plowman ","Date":"Tue, 12 Oct 2021 15:42:59 +0100","Message-ID":"","To":"Hans Verkuil ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]