[{"id":20685,"web_url":"https://patchwork.libcamera.org/comment/20685/","msgid":"<163611375595.275423.15648154330177579284@Monstersaurus>","date":"2021-11-05T12:02:35","subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nQuoting David Plowman (2021-11-04 13:58:00)\n> This is so that applications can choose appropriate color spaces which\n> will then be passed down to the V4L2 devices.\n> \n> There are two fields: the \"requestedColorSpace\" which is the one an\n> application wants, and the \"actualColorSpace\" which is what the\n> underlying hardware can deliver, and which is filled in by the\n> validate() method.\n\nIs it necessary to store both? We don't do that with the other fields...\nthe StreamConfiguration structure is used in different states to handle\nthat I think.\n\nWhen a CameraConfiguration is 'generated' Potential StreamConfigurations\nare 'offered' to the application.\n\nApplications can either fill in those StreamConfigurations, (or start\nfrom an empty one if it will set all fields anyway...) and 'validate'\nthe overall CameraConfiguration.\n\nThe StreamConfiguration at that point contains the 'requested' values\n... and when returned back from validate() - it has the values that can\nbe used by hardware...\n\n\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/stream.h | 4 ++++\n> src/libcamera/stream.cpp | 25 +++++++++++++++++++++++++\n> 2 files changed, 29 insertions(+)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 0c55e716..fe491ff5 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -47,6 +48,9 @@ struct StreamConfiguration {\n> \n> unsigned int bufferCount;\n> \n> + ColorSpace requestedColorSpace;\n> + ColorSpace actualColorSpace;\n\nGiven the above, I think this should be just colorSpace;\n\n> +\n> Stream *stream() const { return stream_; }\n> void setStream(Stream *stream) { stream_ = stream; }\n> const StreamFormats &formats() const { return formats_; }\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index b421e17e..1ddbbb8c 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -329,6 +329,31 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> * \\brief Requested number of buffers to allocate for the stream\n> */\n> \n> +/**\n> + * \\var StreamConfiguration::requestedColorSpace\n> + * \\brief The ColorSpace this stream should have\n> + *\n> + * The generateConfiguration method will generate reasonable default\n> + * values (ColorSpace::Jpeg for stills, ColorSpace::Rec709 for video and\n> + * ColorSpace::Raw for raw streams) but applications are free to overwrite\n> + * this value.\n> + */\n> +\n> +/**\n> + * \\var StreamConfiguration::actualColorSpace\n> + * \\brief The ColorSpace the will be used for this stream\n> + *\n> + * This field is filled in by CameraConfiguration::validate().\n> + * Normally this should match the requestedColorSpace, but it may differ\n> + * if the hardware does not support it.\n> + *\n> + * In general cameras may have different constraints here, for example,\n> + * they may require all output streams to share the same color space.\n> + * Sometimes the fields within this color space may report \"Undefined\"\n> + * values if the hardware drivers are going to use a color space that\n> + * is not recognised by the ColorSpace class.\n> + */\n> +\n> /**\n> * \\fn StreamConfiguration::stream()\n> * \\brief Retrieve the stream associated with the configuration\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 E2680BF415\n\tfor ;\n\tFri, 5 Nov 2021 12:02:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1629C6034A;\n\tFri, 5 Nov 2021 13:02:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9897600B8\n\tfor ;\n\tFri, 5 Nov 2021 13:02:38 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 633A51908;\n\tFri, 5 Nov 2021 13:02:38 +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=\"qA4xNigq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636113758;\n\tbh=61hNPe8+6AdagJ+PskHgs7fGx8027CQASnQHda1ITjQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=qA4xNigqMI1xGb+Bnz46AY1VZapkpQGJlRE/n0E+7IV75v4RDGwNyI0lu5JLu6Anu\n\tJ91l/pBGQCMDpbKfeaQIGXGy3BuPrEkwh6sdIViiDt3t5rQ9hBCVua0A+XtcfEsLaB\n\twRIFXhR5/W8VL155oXrUbGxBJu3k3aCJu/VmEdO0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211104135805.5269-3-david.plowman@raspberrypi.com>","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-3-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 05 Nov 2021 12:02:35 +0000","Message-ID":"<163611375595.275423.15648154330177579284@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","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":20694,"web_url":"https://patchwork.libcamera.org/comment/20694/","msgid":"","date":"2021-11-05T14:38:13","subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again Kieran\n\nSorry, forgot to \"reply all\" on my previous reply, hopefully folks can\nfind the missing email trail below!\n\nOn Fri, 5 Nov 2021 at 13:29, Kieran Bingham\n wrote:\n>\n> Quoting David Plowman (2021-11-05 13:16:44)\n> > Hi Kieran\n> >\n> > Thanks for looking at this patch set!\n> >\n> > On Fri, 5 Nov 2021 at 12:02, Kieran Bingham\n> > wrote:\n> > >\n> > > Hi David,\n> > >\n> > > Quoting David Plowman (2021-11-04 13:58:00)\n> > > > This is so that applications can choose appropriate color spaces which\n> > > > will then be passed down to the V4L2 devices.\n> > > >\n> > > > There are two fields: the \"requestedColorSpace\" which is the one an\n> > > > application wants, and the \"actualColorSpace\" which is what the\n> > > > underlying hardware can deliver, and which is filled in by the\n> > > > validate() method.\n> > >\n> > > Is it necessary to store both? We don't do that with the other fields...\n> > > the StreamConfiguration structure is used in different states to handle\n> > > that I think.\n> > >\n> > > When a CameraConfiguration is 'generated' Potential StreamConfigurations\n> > > are 'offered' to the application.\n> > >\n> > > Applications can either fill in those StreamConfigurations, (or start\n> > > from an empty one if it will set all fields anyway...) and 'validate'\n> > > the overall CameraConfiguration.\n> > >\n> > > The StreamConfiguration at that point contains the 'requested' values\n> > > ... and when returned back from validate() - it has the values that can\n> > > be used by hardware...\n> >\n> > Yes, I think this is one of the critical points that we need to agree\n> > on. So my reasoning was as follows:\n> >\n> > You specify the colour space that you want, but what if the driver\n> > gives you something else. Specifically, what if the driver gives us\n> > some wacky colour space that the ColorSpace class can't represent at\n> > all?\n> >\n> > It feels a bit onerous to insist that we have a representation for\n> > every colour space that we might ever encounter, particularly if there\n> > might be non-V4L2 things in future, so I've gone with the notion of an\n> > \"undefined\" colour space, which can be returned to us from the driver\n> > layer.\n> >\n> > Now this is a dangerous thing so I've explicitly forbidden this as a\n> > value that you can request (what would it mean? sounds like confusion\n> > all round). So the worry is that we make a reasonable request, but the\n> > driver changes it to \"undefined\", and now we can't call validate with\n> > the same configuration.\n>\n> I suspect that at that point it's up to the pipeline handler to provide\n> a reasonable color space?\n\nSo I think this is the crux of the matter. Do we want to insist that\nwe have a representation for every colour space that we might ever\nencounter? If we do, then yes - we can promise to pass a meaningful\nColorSpace back up the stack and we never have to worry about\nanything.\n\nBut colour spaces are in fact infinitely variable (unlike pixel\nformats), though in practice V4L2 doesn't represent them so we'll\nalmost never encounter any. For the moment, anyway. Do we want to\npretend we can add every colour space we will ever encounter? In\npractice it's probably true, but are we sure it will always be like\nthis?\n\n>\n> Can drivers really return an undefined colorspace? What does that mean?\n> (In the same regards as what does 'asking' for an undefined color space\n> mean?).\n\nNote that a colour space is never actually \"undefined\", the driver\nwill have chosen one, it just means \"we don't have a ColorSpace to\ndescribe what it chose\". So after validate() we could be told, \"it's\ngoing to use a colour space that I can't describe to you\", but at the\nsame time it's nonsensical to request a colour space \"that I can't\ndescribe\", which is why that is banned.\n\nI don't know if any of this helps. Perhaps we need a face-to-face.\n\nDavid\n\n>\n> > So my solution was to separate this into a \"requested\" and \"actual\"\n> > colour space. I'm imagining that the user either leaves the\n> > requestedColorSpace field alone, and they'll keep getting back\n> > \"adjusted\" if they call validate again. Or if they don't like that\n> > they could do something like\n> >\n> > if (actualColorSpace.isFullyDefined())\n> > requestedColorSpace = actualColorSpace;\n> >\n> > which will at least get rid of the \"adjusted\" return value in cases\n> > where that's possible. Am I making any sense, or are there better\n> > solutions?\n>\n> I think if userspace continues to ask for a color space which is not\n> supported (and somehow returns an undefined color space, rather than an\n> acceptable one), then we are probably into 'fail to configure'\n> territory.\n>\n> Is it any different from asking the hardware to give me YUYV, but it\n> always returns NV12? I either accept that the hardware can only do NV12,\n> and acknowledge that it's not going to be YUYV, or ... ?\n>\n>\n> > Thanks!\n> >\n> > David\n> >\n> > >\n> > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > include/libcamera/stream.h | 4 ++++\n> > > > src/libcamera/stream.cpp | 25 +++++++++++++++++++++++++\n> > > > 2 files changed, 29 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > > > index 0c55e716..fe491ff5 100644\n> > > > --- a/include/libcamera/stream.h\n> > > > +++ b/include/libcamera/stream.h\n> > > > @@ -12,6 +12,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -47,6 +48,9 @@ struct StreamConfiguration {\n> > > >\n> > > > unsigned int bufferCount;\n> > > >\n> > > > + ColorSpace requestedColorSpace;\n> > > > + ColorSpace actualColorSpace;\n> > >\n> > > Given the above, I think this should be just colorSpace;\n> > >\n> > > > +\n> > > > Stream *stream() const { return stream_; }\n> > > > void setStream(Stream *stream) { stream_ = stream; }\n> > > > const StreamFormats &formats() const { return formats_; }\n> > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > > > index b421e17e..1ddbbb8c 100644\n> > > > --- a/src/libcamera/stream.cpp\n> > > > +++ b/src/libcamera/stream.cpp\n> > > > @@ -329,6 +329,31 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> > > > * \\brief Requested number of buffers to allocate for the stream\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\var StreamConfiguration::requestedColorSpace\n> > > > + * \\brief The ColorSpace this stream should have\n> > > > + *\n> > > > + * The generateConfiguration method will generate reasonable default\n> > > > + * values (ColorSpace::Jpeg for stills, ColorSpace::Rec709 for video and\n> > > > + * ColorSpace::Raw for raw streams) but applications are free to overwrite\n> > > > + * this value.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var StreamConfiguration::actualColorSpace\n> > > > + * \\brief The ColorSpace the will be used for this stream\n> > > > + *\n> > > > + * This field is filled in by CameraConfiguration::validate().\n> > > > + * Normally this should match the requestedColorSpace, but it may differ\n> > > > + * if the hardware does not support it.\n> > > > + *\n> > > > + * In general cameras may have different constraints here, for example,\n> > > > + * they may require all output streams to share the same color space.\n> > > > + * Sometimes the fields within this color space may report \"Undefined\"\n> > > > + * values if the hardware drivers are going to use a color space that\n> > > > + * is not recognised by the ColorSpace class.\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\fn StreamConfiguration::stream()\n> > > > * \\brief Retrieve the stream associated with the configuration\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 A9DB7BDB1C\n\tfor ;\n\tFri, 5 Nov 2021 14:38:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFA066034F;\n\tFri, 5 Nov 2021 15:38:26 +0100 (CET)","from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com\n\t[IPv6:2a00:1450:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02158600B8\n\tfor ;\n\tFri, 5 Nov 2021 15:38:24 +0100 (CET)","by mail-wr1-x42e.google.com with SMTP id t30so14068871wra.10\n\tfor ;\n\tFri, 05 Nov 2021 07:38:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"p/aCl5ni\"; 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\tbh=V5nluUKVgWIB/pLtnt/Lyx3L9YsexQlkd7FwIeqCtCI=;\n\tb=p/aCl5nibl37Xdfcd9SflpQsk2v1jOj3si/CvPoLvR15F2PpRq7wgygwpwDPkyehtr\n\tXwqAzE5fFQrLMwZfOXpmP02Q/pvnNhxmaWn8HedsvAPoP1zeAzcHTLkfTO1/A3sd8e9E\n\treZ+YPlGFVu9hAsQtFz3AveBxunxuvvW1+FNWcBzXSB3evX3Lf9MINk6ptsnGTOnUhIs\n\t4wAB+QfOMkktvz2G9qCkRv1qdPNbs6R/HPytqyClta2qJkNcVlwAkZEJ4Y9ts7XCrP/L\n\tI2IY2Uu56GPeU6eJlmyT1uFcJfwhP++3OyB034RcJUjWIsSPi0gm89uN/GisbvIFVDM6\n\tUqiw==","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;\n\tbh=V5nluUKVgWIB/pLtnt/Lyx3L9YsexQlkd7FwIeqCtCI=;\n\tb=3XSwCmaIPL0Zzsf2T2ayieiuWbJrjTU7vB0SdysaPvoyAbZuQDjpFip95cUZlJX5gx\n\te40YhJCPBj+Ku2eI1znuD0Ve0IvNzlniIyvtt7805rNSkLAKKyBBbWe5weHFx24o58xC\n\tdBpJsQvxKRfKwnBTcaXELNeT2RU+I7W+6tu8yTZuJVIWg/9cLPi73W9/U2sQHaDWXmcS\n\tJw2GK5MBXFTB2MMHP7WrSecc50jYfot7TLrX8ri+4hVDAWcQcLhmEf7qxFwrdxa1B4cn\n\twfgQK/AlcPDv4NHD0Cd6G7TbI8+o65N2A1yEV2O1DzmdkxtB0Xy2BMAnl3FDoDEhDvGM\n\tzsRQ==","X-Gm-Message-State":"AOAM532lDqt31Jo3FrmyJ9gPu7J/sfGwEG/3NUghAf+SHNeqnqx5EewU\n\tkvR67+u+db5hmWq1Kw1iejIvZoIHjSu7rCzWkxngFsxU384=","X-Google-Smtp-Source":"ABdhPJyrbx5FB11m9M8qCyheO04iNqak9ncqB8vSb5WmaKP2CLPcMdnIq85aSHVJ/whrgG2wTi+Lyw3sniKbZNQZYJI=","X-Received":"by 2002:adf:ea90:: with SMTP id\n\ts16mr71061274wrm.288.1636123104417; \n\tFri, 05 Nov 2021 07:38:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-3-david.plowman@raspberrypi.com>\n\t<163611375595.275423.15648154330177579284@Monstersaurus>\n\t\n\t<163611896067.275423.12928866383980122882@Monstersaurus>","In-Reply-To":"<163611896067.275423.12928866383980122882@Monstersaurus>","From":"David Plowman ","Date":"Fri, 5 Nov 2021 14:38:13 +0000","Message-ID":"","To":"Kieran Bingham , \n\tlibcamera devel ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","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":20700,"web_url":"https://patchwork.libcamera.org/comment/20700/","msgid":"<20211106162738.hztxuzmsc3rer4dp@uno.localdomain>","date":"2021-11-06T16:27:38","subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David\n\nOn Fri, Nov 05, 2021 at 02:38:13PM +0000, David Plowman wrote:\n> Hi again Kieran\n>\n> Sorry, forgot to \"reply all\" on my previous reply, hopefully folks can\n> find the missing email trail below!\n>\n> On Fri, 5 Nov 2021 at 13:29, Kieran Bingham\n> wrote:\n> >\n> > Quoting David Plowman (2021-11-05 13:16:44)\n> > > Hi Kieran\n> > >\n> > > Thanks for looking at this patch set!\n> > >\n> > > On Fri, 5 Nov 2021 at 12:02, Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > Quoting David Plowman (2021-11-04 13:58:00)\n> > > > > This is so that applications can choose appropriate color spaces which\n> > > > > will then be passed down to the V4L2 devices.\n> > > > >\n> > > > > There are two fields: the \"requestedColorSpace\" which is the one an\n> > > > > application wants, and the \"actualColorSpace\" which is what the\n> > > > > underlying hardware can deliver, and which is filled in by the\n> > > > > validate() method.\n> > > >\n> > > > Is it necessary to store both? We don't do that with the other fields...\n> > > > the StreamConfiguration structure is used in different states to handle\n> > > > that I think.\n> > > >\n> > > > When a CameraConfiguration is 'generated' Potential StreamConfigurations\n> > > > are 'offered' to the application.\n> > > >\n> > > > Applications can either fill in those StreamConfigurations, (or start\n> > > > from an empty one if it will set all fields anyway...) and 'validate'\n> > > > the overall CameraConfiguration.\n> > > >\n> > > > The StreamConfiguration at that point contains the 'requested' values\n> > > > ... and when returned back from validate() - it has the values that can\n> > > > be used by hardware...\n> > >\n> > > Yes, I think this is one of the critical points that we need to agree\n> > > on. So my reasoning was as follows:\n> > >\n> > > You specify the colour space that you want, but what if the driver\n> > > gives you something else. Specifically, what if the driver gives us\n> > > some wacky colour space that the ColorSpace class can't represent at\n> > > all?\n> > >\n> > > It feels a bit onerous to insist that we have a representation for\n> > > every colour space that we might ever encounter, particularly if there\n> > > might be non-V4L2 things in future, so I've gone with the notion of an\n> > > \"undefined\" colour space, which can be returned to us from the driver\n> > > layer.\n> > >\n> > > Now this is a dangerous thing so I've explicitly forbidden this as a\n> > > value that you can request (what would it mean? sounds like confusion\n> > > all round). So the worry is that we make a reasonable request, but the\n> > > driver changes it to \"undefined\", and now we can't call validate with\n> > > the same configuration.\n> >\n> > I suspect that at that point it's up to the pipeline handler to provide\n> > a reasonable color space?\n>\n> So I think this is the crux of the matter. Do we want to insist that\n> we have a representation for every colour space that we might ever\n> encounter? If we do, then yes - we can promise to pass a meaningful\n> ColorSpace back up the stack and we never have to worry about\n> anything.\n>\n> But colour spaces are in fact infinitely variable (unlike pixel\n> formats), though in practice V4L2 doesn't represent them so we'll\n> almost never encounter any. For the moment, anyway. Do we want to\n> pretend we can add every colour space we will ever encounter? In\n> practice it's probably true, but are we sure it will always be like\n> this?\n>\n> >\n> > Can drivers really return an undefined colorspace? What does that mean?\n> > (In the same regards as what does 'asking' for an undefined color space\n> > mean?).\n>\n> Note that a colour space is never actually \"undefined\", the driver\n> will have chosen one, it just means \"we don't have a ColorSpace to\n> describe what it chose\". So after validate() we could be told, \"it's\n> going to use a colour space that I can't describe to you\", but at the\n> same time it's nonsensical to request a colour space \"that I can't\n> describe\", which is why that is banned.\n>\n> I don't know if any of this helps. Perhaps we need a face-to-face.\n\nOuch, I see this question being asked multiple times in the past\nrevision, and when I saw this I immediately had the same feeling\nsomething was not 'right'.\n\nThe foundamental issue here is that we're making color spaces a first\nclass citizen of the configuration procedure as we require it now to\nbe defined by applications (otherwise we get errors in the logs, more\non this later) but at the same time we cannot correctly:\n- Identify all 'color spaces' as their definition is not precise as\n the one of pixel formats and they can be produced by mixing\n components that do not form a 'well known' standard\n- We cannot adjust it to any meaningful value if the driver returns\n something we don't support\n\nHave you considered, since 'Adjusted' doesn't really play well with an\n'Undefined' color space to just refuse configurations that contain an\nunsupported color space ?\n\nI keep wondering from my little corner if we shouldn't make\nthe color space selection an opt-in feature. Seeing how poorly it\ntreated in V4L2 where a lot of drivers (especially sensor\ndrivers) just use _DEFAULT values makes me wonder how common is for\napplication to actually deal with colorspaces at all. It also seems to\nme that there's no discovery mechanism, IOW an application cannot list\nthe supported color spaces to select one that is support, which means\nit really have to know what platforms it is dealing with. Am I wrong\non this part (please pardon me if this is otherwise obvious).\n\nIn some previous version std::optional<> was hinted and if we would\nlike to have color space selection be an opt-in feature, it seems to\nme std::optional<> applies quite well.\n\n\n>\n> David\n>\n> >\n> > > So my solution was to separate this into a \"requested\" and \"actual\"\n> > > colour space. I'm imagining that the user either leaves the\n> > > requestedColorSpace field alone, and they'll keep getting back\n> > > \"adjusted\" if they call validate again. Or if they don't like that\n> > > they could do something like\n> > >\n> > > if (actualColorSpace.isFullyDefined())\n> > > requestedColorSpace = actualColorSpace;\n> > >\n> > > which will at least get rid of the \"adjusted\" return value in cases\n> > > where that's possible. Am I making any sense, or are there better\n> > > solutions?\n> >\n> > I think if userspace continues to ask for a color space which is not\n> > supported (and somehow returns an undefined color space, rather than an\n> > acceptable one), then we are probably into 'fail to configure'\n> > territory.\n> >\n> > Is it any different from asking the hardware to give me YUYV, but it\n> > always returns NV12? I either accept that the hardware can only do NV12,\n> > and acknowledge that it's not going to be YUYV, or ... ?\n> >\n> >\n> > > Thanks!\n> > >\n> > > David\n> > >\n> > > >\n> > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/stream.h | 4 ++++\n> > > > > src/libcamera/stream.cpp | 25 +++++++++++++++++++++++++\n> > > > > 2 files changed, 29 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > > > > index 0c55e716..fe491ff5 100644\n> > > > > --- a/include/libcamera/stream.h\n> > > > > +++ b/include/libcamera/stream.h\n> > > > > @@ -12,6 +12,7 @@\n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -47,6 +48,9 @@ struct StreamConfiguration {\n> > > > >\n> > > > > unsigned int bufferCount;\n> > > > >\n> > > > > + ColorSpace requestedColorSpace;\n> > > > > + ColorSpace actualColorSpace;\n> > > >\n> > > > Given the above, I think this should be just colorSpace;\n> > > >\n> > > > > +\n> > > > > Stream *stream() const { return stream_; }\n> > > > > void setStream(Stream *stream) { stream_ = stream; }\n> > > > > const StreamFormats &formats() const { return formats_; }\n> > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > > > > index b421e17e..1ddbbb8c 100644\n> > > > > --- a/src/libcamera/stream.cpp\n> > > > > +++ b/src/libcamera/stream.cpp\n> > > > > @@ -329,6 +329,31 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> > > > > * \\brief Requested number of buffers to allocate for the stream\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\var StreamConfiguration::requestedColorSpace\n> > > > > + * \\brief The ColorSpace this stream should have\n> > > > > + *\n> > > > > + * The generateConfiguration method will generate reasonable default\n> > > > > + * values (ColorSpace::Jpeg for stills, ColorSpace::Rec709 for video and\n> > > > > + * ColorSpace::Raw for raw streams) but applications are free to overwrite\n> > > > > + * this value.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\var StreamConfiguration::actualColorSpace\n> > > > > + * \\brief The ColorSpace the will be used for this stream\n> > > > > + *\n> > > > > + * This field is filled in by CameraConfiguration::validate().\n> > > > > + * Normally this should match the requestedColorSpace, but it may differ\n> > > > > + * if the hardware does not support it.\n> > > > > + *\n> > > > > + * In general cameras may have different constraints here, for example,\n> > > > > + * they may require all output streams to share the same color space.\n> > > > > + * Sometimes the fields within this color space may report \"Undefined\"\n> > > > > + * values if the hardware drivers are going to use a color space that\n> > > > > + * is not recognised by the ColorSpace class.\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\fn StreamConfiguration::stream()\n> > > > > * \\brief Retrieve the stream associated with the configuration\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 E0D16BF415\n\tfor ;\n\tSat, 6 Nov 2021 16:26:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F2C26034F;\n\tSat, 6 Nov 2021 17:26:49 +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 AB28A6033A\n\tfor ;\n\tSat, 6 Nov 2021 17:26:47 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 8D04E1BF206;\n\tSat, 6 Nov 2021 16:26:46 +0000 (UTC)"],"Date":"Sat, 6 Nov 2021 17:27:38 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211106162738.hztxuzmsc3rer4dp@uno.localdomain>","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-3-david.plowman@raspberrypi.com>\n\t<163611375595.275423.15648154330177579284@Monstersaurus>\n\t\n\t<163611896067.275423.12928866383980122882@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20707,"web_url":"https://patchwork.libcamera.org/comment/20707/","msgid":"<163637290283.275423.15389409779720089810@Monstersaurus>","date":"2021-11-08T12:01:42","subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-05 14:38:13)\n> Hi again Kieran\n> \n> Sorry, forgot to \"reply all\" on my previous reply, hopefully folks can\n> find the missing email trail below!\n> \n> On Fri, 5 Nov 2021 at 13:29, Kieran Bingham\n> wrote:\n> >\n> > Quoting David Plowman (2021-11-05 13:16:44)\n> > > Hi Kieran\n> > >\n> > > Thanks for looking at this patch set!\n> > >\n> > > On Fri, 5 Nov 2021 at 12:02, Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > Quoting David Plowman (2021-11-04 13:58:00)\n> > > > > This is so that applications can choose appropriate color spaces which\n> > > > > will then be passed down to the V4L2 devices.\n> > > > >\n> > > > > There are two fields: the \"requestedColorSpace\" which is the one an\n> > > > > application wants, and the \"actualColorSpace\" which is what the\n> > > > > underlying hardware can deliver, and which is filled in by the\n> > > > > validate() method.\n> > > >\n> > > > Is it necessary to store both? We don't do that with the other fields...\n> > > > the StreamConfiguration structure is used in different states to handle\n> > > > that I think.\n> > > >\n> > > > When a CameraConfiguration is 'generated' Potential StreamConfigurations\n> > > > are 'offered' to the application.\n> > > >\n> > > > Applications can either fill in those StreamConfigurations, (or start\n> > > > from an empty one if it will set all fields anyway...) and 'validate'\n> > > > the overall CameraConfiguration.\n> > > >\n> > > > The StreamConfiguration at that point contains the 'requested' values\n> > > > ... and when returned back from validate() - it has the values that can\n> > > > be used by hardware...\n> > >\n> > > Yes, I think this is one of the critical points that we need to agree\n> > > on. So my reasoning was as follows:\n> > >\n> > > You specify the colour space that you want, but what if the driver\n> > > gives you something else. Specifically, what if the driver gives us\n> > > some wacky colour space that the ColorSpace class can't represent at\n> > > all?\n> > >\n> > > It feels a bit onerous to insist that we have a representation for\n> > > every colour space that we might ever encounter, particularly if there\n> > > might be non-V4L2 things in future, so I've gone with the notion of an\n> > > \"undefined\" colour space, which can be returned to us from the driver\n> > > layer.\n> > >\n> > > Now this is a dangerous thing so I've explicitly forbidden this as a\n> > > value that you can request (what would it mean? sounds like confusion\n> > > all round). So the worry is that we make a reasonable request, but the\n> > > driver changes it to \"undefined\", and now we can't call validate with\n> > > the same configuration.\n> >\n> > I suspect that at that point it's up to the pipeline handler to provide\n> > a reasonable color space?\n> \n> So I think this is the crux of the matter. Do we want to insist that\n> we have a representation for every colour space that we might ever\n> encounter? If we do, then yes - we can promise to pass a meaningful\n> ColorSpace back up the stack and we never have to worry about\n> anything.\n> \n> But colour spaces are in fact infinitely variable (unlike pixel\n> formats), though in practice V4L2 doesn't represent them so we'll\n> almost never encounter any. For the moment, anyway. Do we want to\n> pretend we can add every colour space we will ever encounter? In\n> practice it's probably true, but are we sure it will always be like\n> this?\n\nMaybe the bit I haven't understood is:\n\n Do we expect 'named' defined color spaces, or are colorspaces (of the\n ColorSpace class) also widely constuctable?\n\nCan I do:\n\n // Use the JPEG as a start point\n ColorSpace myColorSpace = ColorSpace::Jpeg;\n // Except I use the Linear TF for $REASONS\n myColorSpace.transferFunction = TransferFunction::Linear;\n\nI thought we don't have to pre-name/define the colorspaces explicitly -\nthey can be interacted with as a struct as long as they Satisfy the 'isFullyDefined' check?\n\n> > Can drivers really return an undefined colorspace? What does that mean?\n> > (In the same regards as what does 'asking' for an undefined color space\n> > mean?).\n> \n> Note that a colour space is never actually \"undefined\", the driver\n> will have chosen one, it just means \"we don't have a ColorSpace to\n> describe what it chose\". So after validate() we could be told, \"it's\n> going to use a colour space that I can't describe to you\", but at the\n> same time it's nonsensical to request a colour space \"that I can't\n> describe\", which is why that is banned.\n> \n> I don't know if any of this helps. Perhaps we need a face-to-face.\n\nMaybe ;-) I'll go continue the discussion on the top level discussion\nmail, and we can see who we need to get together.\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 989EABF415\n\tfor ;\n\tMon, 8 Nov 2021 12:01:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF3F56033A;\n\tMon, 8 Nov 2021 13:01:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13874600BF\n\tfor ;\n\tMon, 8 Nov 2021 13:01:45 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A15B5E51;\n\tMon, 8 Nov 2021 13:01:44 +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=\"af+YX6jV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636372904;\n\tbh=KpN67Siu7ZbQc0irsSG//68PHSCFWApFkuz4Zqt1+r8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=af+YX6jVkRznDFQ6e4i7+8iVJZaVDXVAQc9QCMirklj2UlWox5zR9UZJ+iervIUAz\n\tp0o/EJNmJjp4PDRtg74EW1xuFYwte7aBRAEsLYTAr1a9M46Hb107BqPmS1riHJ/yg2\n\tik6uBZKhycGNSamvEneEE5k1ALT8rHRvXAHt/4Io=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-3-david.plowman@raspberrypi.com>\n\t<163611375595.275423.15648154330177579284@Monstersaurus>\n\t\n\t<163611896067.275423.12928866383980122882@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"David Plowman ,\n\tlibcamera devel ","Date":"Mon, 08 Nov 2021 12:01:42 +0000","Message-ID":"<163637290283.275423.15389409779720089810@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace\n\tfields to StreamConfiguration","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\" "}}]