[{"id":26178,"web_url":"https://patchwork.libcamera.org/comment/26178/","msgid":"<20230103173658.nk2oqeoh5mkvobsi@uno.localdomain>","date":"2023-01-03T17:36:58","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote:\n> The intention is that the \"main\" colour space is the colour space of\n> the largest non-raw stream. Unfortunately the use of \"config_[i].size\"\n> is clearly incorrect, and has been copied from prior versions of the\n> code. This small change merely corrects the error.\n>\n> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> ---\n>  src/libcamera/camera.cpp | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 2d947a44..0da167a7 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n>  \t * largest non-raw stream with a defined color space (if there is one).\n>  \t */\n>  \tstd::optional<ColorSpace> colorSpace;\n> +\tSize size;\n>\n>  \tfor (auto [i, cfg] : utils::enumerate(config_)) {\n>  \t\tif (!cfg.colorSpace)\n> @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n>  \t\tif (cfg.colorSpace->adjust(cfg.pixelFormat))\n>  \t\t\tstatus = Adjusted;\n>\n> -\t\tif (cfg.colorSpace != ColorSpace::Raw &&\n> -\t\t    (!colorSpace || cfg.size > config_[i].size))\n> +\t\tif (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) {\n\nI'm not too familiar with this part of the code, but seeing\n\n \tfor (auto [i, cfg] : utils::enumerate(config_)) {\n                ...\n\t\tif (cfg.size > config_[i].size))\n\nmakes me indeed think that cfg.size == config_[i].size\n\nHowever I wonder why the condition was preceded by !colorSpace..\nThe same condition was expressed in\n5e5eadabd812 (\"libcamera: camera: Add validateColorSpaces to CameraConfiguration class\")\n\nas\n\n               } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {\n                       index = i;\n\nAnd I guess it was because the second part of the condition was always\nfalse :)\n\nNow that I've tracked down the full history of the bug\nReviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nThanks\n  j\n\n>  \t\t\tcolorSpace = cfg.colorSpace;\n> +\t\t\tsize = cfg.size;\n> +\t\t}\n>  \t}\n>\n>  \tif (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> --\n> 2.30.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A2BB8C322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  3 Jan 2023 17:37:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FF32625CF;\n\tTue,  3 Jan 2023 18:37:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A585661F0F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  3 Jan 2023 18:37:02 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:c40c:10ff:feac:d8bd])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23847108;\n\tTue,  3 Jan 2023 18:37:02 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672767425;\n\tbh=Pblbi1IT453XoHoXfXj0rfLAf0l79a0P4ULI9eFm79c=;\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=x6R3oLutvu4zcZNIt+RzY1oONtvQCZAPe+BN3XjyZE1DdOUfLPqR3fMWI2gQZ61+q\n\tMqfEvWFZZEJj0TyUes/zh19y5Z/gXNsOgND0M+ROs89DgioqYoSJh7y5QJkPM8+vrh\n\tAfb0fVpwq5jWQ4eP8m+/bJacAQ2fW6TVvGrMDb0cZpmIH5y//G8wnIqtpKQ745cB8S\n\t/GBJsHlyWz3SCzGpCdbQ1ERruMl6e1rVT9ZYT+U/SPaiBi8gHJfiCEPi7XLUTnZZuq\n\tI0LD4DKTNmWCHxhz8UfSkm6CYUTrnMRLh3MGd6lEs4a3rnsMO8C9ikr42WdWbdlG+v\n\t6PiWpCml/2IdQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1672767422;\n\tbh=Pblbi1IT453XoHoXfXj0rfLAf0l79a0P4ULI9eFm79c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TxJ+oJ4RmKy9zNcb9fMoLNBdAxFIqRhYUb83ZNXR/yr5EDq6J/T24U64Xh2Ij57sv\n\t3ERaXGCJsBr9Rt82LvuiP/ItukzQfA0x3rb7ZKuNqvoSkk4N2CfJfOWuxE0fXSbRWL\n\trpZwQ9iv6EMXKrKPcPxvJsmRv1d5bAzd+7v1QB3s="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TxJ+oJ4R\"; dkim-atps=neutral","Date":"Tue, 3 Jan 2023 18:36:58 +0100","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20230103173658.nk2oqeoh5mkvobsi@uno.localdomain>","References":"<20230103113313.5423-1-david.plowman@raspberrypi.com>\n\t<20230103113313.5423-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230103113313.5423-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26179,"web_url":"https://patchwork.libcamera.org/comment/26179/","msgid":"<CAHW6GYKf_GT+B+xEiOjSpY1Uea6pxFM+N0K-kSSUWnruDs8Ttg@mail.gmail.com>","date":"2023-01-04T09:41:54","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","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 the review.\n\nI was wondering if you knew anything about how rkisp1 uses this\nfunction (I think it's the only other caller apart from Raspberry Pi,\nat least at the moment).\n\nThe reason I ask is because I think the whole \"streams sharing a\ncolour space\" thing isn't terribly useful any more (YUV and RGB\nstreams are no longer allowed to share a ColorSpace), so I'm wondering\nabout removing it, but one would have to understand the rkisp1\nrequirements first! Would you know anything about that?\n\nThanks!\nDavid\n\nOn Tue, 3 Jan 2023 at 17:37, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi David\n>\n> On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote:\n> > The intention is that the \"main\" colour space is the colour space of\n> > the largest non-raw stream. Unfortunately the use of \"config_[i].size\"\n> > is clearly incorrect, and has been copied from prior versions of the\n> > code. This small change merely corrects the error.\n> >\n> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > ---\n> >  src/libcamera/camera.cpp | 6 ++++--\n> >  1 file changed, 4 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 2d947a44..0da167a7 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> >        * largest non-raw stream with a defined color space (if there is one).\n> >        */\n> >       std::optional<ColorSpace> colorSpace;\n> > +     Size size;\n> >\n> >       for (auto [i, cfg] : utils::enumerate(config_)) {\n> >               if (!cfg.colorSpace)\n> > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> >               if (cfg.colorSpace->adjust(cfg.pixelFormat))\n> >                       status = Adjusted;\n> >\n> > -             if (cfg.colorSpace != ColorSpace::Raw &&\n> > -                 (!colorSpace || cfg.size > config_[i].size))\n> > +             if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) {\n>\n> I'm not too familiar with this part of the code, but seeing\n>\n>         for (auto [i, cfg] : utils::enumerate(config_)) {\n>                 ...\n>                 if (cfg.size > config_[i].size))\n>\n> makes me indeed think that cfg.size == config_[i].size\n>\n> However I wonder why the condition was preceded by !colorSpace..\n> The same condition was expressed in\n> 5e5eadabd812 (\"libcamera: camera: Add validateColorSpaces to CameraConfiguration class\")\n>\n> as\n>\n>                } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {\n>                        index = i;\n>\n> And I guess it was because the second part of the condition was always\n> false :)\n>\n> Now that I've tracked down the full history of the bug\n> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n>\n> Thanks\n>   j\n>\n> >                       colorSpace = cfg.colorSpace;\n> > +                     size = cfg.size;\n> > +             }\n> >       }\n> >\n> >       if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> > --\n> > 2.30.2\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 9E92DBD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  4 Jan 2023 09:42:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6BAD625CE;\n\tWed,  4 Jan 2023 10:42:08 +0100 (CET)","from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com\n\t[IPv6:2607:f8b0:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A6C6625CE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  4 Jan 2023 10:42:07 +0100 (CET)","by mail-oi1-x22f.google.com with SMTP id v70so29108248oie.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 04 Jan 2023 01:42:06 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672825328;\n\tbh=LIosC2QWBcuuw1NuIWmFMGHm0PwynVKUn066pdl2Dgs=;\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=oemRehgJjdlIFLa8q1rGYsOyQ+kjjyUiI3NcIfWztkMtQApA9yP+ohTmWj5ocLy4j\n\tmNF1N0SlhRsduZagn/Y3ru/3kP56a+tkoU3reLXOYzk+dSQL1/dx6AQBykA46jEx/9\n\tkf5V9WytFVBh9k+ckZPDNGGn7BBjfIZtSLrKlbQKPYeVSyFB5UO1f8Lg1GefQOFKMC\n\thqwMMjMHHuyPgnfvw+JRHJImZ2r0xNIfrciww69DGZkW44zc3M86jEqw4eb4wC7NHu\n\tND/yIfB670zOY+7u3QVM3X4/EEydBIYk4/VigBS6Gz2y9u8R0al/jDss04AkQXe48W\n\tDDSVcq8+5mGIg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=QWc+GbPWm6kwJlnEeMMKDKoogDZ9uYap0V7sXJEk8gw=;\n\tb=R8df6qf68ygGIY/SoLfXvVU1vkC8/ZMLHLgFlR71aQWx1Dnz2HKZqw0Mag3eGVxKBd\n\txZz37z3MRvZgThWK6HZNZGZTXKePHKThvLcfWrsYKY4HoWp2FLslkkbdMsM1z9tF5MNw\n\tFtl12lyPW0okn9N31kMnTJ2O67a72EFJcFS/IVNP6tHrKvFVlr1ytS/6q5GcqqNvhhEo\n\t/sRCr0kAm+/XtQdFFk3U1zZGaC8fljnTOWS15gMIAqLHXQjx7ufjwU3p7X2Bfb1Qui55\n\tFxNDEwYfA5jIw+sLBjFKB0yKftzTshKGmzbZamg33rlCR0xQLFtKLD+NqBonTor1uQks\n\tQaKQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"R8df6qf6\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=QWc+GbPWm6kwJlnEeMMKDKoogDZ9uYap0V7sXJEk8gw=;\n\tb=XtBHYzuu9G8/MzTMmcL6cpCgIb1Qgj0tfatl26I55izSNAqWTLb00eMhF+ljoPJCyd\n\tFDGsR7w7IzHKeRBJBNTeoF9+tdewvp1FyDdVrr0kWubZNzJ6A9h+lUePc1VcR8BV0+A+\n\tKG563lM19sBjEOl+YFyHQNpZ+DaOsYjGWu9bOFrPVKKOPGzCsP8x05HwUYCWgae4lMVM\n\t32tBzAU2eMZlBxFy5d6mRY3NLGFVqJzsd1NVbe7pX1Hq+/y4Qu4llVM1pvZZN39Zku6C\n\td4fppBSeTCbLeDtBCO0pTUCDESC5tII0LNBcRSaS4/1zq/HDY5tMLhlBhDBQ4KjunMI3\n\toQzg==","X-Gm-Message-State":"AFqh2koE1WBpY0IufsFoHFm4+pEf1NI0ifX3ZKOhvUaBXruz6FSWn+dq\n\tydkWEQMMehEabt/f2eBnVeXI2bSZIia9qtk4jlnUux9DZ3dk3A==","X-Google-Smtp-Source":"AMrXdXtGLCgdcxlBiFf6cMwkrMJG5xC4qR/FC/JIpd3iuFOgsFwuIxzAp+r/LUu9reTqX6Hx4tuPsmFofWpWzEIosZw=","X-Received":"by 2002:aca:910:0:b0:354:4cfe:aaad with SMTP id\n\t16-20020aca0910000000b003544cfeaaadmr1864198oij.246.1672825325587;\n\tWed, 04 Jan 2023 01:42:05 -0800 (PST)","MIME-Version":"1.0","References":"<20230103113313.5423-1-david.plowman@raspberrypi.com>\n\t<20230103113313.5423-2-david.plowman@raspberrypi.com>\n\t<20230103173658.nk2oqeoh5mkvobsi@uno.localdomain>","In-Reply-To":"<20230103173658.nk2oqeoh5mkvobsi@uno.localdomain>","Date":"Wed, 4 Jan 2023 09:41:54 +0000","Message-ID":"<CAHW6GYKf_GT+B+xEiOjSpY1Uea6pxFM+N0K-kSSUWnruDs8Ttg@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26184,"web_url":"https://patchwork.libcamera.org/comment/26184/","msgid":"<20230105092358.l7utaeswnz3c4vqb@uno.localdomain>","date":"2023-01-05T09:23:58","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David,\n\nOn Wed, Jan 04, 2023 at 09:41:54AM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the review.\n>\n> I was wondering if you knew anything about how rkisp1 uses this\n> function (I think it's the only other caller apart from Raspberry Pi,\n> at least at the moment).\n\nIt seems to me as well it's the only other user apart RPi\n\n>\n> The reason I ask is because I think the whole \"streams sharing a\n> colour space\" thing isn't terribly useful any more (YUV and RGB\n> streams are no longer allowed to share a ColorSpace), so I'm wondering\n\nI've missed good parts of the discussion, but why \"YUV and RGB streams\nare -no longer- allowed to share a ColorSpace\" ?\n\n> about removing it, but one would have to understand the rkisp1\n> requirements first! Would you know anything about that?\n\nI don't know the details, but looking at the commit message of\n\n4267e0bab86d (\"libcamera: pipeline: rkisp1: Implement color space support\")\n\nAs all the processing related to the color space is performed in the\npart of the pipeline shared by all streams, a single color space must\ncover all stream configurations. This is enforced manually when\ngenerating the configuration, and with the validateColorSpaces()\nhelper when validating it.\n\nand the comment in the code\n\n+       /*\n+        * As the ISP can't output different color spaces for the main and self\n+        * path, pick a sensible default color space based on the role of the\n+        * first stream and use it for all streams.\n+        */\n+       std::optional<ColorSpace> colorSpace;\n\nIt seems clear to me that the RkISP1 requires a single colorspace for\nall outputs. That's why it's interesting to know why \"YUV and RGB are\nno longer allowed to share a ColorSpace\".\n\nOfc if you move away from CameraConfiguration::validateColorSpaces()\nand RkISP1 it's the only remaining user we can consider moving the\nfunction back to the pipeline handler implementation.\n\nThanks\n  j\n\n>\n> Thanks!\n> David\n>\n> On Tue, 3 Jan 2023 at 17:37, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:\n> >\n> > Hi David\n> >\n> > On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote:\n> > > The intention is that the \"main\" colour space is the colour space of\n> > > the largest non-raw stream. Unfortunately the use of \"config_[i].size\"\n> > > is clearly incorrect, and has been copied from prior versions of the\n> > > code. This small change merely corrects the error.\n> > >\n> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > ---\n> > >  src/libcamera/camera.cpp | 6 ++++--\n> > >  1 file changed, 4 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 2d947a44..0da167a7 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > >        * largest non-raw stream with a defined color space (if there is one).\n> > >        */\n> > >       std::optional<ColorSpace> colorSpace;\n> > > +     Size size;\n> > >\n> > >       for (auto [i, cfg] : utils::enumerate(config_)) {\n> > >               if (!cfg.colorSpace)\n> > > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > >               if (cfg.colorSpace->adjust(cfg.pixelFormat))\n> > >                       status = Adjusted;\n> > >\n> > > -             if (cfg.colorSpace != ColorSpace::Raw &&\n> > > -                 (!colorSpace || cfg.size > config_[i].size))\n> > > +             if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) {\n> >\n> > I'm not too familiar with this part of the code, but seeing\n> >\n> >         for (auto [i, cfg] : utils::enumerate(config_)) {\n> >                 ...\n> >                 if (cfg.size > config_[i].size))\n> >\n> > makes me indeed think that cfg.size == config_[i].size\n> >\n> > However I wonder why the condition was preceded by !colorSpace..\n> > The same condition was expressed in\n> > 5e5eadabd812 (\"libcamera: camera: Add validateColorSpaces to CameraConfiguration class\")\n> >\n> > as\n> >\n> >                } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {\n> >                        index = i;\n> >\n> > And I guess it was because the second part of the condition was always\n> > false :)\n> >\n> > Now that I've tracked down the full history of the bug\n> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> >\n> > Thanks\n> >   j\n> >\n> > >                       colorSpace = cfg.colorSpace;\n> > > +                     size = cfg.size;\n> > > +             }\n> > >       }\n> > >\n> > >       if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> > > --\n> > > 2.30.2\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DFBE0BD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  5 Jan 2023 09:24:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B184625CF;\n\tThu,  5 Jan 2023 10:24:04 +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 AC375625CC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Jan 2023 10:24:02 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3396749C;\n\tThu,  5 Jan 2023 10:24:02 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672910644;\n\tbh=OFPrUKGHZAWj2aaDX2BqGN5tvivrcM2z5KtRdfosAog=;\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=1WXkb9jWzsFREACEKaKQ+fgURkz39Je3wHgVNc3gCyHKgsoGFkpzma0PebmQKuwu8\n\tCmj/8E4FWQoAmVhKCIAKKXwRtw8wgGAVx6Iy8/FkSh77g3EjdQz36Z22+SpVF24TpF\n\t6YBYsuKQZiTC/BbfEx1pTptgZ5CkxKMxF0RhBmBYXl5JJ2GyE+U8aBYLsO2NFYOJxf\n\tKcKQXoYD/z04LkmEsniaF7ONOksu4z4BTYGPqcAoTTuLIfPJAw4Mo93+5arRRzw420\n\ty/o2t98dfEj9g5CSZNKaxiPtkSnsVSW/4z3pTx4NDqy2R+BFEFbmHc6r80ExYvigYb\n\tiQA+1pXdSMFGg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1672910642;\n\tbh=OFPrUKGHZAWj2aaDX2BqGN5tvivrcM2z5KtRdfosAog=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lG2OoVpxZQglzJtY0/HZJwOhLPQAKn0Anfv3z+2IO7Y/NoDjm0P2TTLCr8jrBgjm2\n\tD6P9Z8cA3xB1B/lpc4p+etlhCXES/HpMtCkgKQw9t2hGyf4xIMLcEe1EDVXctqOOfz\n\tyzD0aFFTPefoRCxJ17+IcGfqSI2SVKaK7E68JtOs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lG2OoVpx\"; dkim-atps=neutral","Date":"Thu, 5 Jan 2023 10:23:58 +0100","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20230105092358.l7utaeswnz3c4vqb@uno.localdomain>","References":"<20230103113313.5423-1-david.plowman@raspberrypi.com>\n\t<20230103113313.5423-2-david.plowman@raspberrypi.com>\n\t<20230103173658.nk2oqeoh5mkvobsi@uno.localdomain>\n\t<CAHW6GYKf_GT+B+xEiOjSpY1Uea6pxFM+N0K-kSSUWnruDs8Ttg@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAHW6GYKf_GT+B+xEiOjSpY1Uea6pxFM+N0K-kSSUWnruDs8Ttg@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26189,"web_url":"https://patchwork.libcamera.org/comment/26189/","msgid":"<CAEmqJPpw0g0ZpYwgYKVUXJu6rmf=B-AKed1Onk4h7phynOgzZQ@mail.gmail.com>","date":"2023-01-06T09:59:29","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for this fix!\n\nOn Tue, 3 Jan 2023 at 11:33, David Plowman via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> The intention is that the \"main\" colour space is the colour space of\n> the largest non-raw stream. Unfortunately the use of \"config_[i].size\"\n> is clearly incorrect, and has been copied from prior versions of the\n> code. This small change merely corrects the error.\n>\n> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n>\n\nReviewed-by: Naushir Patuck <naush@raspberrypi.com>\n\n\n> ---\n>  src/libcamera/camera.cpp | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 2d947a44..0da167a7 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -361,6 +361,7 @@ CameraConfiguration::Status\n> CameraConfiguration::validateColorSpaces(ColorSpaceF\n>          * largest non-raw stream with a defined color space (if there is\n> one).\n>          */\n>         std::optional<ColorSpace> colorSpace;\n> +       Size size;\n>\n>         for (auto [i, cfg] : utils::enumerate(config_)) {\n>                 if (!cfg.colorSpace)\n> @@ -369,9 +370,10 @@ CameraConfiguration::Status\n> CameraConfiguration::validateColorSpaces(ColorSpaceF\n>                 if (cfg.colorSpace->adjust(cfg.pixelFormat))\n>                         status = Adjusted;\n>\n> -               if (cfg.colorSpace != ColorSpace::Raw &&\n> -                   (!colorSpace || cfg.size > config_[i].size))\n> +               if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) {\n>                         colorSpace = cfg.colorSpace;\n> +                       size = cfg.size;\n> +               }\n>         }\n>\n>         if (!colorSpace || !(flags &\n> ColorSpaceFlag::StreamsShareColorSpace))\n> --\n> 2.30.2\n>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 86B67C322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Jan 2023 09:59:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4AE3625DE;\n\tFri,  6 Jan 2023 10:59:48 +0100 (CET)","from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com\n\t[IPv6:2607:f8b0:4864:20::b2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA49161F09\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Jan 2023 10:59:46 +0100 (CET)","by mail-yb1-xb2d.google.com with SMTP id p188so1334482yba.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 06 Jan 2023 01:59:46 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672999188;\n\tbh=yG72r2ZCVK41SGdUuN8xJGUXmUMypY1BiJ35VoiKPdA=;\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=4WMM+XzXGOgTgXgKWEuq+6/joJDIdfFBTidbGTRTD6Pll8ZCgFgOLvz4E3KhEn/hS\n\tZYf0JJNSOZU5dXVYnuO5LOH+2hvvGU2wcBqhLBQQ83oCOiDGDFj8LtGEFP0Xw8BSqA\n\tUMaIEFqnYYcBEibUJkrpI9uP2cdcug7N3p5fRi2lzmyESt3gidUa1HfLgfYvWCH6fF\n\tNbRwjXVhCsRXL5Hkd12iVj4yYQCjKUZOBc2B45dX40UMoOrIv8dfrDFpvFr8ldXuyk\n\t0uxaDJZ71yAUtL3zp8kzCFv7wMrmxIWmzVo2Hby/FpxMm2GboQOvjJcJwEvrnoUFRG\n\tGyAkxACwVAw5Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=HTLwNZT+hixIfgrb+QlwFhgwwDnwq2qygYAY1hwmqZQ=;\n\tb=ikxgCA0Voc47qUPoC03PGNYkuuwzPEu6EmN2Nk1FgRWyJdEY0NpYnSZse6qY7BueMc\n\tnYfg7BUPT3UOzBlE0w0ZToGl3/Apu4TPmeMDrwb74BYdcOpjQyqzJ6pHFhvnm96U3DRA\n\t3Or1YKhwzx5heWtD7fjQ62h2/5llLX2iE60bFRt1KztVj/Gu7KNxUOvAVIL1rW3qqCP6\n\tvbVOh4NMxBrziy72xGuzcCbrOK+oGidf8uCHyZttf4vgrWsP6sDqeat29dDbacP+PTO4\n\tljTteOGu6U7IrxVHPjIHsuVRkrsmaBttatoZjv9ixvEjL++TxEivFlq4fcbXwU3gZU/I\n\tK04w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ikxgCA0V\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=HTLwNZT+hixIfgrb+QlwFhgwwDnwq2qygYAY1hwmqZQ=;\n\tb=BVdRpy2bfJ6WwQ6zxaHrsyP4MZ+8UPmT6Z8htKdTCYYuYLZ3vs5gxEYSJzIu50x6KR\n\tidBd3IE2BBPN0dnBzyjGR+izwGldVf9b5Km/+lVK5f1FVsi4ohjIG27w5vRhLhvsFLYA\n\tjRedkWngQWL9yXPuHChV/VfaKbG71d1z462qNQwq4x50hlPtNRUOmFaaGzkEtWnjd1CE\n\tUd/HZT3NrHUqUkJMAZ8/PIki7C2ogkH3lZ1yFMQhZUSdU947BYigWTPVYyjWsQT0mlG9\n\tesA3+F6HRVI1pmL0/fhgbbf1ILzmwTx8qsngfrzM5lKiTCjqe/84BC5pJL6OLEVJeLsK\n\t8RFg==","X-Gm-Message-State":"AFqh2kq0yBjUbev1SmuqHsb1z9A0A40fcqEa3Ylx1ZaLABOcWFzDdMwY\n\tAgOmwbb6ygrhmHkwUu0um1dBdkLV5Nl2x8qyti17PQ==","X-Google-Smtp-Source":"AMrXdXsrApfvQDfakPKqj4FaFE1kq1S3Z4hyJL73rHNTTaFWlRqa9DJh34VSLLZNzMxe9C7Rq8O8fQXHTEiketXazl4=","X-Received":"by 2002:a05:6902:38d:b0:783:bc5e:3d67 with SMTP id\n\tf13-20020a056902038d00b00783bc5e3d67mr2519775ybs.524.1672999185539;\n\tFri, 06 Jan 2023 01:59:45 -0800 (PST)","MIME-Version":"1.0","References":"<20230103113313.5423-1-david.plowman@raspberrypi.com>\n\t<20230103113313.5423-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20230103113313.5423-2-david.plowman@raspberrypi.com>","Date":"Fri, 6 Jan 2023 09:59:29 +0000","Message-ID":"<CAEmqJPpw0g0ZpYwgYKVUXJu6rmf=B-AKed1Onk4h7phynOgzZQ@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Content-Type":"multipart/alternative; boundary=\"00000000000029fc4e05f1957a32\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26191,"web_url":"https://patchwork.libcamera.org/comment/26191/","msgid":"<CAHW6GYL9T7mJa5snMq3X+7ns41mBmQL610i_Zh80ek_9c8gRnA@mail.gmail.com>","date":"2023-01-06T10:20:29","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Thu, 5 Jan 2023 at 09:24, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi David,\n>\n> On Wed, Jan 04, 2023 at 09:41:54AM +0000, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the review.\n> >\n> > I was wondering if you knew anything about how rkisp1 uses this\n> > function (I think it's the only other caller apart from Raspberry Pi,\n> > at least at the moment).\n>\n> It seems to me as well it's the only other user apart RPi\n>\n> >\n> > The reason I ask is because I think the whole \"streams sharing a\n> > colour space\" thing isn't terribly useful any more (YUV and RGB\n> > streams are no longer allowed to share a ColorSpace), so I'm wondering\n>\n> I've missed good parts of the discussion, but why \"YUV and RGB streams\n> are -no longer- allowed to share a ColorSpace\" ?\n\nA change was made whereby streams that are producing an RGB output are\nforced to have \"None\" for the YCbCr encoding, and \"Full\" for the\nrange. This means if you have a YUV and an RGB stream, they can't\nshare the same ColorSpace. Previously those fields were simply ignored\nfor output formats that didn't need them.\n\n>\n> > about removing it, but one would have to understand the rkisp1\n> > requirements first! Would you know anything about that?\n>\n> I don't know the details, but looking at the commit message of\n>\n> 4267e0bab86d (\"libcamera: pipeline: rkisp1: Implement color space support\")\n>\n> As all the processing related to the color space is performed in the\n> part of the pipeline shared by all streams, a single color space must\n> cover all stream configurations. This is enforced manually when\n> generating the configuration, and with the validateColorSpaces()\n> helper when validating it.\n>\n> and the comment in the code\n>\n> +       /*\n> +        * As the ISP can't output different color spaces for the main and self\n> +        * path, pick a sensible default color space based on the role of the\n> +        * first stream and use it for all streams.\n> +        */\n> +       std::optional<ColorSpace> colorSpace;\n>\n> It seems clear to me that the RkISP1 requires a single colorspace for\n> all outputs. That's why it's interesting to know why \"YUV and RGB are\n> no longer allowed to share a ColorSpace\".\n\nIt depends if RkISP1 can create a YUV and an RGB stream\nsimultaneously. If so, someone who understands it needs to look into\nthe ColorSpace question - it may actually be broken at the moment, I\ncan't tell.\n\nDavid\n\n>\n> Ofc if you move away from CameraConfiguration::validateColorSpaces()\n> and RkISP1 it's the only remaining user we can consider moving the\n> function back to the pipeline handler implementation.\n>\n> Thanks\n>   j\n>\n> >\n> > Thanks!\n> > David\n> >\n> > On Tue, 3 Jan 2023 at 17:37, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:\n> > >\n> > > Hi David\n> > >\n> > > On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote:\n> > > > The intention is that the \"main\" colour space is the colour space of\n> > > > the largest non-raw stream. Unfortunately the use of \"config_[i].size\"\n> > > > is clearly incorrect, and has been copied from prior versions of the\n> > > > code. This small change merely corrects the error.\n> > > >\n> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > ---\n> > > >  src/libcamera/camera.cpp | 6 ++++--\n> > > >  1 file changed, 4 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 2d947a44..0da167a7 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > > >        * largest non-raw stream with a defined color space (if there is one).\n> > > >        */\n> > > >       std::optional<ColorSpace> colorSpace;\n> > > > +     Size size;\n> > > >\n> > > >       for (auto [i, cfg] : utils::enumerate(config_)) {\n> > > >               if (!cfg.colorSpace)\n> > > > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > > >               if (cfg.colorSpace->adjust(cfg.pixelFormat))\n> > > >                       status = Adjusted;\n> > > >\n> > > > -             if (cfg.colorSpace != ColorSpace::Raw &&\n> > > > -                 (!colorSpace || cfg.size > config_[i].size))\n> > > > +             if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) {\n> > >\n> > > I'm not too familiar with this part of the code, but seeing\n> > >\n> > >         for (auto [i, cfg] : utils::enumerate(config_)) {\n> > >                 ...\n> > >                 if (cfg.size > config_[i].size))\n> > >\n> > > makes me indeed think that cfg.size == config_[i].size\n> > >\n> > > However I wonder why the condition was preceded by !colorSpace..\n> > > The same condition was expressed in\n> > > 5e5eadabd812 (\"libcamera: camera: Add validateColorSpaces to CameraConfiguration class\")\n> > >\n> > > as\n> > >\n> > >                } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {\n> > >                        index = i;\n> > >\n> > > And I guess it was because the second part of the condition was always\n> > > false :)\n> > >\n> > > Now that I've tracked down the full history of the bug\n> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > >\n> > > Thanks\n> > >   j\n> > >\n> > > >                       colorSpace = cfg.colorSpace;\n> > > > +                     size = cfg.size;\n> > > > +             }\n> > > >       }\n> > > >\n> > > >       if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> > > > --\n> > > > 2.30.2\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 100BCC322E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Jan 2023 10:20:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 87765625DE;\n\tFri,  6 Jan 2023 11:20:43 +0100 (CET)","from mail-oi1-x232.google.com (mail-oi1-x232.google.com\n\t[IPv6:2607:f8b0:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16FB861F09\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Jan 2023 11:20:42 +0100 (CET)","by mail-oi1-x232.google.com with SMTP id s66so90688oib.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 06 Jan 2023 02:20:42 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673000443;\n\tbh=QeSAsehNxvjvFSJNufyiDDLMeo1oJSw6ZUCsVXl7YfU=;\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=gnLExT1QFlna0QmddpSxyBVwQKk6nyjczH/hO2+WUERmWGXQgQYZOamcRTK0pmDpX\n\tlGY1JcCZiiPg4eC0WTbcEd1Ja3IKFCAQCnXi8Dqns9STtJjqWxz6sm3Ka0cz0KI7tj\n\t13YmP8Ud22/YJz5c+mlz72zW93iFCPfI4Aw1ne1Sa0l+AQ+YbXSo6ok8ldZ8YX7rmj\n\tDk5i62z9/SWarMskuQLKCgTgAibtmeFsZ2o2+ZM7Xozue/NwgTaAFG51g+c1fqSZzr\n\twierD7dbvGW26jn8W8kmTUoY1IOwYn8A1WG8cKGL38ioOfToF5Yk3U9iCO6KvJEoC5\n\tOZD9gD0cOoYsw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=INZ+jVYm8SjvcCx5N57tAKqr5z4WokZkw/Z2W5aiD2Q=;\n\tb=CSGQBFvyfaRL7Iu1zDYI4LpkV31DYNFXCBmb/Z+kcNHAsHqKkMNOBZxnI2dvLeZnhp\n\tKLd1hhdPEM9XG+IdLUtyXOYRh4+XhipCaKlQpZ5KWl4Sqr6qgECJdP+Oywm0F8fdxJbj\n\txL1FdS1YpCOTATh+cWuiyIpqa9hrtoxNDHq2RNX3yDVblv26e1XgTChj0Luxq7+OjwC1\n\tkUXFuG/sHhlqiECC8IqEJ25qBQYYQbYy3wP6lnaJkA5kSX9Bk/6ZVj/XPKJWdv9NZu6h\n\tZ47u3l4/fpQSzh4Iz+1UOyKt5Za6+CQVK64QUIfmhkmNGU9vJw9B0Q8QENWqf54yhTgZ\n\tKRyA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"CSGQBFvy\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=INZ+jVYm8SjvcCx5N57tAKqr5z4WokZkw/Z2W5aiD2Q=;\n\tb=4ireUzmnYU2TY7k5vqsLoYSTxmW9fetXSN6QFAHIkvMOtxajv5VzqC2lW9JOZ4BaO0\n\t1cIGpFnLluqZlJqFzAoXgHYR2hSOesawBk7qaNil3epb1MF9JWgH0kz6IFASwq/NKlLr\n\tZkmLfmG2desmcTujIl53r8hmznwzZQaw/vbsUOWcUHVoo7j2/59rDdzIXYG0O1pXmoHI\n\tSpffCDCVzSVPnh2z7TNvoLPYz7cn/P+nlBBslkMv2soApzYfkWLgrGgUxbPMPgZ5UHby\n\twAA82yA5WwUIwODrv9ME+qZx68EKAwbXfufafvic4MN1lPhCEH+IyYfs1eiPor3d+dnu\n\tqv0g==","X-Gm-Message-State":"AFqh2kru0rDs5RKkrqevB95iPezrXU8aVoglMG/QW53WnTlIQhReZ3s4\n\tdXFy5G5vKDtuTeFrhHU3pVozDwBK3KScoBqj3saY3etQSFDfQA==","X-Google-Smtp-Source":"AMrXdXtHBo8iC0ETriXq98jykADG/a1DZGMCy4D4l9SSpj7eW4QXeVADT2NQXz+PdVXo1unKqiG8JeycpIiFZt53ffk=","X-Received":"by 2002:a05:6808:196:b0:360:fbf1:4489 with SMTP id\n\tw22-20020a056808019600b00360fbf14489mr2974200oic.20.1673000440616;\n\tFri, 06 Jan 2023 02:20:40 -0800 (PST)","MIME-Version":"1.0","References":"<20230103113313.5423-1-david.plowman@raspberrypi.com>\n\t<20230103113313.5423-2-david.plowman@raspberrypi.com>\n\t<20230103173658.nk2oqeoh5mkvobsi@uno.localdomain>\n\t<CAHW6GYKf_GT+B+xEiOjSpY1Uea6pxFM+N0K-kSSUWnruDs8Ttg@mail.gmail.com>\n\t<20230105092358.l7utaeswnz3c4vqb@uno.localdomain>","In-Reply-To":"<20230105092358.l7utaeswnz3c4vqb@uno.localdomain>","Date":"Fri, 6 Jan 2023 10:20:29 +0000","Message-ID":"<CAHW6GYL9T7mJa5snMq3X+7ns41mBmQL610i_Zh80ek_9c8gRnA@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix\n\tvalidateColorSpaces to choose the correct \"main\" colour space","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]