[{"id":24094,"web_url":"https://patchwork.libcamera.org/comment/24094/","msgid":"","date":"2022-07-25T10:46:46","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Umang\n\nThanks for this patch. I was wondering if I could ask for a little\nmore information on how it would be used.\n\nIn the original libcamera ColorSpace implementation I was very keen to\navoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\nthe view that applications really should say what ColorSpace they\nwant, but at the same time I knew there will always be (bad)\napplications that don't think about colour spaces and so I let them\nuse std::optional to allow the ColorSpace to be completely\nunspecified.\n\nIn V4L2, my understanding is that \"_DEFAULT\" normally means that you\ninfer the value from the overall V4L2_COLORSPACE (someone please\ncorrect me if that's wrong!). In libcamera we don't have an \"overall\ncolour space\" in this way, so what would it then mean? For example, is\n\"ColorSpace::Range::Default\" ever the same as\n\"ColorSpace::Range::Full\", or is it always different? How do we (can\nwe even) document what it means?\n\nI'd be interested to hear what other folks think. Possibly I'm being a\nbit paranoid, but then I have been bitten by \"wrong colour space\"\nproblems often enough in the past (usually just before a product\nships!).\n\nThanks!\nDavid\n\nOn Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel\n wrote:\n>\n> Add a Colorspace::Default which corresponds to default V4L2\n> colorspace identifiers.\n>\n> \\todo Add defaults to python colorspace bindings\n>\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/color_space.h | 5 +++++\n> src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-\n> src/libcamera/v4l2_device.cpp | 5 +++++\n> 3 files changed, 38 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> index 086c56c1..0ad8da17 100644\n> --- a/include/libcamera/color_space.h\n> +++ b/include/libcamera/color_space.h\n> @@ -16,6 +16,7 @@ class ColorSpace\n> {\n> public:\n> enum class Primaries {\n> + Default,\n> Raw,\n> Smpte170m,\n> Rec709,\n> @@ -23,12 +24,14 @@ public:\n> };\n>\n> enum class TransferFunction {\n> + Default,\n> Linear,\n> Srgb,\n> Rec709,\n> };\n>\n> enum class YcbcrEncoding {\n> + Default,\n> None,\n> Rec601,\n> Rec709,\n> @@ -36,6 +39,7 @@ public:\n> };\n>\n> enum class Range {\n> + Default,\n> Full,\n> Limited,\n> };\n> @@ -45,6 +49,7 @@ public:\n> {\n> }\n>\n> + static const ColorSpace Default;\n> static const ColorSpace Raw;\n> static const ColorSpace Jpeg;\n> static const ColorSpace Srgb;\n> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> index 895e5c8e..d52f58cf 100644\n> --- a/src/libcamera/color_space.cpp\n> +++ b/src/libcamera/color_space.cpp\n> @@ -50,6 +50,9 @@ namespace libcamera {\n> * \\enum ColorSpace::Primaries\n> * \\brief The color primaries for this color space\n> *\n> + * \\var ColorSpace::Primaries::Default\n> + * \\brief Use the default primaries as defined by the driver\n> + *\n> * \\var ColorSpace::Primaries::Raw\n> * \\brief These are raw colors directly from a sensor, the primaries are\n> * unspecified\n> @@ -68,6 +71,9 @@ namespace libcamera {\n> * \\enum ColorSpace::TransferFunction\n> * \\brief The transfer function used for this color space\n> *\n> + * \\var ColorSpace::TransferFunction::Default\n> + * \\brief Use the default transfer function as defined by the driver\n> + *\n> * \\var ColorSpace::TransferFunction::Linear\n> * \\brief This color space uses a linear (identity) transfer function\n> *\n> @@ -82,6 +88,9 @@ namespace libcamera {\n> * \\enum ColorSpace::YcbcrEncoding\n> * \\brief The Y'CbCr encoding\n> *\n> + * \\var ColorSpace::YcbcrEncoding::Default\n> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n> + *\n> * \\var ColorSpace::YcbcrEncoding::None\n> * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> *\n> @@ -99,6 +108,9 @@ namespace libcamera {\n> * \\enum ColorSpace::Range\n> * \\brief The range (sometimes \"quantisation\") for this color space\n> *\n> + * \\var ColorSpace::Range::Default\n> + * Use the default range as defined by the driver\n> + *\n> * \\var ColorSpace::Range::Full\n> * \\brief This color space uses full range pixel values\n> *\n> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n> {\n> /* Print out a brief name only for standard color spaces. */\n>\n> - static const std::array, 6> colorSpaceNames = { {\n> + static const std::array, 7> colorSpaceNames = { {\n> + { ColorSpace::Default, \"Default\" },\n> { ColorSpace::Raw, \"RAW\" },\n> { ColorSpace::Jpeg, \"JPEG\" },\n> { ColorSpace::Srgb, \"sRGB\" },\n> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n> /* Assemble a name made of the constituent fields. */\n>\n> static const std::map primariesNames = {\n> + { Primaries::Default, \"Default\" },\n> { Primaries::Raw, \"RAW\" },\n> { Primaries::Smpte170m, \"SMPTE170M\" },\n> { Primaries::Rec709, \"Rec709\" },\n> { Primaries::Rec2020, \"Rec2020\" },\n> };\n> static const std::map transferNames = {\n> + { TransferFunction::Default, \"Default\" },\n> { TransferFunction::Linear, \"Linear\" },\n> { TransferFunction::Srgb, \"sRGB\" },\n> { TransferFunction::Rec709, \"Rec709\" },\n> };\n> static const std::map encodingNames = {\n> + { YcbcrEncoding::Default, \"Default\" },\n> { YcbcrEncoding::None, \"None\" },\n> { YcbcrEncoding::Rec601, \"Rec601\" },\n> { YcbcrEncoding::Rec709, \"Rec709\" },\n> { YcbcrEncoding::Rec2020, \"Rec2020\" },\n> };\n> static const std::map rangeNames = {\n> + { Range::Default, \"Default\" },\n> { Range::Full, \"Full\" },\n> { Range::Limited, \"Limited\" },\n> };\n> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n> * \\brief The pixel range used with by color space\n> */\n>\n> +/**\n> + * \\brief A constant representing a default color space\n> + */\n> +const ColorSpace ColorSpace::Default = {\n> + Primaries::Default,\n> + TransferFunction::Default,\n> + YcbcrEncoding::Default,\n> + Range::Default\n> +};\n> +\n> /**\n> * \\brief A constant representing a raw color space (from a sensor)\n> */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 3fc8438f..ecfcf337 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n> };\n>\n> static const std::vector> colorSpaceToV4l2 = {\n> + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n> { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n> };\n>\n> static const std::map primariesToV4l2 = {\n> + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n> { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n> };\n>\n> static const std::map transferFunctionToV4l2 = {\n> + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n> { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> };\n>\n> static const std::map ycbcrEncodingToV4l2 = {\n> + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n> { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> };\n>\n> static const std::map rangeToV4l2 = {\n> + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n> { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> };\n> --\n> 2.31.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 AF55FBE173\n\tfor ;\n\tMon, 25 Jul 2022 10:46:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0ADF963312;\n\tMon, 25 Jul 2022 12:46:59 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC95C6330A\n\tfor ;\n\tMon, 25 Jul 2022 12:46:57 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id e15so13382332edj.2\n\tfor ;\n\tMon, 25 Jul 2022 03:46:57 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658746019;\n\tbh=0edp5FLdOiRM2ezgr3Ltn6dV5U3/X6tIktCCcyv1lOA=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mQNpYATXM2vt5aJpsjhBdubK8g9/8QzC+43eb+zSbE85STSqWTyj38zen39XZNgRI\n\tE+eZIeQsun47kbHpyiw+gAmi9ZASFFOphZNgwuYVqNXZ2AerXWSA4BY6mqhm2efFoa\n\tu5ldaX6JKBOjCgY6jnuzHPaMg/1h8oP7guRCjC9z0lelBv/Np0X9AuLcBHL101Rhaz\n\thC8Vyn7xYuuxr77NQ/jjENoFFYzUPlyd1z488oLBEk5XBJ8oo/vcJV87J553NwjYp/\n\tIsuCRWUUTby8rB1BAJchTGK1NThxgHdB0nhuWxJQrz8rOo7frbrdtakO8miq4muXr7\n\ttGxyc4nWn9EoA==","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=nNWp2Qpgn1OoGf8XfCck5sSe19Hg3kTMgPBIy+Jw6vw=;\n\tb=Ty4wSmgBc+fzaDTG0cjK2nBvVkNeHUjedfX8q6A5N/7ZsrRXEd1fJdjqezNN9ZDoIr\n\tJSalHrSJuouANxaxzUpCjfCIFP5CWm8Q5F5oiOpcAyJw2w62JGeHYVJ0XJ45QD4d1BvP\n\t49w6DyeY6KrIjHgUOw75Xh+zPCj2flZqz5E0LpIxM9lu/FS3U2+Ihm4JYKRPyZnrAq8L\n\t0HEvGYYrk4dx1qW3nRQp/uSrXtLuusIm6DTu0q/ESLwQqKWz+1dr27rE+tPoWb5yv/DG\n\tIdCN17owizG5oYvzxTYW5MdICpF9ahleDBkXXubsg+k2BCjBpDKU4eEp05ERtDP6k+FO\n\tEYjA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Ty4wSmgB\"; dkim-atps=neutral","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=nNWp2Qpgn1OoGf8XfCck5sSe19Hg3kTMgPBIy+Jw6vw=;\n\tb=G/uKrf/Pe1esN1s8OPVmGPEfmaldXxqcIZiQZX5UpIlALuRptIUvjHH+QbKfoFzS7J\n\tizD3T0UgAAP766uu3xOUzmSYEhxf5sP2l9oVV97Tt0P2h0Z1J4ZItfiEdOjcOByKRZdQ\n\t+gLhnbhUVXA1DZ1AnkzatKvUZIrW4pl1dutSlV2t9gQKnkh4e3DrLaGZFSjznmEhiooz\n\teyWwHrMjAwVIl6W3E0V/YsPCOwuATiJ53Y7CswIX5p6FMnuUEum9xaSYr/MXjaknb8yi\n\t8zfy3AjuzuJSUec2k/Y9WqkTrXCEknFFBQX+LQ3xlGU5p6o1fj84H89TnNq5CSDHp5D8\n\t9Dzg==","X-Gm-Message-State":"AJIora/mDM0jj6OBaBmmvmmik4sF6uqKkPVmOh/y8gLS102F6mVvqBn8\n\tGGY4LWAPVgYau5DEw5kUlB9QK5LDaG81ZDx54cHqew==","X-Google-Smtp-Source":"AGRyM1tzXjRiB18Nsj/M0/QO7aJQwpDrwW+TjsxKQZucrzuGr9Q96t1KlKUoRWIAiES/xNHOLx29OokpFDwRg6+681E=","X-Received":"by 2002:a05:6402:35d4:b0:43a:df89:94e2 with SMTP id\n\tz20-20020a05640235d400b0043adf8994e2mr12515655edc.149.1658746017366;\n\tMon, 25 Jul 2022 03:46:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>","In-Reply-To":"<20220724144355.104978-3-umang.jain@ideasonboard.com>","Date":"Mon, 25 Jul 2022 11:46:46 +0100","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24096,"web_url":"https://patchwork.libcamera.org/comment/24096/","msgid":"<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>","date":"2022-07-25T11:15:04","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nOn 7/25/22 16:16, David Plowman wrote:\n> Hi Umang\n>\n> Thanks for this patch. I was wondering if I could ask for a little\n> more information on how it would be used.\n\n\nYes, so the idea here is to provide a colorspace where one(or more) \nidentifiers are specified by the user whereas others can be left upto \nthe driver to fill in.\n\nI wasn't aware of this use case until very recently. In gstreamer, it \nseems possible that the user wants to use(enforce?) a particular \ntransfer function whereas other identifier are left upto the driver to fill.\n\nI am not very sure how would this be helpful (since colorspace \nidentifiers depend on each other I think), and mis-matches between them \nmight not make any sense at all.\n\nBut such a use case as has been identified it seems, atleast in the \ngstreamer case - so to satisfy that, I went ahead with this approach. I \ncouldn't get a better solution where the user specifies a set of \nidentifiers to use for colorspace, at the same time depending on the \ndriver to fill unknown or default values for remaining identifier(s).\n\n>\n> In the original libcamera ColorSpace implementation I was very keen to\n> avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n> the view that applications really should say what ColorSpace they\n> want, but at the same time I knew there will always be (bad)\n> applications that don't think about colour spaces and so I let them\n> use std::optional to allow the ColorSpace to be completely\n> unspecified.\n\n\nRight, so when colorspace is completely unspecified, all values are \ndefault-ed (V4L2Device::fromColorSpace()) in the v4l2_format.\n\nIf I am asked about what's the goal of this patch, I would say that it \nenables the use case where a sub-set of colorspace identifiers are \nknown, while others need to use defaults. Currently there is no default \nstd::optional hence I added one, to facilitate \nthe application layer. The application should ideally copy the \nlibcamera::ColorSpace::Default as base, swap the identifiers it wants to \nuse in the copy, and send the custom colorspace for validation.\n\nAlso note, the mapping is only one way (from libcamera to V4L2) \nintentionally. It shouldn't be the case where driver itself returns \n\"::Default\" back to user.\n\n>\n> In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n> infer the value from the overall V4L2_COLORSPACE (someone please\n> correct me if that's wrong!). In libcamera we don't have an \"overall\n\n\nIf we could infer all the identifier values from \nV4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:\n\nV4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT, \nV4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.\n\n> colour space\" in this way, so what would it then mean? For example, is\n> \"ColorSpace::Range::Default\" ever the same as\n> \"ColorSpace::Range::Full\", or is it always different? How do we (can\n> we even) document what it means?\n\n\nMaybe we should look at V4L2 level first, before libcamera for e.g.\n\nWould V4L2_QUANTIZATION_DEFAULT be ever same as \nV4L2_QUANTIZATION_FULL_RANGE ?\n\n>\n> I'd be interested to hear what other folks think. Possibly I'm being a\n\n\nI am all ears here as well, so let' see\n\n> bit paranoid, but then I have been bitten by \"wrong colour space\"\n> problems often enough in the past (usually just before a product\n> ships!).\n\nouch :-S\n\n>\n> Thanks!\n> David\n>\n> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel\n> wrote:\n>> Add a Colorspace::Default which corresponds to default V4L2\n>> colorspace identifiers.\n>>\n>> \\todo Add defaults to python colorspace bindings\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/color_space.h | 5 +++++\n>> src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-\n>> src/libcamera/v4l2_device.cpp | 5 +++++\n>> 3 files changed, 38 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n>> index 086c56c1..0ad8da17 100644\n>> --- a/include/libcamera/color_space.h\n>> +++ b/include/libcamera/color_space.h\n>> @@ -16,6 +16,7 @@ class ColorSpace\n>> {\n>> public:\n>> enum class Primaries {\n>> + Default,\n>> Raw,\n>> Smpte170m,\n>> Rec709,\n>> @@ -23,12 +24,14 @@ public:\n>> };\n>>\n>> enum class TransferFunction {\n>> + Default,\n>> Linear,\n>> Srgb,\n>> Rec709,\n>> };\n>>\n>> enum class YcbcrEncoding {\n>> + Default,\n>> None,\n>> Rec601,\n>> Rec709,\n>> @@ -36,6 +39,7 @@ public:\n>> };\n>>\n>> enum class Range {\n>> + Default,\n>> Full,\n>> Limited,\n>> };\n>> @@ -45,6 +49,7 @@ public:\n>> {\n>> }\n>>\n>> + static const ColorSpace Default;\n>> static const ColorSpace Raw;\n>> static const ColorSpace Jpeg;\n>> static const ColorSpace Srgb;\n>> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n>> index 895e5c8e..d52f58cf 100644\n>> --- a/src/libcamera/color_space.cpp\n>> +++ b/src/libcamera/color_space.cpp\n>> @@ -50,6 +50,9 @@ namespace libcamera {\n>> * \\enum ColorSpace::Primaries\n>> * \\brief The color primaries for this color space\n>> *\n>> + * \\var ColorSpace::Primaries::Default\n>> + * \\brief Use the default primaries as defined by the driver\n>> + *\n>> * \\var ColorSpace::Primaries::Raw\n>> * \\brief These are raw colors directly from a sensor, the primaries are\n>> * unspecified\n>> @@ -68,6 +71,9 @@ namespace libcamera {\n>> * \\enum ColorSpace::TransferFunction\n>> * \\brief The transfer function used for this color space\n>> *\n>> + * \\var ColorSpace::TransferFunction::Default\n>> + * \\brief Use the default transfer function as defined by the driver\n>> + *\n>> * \\var ColorSpace::TransferFunction::Linear\n>> * \\brief This color space uses a linear (identity) transfer function\n>> *\n>> @@ -82,6 +88,9 @@ namespace libcamera {\n>> * \\enum ColorSpace::YcbcrEncoding\n>> * \\brief The Y'CbCr encoding\n>> *\n>> + * \\var ColorSpace::YcbcrEncoding::Default\n>> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n>> + *\n>> * \\var ColorSpace::YcbcrEncoding::None\n>> * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n>> *\n>> @@ -99,6 +108,9 @@ namespace libcamera {\n>> * \\enum ColorSpace::Range\n>> * \\brief The range (sometimes \"quantisation\") for this color space\n>> *\n>> + * \\var ColorSpace::Range::Default\n>> + * Use the default range as defined by the driver\n>> + *\n>> * \\var ColorSpace::Range::Full\n>> * \\brief This color space uses full range pixel values\n>> *\n>> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n>> {\n>> /* Print out a brief name only for standard color spaces. */\n>>\n>> - static const std::array, 6> colorSpaceNames = { {\n>> + static const std::array, 7> colorSpaceNames = { {\n>> + { ColorSpace::Default, \"Default\" },\n>> { ColorSpace::Raw, \"RAW\" },\n>> { ColorSpace::Jpeg, \"JPEG\" },\n>> { ColorSpace::Srgb, \"sRGB\" },\n>> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n>> /* Assemble a name made of the constituent fields. */\n>>\n>> static const std::map primariesNames = {\n>> + { Primaries::Default, \"Default\" },\n>> { Primaries::Raw, \"RAW\" },\n>> { Primaries::Smpte170m, \"SMPTE170M\" },\n>> { Primaries::Rec709, \"Rec709\" },\n>> { Primaries::Rec2020, \"Rec2020\" },\n>> };\n>> static const std::map transferNames = {\n>> + { TransferFunction::Default, \"Default\" },\n>> { TransferFunction::Linear, \"Linear\" },\n>> { TransferFunction::Srgb, \"sRGB\" },\n>> { TransferFunction::Rec709, \"Rec709\" },\n>> };\n>> static const std::map encodingNames = {\n>> + { YcbcrEncoding::Default, \"Default\" },\n>> { YcbcrEncoding::None, \"None\" },\n>> { YcbcrEncoding::Rec601, \"Rec601\" },\n>> { YcbcrEncoding::Rec709, \"Rec709\" },\n>> { YcbcrEncoding::Rec2020, \"Rec2020\" },\n>> };\n>> static const std::map rangeNames = {\n>> + { Range::Default, \"Default\" },\n>> { Range::Full, \"Full\" },\n>> { Range::Limited, \"Limited\" },\n>> };\n>> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n>> * \\brief The pixel range used with by color space\n>> */\n>>\n>> +/**\n>> + * \\brief A constant representing a default color space\n>> + */\n>> +const ColorSpace ColorSpace::Default = {\n>> + Primaries::Default,\n>> + TransferFunction::Default,\n>> + YcbcrEncoding::Default,\n>> + Range::Default\n>> +};\n>> +\n>> /**\n>> * \\brief A constant representing a raw color space (from a sensor)\n>> */\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index 3fc8438f..ecfcf337 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n>> };\n>>\n>> static const std::vector> colorSpaceToV4l2 = {\n>> + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n>> { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n>> { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n>> { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n>> @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n>> };\n>>\n>> static const std::map primariesToV4l2 = {\n>> + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n>> { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n>> { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n>> { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n>> @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n>> };\n>>\n>> static const std::map transferFunctionToV4l2 = {\n>> + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n>> { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n>> { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n>> { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n>> };\n>>\n>> static const std::map ycbcrEncodingToV4l2 = {\n>> + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n>> { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n>> { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n>> { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n>> };\n>>\n>> static const std::map rangeToV4l2 = {\n>> + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n>> { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n>> { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n>> };\n>> --\n>> 2.31.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 16F5FBE173\n\tfor ;\n\tMon, 25 Jul 2022 11:15:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D18A63312;\n\tMon, 25 Jul 2022 13:15:13 +0200 (CEST)","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 ADB2C6330A\n\tfor ;\n\tMon, 25 Jul 2022 13:15:11 +0200 (CEST)","from [IPV6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d] (unknown\n\t[IPv6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A9EC6DD;\n\tMon, 25 Jul 2022 13:15:09 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658747713;\n\tbh=JKtXmwgkWIQb+XlEZSghbbOXEPJFfLojHqQx2+SawSc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=t5jDm5GqIsHLqgvspT+v5+PNnNy3niUnwMsl3hfni36uXR5584hwvSTzBAuwpqW49\n\tM33TMnc62vtDSkErxlZIHMvz3D3wmjAYaGMP9KMNsHx4v1qAcmSH5ypYxbZI/GEa79\n\tQ00ytau7J4tJP6tc4JkqaIIUsD73V3WH6E/xQrmd4eVeQOtfs4rJryEOOVsB1x7NyL\n\tLsGQROadKNSK95lSVchR6EM0Midmrr9RTFT8Q7ecCYTkbvwmfwz5b/ph3yC37MNaCt\n\t3vZrq4Pctq89sVFXhN1fxEoIPbQsK2Kg0ESmIx57cA40eoeTx8dMWY7iX7KW1xXXL4\n\t5J7PTNwK09jVQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658747711;\n\tbh=JKtXmwgkWIQb+XlEZSghbbOXEPJFfLojHqQx2+SawSc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=UB5xw70MDzohi6rLHvRRtj/efy30jqrwg3yQkA5d167DvuzEkIflzrTcTqLbX6ps4\n\timDdWMxMpRsEMut0aEcNxwAyPB9U1lRVj85e/9EHiyxguti8g9LEFbpnBdsuYhAYZu\n\tAhTyLZ5eNCarwbmktPXxPIaLVzL17bnnC+nwm2iQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UB5xw70M\"; dkim-atps=neutral","Message-ID":"<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>","Date":"Mon, 25 Jul 2022 16:45:04 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"David Plowman ","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24097,"web_url":"https://patchwork.libcamera.org/comment/24097/","msgid":"<259ade47c99bde5d427f330d9ef973b4012c9788.camel@collabora.com>","date":"2022-07-25T12:21:49","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi David,\n\nLe lundi 25 juillet 2022 à 11:46 +0100, David Plowman a écrit :\n> Hi Umang\n> \n> Thanks for this patch. I was wondering if I could ask for a little\n> more information on how it would be used.\n> \n> In the original libcamera ColorSpace implementation I was very keen to\n> avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n> the view that applications really should say what ColorSpace they\n> want, but at the same time I knew there will always be (bad)\n> applications that don't think about colour spaces and so I let them\n> use std::optional to allow the ColorSpace to be completely\n> unspecified.\n\n\"wrong\" application is a bit of an over statement. Even for cameras, the\ncolorspace is not always something that can be freely picked by the application.\nThe V4L2 driver, or the HW may have some limitations. A simple example is UVC,\nyou pretty much get what you get and you can't influence it. For this reason, a\nproper colorspace API needs to work both ways. Which basically imply, an\napplication should be able to not care about selecting / influencing the\ncolorspace and just read whatever the driver/HW produces.\n\nregards,\nNicolas\n\n> \n> In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n> infer the value from the overall V4L2_COLORSPACE (someone please\n> correct me if that's wrong!). In libcamera we don't have an \"overall\n> colour space\" in this way, so what would it then mean? For example, is\n> \"ColorSpace::Range::Default\" ever the same as\n> \"ColorSpace::Range::Full\", or is it always different? How do we (can\n> we even) document what it means?\n> \n> I'd be interested to hear what other folks think. Possibly I'm being a\n> bit paranoid, but then I have been bitten by \"wrong colour space\"\n> problems often enough in the past (usually just before a product\n> ships!).\n> \n> Thanks!\n> David\n> \n> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel\n> wrote:\n> > \n> > Add a Colorspace::Default which corresponds to default V4L2\n> > colorspace identifiers.\n> > \n> > \\todo Add defaults to python colorspace bindings\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > include/libcamera/color_space.h | 5 +++++\n> > src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-\n> > src/libcamera/v4l2_device.cpp | 5 +++++\n> > 3 files changed, 38 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> > index 086c56c1..0ad8da17 100644\n> > --- a/include/libcamera/color_space.h\n> > +++ b/include/libcamera/color_space.h\n> > @@ -16,6 +16,7 @@ class ColorSpace\n> > {\n> > public:\n> > enum class Primaries {\n> > + Default,\n> > Raw,\n> > Smpte170m,\n> > Rec709,\n> > @@ -23,12 +24,14 @@ public:\n> > };\n> > \n> > enum class TransferFunction {\n> > + Default,\n> > Linear,\n> > Srgb,\n> > Rec709,\n> > };\n> > \n> > enum class YcbcrEncoding {\n> > + Default,\n> > None,\n> > Rec601,\n> > Rec709,\n> > @@ -36,6 +39,7 @@ public:\n> > };\n> > \n> > enum class Range {\n> > + Default,\n> > Full,\n> > Limited,\n> > };\n> > @@ -45,6 +49,7 @@ public:\n> > {\n> > }\n> > \n> > + static const ColorSpace Default;\n> > static const ColorSpace Raw;\n> > static const ColorSpace Jpeg;\n> > static const ColorSpace Srgb;\n> > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> > index 895e5c8e..d52f58cf 100644\n> > --- a/src/libcamera/color_space.cpp\n> > +++ b/src/libcamera/color_space.cpp\n> > @@ -50,6 +50,9 @@ namespace libcamera {\n> > * \\enum ColorSpace::Primaries\n> > * \\brief The color primaries for this color space\n> > *\n> > + * \\var ColorSpace::Primaries::Default\n> > + * \\brief Use the default primaries as defined by the driver\n> > + *\n> > * \\var ColorSpace::Primaries::Raw\n> > * \\brief These are raw colors directly from a sensor, the primaries are\n> > * unspecified\n> > @@ -68,6 +71,9 @@ namespace libcamera {\n> > * \\enum ColorSpace::TransferFunction\n> > * \\brief The transfer function used for this color space\n> > *\n> > + * \\var ColorSpace::TransferFunction::Default\n> > + * \\brief Use the default transfer function as defined by the driver\n> > + *\n> > * \\var ColorSpace::TransferFunction::Linear\n> > * \\brief This color space uses a linear (identity) transfer function\n> > *\n> > @@ -82,6 +88,9 @@ namespace libcamera {\n> > * \\enum ColorSpace::YcbcrEncoding\n> > * \\brief The Y'CbCr encoding\n> > *\n> > + * \\var ColorSpace::YcbcrEncoding::Default\n> > + * \\brief Use the default Y'CbCr encoding as defined by the driver\n> > + *\n> > * \\var ColorSpace::YcbcrEncoding::None\n> > * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> > *\n> > @@ -99,6 +108,9 @@ namespace libcamera {\n> > * \\enum ColorSpace::Range\n> > * \\brief The range (sometimes \"quantisation\") for this color space\n> > *\n> > + * \\var ColorSpace::Range::Default\n> > + * Use the default range as defined by the driver\n> > + *\n> > * \\var ColorSpace::Range::Full\n> > * \\brief This color space uses full range pixel values\n> > *\n> > @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n> > {\n> > /* Print out a brief name only for standard color spaces. */\n> > \n> > - static const std::array, 6> colorSpaceNames = { {\n> > + static const std::array, 7> colorSpaceNames = { {\n> > + { ColorSpace::Default, \"Default\" },\n> > { ColorSpace::Raw, \"RAW\" },\n> > { ColorSpace::Jpeg, \"JPEG\" },\n> > { ColorSpace::Srgb, \"sRGB\" },\n> > @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n> > /* Assemble a name made of the constituent fields. */\n> > \n> > static const std::map primariesNames = {\n> > + { Primaries::Default, \"Default\" },\n> > { Primaries::Raw, \"RAW\" },\n> > { Primaries::Smpte170m, \"SMPTE170M\" },\n> > { Primaries::Rec709, \"Rec709\" },\n> > { Primaries::Rec2020, \"Rec2020\" },\n> > };\n> > static const std::map transferNames = {\n> > + { TransferFunction::Default, \"Default\" },\n> > { TransferFunction::Linear, \"Linear\" },\n> > { TransferFunction::Srgb, \"sRGB\" },\n> > { TransferFunction::Rec709, \"Rec709\" },\n> > };\n> > static const std::map encodingNames = {\n> > + { YcbcrEncoding::Default, \"Default\" },\n> > { YcbcrEncoding::None, \"None\" },\n> > { YcbcrEncoding::Rec601, \"Rec601\" },\n> > { YcbcrEncoding::Rec709, \"Rec709\" },\n> > { YcbcrEncoding::Rec2020, \"Rec2020\" },\n> > };\n> > static const std::map rangeNames = {\n> > + { Range::Default, \"Default\" },\n> > { Range::Full, \"Full\" },\n> > { Range::Limited, \"Limited\" },\n> > };\n> > @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n> > * \\brief The pixel range used with by color space\n> > */\n> > \n> > +/**\n> > + * \\brief A constant representing a default color space\n> > + */\n> > +const ColorSpace ColorSpace::Default = {\n> > + Primaries::Default,\n> > + TransferFunction::Default,\n> > + YcbcrEncoding::Default,\n> > + Range::Default\n> > +};\n> > +\n> > /**\n> > * \\brief A constant representing a raw color space (from a sensor)\n> > */\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 3fc8438f..ecfcf337 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n> > };\n> > \n> > static const std::vector> colorSpaceToV4l2 = {\n> > + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n> > { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n> > };\n> > \n> > static const std::map primariesToV4l2 = {\n> > + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n> > { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n> > };\n> > \n> > static const std::map transferFunctionToV4l2 = {\n> > + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n> > { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > };\n> > \n> > static const std::map ycbcrEncodingToV4l2 = {\n> > + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n> > { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > };\n> > \n> > static const std::map rangeToV4l2 = {\n> > + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n> > { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > };\n> > --\n> > 2.31.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 06CD5C3275\n\tfor ;\n\tMon, 25 Jul 2022 12:22:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C00463312;\n\tMon, 25 Jul 2022 14:22:02 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[46.235.227.172])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2F886330A\n\tfor ;\n\tMon, 25 Jul 2022 14:22:00 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 827956601AD9;\n\tMon, 25 Jul 2022 13:21:59 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658751722;\n\tbh=NhkNUkCiJaZ6GSGy5iEVEgZeWhpzZJhAJphVEhdyt6o=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xD3rncuU4EXRh2AtngdIlYfGEhPG8Y/t8GRN/S0fTcHBj/MxssDDyIu6G8jIcUAYM\n\tpAb8qEHbmswGyZCOahyL/phdxAui1+HUCMkBS3pLbVF4/t7dC8Iwrsb7Zrq4CCysb8\n\teIBWh+e9tR9RDc9vCqpdl+l0dI6wkyuaEhtJ/BlE8zJMJAWHQsH/ubyXa25Dr2DN73\n\tW0JhFNxVjiR40NiQqNSlApJPD2pLMvpXIQt6vpbveEgMT89dQxyT38SFAnCqSv3XnX\n\toWsNYSFfGGHF8Ex20FPj5br/zCPjjOTKdoQJWaXmNM0LdaSSlP3iQYNEqUZnCH1uEb\n\tICcm+fjmNuzlw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1658751720;\n\tbh=NhkNUkCiJaZ6GSGy5iEVEgZeWhpzZJhAJphVEhdyt6o=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=KsnRennEK9xWdfJLkfFg/jWgYgWeAbZA2L5JQizim82//lDAiNEDqwZRNuFuRZGOP\n\t4ItL3fnyUud8EI3YhHi8X0vh5e+j0HBp8Ohz/jhScAiyPsPktQk3fItCbTwwGorJAd\n\tqit/DdeAuKvJEh6md83hMF0OuQD+Ws7h5RmqBw6gVtLDb0i0eXJo6YoUy59huhPUVu\n\tBnKJnFp2qSJf6KCoIh2uhMAHvroYV0CXefBQnB/d8kVwgtrtmH1Yo92zJ+hemd8nBN\n\tF8QoJnDVHWRRxfkn8YsxWQAFT+gDV/5Ka0AA6lxUhuCLDIkQxleVRs+5YvyC6U/gcw\n\tlD+DTZoyeoB8w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"KsnRennE\"; dkim-atps=neutral","Message-ID":"<259ade47c99bde5d427f330d9ef973b4012c9788.camel@collabora.com>","To":"David Plowman , Umang Jain\n\t","Date":"Mon, 25 Jul 2022 08:21:49 -0400","In-Reply-To":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24109,"web_url":"https://patchwork.libcamera.org/comment/24109/","msgid":"","date":"2022-07-25T15:40:44","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\n\nOn Mon, 25 Jul 2022 at 13:22, Nicolas Dufresne via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> Hi David,\n>\n> Le lundi 25 juillet 2022 à 11:46 +0100, David Plowman a écrit :\n> > Hi Umang\n> >\n> > Thanks for this patch. I was wondering if I could ask for a little\n> > more information on how it would be used.\n> >\n> > In the original libcamera ColorSpace implementation I was very keen to\n> > avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n> > the view that applications really should say what ColorSpace they\n> > want, but at the same time I knew there will always be (bad)\n> > applications that don't think about colour spaces and so I let them\n> > use std::optional to allow the ColorSpace to be completely\n> > unspecified.\n>\n> \"wrong\" application is a bit of an over statement. Even for cameras, the\n> colorspace is not always something that can be freely picked by the\n> application.\n> The V4L2 driver, or the HW may have some limitations. A simple example is\n> UVC,\n> you pretty much get what you get and you can't influence it. For this\n> reason, a\n> proper colorspace API needs to work both ways. Which basically imply, an\n> application should be able to not care about selecting / influencing the\n> colorspace and just read whatever the driver/HW produces.\n>\n\nWouldn't the pipeline_handler::validate() take care of this? It ought to\nknow the\nsensor output colourspace, and return that regardless of what the\napplication\nmay have requested. Apologies if I got this wrong.\n\nNaush\n\n\n\n>\n> regards,\n> Nicolas\n>\n> >\n> > In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n> > infer the value from the overall V4L2_COLORSPACE (someone please\n> > correct me if that's wrong!). In libcamera we don't have an \"overall\n> > colour space\" in this way, so what would it then mean? For example, is\n> > \"ColorSpace::Range::Default\" ever the same as\n> > \"ColorSpace::Range::Full\", or is it always different? How do we (can\n> > we even) document what it means?\n> >\n> > I'd be interested to hear what other folks think. Possibly I'm being a\n> > bit paranoid, but then I have been bitten by \"wrong colour space\"\n> > problems often enough in the past (usually just before a product\n> > ships!).\n> >\n> > Thanks!\n> > David\n> >\n> > On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel\n> > wrote:\n> > >\n> > > Add a Colorspace::Default which corresponds to default V4L2\n> > > colorspace identifiers.\n> > >\n> > > \\todo Add defaults to python colorspace bindings\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > include/libcamera/color_space.h | 5 +++++\n> > > src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-\n> > > src/libcamera/v4l2_device.cpp | 5 +++++\n> > > 3 files changed, 38 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/color_space.h\n> b/include/libcamera/color_space.h\n> > > index 086c56c1..0ad8da17 100644\n> > > --- a/include/libcamera/color_space.h\n> > > +++ b/include/libcamera/color_space.h\n> > > @@ -16,6 +16,7 @@ class ColorSpace\n> > > {\n> > > public:\n> > > enum class Primaries {\n> > > + Default,\n> > > Raw,\n> > > Smpte170m,\n> > > Rec709,\n> > > @@ -23,12 +24,14 @@ public:\n> > > };\n> > >\n> > > enum class TransferFunction {\n> > > + Default,\n> > > Linear,\n> > > Srgb,\n> > > Rec709,\n> > > };\n> > >\n> > > enum class YcbcrEncoding {\n> > > + Default,\n> > > None,\n> > > Rec601,\n> > > Rec709,\n> > > @@ -36,6 +39,7 @@ public:\n> > > };\n> > >\n> > > enum class Range {\n> > > + Default,\n> > > Full,\n> > > Limited,\n> > > };\n> > > @@ -45,6 +49,7 @@ public:\n> > > {\n> > > }\n> > >\n> > > + static const ColorSpace Default;\n> > > static const ColorSpace Raw;\n> > > static const ColorSpace Jpeg;\n> > > static const ColorSpace Srgb;\n> > > diff --git a/src/libcamera/color_space.cpp\n> b/src/libcamera/color_space.cpp\n> > > index 895e5c8e..d52f58cf 100644\n> > > --- a/src/libcamera/color_space.cpp\n> > > +++ b/src/libcamera/color_space.cpp\n> > > @@ -50,6 +50,9 @@ namespace libcamera {\n> > > * \\enum ColorSpace::Primaries\n> > > * \\brief The color primaries for this color space\n> > > *\n> > > + * \\var ColorSpace::Primaries::Default\n> > > + * \\brief Use the default primaries as defined by the driver\n> > > + *\n> > > * \\var ColorSpace::Primaries::Raw\n> > > * \\brief These are raw colors directly from a sensor, the primaries\n> are\n> > > * unspecified\n> > > @@ -68,6 +71,9 @@ namespace libcamera {\n> > > * \\enum ColorSpace::TransferFunction\n> > > * \\brief The transfer function used for this color space\n> > > *\n> > > + * \\var ColorSpace::TransferFunction::Default\n> > > + * \\brief Use the default transfer function as defined by the driver\n> > > + *\n> > > * \\var ColorSpace::TransferFunction::Linear\n> > > * \\brief This color space uses a linear (identity) transfer function\n> > > *\n> > > @@ -82,6 +88,9 @@ namespace libcamera {\n> > > * \\enum ColorSpace::YcbcrEncoding\n> > > * \\brief The Y'CbCr encoding\n> > > *\n> > > + * \\var ColorSpace::YcbcrEncoding::Default\n> > > + * \\brief Use the default Y'CbCr encoding as defined by the driver\n> > > + *\n> > > * \\var ColorSpace::YcbcrEncoding::None\n> > > * \\brief There is no defined Y'CbCr encoding (used for non-YUV\n> formats)\n> > > *\n> > > @@ -99,6 +108,9 @@ namespace libcamera {\n> > > * \\enum ColorSpace::Range\n> > > * \\brief The range (sometimes \"quantisation\") for this color space\n> > > *\n> > > + * \\var ColorSpace::Range::Default\n> > > + * Use the default range as defined by the driver\n> > > + *\n> > > * \\var ColorSpace::Range::Full\n> > > * \\brief This color space uses full range pixel values\n> > > *\n> > > @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n> > > {\n> > > /* Print out a brief name only for standard color spaces. */\n> > >\n> > > - static const std::array,\n> 6> colorSpaceNames = { {\n> > > + static const std::array,\n> 7> colorSpaceNames = { {\n> > > + { ColorSpace::Default, \"Default\" },\n> > > { ColorSpace::Raw, \"RAW\" },\n> > > { ColorSpace::Jpeg, \"JPEG\" },\n> > > { ColorSpace::Srgb, \"sRGB\" },\n> > > @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n> > > /* Assemble a name made of the constituent fields. */\n> > >\n> > > static const std::map primariesNames =\n> {\n> > > + { Primaries::Default, \"Default\" },\n> > > { Primaries::Raw, \"RAW\" },\n> > > { Primaries::Smpte170m, \"SMPTE170M\" },\n> > > { Primaries::Rec709, \"Rec709\" },\n> > > { Primaries::Rec2020, \"Rec2020\" },\n> > > };\n> > > static const std::map\n> transferNames = {\n> > > + { TransferFunction::Default, \"Default\" },\n> > > { TransferFunction::Linear, \"Linear\" },\n> > > { TransferFunction::Srgb, \"sRGB\" },\n> > > { TransferFunction::Rec709, \"Rec709\" },\n> > > };\n> > > static const std::map\n> encodingNames = {\n> > > + { YcbcrEncoding::Default, \"Default\" },\n> > > { YcbcrEncoding::None, \"None\" },\n> > > { YcbcrEncoding::Rec601, \"Rec601\" },\n> > > { YcbcrEncoding::Rec709, \"Rec709\" },\n> > > { YcbcrEncoding::Rec2020, \"Rec2020\" },\n> > > };\n> > > static const std::map rangeNames = {\n> > > + { Range::Default, \"Default\" },\n> > > { Range::Full, \"Full\" },\n> > > { Range::Limited, \"Limited\" },\n> > > };\n> > > @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const\n> std::optional &colorSpace)\n> > > * \\brief The pixel range used with by color space\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief A constant representing a default color space\n> > > + */\n> > > +const ColorSpace ColorSpace::Default = {\n> > > + Primaries::Default,\n> > > + TransferFunction::Default,\n> > > + YcbcrEncoding::Default,\n> > > + Range::Default\n> > > +};\n> > > +\n> > > /**\n> > > * \\brief A constant representing a raw color space (from a sensor)\n> > > */\n> > > diff --git a/src/libcamera/v4l2_device.cpp\n> b/src/libcamera/v4l2_device.cpp\n> > > index 3fc8438f..ecfcf337 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -770,6 +770,7 @@ static const std::map\n> v4l2ToRange = {\n> > > };\n> > >\n> > > static const std::vector>\n> colorSpaceToV4l2 = {\n> > > + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n> > > { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > > { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > > { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > > @@ -779,6 +780,7 @@ static const std::vector v4l2_colorspace>> colorSpaceToV4l\n> > > };\n> > >\n> > > static const std::map\n> primariesToV4l2 = {\n> > > + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n> > > { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > > { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M\n> },\n> > > { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > > @@ -786,18 +788,21 @@ static const std::map v4l2_colorspace> primariesToV4l2 =\n> > > };\n> > >\n> > > static const std::map\n> transferFunctionToV4l2 = {\n> > > + { ColorSpace::TransferFunction::Default,\n> V4L2_XFER_FUNC_DEFAULT },\n> > > { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > > { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > > { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > > };\n> > >\n> > > static const std::map\n> ycbcrEncodingToV4l2 = {\n> > > + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n> > > { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > > { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > > { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > > };\n> > >\n> > > static const std::map\n> rangeToV4l2 = {\n> > > + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n> > > { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > > { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> > > };\n> > > --\n> > > 2.31.1\n> > >\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 203C8C3275\n\tfor ;\n\tMon, 25 Jul 2022 15:41:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 708B363312;\n\tMon, 25 Jul 2022 17:41:03 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA4AF6330A\n\tfor ;\n\tMon, 25 Jul 2022 17:41:01 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id t1so18630384lft.8\n\tfor ;\n\tMon, 25 Jul 2022 08:41:01 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658763663;\n\tbh=LlZ24Mht1dv0mQb7th0LIbOglyoCpLdqqCgj9cCdDvE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=honkObcqSjzxiTDo2cpuDsgemOX1P3zF5PoXFx8q9LmQibzVDMBuuCAQoG5O4seE2\n\t5lzOTouuQRNMdPAyp+VgI7LOg7OUzZnpPngoIv7J+NXkiIwtc4Idbw3tc5gIqTlPRI\n\t5a3gub1rP15Fs2yDyMIHEZAAiBNSkrOupWbutoV6GrxoxrKGkIv7tPpuoF5MCzOrFi\n\t9rfx9fPPUbaL0JaBfq7/hrGKEK63pdb2O4tz6752U5wVqsbklfl78aL8o+6YYtKnO8\n\tVCB6iNg/v7Derr9YbgmLj79QyEaxtb5FgLkHKsBKHvR9L9hobvtmQ0VAUjCo6urq50\n\t8fn2EztXlWesg==","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=JePMONN1uan0b6jA5SewaRHrj1uc0UyFZ9MvsHiPuNE=;\n\tb=E75kl9070v1zRB85IvftOdwqIcLEyPlchKAMOeJsyXjf6Puq0GTuvkP3e6XrzdIqBq\n\t860tNkkhshV/H7fCIqbllcerTijTGBWQxQxTOLVCNUEabFeudm77iYCcSLQIfp3At/sr\n\tfL979+OfBcv2N1t4qSVs8/iiuf+fvcoBUxsiKLj+m088MUwxu2GQ0W+LE59Tuq7rSlUc\n\tJn2+PLf5lxBSBBMSxu1jMSvcyMh22bPd2LQWpqHt4AwgEe2RDKu93hM9JpKHufnMnoL+\n\tV+ZNxTrhWd6jr80RN7bBR553H66orQGGBm0jW/3H4GYrqNt77VepRGtp+f7qh4QI1FON\n\tmSLw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"E75kl907\"; dkim-atps=neutral","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=JePMONN1uan0b6jA5SewaRHrj1uc0UyFZ9MvsHiPuNE=;\n\tb=amjkGeemFSGF1hfv3ZWdd65Ya+H6PX3LCXxm1wE1qj0tGGO9FxXFkcFD6QxFno8RPh\n\t3MHv01TxtUKPLntSNVzf2WQg3lI59GecSDk0qT3qiCj+baLTW361eSZ1Cj6kWH7WoCMg\n\tsYXNpB/a7psw5xgnaiR20wV1iXg0rMA9UnbSpPyildzENwOGFFD8ItArp91QhMIb87Ce\n\tYN2ZgGHcUbP/feH1S+eERh5ErOFy0sPlF0g5+GvJZ8cOSy4jTDKvYy8N0dv4rLW+DBZg\n\td+ctu4II9VH5cqcZ5Fn5U5GVMA90DoFEvHuH79yuVIgY2Ii+PQCFMVU8i8I2CkddSFIP\n\tQL1w==","X-Gm-Message-State":"AJIora9ZypytDeWTUafz+eqBzA3/WYiDrHX1/1OzHb8cJclFZXMBP7CF\n\ti9qlYchTq+W2x+56QuAa/5Vc/M/MmPzFopoTxz4Abg==","X-Google-Smtp-Source":"AGRyM1tWkleL35njns8qDWkqNY0mhslkoopbvnwqCW3e+m6Ny+ylXFH7FCacb01jcum7GOP5y2peqyXhpe+i5feeWco=","X-Received":"by 2002:a05:6512:b2a:b0:48a:2aaf:2ad3 with SMTP id\n\tw42-20020a0565120b2a00b0048a2aaf2ad3mr5369041lfu.552.1658763660888;\n\tMon, 25 Jul 2022 08:41:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<259ade47c99bde5d427f330d9ef973b4012c9788.camel@collabora.com>","In-Reply-To":"<259ade47c99bde5d427f330d9ef973b4012c9788.camel@collabora.com>","Date":"Mon, 25 Jul 2022 16:40:44 +0100","Message-ID":"","To":"Nicolas Dufresne ","Content-Type":"multipart/alternative; boundary=\"000000000000c6266205e4a302ea\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24142,"web_url":"https://patchwork.libcamera.org/comment/24142/","msgid":"","date":"2022-07-26T10:12:32","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Umang, everyone\n\nThanks for the reply. Perhaps I can add some more thoughts about this question.\n\nOn Mon, 25 Jul 2022 at 12:15, Umang Jain wrote:\n>\n> Hi David,\n>\n> On 7/25/22 16:16, David Plowman wrote:\n> > Hi Umang\n> >\n> > Thanks for this patch. I was wondering if I could ask for a little\n> > more information on how it would be used.\n>\n>\n> Yes, so the idea here is to provide a colorspace where one(or more)\n> identifiers are specified by the user whereas others can be left upto\n> the driver to fill in.\n>\n> I wasn't aware of this use case until very recently. In gstreamer, it\n> seems possible that the user wants to use(enforce?) a particular\n> transfer function whereas other identifier are left upto the driver to fill.\n>\n> I am not very sure how would this be helpful (since colorspace\n> identifiers depend on each other I think), and mis-matches between them\n> might not make any sense at all.\n>\n> But such a use case as has been identified it seems, atleast in the\n> gstreamer case - so to satisfy that, I went ahead with this approach. I\n> couldn't get a better solution where the user specifies a set of\n> identifiers to use for colorspace, at the same time depending on the\n> driver to fill unknown or default values for remaining identifier(s).\n>\n> >\n> > In the original libcamera ColorSpace implementation I was very keen to\n> > avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n> > the view that applications really should say what ColorSpace they\n> > want, but at the same time I knew there will always be (bad)\n> > applications that don't think about colour spaces and so I let them\n> > use std::optional to allow the ColorSpace to be completely\n> > unspecified.\n>\n>\n> Right, so when colorspace is completely unspecified, all values are\n> default-ed (V4L2Device::fromColorSpace()) in the v4l2_format.\n>\n> If I am asked about what's the goal of this patch, I would say that it\n> enables the use case where a sub-set of colorspace identifiers are\n> known, while others need to use defaults. Currently there is no default\n> std::optional hence I added one, to facilitate\n> the application layer. The application should ideally copy the\n> libcamera::ColorSpace::Default as base, swap the identifiers it wants to\n> use in the copy, and send the custom colorspace for validation.\n>\n> Also note, the mapping is only one way (from libcamera to V4L2)\n> intentionally. It shouldn't https://github.com/raspberrypi/picamera2#installationbe the case where driver itself returns\n> \"::Default\" back to user.\n>\n> >\n> > In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n> > infer the value from the overall V4L2_COLORSPACE (someone please\n> > correct me if that's wrong!). In libcamera we don't have an \"overall\n>\n>\n> If we could infer all the identifier values from\n> V4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:\n>\n> V4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT,\n> V4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.\n>\n> > colour space\" in this way, so what would it then mean? For example, is\n> > \"ColorSpace::Range::Default\" ever the same as\n> > \"ColorSpace::Range::Full\", or is it always different? How do we (can\n> > we even) document what it means?\n>\n>\n> Maybe we should look at V4L2 level first, before libcamera for e.g.\n>\n> Would V4L2_QUANTIZATION_DEFAULT be ever same as\n> V4L2_QUANTIZATION_FULL_RANGE ?\n>\n> >\n> > I'd be interested to hear what other folks think. Possibly I'm being a\n>\n>\n> I am all ears here as well, so let' see\n>\n> > bit paranoid, but then I have been bitten by \"wrong colour space\"\n> > problems often enough in the past (usually just before a product\n> > ships!).\n>\n> ouch :-S\n\nMy overall feeling is that I don't like having \"default\" things\nbecause I'm worried that they'll start appearing when you don't expect\nthem. And then how would you know what they meant? As you know by now,\nI'm a bit paranoid about colour spaces going wrong...\n\nHowever, I see the point about these values possibly being convenient\nfor negotiating colour spaces with (for example) gstreamer. But\nperhaps we could reduce the risk of them spreading, maybe we might\nconsider:\n\n* Documenting clearly that \"default\" values are only \"inputs\" (what\nyou want), and are never returned (what you got).\n* Adding a valid() method that complains if a ColorSpace has any\nundefined/default values in it?\n* When a function returns a ColorSpace that has been set, can we make\nthem check that the returned value is \"valid()\"?\n* I also wasn't quite sure why a ColorSpace::Default is needed, would\na ColorSpace be regarded as being \"default\" if every field is\n\"default\", or just some of them? Or could an unset std::optional be\nused instead?\n\nWhat do you think?\n\nThanks!\nDavid\n\n>\n> >\n> > Thanks!\n> > David\n> >\n> > On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel\n> > wrote:\n> >> Add a Colorspace::Default which corresponds to default V4L2\n> >> colorspace identifiers.\n> >>\n> >> \\todo Add defaults to python colorspace bindings\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> include/libcamera/color_space.h | 5 +++++\n> >> src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-\n> >> src/libcamera/v4l2_device.cpp | 5 +++++\n> >> 3 files changed, 38 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> >> index 086c56c1..0ad8da17 100644\n> >> --- a/include/libcamera/color_space.h\n> >> +++ b/include/libcamera/color_space.h\n> >> @@ -16,6 +16,7 @@ class ColorSpace\n> >> {\n> >> public:\n> >> enum class Primaries {\n> >> + Default,\n> >> Raw,\n> >> Smpte170m,\n> >> Rec709,\n> >> @@ -23,12 +24,14 @@ public:\n> >> };\n> >>\n> >> enum class TransferFunction {\n> >> + Default,\n> >> Linear,\n> >> Srgb,\n> >> Rec709,\n> >> };\n> >>\n> >> enum class YcbcrEncoding {\n> >> + Default,\n> >> None,\n> >> Rec601,\n> >> Rec709,\n> >> @@ -36,6 +39,7 @@ public:\n> >> };\n> >>\n> >> enum class Range {\n> >> + Default,\n> >> Full,\n> >> Limited,\n> >> };\n> >> @@ -45,6 +49,7 @@ public:\n> >> {\n> >> }\n> >>\n> >> + static const ColorSpace Default;\n> >> static const ColorSpace Raw;\n> >> static const ColorSpace Jpeg;\n> >> static const ColorSpace Srgb;\n> >> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> >> index 895e5c8e..d52f58cf 100644\n> >> --- a/src/libcamera/color_space.cpp\n> >> +++ b/src/libcamera/color_space.cpp\n> >> @@ -50,6 +50,9 @@ namespace libcamera {\n> >> * \\enum ColorSpace::Primaries\n> >> * \\brief The color primaries for this color space\n> >> *\n> >> + * \\var ColorSpace::Primaries::Default\n> >> + * \\brief Use the default primaries as defined by the driver\n> >> + *\n> >> * \\var ColorSpace::Primaries::Raw\n> >> * \\brief These are raw colors directly from a sensor, the primaries are\n> >> * unspecified\n> >> @@ -68,6 +71,9 @@ namespace libcamera {\n> >> * \\enum ColorSpace::TransferFunction\n> >> * \\brief The transfer function used for this color space\n> >> *\n> >> + * \\var ColorSpace::TransferFunction::Default\n> >> + * \\brief Use the default transfer function as defined by the driver\n> >> + *\n> >> * \\var ColorSpace::TransferFunction::Linear\n> >> * \\brief This color space uses a linear (identity) transfer function\n> >> *\n> >> @@ -82,6 +88,9 @@ namespace libcamera {\n> >> * \\enum ColorSpace::YcbcrEncoding\n> >> * \\brief The Y'CbCr encoding\n> >> *\n> >> + * \\var ColorSpace::YcbcrEncoding::Default\n> >> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n> >> + *\n> >> * \\var ColorSpace::YcbcrEncoding::None\n> >> * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> >> *\n> >> @@ -99,6 +108,9 @@ namespace libcamera {\n> >> * \\enum ColorSpace::Range\n> >> * \\brief The range (sometimes \"quantisation\") for this color space\n> >> *\n> >> + * \\var ColorSpace::Range::Default\n> >> + * Use the default range as defined by the driver\n> >> + *\n> >> * \\var ColorSpace::Range::Full\n> >> * \\brief This color space uses full range pixel values\n> >> *\n> >> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n> >> {\n> >> /* Print out a brief name only for standard color spaces. */\n> >>\n> >> - static const std::array, 6> colorSpaceNames = { {\n> >> + static const std::array, 7> colorSpaceNames = { {\n> >> + { ColorSpace::Default, \"Default\" },\n> >> { ColorSpace::Raw, \"RAW\" },\n> >> { ColorSpace::Jpeg, \"JPEG\" },\n> >> { ColorSpace::Srgb, \"sRGB\" },\n> >> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n> >> /* Assemble a name made of the constituent fields. */\n> >>\n> >> static const std::map primariesNames = {\n> >> + { Primaries::Default, \"Default\" },\n> >> { Primaries::Raw, \"RAW\" },\n> >> { Primaries::Smpte170m, \"SMPTE170M\" },\n> >> { Primaries::Rec709, \"Rec709\" },\n> >> { Primaries::Rec2020, \"Rec2020\" },\n> >> };\n> >> static const std::map transferNames = {\n> >> + { TransferFunction::Default, \"Default\" },\n> >> { TransferFunction::Linear, \"Linear\" },\n> >> { TransferFunction::Srgb, \"sRGB\" },\n> >> { TransferFunction::Rec709, \"Rec709\" },\n> >> };\n> >> static const std::map encodingNames = {\n> >> + { YcbcrEncoding::Default, \"Default\" },\n> >> { YcbcrEncoding::None, \"None\" },\n> >> { YcbcrEncoding::Rec601, \"Rec601\" },\n> >> { YcbcrEncoding::Rec709, \"Rec709\" },\n> >> { YcbcrEncoding::Rec2020, \"Rec2020\" },\n> >> };\n> >> static const std::map rangeNames = {\n> >> + { Range::Default, \"Default\" },\n> >> { Range::Full, \"Full\" },\n> >> { Range::Limited, \"Limited\" },\n> >> };\n> >> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n> >> * \\brief The pixel range used with by color space\n> >> */\n> >>\n> >> +/**\n> >> + * \\brief A constant representing a default color space\n> >> + */\n> >> +const ColorSpace ColorSpace::Default = {\n> >> + Primaries::Default,\n> >> + TransferFunction::Default,\n> >> + YcbcrEncoding::Default,\n> >> + Range::Default\n> >> +};\n> >> +\n> >> /**\n> >> * \\brief A constant representing a raw color space (from a sensor)\n> >> */\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index 3fc8438f..ecfcf337 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n> >> };\n> >>\n> >> static const std::vector> colorSpaceToV4l2 = {\n> >> + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n> >> { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> >> { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> >> { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> >> @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n> >> };\n> >>\n> >> static const std::map primariesToV4l2 = {\n> >> + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n> >> { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> >> { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> >> { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> >> @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n> >> };\n> >>\n> >> static const std::map transferFunctionToV4l2 = {\n> >> + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n> >> { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> >> { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> >> { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> >> };\n> >>\n> >> static const std::map ycbcrEncodingToV4l2 = {\n> >> + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n> >> { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> >> { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> >> { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> >> };\n> >>\n> >> static const std::map rangeToV4l2 = {\n> >> + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n> >> { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> >> { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n> >> };\n> >> --\n> >> 2.31.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 402F0BE173\n\tfor ;\n\tTue, 26 Jul 2022 10:12:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 708A263312;\n\tTue, 26 Jul 2022 12:12:45 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BCE4D60487\n\tfor ;\n\tTue, 26 Jul 2022 12:12:43 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id va17so25322386ejb.0\n\tfor ;\n\tTue, 26 Jul 2022 03:12:43 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658830365;\n\tbh=xX+ndxhQF5TplzlD7rSh9GpJWkGJXi1wsGIj0egbxGs=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HYhKpqyAx5EumQD6On7+9F6b8uCe91bU7ILeS9YrUHhJqHHeLwGT690zng/HRaj5T\n\trVUhW2lRNgarwEd+aDovnyx8S49ORkZkjbWRbMRHpcR2HNRlT1D6vwwJSHs+By/eAh\n\t2vdMcMjYD/8MO7V0y2oYfYCf0aMOMYudqt39s96t+QRPKyoxXlFganYIntw1t3r5mr\n\tlZHT+ANgJcQr2cd/9wcW6Dt0ebkZLk0JpY/wiMPM6ucH6ICtiikvftCKr11/zPUPYy\n\txOTd0sRA57NLVQRW7Mp8S9KVDQeHmpWJRLs2ySrPOLB+ddDG3D3Ra+5Z2NEKyGyOz0\n\tP3nW/5pZDWz/w==","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=yeSgnlssCvHQ7vft2JxUADSwM2AQh8cHTLRH8LQ9vtU=;\n\tb=FwNB8JTm6JVP/n4xW15IOQnOHorPGI+2IWRoEJV8wZM9zGWStIWzAu42k2dac2xH1n\n\txW99k0AVMpcpd6rgcaNjLTbgKuYNtPtwEVGimwgOLyQ3b3eKQdbaOn/o4wcZRXCijEzr\n\tRooL1FphL5hQD2xbh0bMXUqpeBLcKoh1+lM7uwM7q+ypVCJnNURFrW1FBctIdR7q/ea9\n\tqnz/QkuwU236LB6Pq+LRaa0px40MDA5wQfEfusr8sXm+zNpLkKXhXo1V0687xWweygww\n\t4HwnSxtmm45F4oJnEU9pT6QWtXjoeN/EWAkh18Cx0ewpONWNHhfKXhbwz4ej9emy5Jhk\n\tqYWQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"FwNB8JTm\"; dkim-atps=neutral","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=yeSgnlssCvHQ7vft2JxUADSwM2AQh8cHTLRH8LQ9vtU=;\n\tb=ngpY/lD9Xw2lMnYPc7xNf68AX4nPqfGQ85UuuIbPeHkk3Aqsl+4L7BOdbQAANYZ80p\n\tj9Vwo+L7qtBFcJrytpLZxaNNWbBr9zZ1dvvkfpCas0SwnzdoVmLRi34P8sj0Ba1u6uZG\n\tal2eRnIV0Apru3E9o7hHBnGwzIoa/kM/34WkFNL1hf2/62FTplpqiEuZ0/l0IT0KuisC\n\t0nkxm28Qs3xfjbe2cBGewJOWTC0xMBLvDoyDus3BGipbTMRWj0TvXsL4+/D/EazOjxH6\n\tlWGgT/czAvsnHAYlEtOsbkaNh8npCqAmCktzeWapuFTRCItaZCchO1DGi+AgGXnraYOw\n\t/HMQ==","X-Gm-Message-State":"AJIora/23yVp38Py55ZmQ7yPl5JLsmkftma0a5CK4hJutzzC/hh4+9Mt\n\t27f72AY/oDspgeCuVmscNc3xcW3kl17s7VcWt6525A==","X-Google-Smtp-Source":"AGRyM1tJ0godjjBZ1Jje/C0pA0g1jJkb0BTytzIspu+NghKGsYHAiVZnIzNbwNTi7N4EtMP9GA1kwtlBUQMjNDsRLMY=","X-Received":"by 2002:a17:906:9b8e:b0:72f:c504:461 with SMTP id\n\tdd14-20020a1709069b8e00b0072fc5040461mr12846057ejc.655.1658830362917;\n\tTue, 26 Jul 2022 03:12:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>","In-Reply-To":"<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>","Date":"Tue, 26 Jul 2022 11:12:32 +0100","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24145,"web_url":"https://patchwork.libcamera.org/comment/24145/","msgid":"<72bd1d6c-21f2-61df-ff17-5a9f2447c6ee@ideasonboard.com>","date":"2022-07-26T12:12:08","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nOn 7/26/22 15:42, David Plowman wrote:\n> Hi Umang, everyone\n>\n> Thanks for the reply. Perhaps I can add some more thoughts about this question.\n\n\nThank you for your thoughts.\n\n>\n> On Mon, 25 Jul 2022 at 12:15, Umang Jain wrote:\n>> Hi David,\n>>\n>> On 7/25/22 16:16, David Plowman wrote:\n>>> Hi Umang\n>>>\n>>> Thanks for this patch. I was wondering if I could ask for a little\n>>> more information on how it would be used.\n>>\n>> Yes, so the idea here is to provide a colorspace where one(or more)\n>> identifiers are specified by the user whereas others can be left upto\n>> the driver to fill in.\n>>\n>> I wasn't aware of this use case until very recently. In gstreamer, it\n>> seems possible that the user wants to use(enforce?) a particular\n>> transfer function whereas other identifier are left upto the driver to fill.\n>>\n>> I am not very sure how would this be helpful (since colorspace\n>> identifiers depend on each other I think), and mis-matches between them\n>> might not make any sense at all.\n>>\n>> But such a use case as has been identified it seems, atleast in the\n>> gstreamer case - so to satisfy that, I went ahead with this approach. I\n>> couldn't get a better solution where the user specifies a set of\n>> identifiers to use for colorspace, at the same time depending on the\n>> driver to fill unknown or default values for remaining identifier(s).\n>>\n>>> In the original libcamera ColorSpace implementation I was very keen to\n>>> avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n>>> the view that applications really should say what ColorSpace they\n>>> want, but at the same time I knew there will always be (bad)\n>>> applications that don't think about colour spaces and so I let them\n>>> use std::optional to allow the ColorSpace to be completely\n>>> unspecified.\n>>\n>> Right, so when colorspace is completely unspecified, all values are\n>> default-ed (V4L2Device::fromColorSpace()) in the v4l2_format.\n>>\n>> If I am asked about what's the goal of this patch, I would say that it\n>> enables the use case where a sub-set of colorspace identifiers are\n>> known, while others need to use defaults. Currently there is no default\n>> std::optional hence I added one, to facilitate\n>> the application layer. The application should ideally copy the\n>> libcamera::ColorSpace::Default as base, swap the identifiers it wants to\n>> use in the copy, and send the custom colorspace for validation.\n>>\n>> Also note, the mapping is only one way (from libcamera to V4L2)\n>> intentionally. It shouldn't https://github.com/raspberrypi/picamera2#installationbe the case where driver itself returns\n>> \"::Default\" back to user.\n>>\n>>> In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n>>> infer the value from the overall V4L2_COLORSPACE (someone please\n>>> correct me if that's wrong!). In libcamera we don't have an \"overall\n>>\n>> If we could infer all the identifier values from\n>> V4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:\n>>\n>> V4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT,\n>> V4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.\n>>\n>>> colour space\" in this way, so what would it then mean? For example, is\n>>> \"ColorSpace::Range::Default\" ever the same as\n>>> \"ColorSpace::Range::Full\", or is it always different? How do we (can\n>>> we even) document what it means?\n>>\n>> Maybe we should look at V4L2 level first, before libcamera for e.g.\n>>\n>> Would V4L2_QUANTIZATION_DEFAULT be ever same as\n>> V4L2_QUANTIZATION_FULL_RANGE ?\n>>\n>>> I'd be interested to hear what other folks think. Possibly I'm being a\n>>\n>> I am all ears here as well, so let' see\n>>\n>>> bit paranoid, but then I have been bitten by \"wrong colour space\"\n>>> problems often enough in the past (usually just before a product\n>>> ships!).\n>> ouch :-S\n> My overall feeling is that I don't like having \"default\" things\n> because I'm worried that they'll start appearing when you don't expect\n> them. And then how would you know what they meant? As you know by now,\n> I'm a bit paranoid about colour spaces going wrong...\n>\n> However, I see the point about these values possibly being convenient\n> for negotiating colour spaces with (for example) gstreamer. But\n> perhaps we could reduce the risk of them spreading, maybe we might\n> consider:\n>\n> * Documenting clearly that \"default\" values are only \"inputs\" (what\n> you want), and are never returned (what you got).\n\n\nI've already made it impossible to return \"default\" values in this \npatch. That's the reasoning behind you see the mappings going from \nlibcamera to V4L2 /only/ and not other way around.\n\nYes we can clarify the document on that front.\n\n> * Adding a valid() method that complains if a ColorSpace has any\n> undefined/default values in it?\n> * When a function returns a ColorSpace that has been set, can we make\n> them check that the returned value is \"valid()\"?\n\n\nThe driver always return a colorspace which is supported right? I \nbelieve a supported colorspace would not _DEFAULT identifier.\n\nvalid() on the returned colorspace means we don't trust the driver? It \nmight have set one of the fields as _DEFAULT ?\n\nI am not sure what valid() will try to achieve here. Provided there is \nalready handled case where any the mappings is missing in \nlibcamera(colorspace will be std::nullopt) [1]\n\n[1] \nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n816\n\n> * I also wasn't quite sure why a ColorSpace::Default is needed, would\n\n\nConvenience\n\n> a ColorSpace be regarded as being \"default\" if every field is\n> \"default\", or just some of them? Or could an unset std::optional be\n> used instead?\n\n\nIf some fields are specific values than the \"default\" identifiers, it's \nnot a default colorspace. A default colorspace is when \"all\" fields are \ndefault and is equal to libcamera::ColorSpace::Default.\n\nA std::optional is initialised with std::nullopt.  How would I set a \ncolorspace where I want a specific transfer function to be used in that? \nWill it still remain \"unset\" ?\n\n> What do you think?\n>\n> Thanks!\n> David\n>\n>>> Thanks!\n>>> David\n>>>\n>>> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel\n>>> wrote:\n>>>> Add a Colorspace::Default which corresponds to default V4L2\n>>>> colorspace identifiers.\n>>>>\n>>>> \\todo Add defaults to python colorspace bindings\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> include/libcamera/color_space.h | 5 +++++\n>>>> src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-\n>>>> src/libcamera/v4l2_device.cpp | 5 +++++\n>>>> 3 files changed, 38 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n>>>> index 086c56c1..0ad8da17 100644\n>>>> --- a/include/libcamera/color_space.h\n>>>> +++ b/include/libcamera/color_space.h\n>>>> @@ -16,6 +16,7 @@ class ColorSpace\n>>>> {\n>>>> public:\n>>>> enum class Primaries {\n>>>> + Default,\n>>>> Raw,\n>>>> Smpte170m,\n>>>> Rec709,\n>>>> @@ -23,12 +24,14 @@ public:\n>>>> };\n>>>>\n>>>> enum class TransferFunction {\n>>>> + Default,\n>>>> Linear,\n>>>> Srgb,\n>>>> Rec709,\n>>>> };\n>>>>\n>>>> enum class YcbcrEncoding {\n>>>> + Default,\n>>>> None,\n>>>> Rec601,\n>>>> Rec709,\n>>>> @@ -36,6 +39,7 @@ public:\n>>>> };\n>>>>\n>>>> enum class Range {\n>>>> + Default,\n>>>> Full,\n>>>> Limited,\n>>>> };\n>>>> @@ -45,6 +49,7 @@ public:\n>>>> {\n>>>> }\n>>>>\n>>>> + static const ColorSpace Default;\n>>>> static const ColorSpace Raw;\n>>>> static const ColorSpace Jpeg;\n>>>> static const ColorSpace Srgb;\n>>>> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n>>>> index 895e5c8e..d52f58cf 100644\n>>>> --- a/src/libcamera/color_space.cpp\n>>>> +++ b/src/libcamera/color_space.cpp\n>>>> @@ -50,6 +50,9 @@ namespace libcamera {\n>>>> * \\enum ColorSpace::Primaries\n>>>> * \\brief The color primaries for this color space\n>>>> *\n>>>> + * \\var ColorSpace::Primaries::Default\n>>>> + * \\brief Use the default primaries as defined by the driver\n>>>> + *\n>>>> * \\var ColorSpace::Primaries::Raw\n>>>> * \\brief These are raw colors directly from a sensor, the primaries are\n>>>> * unspecified\n>>>> @@ -68,6 +71,9 @@ namespace libcamera {\n>>>> * \\enum ColorSpace::TransferFunction\n>>>> * \\brief The transfer function used for this color space\n>>>> *\n>>>> + * \\var ColorSpace::TransferFunction::Default\n>>>> + * \\brief Use the default transfer function as defined by the driver\n>>>> + *\n>>>> * \\var ColorSpace::TransferFunction::Linear\n>>>> * \\brief This color space uses a linear (identity) transfer function\n>>>> *\n>>>> @@ -82,6 +88,9 @@ namespace libcamera {\n>>>> * \\enum ColorSpace::YcbcrEncoding\n>>>> * \\brief The Y'CbCr encoding\n>>>> *\n>>>> + * \\var ColorSpace::YcbcrEncoding::Default\n>>>> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n>>>> + *\n>>>> * \\var ColorSpace::YcbcrEncoding::None\n>>>> * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n>>>> *\n>>>> @@ -99,6 +108,9 @@ namespace libcamera {\n>>>> * \\enum ColorSpace::Range\n>>>> * \\brief The range (sometimes \"quantisation\") for this color space\n>>>> *\n>>>> + * \\var ColorSpace::Range::Default\n>>>> + * Use the default range as defined by the driver\n>>>> + *\n>>>> * \\var ColorSpace::Range::Full\n>>>> * \\brief This color space uses full range pixel values\n>>>> *\n>>>> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n>>>> {\n>>>> /* Print out a brief name only for standard color spaces. */\n>>>>\n>>>> - static const std::array, 6> colorSpaceNames = { {\n>>>> + static const std::array, 7> colorSpaceNames = { {\n>>>> + { ColorSpace::Default, \"Default\" },\n>>>> { ColorSpace::Raw, \"RAW\" },\n>>>> { ColorSpace::Jpeg, \"JPEG\" },\n>>>> { ColorSpace::Srgb, \"sRGB\" },\n>>>> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n>>>> /* Assemble a name made of the constituent fields. */\n>>>>\n>>>> static const std::map primariesNames = {\n>>>> + { Primaries::Default, \"Default\" },\n>>>> { Primaries::Raw, \"RAW\" },\n>>>> { Primaries::Smpte170m, \"SMPTE170M\" },\n>>>> { Primaries::Rec709, \"Rec709\" },\n>>>> { Primaries::Rec2020, \"Rec2020\" },\n>>>> };\n>>>> static const std::map transferNames = {\n>>>> + { TransferFunction::Default, \"Default\" },\n>>>> { TransferFunction::Linear, \"Linear\" },\n>>>> { TransferFunction::Srgb, \"sRGB\" },\n>>>> { TransferFunction::Rec709, \"Rec709\" },\n>>>> };\n>>>> static const std::map encodingNames = {\n>>>> + { YcbcrEncoding::Default, \"Default\" },\n>>>> { YcbcrEncoding::None, \"None\" },\n>>>> { YcbcrEncoding::Rec601, \"Rec601\" },\n>>>> { YcbcrEncoding::Rec709, \"Rec709\" },\n>>>> { YcbcrEncoding::Rec2020, \"Rec2020\" },\n>>>> };\n>>>> static const std::map rangeNames = {\n>>>> + { Range::Default, \"Default\" },\n>>>> { Range::Full, \"Full\" },\n>>>> { Range::Limited, \"Limited\" },\n>>>> };\n>>>> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n>>>> * \\brief The pixel range used with by color space\n>>>> */\n>>>>\n>>>> +/**\n>>>> + * \\brief A constant representing a default color space\n>>>> + */\n>>>> +const ColorSpace ColorSpace::Default = {\n>>>> + Primaries::Default,\n>>>> + TransferFunction::Default,\n>>>> + YcbcrEncoding::Default,\n>>>> + Range::Default\n>>>> +};\n>>>> +\n>>>> /**\n>>>> * \\brief A constant representing a raw color space (from a sensor)\n>>>> */\n>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>>> index 3fc8438f..ecfcf337 100644\n>>>> --- a/src/libcamera/v4l2_device.cpp\n>>>> +++ b/src/libcamera/v4l2_device.cpp\n>>>> @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n>>>> };\n>>>>\n>>>> static const std::vector> colorSpaceToV4l2 = {\n>>>> + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n>>>> { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n>>>> { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n>>>> { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n>>>> @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n>>>> };\n>>>>\n>>>> static const std::map primariesToV4l2 = {\n>>>> + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n>>>> { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n>>>> { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n>>>> { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n>>>> @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n>>>> };\n>>>>\n>>>> static const std::map transferFunctionToV4l2 = {\n>>>> + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n>>>> { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n>>>> { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n>>>> { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n>>>> };\n>>>>\n>>>> static const std::map ycbcrEncodingToV4l2 = {\n>>>> + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n>>>> { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n>>>> { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n>>>> { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n>>>> };\n>>>>\n>>>> static const std::map rangeToV4l2 = {\n>>>> + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n>>>> { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n>>>> { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\n>>>> };\n>>>> --\n>>>> 2.31.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 E7C6FC3275\n\tfor ;\n\tTue, 26 Jul 2022 12:12:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24B6363312;\n\tTue, 26 Jul 2022 14:12:17 +0200 (CEST)","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 9E01160487\n\tfor ;\n\tTue, 26 Jul 2022 14:12:15 +0200 (CEST)","from [IPV6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d] (unknown\n\t[IPv6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF3CA735;\n\tTue, 26 Jul 2022 14:12:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658837537;\n\tbh=RClsyyK7k9UlsI58g2421RlrPKgLL0RX6CqGToXLAWo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ewn0Rsbhe8d1y4hLkwZ9rEjqg8De0dUt8S3h6F3Y82+AlP4dE2wNI1jnOXZdQRDjC\n\t4sRF/9f6FtkUQ2gDUuUf9imnwHbJaij19eQ+psgMxaa05IZXPZAJOc0kpaWbDAnWBe\n\t3zoutS4uhlEtXuVisp/EZPh3al0NrwRGmcakCpjCodSDbmKArs7mwTxwhm5D1+Hce4\n\t1GnzpTpT/mWd/U6FC8t74tztToLPbhnI54TiE43UKQ1+xLRKQgnwAoPVTC+lSzEmZ2\n\t4H5z3eW/61e6mLoYCRcTPLYKIjLjySqSN9qzzx7H2eC3H4Niif3yQneDTi/+RWrkNx\n\tVhodvkEgmiG7Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658837535;\n\tbh=RClsyyK7k9UlsI58g2421RlrPKgLL0RX6CqGToXLAWo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=lznf6qY8nPMxpfq/i6LdEBO29YkOxs1IZDPnZ1w5yKSVC4zXu4nEwVaRl8AURqmoU\n\t6rM/3vNRwDYjH7OdlnpS51BhRno47NT1ogXtqHR5/w25O4ZznoKqxDPttHNMDRFVih\n\thBHC51R6fSntmwB5cHreIeWcUssmgr5kG8XfsEhM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lznf6qY8\"; dkim-atps=neutral","Message-ID":"<72bd1d6c-21f2-61df-ff17-5a9f2447c6ee@ideasonboard.com>","Date":"Tue, 26 Jul 2022 17:42:08 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"David Plowman ","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24150,"web_url":"https://patchwork.libcamera.org/comment/24150/","msgid":"","date":"2022-07-26T15:47:01","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 7/26/22 17:42, Umang Jain via libcamera-devel wrote:\n> Hi David,\n>\n> On 7/26/22 15:42, David Plowman wrote:\n>> Hi Umang, everyone\n>>\n>> Thanks for the reply. Perhaps I can add some more thoughts about this \n>> question.\n>\n>\n> Thank you for your thoughts.\n>\n>>\n>> On Mon, 25 Jul 2022 at 12:15, Umang Jain \n>> wrote:\n>>> Hi David,\n>>>\n>>> On 7/25/22 16:16, David Plowman wrote:\n>>>> Hi Umang\n>>>>\n>>>> Thanks for this patch. I was wondering if I could ask for a little\n>>>> more information on how it would be used.\n>>>\n>>> Yes, so the idea here is to provide a colorspace where one(or more)\n>>> identifiers are specified by the user whereas others can be left upto\n>>> the driver to fill in.\n>>>\n>>> I wasn't aware of this use case until very recently. In gstreamer, it\n>>> seems possible that the user wants to use(enforce?) a particular\n>>> transfer function whereas other identifier are left upto the driver \n>>> to fill.\n>>>\n>>> I am not very sure how would this be helpful (since colorspace\n>>> identifiers depend on each other I think), and mis-matches between them\n>>> might not make any sense at all.\n>>>\n>>> But such a use case as has been identified it seems, atleast in the\n>>> gstreamer case - so to satisfy that, I went ahead with this approach. I\n>>> couldn't get a better solution where the user specifies a set of\n>>> identifiers to use for colorspace, at the same time depending on the\n>>> driver to fill unknown or default values for remaining identifier(s).\n>>>\n>>>> In the original libcamera ColorSpace implementation I was very keen to\n>>>> avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n>>>> the view that applications really should say what ColorSpace they\n>>>> want, but at the same time I knew there will always be (bad)\n>>>> applications that don't think about colour spaces and so I let them\n>>>> use std::optional to allow the ColorSpace to be completely\n>>>> unspecified.\n>>>\n>>> Right, so when colorspace is completely unspecified, all values are\n>>> default-ed (V4L2Device::fromColorSpace()) in the v4l2_format.\n>>>\n>>> If I am asked about what's the goal of this patch, I would say that it\n>>> enables the use case where a sub-set of colorspace identifiers are\n>>> known, while others need to use defaults. Currently there is no default\n>>> std::optional hence I added one, to facilitate\n>>> the application layer. The application should ideally copy the\n>>> libcamera::ColorSpace::Default as base, swap the identifiers it \n>>> wants to\n>>> use in the copy, and send the custom colorspace for validation.\n>>>\n>>> Also note, the mapping is only one way (from libcamera to V4L2)\n>>> intentionally. It shouldn't \n>>> https://github.com/raspberrypi/picamera2#installationbe the case \n>>> where driver itself returns\n>>> \"::Default\" back to user.\n>>>\n>>>> In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n>>>> infer the value from the overall V4L2_COLORSPACE (someone please\n>>>> correct me if that's wrong!). In libcamera we don't have an \"overall\n>>>\n>>> If we could infer all the identifier values from\n>>> V4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:\n>>>\n>>> V4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT,\n>>> V4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.\n>>>\n>>>> colour space\" in this way, so what would it then mean? For example, is\n>>>> \"ColorSpace::Range::Default\" ever the same as\n>>>> \"ColorSpace::Range::Full\", or is it always different? How do we (can\n>>>> we even) document what it means?\n>>>\n>>> Maybe we should look at V4L2 level first, before libcamera for e.g.\n>>>\n>>> Would V4L2_QUANTIZATION_DEFAULT be ever same as\n>>> V4L2_QUANTIZATION_FULL_RANGE ?\n>>>\n>>>> I'd be interested to hear what other folks think. Possibly I'm being a\n>>>\n>>> I am all ears here as well, so let' see\n>>>\n>>>> bit paranoid, but then I have been bitten by \"wrong colour space\"\n>>>> problems often enough in the past (usually just before a product\n>>>> ships!).\n>>> ouch :-S\n>> My overall feeling is that I don't like having \"default\" things\n>> because I'm worried that they'll start appearing when you don't expect\n>> them. And then how would you know what they meant? As you know by now,\n>> I'm a bit paranoid about colour spaces going wrong...\n>>\n>> However, I see the point about these values possibly being convenient\n>> for negotiating colour spaces with (for example) gstreamer. But\n>> perhaps we could reduce the risk of them spreading, maybe we might\n>> consider:\n>>\n>> * Documenting clearly that \"default\" values are only \"inputs\" (what\n>> you want), and are never returned (what you got).\n>\n>\n> I've already made it impossible to return \"default\" values in this \n> patch. That's the reasoning behind you see the mappings going from \n> libcamera to V4L2 /only/ and not other way around.\n>\n> Yes we can clarify the document on that front.\n>\n>> * Adding a valid() method that complains if a ColorSpace has any\n>> undefined/default values in it?\n>> * When a function returns a ColorSpace that has been set, can we make\n>> them check that the returned value is \"valid()\"?\n>\n>\n> The driver always return a colorspace which is supported right? I \n> believe a supported colorspace would not _DEFAULT identifier.\n\n\nTo answer this, I have been clarified on #linux-media that a driver can \nreport _DEFAULT identifiers  back to the userspace.\n\nBut libcamera doesn't map to _DEFAULT identifiers in \nV4L2Device::toColorSpace() either, so we should be good here? Or still \nwe need additional valid()?\n\n>\n> valid() on the returned colorspace means we don't trust the driver? It \n> might have set one of the fields as _DEFAULT ?\n>\n> I am not sure what valid() will try to achieve here. Provided there is \n> already handled case where any the mappings is missing in \n> libcamera(colorspace will be std::nullopt) [1]\n>\n> [1] \n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n816\n>\n>> * I also wasn't quite sure why a ColorSpace::Default is needed, would\n>\n>\n> Convenience\n>\n>> a ColorSpace be regarded as being \"default\" if every field is\n>> \"default\", or just some of them? Or could an unset std::optional be\n>> used instead?\n>\n>\n> If some fields are specific values than the \"default\" identifiers, \n> it's not a default colorspace. A default colorspace is when \"all\" \n> fields are default and is equal to libcamera::ColorSpace::Default.\n>\n> A std::optional is initialised with std::nullopt.  How would I set a \n> colorspace where I want a specific transfer function to be used in \n> that? Will it still remain \"unset\" ?\n>\n>> What do you think?\n>>\n>> Thanks!\n>> David\n>>\n>>>> Thanks!\n>>>> David\n>>>>\n>>>> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel\n>>>> wrote:\n>>>>> Add a Colorspace::Default which corresponds to default V4L2\n>>>>> colorspace identifiers.\n>>>>>\n>>>>> \\todo Add defaults to python colorspace bindings\n>>>>>\n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>>    include/libcamera/color_space.h |  5 +++++\n>>>>>    src/libcamera/color_space.cpp   | 29 ++++++++++++++++++++++++++++-\n>>>>>    src/libcamera/v4l2_device.cpp   |  5 +++++\n>>>>>    3 files changed, 38 insertions(+), 1 deletion(-)\n>>>>>\n>>>>> diff --git a/include/libcamera/color_space.h \n>>>>> b/include/libcamera/color_space.h\n>>>>> index 086c56c1..0ad8da17 100644\n>>>>> --- a/include/libcamera/color_space.h\n>>>>> +++ b/include/libcamera/color_space.h\n>>>>> @@ -16,6 +16,7 @@ class ColorSpace\n>>>>>    {\n>>>>>    public:\n>>>>>           enum class Primaries {\n>>>>> +               Default,\n>>>>>                   Raw,\n>>>>>                   Smpte170m,\n>>>>>                   Rec709,\n>>>>> @@ -23,12 +24,14 @@ public:\n>>>>>           };\n>>>>>\n>>>>>           enum class TransferFunction {\n>>>>> +               Default,\n>>>>>                   Linear,\n>>>>>                   Srgb,\n>>>>>                   Rec709,\n>>>>>           };\n>>>>>\n>>>>>           enum class YcbcrEncoding {\n>>>>> +               Default,\n>>>>>                   None,\n>>>>>                   Rec601,\n>>>>>                   Rec709,\n>>>>> @@ -36,6 +39,7 @@ public:\n>>>>>           };\n>>>>>\n>>>>>           enum class Range {\n>>>>> +               Default,\n>>>>>                   Full,\n>>>>>                   Limited,\n>>>>>           };\n>>>>> @@ -45,6 +49,7 @@ public:\n>>>>>           {\n>>>>>           }\n>>>>>\n>>>>> +       static const ColorSpace Default;\n>>>>>           static const ColorSpace Raw;\n>>>>>           static const ColorSpace Jpeg;\n>>>>>           static const ColorSpace Srgb;\n>>>>> diff --git a/src/libcamera/color_space.cpp \n>>>>> b/src/libcamera/color_space.cpp\n>>>>> index 895e5c8e..d52f58cf 100644\n>>>>> --- a/src/libcamera/color_space.cpp\n>>>>> +++ b/src/libcamera/color_space.cpp\n>>>>> @@ -50,6 +50,9 @@ namespace libcamera {\n>>>>>     * \\enum ColorSpace::Primaries\n>>>>>     * \\brief The color primaries for this color space\n>>>>>     *\n>>>>> + * \\var ColorSpace::Primaries::Default\n>>>>> + * \\brief Use the default primaries as defined by the driver\n>>>>> + *\n>>>>>     * \\var ColorSpace::Primaries::Raw\n>>>>>     * \\brief These are raw colors directly from a sensor, the \n>>>>> primaries are\n>>>>>     * unspecified\n>>>>> @@ -68,6 +71,9 @@ namespace libcamera {\n>>>>>     * \\enum ColorSpace::TransferFunction\n>>>>>     * \\brief The transfer function used for this color space\n>>>>>     *\n>>>>> + * \\var ColorSpace::TransferFunction::Default\n>>>>> + * \\brief Use the default transfer function as defined by the driver\n>>>>> + *\n>>>>>     * \\var ColorSpace::TransferFunction::Linear\n>>>>>     * \\brief This color space uses a linear (identity) transfer \n>>>>> function\n>>>>>     *\n>>>>> @@ -82,6 +88,9 @@ namespace libcamera {\n>>>>>     * \\enum ColorSpace::YcbcrEncoding\n>>>>>     * \\brief The Y'CbCr encoding\n>>>>>     *\n>>>>> + * \\var ColorSpace::YcbcrEncoding::Default\n>>>>> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n>>>>> + *\n>>>>>     * \\var ColorSpace::YcbcrEncoding::None\n>>>>>     * \\brief There is no defined Y'CbCr encoding (used for non-YUV \n>>>>> formats)\n>>>>>     *\n>>>>> @@ -99,6 +108,9 @@ namespace libcamera {\n>>>>>     * \\enum ColorSpace::Range\n>>>>>     * \\brief The range (sometimes \"quantisation\") for this color \n>>>>> space\n>>>>>     *\n>>>>> + * \\var ColorSpace::Range::Default\n>>>>> + * Use the default range as defined by the driver\n>>>>> + *\n>>>>>     * \\var ColorSpace::Range::Full\n>>>>>     * \\brief This color space uses full range pixel values\n>>>>>     *\n>>>>> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n>>>>>    {\n>>>>>           /* Print out a brief name only for standard color \n>>>>> spaces. */\n>>>>>\n>>>>> -       static const std::array>>>> *>, 6> colorSpaceNames = { {\n>>>>> +       static const std::array>>>> *>, 7> colorSpaceNames = { {\n>>>>> +               { ColorSpace::Default, \"Default\" },\n>>>>>                   { ColorSpace::Raw, \"RAW\" },\n>>>>>                   { ColorSpace::Jpeg, \"JPEG\" },\n>>>>>                   { ColorSpace::Srgb, \"sRGB\" },\n>>>>> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n>>>>>           /* Assemble a name made of the constituent fields. */\n>>>>>\n>>>>>           static const std::map \n>>>>> primariesNames = {\n>>>>> +               { Primaries::Default, \"Default\" },\n>>>>>                   { Primaries::Raw, \"RAW\" },\n>>>>>                   { Primaries::Smpte170m, \"SMPTE170M\" },\n>>>>>                   { Primaries::Rec709, \"Rec709\" },\n>>>>>                   { Primaries::Rec2020, \"Rec2020\" },\n>>>>>           };\n>>>>>           static const std::map \n>>>>> transferNames = {\n>>>>> +               { TransferFunction::Default, \"Default\" },\n>>>>>                   { TransferFunction::Linear, \"Linear\" },\n>>>>>                   { TransferFunction::Srgb, \"sRGB\" },\n>>>>>                   { TransferFunction::Rec709, \"Rec709\" },\n>>>>>           };\n>>>>>           static const std::map \n>>>>> encodingNames = {\n>>>>> +               { YcbcrEncoding::Default, \"Default\" },\n>>>>>                   { YcbcrEncoding::None, \"None\" },\n>>>>>                   { YcbcrEncoding::Rec601, \"Rec601\" },\n>>>>>                   { YcbcrEncoding::Rec709, \"Rec709\" },\n>>>>>                   { YcbcrEncoding::Rec2020, \"Rec2020\" },\n>>>>>           };\n>>>>>           static const std::map rangeNames = {\n>>>>> +               { Range::Default, \"Default\" },\n>>>>>                   { Range::Full, \"Full\" },\n>>>>>                   { Range::Limited, \"Limited\" },\n>>>>>           };\n>>>>> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const \n>>>>> std::optional &colorSpace)\n>>>>>     * \\brief The pixel range used with by color space\n>>>>>     */\n>>>>>\n>>>>> +/**\n>>>>> + * \\brief A constant representing a default color space\n>>>>> + */\n>>>>> +const ColorSpace ColorSpace::Default = {\n>>>>> +       Primaries::Default,\n>>>>> +       TransferFunction::Default,\n>>>>> +       YcbcrEncoding::Default,\n>>>>> +       Range::Default\n>>>>> +};\n>>>>> +\n>>>>>    /**\n>>>>>     * \\brief A constant representing a raw color space (from a \n>>>>> sensor)\n>>>>>     */\n>>>>> diff --git a/src/libcamera/v4l2_device.cpp \n>>>>> b/src/libcamera/v4l2_device.cpp\n>>>>> index 3fc8438f..ecfcf337 100644\n>>>>> --- a/src/libcamera/v4l2_device.cpp\n>>>>> +++ b/src/libcamera/v4l2_device.cpp\n>>>>> @@ -770,6 +770,7 @@ static const std::map>>>> ColorSpace::Range> v4l2ToRange = {\n>>>>>    };\n>>>>>\n>>>>>    static const std::vector>>>> v4l2_colorspace>> colorSpaceToV4l2 = {\n>>>>> +       { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n>>>>>           { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n>>>>>           { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n>>>>>           { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n>>>>> @@ -779,6 +780,7 @@ static const std::vector>>>> v4l2_colorspace>> colorSpaceToV4l\n>>>>>    };\n>>>>>\n>>>>>    static const std::map \n>>>>> primariesToV4l2 = {\n>>>>> +       { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n>>>>>           { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n>>>>>           { ColorSpace::Primaries::Smpte170m, \n>>>>> V4L2_COLORSPACE_SMPTE170M },\n>>>>>           { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n>>>>> @@ -786,18 +788,21 @@ static const std::map>>>> v4l2_colorspace> primariesToV4l2 =\n>>>>>    };\n>>>>>\n>>>>>    static const std::map>>>> v4l2_xfer_func> transferFunctionToV4l2 = {\n>>>>> +       { ColorSpace::TransferFunction::Default, \n>>>>> V4L2_XFER_FUNC_DEFAULT },\n>>>>>           { ColorSpace::TransferFunction::Linear, \n>>>>> V4L2_XFER_FUNC_NONE },\n>>>>>           { ColorSpace::TransferFunction::Srgb, \n>>>>> V4L2_XFER_FUNC_SRGB },\n>>>>>           { ColorSpace::TransferFunction::Rec709, \n>>>>> V4L2_XFER_FUNC_709 },\n>>>>>    };\n>>>>>\n>>>>>    static const std::map>>>> v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = {\n>>>>> +       { ColorSpace::YcbcrEncoding::Default, \n>>>>> V4L2_YCBCR_ENC_DEFAULT },\n>>>>>           { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n>>>>>           { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n>>>>>           { ColorSpace::YcbcrEncoding::Rec2020, \n>>>>> V4L2_YCBCR_ENC_BT2020 },\n>>>>>    };\n>>>>>\n>>>>>    static const std::map \n>>>>> rangeToV4l2 = {\n>>>>> +       { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n>>>>>           { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n>>>>>           { ColorSpace::Range::Limited, \n>>>>> V4L2_QUANTIZATION_LIM_RANGE },\n>>>>>    };\n>>>>> -- \n>>>>> 2.31.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 A100EBE173\n\tfor ;\n\tTue, 26 Jul 2022 15:47:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF2E263312;\n\tTue, 26 Jul 2022 17:47:09 +0200 (CEST)","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 2426460487\n\tfor ;\n\tTue, 26 Jul 2022 17:47:09 +0200 (CEST)","from [IPV6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d] (unknown\n\t[IPv6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C210F735;\n\tTue, 26 Jul 2022 17:47:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658850429;\n\tbh=zhtr2o5s1zVgfA/qOGvU7bwvDdpm5RyZY8qKorlM6Iw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Mdtouq36LGp2KSQCUOIOmOUk9Io3ZRB9T5iW1JyBR0keJX85XvyT/fFB3Z46TA1vp\n\tN3YMUf1RZQ6wjwwKD55h0rZpAw1RZTuaXyKFGhjYZrbAKFfmW0G790+rwEBS2PkC4s\n\tAvKrJ0Jc6Yn7A0uh4vKYcVLgBRliEZ4H26X+HYQtjod53iajpzXQ0AcgsLr62doQQd\n\tJ8cW6mWnkACTAnxus8mcCDTco9MyknOpnvFM0Ck5/X2wpzRoF8PRCvvEMjFIAmoTwe\n\tTcWPjSz6fWNO8sD3gB47lIbvOrlBDsWIaYDGmNnamiBYnphNVkP4Rhhxr4NsnUy5d9\n\tBhYj8pfgkt38w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658850428;\n\tbh=zhtr2o5s1zVgfA/qOGvU7bwvDdpm5RyZY8qKorlM6Iw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=STGbuMmP0Oo6GHKPGOHeJVAHb6Iw9MytqZ9JOWuXSt8OSvU2n3fSBWmeJ/d4D1f/A\n\t+7wJZ9PxwqNhuzA1H+Q9jXAtUxUQGQUNY3s8yIPBipwfMS0j2VQ7UGUyvMw0vQzcr/\n\t18AYXJ5ZHcAQZV9nPaYos/aiuXC3wD9U9nBU0J7A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"STGbuMmP\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 26 Jul 2022 21:17:01 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"David Plowman ","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>\n\t\n\t<72bd1d6c-21f2-61df-ff17-5a9f2447c6ee@ideasonboard.com>","In-Reply-To":"<72bd1d6c-21f2-61df-ff17-5a9f2447c6ee@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tvedantparanjape160201@gmail.com, nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24162,"web_url":"https://patchwork.libcamera.org/comment/24162/","msgid":"","date":"2022-07-26T22:40:31","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Jul 25, 2022 at 04:40:44PM +0100, Naushir Patuck via libcamera-devel wrote:\n> On Mon, 25 Jul 2022 at 13:22, Nicolas Dufresne via libcamera-devel wrote:\n> > Le lundi 25 juillet 2022 à 11:46 +0100, David Plowman a écrit :\n> > > Hi Umang\n> > >\n> > > Thanks for this patch. I was wondering if I could ask for a little\n> > > more information on how it would be used.\n> > >\n> > > In the original libcamera ColorSpace implementation I was very keen to\n> > > avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n> > > the view that applications really should say what ColorSpace they\n> > > want, but at the same time I knew there will always be (bad)\n> > > applications that don't think about colour spaces and so I let them\n> > > use std::optional to allow the ColorSpace to be completely\n> > > unspecified.\n> >\n> > \"wrong\" application is a bit of an over statement. Even for cameras, the\n> > colorspace is not always something that can be freely picked by the application.\n> > The V4L2 driver, or the HW may have some limitations. A simple example is UVC,\n> > you pretty much get what you get and you can't influence it. For this reason, a\n> > proper colorspace API needs to work both ways. Which basically imply, an\n> > application should be able to not care about selecting / influencing the\n> > colorspace and just read whatever the driver/HW produces.\n> \n> Wouldn't the pipeline_handler::validate() take care of this? It ought to know the\n> sensor output colourspace, and return that regardless of what the application\n> may have requested. Apologies if I got this wrong.\n\nYes, the validate() function should handle that and expose the\ncolorspace that will be produced, if no colorspace has been selected or\nif the selected colorspace isn't supported.\n\nI quite side with David here in the sense that I think it's wrong for\napplications to not care about colorspace, but Nicolas is absolutely\nright that we often can't select a desired colorspace with a 100%\nguarantee it will be produced. I'd like to force applications to think\nabout it. Conceptually speaking, it would be totally fine to just let\nlibcamera pick a colorspace and then propagate it to the consumer(s)\n(and validate that said consumers can accept that colorspace), but we\ncan't in libcamera check what the application does with the colorspace\nwe report.\n\nForcing an application to select a colorspace explicitly (removing the\nstd::optional) would be a different way to try and get the developers to\nthink about what they're doing, but I don't want to do that until we\ngive a way to applications to enumerate supported colorspaces. Even\nthen, I'm not sure it would be the right thing to do.\n\n> > > In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n> > > infer the value from the overall V4L2_COLORSPACE (someone please\n> > > correct me if that's wrong!). In libcamera we don't have an \"overall\n> > > colour space\" in this way, so what would it then mean? For example, is\n> > > \"ColorSpace::Range::Default\" ever the same as\n> > > \"ColorSpace::Range::Full\", or is it always different? How do we (can\n> > > we even) document what it means?\n> > >\n> > > I'd be interested to hear what other folks think. Possibly I'm being a\n> > > bit paranoid, but then I have been bitten by \"wrong colour space\"\n> > > problems often enough in the past (usually just before a product\n> > > ships!).\n\nI'm also interested in answers to the above questions :-) Maybe they're\nin the rest of the mail thread, I'll read that now.\n\n> > > On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel wrote:\n> > > >\n> > > > Add a Colorspace::Default which corresponds to default V4L2\n> > > > colorspace identifiers.\n> > > >\n> > > > \\todo Add defaults to python colorspace bindings\n> > > >\n> > > > Signed-off-by: Umang Jain \n> > > > ---\n> > > > include/libcamera/color_space.h | 5 +++++\n> > > > src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-\n> > > > src/libcamera/v4l2_device.cpp | 5 +++++\n> > > > 3 files changed, 38 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> > > > index 086c56c1..0ad8da17 100644\n> > > > --- a/include/libcamera/color_space.h\n> > > > +++ b/include/libcamera/color_space.h\n> > > > @@ -16,6 +16,7 @@ class ColorSpace\n> > > > {\n> > > > public:\n> > > > enum class Primaries {\n> > > > + Default,\n> > > > Raw,\n> > > > Smpte170m,\n> > > > Rec709,\n> > > > @@ -23,12 +24,14 @@ public:\n> > > > };\n> > > >\n> > > > enum class TransferFunction {\n> > > > + Default,\n> > > > Linear,\n> > > > Srgb,\n> > > > Rec709,\n> > > > };\n> > > >\n> > > > enum class YcbcrEncoding {\n> > > > + Default,\n> > > > None,\n> > > > Rec601,\n> > > > Rec709,\n> > > > @@ -36,6 +39,7 @@ public:\n> > > > };\n> > > >\n> > > > enum class Range {\n> > > > + Default,\n> > > > Full,\n> > > > Limited,\n> > > > };\n> > > > @@ -45,6 +49,7 @@ public:\n> > > > {\n> > > > }\n> > > >\n> > > > + static const ColorSpace Default;\n> > > > static const ColorSpace Raw;\n> > > > static const ColorSpace Jpeg;\n> > > > static const ColorSpace Srgb;\n> > > > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> > > > index 895e5c8e..d52f58cf 100644\n> > > > --- a/src/libcamera/color_space.cpp\n> > > > +++ b/src/libcamera/color_space.cpp\n> > > > @@ -50,6 +50,9 @@ namespace libcamera {\n> > > > * \\enum ColorSpace::Primaries\n> > > > * \\brief The color primaries for this color space\n> > > > *\n> > > > + * \\var ColorSpace::Primaries::Default\n> > > > + * \\brief Use the default primaries as defined by the driver\n> > > > + *\n> > > > * \\var ColorSpace::Primaries::Raw\n> > > > * \\brief These are raw colors directly from a sensor, the primaries are\n> > > > * unspecified\n> > > > @@ -68,6 +71,9 @@ namespace libcamera {\n> > > > * \\enum ColorSpace::TransferFunction\n> > > > * \\brief The transfer function used for this color space\n> > > > *\n> > > > + * \\var ColorSpace::TransferFunction::Default\n> > > > + * \\brief Use the default transfer function as defined by the driver\n> > > > + *\n> > > > * \\var ColorSpace::TransferFunction::Linear\n> > > > * \\brief This color space uses a linear (identity) transfer function\n> > > > *\n> > > > @@ -82,6 +88,9 @@ namespace libcamera {\n> > > > * \\enum ColorSpace::YcbcrEncoding\n> > > > * \\brief The Y'CbCr encoding\n> > > > *\n> > > > + * \\var ColorSpace::YcbcrEncoding::Default\n> > > > + * \\brief Use the default Y'CbCr encoding as defined by the driver\n> > > > + *\n> > > > * \\var ColorSpace::YcbcrEncoding::None\n> > > > * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> > > > *\n> > > > @@ -99,6 +108,9 @@ namespace libcamera {\n> > > > * \\enum ColorSpace::Range\n> > > > * \\brief The range (sometimes \"quantisation\") for this color space\n> > > > *\n> > > > + * \\var ColorSpace::Range::Default\n> > > > + * Use the default range as defined by the driver\n> > > > + *\n> > > > * \\var ColorSpace::Range::Full\n> > > > * \\brief This color space uses full range pixel values\n> > > > *\n> > > > @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n> > > > {\n> > > > /* Print out a brief name only for standard color spaces. */\n> > > >\n> > > > - static const std::array, 6> colorSpaceNames = { {\n> > > > + static const std::array, 7> colorSpaceNames = { {\n> > > > + { ColorSpace::Default, \"Default\" },\n> > > > { ColorSpace::Raw, \"RAW\" },\n> > > > { ColorSpace::Jpeg, \"JPEG\" },\n> > > > { ColorSpace::Srgb, \"sRGB\" },\n> > > > @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n> > > > /* Assemble a name made of the constituent fields. */\n> > > >\n> > > > static const std::map primariesNames = {\n> > > > + { Primaries::Default, \"Default\" },\n> > > > { Primaries::Raw, \"RAW\" },\n> > > > { Primaries::Smpte170m, \"SMPTE170M\" },\n> > > > { Primaries::Rec709, \"Rec709\" },\n> > > > { Primaries::Rec2020, \"Rec2020\" },\n> > > > };\n> > > > static const std::map transferNames = {\n> > > > + { TransferFunction::Default, \"Default\" },\n> > > > { TransferFunction::Linear, \"Linear\" },\n> > > > { TransferFunction::Srgb, \"sRGB\" },\n> > > > { TransferFunction::Rec709, \"Rec709\" },\n> > > > };\n> > > > static const std::map encodingNames = {\n> > > > + { YcbcrEncoding::Default, \"Default\" },\n> > > > { YcbcrEncoding::None, \"None\" },\n> > > > { YcbcrEncoding::Rec601, \"Rec601\" },\n> > > > { YcbcrEncoding::Rec709, \"Rec709\" },\n> > > > { YcbcrEncoding::Rec2020, \"Rec2020\" },\n> > > > };\n> > > > static const std::map rangeNames = {\n> > > > + { Range::Default, \"Default\" },\n> > > > { Range::Full, \"Full\" },\n> > > > { Range::Limited, \"Limited\" },\n> > > > };\n> > > > @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n> > > > * \\brief The pixel range used with by color space\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief A constant representing a default color space\n> > > > + */\n> > > > +const ColorSpace ColorSpace::Default = {\n> > > > + Primaries::Default,\n> > > > + TransferFunction::Default,\n> > > > + YcbcrEncoding::Default,\n> > > > + Range::Default\n> > > > +};\n> > > > +\n> > > > /**\n> > > > * \\brief A constant representing a raw color space (from a sensor)\n> > > > */\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index 3fc8438f..ecfcf337 100644\n> > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n> > > > };\n> > > >\n> > > > static const std::vector> colorSpaceToV4l2 = {\n> > > > + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n> > > > { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> > > > { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> > > > { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> > > > @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n> > > > };\n> > > >\n> > > > static const std::map primariesToV4l2 = {\n> > > > + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n> > > > { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> > > > { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> > > > { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> > > > @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n> > > > };\n> > > >\n> > > > static const std::map transferFunctionToV4l2 = {\n> > > > + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n> > > > { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> > > > { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> > > > { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> > > > };\n> > > >\n> > > > static const std::map ycbcrEncodingToV4l2 = {\n> > > > + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n> > > > { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> > > > { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> > > > { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> > > > };\n> > > >\n> > > > static const std::map rangeToV4l2 = {\n> > > > + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n> > > > { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> > > > { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\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 8B5C6BE173\n\tfor ;\n\tTue, 26 Jul 2022 22:40:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1C8E63312;\n\tWed, 27 Jul 2022 00:40:34 +0200 (CEST)","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 AC3E7603E8\n\tfor ;\n\tWed, 27 Jul 2022 00:40:33 +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 1776B56D;\n\tWed, 27 Jul 2022 00:40:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658875235;\n\tbh=GiG4QvMfMkkOHbcugeuPP48s5SQ+bWVgbSX7Vqfd66g=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=cWjyp2jzwi/JJq7/k1gpH+ha7AWfKwrqUGkGj8I2r0d2Y4mdJlsG+wO32yt5U/aOd\n\tAKEqBD2NVUlXTKxacSjZmg3mfLcQraRMxRoQGz57CZB0/I1M1naleDwV2yxLMgj2CM\n\t6s1bm/jxAGyRZPBheXPflOgpySExtkXiYXjvmy196e1qOHiWsE1niRjV0HLayFJBxZ\n\tjUQNabsCU09v04lTgKyrsVPRivlgkaV+QOuUz7rJwYFoTJGQJvRnmHbuI5CmcYFtJ8\n\tSayinxirG1ABmfKt1x94hYF7N7kGelz/PxhXw/vXbnXvE3dZuqbYJ8+Y72k8r5MHNe\n\tBvhoeG15R/Q7A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658875233;\n\tbh=GiG4QvMfMkkOHbcugeuPP48s5SQ+bWVgbSX7Vqfd66g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=frCCvyPtX3ljszjchncJrBT6vk+sk5CLoUNQyc7qnhtSkP7hWIREkhDGdbWOCRD5o\n\tUiVWTEGZhxAQ+tUaXca0jKQSOC30fp/LqYMcYlPVTCIJlttTC/gjWPqW1sHOjXtIGd\n\tMMK0NxeydP24z2+tRPyu6Z5N+bOf9Knzoj1nHPN8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"frCCvyPt\"; dkim-atps=neutral","Date":"Wed, 27 Jul 2022 01:40:31 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<259ade47c99bde5d427f330d9ef973b4012c9788.camel@collabora.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"rishikeshdonadkar@gmail.com, vedantparanjape160201@gmail.com,\n\tNicolas Dufresne ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24163,"web_url":"https://patchwork.libcamera.org/comment/24163/","msgid":"","date":"2022-07-27T00:01:58","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\n(CC'ing Hans Verkuil)\n\nOn Tue, Jul 26, 2022 at 09:17:01PM +0530, Umang Jain via libcamera-devel wrote:\n> On 7/26/22 17:42, Umang Jain via libcamera-devel wrote:\n> > On 7/26/22 15:42, David Plowman wrote:\n> >> On Mon, 25 Jul 2022 at 12:15, Umang Jain wrote:\n> >>> On 7/25/22 16:16, David Plowman wrote:\n> >>>> Hi Umang\n> >>>>\n> >>>> Thanks for this patch. I was wondering if I could ask for a little\n> >>>> more information on how it would be used.\n> >>>\n> >>> Yes, so the idea here is to provide a colorspace where one(or more)\n> >>> identifiers are specified by the user whereas others can be left upto\n> >>> the driver to fill in.\n> >>>\n> >>> I wasn't aware of this use case until very recently. In gstreamer, it\n> >>> seems possible that the user wants to use(enforce?) a particular\n> >>> transfer function whereas other identifier are left upto the driver \n> >>> to fill.\n> >>>\n> >>> I am not very sure how would this be helpful (since colorspace\n> >>> identifiers depend on each other I think), and mis-matches between them\n> >>> might not make any sense at all.\n> >>>\n> >>> But such a use case as has been identified it seems, atleast in the\n> >>> gstreamer case - so to satisfy that, I went ahead with this approach. I\n> >>> couldn't get a better solution where the user specifies a set of\n> >>> identifiers to use for colorspace, at the same time depending on the\n> >>> driver to fill unknown or default values for remaining identifier(s).\n> >>>\n> >>>> In the original libcamera ColorSpace implementation I was very keen to\n> >>>> avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n> >>>> the view that applications really should say what ColorSpace they\n> >>>> want, but at the same time I knew there will always be (bad)\n> >>>> applications that don't think about colour spaces and so I let them\n> >>>> use std::optional to allow the ColorSpace to be completely\n> >>>> unspecified.\n> >>>\n> >>> Right, so when colorspace is completely unspecified, all values are\n> >>> default-ed (V4L2Device::fromColorSpace()) in the v4l2_format.\n\nRight, so when std::optional is std::nullopt, we set all the\ncolorspace-related fields in V4L2VideoDevice::setFormat() (and in the\nequivalent implementation for v4L2Subdevice) to *_DEFAULT. The V4L2\ndriver then has to return a non-DEFAULT .colorspace, and possibly\nnon-DEFAULT values for .xfer_func, .ycbcr_enc and .quantization. This\ndefines without any ambiguity the exact colorspace information, and we\ncan create a fully defined ColorSpace out of that (if any of the\n.xfer_func, .ycbcr_enc or .quantization fields are returned as DEFAULT,\nwe pick the default value that corresponds to the .colorspace, which\nV4L2 guarantee will never be returned as DEFAULT by drivers).\n\nSo far I don't see an issue.\n\n> >>> If I am asked about what's the goal of this patch, I would say that it\n> >>> enables the use case where a sub-set of colorspace identifiers are\n> >>> known, while others need to use defaults. Currently there is no default\n> >>> std::optional hence I added one, to facilitate\n> >>> the application layer. The application should ideally copy the\n> >>> libcamera::ColorSpace::Default as base, swap the identifiers it wants to\n> >>> use in the copy, and send the custom colorspace for validation.\n\nOur current API supports the following well-defined cases, where an\napplication can request\n\n- A well-defined colorspace (e.g. ColorSpace::Rec709), with everything\n it implies (the primaries, transfer function, YCbCr encoding and\n quantization range defined by the colorspace). For this, an\n application simply uses one of the presets.\n\n- A well-defined colorspace, with one or multiple fields overridden to\n different values (e.g. ColorSpace::Rec709 with a Full quantization\n range). For this, an application copies one of the presets and\n modifies one or multiple fields to well-defined values.\n\n- A completely custom colorspace (e.g. Rec709 primaries, Srgb transfer\n unction, Rec2020 YCbCr encoding and Full quantization range). For this\n an application can construct the colorspace manually.\n\nWhat we don't support today, and this is what I understand you want to\ndo with this patch, is for an application to tell libcamera \"give me any\ncolorspace, as long as the quantization is full range\". Is this right ?\n\nIf that's the use case you're after, would it be an issue if the\napplication used\n\n\tColorSpace cs = ColorSpace::Srgb;\n\tcs.range = ColorSpace::Range::Full;\n\n\tStreamConfiguration &cfg = ...;\n\tcfg.colorSpace = cs;\n\n? The first two lines will be equivalent to\n\n\tColorSpace cs(Primaries::Rec709, TransferFunction::Srgb,\n\t\t YcbcrEncoding::Rec601, Range::Full);\n\nwhich would then be translated to the V4L2 format\n\n\t.colorspace = V4L2_COLORSPACE_REC709;\n\t.xfer_func = V4L2_XFER_FUNC_SRGB;\n\t.ycbcr_enc = V4L2_YCBCR_ENC_601;\n\t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n\nHow a driver will handle this if this particular combination isn't\nsupported is unfortunately not clearly documented by V4L2, se we may\nwell get back\n\n\t.colorspace = V4L2_COLORSPACE_REC709;\n\t.xfer_func = V4L2_XFER_FUNC_709;\n\t.ycbcr_enc = V4L2_YCBCR_ENC_709;\n\t.quantization = V4L2_QUANTIZATION_LIM_RANGE;\n\neven if the driver would support\n\n\t.colorspace = V4L2_COLORSPACE_REC709;\n\t.xfer_func = V4L2_XFER_FUNC_709;\n\t.ycbcr_enc = V4L2_YCBCR_ENC_709;\n\t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n\nif asked directly, given that V4L2 doesn't describe how to adjust a\ncolorspace that isn't supported.\n\nRequesting\n\n\t.colorspace = V4L2_COLORSPACE_DEFAULT;\n\t.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n\t.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n\t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n\ncould possibly help, except that we have only two drivers that even try\nto honour the *_CSC flags, let alone implement correct colorspace\nbehaviour.\n\nOn a side note, I have a small feeling we may be trying to solve\nproblems that will never appear in practice.\n\n> >>> Also note, the mapping is only one way (from libcamera to V4L2)\n> >>> intentionally. It shouldn't \n> >>> https://github.com/raspberrypi/picamera2#installationbe the case \n> >>> where driver itself returns\n> >>> \"::Default\" back to user.\n> >>>\n> >>>> In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n> >>>> infer the value from the overall V4L2_COLORSPACE (someone please\n> >>>> correct me if that's wrong!). In libcamera we don't have an \"overall\n> >>>\n> >>> If we could infer all the identifier values from\n> >>> V4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:\n> >>>\n> >>> V4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT,\n> >>> V4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.\n> >>>\n> >>>> colour space\" in this way, so what would it then mean? For example, is\n> >>>> \"ColorSpace::Range::Default\" ever the same as\n> >>>> \"ColorSpace::Range::Full\", or is it always different? How do we (can\n> >>>> we even) document what it means?\n> >>>\n> >>> Maybe we should look at V4L2 level first, before libcamera for e.g.\n> >>>\n> >>> Would V4L2_QUANTIZATION_DEFAULT be ever same as\n> >>> V4L2_QUANTIZATION_FULL_RANGE ?\n\nWe need to make sure we can implement the behaviour defined in the\nlibcamera public API using the V4L2-based pipeline handlers, but the\nV4L2 API doesn't dictate how we should handle colorspaces. If anything,\nwe should influence V4L2 with real world use cases.\n\n> >>>> I'd be interested to hear what other folks think. Possibly I'm being a\n> >>>\n> >>> I am all ears here as well, so let' see\n> >>>\n> >>>> bit paranoid, but then I have been bitten by \"wrong colour space\"\n> >>>> problems often enough in the past (usually just before a product\n> >>>> ships!).\n> >>> \n> >>> ouch :-S\n> >> \n> >> My overall feeling is that I don't like having \"default\" things\n> >> because I'm worried that they'll start appearing when you don't expect\n> >> them. And then how would you know what they meant? As you know by now,\n> >> I'm a bit paranoid about colour spaces going wrong...\n> >>\n> >> However, I see the point about these values possibly being convenient\n> >> for negotiating colour spaces with (for example) gstreamer. But\n> >> perhaps we could reduce the risk of them spreading, maybe we might\n> >> consider:\n> >>\n> >> * Documenting clearly that \"default\" values are only \"inputs\" (what\n> >> you want), and are never returned (what you got).\n\nWe should indeed never return any of the colorspace fields set to\n\"Default\".\n\n> > I've already made it impossible to return \"default\" values in this \n> > patch. That's the reasoning behind you see the mappings going from \n> > libcamera to V4L2 /only/ and not other way around.\n> >\n> > Yes we can clarify the document on that front.\n> >\n> >> * Adding a valid() method that complains if a ColorSpace has any\n> >> undefined/default values in it?\n> >> * When a function returns a ColorSpace that has been set, can we make\n> >> them check that the returned value is \"valid()\"?\n> >\n> > The driver always return a colorspace which is supported right? I \n> > believe a supported colorspace would not _DEFAULT identifier.\n> \n> To answer this, I have been clarified on #linux-media that a driver can \n> report _DEFAULT identifiers  back to the userspace.\n\nOnly for .xfer_func, .ycbcr_enc and .quantization, never for\n.colorspace, so we can always map a V4L2 *_DEFAULT value that we get\nback from a driver to well-defined values for all fields.\n\n> But libcamera doesn't map to _DEFAULT identifiers in \n> V4L2Device::toColorSpace() either, so we should be good here? Or still \n> we need additional valid()?\n\nOn a side note, we have bugs in our colorspace handling. V4L2 considers\nthe colorspace fields as read-only from an application point of view,\nboth on video devices and on subdevs, unless the\nV4L2_PIX_FMT_FLAG_SET_CSC or V4L2_MBUS_FRAMEFMT_SET_CSC flags\n(respectively) are set when calling the S_FMT ioctl. Only vivid\nimplements this correctly for video nodes, and only the rkisp1 driver\nhandles the V4L2_MBUS_FRAMEFMT_SET_CSC on subdevs.\n\nFurthermore, how the *_DEFAULT values operate when setting them is not\nvery well defined, I've tried to clarify that in #linux-media on OFTC\ntoday, and I've been told by Hans that setting\n\n\t.flags = V4L2_MBUS_FRAMEFMT_SET_CSC;\n\t.colorspace = REC709;\n\t.xfer_func = DEFAULT;\n\t.ycbcr_enc = DEFAULT;\n\t.quantization = DEFAULT;\n\non a subdev source pad means that the application wants to override the\ncolor primaries and nothing else, while I interpret the documentation\n([1]) as saying that the application wants to set the .xfer_func,\n.ycbcr_enc and .quantization to the default values specified by REC709.\nDiscussions are still ongoing.\n\nhttps://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id1\n\n> > valid() on the returned colorspace means we don't trust the driver? It \n> > might have set one of the fields as _DEFAULT ?\n> >\n> > I am not sure what valid() will try to achieve here. Provided there is \n> > already handled case where any the mappings is missing in \n> > libcamera(colorspace will be std::nullopt) [1]\n> >\n> > [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n816\n> >\n> >> * I also wasn't quite sure why a ColorSpace::Default is needed, would\n> >\n> > Convenience\n> >\n> >> a ColorSpace be regarded as being \"default\" if every field is\n> >> \"default\", or just some of them? Or could an unset std::optional be\n> >> used instead?\n> >\n> > If some fields are specific values than the \"default\" identifiers, \n> > it's not a default colorspace. A default colorspace is when \"all\" \n> > fields are default and is equal to libcamera::ColorSpace::Default.\n> >\n> > A std::optional is initialised with std::nullopt.  How would I set a \n> > colorspace where I want a specific transfer function to be used in \n> > that? Will it still remain \"unset\" ?\n\nI do see an overlap between setting the colorspace to nullopt and\nsetting it to a Default colorspace, which isn't very nice (in general\nmultiple ways to do the same thing is a bad idea).\n\n> >> What do you think?\n> >> \n> >>>> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel wrote:\n> >>>>> Add a Colorspace::Default which corresponds to default V4L2\n> >>>>> colorspace identifiers.\n> >>>>>\n> >>>>> \\todo Add defaults to python colorspace bindings\n> >>>>>\n> >>>>> Signed-off-by: Umang Jain \n> >>>>> ---\n> >>>>>    include/libcamera/color_space.h |  5 +++++\n> >>>>>    src/libcamera/color_space.cpp   | 29 ++++++++++++++++++++++++++++-\n> >>>>>    src/libcamera/v4l2_device.cpp   |  5 +++++\n> >>>>>    3 files changed, 38 insertions(+), 1 deletion(-)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> >>>>> index 086c56c1..0ad8da17 100644\n> >>>>> --- a/include/libcamera/color_space.h\n> >>>>> +++ b/include/libcamera/color_space.h\n> >>>>> @@ -16,6 +16,7 @@ class ColorSpace\n> >>>>>  {\n> >>>>>  public:\n> >>>>>           enum class Primaries {\n> >>>>> +               Default,\n> >>>>>                   Raw,\n> >>>>>                   Smpte170m,\n> >>>>>                   Rec709,\n> >>>>> @@ -23,12 +24,14 @@ public:\n> >>>>>           };\n> >>>>>\n> >>>>>           enum class TransferFunction {\n> >>>>> +               Default,\n> >>>>>                   Linear,\n> >>>>>                   Srgb,\n> >>>>>                   Rec709,\n> >>>>>           };\n> >>>>>\n> >>>>>           enum class YcbcrEncoding {\n> >>>>> +               Default,\n> >>>>>                   None,\n> >>>>>                   Rec601,\n> >>>>>                   Rec709,\n> >>>>> @@ -36,6 +39,7 @@ public:\n> >>>>>           };\n> >>>>>\n> >>>>>           enum class Range {\n> >>>>> +               Default,\n> >>>>>                   Full,\n> >>>>>                   Limited,\n> >>>>>           };\n> >>>>> @@ -45,6 +49,7 @@ public:\n> >>>>>           {\n> >>>>>           }\n> >>>>>\n> >>>>> +       static const ColorSpace Default;\n> >>>>>           static const ColorSpace Raw;\n> >>>>>           static const ColorSpace Jpeg;\n> >>>>>           static const ColorSpace Srgb;\n> >>>>> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> >>>>> index 895e5c8e..d52f58cf 100644\n> >>>>> --- a/src/libcamera/color_space.cpp\n> >>>>> +++ b/src/libcamera/color_space.cpp\n> >>>>> @@ -50,6 +50,9 @@ namespace libcamera {\n> >>>>>   * \\enum ColorSpace::Primaries\n> >>>>>   * \\brief The color primaries for this color space\n> >>>>>   *\n> >>>>> + * \\var ColorSpace::Primaries::Default\n> >>>>> + * \\brief Use the default primaries as defined by the driver\n> >>>>> + *\n> >>>>>   * \\var ColorSpace::Primaries::Raw\n> >>>>>   * \\brief These are raw colors directly from a sensor, the primaries are\n> >>>>>   * unspecified\n> >>>>> @@ -68,6 +71,9 @@ namespace libcamera {\n> >>>>>   * \\enum ColorSpace::TransferFunction\n> >>>>>   * \\brief The transfer function used for this color space\n> >>>>>   *\n> >>>>> + * \\var ColorSpace::TransferFunction::Default\n> >>>>> + * \\brief Use the default transfer function as defined by the driver\n> >>>>> + *\n> >>>>>   * \\var ColorSpace::TransferFunction::Linear\n> >>>>>   * \\brief This color space uses a linear (identity) transfer function\n> >>>>>   *\n> >>>>> @@ -82,6 +88,9 @@ namespace libcamera {\n> >>>>>   * \\enum ColorSpace::YcbcrEncoding\n> >>>>>   * \\brief The Y'CbCr encoding\n> >>>>>   *\n> >>>>> + * \\var ColorSpace::YcbcrEncoding::Default\n> >>>>> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n> >>>>> + *\n> >>>>>   * \\var ColorSpace::YcbcrEncoding::None\n> >>>>>   * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> >>>>>   *\n> >>>>> @@ -99,6 +108,9 @@ namespace libcamera {\n> >>>>>   * \\enum ColorSpace::Range\n> >>>>>   * \\brief The range (sometimes \"quantisation\") for this color space\n> >>>>>   *\n> >>>>> + * \\var ColorSpace::Range::Default\n> >>>>> + * Use the default range as defined by the driver\n> >>>>> + *\n> >>>>>   * \\var ColorSpace::Range::Full\n> >>>>>   * \\brief This color space uses full range pixel values\n> >>>>>   *\n> >>>>> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n> >>>>>    {\n> >>>>>           /* Print out a brief name only for standard color spaces. */\n> >>>>>\n> >>>>> -       static const std::array, 6> colorSpaceNames = { {\n> >>>>> +       static const std::array, 7> colorSpaceNames = { {\n> >>>>> +               { ColorSpace::Default, \"Default\" },\n> >>>>>                   { ColorSpace::Raw, \"RAW\" },\n> >>>>>                   { ColorSpace::Jpeg, \"JPEG\" },\n> >>>>>                   { ColorSpace::Srgb, \"sRGB\" },\n> >>>>> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n> >>>>>           /* Assemble a name made of the constituent fields. */\n> >>>>>\n> >>>>>           static const std::map primariesNames = {\n> >>>>> +               { Primaries::Default, \"Default\" },\n> >>>>>                   { Primaries::Raw, \"RAW\" },\n> >>>>>                   { Primaries::Smpte170m, \"SMPTE170M\" },\n> >>>>>                   { Primaries::Rec709, \"Rec709\" },\n> >>>>>                   { Primaries::Rec2020, \"Rec2020\" },\n> >>>>>           };\n> >>>>>           static const std::map transferNames = {\n> >>>>> +               { TransferFunction::Default, \"Default\" },\n> >>>>>                   { TransferFunction::Linear, \"Linear\" },\n> >>>>>                   { TransferFunction::Srgb, \"sRGB\" },\n> >>>>>                   { TransferFunction::Rec709, \"Rec709\" },\n> >>>>>           };\n> >>>>>           static const std::map encodingNames = {\n> >>>>> +               { YcbcrEncoding::Default, \"Default\" },\n> >>>>>                   { YcbcrEncoding::None, \"None\" },\n> >>>>>                   { YcbcrEncoding::Rec601, \"Rec601\" },\n> >>>>>                   { YcbcrEncoding::Rec709, \"Rec709\" },\n> >>>>>                   { YcbcrEncoding::Rec2020, \"Rec2020\" },\n> >>>>>           };\n> >>>>>           static const std::map rangeNames = {\n> >>>>> +               { Range::Default, \"Default\" },\n> >>>>>                   { Range::Full, \"Full\" },\n> >>>>>                   { Range::Limited, \"Limited\" },\n> >>>>>           };\n> >>>>> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n> >>>>>   * \\brief The pixel range used with by color space\n> >>>>>   */\n> >>>>>\n> >>>>> +/**\n> >>>>> + * \\brief A constant representing a default color space\n> >>>>> + */\n> >>>>> +const ColorSpace ColorSpace::Default = {\n> >>>>> +       Primaries::Default,\n> >>>>> +       TransferFunction::Default,\n> >>>>> +       YcbcrEncoding::Default,\n> >>>>> +       Range::Default\n> >>>>> +};\n> >>>>> +\n> >>>>>  /**\n> >>>>>   * \\brief A constant representing a raw color space (from a sensor)\n> >>>>>   */\n> >>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>>>> index 3fc8438f..ecfcf337 100644\n> >>>>> --- a/src/libcamera/v4l2_device.cpp\n> >>>>> +++ b/src/libcamera/v4l2_device.cpp\n> >>>>> @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n> >>>>>  };\n> >>>>>\n> >>>>>  static const std::vector> colorSpaceToV4l2 = {\n> >>>>> +       { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n> >>>>>           { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> >>>>>           { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> >>>>>           { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> >>>>> @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n> >>>>>  };\n> >>>>>\n> >>>>>  static const std::map primariesToV4l2 = {\n> >>>>> +       { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n> >>>>>           { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> >>>>>           { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> >>>>>           { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> >>>>> @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n> >>>>>  };\n> >>>>>\n> >>>>>  static const std::map transferFunctionToV4l2 = {\n> >>>>> +       { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n> >>>>>           { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> >>>>>           { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> >>>>>           { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> >>>>>  };\n> >>>>>\n> >>>>>  static const std::map ycbcrEncodingToV4l2 = {\n> >>>>> +       { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n> >>>>>           { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> >>>>>           { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> >>>>>           { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> >>>>>  };\n> >>>>>\n> >>>>>  static const std::map rangeToV4l2 = {\n> >>>>> +       { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n> >>>>>           { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> >>>>>           { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\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 ED1C0C3275\n\tfor ;\n\tWed, 27 Jul 2022 00:02:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 346EF63312;\n\tWed, 27 Jul 2022 02:02:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03A4D603E8\n\tfor ;\n\tWed, 27 Jul 2022 02:02:00 +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 2544156D;\n\tWed, 27 Jul 2022 02:02:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658880123;\n\tbh=JPGMqndB8J9THySyvctGDMDfaBNDEfQQQnvmGW2KJP0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eevAnUyodSLNkwYajxBgxjAk5zcn1SSKeZNYDG++Fbn6mEtIGMwlmdBpae3i8MvX+\n\tkrCOP3YqSTpOlwXBdH1NbGu1w/XnfpJ3b5sCV+eovPnyv6NojBr+yLN83Ivy3AXAWi\n\tv+ReyfkCuP4Kcaomk+cfkoIxiVJLBHxnHkemy3f2aWMZedcygn9jD07o2EITEGnntR\n\tgWAUgsXOfDfXKD2hj5jhSBxomBcDj3/Ipx/Ow4xETbeC0hBW+DUNqGbVWlEy5MBOP2\n\t2I7St1dx0UOuBs+Bype/zsIPSr3gP8ZkBWAWlRpj7V25++xyxzNyEqI5TU30SYJJRq\n\t5RDc53LW2nfNg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658880120;\n\tbh=JPGMqndB8J9THySyvctGDMDfaBNDEfQQQnvmGW2KJP0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DF6mwYudKZ93fHm9FkAF0qT4tl5MCYGzv5ApR9ZZXZEPDb6psjia1SEy6wUYb3Ofv\n\treeKskK9WiI0vZm4DVvsMjTJsh9P1YJR5+tWts0cXr2pbjpl3rQimy7JR/WGW/MHGx\n\t3p+RdU8hQMvnNgxPJYT41rVDiQzWPV2iaYrLXWUk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DF6mwYud\"; dkim-atps=neutral","Date":"Wed, 27 Jul 2022 03:01:58 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>\n\t\n\t<72bd1d6c-21f2-61df-ff17-5a9f2447c6ee@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"vedantparanjape160201@gmail.com, rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tHans Verkuil , nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24210,"web_url":"https://patchwork.libcamera.org/comment/24210/","msgid":"<492dfd6c-908a-0caf-0a4b-a2d80885a518@ideasonboard.com>","date":"2022-07-28T08:04:53","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for detailed response.\n\nOn 7/27/22 05:31, Laurent Pinchart wrote:\n> Hello,\n>\n> (CC'ing Hans Verkuil)\n>\n> On Tue, Jul 26, 2022 at 09:17:01PM +0530, Umang Jain via libcamera-devel wrote:\n>> On 7/26/22 17:42, Umang Jain via libcamera-devel wrote:\n>>> On 7/26/22 15:42, David Plowman wrote:\n>>>> On Mon, 25 Jul 2022 at 12:15, Umang Jain wrote:\n>>>>> On 7/25/22 16:16, David Plowman wrote:\n>>>>>> Hi Umang\n>>>>>>\n>>>>>> Thanks for this patch. I was wondering if I could ask for a little\n>>>>>> more information on how it would be used.\n>>>>> Yes, so the idea here is to provide a colorspace where one(or more)\n>>>>> identifiers are specified by the user whereas others can be left upto\n>>>>> the driver to fill in.\n>>>>>\n>>>>> I wasn't aware of this use case until very recently. In gstreamer, it\n>>>>> seems possible that the user wants to use(enforce?) a particular\n>>>>> transfer function whereas other identifier are left upto the driver\n>>>>> to fill.\n>>>>>\n>>>>> I am not very sure how would this be helpful (since colorspace\n>>>>> identifiers depend on each other I think), and mis-matches between them\n>>>>> might not make any sense at all.\n>>>>>\n>>>>> But such a use case as has been identified it seems, atleast in the\n>>>>> gstreamer case - so to satisfy that, I went ahead with this approach. I\n>>>>> couldn't get a better solution where the user specifies a set of\n>>>>> identifiers to use for colorspace, at the same time depending on the\n>>>>> driver to fill unknown or default values for remaining identifier(s).\n>>>>>\n>>>>>> In the original libcamera ColorSpace implementation I was very keen to\n>>>>>> avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n>>>>>> the view that applications really should say what ColorSpace they\n>>>>>> want, but at the same time I knew there will always be (bad)\n>>>>>> applications that don't think about colour spaces and so I let them\n>>>>>> use std::optional to allow the ColorSpace to be completely\n>>>>>> unspecified.\n>>>>> Right, so when colorspace is completely unspecified, all values are\n>>>>> default-ed (V4L2Device::fromColorSpace()) in the v4l2_format.\n> Right, so when std::optional is std::nullopt, we set all the\n> colorspace-related fields in V4L2VideoDevice::setFormat() (and in the\n> equivalent implementation for v4L2Subdevice) to *_DEFAULT. The V4L2\n> driver then has to return a non-DEFAULT .colorspace, and possibly\n> non-DEFAULT values for .xfer_func, .ycbcr_enc and .quantization. This\n> defines without any ambiguity the exact colorspace information, and we\n> can create a fully defined ColorSpace out of that (if any of the\n> .xfer_func, .ycbcr_enc or .quantization fields are returned as DEFAULT,\n> we pick the default value that corresponds to the .colorspace, which\n> V4L2 guarantee will never be returned as DEFAULT by drivers).\n>\n> So far I don't see an issue.\n\n\nYes and I see that is how it is implemented in libcamera.\n\n>\n>>>>> If I am asked about what's the goal of this patch, I would say that it\n>>>>> enables the use case where a sub-set of colorspace identifiers are\n>>>>> known, while others need to use defaults. Currently there is no default\n>>>>> std::optional hence I added one, to facilitate\n>>>>> the application layer. The application should ideally copy the\n>>>>> libcamera::ColorSpace::Default as base, swap the identifiers it wants to\n>>>>> use in the copy, and send the custom colorspace for validation.\n> Our current API supports the following well-defined cases, where an\n> application can request\n>\n> - A well-defined colorspace (e.g. ColorSpace::Rec709), with everything\n> it implies (the primaries, transfer function, YCbCr encoding and\n> quantization range defined by the colorspace). For this, an\n> application simply uses one of the presets.\n>\n> - A well-defined colorspace, with one or multiple fields overridden to\n> different values (e.g. ColorSpace::Rec709 with a Full quantization\n> range). For this, an application copies one of the presets and\n> modifies one or multiple fields to well-defined values.\n>\n> - A completely custom colorspace (e.g. Rec709 primaries, Srgb transfer\n> unction, Rec2020 YCbCr encoding and Full quantization range). For this\n> an application can construct the colorspace manually.\n>\n> What we don't support today, and this is what I understand you want to\n> do with this patch, is for an application to tell libcamera \"give me any\n> colorspace, as long as the quantization is full range\". Is this right ?\n\n\nYes, on those lines...\n\n>\n> If that's the use case you're after, would it be an issue if the\n> application used\n>\n> \tColorSpace cs = ColorSpace::Srgb;\n> \tcs.range = ColorSpace::Range::Full;\n\n\nOkay, so as you have mentioned about \"give me any colorspace\" part. Does \nit mean, use any preset on application side? Like you have with \nColorSpace::Srgb ? To me it speaks as - \"Use ColorSpace::Srgb with full \nquantization range if possible\"\n\nWhat I want and seems to reflect the pharse: \"give me any colorspace, as \nlong as the quantization is full range\" is:\n\n\tColorSpace cs = ColorSpace::Default;\n\tcs.range = ColorSpace::Range::Full;\n\nSince I am assuming (or the application) isn't aware of the colorspace \nin the first place.\n\nNow you can ask, if application isn't aware of colorspace, how come it's \nmore aware (specific) about (xfer_func, ycbcr_enc, quantization etc.), \nwhich I agree is a valid question and I don't know the answer.\n\nSo this plugs into the point that we might be solving problems that \nmight not exist in the practical world\n>\n> \tStreamConfiguration &cfg = ...;\n> \tcfg.colorSpace = cs;\n>\n> ? The first two lines will be equivalent to\n>\n> \tColorSpace cs(Primaries::Rec709, TransferFunction::Srgb,\n> \t\t YcbcrEncoding::Rec601, Range::Full);\n>\n> which would then be translated to the V4L2 format\n>\n> \t.colorspace = V4L2_COLORSPACE_REC709;\n> \t.xfer_func = V4L2_XFER_FUNC_SRGB;\n> \t.ycbcr_enc = V4L2_YCBCR_ENC_601;\n> \t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n>\n> How a driver will handle this if this particular combination isn't\n> supported is unfortunately not clearly documented by V4L2, se we may\n> well get back\n>\n> \t.colorspace = V4L2_COLORSPACE_REC709;\n> \t.xfer_func = V4L2_XFER_FUNC_709;\n> \t.ycbcr_enc = V4L2_YCBCR_ENC_709;\n> \t.quantization = V4L2_QUANTIZATION_LIM_RANGE;\n>\n> even if the driver would support\n>\n> \t.colorspace = V4L2_COLORSPACE_REC709;\n> \t.xfer_func = V4L2_XFER_FUNC_709;\n> \t.ycbcr_enc = V4L2_YCBCR_ENC_709;\n> \t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n>\n> if asked directly, given that V4L2 doesn't describe how to adjust a\n> colorspace that isn't supported.\n>\n> Requesting\n>\n> \t.colorspace = V4L2_COLORSPACE_DEFAULT;\n> \t.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> \t.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> \t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n>\n> could possibly help, except that we have only two drivers that even try\n> to honour the *_CSC flags, let alone implement correct colorspace\n> behaviour.\n\n\nOh, I didn't even knew about those flags!\n\n>\n> On a side note, I have a small feeling we may be trying to solve\n> problems that will never appear in practice.\n\n\nThat feeling is growing inside of me as welln but validation is required.\n\n>\n>>>>> Also note, the mapping is only one way (from libcamera to V4L2)\n>>>>> intentionally. It shouldn't\n>>>>> https://github.com/raspberrypi/picamera2#installationbe the case\n>>>>> where driver itself returns\n>>>>> \"::Default\" back to user.\n>>>>>\n>>>>>> In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n>>>>>> infer the value from the overall V4L2_COLORSPACE (someone please\n>>>>>> correct me if that's wrong!). In libcamera we don't have an \"overall\n>>>>> If we could infer all the identifier values from\n>>>>> V4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:\n>>>>>\n>>>>> V4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT,\n>>>>> V4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.\n>>>>>\n>>>>>> colour space\" in this way, so what would it then mean? For example, is\n>>>>>> \"ColorSpace::Range::Default\" ever the same as\n>>>>>> \"ColorSpace::Range::Full\", or is it always different? How do we (can\n>>>>>> we even) document what it means?\n>>>>> Maybe we should look at V4L2 level first, before libcamera for e.g.\n>>>>>\n>>>>> Would V4L2_QUANTIZATION_DEFAULT be ever same as\n>>>>> V4L2_QUANTIZATION_FULL_RANGE ?\n> We need to make sure we can implement the behaviour defined in the\n> libcamera public API using the V4L2-based pipeline handlers, but the\n> V4L2 API doesn't dictate how we should handle colorspaces. If anything,\n> we should influence V4L2 with real world use cases.\n\n\nI can dig into various GStreamer plugins and various applications that \nuse those plugins and see how colorspaces is used/requested/ or intended \nto be used. Would it be a good exercise ?\n\nAre you aware any other places (except GStreamer) that can be helpful \nhere as well?\n\n>\n>>>>>> I'd be interested to hear what other folks think. Possibly I'm being a\n>>>>> I am all ears here as well, so let' see\n>>>>>\n>>>>>> bit paranoid, but then I have been bitten by \"wrong colour space\"\n>>>>>> problems often enough in the past (usually just before a product\n>>>>>> ships!).\n>>>>> ouch :-S\n>>>> My overall feeling is that I don't like having \"default\" things\n>>>> because I'm worried that they'll start appearing when you don't expect\n>>>> them. And then how would you know what they meant? As you know by now,\n>>>> I'm a bit paranoid about colour spaces going wrong...\n>>>>\n>>>> However, I see the point about these values possibly being convenient\n>>>> for negotiating colour spaces with (for example) gstreamer. But\n>>>> perhaps we could reduce the risk of them spreading, maybe we might\n>>>> consider:\n>>>>\n>>>> * Documenting clearly that \"default\" values are only \"inputs\" (what\n>>>> you want), and are never returned (what you got).\n> We should indeed never return any of the colorspace fields set to\n> \"Default\".\n>\n>>> I've already made it impossible to return \"default\" values in this\n>>> patch. That's the reasoning behind you see the mappings going from\n>>> libcamera to V4L2 /only/ and not other way around.\n>>>\n>>> Yes we can clarify the document on that front.\n>>>\n>>>> * Adding a valid() method that complains if a ColorSpace has any\n>>>> undefined/default values in it?\n>>>> * When a function returns a ColorSpace that has been set, can we make\n>>>> them check that the returned value is \"valid()\"?\n>>> The driver always return a colorspace which is supported right? I\n>>> believe a supported colorspace would not _DEFAULT identifier.\n>> To answer this, I have been clarified on #linux-media that a driver can\n>> report _DEFAULT identifiers  back to the userspace.\n> Only for .xfer_func, .ycbcr_enc and .quantization, never for\n> .colorspace, so we can always map a V4L2 *_DEFAULT value that we get\n> back from a driver to well-defined values for all fields.\n>\n>> But libcamera doesn't map to _DEFAULT identifiers in\n>> V4L2Device::toColorSpace() either, so we should be good here? Or still\n>> we need additional valid()?\n> On a side note, we have bugs in our colorspace handling. V4L2 considers\n> the colorspace fields as read-only from an application point of view,\n> both on video devices and on subdevs, unless the\n> V4L2_PIX_FMT_FLAG_SET_CSC or V4L2_MBUS_FRAMEFMT_SET_CSC flags\n> (respectively) are set when calling the S_FMT ioctl. Only vivid\n> implements this correctly for video nodes, and only the rkisp1 driver\n> handles the V4L2_MBUS_FRAMEFMT_SET_CSC on subdevs.\n\n\nSo whenever the std::optional has a value, libcamera should \nset those flags implicitly on behalf of application right?\n\n>\n> Furthermore, how the *_DEFAULT values operate when setting them is not\n> very well defined, I've tried to clarify that in #linux-media on OFTC\n> today, and I've been told by Hans that setting\n>\n> \t.flags = V4L2_MBUS_FRAMEFMT_SET_CSC;\n> \t.colorspace = REC709;\n> \t.xfer_func = DEFAULT;\n> \t.ycbcr_enc = DEFAULT;\n> \t.quantization = DEFAULT;\n>\n> on a subdev source pad means that the application wants to override the\n> color primaries and nothing else, while I interpret the documentation\n> ([1]) as saying that the application wants to set the .xfer_func,\n> .ycbcr_enc and .quantization to the default values specified by REC709.\n> Discussions are still ongoing.\n>\n> https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id1\n\n\nYes I see that confusion here. Usage of *_DEFAULTS should definitely be \ndefined.\n\n>\n>>> valid() on the returned colorspace means we don't trust the driver? It\n>>> might have set one of the fields as _DEFAULT ?\n>>>\n>>> I am not sure what valid() will try to achieve here. Provided there is\n>>> already handled case where any the mappings is missing in\n>>> libcamera(colorspace will be std::nullopt) [1]\n>>>\n>>> [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n816\n>>>\n>>>> * I also wasn't quite sure why a ColorSpace::Default is needed, would\n>>> Convenience\n>>>\n>>>> a ColorSpace be regarded as being \"default\" if every field is\n>>>> \"default\", or just some of them? Or could an unset std::optional be\n>>>> used instead?\n>>> If some fields are specific values than the \"default\" identifiers,\n>>> it's not a default colorspace. A default colorspace is when \"all\"\n>>> fields are default and is equal to libcamera::ColorSpace::Default.\n>>>\n>>> A std::optional is initialised with std::nullopt.  How would I set a\n>>> colorspace where I want a specific transfer function to be used in\n>>> that? Will it still remain \"unset\" ?\n> I do see an overlap between setting the colorspace to nullopt and\n> setting it to a Default colorspace, which isn't very nice (in general\n> multiple ways to do the same thing is a bad idea).\n\n\nAck.\n\n>\n>>>> What do you think?\n>>>>\n>>>>>> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel wrote:\n>>>>>>> Add a Colorspace::Default which corresponds to default V4L2\n>>>>>>> colorspace identifiers.\n>>>>>>>\n>>>>>>> \\todo Add defaults to python colorspace bindings\n>>>>>>>\n>>>>>>> Signed-off-by: Umang Jain \n>>>>>>> ---\n>>>>>>>    include/libcamera/color_space.h |  5 +++++\n>>>>>>>    src/libcamera/color_space.cpp   | 29 ++++++++++++++++++++++++++++-\n>>>>>>>    src/libcamera/v4l2_device.cpp   |  5 +++++\n>>>>>>>    3 files changed, 38 insertions(+), 1 deletion(-)\n>>>>>>>\n>>>>>>> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n>>>>>>> index 086c56c1..0ad8da17 100644\n>>>>>>> --- a/include/libcamera/color_space.h\n>>>>>>> +++ b/include/libcamera/color_space.h\n>>>>>>> @@ -16,6 +16,7 @@ class ColorSpace\n>>>>>>>  {\n>>>>>>>  public:\n>>>>>>>           enum class Primaries {\n>>>>>>> +               Default,\n>>>>>>>                   Raw,\n>>>>>>>                   Smpte170m,\n>>>>>>>                   Rec709,\n>>>>>>> @@ -23,12 +24,14 @@ public:\n>>>>>>>           };\n>>>>>>>\n>>>>>>>           enum class TransferFunction {\n>>>>>>> +               Default,\n>>>>>>>                   Linear,\n>>>>>>>                   Srgb,\n>>>>>>>                   Rec709,\n>>>>>>>           };\n>>>>>>>\n>>>>>>>           enum class YcbcrEncoding {\n>>>>>>> +               Default,\n>>>>>>>                   None,\n>>>>>>>                   Rec601,\n>>>>>>>                   Rec709,\n>>>>>>> @@ -36,6 +39,7 @@ public:\n>>>>>>>           };\n>>>>>>>\n>>>>>>>           enum class Range {\n>>>>>>> +               Default,\n>>>>>>>                   Full,\n>>>>>>>                   Limited,\n>>>>>>>           };\n>>>>>>> @@ -45,6 +49,7 @@ public:\n>>>>>>>           {\n>>>>>>>           }\n>>>>>>>\n>>>>>>> +       static const ColorSpace Default;\n>>>>>>>           static const ColorSpace Raw;\n>>>>>>>           static const ColorSpace Jpeg;\n>>>>>>>           static const ColorSpace Srgb;\n>>>>>>> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n>>>>>>> index 895e5c8e..d52f58cf 100644\n>>>>>>> --- a/src/libcamera/color_space.cpp\n>>>>>>> +++ b/src/libcamera/color_space.cpp\n>>>>>>> @@ -50,6 +50,9 @@ namespace libcamera {\n>>>>>>>   * \\enum ColorSpace::Primaries\n>>>>>>>   * \\brief The color primaries for this color space\n>>>>>>>   *\n>>>>>>> + * \\var ColorSpace::Primaries::Default\n>>>>>>> + * \\brief Use the default primaries as defined by the driver\n>>>>>>> + *\n>>>>>>>   * \\var ColorSpace::Primaries::Raw\n>>>>>>>   * \\brief These are raw colors directly from a sensor, the primaries are\n>>>>>>>   * unspecified\n>>>>>>> @@ -68,6 +71,9 @@ namespace libcamera {\n>>>>>>>   * \\enum ColorSpace::TransferFunction\n>>>>>>>   * \\brief The transfer function used for this color space\n>>>>>>>   *\n>>>>>>> + * \\var ColorSpace::TransferFunction::Default\n>>>>>>> + * \\brief Use the default transfer function as defined by the driver\n>>>>>>> + *\n>>>>>>>   * \\var ColorSpace::TransferFunction::Linear\n>>>>>>>   * \\brief This color space uses a linear (identity) transfer function\n>>>>>>>   *\n>>>>>>> @@ -82,6 +88,9 @@ namespace libcamera {\n>>>>>>>   * \\enum ColorSpace::YcbcrEncoding\n>>>>>>>   * \\brief The Y'CbCr encoding\n>>>>>>>   *\n>>>>>>> + * \\var ColorSpace::YcbcrEncoding::Default\n>>>>>>> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n>>>>>>> + *\n>>>>>>>   * \\var ColorSpace::YcbcrEncoding::None\n>>>>>>>   * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n>>>>>>>   *\n>>>>>>> @@ -99,6 +108,9 @@ namespace libcamera {\n>>>>>>>   * \\enum ColorSpace::Range\n>>>>>>>   * \\brief The range (sometimes \"quantisation\") for this color space\n>>>>>>>   *\n>>>>>>> + * \\var ColorSpace::Range::Default\n>>>>>>> + * Use the default range as defined by the driver\n>>>>>>> + *\n>>>>>>>   * \\var ColorSpace::Range::Full\n>>>>>>>   * \\brief This color space uses full range pixel values\n>>>>>>>   *\n>>>>>>> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n>>>>>>>    {\n>>>>>>>           /* Print out a brief name only for standard color spaces. */\n>>>>>>>\n>>>>>>> -       static const std::array, 6> colorSpaceNames = { {\n>>>>>>> +       static const std::array, 7> colorSpaceNames = { {\n>>>>>>> +               { ColorSpace::Default, \"Default\" },\n>>>>>>>                   { ColorSpace::Raw, \"RAW\" },\n>>>>>>>                   { ColorSpace::Jpeg, \"JPEG\" },\n>>>>>>>                   { ColorSpace::Srgb, \"sRGB\" },\n>>>>>>> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n>>>>>>>           /* Assemble a name made of the constituent fields. */\n>>>>>>>\n>>>>>>>           static const std::map primariesNames = {\n>>>>>>> +               { Primaries::Default, \"Default\" },\n>>>>>>>                   { Primaries::Raw, \"RAW\" },\n>>>>>>>                   { Primaries::Smpte170m, \"SMPTE170M\" },\n>>>>>>>                   { Primaries::Rec709, \"Rec709\" },\n>>>>>>>                   { Primaries::Rec2020, \"Rec2020\" },\n>>>>>>>           };\n>>>>>>>           static const std::map transferNames = {\n>>>>>>> +               { TransferFunction::Default, \"Default\" },\n>>>>>>>                   { TransferFunction::Linear, \"Linear\" },\n>>>>>>>                   { TransferFunction::Srgb, \"sRGB\" },\n>>>>>>>                   { TransferFunction::Rec709, \"Rec709\" },\n>>>>>>>           };\n>>>>>>>           static const std::map encodingNames = {\n>>>>>>> +               { YcbcrEncoding::Default, \"Default\" },\n>>>>>>>                   { YcbcrEncoding::None, \"None\" },\n>>>>>>>                   { YcbcrEncoding::Rec601, \"Rec601\" },\n>>>>>>>                   { YcbcrEncoding::Rec709, \"Rec709\" },\n>>>>>>>                   { YcbcrEncoding::Rec2020, \"Rec2020\" },\n>>>>>>>           };\n>>>>>>>           static const std::map rangeNames = {\n>>>>>>> +               { Range::Default, \"Default\" },\n>>>>>>>                   { Range::Full, \"Full\" },\n>>>>>>>                   { Range::Limited, \"Limited\" },\n>>>>>>>           };\n>>>>>>> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n>>>>>>>   * \\brief The pixel range used with by color space\n>>>>>>>   */\n>>>>>>>\n>>>>>>> +/**\n>>>>>>> + * \\brief A constant representing a default color space\n>>>>>>> + */\n>>>>>>> +const ColorSpace ColorSpace::Default = {\n>>>>>>> +       Primaries::Default,\n>>>>>>> +       TransferFunction::Default,\n>>>>>>> +       YcbcrEncoding::Default,\n>>>>>>> +       Range::Default\n>>>>>>> +};\n>>>>>>> +\n>>>>>>>  /**\n>>>>>>>   * \\brief A constant representing a raw color space (from a sensor)\n>>>>>>>   */\n>>>>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>>>>>> index 3fc8438f..ecfcf337 100644\n>>>>>>> --- a/src/libcamera/v4l2_device.cpp\n>>>>>>> +++ b/src/libcamera/v4l2_device.cpp\n>>>>>>> @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n>>>>>>>  };\n>>>>>>>\n>>>>>>>  static const std::vector> colorSpaceToV4l2 = {\n>>>>>>> +       { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n>>>>>>>           { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n>>>>>>>           { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n>>>>>>>           { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n>>>>>>> @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n>>>>>>>  };\n>>>>>>>\n>>>>>>>  static const std::map primariesToV4l2 = {\n>>>>>>> +       { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n>>>>>>>           { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n>>>>>>>           { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n>>>>>>>           { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n>>>>>>> @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n>>>>>>>  };\n>>>>>>>\n>>>>>>>  static const std::map transferFunctionToV4l2 = {\n>>>>>>> +       { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n>>>>>>>           { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n>>>>>>>           { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n>>>>>>>           { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n>>>>>>>  };\n>>>>>>>\n>>>>>>>  static const std::map ycbcrEncodingToV4l2 = {\n>>>>>>> +       { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n>>>>>>>           { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n>>>>>>>           { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n>>>>>>>           { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n>>>>>>>  };\n>>>>>>>\n>>>>>>>  static const std::map rangeToV4l2 = {\n>>>>>>> +       { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n>>>>>>>           { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n>>>>>>>           { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\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 43255C3275\n\tfor ;\n\tThu, 28 Jul 2022 08:05:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95CEA63311;\n\tThu, 28 Jul 2022 10:05:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7A33603EB\n\tfor ;\n\tThu, 28 Jul 2022 10:05:01 +0200 (CEST)","from [IPV6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d] (unknown\n\t[IPv6:2401:4900:1f3e:f7a:bc8f:12ed:b45f:c35d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 207136D4;\n\tThu, 28 Jul 2022 10:04:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658995503;\n\tbh=41gvV1XYdOS1vtkb866w2aGqIVLZ5RjZpJqX+XSBh5M=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Q7JP4zTnUdPqfUK5imlw2IVqki04w8+KuYMdPiIyqCJUWUaFZcI7eD+YHSuQjfqqf\n\tSaXHrCuphJ4492OoqwDYz4C2cmusgyoGtMqiUMgIwAtzyZmnLApsDweESrUQDZS3FT\n\tHJj0Vt38X9nJupxy2YkHR3HhDGmOtO+ycAzPKL/qUpYYpKSTUPPv7DTXl/1IZuLKuf\n\trz5NoN9r6R+nX7jj0OOjegUK22z3xYq2Uz080mbzDITjVzD979amkmdraUDmfdO+lV\n\tFh/XI9BRigwZhrvlx2PeiMqZrhAg3y7owoQ1fI/E8/k4ppO+ItmkOLe7JomB4Y1SLd\n\tuBzkfiBYcp5bg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658995501;\n\tbh=41gvV1XYdOS1vtkb866w2aGqIVLZ5RjZpJqX+XSBh5M=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Zq9o0+rARcdYiTSYEs89PeQ99RpMM/TQ+ek/+EkTYr1SAO1EoxTI6kPALIl3rqiAu\n\tO5X3KNPlHSGjZxGR1teOASwIfEkdqNFT9nTjSKJdy1cn/1Pk8cnqGVv9YgYQznnA5J\n\tS9W5+wp03MUBvHQkF/kbB/6IGB16OxYRg6gk/PYU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Zq9o0+rA\"; dkim-atps=neutral","Message-ID":"<492dfd6c-908a-0caf-0a4b-a2d80885a518@ideasonboard.com>","Date":"Thu, 28 Jul 2022 13:34:53 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>\n\t\n\t<72bd1d6c-21f2-61df-ff17-5a9f2447c6ee@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"vedantparanjape160201@gmail.com, rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tHans Verkuil , nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24245,"web_url":"https://patchwork.libcamera.org/comment/24245/","msgid":"","date":"2022-07-29T15:19:11","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Jul 28, 2022 at 01:34:53PM +0530, Umang Jain wrote:\n> On 7/27/22 05:31, Laurent Pinchart wrote:\n> > On Tue, Jul 26, 2022 at 09:17:01PM +0530, Umang Jain via libcamera-devel wrote:\n> >> On 7/26/22 17:42, Umang Jain via libcamera-devel wrote:\n> >>> On 7/26/22 15:42, David Plowman wrote:\n> >>>> On Mon, 25 Jul 2022 at 12:15, Umang Jain wrote:\n> >>>>> On 7/25/22 16:16, David Plowman wrote:\n> >>>>>> Hi Umang\n> >>>>>>\n> >>>>>> Thanks for this patch. I was wondering if I could ask for a little\n> >>>>>> more information on how it would be used.\n> >>>>>\n> >>>>> Yes, so the idea here is to provide a colorspace where one(or more)\n> >>>>> identifiers are specified by the user whereas others can be left upto\n> >>>>> the driver to fill in.\n> >>>>>\n> >>>>> I wasn't aware of this use case until very recently. In gstreamer, it\n> >>>>> seems possible that the user wants to use(enforce?) a particular\n> >>>>> transfer function whereas other identifier are left upto the driver\n> >>>>> to fill.\n> >>>>>\n> >>>>> I am not very sure how would this be helpful (since colorspace\n> >>>>> identifiers depend on each other I think), and mis-matches between them\n> >>>>> might not make any sense at all.\n> >>>>>\n> >>>>> But such a use case as has been identified it seems, atleast in the\n> >>>>> gstreamer case - so to satisfy that, I went ahead with this approach. I\n> >>>>> couldn't get a better solution where the user specifies a set of\n> >>>>> identifiers to use for colorspace, at the same time depending on the\n> >>>>> driver to fill unknown or default values for remaining identifier(s).\n> >>>>>\n> >>>>>> In the original libcamera ColorSpace implementation I was very keen to\n> >>>>>> avoid \"unknown\" or \"default\" values inside ColorSpaces. I was taking\n> >>>>>> the view that applications really should say what ColorSpace they\n> >>>>>> want, but at the same time I knew there will always be (bad)\n> >>>>>> applications that don't think about colour spaces and so I let them\n> >>>>>> use std::optional to allow the ColorSpace to be completely\n> >>>>>> unspecified.\n> >>>>>\n> >>>>> Right, so when colorspace is completely unspecified, all values are\n> >>>>> default-ed (V4L2Device::fromColorSpace()) in the v4l2_format.\n> >\n> > Right, so when std::optional is std::nullopt, we set all the\n> > colorspace-related fields in V4L2VideoDevice::setFormat() (and in the\n> > equivalent implementation for v4L2Subdevice) to *_DEFAULT. The V4L2\n> > driver then has to return a non-DEFAULT .colorspace, and possibly\n> > non-DEFAULT values for .xfer_func, .ycbcr_enc and .quantization. This\n> > defines without any ambiguity the exact colorspace information, and we\n> > can create a fully defined ColorSpace out of that (if any of the\n> > .xfer_func, .ycbcr_enc or .quantization fields are returned as DEFAULT,\n> > we pick the default value that corresponds to the .colorspace, which\n> > V4L2 guarantee will never be returned as DEFAULT by drivers).\n> >\n> > So far I don't see an issue.\n> \n> Yes and I see that is how it is implemented in libcamera.\n> \n> >>>>> If I am asked about what's the goal of this patch, I would say that it\n> >>>>> enables the use case where a sub-set of colorspace identifiers are\n> >>>>> known, while others need to use defaults. Currently there is no default\n> >>>>> std::optional hence I added one, to facilitate\n> >>>>> the application layer. The application should ideally copy the\n> >>>>> libcamera::ColorSpace::Default as base, swap the identifiers it wants to\n> >>>>> use in the copy, and send the custom colorspace for validation.\n> >\n> > Our current API supports the following well-defined cases, where an\n> > application can request\n> >\n> > - A well-defined colorspace (e.g. ColorSpace::Rec709), with everything\n> > it implies (the primaries, transfer function, YCbCr encoding and\n> > quantization range defined by the colorspace). For this, an\n> > application simply uses one of the presets.\n> >\n> > - A well-defined colorspace, with one or multiple fields overridden to\n> > different values (e.g. ColorSpace::Rec709 with a Full quantization\n> > range). For this, an application copies one of the presets and\n> > modifies one or multiple fields to well-defined values.\n> >\n> > - A completely custom colorspace (e.g. Rec709 primaries, Srgb transfer\n> > unction, Rec2020 YCbCr encoding and Full quantization range). For this\n> > an application can construct the colorspace manually.\n> >\n> > What we don't support today, and this is what I understand you want to\n> > do with this patch, is for an application to tell libcamera \"give me any\n> > colorspace, as long as the quantization is full range\". Is this right ?\n> \n> Yes, on those lines...\n> \n> > If that's the use case you're after, would it be an issue if the\n> > application used\n> >\n> > \tColorSpace cs = ColorSpace::Srgb;\n> > \tcs.range = ColorSpace::Range::Full;\n> \n> Okay, so as you have mentioned about \"give me any colorspace\" part. Does \n> it mean, use any preset on application side? Like you have with \n> ColorSpace::Srgb ? To me it speaks as - \"Use ColorSpace::Srgb with full \n> quantization range if possible\"\n\nI think \"ColorSpace::Srgb with full quantization range\" is a bad\nconcept. A color space is defined by four independent parameters. It\nhappens that some combination of those parameters don't necessarily make\nmuch sense, and some other combinations are widely used. That's where\nthe presets come from, they combine well-defined values for all four\nparameters under a standard name, just for convenience.\n\nTo me, \n\n\tColorSpace cs = ColorSpace::Srgb;\n\tcs.range = ColorSpace::Range::Full;\n\nshould be regarded as the same as\n\n\tColorSpace cs{\n\t\tPrimaries::Rec709,\n\t\tTransferFunction::Srgb,\n\t\tYcbcrEncoding::Rec601,\n\t\tRange::Full\n\t};\n\nwhich is actuall the same as ColorSpace::Jpeg. I wouldn't consider it\nmore \"Srgb with full quantization range\" than it is \"Smpte170m with\nRec709 primaries and full quantization range\". Once you modify one field\nof a ColorSpace preset, I think we should consider it as having lost\nany relation with that preset.\n\n> What I want and seems to reflect the pharse: \"give me any colorspace, as \n> long as the quantization is full range\" is:\n> \n> \tColorSpace cs = ColorSpace::Default;\n> \tcs.range = ColorSpace::Range::Full;\n> \n> Since I am assuming (or the application) isn't aware of the colorspace \n> in the first place.\n> \n> Now you can ask, if application isn't aware of colorspace, how come it's \n> more aware (specific) about (xfer_func, ycbcr_enc, quantization etc.), \n> which I agree is a valid question and I don't know the answer.\n> \n> So this plugs into the point that we might be solving problems that \n> might not exist in the practical world\n\nYes, and I think that applications should be aware of color spaces :-)\nIt's our job to make that awareness as painless as possible, but we\ncan't do more than that.\n\n> > \tStreamConfiguration &cfg = ...;\n> > \tcfg.colorSpace = cs;\n> >\n> > ? The first two lines will be equivalent to\n> >\n> > \tColorSpace cs(Primaries::Rec709, TransferFunction::Srgb,\n> > \t\t YcbcrEncoding::Rec601, Range::Full);\n> >\n> > which would then be translated to the V4L2 format\n> >\n> > \t.colorspace = V4L2_COLORSPACE_REC709;\n> > \t.xfer_func = V4L2_XFER_FUNC_SRGB;\n> > \t.ycbcr_enc = V4L2_YCBCR_ENC_601;\n> > \t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n> >\n> > How a driver will handle this if this particular combination isn't\n> > supported is unfortunately not clearly documented by V4L2, se we may\n> > well get back\n> >\n> > \t.colorspace = V4L2_COLORSPACE_REC709;\n> > \t.xfer_func = V4L2_XFER_FUNC_709;\n> > \t.ycbcr_enc = V4L2_YCBCR_ENC_709;\n> > \t.quantization = V4L2_QUANTIZATION_LIM_RANGE;\n> >\n> > even if the driver would support\n> >\n> > \t.colorspace = V4L2_COLORSPACE_REC709;\n> > \t.xfer_func = V4L2_XFER_FUNC_709;\n> > \t.ycbcr_enc = V4L2_YCBCR_ENC_709;\n> > \t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n> >\n> > if asked directly, given that V4L2 doesn't describe how to adjust a\n> > colorspace that isn't supported.\n> >\n> > Requesting\n> >\n> > \t.colorspace = V4L2_COLORSPACE_DEFAULT;\n> > \t.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> > \t.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> > \t.quantization = V4L2_QUANTIZATION_FULL_RANGE;\n> >\n> > could possibly help, except that we have only two drivers that even try\n> > to honour the *_CSC flags, let alone implement correct colorspace\n> > behaviour.\n> \n> Oh, I didn't even knew about those flags!\n> \n> > On a side note, I have a small feeling we may be trying to solve\n> > problems that will never appear in practice.\n> \n> That feeling is growing inside of me as welln but validation is required.\n> \n> >>>>> Also note, the mapping is only one way (from libcamera to V4L2)\n> >>>>> intentionally. It shouldn't\n> >>>>> https://github.com/raspberrypi/picamera2#installationbe the case\n> >>>>> where driver itself returns\n> >>>>> \"::Default\" back to user.\n> >>>>>\n> >>>>>> In V4L2, my understanding is that \"_DEFAULT\" normally means that you\n> >>>>>> infer the value from the overall V4L2_COLORSPACE (someone please\n> >>>>>> correct me if that's wrong!). In libcamera we don't have an \"overall\n> >>>>> If we could infer all the identifier values from\n> >>>>> V4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:\n> >>>>>\n> >>>>> V4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT,\n> >>>>> V4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.\n> >>>>>\n> >>>>>> colour space\" in this way, so what would it then mean? For example, is\n> >>>>>> \"ColorSpace::Range::Default\" ever the same as\n> >>>>>> \"ColorSpace::Range::Full\", or is it always different? How do we (can\n> >>>>>> we even) document what it means?\n> >>>>> Maybe we should look at V4L2 level first, before libcamera for e.g.\n> >>>>>\n> >>>>> Would V4L2_QUANTIZATION_DEFAULT be ever same as\n> >>>>> V4L2_QUANTIZATION_FULL_RANGE ?\n> >\n> > We need to make sure we can implement the behaviour defined in the\n> > libcamera public API using the V4L2-based pipeline handlers, but the\n> > V4L2 API doesn't dictate how we should handle colorspaces. If anything,\n> > we should influence V4L2 with real world use cases.\n> \n> I can dig into various GStreamer plugins and various applications that \n> use those plugins and see how colorspaces is used/requested/ or intended \n> to be used. Would it be a good exercise ?\n> \n> Are you aware any other places (except GStreamer) that can be helpful \n> here as well?\n\nColorspaces are probably handled in, for instance, ffmpeg, but I'm not\nsure it would be worth it studying that code. Also, based on our\ndiscussions from yesterday, I think we concluded we don't need Default\nvalues for the four color space enums, so part of the discussion in this\ne-mail isn't applicable anymore.\n\n> >>>>>> I'd be interested to hear what other folks think. Possibly I'm being a\n> >>>>> I am all ears here as well, so let' see\n> >>>>>\n> >>>>>> bit paranoid, but then I have been bitten by \"wrong colour space\"\n> >>>>>> problems often enough in the past (usually just before a product\n> >>>>>> ships!).\n> >>>>>\n> >>>>> ouch :-S\n> >>>>\n> >>>> My overall feeling is that I don't like having \"default\" things\n> >>>> because I'm worried that they'll start appearing when you don't expect\n> >>>> them. And then how would you know what they meant? As you know by now,\n> >>>> I'm a bit paranoid about colour spaces going wrong...\n> >>>>\n> >>>> However, I see the point about these values possibly being convenient\n> >>>> for negotiating colour spaces with (for example) gstreamer. But\n> >>>> perhaps we could reduce the risk of them spreading, maybe we might\n> >>>> consider:\n> >>>>\n> >>>> * Documenting clearly that \"default\" values are only \"inputs\" (what\n> >>>> you want), and are never returned (what you got).\n> >\n> > We should indeed never return any of the colorspace fields set to\n> > \"Default\".\n> >\n> >>> I've already made it impossible to return \"default\" values in this\n> >>> patch. That's the reasoning behind you see the mappings going from\n> >>> libcamera to V4L2 /only/ and not other way around.\n> >>>\n> >>> Yes we can clarify the document on that front.\n> >>>\n> >>>> * Adding a valid() method that complains if a ColorSpace has any\n> >>>> undefined/default values in it?\n> >>>> * When a function returns a ColorSpace that has been set, can we make\n> >>>> them check that the returned value is \"valid()\"?\n> >>>\n> >>> The driver always return a colorspace which is supported right? I\n> >>> believe a supported colorspace would not _DEFAULT identifier.\n> >>\n> >> To answer this, I have been clarified on #linux-media that a driver can\n> >> report _DEFAULT identifiers  back to the userspace.\n> >\n> > Only for .xfer_func, .ycbcr_enc and .quantization, never for\n> > .colorspace, so we can always map a V4L2 *_DEFAULT value that we get\n> > back from a driver to well-defined values for all fields.\n> >\n> >> But libcamera doesn't map to _DEFAULT identifiers in\n> >> V4L2Device::toColorSpace() either, so we should be good here? Or still\n> >> we need additional valid()?\n> >\n> > On a side note, we have bugs in our colorspace handling. V4L2 considers\n> > the colorspace fields as read-only from an application point of view,\n> > both on video devices and on subdevs, unless the\n> > V4L2_PIX_FMT_FLAG_SET_CSC or V4L2_MBUS_FRAMEFMT_SET_CSC flags\n> > (respectively) are set when calling the S_FMT ioctl. Only vivid\n> > implements this correctly for video nodes, and only the rkisp1 driver\n> > handles the V4L2_MBUS_FRAMEFMT_SET_CSC on subdevs.\n> \n> So whenever the std::optional has a value, libcamera should \n> set those flags implicitly on behalf of application right?\n\nYes, I believe so. I'm sure we'll run into corner cases of course, but\nthat's business as usual, we'll handle them :-)\n\n> > Furthermore, how the *_DEFAULT values operate when setting them is not\n> > very well defined, I've tried to clarify that in #linux-media on OFTC\n> > today, and I've been told by Hans that setting\n> >\n> > \t.flags = V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > \t.colorspace = REC709;\n> > \t.xfer_func = DEFAULT;\n> > \t.ycbcr_enc = DEFAULT;\n> > \t.quantization = DEFAULT;\n> >\n> > on a subdev source pad means that the application wants to override the\n> > color primaries and nothing else, while I interpret the documentation\n> > ([1]) as saying that the application wants to set the .xfer_func,\n> > .ycbcr_enc and .quantization to the default values specified by REC709.\n> > Discussions are still ongoing.\n> >\n> > https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id1\n> \n> Yes I see that confusion here. Usage of *_DEFAULTS should definitely be \n> defined.\n> \n> >>> valid() on the returned colorspace means we don't trust the driver? It\n> >>> might have set one of the fields as _DEFAULT ?\n> >>>\n> >>> I am not sure what valid() will try to achieve here. Provided there is\n> >>> already handled case where any the mappings is missing in\n> >>> libcamera(colorspace will be std::nullopt) [1]\n> >>>\n> >>> [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n816\n> >>>\n> >>>> * I also wasn't quite sure why a ColorSpace::Default is needed, would\n> >>> Convenience\n> >>>\n> >>>> a ColorSpace be regarded as being \"default\" if every field is\n> >>>> \"default\", or just some of them? Or could an unset std::optional be\n> >>>> used instead?\n> >>>\n> >>> If some fields are specific values than the \"default\" identifiers,\n> >>> it's not a default colorspace. A default colorspace is when \"all\"\n> >>> fields are default and is equal to libcamera::ColorSpace::Default.\n> >>>\n> >>> A std::optional is initialised with std::nullopt.  How would I set a\n> >>> colorspace where I want a specific transfer function to be used in\n> >>> that? Will it still remain \"unset\" ?\n> >\n> > I do see an overlap between setting the colorspace to nullopt and\n> > setting it to a Default colorspace, which isn't very nice (in general\n> > multiple ways to do the same thing is a bad idea).\n> \n> Ack.\n> \n> >>>> What do you think?\n> >>>>\n> >>>>>> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel wrote:\n> >>>>>>> Add a Colorspace::Default which corresponds to default V4L2\n> >>>>>>> colorspace identifiers.\n> >>>>>>>\n> >>>>>>> \\todo Add defaults to python colorspace bindings\n> >>>>>>>\n> >>>>>>> Signed-off-by: Umang Jain \n> >>>>>>> ---\n> >>>>>>>    include/libcamera/color_space.h |  5 +++++\n> >>>>>>>    src/libcamera/color_space.cpp   | 29 ++++++++++++++++++++++++++++-\n> >>>>>>>    src/libcamera/v4l2_device.cpp   |  5 +++++\n> >>>>>>>    3 files changed, 38 insertions(+), 1 deletion(-)\n> >>>>>>>\n> >>>>>>> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h\n> >>>>>>> index 086c56c1..0ad8da17 100644\n> >>>>>>> --- a/include/libcamera/color_space.h\n> >>>>>>> +++ b/include/libcamera/color_space.h\n> >>>>>>> @@ -16,6 +16,7 @@ class ColorSpace\n> >>>>>>>  {\n> >>>>>>>  public:\n> >>>>>>>           enum class Primaries {\n> >>>>>>> +               Default,\n> >>>>>>>                   Raw,\n> >>>>>>>                   Smpte170m,\n> >>>>>>>                   Rec709,\n> >>>>>>> @@ -23,12 +24,14 @@ public:\n> >>>>>>>           };\n> >>>>>>>\n> >>>>>>>           enum class TransferFunction {\n> >>>>>>> +               Default,\n> >>>>>>>                   Linear,\n> >>>>>>>                   Srgb,\n> >>>>>>>                   Rec709,\n> >>>>>>>           };\n> >>>>>>>\n> >>>>>>>           enum class YcbcrEncoding {\n> >>>>>>> +               Default,\n> >>>>>>>                   None,\n> >>>>>>>                   Rec601,\n> >>>>>>>                   Rec709,\n> >>>>>>> @@ -36,6 +39,7 @@ public:\n> >>>>>>>           };\n> >>>>>>>\n> >>>>>>>           enum class Range {\n> >>>>>>> +               Default,\n> >>>>>>>                   Full,\n> >>>>>>>                   Limited,\n> >>>>>>>           };\n> >>>>>>> @@ -45,6 +49,7 @@ public:\n> >>>>>>>           {\n> >>>>>>>           }\n> >>>>>>>\n> >>>>>>> +       static const ColorSpace Default;\n> >>>>>>>           static const ColorSpace Raw;\n> >>>>>>>           static const ColorSpace Jpeg;\n> >>>>>>>           static const ColorSpace Srgb;\n> >>>>>>> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp\n> >>>>>>> index 895e5c8e..d52f58cf 100644\n> >>>>>>> --- a/src/libcamera/color_space.cpp\n> >>>>>>> +++ b/src/libcamera/color_space.cpp\n> >>>>>>> @@ -50,6 +50,9 @@ namespace libcamera {\n> >>>>>>>   * \\enum ColorSpace::Primaries\n> >>>>>>>   * \\brief The color primaries for this color space\n> >>>>>>>   *\n> >>>>>>> + * \\var ColorSpace::Primaries::Default\n> >>>>>>> + * \\brief Use the default primaries as defined by the driver\n> >>>>>>> + *\n> >>>>>>>   * \\var ColorSpace::Primaries::Raw\n> >>>>>>>   * \\brief These are raw colors directly from a sensor, the primaries are\n> >>>>>>>   * unspecified\n> >>>>>>> @@ -68,6 +71,9 @@ namespace libcamera {\n> >>>>>>>   * \\enum ColorSpace::TransferFunction\n> >>>>>>>   * \\brief The transfer function used for this color space\n> >>>>>>>   *\n> >>>>>>> + * \\var ColorSpace::TransferFunction::Default\n> >>>>>>> + * \\brief Use the default transfer function as defined by the driver\n> >>>>>>> + *\n> >>>>>>>   * \\var ColorSpace::TransferFunction::Linear\n> >>>>>>>   * \\brief This color space uses a linear (identity) transfer function\n> >>>>>>>   *\n> >>>>>>> @@ -82,6 +88,9 @@ namespace libcamera {\n> >>>>>>>   * \\enum ColorSpace::YcbcrEncoding\n> >>>>>>>   * \\brief The Y'CbCr encoding\n> >>>>>>>   *\n> >>>>>>> + * \\var ColorSpace::YcbcrEncoding::Default\n> >>>>>>> + * \\brief Use the default Y'CbCr encoding as defined by the driver\n> >>>>>>> + *\n> >>>>>>>   * \\var ColorSpace::YcbcrEncoding::None\n> >>>>>>>   * \\brief There is no defined Y'CbCr encoding (used for non-YUV formats)\n> >>>>>>>   *\n> >>>>>>> @@ -99,6 +108,9 @@ namespace libcamera {\n> >>>>>>>   * \\enum ColorSpace::Range\n> >>>>>>>   * \\brief The range (sometimes \"quantisation\") for this color space\n> >>>>>>>   *\n> >>>>>>> + * \\var ColorSpace::Range::Default\n> >>>>>>> + * Use the default range as defined by the driver\n> >>>>>>> + *\n> >>>>>>>   * \\var ColorSpace::Range::Full\n> >>>>>>>   * \\brief This color space uses full range pixel values\n> >>>>>>>   *\n> >>>>>>> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const\n> >>>>>>>    {\n> >>>>>>>           /* Print out a brief name only for standard color spaces. */\n> >>>>>>>\n> >>>>>>> -       static const std::array, 6> colorSpaceNames = { {\n> >>>>>>> +       static const std::array, 7> colorSpaceNames = { {\n> >>>>>>> +               { ColorSpace::Default, \"Default\" },\n> >>>>>>>                   { ColorSpace::Raw, \"RAW\" },\n> >>>>>>>                   { ColorSpace::Jpeg, \"JPEG\" },\n> >>>>>>>                   { ColorSpace::Srgb, \"sRGB\" },\n> >>>>>>> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const\n> >>>>>>>           /* Assemble a name made of the constituent fields. */\n> >>>>>>>\n> >>>>>>>           static const std::map primariesNames = {\n> >>>>>>> +               { Primaries::Default, \"Default\" },\n> >>>>>>>                   { Primaries::Raw, \"RAW\" },\n> >>>>>>>                   { Primaries::Smpte170m, \"SMPTE170M\" },\n> >>>>>>>                   { Primaries::Rec709, \"Rec709\" },\n> >>>>>>>                   { Primaries::Rec2020, \"Rec2020\" },\n> >>>>>>>           };\n> >>>>>>>           static const std::map transferNames = {\n> >>>>>>> +               { TransferFunction::Default, \"Default\" },\n> >>>>>>>                   { TransferFunction::Linear, \"Linear\" },\n> >>>>>>>                   { TransferFunction::Srgb, \"sRGB\" },\n> >>>>>>>                   { TransferFunction::Rec709, \"Rec709\" },\n> >>>>>>>           };\n> >>>>>>>           static const std::map encodingNames = {\n> >>>>>>> +               { YcbcrEncoding::Default, \"Default\" },\n> >>>>>>>                   { YcbcrEncoding::None, \"None\" },\n> >>>>>>>                   { YcbcrEncoding::Rec601, \"Rec601\" },\n> >>>>>>>                   { YcbcrEncoding::Rec709, \"Rec709\" },\n> >>>>>>>                   { YcbcrEncoding::Rec2020, \"Rec2020\" },\n> >>>>>>>           };\n> >>>>>>>           static const std::map rangeNames = {\n> >>>>>>> +               { Range::Default, \"Default\" },\n> >>>>>>>                   { Range::Full, \"Full\" },\n> >>>>>>>                   { Range::Limited, \"Limited\" },\n> >>>>>>>           };\n> >>>>>>> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional &colorSpace)\n> >>>>>>>   * \\brief The pixel range used with by color space\n> >>>>>>>   */\n> >>>>>>>\n> >>>>>>> +/**\n> >>>>>>> + * \\brief A constant representing a default color space\n> >>>>>>> + */\n> >>>>>>> +const ColorSpace ColorSpace::Default = {\n> >>>>>>> +       Primaries::Default,\n> >>>>>>> +       TransferFunction::Default,\n> >>>>>>> +       YcbcrEncoding::Default,\n> >>>>>>> +       Range::Default\n> >>>>>>> +};\n> >>>>>>> +\n> >>>>>>>  /**\n> >>>>>>>   * \\brief A constant representing a raw color space (from a sensor)\n> >>>>>>>   */\n> >>>>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>>>>>> index 3fc8438f..ecfcf337 100644\n> >>>>>>> --- a/src/libcamera/v4l2_device.cpp\n> >>>>>>> +++ b/src/libcamera/v4l2_device.cpp\n> >>>>>>> @@ -770,6 +770,7 @@ static const std::map v4l2ToRange = {\n> >>>>>>>  };\n> >>>>>>>\n> >>>>>>>  static const std::vector> colorSpaceToV4l2 = {\n> >>>>>>> +       { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },\n> >>>>>>>           { ColorSpace::Raw, V4L2_COLORSPACE_RAW },\n> >>>>>>>           { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },\n> >>>>>>>           { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },\n> >>>>>>> @@ -779,6 +780,7 @@ static const std::vector> colorSpaceToV4l\n> >>>>>>>  };\n> >>>>>>>\n> >>>>>>>  static const std::map primariesToV4l2 = {\n> >>>>>>> +       { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },\n> >>>>>>>           { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },\n> >>>>>>>           { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },\n> >>>>>>>           { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },\n> >>>>>>> @@ -786,18 +788,21 @@ static const std::map primariesToV4l2 =\n> >>>>>>>  };\n> >>>>>>>\n> >>>>>>>  static const std::map transferFunctionToV4l2 = {\n> >>>>>>> +       { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },\n> >>>>>>>           { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },\n> >>>>>>>           { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },\n> >>>>>>>           { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },\n> >>>>>>>  };\n> >>>>>>>\n> >>>>>>>  static const std::map ycbcrEncodingToV4l2 = {\n> >>>>>>> +       { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },\n> >>>>>>>           { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },\n> >>>>>>>           { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },\n> >>>>>>>           { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },\n> >>>>>>>  };\n> >>>>>>>\n> >>>>>>>  static const std::map rangeToV4l2 = {\n> >>>>>>> +       { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },\n> >>>>>>>           { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },\n> >>>>>>>           { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },\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 00121BE173\n\tfor ;\n\tFri, 29 Jul 2022 15:19:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2681A63312;\n\tFri, 29 Jul 2022 17:19:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2CD64603EC\n\tfor ;\n\tFri, 29 Jul 2022 17:19:15 +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 6D11B6D4;\n\tFri, 29 Jul 2022 17:19:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659107957;\n\tbh=+CviV2WMvZ/Po5TtSQgow7396a1FbrEbX36bs8xJ2Qo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=L+p4FqvAT6l8b+b78nMKd6QeNmqu02b14HxmRiI7vOkp0HQP9vjhn++xSPAVwDm5n\n\tEQJZrlkNlea/nXORCmiShdlLZ6NZ8Dho7peTQVwyPSUgG26x+ehQ9Y2cJAx8Yv68gr\n\t0uPQVCWmD6sDz2wPYJUVXfpkAYJ2ycu3naIx/NONJZ9cEFl6vV1iIdgzC99d/ZNwHR\n\tdcb2nJmoOr+CA1vogGqHDY+ie59+j/4x31haOjOJVGPlGUIDfMqraLKDlIZmTkfBbB\n\tPfaXLBB8pakcEWMAZU0eWXdwMj/B+F/qiRm5PufdoR15w9iOmi7N0okVHOKv4Emnft\n\t5lrs50vwydvkg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659107954;\n\tbh=+CviV2WMvZ/Po5TtSQgow7396a1FbrEbX36bs8xJ2Qo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZkCm72JDru6GwgFwm32RRfcXyrUxWOIAmC023GXBm5gEUNBHwPt5EvUNlkyK621Mn\n\t1EPZuk2/kKm2qvfhOx8jIyxkmNJ2qtMqFpOENDJ4srmdqbF84lYYqVX1nYNwcZjNPv\n\tkhJ5+VpHMf9XSvxgSb0k8YdxdCYEZ73teCjYqwmo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZkCm72JD\"; dkim-atps=neutral","Date":"Fri, 29 Jul 2022 18:19:11 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220724144355.104978-1-umang.jain@ideasonboard.com>\n\t<20220724144355.104978-3-umang.jain@ideasonboard.com>\n\t\n\t<28b3d9bb-8bf7-57b3-77ad-f55dde519c44@ideasonboard.com>\n\t\n\t<72bd1d6c-21f2-61df-ff17-5a9f2447c6ee@ideasonboard.com>\n\t\n\t\n\t<492dfd6c-908a-0caf-0a4b-a2d80885a518@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<492dfd6c-908a-0caf-0a4b-a2d80885a518@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a\n\tdefault colorspace","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"vedantparanjape160201@gmail.com, rishikeshdonadkar@gmail.com,\n\tlibcamera devel ,\n\tHans Verkuil , nicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]