[{"id":20977,"web_url":"https://patchwork.libcamera.org/comment/20977/","msgid":"<86e5a845-da91-dd3d-ad55-d9cccd49aa6e@ideasonboard.com>","date":"2021-11-17T10:15:23","subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: ipu3: agc: Standardize\n\tvocabulary on \"relative luminance\"","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 AGC computes the average relative luminance of the frame and calls\n> the value \"normalized luma\", \"brightness\" or \"initialY\". The latter is\n> the most accurate term, as the relative luminance is abbreviated Y, but\n> the \"initial\" prefix isn't accurate.\n> \n> Standardize the vocabulary on \"relative luminance\" in code and comments,\n> abbreviating it to Y when needed.\n> \n> While at it, rename variables to match the libcamera coding style.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 44 ++++++++++++++++-----------------\n> src/ipa/ipu3/algorithms/agc.h | 8 +++---\n> 2 files changed, 26 insertions(+), 26 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 43a39ffd57d6..d5840fb0aa97 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -62,12 +62,12 @@ static constexpr double kEvGainTarget = 0.5;\n> static constexpr uint32_t kNumStartupFrames = 10;\n> \n> /*\n> - * Normalized luma value target.\n> + * Relative luminance target.\n> *\n> * It's a number that's chosen so that, when the camera points at a grey\n> * target, the resulting image brightness is considered right.\n> */\n> -static constexpr double kNormalizedLumaTarget = 0.16;\n> +static constexpr double kRelativeLuminanceTarget = 0.16;\n> \n> Agc::Agc()\n> \t: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minShutterSpeed_(0s),\n> @@ -250,12 +250,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> }\n> \n> /**\n> - * \\brief Estimate the average brightness of the frame\n> + * \\brief Estimate the relative luminance of the frame with a given gain\n> * \\param[in] frameContext The shared IPA frame context\n> * \\param[in] grid The grid used to store the statistics in the IPU3\n> * \\param[in] stats The IPU3 statistics and ISP results\n> * \\param[in] currentYGain The gain calculated on the current brightness level\n> - * \\return The normalized luma\n> + * \\return The relative luminance\n> *\n> * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color\n> * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a\n> @@ -263,12 +263,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> * luma here.\n> *\n> * More detailed information can be found in:\n> - * https://en.wikipedia.org/wiki/Luma_(video)\n> + * https://en.wikipedia.org/wiki/Relative_luminance\n> */\n> -double Agc::computeInitialY(IPAFrameContext &frameContext,\n> -\t\t\t const ipu3_uapi_grid_config &grid,\n> -\t\t\t const ipu3_uapi_stats_3a *stats,\n> -\t\t\t double currentYGain)\n> +double Agc::estimateLuminance(IPAFrameContext &frameContext,\n> +\t\t\t const ipu3_uapi_grid_config &grid,\n> +\t\t\t const ipu3_uapi_stats_3a *stats,\n> +\t\t\t double currentYGain)\n\nShouldn't it be named estimateRelativeLuminance then ? Or is it too long \nmaybe...\n\n> {\n> \tdouble redSum = 0, greenSum = 0, blueSum = 0;\n> \n> @@ -288,14 +288,14 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> \t}\n> \n> \t/*\n> -\t * Estimate the sum of the brightness values, weighted with the gains\n> -\t * applied on the channels in AWB as the Rec. 601 luma.\n> +\t * Compute the relative luminance using the Rec. 601 formula, and\n> +\t * normalize it.\n> \t */\n\nRelative luminance is normalized, as it is between 0.0 and 1.0 :-).\nWhy did you remove the gain weights mention though ?\n\n> -\tdouble Y_sum = redSum * frameContext.awb.gains.red * .299 +\n> -\t\t greenSum * frameContext.awb.gains.green * .587 +\n> -\t\t blueSum * frameContext.awb.gains.blue * .114;\n> +\tdouble ySum = redSum * frameContext.awb.gains.red * .299 +\n> +\t\t greenSum * frameContext.awb.gains.green * .587 +\n> +\t\t blueSum * frameContext.awb.gains.blue * .114;\n> \n> -\treturn Y_sum / (grid.height * grid.width) / 255;\n> +\treturn ySum / (grid.height * grid.width) / 255;\n\nHow could we let that one... :-/\n\n> }\n> \n> /**\n> @@ -311,22 +311,22 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n> \n> \tdouble currentYGain = 1.0;\n> -\tdouble targetY = kNormalizedLumaTarget;\n> +\tdouble yTarget = kRelativeLuminanceTarget;\n> \n> \t/*\n> \t * Do this calculation a few times as brightness increase can be\n> \t * non-linear when there are saturated regions.\n> \t */\n> -\tfor (int i = 0; i < 8; i++) {\n> -\t\tdouble initialY = computeInitialY(context.frameContext,\n> +\tfor (unsigned int i = 0; i < 8; i++) {\n> +\t\tdouble yValue = estimateLuminance(context.frameContext,\n> \t\t\t\t\t\t context.configuration.grid.bdsGrid,\n> \t\t\t\t\t\t stats, currentYGain);\n> -\t\tdouble extra_gain = std::min(10.0, targetY / (initialY + .001));\n> +\t\tdouble extra_gain = std::min(10.0, yTarget / (yValue + .001));\n> \n> \t\tcurrentYGain *= extra_gain;\n> -\t\tLOG(IPU3Agc, Debug) << \"Initial Y \" << initialY\n> -\t\t\t\t << \" target \" << targetY\n> -\t\t\t\t << \" gives gain \" << currentYGain;\n> +\t\tLOG(IPU3Agc, Debug) << \"Y value: \" << yValue\n> +\t\t\t\t << \", Y target: \" << yTarget\n> +\t\t\t\t << \", gives gain \" << currentYGain;\n> \t\tif (extra_gain < 1.01)\n> \t\t\tbreak;\n> \t}\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 31c5a6e519d4..943c354a820e 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -35,10 +35,10 @@ private:\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> \tvoid filterExposure();\n> \tvoid computeExposure(IPAFrameContext &frameContext, double currentYGain);\n> -\tdouble computeInitialY(IPAFrameContext &frameContext,\n> -\t\t\t const ipu3_uapi_grid_config &grid,\n> -\t\t\t const ipu3_uapi_stats_3a *stats,\n> -\t\t\t double currentYGain);\n> +\tdouble estimateLuminance(IPAFrameContext &frameContext,\n> +\t\t\t\t const ipu3_uapi_grid_config &grid,\n> +\t\t\t\t const ipu3_uapi_stats_3a *stats,\n> +\t\t\t\t double currentYGain);\n> \n> \tuint64_t frameCount_;\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 911C7BDB1C\n\tfor ;\n\tWed, 17 Nov 2021 10:15:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9AD860376;\n\tWed, 17 Nov 2021 11:15:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18837600B5\n\tfor ;\n\tWed, 17 Nov 2021 11:15:26 +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 AF8B5E7;\n\tWed, 17 Nov 2021 11:15:25 +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=\"Oc3LRWzJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637144125;\n\tbh=e5PqfTz8pFR4fNM0w6tBP0ot2JuphtmXKjuSU8y65+s=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Oc3LRWzJKpeN5GO4msWpu9rP9Fktz+PvwaXXYjAiD0iCfedVXR8NZo9GRSRqt3Vg6\n\t6WMKERwKFMYMFkAZHrF/gIeYQOFvLVUAM4hLFivL8fAnQ7QgIm3xN9vlwQ3pWFE1Zk\n\trdYSckS4eROwqcbyNKWymoock1U04igS81rNHJKk=","Message-ID":"<86e5a845-da91-dd3d-ad55-d9cccd49aa6e@ideasonboard.com>","Date":"Wed, 17 Nov 2021 11:15:23 +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-3-laurent.pinchart@ideasonboard.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<20211116162615.27777-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: ipu3: agc: Standardize\n\tvocabulary on \"relative luminance\"","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":20980,"web_url":"https://patchwork.libcamera.org/comment/20980/","msgid":"","date":"2021-11-17T10:26:26","subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: ipu3: agc: Standardize\n\tvocabulary on \"relative luminance\"","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:15:23AM +0100, Jean-Michel Hautbois wrote:\n> On 16/11/2021 17:26, Laurent Pinchart wrote:\n> > The AGC computes the average relative luminance of the frame and calls\n> > the value \"normalized luma\", \"brightness\" or \"initialY\". The latter is\n> > the most accurate term, as the relative luminance is abbreviated Y, but\n> > the \"initial\" prefix isn't accurate.\n> > \n> > Standardize the vocabulary on \"relative luminance\" in code and comments,\n> > abbreviating it to Y when needed.\n> > \n> > While at it, rename variables to match the libcamera coding style.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 44 ++++++++++++++++-----------------\n> > src/ipa/ipu3/algorithms/agc.h | 8 +++---\n> > 2 files changed, 26 insertions(+), 26 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index 43a39ffd57d6..d5840fb0aa97 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -62,12 +62,12 @@ static constexpr double kEvGainTarget = 0.5;\n> > static constexpr uint32_t kNumStartupFrames = 10;\n> > \n> > /*\n> > - * Normalized luma value target.\n> > + * Relative luminance target.\n> > *\n> > * It's a number that's chosen so that, when the camera points at a grey\n> > * target, the resulting image brightness is considered right.\n> > */\n> > -static constexpr double kNormalizedLumaTarget = 0.16;\n> > +static constexpr double kRelativeLuminanceTarget = 0.16;\n> > \n> > Agc::Agc()\n> > \t: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minShutterSpeed_(0s),\n> > @@ -250,12 +250,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> > }\n> > \n> > /**\n> > - * \\brief Estimate the average brightness of the frame\n> > + * \\brief Estimate the relative luminance of the frame with a given gain\n> > * \\param[in] frameContext The shared IPA frame context\n> > * \\param[in] grid The grid used to store the statistics in the IPU3\n> > * \\param[in] stats The IPU3 statistics and ISP results\n> > * \\param[in] currentYGain The gain calculated on the current brightness level\n> > - * \\return The normalized luma\n> > + * \\return The relative luminance\n> > *\n> > * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color\n> > * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a\n> > @@ -263,12 +263,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> > * luma here.\n> > *\n> > * More detailed information can be found in:\n> > - * https://en.wikipedia.org/wiki/Luma_(video)\n> > + * https://en.wikipedia.org/wiki/Relative_luminance\n> > */\n> > -double Agc::computeInitialY(IPAFrameContext &frameContext,\n> > -\t\t\t const ipu3_uapi_grid_config &grid,\n> > -\t\t\t const ipu3_uapi_stats_3a *stats,\n> > -\t\t\t double currentYGain)\n> > +double Agc::estimateLuminance(IPAFrameContext &frameContext,\n> > +\t\t\t const ipu3_uapi_grid_config &grid,\n> > +\t\t\t const ipu3_uapi_stats_3a *stats,\n> > +\t\t\t double currentYGain)\n> \n> Shouldn't it be named estimateRelativeLuminance then ? Or is it too long \n> maybe...\n\nI found it a bit long. If you think it's better, I'm ok with it.\n\n> > {\n> > \tdouble redSum = 0, greenSum = 0, blueSum = 0;\n> > \n> > @@ -288,14 +288,14 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> > \t}\n> > \n> > \t/*\n> > -\t * Estimate the sum of the brightness values, weighted with the gains\n> > -\t * applied on the channels in AWB as the Rec. 601 luma.\n> > +\t * Compute the relative luminance using the Rec. 601 formula, and\n> > +\t * normalize it.\n> > \t */\n> \n> Relative luminance is normalized, as it is between 0.0 and 1.0 :-).\n> Why did you remove the gain weights mention though ?\n\nThose are not weights but gains. It should be mentioned though, I'll\nupdate it.\n\nWhy do we multiply by the AWB gains here ?\n\n> > -\tdouble Y_sum = redSum * frameContext.awb.gains.red * .299 +\n> > -\t\t greenSum * frameContext.awb.gains.green * .587 +\n> > -\t\t blueSum * frameContext.awb.gains.blue * .114;\n> > +\tdouble ySum = redSum * frameContext.awb.gains.red * .299 +\n> > +\t\t greenSum * frameContext.awb.gains.green * .587 +\n> > +\t\t blueSum * frameContext.awb.gains.blue * .114;\n> > \n> > -\treturn Y_sum / (grid.height * grid.width) / 255;\n> > +\treturn ySum / (grid.height * grid.width) / 255;\n> \n> How could we let that one... :-/\n> \n> > }\n> > \n> > /**\n> > @@ -311,22 +311,22 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> > \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n> > \n> > \tdouble currentYGain = 1.0;\n> > -\tdouble targetY = kNormalizedLumaTarget;\n> > +\tdouble yTarget = kRelativeLuminanceTarget;\n> > \n> > \t/*\n> > \t * Do this calculation a few times as brightness increase can be\n> > \t * non-linear when there are saturated regions.\n> > \t */\n> > -\tfor (int i = 0; i < 8; i++) {\n> > -\t\tdouble initialY = computeInitialY(context.frameContext,\n> > +\tfor (unsigned int i = 0; i < 8; i++) {\n> > +\t\tdouble yValue = estimateLuminance(context.frameContext,\n> > \t\t\t\t\t\t context.configuration.grid.bdsGrid,\n> > \t\t\t\t\t\t stats, currentYGain);\n> > -\t\tdouble extra_gain = std::min(10.0, targetY / (initialY + .001));\n> > +\t\tdouble extra_gain = std::min(10.0, yTarget / (yValue + .001));\n> > \n> > \t\tcurrentYGain *= extra_gain;\n> > -\t\tLOG(IPU3Agc, Debug) << \"Initial Y \" << initialY\n> > -\t\t\t\t << \" target \" << targetY\n> > -\t\t\t\t << \" gives gain \" << currentYGain;\n> > +\t\tLOG(IPU3Agc, Debug) << \"Y value: \" << yValue\n> > +\t\t\t\t << \", Y target: \" << yTarget\n> > +\t\t\t\t << \", gives gain \" << currentYGain;\n> > \t\tif (extra_gain < 1.01)\n> > \t\t\tbreak;\n> > \t}\n> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> > index 31c5a6e519d4..943c354a820e 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.h\n> > +++ b/src/ipa/ipu3/algorithms/agc.h\n> > @@ -35,10 +35,10 @@ private:\n> > \t\t\t const ipu3_uapi_grid_config &grid);\n> > \tvoid filterExposure();\n> > \tvoid computeExposure(IPAFrameContext &frameContext, double currentYGain);\n> > -\tdouble computeInitialY(IPAFrameContext &frameContext,\n> > -\t\t\t const ipu3_uapi_grid_config &grid,\n> > -\t\t\t const ipu3_uapi_stats_3a *stats,\n> > -\t\t\t double currentYGain);\n> > +\tdouble estimateLuminance(IPAFrameContext &frameContext,\n> > +\t\t\t\t const ipu3_uapi_grid_config &grid,\n> > +\t\t\t\t const ipu3_uapi_stats_3a *stats,\n> > +\t\t\t\t double currentYGain);\n> > \n> > \tuint64_t frameCount_;\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 7D054BDB1C\n\tfor ;\n\tWed, 17 Nov 2021 10:26:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFA1B60376;\n\tWed, 17 Nov 2021 11:26:50 +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 64495600B5\n\tfor ;\n\tWed, 17 Nov 2021 11:26:49 +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 DB48C51D;\n\tWed, 17 Nov 2021 11:26:48 +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=\"Ig1dFMeG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637144809;\n\tbh=dx0LPEynWfO/i2VA+FQOIte9km1NV2vwQsaBP0/b2vk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ig1dFMeGYeW2u+vqkZYwctbZzk+OrCikD4UjEFEaH3uMMS1axdBcLfQtFdcbEsW1m\n\twFN3uBHpZJ9N2qLHQmMilc3N9zPIc3udw3waUm8xA99tkj7aGmK6QlkACHhFA15J8p\n\toVybPJYhFNuJrAONSRMPNiHDCIIHQmc3T+w1UK2E=","Date":"Wed, 17 Nov 2021 12:26:26 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211116162615.27777-1-laurent.pinchart@ideasonboard.com>\n\t<20211116162615.27777-3-laurent.pinchart@ideasonboard.com>\n\t<86e5a845-da91-dd3d-ad55-d9cccd49aa6e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<86e5a845-da91-dd3d-ad55-d9cccd49aa6e@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: ipu3: agc: Standardize\n\tvocabulary on \"relative luminance\"","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":20982,"web_url":"https://patchwork.libcamera.org/comment/20982/","msgid":"","date":"2021-11-17T10:30:14","subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: ipu3: agc: Standardize\n\tvocabulary on \"relative luminance\"","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 17/11/2021 11:26, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> On Wed, Nov 17, 2021 at 11:15:23AM +0100, Jean-Michel Hautbois wrote:\n>> On 16/11/2021 17:26, Laurent Pinchart wrote:\n>>> The AGC computes the average relative luminance of the frame and calls\n>>> the value \"normalized luma\", \"brightness\" or \"initialY\". The latter is\n>>> the most accurate term, as the relative luminance is abbreviated Y, but\n>>> the \"initial\" prefix isn't accurate.\n>>>\n>>> Standardize the vocabulary on \"relative luminance\" in code and comments,\n>>> abbreviating it to Y when needed.\n>>>\n>>> While at it, rename variables to match the libcamera coding style.\n>>>\n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> src/ipa/ipu3/algorithms/agc.cpp | 44 ++++++++++++++++-----------------\n>>> src/ipa/ipu3/algorithms/agc.h | 8 +++---\n>>> 2 files changed, 26 insertions(+), 26 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>>> index 43a39ffd57d6..d5840fb0aa97 100644\n>>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>>> @@ -62,12 +62,12 @@ static constexpr double kEvGainTarget = 0.5;\n>>> static constexpr uint32_t kNumStartupFrames = 10;\n>>> \n>>> /*\n>>> - * Normalized luma value target.\n>>> + * Relative luminance target.\n>>> *\n>>> * It's a number that's chosen so that, when the camera points at a grey\n>>> * target, the resulting image brightness is considered right.\n>>> */\n>>> -static constexpr double kNormalizedLumaTarget = 0.16;\n>>> +static constexpr double kRelativeLuminanceTarget = 0.16;\n>>> \n>>> Agc::Agc()\n>>> \t: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minShutterSpeed_(0s),\n>>> @@ -250,12 +250,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n>>> }\n>>> \n>>> /**\n>>> - * \\brief Estimate the average brightness of the frame\n>>> + * \\brief Estimate the relative luminance of the frame with a given gain\n>>> * \\param[in] frameContext The shared IPA frame context\n>>> * \\param[in] grid The grid used to store the statistics in the IPU3\n>>> * \\param[in] stats The IPU3 statistics and ISP results\n>>> * \\param[in] currentYGain The gain calculated on the current brightness level\n>>> - * \\return The normalized luma\n>>> + * \\return The relative luminance\n>>> *\n>>> * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color\n>>> * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a\n>>> @@ -263,12 +263,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n>>> * luma here.\n>>> *\n>>> * More detailed information can be found in:\n>>> - * https://en.wikipedia.org/wiki/Luma_(video)\n>>> + * https://en.wikipedia.org/wiki/Relative_luminance\n>>> */\n>>> -double Agc::computeInitialY(IPAFrameContext &frameContext,\n>>> -\t\t\t const ipu3_uapi_grid_config &grid,\n>>> -\t\t\t const ipu3_uapi_stats_3a *stats,\n>>> -\t\t\t double currentYGain)\n>>> +double Agc::estimateLuminance(IPAFrameContext &frameContext,\n>>> +\t\t\t const ipu3_uapi_grid_config &grid,\n>>> +\t\t\t const ipu3_uapi_stats_3a *stats,\n>>> +\t\t\t double currentYGain)\n>>\n>> Shouldn't it be named estimateRelativeLuminance then ? Or is it too long\n>> maybe...\n> \n> I found it a bit long. If you think it's better, I'm ok with it.\n> \n>>> {\n>>> \tdouble redSum = 0, greenSum = 0, blueSum = 0;\n>>> \n>>> @@ -288,14 +288,14 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n>>> \t}\n>>> \n>>> \t/*\n>>> -\t * Estimate the sum of the brightness values, weighted with the gains\n>>> -\t * applied on the channels in AWB as the Rec. 601 luma.\n>>> +\t * Compute the relative luminance using the Rec. 601 formula, and\n>>> +\t * normalize it.\n>>> \t */\n>>\n>> Relative luminance is normalized, as it is between 0.0 and 1.0 :-).\n>> Why did you remove the gain weights mention though ?\n> \n> Those are not weights but gains. It should be mentioned though, I'll\n> update it.\n> \n> Why do we multiply by the AWB gains here ?\n\nBecause we want to estimate the brightness based on what the scene \nreally is (or as much as possible) so we need corrected channels for \nthat to be accurate.\n\n> \n>>> -\tdouble Y_sum = redSum * frameContext.awb.gains.red * .299 +\n>>> -\t\t greenSum * frameContext.awb.gains.green * .587 +\n>>> -\t\t blueSum * frameContext.awb.gains.blue * .114;\n>>> +\tdouble ySum = redSum * frameContext.awb.gains.red * .299 +\n>>> +\t\t greenSum * frameContext.awb.gains.green * .587 +\n>>> +\t\t blueSum * frameContext.awb.gains.blue * .114;\n>>> \n>>> -\treturn Y_sum / (grid.height * grid.width) / 255;\n>>> +\treturn ySum / (grid.height * grid.width) / 255;\n>>\n>> How could we let that one... :-/\n>>\n>>> }\n>>> \n>>> /**\n>>> @@ -311,22 +311,22 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>>> \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n>>> \n>>> \tdouble currentYGain = 1.0;\n>>> -\tdouble targetY = kNormalizedLumaTarget;\n>>> +\tdouble yTarget = kRelativeLuminanceTarget;\n>>> \n>>> \t/*\n>>> \t * Do this calculation a few times as brightness increase can be\n>>> \t * non-linear when there are saturated regions.\n>>> \t */\n>>> -\tfor (int i = 0; i < 8; i++) {\n>>> -\t\tdouble initialY = computeInitialY(context.frameContext,\n>>> +\tfor (unsigned int i = 0; i < 8; i++) {\n>>> +\t\tdouble yValue = estimateLuminance(context.frameContext,\n>>> \t\t\t\t\t\t context.configuration.grid.bdsGrid,\n>>> \t\t\t\t\t\t stats, currentYGain);\n>>> -\t\tdouble extra_gain = std::min(10.0, targetY / (initialY + .001));\n>>> +\t\tdouble extra_gain = std::min(10.0, yTarget / (yValue + .001));\n>>> \n>>> \t\tcurrentYGain *= extra_gain;\n>>> -\t\tLOG(IPU3Agc, Debug) << \"Initial Y \" << initialY\n>>> -\t\t\t\t << \" target \" << targetY\n>>> -\t\t\t\t << \" gives gain \" << currentYGain;\n>>> +\t\tLOG(IPU3Agc, Debug) << \"Y value: \" << yValue\n>>> +\t\t\t\t << \", Y target: \" << yTarget\n>>> +\t\t\t\t << \", gives gain \" << currentYGain;\n>>> \t\tif (extra_gain < 1.01)\n>>> \t\t\tbreak;\n>>> \t}\n>>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>>> index 31c5a6e519d4..943c354a820e 100644\n>>> --- a/src/ipa/ipu3/algorithms/agc.h\n>>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>>> @@ -35,10 +35,10 @@ private:\n>>> \t\t\t const ipu3_uapi_grid_config &grid);\n>>> \tvoid filterExposure();\n>>> \tvoid computeExposure(IPAFrameContext &frameContext, double currentYGain);\n>>> -\tdouble computeInitialY(IPAFrameContext &frameContext,\n>>> -\t\t\t const ipu3_uapi_grid_config &grid,\n>>> -\t\t\t const ipu3_uapi_stats_3a *stats,\n>>> -\t\t\t double currentYGain);\n>>> +\tdouble estimateLuminance(IPAFrameContext &frameContext,\n>>> +\t\t\t\t const ipu3_uapi_grid_config &grid,\n>>> +\t\t\t\t const ipu3_uapi_stats_3a *stats,\n>>> +\t\t\t\t double currentYGain);\n>>> \n>>> \tuint64_t frameCount_;\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 C605BBF415\n\tfor ;\n\tWed, 17 Nov 2021 10:30:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18D4960376;\n\tWed, 17 Nov 2021 11:30:19 +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 DCA4D600B5\n\tfor ;\n\tWed, 17 Nov 2021 11:30:17 +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 84527E7;\n\tWed, 17 Nov 2021 11:30:17 +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=\"PYBts7xN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637145017;\n\tbh=iY2MGpPd8nDPEAW83XARfUZ1IDf+9zRTP7AT9eCF0QA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=PYBts7xNJhfpMi++UhvpvKk80DBNTojvBkuwI/63tFDjEK2HVPP0rvQeOldrwWkdr\n\t96D7aMryF3ULl7bMu1N4/v4rJr/TRkwFFmxOFPG0c7zrkH+XmCygSRziVu4xbpfXr/\n\ti2HDLoMBbk/TyxQRRgo1NRmna21IBA9CmXD67bek=","Message-ID":"","Date":"Wed, 17 Nov 2021 11:30:14 +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 ","References":"<20211116162615.27777-1-laurent.pinchart@ideasonboard.com>\n\t<20211116162615.27777-3-laurent.pinchart@ideasonboard.com>\n\t<86e5a845-da91-dd3d-ad55-d9cccd49aa6e@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/5] ipa: ipu3: agc: Standardize\n\tvocabulary on \"relative luminance\"","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\" "}}]