[{"id":20415,"web_url":"https://patchwork.libcamera.org/comment/20415/","msgid":"","date":"2021-10-25T07:36:11","subject":"Re: [libcamera-devel] [PATCH v3 1/7] libcamera: Add ColorSpace class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your work.\n\nOn Wed, 20 Oct 2021 at 12:08, David Plowman \nwrote:\n\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> ---\n> include/libcamera/color_space.h | 88 +++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/color_space.cpp | 256 ++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 346 insertions(+)\n> create mode 100644 include/libcamera/color_space.h\n> create mode 100644 src/libcamera/color_space.cpp\n>\n> diff --git a/include/libcamera/color_space.h\n> b/include/libcamera/color_space.h\n> new file mode 100644\n> index 00000000..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,\n> YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined)\n> + {\n> + }\n> +\n> + constexpr ColorSpace(Primaries p, YcbcrEncoding e,\n> TransferFunction t, Range r)\n> + : primaries(p), ycbcrEncoding(e), transferFunction(t),\n> 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\nOther classes seem to use an isValid() member function. Should we do the\nsame here?\nGranted, this is not exactly the same, a false return value from\nisFullyDefined() might be\nallowed for a valid colourspace?\n\n\n\n> +\n> + const std::string toString() const;\n> +};\n> +\n> +constexpr ColorSpace ColorSpace::Undefined = { Primaries::Undefined,\n> YcbcrEncoding::Undefined, TransferFunction::Undefined, Range::Undefined };\n> +constexpr ColorSpace ColorSpace::Raw = { Primaries::Raw,\n> YcbcrEncoding::Rec601, TransferFunction::Linear, Range::Full };\n> +constexpr ColorSpace ColorSpace::Jpeg = { Primaries::Rec709,\n> YcbcrEncoding::Rec601, TransferFunction::Srgb, Range::Full };\n> +constexpr ColorSpace ColorSpace::Smpte170m = { Primaries::Smpte170m,\n> YcbcrEncoding::Rec601, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec709 = { Primaries::Rec709,\n> YcbcrEncoding::Rec709, TransferFunction::Rec709, Range::Limited };\n> +constexpr ColorSpace ColorSpace::Rec2020 = { Primaries::Rec2020,\n> YcbcrEncoding::Rec2020, TransferFunction::Rec709, Range::Limited };\n> +\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);\n> +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace\n> &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..aec4107f\n> --- /dev/null\n> +++ b/src/libcamera/color_space.cpp\n> @@ -0,0 +1,256 @@\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,\n> please\n> + * see:\n> + *\n> + * https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-srgb\">sRGB\n> and https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-jpeg\n> \">JPEG\n> +>* https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-smpte-170m\">SMPTE\n> 170M\n> + * https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-rec709\n> \">Rec.709\n> + * https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/pixfmt-007.html#col-bt2020\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> + * \\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\n>\n\nPerhaps add the range for 10-bit as well?\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 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\n> 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> +\n> + static const std::vector>\n> 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(),\n> colorSpaceNames.end(),\n> + [this](const auto &item) {\n> + return *this == item.first;\n> + });\n>\n\nPerhaps this might be better done with a std::map or even embedding the\nstring\ninto a member variable? Not too fussed either way.\n\nReviewed-by: Naushir Patuck \n\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> \"/\"\n> + << std::string(encodingNames[static_cast(ycbcrEncoding)])\n> << \"/\"\n> + <<\n> std::string(transferFunctionNames[static_cast(transferFunction)]) <<\n> \"/\"\n> + << std::string(rangeNames[static_cast(range)]);\n> +\n> + return ss.str();\n> +}\n> +\n> +/**\n> + * \\var ColorSpace::primaries\n> + * \\brief The color primaries\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::ycbcrEncoding\n> + * \\brief The Y'CbCr encoding\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::transferFunction\n> + * \\brief The transfer function for this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::range\n> + * \\brief The pixel range used by this color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Undefined\n> + * \\brief A constant representing a fully undefined color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Raw\n> + * \\brief A constant representing a raw color space (from a sensor)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Jpeg\n> + * \\brief A constant representing the JPEG color space used for\n> + * encoding JPEG images (and regarded as being the same as the sRGB\n> + * color space)\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Smpte170m\n> + * \\brief A constant representing the SMPTE170M color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec709\n> + * \\brief A constant representing the Rec.709 color space\n> + */\n> +\n> +/**\n> + * \\var ColorSpace::Rec2020\n> + * \\brief A constant representing the Rec.2020 color space\n> + */\n> +\n> +/**\n> + * \\brief Compare color spaces for equality\n> + * \\return True if the two color spaces are identical, false otherwise\n> + */\n> +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)\n> +{\n> + return lhs.primaries == rhs.primaries &&\n> + lhs.ycbcrEncoding == rhs.ycbcrEncoding &&\n> + lhs.transferFunction == rhs.transferFunction &&\n> + lhs.range == rhs.range;\n> +}\n> +\n> +/**\n> + * \\fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)\n> + * \\brief Compare color spaces for inequality\n> + * \\return True if the two color spaces are not identical, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 243dd3c1..8dc5d39d 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>\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 AC5B5BF415\n\tfor ;\n\tMon, 25 Oct 2021 07:36:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01D5A64872;\n\tMon, 25 Oct 2021 09:36:29 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E015860124\n\tfor ;\n\tMon, 25 Oct 2021 09:36:27 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id u5so10406179ljo.8\n\tfor ;\n\tMon, 25 Oct 2021 00:36:27 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"InxUkxDD\"; 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=Gz+wGYDJ2dwpBrQDwhj1HSfSs7BcTanTZ5C5VPlJN8M=;\n\tb=InxUkxDDe/nWB9w/0pfjWZj91If16WHeiKfMKhOll7i+jZNjD1fftRc34wcParPft8\n\tSbkfyZkjTrqiXK+O7/U71KHVkWOhpFj9Esg/WzMNsrFyaCSYVQrPygwCGdA7MtwRVx9h\n\t1Fscy4eo/qiT9irBFyps2e+1ilp18actSTT5IIvEru6g6OfCKscwznpN5z+LWtpSLVTQ\n\tstLz8Adq/uRBW1FnfbBtCfl7R+iEX5mJ8eh+mtlAD2jSX4GApJ6UsFdXd+sweaipEmFX\n\tg6s8oKKEKyXR+hCYNcEuK2/PV0SP+U/QDxLeLQ+T+Okp/l79RGthV6TZxcE0LyzcwGat\n\tLJUA==","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=Gz+wGYDJ2dwpBrQDwhj1HSfSs7BcTanTZ5C5VPlJN8M=;\n\tb=U9+tXrl2TWNJaE+J+sBYnfg6Uz+NOWZNbj1W8goTIziGnbFX7Ap9/OZSlMZVTdV/I1\n\taEZ3S8sHyHPb25l9OibA/Uj8caFX+HkyipxgACtoC+h++Oy//aBBqDokZh9RmpKUEUe9\n\tXOM00zDjgi/YH79wBS847TRM7/3XrKu+8w0Zw/AGpZ3XefhalEb1KPLNq/czrylUXOMW\n\tAK1oRPuuJrLP8Z3WyC2LwUfNikGNRubDBST0sLOUYcmuVBeksdthJ4HUWFOF2O+GObJt\n\tBHUeKbExD78mvM+Er8Qavru2aT7ZUm+D0WTtwcYnaEwvfz6WkOlfz0NPN9ux4CcoHJ77\n\tQhcw==","X-Gm-Message-State":"AOAM531KtA6mLJohWU7EBVN5XcijW3wlYPEPIliFNkMq6XecD5zXrQEe\n\th0Mn/9LihQw1nk9vY5AuZaTU8agQFzxhAjl1MU5hYQ==","X-Google-Smtp-Source":"ABdhPJwbFFqVCG/A+HJFVRGkwSLAtV1hEutf4vvV8mn9ojgZAhonQJx40cDhEZUsubcPvSVBU/gS88jtfYesIreDVaU=","X-Received":"by 2002:a2e:800c:: with SMTP id\n\tj12mr17468371ljg.280.1635147386975; \n\tMon, 25 Oct 2021 00:36:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20211020110825.12902-1-david.plowman@raspberrypi.com>\n\t<20211020110825.12902-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20211020110825.12902-2-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Mon, 25 Oct 2021 08:36:11 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000002806df05cf286b9f\"","Subject":"Re: [libcamera-devel] [PATCH v3 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20423,"web_url":"https://patchwork.libcamera.org/comment/20423/","msgid":"","date":"2021-10-25T09:27:47","subject":"Re: [libcamera-devel] [PATCH v3 1/7] libcamera: Add ColorSpace class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for all the reviews, obviously I'll make up a v4 shortly!\n\nOn Mon, 25 Oct 2021 at 08:36, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for your work.\n>\n> On Wed, 20 Oct 2021 at 12:08, David Plowman wrote:\n>>\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>> ---\n>> include/libcamera/color_space.h | 88 +++++++++++\n>> include/libcamera/meson.build | 1 +\n>> src/libcamera/color_space.cpp | 256 ++++++++++++++++++++++++++++++++\n>> src/libcamera/meson.build | 1 +\n>> 4 files changed, 346 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>\n> Other classes seem to use an isValid() member function. Should we do the same here?\n> Granted, this is not exactly the same, a false return value from isFullyDefined() might be\n> allowed for a valid colourspace?\n\nI actually prefer isFullyDefined (\"are any fields undefined or not?\")\nbecause \"valid\" seems like a slightly slippery concept here. You could\nhave a driver choose a valid colour space but which the class doesn't\nrecognise. Is that \"valid\"? I'm not sure...\n\n>\n>\n>>\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>> +\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..aec4107f\n>> --- /dev/null\n>> +++ b/src/libcamera/color_space.cpp\n>> @@ -0,0 +1,256 @@\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>> + * \\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\n>\n>\n> Perhaps add the range for 10-bit as well?\n\nGood idea.\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 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>> +\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>\n>\n> Perhaps this might be better done with a std::map or even embedding the string\n> into a member variable? Not too fussed either way.\n\nI've not not noticed other classes do that for debug strings, but It's\nnot a bad idea. What do other folks think?\n\nI actually ran some little experiments with vectors vs. maps, just to\nget some hints as to when it's worth the trouble of defining an\nordering (which you need for a map), or a hash (which you need for an\nunordered_map). Running on a Pi I found:\n\n* A std::map becomes faster than searching a vector of pairs after\nabout 20 or 30 entries.\n* A std::unordered_map becomes faster than searching a vector at just\nover 150 entries.\n* A std::unordered_map requires \"a few thousand\" entries to become\nfaster than a std::map.\n\nThanks!\n\nDavid\n\n>\n> Reviewed-by: Naushir Patuck \n>\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>> + */\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 243dd3c1..8dc5d39d 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 9CE26BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 09:28:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3EA464870;\n\tMon, 25 Oct 2021 11:27:59 +0200 (CEST)","from mail-wm1-x333.google.com (mail-wm1-x333.google.com\n\t[IPv6:2a00:1450:4864:20::333])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 438D160124\n\tfor ;\n\tMon, 25 Oct 2021 11:27:58 +0200 (CEST)","by mail-wm1-x333.google.com with SMTP id\n\tb133-20020a1c808b000000b0032ca4d18aebso8613043wmd.2\n\tfor ;\n\tMon, 25 Oct 2021 02:27:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"IzYlRJ0o\"; 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=GyjPRk5908p1gvNBdK6f+vc/gXdvklViULEtcxHaeNQ=;\n\tb=IzYlRJ0oovdr1kAhKEm8WkceAtms3mEuNGSpFksPn9/+P8NWPe7/VT0cLncPGMJQmN\n\tI7C6jIFGDwOS29ogYFb0HQ82to1T2ZdowZQjtIjrOdTjW2/FvCdRgKK7SKcP6g7FbMEf\n\tzCAlwJfLOr4trsI5OUFge20UVpNAGuGp0hABA48hkcR6bi1rbIiwnSneD4WncomEkNIO\n\tadgzP48f8obwdaoR1rj8UsQ0wX3GYc3mZy3P9HX8qhAYIQ6bj12D6+bgXDvIbhmJkwgR\n\tX5HOg7uoAzV+BqEbXMw9NwwdWNfVaRYEfaJuB8rLft3Hp/IDwq04l1bz0GeHYQsSBKnM\n\tLK3w==","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=GyjPRk5908p1gvNBdK6f+vc/gXdvklViULEtcxHaeNQ=;\n\tb=1ISmJXKxPVT7/iEB16RO/o1eajzzRhnzLkoILUSwW3SPFhw+Tu1JnrRPCTXXbrSQ2a\n\tGnLsMG4nT+xsFoXu5AomcsgAqbi7Hyc4GiPzdUo/p3bDElIe/EUzEAv6XkOQ+CSYWDF0\n\t2f14ZDcYmyM+5oHBcZiMyLVUW9m5hv45I0JaCiGk2vmQqfWgaemw+ejrFLtosnZP/WxB\n\tmVmBs0B4bzPVn8mS8fALK1rl2iVA5vWsnFDK+3ilgi5bLTv8nbsD9v7c8PdaGdX8S98H\n\tVel7CQhvWBsoXvpoTkQ7uVEYeVnEooQIfpitfx8QaoqybgaOVLsCDiDEwjkj6fZW0Om8\n\tPaCg==","X-Gm-Message-State":"AOAM533zIh+3VelyC2WaUIZbcgorH956C7e6xCInFIpnviMlIJOFMi8l\n\ta67C+vVh48EYlFwqqAnMR31akXSR+W7Oi0gz1eNzulIW2xo=","X-Google-Smtp-Source":"ABdhPJxu0kCRh7d1210RNx++hJQefk2/Ng0WrFJSr8Ojdb3oDqHoH06O6GcQuMURypWm+ovv5ejhOmv6tdsCh1MaNc4=","X-Received":"by 2002:a05:600c:1c21:: with SMTP id\n\tj33mr47899038wms.163.1635154077737; \n\tMon, 25 Oct 2021 02:27:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20211020110825.12902-1-david.plowman@raspberrypi.com>\n\t<20211020110825.12902-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 25 Oct 2021 10:27:47 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]