[{"id":21122,"web_url":"https://patchwork.libcamera.org/comment/21122/","msgid":"","date":"2021-11-23T11:43:09","subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nOne clarification below please,\n\nOn 11/18/21 8:49 PM, David Plowman wrote:\n> This method forces raw streams to have the \"raw\" color space, and also\n> optionally makes all output streams to share the same color space as\n> some platforms may require this.\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/camera.h | 2 ++\n> src/libcamera/camera.cpp | 51 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 53 insertions(+)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 601ee46e..fdab4410 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -69,6 +69,8 @@ public:\n> protected:\n> \tCameraConfiguration();\n> \n> +\tStatus validateColorSpaces(bool sharedColorSpace);\n> +\n> \tstd::vector config_;\n> };\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 400a7cf0..c1541685 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -20,6 +20,7 @@\n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> \n> /**\n> @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const\n> \treturn config_.size();\n> }\n> \n> +static bool isRaw(const PixelFormat &pixFmt)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.isValid() &&\n> +\t info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +}\n> +\n> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)\n> +{\n> +\tStatus status = Valid;\n> +\n> +\t/* Find the largest non-raw stream (if any). */\n> +\tint index = -1;\n> +\tfor (unsigned int i = 0; i < config_.size(); i++) {\n> +\t\tconst StreamConfiguration &cfg = config_[i];\n> +\t\tif (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))\n> +\t\t\tindex = i;\n> +\t}\n> +\n> +\t/*\n> +\t * Here we force raw streams to the correct color space. We handle\n> +\t * the case where all output streams are to share a color space,\n> +\t * which will match the color space of the largest (non-raw) stream.\n> +\t */\n\n\nI am questioning this policy, whether or not, let it best be left to \nspecific platform instead? Or let me take a step back and ask how does \none arrive at this policy or is it somewhat a convention?\n\n> +\tfor (auto &cfg : config_) {\n> +\t\tstd::optional initialColorSpace = cfg.colorSpace;\n> +\n> +\t\tif (isRaw(cfg.pixelFormat))\n> +\t\t\tcfg.colorSpace = ColorSpace::Raw;\n> +\t\telse if (sharedColorSpace) {\n> +\t\t\t/*\n> +\t\t\t * When all outputs share a color space, use the biggest\n> +\t\t\t * output if that was set. If that was blank, but this\n> +\t\t\t * stream's is not, then use this stream's color space for\n> +\t\t\t * largest stream.\n> +\t\t\t * (index must be != -1 if there are any non-raw streams.)\n> +\t\t\t */\n> +\t\t\tif (config_[index].colorSpace)\n> +\t\t\t\tcfg.colorSpace = config_[index].colorSpace;\n> +\t\t\telse if (cfg.colorSpace)\n> +\t\t\t\tconfig_[index].colorSpace = cfg.colorSpace;\n> +\t\t}\n> +\n> +\t\tif (cfg.colorSpace != initialColorSpace)\n> +\t\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> /**\n> * \\var CameraConfiguration::transform\n> * \\brief User-specified transform to be applied to the image","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 F1889BDB13\n\tfor ;\n\tTue, 23 Nov 2021 11:43:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5088D60230;\n\tTue, 23 Nov 2021 12:43:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C36C060121\n\tfor ;\n\tTue, 23 Nov 2021 12:43:16 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BB2FA1B;\n\tTue, 23 Nov 2021 12:43:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EvO2q62W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637667796;\n\tbh=RPwHCbhUR5KturfnT6+9j++oMR6rwTJTxj5lBl/gi8A=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=EvO2q62W+F1jYv3diRDnrV25SMw0znV5W1uXM0BtCE5lNln7SCec0K8u8lkJ4Onfm\n\tnF//jL27cd7HecrhI//x+3CUvWSDbZF0jCPSTfJ16zZAbrXPmcyy7R2wAZuqhYkHte\n\tc3th30YTsB51qM0GGI1WZ/3Ur/+PVELShjKOUgd8=","To":"David Plowman ,\n\tlaurent.pinchart@ideasonboard.com, kieran.bingham@ideasonboard.com,\n\thverkuil-cisco@xs4all.nl, tfiga@google.com, jacopo@jmondi.org,\n\tnaush@raspberrypi.com, libcamera-devel@lists.libcamera.org","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-7-david.plowman@raspberrypi.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 23 Nov 2021 17:13:09 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211118151933.15627-7-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21195,"web_url":"https://patchwork.libcamera.org/comment/21195/","msgid":"","date":"2021-11-24T12:06:43","subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","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 the question below!\n\nOn Tue, 23 Nov 2021 at 11:43, Umang Jain wrote:\n>\n> Hi David,\n>\n> One clarification below please,\n>\n> On 11/18/21 8:49 PM, David Plowman wrote:\n> > This method forces raw streams to have the \"raw\" color space, and also\n> > optionally makes all output streams to share the same color space as\n> > some platforms may require this.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/camera.h | 2 ++\n> > src/libcamera/camera.cpp | 51 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 53 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 601ee46e..fdab4410 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -69,6 +69,8 @@ public:\n> > protected:\n> > CameraConfiguration();\n> >\n> > + Status validateColorSpaces(bool sharedColorSpace);\n> > +\n> > std::vector config_;\n> > };\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 400a7cf0..c1541685 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -20,6 +20,7 @@\n> >\n> > #include \"libcamera/internal/camera.h\"\n> > #include \"libcamera/internal/camera_controls.h\"\n> > +#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > /**\n> > @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const\n> > return config_.size();\n> > }\n> >\n> > +static bool isRaw(const PixelFormat &pixFmt)\n> > +{\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > + return info.isValid() &&\n> > + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > +}\n> > +\n> > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)\n> > +{\n> > + Status status = Valid;\n> > +\n> > + /* Find the largest non-raw stream (if any). */\n> > + int index = -1;\n> > + for (unsigned int i = 0; i < config_.size(); i++) {\n> > + const StreamConfiguration &cfg = config_[i];\n> > + if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))\n> > + index = i;\n> > + }\n> > +\n> > + /*\n> > + * Here we force raw streams to the correct color space. We handle\n> > + * the case where all output streams are to share a color space,\n> > + * which will match the color space of the largest (non-raw) stream.\n> > + */\n>\n>\n> I am questioning this policy, whether or not, let it best be left to\n> specific platform instead? Or let me take a step back and ask how does\n> one arrive at this policy or is it somewhat a convention?\n\nYes, it's a good point. I think it's mostly a platform-specific thing.\nOn the other hand, I can imagine that at least some other platforms\nwill be like the Pi in this respect (having a single output colour\nspace, though more than one output stream). So that's why I decided to\nput the code somewhere common, where pipeline handlers can call it if\nthey want.\n\nIf there are other behaviours or constraints that we think might be\n\"common\", maybe we could add those too?\n\nBest regards\nDavid\n\n>\n> > + for (auto &cfg : config_) {\n> > + std::optional initialColorSpace = cfg.colorSpace;\n> > +\n> > + if (isRaw(cfg.pixelFormat))\n> > + cfg.colorSpace = ColorSpace::Raw;\n> > + else if (sharedColorSpace) {\n> > + /*\n> > + * When all outputs share a color space, use the biggest\n> > + * output if that was set. If that was blank, but this\n> > + * stream's is not, then use this stream's color space for\n> > + * largest stream.\n> > + * (index must be != -1 if there are any non-raw streams.)\n> > + */\n> > + if (config_[index].colorSpace)\n> > + cfg.colorSpace = config_[index].colorSpace;\n> > + else if (cfg.colorSpace)\n> > + config_[index].colorSpace = cfg.colorSpace;\n> > + }\n> > +\n> > + if (cfg.colorSpace != initialColorSpace)\n> > + status = Adjusted;\n> > + }\n> > +\n> > + return status;\n> > +}\n> > +\n> > /**\n> > * \\var CameraConfiguration::transform\n> > * \\brief User-specified transform to be applied to the image","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 C1037BDB13\n\tfor ;\n\tWed, 24 Nov 2021 12:06:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1528C6038A;\n\tWed, 24 Nov 2021 13:06:58 +0100 (CET)","from mail-wr1-x431.google.com (mail-wr1-x431.google.com\n\t[IPv6:2a00:1450:4864:20::431])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8E2E60128\n\tfor ;\n\tWed, 24 Nov 2021 13:06:56 +0100 (CET)","by mail-wr1-x431.google.com with SMTP id d24so3866288wra.0\n\tfor ;\n\tWed, 24 Nov 2021 04:06:56 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"sG8et27d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=4arVLHDPrjyMslVB1m/fYM09L9QB8E6nxs9lHpDg3GA=;\n\tb=sG8et27drREH7VjKlrAmmIOrXUVlgaHvlxqGD54Wdn1i3YlA2tYFfqak5KCJrR1F3D\n\tOB7LQBEbt5wVry8iSlfYQ/maliCJJtA11nKoNMcnx/hULurYACS1s2auNvwaFK3puXhx\n\t4mC8MMw9Cg/uKEZ+zPhxN7HJGgsH4uu6YyPwpjQ5DvtBIvl8wjjItRhA7vWp7ONENjQl\n\tZXlYkxd699j9043s5mC1hU4Ve3TNGCV/OuVqzFjVuu4Kk+PoSQYw4uwKOdoCwn0P9UG3\n\tejMkfwTwzIpqeUTcwVTdn2LHycnk4yRP3fd1rKD+JPp80qUQa6eFAnL76LmnDottrcKK\n\taOKg==","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=4arVLHDPrjyMslVB1m/fYM09L9QB8E6nxs9lHpDg3GA=;\n\tb=ND2vDiJ5lOaCpWjpSGhOlkEf09Q1pE5iGAhal0qV3A4z7XQxRk8HrbRHnWjqJvbqop\n\tKuXZU9EvgPG3ETURlo1MwZqn9JENfpzpqBZx099u+dbnsvGWhZDXTZGjPck/OQLEsj5l\n\tj0C+R/sub0E4tyrgRXNXyEwqRM06sJ4LvEHEiQ5vMJm69DeCKerwSGXD73DBxoRvQy2k\n\tWmuBSKC7Q8eCgwKZK/fViGtVY/lfCSQTiPKRd84mf9NHkHRxPg1F6wzORun5S+cs+ncx\n\tO8OPDG/sYq2trKMYkOS/k59z4G55NQiWItvoG23cXbB26mF2/uF/YCn8iVHg9KpMbiPL\n\t/Mng==","X-Gm-Message-State":"AOAM532lJIJQAcncD4oC/5IBROvxkmwa6XO/vIkh6qtWZKNDqHM4KrWa\n\t1tHM1HLkdmsEtz0oNiE+Z8KKwJ68X39E0NgKZfOZoQ==","X-Google-Smtp-Source":"ABdhPJw2f1afJ9CHK6ZeFrXV75QxzvaJlKIk7oN8yK/duqyDI4+jXoByWG9GCnptCyTE2N+CGxAwHdIFu2MiWeBzTIE=","X-Received":"by 2002:a5d:4846:: with SMTP id\n\tn6mr17491689wrs.249.1637755614241; \n\tWed, 24 Nov 2021 04:06:54 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-7-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 24 Nov 2021 12:06:43 +0000","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21211,"web_url":"https://patchwork.libcamera.org/comment/21211/","msgid":"<20211124235602.jl5mzj2smgxpptpo@uno.localdomain>","date":"2021-11-24T23:56:02","subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Thu, Nov 18, 2021 at 03:19:32PM +0000, David Plowman wrote:\n> This method forces raw streams to have the \"raw\" color space, and also\n> optionally makes all output streams to share the same color space as\n> some platforms may require this.\n\nI feel like it's a bit early, we don't have much platforms supporting\ncolorspaces and it might be the requirements are different ?\n\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/camera.h | 2 ++\n> src/libcamera/camera.cpp | 51 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 53 insertions(+)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 601ee46e..fdab4410 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -69,6 +69,8 @@ public:\n> protected:\n> \tCameraConfiguration();\n>\n> +\tStatus validateColorSpaces(bool sharedColorSpace);\n> +\n> \tstd::vector config_;\n> };\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 400a7cf0..c1541685 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -20,6 +20,7 @@\n>\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> /**\n> @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const\n> \treturn config_.size();\n> }\n>\n> +static bool isRaw(const PixelFormat &pixFmt)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.isValid() &&\n> +\t info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +}\n\nI've seen this pattern so many times I'm surprised we don't have an\nhelper\n\n> +\n> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)\n\nShouldn't this be documented ?\n\n> +{\n> +\tStatus status = Valid;\n> +\n> +\t/* Find the largest non-raw stream (if any). */\n> +\tint index = -1;\n> +\tfor (unsigned int i = 0; i < config_.size(); i++) {\n\nfor (auto &[i, cfg] utils::enumerate(config_)) is nicer\n\n> +\t\tconst StreamConfiguration &cfg = config_[i];\n> +\t\tif (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))\n> +\t\t\tindex = i;\n> +\t}\n\nIf you make sure the selected index has a colorspace the rest of the\nfunction can be simplified\n\n StreamConfiguration largerYUV;\n for (auto const &cfg : configs_) {\n if (isRaw(cfg.pixelformat))\n continue;\n\n if (cfg.size > largerYUV.size && cfg.colorSpace)\n largerYUV = cfg;\n }\n\n if (!largerYUV.colorSpace)\n ...\n\nAnd you could catch the case where there are no colorspaces at all\nearlier (what happens in that case ?)\n\n\n> +\n> +\t/*\n> +\t * Here we force raw streams to the correct color space. We handle\n> +\t * the case where all output streams are to share a color space,\n> +\t * which will match the color space of the largest (non-raw) stream.\n> +\t */\n> +\tfor (auto &cfg : config_) {\n> +\t\tstd::optional initialColorSpace = cfg.colorSpace;\n> +\n> +\t\tif (isRaw(cfg.pixelFormat))\n> +\t\t\tcfg.colorSpace = ColorSpace::Raw;\n\n\t\tif (isRaw(cfg.pixelFormat)) {\n if (cfg.colorSpace == ColorSpace::Raw)\n continue;\n\n\t\t\tcfg.colorSpace = ColorSpace::Raw;\n status = Adjusted;\n continue;\n }\n\n> +\t\telse if (sharedColorSpace) {\n\nthe rest of the function can then be\n\n if (!sharedColorSpace)\n continue;\n\n if (cfg->colorSpace == largerYUV.colorSpace)\n continue;\n\n cfg.colorspace = largerYUV.colorspace\n status = Adjusted;\n }\n\n return status;\n\nAgain not tested, I might have missed something indeed\n\nI would also call the \"sharedColorSpace\" flag \"forceColorSpace\" ?\n\n> +\t\t\t/*\n> +\t\t\t * When all outputs share a color space, use the biggest\n> +\t\t\t * output if that was set. If that was blank, but this\n> +\t\t\t * stream's is not, then use this stream's color space for\n> +\t\t\t * largest stream.\n> +\t\t\t * (index must be != -1 if there are any non-raw streams.)\n> +\t\t\t */\n> +\t\t\tif (config_[index].colorSpace)\n\nCan index be -1 here ?\n\n> +\t\t\t\tcfg.colorSpace = config_[index].colorSpace;\n> +\t\t\telse if (cfg.colorSpace)\n\nThanks\n j\n\n> +\t\t\t\tconfig_[index].colorSpace = cfg.colorSpace;\n> +\t\t}\n> +\n> +\t\tif (cfg.colorSpace != initialColorSpace)\n> +\t\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> /**\n> * \\var CameraConfiguration::transform\n> * \\brief User-specified transform to be applied to the image\n> --\n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7F595BDB13\n\tfor ;\n\tWed, 24 Nov 2021 23:55:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1185060371;\n\tThu, 25 Nov 2021 00:55:12 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D557260228\n\tfor ;\n\tThu, 25 Nov 2021 00:55:10 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 4B00D1BF207;\n\tWed, 24 Nov 2021 23:55:09 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 00:56:02 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211124235602.jl5mzj2smgxpptpo@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-7-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211118151933.15627-7-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\thverkuil-cisco@xs4all.nl","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21215,"web_url":"https://patchwork.libcamera.org/comment/21215/","msgid":"<5c3058ee-b5cd-d824-c650-d67916a1d18a@ideasonboard.com>","date":"2021-11-25T04:26:16","subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nOn 11/24/21 5:36 PM, David Plowman wrote:\n> Hi Umang\n>\n> Thanks for the question below!\n>\n> On Tue, 23 Nov 2021 at 11:43, Umang Jain wrote:\n>> Hi David,\n>>\n>> One clarification below please,\n>>\n>> On 11/18/21 8:49 PM, David Plowman wrote:\n>>> This method forces raw streams to have the \"raw\" color space, and also\n>>> optionally makes all output streams to share the same color space as\n>>> some platforms may require this.\n>>>\n>>> Signed-off-by: David Plowman \n>>> ---\n>>> include/libcamera/camera.h | 2 ++\n>>> src/libcamera/camera.cpp | 51 ++++++++++++++++++++++++++++++++++++++\n>>> 2 files changed, 53 insertions(+)\n>>>\n>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n>>> index 601ee46e..fdab4410 100644\n>>> --- a/include/libcamera/camera.h\n>>> +++ b/include/libcamera/camera.h\n>>> @@ -69,6 +69,8 @@ public:\n>>> protected:\n>>> CameraConfiguration();\n>>>\n>>> + Status validateColorSpaces(bool sharedColorSpace);\n>>> +\n>>> std::vector config_;\n>>> };\n>>>\n>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>> index 400a7cf0..c1541685 100644\n>>> --- a/src/libcamera/camera.cpp\n>>> +++ b/src/libcamera/camera.cpp\n>>> @@ -20,6 +20,7 @@\n>>>\n>>> #include \"libcamera/internal/camera.h\"\n>>> #include \"libcamera/internal/camera_controls.h\"\n>>> +#include \"libcamera/internal/formats.h\"\n>>> #include \"libcamera/internal/pipeline_handler.h\"\n>>>\n>>> /**\n>>> @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const\n>>> return config_.size();\n>>> }\n>>>\n>>> +static bool isRaw(const PixelFormat &pixFmt)\n>>> +{\n>>> + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n>>> + return info.isValid() &&\n>>> + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>>> +}\n>>> +\n>>> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)\n>>> +{\n>>> + Status status = Valid;\n>>> +\n>>> + /* Find the largest non-raw stream (if any). */\n>>> + int index = -1;\n>>> + for (unsigned int i = 0; i < config_.size(); i++) {\n>>> + const StreamConfiguration &cfg = config_[i];\n>>> + if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))\n>>> + index = i;\n>>> + }\n>>> +\n>>> + /*\n>>> + * Here we force raw streams to the correct color space. We handle\n>>> + * the case where all output streams are to share a color space,\n>>> + * which will match the color space of the largest (non-raw) stream.\n>>> + */\n>>\n>> I am questioning this policy, whether or not, let it best be left to\n>> specific platform instead? Or let me take a step back and ask how does\n>> one arrive at this policy or is it somewhat a convention?\n> Yes, it's a good point. I think it's mostly a platform-specific thing.\n> On the other hand, I can imagine that at least some other platforms\n> will be like the Pi in this respect (having a single output colour\n> space, though more than one output stream). So that's why I decided to\n> put the code somewhere common, where pipeline handlers can call it if\n> they want.\n>\n> If there are other behaviours or constraints that we think might be\n> \"common\", maybe we could add those too?\n\n\nI see, it's fine for now, I can think pipeline-handler can chose to do \ntheir own thing, when diverged cases arrive or spotted. As Jacopo said, \nit seems a bit-early but also seem okay to me.\n\n> Best regards\n> David\n>\n>>> + for (auto &cfg : config_) {\n>>> + std::optional initialColorSpace = cfg.colorSpace;\n>>> +\n>>> + if (isRaw(cfg.pixelFormat))\n>>> + cfg.colorSpace = ColorSpace::Raw;\n>>> + else if (sharedColorSpace) {\n>>> + /*\n>>> + * When all outputs share a color space, use the biggest\n>>> + * output if that was set. If that was blank, but this\n>>> + * stream's is not, then use this stream's color space for\n>>> + * largest stream.\n>>> + * (index must be != -1 if there are any non-raw streams.)\n>>> + */\n>>> + if (config_[index].colorSpace)\n>>> + cfg.colorSpace = config_[index].colorSpace;\n>>> + else if (cfg.colorSpace)\n>>> + config_[index].colorSpace = cfg.colorSpace;\n>>> + }\n>>> +\n>>> + if (cfg.colorSpace != initialColorSpace)\n>>> + status = Adjusted;\n>>> + }\n>>> +\n>>> + return status;\n>>> +}\n>>> +\n>>> /**\n>>> * \\var CameraConfiguration::transform\n>>> * \\brief User-specified transform to be applied to the image","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 ABAAFBDB13\n\tfor ;\n\tThu, 25 Nov 2021 04:26:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D95B360233;\n\tThu, 25 Nov 2021 05:26:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C1336011E\n\tfor ;\n\tThu, 25 Nov 2021 05:26:23 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E0D7F90E;\n\tThu, 25 Nov 2021 05:26:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WfexYBC7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637814382;\n\tbh=tDc2jXENpICmdtRjlXNi3AhdcJqGJTc11FpUVKC5e2I=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=WfexYBC7iOtcMCAZbNZJHw+sJ270nlpNKEWGvYVfcN2Su3uZtua3qwC7D9xej3KZH\n\te4cH9+cJR+g7JcA5FlYbJDzYhktwfZSfbQbEStUe8pA7O6KGG4KkUv59Z13HCBLqN9\n\tsh+TfdZRPvIP/G2yh3o8OC8Pv9aIqFwU/8GPnKUA=","To":"David Plowman ","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-7-david.plowman@raspberrypi.com>\n\t\n\t","From":"Umang Jain ","Message-ID":"<5c3058ee-b5cd-d824-c650-d67916a1d18a@ideasonboard.com>","Date":"Thu, 25 Nov 2021 09:56:16 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21247,"web_url":"https://patchwork.libcamera.org/comment/21247/","msgid":"","date":"2021-11-25T12:58:44","subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for reviewing this.\n\nOn Wed, 24 Nov 2021 at 23:55, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Thu, Nov 18, 2021 at 03:19:32PM +0000, David Plowman wrote:\n> > This method forces raw streams to have the \"raw\" color space, and also\n> > optionally makes all output streams to share the same color space as\n> > some platforms may require this.\n>\n> I feel like it's a bit early, we don't have much platforms supporting\n> colorspaces and it might be the requirements are different ?\n\nI added this function because - a very long time ago now! - there had\nbeen a worry about different pipeline handlers writing their own bits\nof code, so I wanted to create something that people could share. But\nI agree that the jury is still out on how useful this is.\n\n>\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/camera.h | 2 ++\n> > src/libcamera/camera.cpp | 51 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 53 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 601ee46e..fdab4410 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -69,6 +69,8 @@ public:\n> > protected:\n> > CameraConfiguration();\n> >\n> > + Status validateColorSpaces(bool sharedColorSpace);\n> > +\n> > std::vector config_;\n> > };\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 400a7cf0..c1541685 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -20,6 +20,7 @@\n> >\n> > #include \"libcamera/internal/camera.h\"\n> > #include \"libcamera/internal/camera_controls.h\"\n> > +#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > /**\n> > @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const\n> > return config_.size();\n> > }\n> >\n> > +static bool isRaw(const PixelFormat &pixFmt)\n> > +{\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > + return info.isValid() &&\n> > + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > +}\n>\n> I've seen this pattern so many times I'm surprised we don't have an\n> helper\n>\n> > +\n> > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)\n>\n> Shouldn't this be documented ?\n\nOf course, I must have forgotten.\n\n>\n> > +{\n> > + Status status = Valid;\n> > +\n> > + /* Find the largest non-raw stream (if any). */\n> > + int index = -1;\n> > + for (unsigned int i = 0; i < config_.size(); i++) {\n>\n> for (auto &[i, cfg] utils::enumerate(config_)) is nicer\n\nOoh, it gets ever more like Python! I didn't know you could do that...\n\n>\n> > + const StreamConfiguration &cfg = config_[i];\n> > + if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))\n> > + index = i;\n> > + }\n>\n> If you make sure the selected index has a colorspace the rest of the\n> function can be simplified\n>\n> StreamConfiguration largerYUV;\n> for (auto const &cfg : configs_) {\n> if (isRaw(cfg.pixelformat))\n> continue;\n>\n> if (cfg.size > largerYUV.size && cfg.colorSpace)\n> largerYUV = cfg;\n> }\n>\n> if (!largerYUV.colorSpace)\n> ...\n>\n> And you could catch the case where there are no colorspaces at all\n> earlier (what happens in that case ?)\n>\n>\n> > +\n> > + /*\n> > + * Here we force raw streams to the correct color space. We handle\n> > + * the case where all output streams are to share a color space,\n> > + * which will match the color space of the largest (non-raw) stream.\n> > + */\n> > + for (auto &cfg : config_) {\n> > + std::optional initialColorSpace = cfg.colorSpace;\n> > +\n> > + if (isRaw(cfg.pixelFormat))\n> > + cfg.colorSpace = ColorSpace::Raw;\n>\n> if (isRaw(cfg.pixelFormat)) {\n> if (cfg.colorSpace == ColorSpace::Raw)\n> continue;\n>\n> cfg.colorSpace = ColorSpace::Raw;\n> status = Adjusted;\n> continue;\n> }\n>\n> > + else if (sharedColorSpace) {\n>\n> the rest of the function can then be\n>\n> if (!sharedColorSpace)\n> continue;\n>\n> if (cfg->colorSpace == largerYUV.colorSpace)\n> continue;\n>\n> cfg.colorspace = largerYUV.colorspace\n> status = Adjusted;\n> }\n>\n> return status;\n>\n> Again not tested, I might have missed something indeed\n>\n> I would also call the \"sharedColorSpace\" flag \"forceColorSpace\" ?\n\nWith \"forceColorSpace\" I'm left wondering what I am \"forcing\"... at\nleast \"sharedColorSpace\" implies that some colour spaces are somehow\nbeing shared. Perhaps we could have \"shareOutputColorSpaces\" if that's\nnot too long? That's 100% clear, I think!\n\n>\n> > + /*\n> > + * When all outputs share a color space, use the biggest\n> > + * output if that was set. If that was blank, but this\n> > + * stream's is not, then use this stream's color space for\n> > + * largest stream.\n> > + * (index must be != -1 if there are any non-raw streams.)\n> > + */\n> > + if (config_[index].colorSpace)\n>\n> Can index be -1 here ?\n>\n\nI'll have another look at all this and see if I can make it all a bit\n\"more obvious\".\n\nThanks!\nDavid\n\n> > + cfg.colorSpace = config_[index].colorSpace;\n> > + else if (cfg.colorSpace)\n>\n> Thanks\n> j\n>\n> > + config_[index].colorSpace = cfg.colorSpace;\n> > + }\n> > +\n> > + if (cfg.colorSpace != initialColorSpace)\n> > + status = Adjusted;\n> > + }\n> > +\n> > + return status;\n> > +}\n> > +\n> > /**\n> > * \\var CameraConfiguration::transform\n> > * \\brief User-specified transform to be applied to the image\n> > --\n> > 2.20.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E527CBDB13\n\tfor ;\n\tThu, 25 Nov 2021 12:58:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22EC86036F;\n\tThu, 25 Nov 2021 13:58:57 +0100 (CET)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D567C60231\n\tfor ;\n\tThu, 25 Nov 2021 13:58:55 +0100 (CET)","by mail-wr1-x42f.google.com with SMTP id a18so11454643wrn.6\n\tfor ;\n\tThu, 25 Nov 2021 04:58:55 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"NjfSjrJt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=xP3TMOmO7A69C0Vo7UtAx4aGZ+mC/lmT4EdHGcJTlG4=;\n\tb=NjfSjrJtPm1yN1rMv0eNBqcEX4fI2Se8jshkrUCrWXodWWcht/9Mkt3+trCAgYcygH\n\tSgzL5a5bNPYSLjz96qXx7GikMRPRZc+I5O/UToVX6CFNofu7jRMSag4I0wUyrXSgNmwS\n\tF/4Aqh0FIYdHckqd/V0jmAxMQJCUNV5JgLLOP5aylP4tQCEicd2Z8iVELVpPE/2MXZ1x\n\tGjIULP9Ne7PUr3oibn1e0Piho7fB0bupnVzs12nyFXJID5MKLnhk6S9FsDw2wg82+y7o\n\tHkxaT27aWfsZaXWXoTx24yu+ZZvhVVXmf+Whi0tqLnCea5kMpBipLXe1X8Z8wWU2L1u2\n\t3teQ==","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=xP3TMOmO7A69C0Vo7UtAx4aGZ+mC/lmT4EdHGcJTlG4=;\n\tb=bHGAziJPuaeE6Jc+Ix8oipduQ4mQf2kbYDvsWWJHnc8baq1CsZvr0qaOhp5bREmX3m\n\tyr5EvkUgFLsxVDKpZ8MHp97KrwEbn+bLjbmJWNWyYsZjZyZVripFvG6ROQLg4LNMLCCm\n\tcY7YoCm616jbAJRWmxmNGjCP2p47D1tqcs0jZgt362aoXtz7pBcuA+weKMeQDh+HrM9J\n\tNq7n9tGMlSFwd7yQenkwUag5yks1vQgr7Si6H02OgBzS8hHojuUsD/82NIr052pb/o5r\n\tPTxVT4lAauVgz2rPCNhONwaQI0RAQMrs0ToT8k8DKJIIgoqLbKPZb07YuzDPWtoS223l\n\tMfdQ==","X-Gm-Message-State":"AOAM532KlVbRsmfe6WahWKmOee3fA0FDh8pKbSAYj/BJ1BfUkG7cYSCD\n\tLtNYS1kvvu952M302Jy9nZwewYp6cT4HmI1+/Syq+Q==","X-Google-Smtp-Source":"ABdhPJw+FtwYiaDu56QMKRA8/S6btQoV19uujbIXKhFDH4Lrbw1E/OSrWvZJubZXqFFpEyUG+2ThIeZg1gw3YOaUiBk=","X-Received":"by 2002:adf:e390:: with SMTP id\n\te16mr6240789wrm.494.1637845135317; \n\tThu, 25 Nov 2021 04:58:55 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-7-david.plowman@raspberrypi.com>\n\t<20211124235602.jl5mzj2smgxpptpo@uno.localdomain>","In-Reply-To":"<20211124235602.jl5mzj2smgxpptpo@uno.localdomain>","From":"David Plowman ","Date":"Thu, 25 Nov 2021 12:58:44 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]