[{"id":20975,"web_url":"https://patchwork.libcamera.org/comment/20975/","msgid":"<15acc602-48ac-87ed-d8b2-6d1ecdca4c3b@ideasonboard.com>","date":"2021-11-17T10:09:55","subject":"Re: [libcamera-devel] [PATCH 1/5] ipa: ipu3: agc: Drop\n\tkMaxLuminance constant","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nThanks for the patch !\n\nOn 16/11/2021 17:26, Laurent Pinchart wrote:\n> The kMaxLuminance constant is badly named, it's not a maximum luminance,\n> but the maximum integer value output by the AWB statistics engine for\n> per-channel averages. The constant is used in a single place, hardcoding\n> the value is actually more readable.\n> \n> Signed-off-by: Laurent Pinchart \n\nI hesitated and made it a constexpr to avoid \"magic values\"... :-)\n\nReviewed-by: Jean-Michel Hautbois \n\n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 6 +-----\n> 1 file changed, 1 insertion(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index bd02c474611c..43a39ffd57d6 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5;\n> /* Number of frames to wait before calculating stats on minimum exposure */\n> static constexpr uint32_t kNumStartupFrames = 10;\n> \n> -/* Maximum luminance used for brightness normalization */\n> -static constexpr uint32_t kMaxLuminance = 255;\n> -\n> /*\n> * Normalized luma value target.\n> *\n> @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> \t\t greenSum * frameContext.awb.gains.green * .587 +\n> \t\t blueSum * frameContext.awb.gains.blue * .114;\n> \n> -\t/* Return the normalized relative luminance. */\n> -\treturn Y_sum / (grid.height * grid.width) / kMaxLuminance;\n> +\treturn Y_sum / (grid.height * grid.width) / 255;\n> }\n> \n> /**\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0872ABF415\n\tfor ;\n\tWed, 17 Nov 2021 10:10:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0DBD6037A;\n\tWed, 17 Nov 2021 11:09:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0418600B5\n\tfor ;\n\tWed, 17 Nov 2021 11:09:57 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:f372:e247:b88:b5dd] (unknown\n\t[IPv6:2a01:e0a:169:7140:f372:e247:b88:b5dd])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 645B1E7;\n\tWed, 17 Nov 2021 11:09: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=\"UK1w6XFh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637143797;\n\tbh=5XN904hrXGPCH7Gy3vLxOyqVsGNlZZpNlROfso8O3Lo=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=UK1w6XFhop70UgT9RanOe7ciuzgRTa/IZVHY0IoYDKWblLu6SlfEc2Qx/nZX92WX2\n\t7yW8JUQrOvf1ImuCBMAShmu9OnVUs1S+dCEVRLM9jpnqBrfCLhwfnbt/SbG6fk1sJ2\n\tNXQlhjt7vNqAttSR7Q8XZZiIMxRCfIL5r5CktUcY=","Message-ID":"<15acc602-48ac-87ed-d8b2-6d1ecdca4c3b@ideasonboard.com>","Date":"Wed, 17 Nov 2021 11:09:55 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211116162615.27777-1-laurent.pinchart@ideasonboard.com>\n\t<20211116162615.27777-2-laurent.pinchart@ideasonboard.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<20211116162615.27777-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/5] ipa: ipu3: agc: Drop\n\tkMaxLuminance constant","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":20976,"web_url":"https://patchwork.libcamera.org/comment/20976/","msgid":"","date":"2021-11-17T10:12:56","subject":"Re: [libcamera-devel] [PATCH 1/5] ipa: ipu3: agc: Drop\n\tkMaxLuminance constant","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Wed, Nov 17, 2021 at 11:09:55AM +0100, Jean-Michel Hautbois wrote:\n> Hi Laurent,\n> \n> Thanks for the patch !\n> \n> On 16/11/2021 17:26, Laurent Pinchart wrote:\n> > The kMaxLuminance constant is badly named, it's not a maximum luminance,\n> > but the maximum integer value output by the AWB statistics engine for\n> > per-channel averages. The constant is used in a single place, hardcoding\n> > the value is actually more readable.\n> > \n> > Signed-off-by: Laurent Pinchart \n> \n> I hesitated and made it a constexpr to avoid \"magic values\"... :-)\n\nI also prefer avoiding magic values. Here I think \"kMaxLuminance\" isn't\na very good name. It's really the maximum value of the RGB averages\ncomputed by the ImgU in AWB statistics. As it's only used in a single\nfunction, and as I couldn't think of a good name for the constant, I\ndecided to drop it.\n\n> Reviewed-by: Jean-Michel Hautbois \n> \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 6 +-----\n> > 1 file changed, 1 insertion(+), 5 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index bd02c474611c..43a39ffd57d6 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5;\n> > /* Number of frames to wait before calculating stats on minimum exposure */\n> > static constexpr uint32_t kNumStartupFrames = 10;\n> > \n> > -/* Maximum luminance used for brightness normalization */\n> > -static constexpr uint32_t kMaxLuminance = 255;\n> > -\n> > /*\n> > * Normalized luma value target.\n> > *\n> > @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> > \t\t greenSum * frameContext.awb.gains.green * .587 +\n> > \t\t blueSum * frameContext.awb.gains.blue * .114;\n> > \n> > -\t/* Return the normalized relative luminance. */\n> > -\treturn Y_sum / (grid.height * grid.width) / kMaxLuminance;\n> > +\treturn Y_sum / (grid.height * grid.width) / 255;\n> > }\n> > \n> > /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0A110BDB1C\n\tfor ;\n\tWed, 17 Nov 2021 10:13:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5514F60376;\n\tWed, 17 Nov 2021 11:13: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 C7F4D600B5\n\tfor ;\n\tWed, 17 Nov 2021 11:13:18 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 67798E7;\n\tWed, 17 Nov 2021 11:13:18 +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=\"XC9WohFA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637143998;\n\tbh=/aUOG01jmc5g5KldWgeBmx7bJbsQ9+fdUtdv031tFwQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XC9WohFAqKyOJBpDy92RxtVaC1BawsSX0Xxc2C2TYo44H7/l7dim5kfXI1IZnI0IM\n\tiG6M6Qox/xeZHFLC7RmUnpvkyDvTon4Jw7WBNVtpo8XEnAYHT6XxDDhRSs1MlUG3XP\n\tQdQsjj4BZnn5MQBaVMDlFq65TJ9Q/6a/NRPoBjeU=","Date":"Wed, 17 Nov 2021 12:12:56 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211116162615.27777-1-laurent.pinchart@ideasonboard.com>\n\t<20211116162615.27777-2-laurent.pinchart@ideasonboard.com>\n\t<15acc602-48ac-87ed-d8b2-6d1ecdca4c3b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<15acc602-48ac-87ed-d8b2-6d1ecdca4c3b@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/5] ipa: ipu3: agc: Drop\n\tkMaxLuminance constant","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20994,"web_url":"https://patchwork.libcamera.org/comment/20994/","msgid":"<163722831113.420308.17971774516730901062@Monstersaurus>","date":"2021-11-18T09:38:31","subject":"Re: [libcamera-devel] [PATCH 1/5] ipa: ipu3: agc: Drop\n\tkMaxLuminance constant","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-11-17 10:12:56)\n> Hi Jean-Michel,\n> \n> On Wed, Nov 17, 2021 at 11:09:55AM +0100, Jean-Michel Hautbois wrote:\n> > Hi Laurent,\n> > \n> > Thanks for the patch !\n> > \n> > On 16/11/2021 17:26, Laurent Pinchart wrote:\n> > > The kMaxLuminance constant is badly named, it's not a maximum luminance,\n> > > but the maximum integer value output by the AWB statistics engine for\n> > > per-channel averages. The constant is used in a single place, hardcoding\n> > > the value is actually more readable.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > \n> > I hesitated and made it a constexpr to avoid \"magic values\"... :-)\n> \n> I also prefer avoiding magic values. Here I think \"kMaxLuminance\" isn't\n> a very good name. It's really the maximum value of the RGB averages\n> computed by the ImgU in AWB statistics. As it's only used in a single\n> function, and as I couldn't think of a good name for the constant, I\n> decided to drop it.\n\nI thought MaxLuminance conveyed that. As in - the maximum lumincance is\n255, so the maximum average possible is 255.\n\nBut I don't mind it being hardcoded if that's prefered.\n\n\nReviewed-by: Kieran Bingham \n\n> > Reviewed-by: Jean-Michel Hautbois \n> > \n> > > ---\n> > > src/ipa/ipu3/algorithms/agc.cpp | 6 +-----\n> > > 1 file changed, 1 insertion(+), 5 deletions(-)\n> > > \n> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > > index bd02c474611c..43a39ffd57d6 100644\n> > > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > > @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5;\n> > > /* Number of frames to wait before calculating stats on minimum exposure */\n> > > static constexpr uint32_t kNumStartupFrames = 10;\n> > > \n> > > -/* Maximum luminance used for brightness normalization */\n> > > -static constexpr uint32_t kMaxLuminance = 255;\n> > > -\n> > > /*\n> > > * Normalized luma value target.\n> > > *\n> > > @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> > > greenSum * frameContext.awb.gains.green * .587 +\n> > > blueSum * frameContext.awb.gains.blue * .114;\n> > > \n> > > - /* Return the normalized relative luminance. */\n> > > - return Y_sum / (grid.height * grid.width) / kMaxLuminance;\n> > > + return Y_sum / (grid.height * grid.width) / 255;\n> > > }\n> > > \n> > > /**\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 5499DBDB1C\n\tfor ;\n\tThu, 18 Nov 2021 09:38:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 73EA360233;\n\tThu, 18 Nov 2021 10:38:36 +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 2118E60230\n\tfor ;\n\tThu, 18 Nov 2021 10:38:34 +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 99E993E5;\n\tThu, 18 Nov 2021 10:38:33 +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=\"S85rNkgR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637228313;\n\tbh=J3WGVrb5u/9RiEGPypjRuhRcbAJ8sLFqoSZZfUUj31w=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=S85rNkgRtatE+cQDZIBM+XwsRhCMq0iqwi+CwT/45zXD/nClkXwF7RqUAor6hov+L\n\t8lMyTdKjSZPiukeSru01lWnbsMvpsw655eHpptyDgmNIkOL48g2Qepbjw+jQazTTtv\n\t2uz/GbrNE4kkm289tUHtTstAB+9lBzf6QzNTmbC0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211116162615.27777-1-laurent.pinchart@ideasonboard.com>\n\t<20211116162615.27777-2-laurent.pinchart@ideasonboard.com>\n\t<15acc602-48ac-87ed-d8b2-6d1ecdca4c3b@ideasonboard.com>\n\t","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tLaurent Pinchart ","Date":"Thu, 18 Nov 2021 09:38:31 +0000","Message-ID":"<163722831113.420308.17971774516730901062@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/5] ipa: ipu3: agc: Drop\n\tkMaxLuminance constant","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]