[{"id":33430,"web_url":"https://patchwork.libcamera.org/comment/33430/","msgid":"","date":"2025-02-24T09:13:33","subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch. \n\nOn Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote:\n> The lux value can never be negative. Pass it as an unsigned int.\n> \n\nI don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe\none exception). They just produce too many ways to create hidden bugs.\nBut so be it :-)\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> Do we need a Lux type ? And maybe a ColourTemperature type ?\n> ---\n> src/ipa/libipa/awb.h | 2 +-\n> src/ipa/libipa/awb_bayes.cpp | 2 +-\n> src/ipa/libipa/awb_bayes.h | 2 +-\n> src/ipa/libipa/awb_grey.cpp | 2 +-\n> src/ipa/libipa/awb_grey.h | 2 +-\n> 5 files changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> index 4a1b012a4334..5c298d3b6f69 100644\n> --- a/src/ipa/libipa/awb.h\n> +++ b/src/ipa/libipa/awb.h\n> @@ -35,7 +35,7 @@ public:\n> \tvirtual ~AwbAlgorithm() = default;\n> \n> \tvirtual int init(const YamlObject &tuningData) = 0;\n> -\tvirtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0;\n> +\tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> \tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> \n> \tconst ControlInfoMap::Map &controls() const\n> diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> index e75bfcd693d9..9287b884cb95 100644\n> --- a/src/ipa/libipa/awb_bayes.cpp\n> +++ b/src/ipa/libipa/awb_bayes.cpp\n> @@ -275,7 +275,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> \treturn { { gains[0], 1.0, gains[1] } };\n> }\n> \n> -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux)\n> +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> {\n> \tipa::Pwl prior;\n> \tif (lux > 0) {\n\nThis if clause should then be removed, too.\n\nRegards,\nStefan\n\n> diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> index 47db7243273f..23bf88061118 100644\n> --- a/src/ipa/libipa/awb_bayes.h\n> +++ b/src/ipa/libipa/awb_bayes.h\n> @@ -34,7 +34,7 @@ public:\n> \tAwbBayes() = default;\n> \n> \tint init(const YamlObject &tuningData) override;\n> -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> \tRGB gainsFromColourTemperature(double temperatureK) override;\n> \tvoid handleControls(const ControlList &controls) override;\n> \n> diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> index 06ffd45618d8..e979cc4d1a3e 100644\n> --- a/src/ipa/libipa/awb_grey.cpp\n> +++ b/src/ipa/libipa/awb_grey.cpp\n> @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData)\n> *\n> * \\return The AWB result\n> */\n> -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)\n> +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux)\n> {\n> \tAwbResult result;\n> \tauto means = stats.getRGBMeans();\n> diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> index 1a365e616a98..e3c34201dbc9 100644\n> --- a/src/ipa/libipa/awb_grey.h\n> +++ b/src/ipa/libipa/awb_grey.h\n> @@ -23,7 +23,7 @@ public:\n> \tAwbGrey() = default;\n> \n> \tint init(const YamlObject &tuningData) override;\n> -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> \tRGB gainsFromColourTemperature(double colourTemperature) override;\n> \n> private:\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 0A90CC32A9\n\tfor ;\n\tMon, 24 Feb 2025 09:13:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 474B9686CB;\n\tMon, 24 Feb 2025 10:13:38 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CBBEE686A6\n\tfor ;\n\tMon, 24 Feb 2025 10:13:36 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c02b:2ffa:8001:90d4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65628220;\n\tMon, 24 Feb 2025 10:12:10 +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=\"F3AlbOtw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740388330;\n\tbh=r8zdoon0vR/QLOvoxyuKE0YVBjOJDcJPQ+etthFTIsU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F3AlbOtwE/0nEhoogYUqCUbaKF1S2rWbyxqLmGn6w9zpEw1OrRVma+B2xNxqj4pi+\n\tpM6PT1ONYTXagxS1TjYcjOSbrGsmSPKV6hb+ZNStsekhRuIy7aeWIOsXHXbSle/CGv\n\tMLnvklfqjmjSiFIwB5ddxYcGkRwyWJqdSgraj7WQ=","Date":"Mon, 24 Feb 2025 10:13:33 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","Message-ID":"","References":"<20250223230403.1226-1-laurent.pinchart@ideasonboard.com>\n\t<20250223230403.1226-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250223230403.1226-7-laurent.pinchart@ideasonboard.com>","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":33434,"web_url":"https://patchwork.libcamera.org/comment/33434/","msgid":"<20250224092652.GB29646@pendragon.ideasonboard.com>","date":"2025-02-24T09:26:52","subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nOn Mon, Feb 24, 2025 at 10:13:33AM +0100, Stefan Klug wrote:\n> Hi Laurent,\n> \n> Thank you for the patch. \n> \n> On Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote:\n> > The lux value can never be negative. Pass it as an unsigned int.\n> \n> I don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe\n> one exception). They just produce too many ways to create hidden bugs.\n> But so be it :-)\n\nI think I recall we discussed pros and cons.\n\nIt doesn't address the signedness issue in general, but how about\ncreating a Lux type ?\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Do we need a Lux type ? And maybe a ColourTemperature type ?\n> > ---\n> > src/ipa/libipa/awb.h | 2 +-\n> > src/ipa/libipa/awb_bayes.cpp | 2 +-\n> > src/ipa/libipa/awb_bayes.h | 2 +-\n> > src/ipa/libipa/awb_grey.cpp | 2 +-\n> > src/ipa/libipa/awb_grey.h | 2 +-\n> > 5 files changed, 5 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> > index 4a1b012a4334..5c298d3b6f69 100644\n> > --- a/src/ipa/libipa/awb.h\n> > +++ b/src/ipa/libipa/awb.h\n> > @@ -35,7 +35,7 @@ public:\n> > \tvirtual ~AwbAlgorithm() = default;\n> > \n> > \tvirtual int init(const YamlObject &tuningData) = 0;\n> > -\tvirtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0;\n> > +\tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> > \tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> > \n> > \tconst ControlInfoMap::Map &controls() const\n> > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> > index e75bfcd693d9..9287b884cb95 100644\n> > --- a/src/ipa/libipa/awb_bayes.cpp\n> > +++ b/src/ipa/libipa/awb_bayes.cpp\n> > @@ -275,7 +275,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > \treturn { { gains[0], 1.0, gains[1] } };\n> > }\n> > \n> > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux)\n> > +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> > {\n> > \tipa::Pwl prior;\n> > \tif (lux > 0) {\n> \n> This if clause should then be removed, too.\n\nNo, this tests for > 0, not >= 0. 0 is still a magic value. a\nstd::optional would express that better if we want to go\nthat way.\n\n> > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> > index 47db7243273f..23bf88061118 100644\n> > --- a/src/ipa/libipa/awb_bayes.h\n> > +++ b/src/ipa/libipa/awb_bayes.h\n> > @@ -34,7 +34,7 @@ public:\n> > \tAwbBayes() = default;\n> > \n> > \tint init(const YamlObject &tuningData) override;\n> > -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> > +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > \tRGB gainsFromColourTemperature(double temperatureK) override;\n> > \tvoid handleControls(const ControlList &controls) override;\n> > \n> > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> > index 06ffd45618d8..e979cc4d1a3e 100644\n> > --- a/src/ipa/libipa/awb_grey.cpp\n> > +++ b/src/ipa/libipa/awb_grey.cpp\n> > @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData)\n> > *\n> > * \\return The AWB result\n> > */\n> > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)\n> > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux)\n> > {\n> > \tAwbResult result;\n> > \tauto means = stats.getRGBMeans();\n> > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> > index 1a365e616a98..e3c34201dbc9 100644\n> > --- a/src/ipa/libipa/awb_grey.h\n> > +++ b/src/ipa/libipa/awb_grey.h\n> > @@ -23,7 +23,7 @@ public:\n> > \tAwbGrey() = default;\n> > \n> > \tint init(const YamlObject &tuningData) override;\n> > -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> > +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > \tRGB gainsFromColourTemperature(double colourTemperature) override;\n> > \n> > private:","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 36864C32A9\n\tfor ;\n\tMon, 24 Feb 2025 09:27:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19028686D1;\n\tMon, 24 Feb 2025 10:27:11 +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 D7062686CB\n\tfor ;\n\tMon, 24 Feb 2025 10:27:09 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 558FF220;\n\tMon, 24 Feb 2025 10:25:43 +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=\"h11AvnIS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740389143;\n\tbh=6Khii/Oj4jIS7eto7KmLbb4hheQTP57K3Lq7FPAxeUc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h11AvnISjFs5KTG3kn/Q23dWiFKMk6fac20yMsBFji7NRqxtFZ0ozELupRVuSz2N/\n\tVp9ey+FWDSdccSNKrbCUP6cUMq5d6C21GrwXaFDnO1VMJdKNT9xrGCyVKujUrpCUNa\n\tMxUFgcZNXY4QZScaiNSPTC7drSbWbSQ0O2fmTGrE=","Date":"Mon, 24 Feb 2025 11:26:52 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","Message-ID":"<20250224092652.GB29646@pendragon.ideasonboard.com>","References":"<20250223230403.1226-1-laurent.pinchart@ideasonboard.com>\n\t<20250223230403.1226-7-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":33444,"web_url":"https://patchwork.libcamera.org/comment/33444/","msgid":"<563jjz5mzdeatb6l6izgtnrlvd2k73eszpltjigg37ganhvbxz@gitwzsnw6y6m>","date":"2025-02-24T10:20:16","subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Mon, Feb 24, 2025 at 11:26:52AM +0200, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> On Mon, Feb 24, 2025 at 10:13:33AM +0100, Stefan Klug wrote:\n> > Hi Laurent,\n> > \n> > Thank you for the patch. \n> > \n> > On Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote:\n> > > The lux value can never be negative. Pass it as an unsigned int.\n> > \n> > I don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe\n> > one exception). They just produce too many ways to create hidden bugs.\n> > But so be it :-)\n> \n> I think I recall we discussed pros and cons.\n> \n> It doesn't address the signedness issue in general, but how about\n> creating a Lux type ?\n\nNo, please not. Then we also need to create a colour temperature type at\nno real benefit.\n\n> \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Do we need a Lux type ? And maybe a ColourTemperature type ?\n> > > ---\n> > > src/ipa/libipa/awb.h | 2 +-\n> > > src/ipa/libipa/awb_bayes.cpp | 2 +-\n> > > src/ipa/libipa/awb_bayes.h | 2 +-\n> > > src/ipa/libipa/awb_grey.cpp | 2 +-\n> > > src/ipa/libipa/awb_grey.h | 2 +-\n> > > 5 files changed, 5 insertions(+), 5 deletions(-)\n> > > \n> > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> > > index 4a1b012a4334..5c298d3b6f69 100644\n> > > --- a/src/ipa/libipa/awb.h\n> > > +++ b/src/ipa/libipa/awb.h\n> > > @@ -35,7 +35,7 @@ public:\n> > > \tvirtual ~AwbAlgorithm() = default;\n> > > \n> > > \tvirtual int init(const YamlObject &tuningData) = 0;\n> > > -\tvirtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0;\n> > > +\tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> > > \tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> > > \n> > > \tconst ControlInfoMap::Map &controls() const\n> > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> > > index e75bfcd693d9..9287b884cb95 100644\n> > > --- a/src/ipa/libipa/awb_bayes.cpp\n> > > +++ b/src/ipa/libipa/awb_bayes.cpp\n> > > @@ -275,7 +275,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > \treturn { { gains[0], 1.0, gains[1] } };\n> > > }\n> > > \n> > > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux)\n> > > +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> > > {\n> > > \tipa::Pwl prior;\n> > > \tif (lux > 0) {\n> > \n> > This if clause should then be removed, too.\n> \n> No, this tests for > 0, not >= 0. 0 is still a magic value. a\n> std::optional would express that better if we want to go\n> that way.\n\nUps. Right. Lets keep that one simple (stick to the uint)\n\nReviewed-by: Stefan Klug \n\n> \n> > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> > > index 47db7243273f..23bf88061118 100644\n> > > --- a/src/ipa/libipa/awb_bayes.h\n> > > +++ b/src/ipa/libipa/awb_bayes.h\n> > > @@ -34,7 +34,7 @@ public:\n> > > \tAwbBayes() = default;\n> > > \n> > > \tint init(const YamlObject &tuningData) override;\n> > > -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> > > +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > \tRGB gainsFromColourTemperature(double temperatureK) override;\n> > > \tvoid handleControls(const ControlList &controls) override;\n> > > \n> > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> > > index 06ffd45618d8..e979cc4d1a3e 100644\n> > > --- a/src/ipa/libipa/awb_grey.cpp\n> > > +++ b/src/ipa/libipa/awb_grey.cpp\n> > > @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData)\n> > > *\n> > > * \\return The AWB result\n> > > */\n> > > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)\n> > > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux)\n> > > {\n> > > \tAwbResult result;\n> > > \tauto means = stats.getRGBMeans();\n> > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> > > index 1a365e616a98..e3c34201dbc9 100644\n> > > --- a/src/ipa/libipa/awb_grey.h\n> > > +++ b/src/ipa/libipa/awb_grey.h\n> > > @@ -23,7 +23,7 @@ public:\n> > > \tAwbGrey() = default;\n> > > \n> > > \tint init(const YamlObject &tuningData) override;\n> > > -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> > > +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > \tRGB gainsFromColourTemperature(double colourTemperature) override;\n> > > \n> > > private:\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 A5419C324E\n\tfor ;\n\tMon, 24 Feb 2025 10:20:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C672C686DE;\n\tMon, 24 Feb 2025 11:20:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71DB4686A6\n\tfor ;\n\tMon, 24 Feb 2025 11:20:18 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c02b:2ffa:8001:90d4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B0C0220;\n\tMon, 24 Feb 2025 11:18:52 +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=\"VGc5q+qU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740392332;\n\tbh=GOWmnDdREDAytr0l0WqMHc/dvhnnW0a54GOS+eIRkLs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VGc5q+qUhhyChlCiqMPjqx+BFPpDkQU0B7AAHGcmI6KvOZV+MxpFQMPWgTZtg7pK9\n\tRLPPB7dil9e8NYZqlRvWK0dYGFbhe4Fq7g+CmI+wbAvf77sZWhrMoer2KmkHva88f/\n\tzyi83jMQrPi7+AzkMF35UuZ3HeJg/ljKb2JuGXkY=","Date":"Mon, 24 Feb 2025 11:20:16 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","Message-ID":"<563jjz5mzdeatb6l6izgtnrlvd2k73eszpltjigg37ganhvbxz@gitwzsnw6y6m>","References":"<20250223230403.1226-1-laurent.pinchart@ideasonboard.com>\n\t<20250223230403.1226-7-laurent.pinchart@ideasonboard.com>\n\t\n\t<20250224092652.GB29646@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250224092652.GB29646@pendragon.ideasonboard.com>","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":33449,"web_url":"https://patchwork.libcamera.org/comment/33449/","msgid":"<20250224132405.GD25447@pendragon.ideasonboard.com>","date":"2025-02-24T13:24:05","subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Feb 24, 2025 at 11:20:16AM +0100, Stefan Klug wrote:\n> On Mon, Feb 24, 2025 at 11:26:52AM +0200, Laurent Pinchart wrote:\n> > On Mon, Feb 24, 2025 at 10:13:33AM +0100, Stefan Klug wrote:\n> > > On Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote:\n> > > > The lux value can never be negative. Pass it as an unsigned int.\n> > > \n> > > I don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe\n> > > one exception). They just produce too many ways to create hidden bugs.\n> > > But so be it :-)\n> > \n> > I think I recall we discussed pros and cons.\n> > \n> > It doesn't address the signedness issue in general, but how about\n> > creating a Lux type ?\n> \n> No, please not. Then we also need to create a colour temperature type at\n> no real benefit.\n\nThe idea was to standardize the representation. We have integer vs.\ndouble used in different places. I'm not pushing strongly for this\nthough.\n\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > Do we need a Lux type ? And maybe a ColourTemperature type ?\n> > > > ---\n> > > > src/ipa/libipa/awb.h | 2 +-\n> > > > src/ipa/libipa/awb_bayes.cpp | 2 +-\n> > > > src/ipa/libipa/awb_bayes.h | 2 +-\n> > > > src/ipa/libipa/awb_grey.cpp | 2 +-\n> > > > src/ipa/libipa/awb_grey.h | 2 +-\n> > > > 5 files changed, 5 insertions(+), 5 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h\n> > > > index 4a1b012a4334..5c298d3b6f69 100644\n> > > > --- a/src/ipa/libipa/awb.h\n> > > > +++ b/src/ipa/libipa/awb.h\n> > > > @@ -35,7 +35,7 @@ public:\n> > > > \tvirtual ~AwbAlgorithm() = default;\n> > > > \n> > > > \tvirtual int init(const YamlObject &tuningData) = 0;\n> > > > -\tvirtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0;\n> > > > +\tvirtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;\n> > > > \tvirtual RGB gainsFromColourTemperature(double colourTemperature) = 0;\n> > > > \n> > > > \tconst ControlInfoMap::Map &controls() const\n> > > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp\n> > > > index e75bfcd693d9..9287b884cb95 100644\n> > > > --- a/src/ipa/libipa/awb_bayes.cpp\n> > > > +++ b/src/ipa/libipa/awb_bayes.cpp\n> > > > @@ -275,7 +275,7 @@ RGB AwbBayes::gainsFromColourTemperature(double colourTemperature)\n> > > > \treturn { { gains[0], 1.0, gains[1] } };\n> > > > }\n> > > > \n> > > > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux)\n> > > > +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)\n> > > > {\n> > > > \tipa::Pwl prior;\n> > > > \tif (lux > 0) {\n> > > \n> > > This if clause should then be removed, too.\n> > \n> > No, this tests for > 0, not >= 0. 0 is still a magic value. a\n> > std::optional would express that better if we want to go\n> > that way.\n> \n> Ups. Right. Lets keep that one simple (stick to the uint)\n> \n> Reviewed-by: Stefan Klug \n> \n> > > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h\n> > > > index 47db7243273f..23bf88061118 100644\n> > > > --- a/src/ipa/libipa/awb_bayes.h\n> > > > +++ b/src/ipa/libipa/awb_bayes.h\n> > > > @@ -34,7 +34,7 @@ public:\n> > > > \tAwbBayes() = default;\n> > > > \n> > > > \tint init(const YamlObject &tuningData) override;\n> > > > -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> > > > +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > > \tRGB gainsFromColourTemperature(double temperatureK) override;\n> > > > \tvoid handleControls(const ControlList &controls) override;\n> > > > \n> > > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp\n> > > > index 06ffd45618d8..e979cc4d1a3e 100644\n> > > > --- a/src/ipa/libipa/awb_grey.cpp\n> > > > +++ b/src/ipa/libipa/awb_grey.cpp\n> > > > @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData)\n> > > > *\n> > > > * \\return The AWB result\n> > > > */\n> > > > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux)\n> > > > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux)\n> > > > {\n> > > > \tAwbResult result;\n> > > > \tauto means = stats.getRGBMeans();\n> > > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h\n> > > > index 1a365e616a98..e3c34201dbc9 100644\n> > > > --- a/src/ipa/libipa/awb_grey.h\n> > > > +++ b/src/ipa/libipa/awb_grey.h\n> > > > @@ -23,7 +23,7 @@ public:\n> > > > \tAwbGrey() = default;\n> > > > \n> > > > \tint init(const YamlObject &tuningData) override;\n> > > > -\tAwbResult calculateAwb(const AwbStats &stats, int lux) override;\n> > > > +\tAwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;\n> > > > \tRGB gainsFromColourTemperature(double colourTemperature) override;\n> > > > \n> > > > private:","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 5FABFC32A9\n\tfor ;\n\tMon, 24 Feb 2025 13:24:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B8B6686ED;\n\tMon, 24 Feb 2025 14:24: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 D343961856\n\tfor ;\n\tMon, 24 Feb 2025 14:24:23 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2F25A455;\n\tMon, 24 Feb 2025 14:22:57 +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=\"mZwKFk+S\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740403377;\n\tbh=XFno/sbdgQljagwfV8aY9zovtLWxMnDjYRkTxvtHIuo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mZwKFk+SNtd9dx+LlptHMlyOxvOkDYBg7zbzKCGinD3u+/OhQ0jlf4fgzF+40oj9q\n\tMXoRWSqmcWZEjbIgpOX90TcPH2Wt4R42XxhEd1FOv56qd/XA81iuv9G09qPHR5za1s\n\tEa859wIxqzrCmXJOIuwnpzqI0TO7icoDUHXUT1lI=","Date":"Mon, 24 Feb 2025 15:24:05 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 06/15] libipa: awb: Pass lux value to calculateAwb() as\n\tunsigned int","Message-ID":"<20250224132405.GD25447@pendragon.ideasonboard.com>","References":"<20250223230403.1226-1-laurent.pinchart@ideasonboard.com>\n\t<20250223230403.1226-7-laurent.pinchart@ideasonboard.com>\n\t\n\t<20250224092652.GB29646@pendragon.ideasonboard.com>\n\t<563jjz5mzdeatb6l6izgtnrlvd2k73eszpltjigg37ganhvbxz@gitwzsnw6y6m>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<563jjz5mzdeatb6l6izgtnrlvd2k73eszpltjigg37ganhvbxz@gitwzsnw6y6m>","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\" "}}]