[{"id":20978,"web_url":"https://patchwork.libcamera.org/comment/20978/","msgid":"<1a9bc3b0-8b05-3bca-d4d1-606bb270b7a2@ideasonboard.com>","date":"2021-11-17T10:21:10","subject":"Re: [libcamera-devel] [PATCH 3/5] ipa: ipu3: agc: Rename\n\tcurrentYGain","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 \"current\" prefix in the currentYGain variable name is confusing:\n> \n> - In Agc::estimateLuminance(), the variable contains the gain to be\n> applied to the image, which is neither a \"current\" gain nor a \"Y\"\n> gain. Rename it to \"gain\".\n\nIt is the gain to apply for the image to be at the desired relative \nluminance level.\n\n> \n> - In Agc::computeExposure(), the variable contains the gain computed by\n> the relative luminance method, so rename it to \"yGain\".\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 | 32 ++++++++++++++++----------------\n> src/ipa/ipu3/algorithms/agc.h | 4 ++--\n> 2 files changed, 18 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index d5840fb0aa97..6aab9fd5ebb5 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -173,9 +173,9 @@ void Agc::filterExposure()\n> /**\n> * \\brief Estimate the new exposure and gain values\n> * \\param[inout] frameContext The shared IPA frame Context\n> - * \\param[in] currentYGain The gain calculated on the current brightness level\n> + * \\param[in] yGain The gain calculated based on the relative luminance target\n> */\n> -void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> +void Agc::computeExposure(IPAFrameContext &frameContext, double yGain)\n> {\n> \t/* Get the effective exposure and gain applied on the sensor. */\n> \tuint32_t exposure = frameContext.sensor.exposure;\n> @@ -189,8 +189,8 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> \t */\n> \tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> -\tif (evGain < currentYGain)\n> -\t\tevGain = currentYGain;\n> +\tif (evGain < yGain)\n> +\t\tevGain = yGain;\n> \n> \t/* Consider within 1% of the target as correctly exposed */\n> \tif (std::abs(evGain - 1.0) < 0.01)\n> @@ -254,7 +254,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\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> + * \\param[in] gain The analogue gain to apply to the frame\n\nI don't think so, it is a global gain to apply, not only the analogue \ngain. The goal is to get as closed as possible to a target. This gain \nwill then be splitted into shutter speed and analogue gain.\n\n> * \\return The relative luminance\n> *\n> * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color\n> @@ -268,7 +268,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, 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> +\t\t\t double gain)\n> {\n> \tdouble redSum = 0, greenSum = 0, blueSum = 0;\n> \n> @@ -281,9 +281,9 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext,\n> \t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n> \t\t\t\t);\n> \n> -\t\t\tredSum += cell->R_avg * currentYGain;\n> -\t\t\tgreenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * currentYGain;\n> -\t\t\tblueSum += cell->B_avg * currentYGain;\n> +\t\t\tredSum += cell->R_avg * gain;\n> +\t\t\tgreenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain;\n> +\t\t\tblueSum += cell->B_avg * gain;\n> \t\t}\n> \t}\n> \n> @@ -310,7 +310,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n> \n> -\tdouble currentYGain = 1.0;\n> +\tdouble yGain = 1.0;\n> \tdouble yTarget = kRelativeLuminanceTarget;\n> \n> \t/*\n> @@ -320,18 +320,18 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\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, yTarget / (yValue + .001));\n> +\t\t\t\t\t\t stats, yGain);\n> +\t\tdouble extraGain = std::min(10.0, yTarget / (yValue + .001));\n> \n> -\t\tcurrentYGain *= extra_gain;\n> +\t\tyGain *= extraGain;\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\t\t << \", gives gain \" << yGain;\n> +\t\tif (extraGain < 1.01)\n> \t\t\tbreak;\n> \t}\n> \n> -\tcomputeExposure(context.frameContext, currentYGain);\n> +\tcomputeExposure(context.frameContext, yGain);\n> \tframeCount_++;\n> }\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 943c354a820e..0c868d6737f1 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -34,11 +34,11 @@ private:\n> \tvoid measureBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> \tvoid filterExposure();\n> -\tvoid computeExposure(IPAFrameContext &frameContext, double currentYGain);\n> +\tvoid computeExposure(IPAFrameContext &frameContext, double yGain);\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> +\t\t\t\t double gain);\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 E96CBBF415\n\tfor ;\n\tWed, 17 Nov 2021 10:21:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3051660376;\n\tWed, 17 Nov 2021 11:21:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4041600B5\n\tfor ;\n\tWed, 17 Nov 2021 11:21:13 +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 96666E7;\n\tWed, 17 Nov 2021 11:21:13 +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=\"PdmkbZss\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637144473;\n\tbh=IeVEFLCOS1V7ORSq3M9v1r749onckZFb5Ry92oiWsOc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=PdmkbZss94TvuO1PPfLrrFCj662efUVho3YtIyKZvwTKUrcSUVEi69YrhWFeZ8b0u\n\ttKJyCkDDIQVnwiPjnqM29jf5FlukRkPvxgUdPdaG9N9ecyUG7DoFukEx2xsiCgRAxC\n\tDIlqd6Jk8l5LrIgar+BFN3t+0h0ho34RZHoM5++c=","Message-ID":"<1a9bc3b0-8b05-3bca-d4d1-606bb270b7a2@ideasonboard.com>","Date":"Wed, 17 Nov 2021 11:21:10 +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-4-laurent.pinchart@ideasonboard.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<20211116162615.27777-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/5] ipa: ipu3: agc: Rename\n\tcurrentYGain","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":20983,"web_url":"https://patchwork.libcamera.org/comment/20983/","msgid":"","date":"2021-11-17T10:34:33","subject":"Re: [libcamera-devel] [PATCH 3/5] ipa: ipu3: agc: Rename\n\tcurrentYGain","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:21:10AM +0100, Jean-Michel Hautbois wrote:\n> On 16/11/2021 17:26, Laurent Pinchart wrote:\n> > The \"current\" prefix in the currentYGain variable name is confusing:\n> > \n> > - In Agc::estimateLuminance(), the variable contains the gain to be\n> > applied to the image, which is neither a \"current\" gain nor a \"Y\"\n> > gain. Rename it to \"gain\".\n> \n> It is the gain to apply for the image to be at the desired relative \n> luminance level.\n\nNot exactly. The estimateLuminance() function estimates the luminance of\nthe image that would have been produced if that extra gain had been\napplied. The caller uses an iterative approach to try and find the gain\nthat will result in a given average relative luminance, but from the\npoint of view of the estimateLuminance() function, that's not relevant.\n\n> > - In Agc::computeExposure(), the variable contains the gain computed by\n> > the relative luminance method, so rename it to \"yGain\".\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 | 32 ++++++++++++++++----------------\n> > src/ipa/ipu3/algorithms/agc.h | 4 ++--\n> > 2 files changed, 18 insertions(+), 18 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index d5840fb0aa97..6aab9fd5ebb5 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -173,9 +173,9 @@ void Agc::filterExposure()\n> > /**\n> > * \\brief Estimate the new exposure and gain values\n> > * \\param[inout] frameContext The shared IPA frame Context\n> > - * \\param[in] currentYGain The gain calculated on the current brightness level\n> > + * \\param[in] yGain The gain calculated based on the relative luminance target\n> > */\n> > -void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> > +void Agc::computeExposure(IPAFrameContext &frameContext, double yGain)\n> > {\n> > \t/* Get the effective exposure and gain applied on the sensor. */\n> > \tuint32_t exposure = frameContext.sensor.exposure;\n> > @@ -189,8 +189,8 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> > \t */\n> > \tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> > \n> > -\tif (evGain < currentYGain)\n> > -\t\tevGain = currentYGain;\n> > +\tif (evGain < yGain)\n> > +\t\tevGain = yGain;\n> > \n> > \t/* Consider within 1% of the target as correctly exposed */\n> > \tif (std::abs(evGain - 1.0) < 0.01)\n> > @@ -254,7 +254,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\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> > + * \\param[in] gain The analogue gain to apply to the frame\n> \n> I don't think so, it is a global gain to apply, not only the analogue \n> gain. The goal is to get as closed as possible to a target. This gain \n> will then be splitted into shutter speed and analogue gain.\n\nIndeed. I'll replace that to \"The gain to apply to the frame\".\n\nWe probably need a patch on top to use different names for \"global\ngains\" (before they're split in analogue gain and integration time, and\ndigital gain in the future), and analogue/digital gains.\n\n> > * \\return The relative luminance\n> > *\n> > * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color\n> > @@ -268,7 +268,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, 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> > +\t\t\t double gain)\n> > {\n> > \tdouble redSum = 0, greenSum = 0, blueSum = 0;\n> > \n> > @@ -281,9 +281,9 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext,\n> > \t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n> > \t\t\t\t);\n> > \n> > -\t\t\tredSum += cell->R_avg * currentYGain;\n> > -\t\t\tgreenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * currentYGain;\n> > -\t\t\tblueSum += cell->B_avg * currentYGain;\n> > +\t\t\tredSum += cell->R_avg * gain;\n> > +\t\t\tgreenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain;\n> > +\t\t\tblueSum += cell->B_avg * gain;\n> > \t\t}\n> > \t}\n> > \n> > @@ -310,7 +310,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> > {\n> > \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n> > \n> > -\tdouble currentYGain = 1.0;\n> > +\tdouble yGain = 1.0;\n> > \tdouble yTarget = kRelativeLuminanceTarget;\n> > \n> > \t/*\n> > @@ -320,18 +320,18 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\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, yTarget / (yValue + .001));\n> > +\t\t\t\t\t\t stats, yGain);\n> > +\t\tdouble extraGain = std::min(10.0, yTarget / (yValue + .001));\n> > \n> > -\t\tcurrentYGain *= extra_gain;\n> > +\t\tyGain *= extraGain;\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\t\t << \", gives gain \" << yGain;\n> > +\t\tif (extraGain < 1.01)\n> > \t\t\tbreak;\n> > \t}\n> > \n> > -\tcomputeExposure(context.frameContext, currentYGain);\n> > +\tcomputeExposure(context.frameContext, yGain);\n> > \tframeCount_++;\n> > }\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> > index 943c354a820e..0c868d6737f1 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.h\n> > +++ b/src/ipa/ipu3/algorithms/agc.h\n> > @@ -34,11 +34,11 @@ private:\n> > \tvoid measureBrightness(const ipu3_uapi_stats_3a *stats,\n> > \t\t\t const ipu3_uapi_grid_config &grid);\n> > \tvoid filterExposure();\n> > -\tvoid computeExposure(IPAFrameContext &frameContext, double currentYGain);\n> > +\tvoid computeExposure(IPAFrameContext &frameContext, double yGain);\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> > +\t\t\t\t double gain);\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 95A35BDB1C\n\tfor ;\n\tWed, 17 Nov 2021 10:34:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E49D96032C;\n\tWed, 17 Nov 2021 11:34:57 +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 63A0C600B5\n\tfor ;\n\tWed, 17 Nov 2021 11:34:56 +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 E9C37E7;\n\tWed, 17 Nov 2021 11:34:55 +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=\"FzvsP2LE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637145296;\n\tbh=WNFt5w4LM2hvgKx8Q8OPv7dh3ZoyeCsyhj5CfoAjbt4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FzvsP2LE3rQAVKdps1kpAuVAw1lhH+LV3uM6Nxy6I5O85UdI+tcJXV3PizTrQ/kcL\n\taVzYE+Lti0dIR+65Ag33fFOGISYvBobB09suKrm9z4RbpXCEpDOuWju2HmBNiXA92a\n\tRQgDeFtztaZIZE8Lr2fejYclzpOD5+tB2UN+H2l4=","Date":"Wed, 17 Nov 2021 12:34:33 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211116162615.27777-1-laurent.pinchart@ideasonboard.com>\n\t<20211116162615.27777-4-laurent.pinchart@ideasonboard.com>\n\t<1a9bc3b0-8b05-3bca-d4d1-606bb270b7a2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1a9bc3b0-8b05-3bca-d4d1-606bb270b7a2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/5] ipa: ipu3: agc: Rename\n\tcurrentYGain","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\" "}}]