[{"id":21093,"web_url":"https://patchwork.libcamera.org/comment/21093/","msgid":"<11c6162b-29e1-9c91-43be-520ac6bf3448@ideasonboard.com>","date":"2021-11-22T06:42:04","subject":"Re: [libcamera-devel] [PATCH v2 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\nOn 19/11/2021 22:02, 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> - 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 \nReviewed-by: Jean-Michel Hautbois \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 9cd2ded501ed..2d196fd63c7e 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 gain to apply to the frame\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 BE52CBF415\n\tfor ;\n\tMon, 22 Nov 2021 06:42:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 78CA66036F;\n\tMon, 22 Nov 2021 07:42:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D84D60228\n\tfor ;\n\tMon, 22 Nov 2021 07:42:07 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:933b:1c23:4e2b:3c0f] (unknown\n\t[IPv6:2a01:e0a:169:7140:933b:1c23:4e2b:3c0f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D19A592A;\n\tMon, 22 Nov 2021 07:42:06 +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=\"uMJL7CQH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637563326;\n\tbh=N/kV4XcQOfi/MV7ChTL3oxLKgRshD1bTDHyFtpPeIgY=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=uMJL7CQH6MSPWDYs8zEzmulFJqOeuB15SWsg9fDow3WpA+oam+KNb8rP12pOAXP5a\n\th5a1Qf9QdzbDsawmG+GA1+qDuxpbxAwx9xgMMCFU22FVXSupwE4t8CVMDxtKs/fh1I\n\tCxLmjWf6JJgZNZlcH05GoOynK78u095+wt9vzLho=","Message-ID":"<11c6162b-29e1-9c91-43be-520ac6bf3448@ideasonboard.com>","Date":"Mon, 22 Nov 2021 07:42:04 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211119210239.18540-1-laurent.pinchart@ideasonboard.com>\n\t<20211119210239.18540-4-laurent.pinchart@ideasonboard.com>","From":"Jean-Michel Hautbois ","In-Reply-To":"<20211119210239.18540-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 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":21104,"web_url":"https://patchwork.libcamera.org/comment/21104/","msgid":"<163759252072.1089182.12673447162597079985@Monstersaurus>","date":"2021-11-22T14:48:40","subject":"Re: [libcamera-devel] [PATCH v2 3/5] ipa: ipu3: agc: Rename\n\tcurrentYGain","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-19 21:02:37)\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> - 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\nclang-tidy seems to have extra methods to be able to validate variable\nname 'style's. Including the ability to ensure things like a postfix on\nprivate member variables.\n\nclang-tidy requires going through the compiler though, so needs a build\n- and might not be so easy to integrate to checkstyle. (It's also a lot\nslower, so more appropriate for some integration time, or server side\nchecks).\n\nanyway,\n\nReviewed-by: Kieran Bingham \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 9cd2ded501ed..2d196fd63c7e 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> /* Get the effective exposure and gain applied on the sensor. */\n> uint32_t exposure = frameContext.sensor.exposure;\n> @@ -189,8 +189,8 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)\n> */\n> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> - if (evGain < currentYGain)\n> - evGain = currentYGain;\n> + if (evGain < yGain)\n> + evGain = yGain;\n> \n> /* Consider within 1% of the target as correctly exposed */\n> if (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 gain to apply to the frame\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> const ipu3_uapi_grid_config &grid,\n> const ipu3_uapi_stats_3a *stats,\n> - double currentYGain)\n> + double gain)\n> {\n> double redSum = 0, greenSum = 0, blueSum = 0;\n> \n> @@ -281,9 +281,9 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext,\n> &stats->awb_raw_buffer.meta_data[cellPosition]\n> );\n> \n> - redSum += cell->R_avg * currentYGain;\n> - greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * currentYGain;\n> - blueSum += cell->B_avg * currentYGain;\n> + redSum += cell->R_avg * gain;\n> + greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain;\n> + blueSum += cell->B_avg * gain;\n> }\n> }\n> \n> @@ -310,7 +310,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> measureBrightness(stats, context.configuration.grid.bdsGrid);\n> \n> - double currentYGain = 1.0;\n> + double yGain = 1.0;\n> double yTarget = kRelativeLuminanceTarget;\n> \n> /*\n> @@ -320,18 +320,18 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> for (unsigned int i = 0; i < 8; i++) {\n> double yValue = estimateLuminance(context.frameContext,\n> context.configuration.grid.bdsGrid,\n> - stats, currentYGain);\n> - double extra_gain = std::min(10.0, yTarget / (yValue + .001));\n> + stats, yGain);\n> + double extraGain = std::min(10.0, yTarget / (yValue + .001));\n> \n> - currentYGain *= extra_gain;\n> + yGain *= extraGain;\n> LOG(IPU3Agc, Debug) << \"Y value: \" << yValue\n> << \", Y target: \" << yTarget\n> - << \", gives gain \" << currentYGain;\n> - if (extra_gain < 1.01)\n> + << \", gives gain \" << yGain;\n> + if (extraGain < 1.01)\n> break;\n> }\n> \n> - computeExposure(context.frameContext, currentYGain);\n> + computeExposure(context.frameContext, yGain);\n> frameCount_++;\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> void measureBrightness(const ipu3_uapi_stats_3a *stats,\n> const ipu3_uapi_grid_config &grid);\n> void filterExposure();\n> - void computeExposure(IPAFrameContext &frameContext, double currentYGain);\n> + void computeExposure(IPAFrameContext &frameContext, double yGain);\n> double estimateLuminance(IPAFrameContext &frameContext,\n> const ipu3_uapi_grid_config &grid,\n> const ipu3_uapi_stats_3a *stats,\n> - double currentYGain);\n> + double gain);\n> \n> uint64_t frameCount_;\n> \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 E4264BDB13\n\tfor ;\n\tMon, 22 Nov 2021 14:48:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27D1B6038A;\n\tMon, 22 Nov 2021 15:48:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9493C6022D\n\tfor ;\n\tMon, 22 Nov 2021 15:48:43 +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 2D36A14C3;\n\tMon, 22 Nov 2021 15:48: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=\"dbvviixj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637592523;\n\tbh=r7TnO7EKqTILCUsbvUNbIRZcp9KCR2un7ksP+Cs2AXA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=dbvviixjyBDleazvHBSPnMLEEBZ6CmKoO6DEE0wxk/VsG3Rb7ZHswOmo9hDQfQpVH\n\t8xxjxbsrbe8v/zzCavIgMO/RG01bndc+7Mok2uC2TyOTJF1sFR5QweADwOUWZ7TNNE\n\tpvmwhuob0m0UqTWvddYARyKXhifiszUkH2p5UpnA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211119210239.18540-4-laurent.pinchart@ideasonboard.com>","References":"<20211119210239.18540-1-laurent.pinchart@ideasonboard.com>\n\t<20211119210239.18540-4-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 22 Nov 2021 14:48:40 +0000","Message-ID":"<163759252072.1089182.12673447162597079985@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}}]