[{"id":20688,"web_url":"https://patchwork.libcamera.org/comment/20688/","msgid":"<163611602916.275423.16979175813536684049@Monstersaurus>","date":"2021-11-05T12:40:29","subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: Add ColorSpace class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-04 13:57:59)\n> This class represents a color space by defining its color primaries,\n> YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> output is full or limited range.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> include/libcamera/color_space.h | 88 +++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/color_space.cpp | 257 ++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 347 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..2af9da31\n> --- /dev/null\n> +++ b/include/libcamera/color_space.h\n> @@ -0,0 +1,88 @@\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 Primaries : int {\n> + Undefined,\n> + Raw,\n> + Smpte170m,\n> + Rec709,\n> + Rec2020,\n> + };\n> +\n> + enum class YcbcrEncoding : int {\n> + Undefined,\n> + Rec601,\n> + Rec709,\n> + Rec2020,\n> + };\n> +\n> + enum class TransferFunction : int {\n> + Undefined,\n> + Linear,\n> + Srgb,\n> + Rec709,\n> + };\n> +\n> + enum class Range : int {\n> + Undefined,\n> + Full,\n> + Limited,\n> + };\n> +\n> + constexpr ColorSpace()\n> + : ColorSpace(Primaries::Undefined, YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined)\n> + {\n> + }\n> +\n> + constexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> + : primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> + {\n> + }\n> +\n> + static const ColorSpace Undefined;\n> + static const ColorSpace Raw;\n> + static const ColorSpace Jpeg;\n> + static const ColorSpace Smpte170m;\n> + static const ColorSpace Rec709;\n> + static const ColorSpace Rec2020;\n> +\n> + Primaries primaries;\n> + YcbcrEncoding ycbcrEncoding;\n> + TransferFunction transferFunction;\n> + Range range;\n> +\n> + bool isFullyDefined() const;\n> +\n> + const std::string toString() const;\n> +};\n> +\n> +constexpr ColorSpace ColorSpace::Undefined = { Primaries::Undefined, YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined };\n> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n\nIs the mix between static const and constexpr correct in those? (I guess\nit compiles so it's fine?)\n\nShould the definitions be in the .cpp file ? I guess they're 'simple'\ndata storage but I think the usual aim is to reduce header sizes for\nparsing.\n\nThough this is the public API... perhaps theres a distinct reason for\nopenly declaring the colorspace definition in the header so that it is\nclear to the users. So I think that makes it worthy of staying here too.\n\n> +\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> + return !(lhs == rhs);\n> +}\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 7155ff20..131e1740 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> libcamera_public_headers = files([\n> 'camera.h',\n> 'camera_manager.h',\n> + 'color_space.h',\n> 'compiler.h',\n> 'controls.h',\n> 'file_descriptor.h',\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> new file mode 100644\n> index 00000000..dfd9fa1d\n> --- /dev/null\n> +++ b/src/libcamera/color_space.cpp\n> @@ -0,0 +1,257 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.cpp - color spaces.\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +/**\n> + * \\file color_space.h\n> + * \\brief Class and enums to represent color spaces\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class ColorSpace\n> + * \\brief Class to describe a color space\n> + *\n> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> + * the transfer function associated with the color space, and the range\n> + * (sometimes also referred to as the quantisation) of the color space.\n> + *\n> + * Certain combinations of these fields form well-known standard color\n> + * spaces such as \"JPEG\" or \"REC709\". Applications must not request color\n> + * spaces with undefined fields, but the \"Undefined\" value may be\n> + * returned if the camera drivers decide to use a color space that is\n> + * not recognised by the ColorSpace class.\n> + *\n> + * For more information on the specific color spaces described here, please\n> + * see:\n> + *\n> + * sRGB and JPEG\n\nI think that can be wrapped to\n\nsRGB\nand JPEG\n\nat least.\n\n> +>* SMPTE 170M\n> + * Rec.709\n> + * Rec.2020\n\nCan we do better than v4.8? That was EOL January 2017...\n\nI think the documentation all got shuffled around so changing /v4.8/ to\n/latest/ doesn't get the right thing anymore.\n\nPerhaps:\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-defs.html\n\nand\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-srgb\n\n\nOr use v5.15 in place of latest to lock in to more permanant version\nlinks?\n\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Primaries\n> + * \\brief The color primaries for this color space\n> + *\n> + * \\var ColorSpace::Primaries::Undefined\n> + * \\brief The color primaries are undefined\n> + * \\var ColorSpace::Primaries::Raw\n> + * \\brief These are raw colors directly from a sensor\n> + * \\var ColorSpace::Primaries::Smpte170m\n> + * \\brief SMPTE 170M color primaries\n> + * \\var ColorSpace::Primaries::Rec709\n> + * \\brief Rec.709 color primaries\n> + * \\var ColorSpace::Primaries::Rec2020\n> + * \\brief Rec.2020 color primaries\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::YcbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + *\n> + * \\var ColorSpace::YcbcrEncoding::Undefined\n> + * \\brief The Y'CbCr encoding is undefined\n> + * \\var ColorSpace::YcbcrEncoding::Rec601\n> + * \\brief Rec.601 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec709\n> + * \\brief Rec.709 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> + * \\brief Rec.2020 Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::TransferFunction\n> + * \\brief The transfer function used for this color space\n> + *\n> + * \\var ColorSpace::TransferFunction::Undefined\n> + * \\brief The transfer function is not specified\n> + * \\var ColorSpace::TransferFunction::Linear\n> + * \\brief This color space uses a linear (identity) transfer function\n> + * \\var ColorSpace::TransferFunction::Srgb\n> + * \\brief sRGB transfer function\n> + * \\var ColorSpace::TransferFunction::Rec709\n> + * \\brief Rec709 transfer function\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Range\n> + * \\brief The range (sometimes \"quantisation\") for this color space\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, being\n> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n> + * \\brief Construct a ColorSpace from explicit values\n> + * \\param[in] e The Y'CbCr encoding\n> + * \\param[in] t The transfer function for the color space\n> + * \\param[in] r The range of the pixel values in this color space\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace()\n> + * \\brief Construct a color space with undefined encoding, transfer function\n> + * and range\n> + */\n> +\n> +/**\n> + * \\brief Check if all the fields of the color space are defined\n> + * \\return Return true if all the fields of the color space are defined,\n> + * otherwise false\n> + */\n> +bool ColorSpace::isFullyDefined() const\n> +{\n> + return primaries != Primaries::Undefined &&\n> + ycbcrEncoding != YcbcrEncoding::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> + /* Print out a brief name only for standard color sapces. */\n\ns/sapces/spaces/\n\n> +\n> + static const std::vector> colorSpaceNames = {\n> + { ColorSpace::Undefined, \"Undefined\" },\n> + { ColorSpace::Raw, \"Raw\" },\n> + { ColorSpace::Jpeg, \"Jpeg\" },\n> + { ColorSpace::Smpte170m, \"Smpte170m\" },\n> + { ColorSpace::Rec709, \"Rec709\" },\n> + { ColorSpace::Rec2020, \"Rec2020\" },\n> + };\n> + auto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> + [this](const auto &item) {\n> + return *this == item.first;\n> + });\n> + if (it != colorSpaceNames.end())\n> + return std::string(it->second);\n> +\n> + static const char *primariesNames[] = {\n> + \"Undefined\",\n> + \"Raw\",\n> + \"Smpte170m\",\n> + \"Rec709\",\n> + \"Rec2020\",\n> + };\n> + static const char *encodingNames[] = {\n> + \"Undefined\",\n> + \"Rec601\",\n> + \"Rec709\",\n> + \"Rec2020\",\n> + };\n> + static const char *transferFunctionNames[] = {\n> + \"Undefined\",\n> + \"Linear\",\n> + \"Srgb\",\n> + \"Rec709\",\n> + };\n> + static const char *rangeNames[] = {\n> + \"Undefined\",\n> + \"Full\",\n> + \"Limited\",\n> + };\n> +\n> + std::stringstream ss;\n> + ss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> + << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> + << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> + << std::string(rangeNames[static_cast(range)]);\n> +\n> + return ss.str();\n> +}\n> +\n> +/**\n> + * \\var ColorSpace::primaries\n> + * \\brief The color primaries\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::ycbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::transferFunction\n> + * \\brief The transfer function for this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::range\n> + * \\brief The pixel range used by this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Undefined\n> + * \\brief A constant representing a fully undefined color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Raw\n> + * \\brief A constant representing a raw color space (from a sensor)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Jpeg\n> + * \\brief A constant representing the JPEG color space used for\n> + * encoding JPEG images (and regarded as being the same as the sRGB\n> + * color space)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Smpte170m\n> + * \\brief A constant representing the SMPTE170M color space\n\nI wonder if these are a good opportunity to link to the specific\ndocumentation from V4L2...\n\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-smpte-170m\n\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec709\n> + * \\brief A constant representing the Rec.709 color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec2020\n> + * \\brief A constant representing the Rec.2020 color space\n> + */\n> +\n> +/**\n> + * \\brief Compare color spaces for equality\n> + * \\return True if the two color spaces are identical, false otherwise\n> + */\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> + return lhs.primaries == rhs.primaries &&\n> + lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> + lhs.transferFunction == rhs.transferFunction &&\n> + lhs.range == rhs.range;\n> +}\n> +\n> +/**\n> + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> + * \\brief Compare color spaces for inequality\n> + * \\return True if the two color spaces are not identical, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777..e7371d20 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -8,6 +8,7 @@ libcamera_sources = files([\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> 'camera_sensor_properties.cpp',\n> + 'color_space.cpp',\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> -- \n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C15ACBDB1C\n\tfor ;\n\tFri, 5 Nov 2021 12:40:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 29BED6034F;\n\tFri, 5 Nov 2021 13:40:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1575C600B8\n\tfor ;\n\tFri, 5 Nov 2021 13:40:32 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A1B1D1908;\n\tFri, 5 Nov 2021 13:40:31 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Z2teKC8a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636116031;\n\tbh=58do0cmzh5SHMzN682AvH2MyrtoIYs5ZZKBBL9jN6lQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Z2teKC8aF4UqYcSEVcsZjfnbNLfLKb+hGD7T99BDyDnYdZslYKySSW4Aj6rcUSJd1\n\tVDikpDvaH2+14KUea3Rtf+t3sL40zHl/UTFy4SfIKqvkfLjuJDWveLDs5Fgp3lgW8t\n\tE5+eOoW4R3gGc/MSRtGPvmyHOQuqA0qeu1h+d8sg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211104135805.5269-2-david.plowman@raspberrypi.com>","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-2-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 05 Nov 2021 12:40:29 +0000","Message-ID":"<163611602916.275423.16979175813536684049@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20689,"web_url":"https://patchwork.libcamera.org/comment/20689/","msgid":"<163611624228.275423.5418289618541115107@Monstersaurus>","date":"2021-11-05T12:44:02","subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: Add ColorSpace class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-04 13:57:59)\n> This class represents a color space by defining its color primaries,\n> YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> output is full or limited range.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n\nAhh, I'm afraid also get some doxygen warnings that I expect come from\nthis patch:\n\n[95/105] Generating doxygen with a custom command\n/home/kbingham/iob/libcamera/libcamera/src/libcamera/color_space.cpp:103: warning: no matching class member found for\n libcamera::ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\nPossible candidates:\n 'constexpr libcamera::ColorSpace::ColorSpace()'\n 'constexpr libcamera::ColorSpace::ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)'\n\n/home/kbingham/iob/libcamera/libcamera/include/libcamera/color_space.h:51: warning: Member ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r) (function) of class libcamera::ColorSpace is not documented.\n\n--\nKieran","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 D575BBF415\n\tfor ;\n\tFri, 5 Nov 2021 12:44:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CB086034A;\n\tFri, 5 Nov 2021 13:44:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 208E3600B8\n\tfor ;\n\tFri, 5 Nov 2021 13:44:05 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B35C3192B;\n\tFri, 5 Nov 2021 13:44:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fRX/PY5E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636116244;\n\tbh=C5WMx4VgHDXaY4lsSGrQGQvznE3/RZvV7U68JJuKc+o=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=fRX/PY5E+CUUJk1c8/8skpH93e/z7CVe9Ydkd8BdsEVNIJTrgUMX++9TTokK0SOCy\n\t0Yp81KIDS4HwC2qnzm40xNWdx4Q8kke/LotgD27dvXb6PIsV8gERgBBA4+7V6fE2+T\n\tZVFMyh6qnG8XC4Oz5VG0WeuDD0xh7uYQSMxDTUb4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211104135805.5269-2-david.plowman@raspberrypi.com>","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-2-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 05 Nov 2021 12:44:02 +0000","Message-ID":"<163611624228.275423.5418289618541115107@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20699,"web_url":"https://patchwork.libcamera.org/comment/20699/","msgid":"<20211106161119.qqzt5jpmlkxxixie@uno.localdomain>","date":"2021-11-06T16:11:19","subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: Add ColorSpace class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n I'm sorry for getting here late and I praise your persistency in\n pushing this forward.\n\nI read all the previous version and I realized how ignorant was I\nabout color spaces and seeing that you had gone through them with both\nLaurent and Hans I will limit my comments to the implementation\nseeing that the underlying concepts have been discussed in detail.\n\n\nOn Thu, Nov 04, 2021 at 01:57:59PM +0000, David Plowman wrote:\n> This class represents a color space by defining its color primaries,\n> YCbCr encoding, the transfer (gamma) function it uses, and whether the\n> output is full or limited range.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> include/libcamera/color_space.h | 88 +++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/color_space.cpp | 257 ++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 347 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..2af9da31\n> --- /dev/null\n> +++ b/include/libcamera/color_space.h\n> @@ -0,0 +1,88 @@\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\nNit: \"Color space\" :)\n\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 Primaries : int {\n> +\t\tUndefined,\n> +\t\tRaw,\n> +\t\tSmpte170m,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class YcbcrEncoding : int {\n> +\t\tUndefined,\n> +\t\tRec601,\n> +\t\tRec709,\n> +\t\tRec2020,\n> +\t};\n> +\n> +\tenum class TransferFunction : int {\n> +\t\tUndefined,\n> +\t\tLinear,\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()\n> +\t\t: ColorSpace(Primaries::Undefined, YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined)\n> +\t{\n> +\t}\n> +\n> +\tconstexpr ColorSpace(Primaries p, YcbcrEncoding e, TransferFunction t, Range r)\n> +\t\t: primaries(p), ycbcrEncoding(e), transferFunction(t), range(r)\n> +\t{\n> +\t}\n> +\n> +\tstatic const ColorSpace Undefined;\n> +\tstatic const ColorSpace Raw;\n> +\tstatic const ColorSpace Jpeg;\n> +\tstatic const ColorSpace Smpte170m;\n> +\tstatic const ColorSpace Rec709;\n> +\tstatic const ColorSpace Rec2020;\n> +\n> +\tPrimaries primaries;\n> +\tYcbcrEncoding ycbcrEncoding;\n> +\tTransferFunction transferFunction;\n> +\tRange range;\n> +\n> +\tbool isFullyDefined() const;\n> +\n> +\tconst std::string toString() const;\n> +};\n\nContradicting myself after just a few lines, I have a question on the\n\"concepts\" :)\n\nDo you expect applications to define their own ColorSpace by\nassembling together the different components that define a color space\n(Primaries, YcbcrEncoding, TransferFunction and Range) or do you\nexpect them to use the below defined ones only ?\n\n> +\n> +constexpr ColorSpace ColorSpace::Undefined = { Primaries::Undefined, YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined };\n> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw, YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709, YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m, YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709, YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020, YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n\nReading this\nhttps://isocpp.org/blog/2018/05/quick-q-use-of-constexpr-in-header-file\n\nmy understanding is that each translation unit including color_space.h\nwill have its own copy of these instances.\n\nShouldn't you declare them as 'extern const' and define them in the\n.cpp file as we do for controls, or since we're now with C++17 use the\n'inline' keyword that allows you to define them in the header with\nautomatic external linkage ?\nhttps://en.cppreference.com/w/cpp/language/inline\n\n> +\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn !(lhs == rhs);\n> +}\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 7155ff20..131e1740 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_include_dir = 'libcamera' / 'libcamera'\n> libcamera_public_headers = files([\n> 'camera.h',\n> 'camera_manager.h',\n> + 'color_space.h',\n> 'compiler.h',\n> 'controls.h',\n> 'file_descriptor.h',\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> new file mode 100644\n> index 00000000..dfd9fa1d\n> --- /dev/null\n> +++ b/src/libcamera/color_space.cpp\n> @@ -0,0 +1,257 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * color_space.cpp - color spaces.\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +/**\n> + * \\file color_space.h\n> + * \\brief Class and enums to represent color spaces\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class ColorSpace\n> + * \\brief Class to describe a color space\n> + *\n> + * The ColorSpace class defines the color primaries, the Y'CbCr encoding,\n> + * the transfer function associated with the color space, and the range\n> + * (sometimes also referred to as the quantisation) of the color space.\n> + *\n> + * Certain combinations of these fields form well-known standard color\n> + * spaces such as \"JPEG\" or \"REC709\". Applications must not request color\n> + * spaces with undefined fields, but the \"Undefined\" value may be\n> + * returned if the camera drivers decide to use a color space that is\n> + * not recognised by the ColorSpace class.\n> + *\n> + * For more information on the specific color spaces described here, please\n> + * see:\n> + *\n> + * sRGB and JPEG\n> +>* SMPTE 170M\n> + * Rec.709\n> + * Rec.2020\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Primaries\n> + * \\brief The color primaries for this color space\n> + *\n> + * \\var ColorSpace::Primaries::Undefined\n> + * \\brief The color primaries are undefined\n\nSo \"Undefined\" is meant not to be used by application but only\nreturned in case libcamera is not capable of interpreting what's\nreported by the driver. I'm tempted to say this should be repeated in\neach xx::xx::Undefined definition, but I might just be paranoid.\n\nThinking out loud:\nDepending on the expectations of how application should use ColorSpace\ninstances (create their own ones or just use the 'well known'\ncombinations) I wonder if we shouldn't restrict the visibility of\n\"Undefined\" (by removing it :)\nAs you're using un-typed enums (plain enums) we could\njust define the \"ColorSpace::Undefined\" instance with all members\ninitialized to -1 and in case one of the 4 components (primaries,\nencoding, range and quantisation) is not recognized by libcamera\nset StreamConfiguration.colorSpace = ColorSpace::Undefined.\n\n> + * \\var ColorSpace::Primaries::Raw\n> + * \\brief These are raw colors directly from a sensor\n\nIt surprises me to see this here, as all other standards define how\nprimaries are obtained from a standard illuminant, but this seems to\nsuggest \"whatever the sensor produces\". I assume this has been\ndiscussed already, so I might just be missing something obvious.\n\n> + * \\var ColorSpace::Primaries::Smpte170m\n> + * \\brief SMPTE 170M color primaries\n> + * \\var ColorSpace::Primaries::Rec709\n> + * \\brief Rec.709 color primaries\n> + * \\var ColorSpace::Primaries::Rec2020\n> + * \\brief Rec.2020 color primaries\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::YcbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + *\n> + * \\var ColorSpace::YcbcrEncoding::Undefined\n> + * \\brief The Y'CbCr encoding is undefined\n> + * \\var ColorSpace::YcbcrEncoding::Rec601\n> + * \\brief Rec.601 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec709\n> + * \\brief Rec.709 Y'CbCr encoding\n> + * \\var ColorSpace::YcbcrEncoding::Rec2020\n> + * \\brief Rec.2020 Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::TransferFunction\n> + * \\brief The transfer function used for this color space\n> + *\n> + * \\var ColorSpace::TransferFunction::Undefined\n> + * \\brief The transfer function is not specified\n> + * \\var ColorSpace::TransferFunction::Linear\n> + * \\brief This color space uses a linear (identity) transfer function\n> + * \\var ColorSpace::TransferFunction::Srgb\n> + * \\brief sRGB transfer function\n> + * \\var ColorSpace::TransferFunction::Rec709\n> + * \\brief Rec709 transfer function\n> + */\n> +\n> +/**\n> + * \\enum ColorSpace::Range\n> + * \\brief The range (sometimes \"quantisation\") for this color space\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, being\n> + * 16 to 235 for Y' and 16 to 240 for Cb and Cr (8 bits per sample)\n> + * or 64 to 940 for Y' and 16 to 960 for Cb and Cr (10 bits)\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)\n> + * \\brief Construct a ColorSpace from explicit values\n> + * \\param[in] e The Y'CbCr encoding\n> + * \\param[in] t The transfer function for the color space\n> + * \\param[in] r The range of the pixel values in this color space\n> + */\n> +\n> +/**\n> + * \\fn ColorSpace::ColorSpace()\n> + * \\brief Construct a color space with undefined encoding, transfer function\n> + * and range\n> + */\n> +\n> +/**\n> + * \\brief Check if all the fields of the color space are defined\n> + * \\return Return true if all the fields of the color space are defined,\n> + * otherwise false\n> + */\n> +bool ColorSpace::isFullyDefined() const\n> +{\n> +\treturn primaries != Primaries::Undefined &&\n> +\t ycbcrEncoding != YcbcrEncoding::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> +\t/* Print out a brief name only for standard color sapces. */\n\nspaces\n\nBut I think the comment can be dropped..\n\nThanks\n j\n\n> +\n> +\tstatic const std::vector> colorSpaceNames = {\n> +\t\t{ ColorSpace::Undefined, \"Undefined\" },\n> +\t\t{ ColorSpace::Raw, \"Raw\" },\n> +\t\t{ ColorSpace::Jpeg, \"Jpeg\" },\n> +\t\t{ ColorSpace::Smpte170m, \"Smpte170m\" },\n> +\t\t{ ColorSpace::Rec709, \"Rec709\" },\n> +\t\t{ ColorSpace::Rec2020, \"Rec2020\" },\n> +\t};\n> +\tauto it = std::find_if(colorSpaceNames.begin(), colorSpaceNames.end(),\n> +\t\t\t [this](const auto &item) {\n> +\t\t\t\t return *this == item.first;\n> +\t\t\t });\n> +\tif (it != colorSpaceNames.end())\n> +\t\treturn std::string(it->second);\n> +\n> +\tstatic const char *primariesNames[] = {\n> +\t\t\"Undefined\",\n> +\t\t\"Raw\",\n> +\t\t\"Smpte170m\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n> +\tstatic const char *encodingNames[] = {\n> +\t\t\"Undefined\",\n> +\t\t\"Rec601\",\n> +\t\t\"Rec709\",\n> +\t\t\"Rec2020\",\n> +\t};\n> +\tstatic const char *transferFunctionNames[] = {\n> +\t\t\"Undefined\",\n> +\t\t\"Linear\",\n> +\t\t\"Srgb\",\n> +\t\t\"Rec709\",\n> +\t};\n> +\tstatic const char *rangeNames[] = {\n> +\t\t\"Undefined\",\n> +\t\t\"Full\",\n> +\t\t\"Limited\",\n> +\t};\n> +\n> +\tstd::stringstream ss;\n> +\tss << std::string(primariesNames[static_cast(primaries)]) << \"/\"\n> +\t << std::string(encodingNames[static_cast(ycbcrEncoding)]) << \"/\"\n> +\t << std::string(transferFunctionNames[static_cast(transferFunction)]) << \"/\"\n> +\t << std::string(rangeNames[static_cast(range)]);\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +/**\n> + * \\var ColorSpace::primaries\n> + * \\brief The color primaries\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::ycbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::transferFunction\n> + * \\brief The transfer function for this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::range\n> + * \\brief The pixel range used by this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Undefined\n> + * \\brief A constant representing a fully undefined color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Raw\n> + * \\brief A constant representing a raw color space (from a sensor)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Jpeg\n> + * \\brief A constant representing the JPEG color space used for\n> + * encoding JPEG images (and regarded as being the same as the sRGB\n> + * color space)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Smpte170m\n> + * \\brief A constant representing the SMPTE170M color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec709\n> + * \\brief A constant representing the Rec.709 color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec2020\n> + * \\brief A constant representing the Rec.2020 color space\n> + */\n> +\n> +/**\n> + * \\brief Compare color spaces for equality\n> + * \\return True if the two color spaces are identical, false otherwise\n> + */\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> +\treturn lhs.primaries == rhs.primaries &&\n> +\t lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> +\t lhs.transferFunction == rhs.transferFunction &&\n> +\t lhs.range == rhs.range;\n> +}\n> +\n> +/**\n> + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> + * \\brief Compare color spaces for inequality\n> + * \\return True if the two color spaces are not identical, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777..e7371d20 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -8,6 +8,7 @@ libcamera_sources = files([\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> 'camera_sensor_properties.cpp',\n> + 'color_space.cpp',\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> --\n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A6FEBDB1C\n\tfor ;\n\tSat, 6 Nov 2021 16:10:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB39260345;\n\tSat, 6 Nov 2021 17:10:29 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77F706033A\n\tfor ;\n\tSat, 6 Nov 2021 17:10:28 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 9E8ABC000C;\n\tSat, 6 Nov 2021 16:10:27 +0000 (UTC)"],"Date":"Sat, 6 Nov 2021 17:11:19 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211106161119.qqzt5jpmlkxxixie@uno.localdomain>","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211104135805.5269-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: Add ColorSpace class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]