[{"id":20897,"web_url":"https://patchwork.libcamera.org/comment/20897/","msgid":"<37c9aa62-b910-1fa7-3d2b-57f2e72055e2@ideasonboard.com>","date":"2021-11-12T08:37:05","subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM,\n\nThank you for the patch.\n\nOn 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:\n> Now that we have the real exposure applied at each frame, remove the\n> early return based on a frame counter and compute the gain for each\n> frame.\n>\n> Instead of that, introduce a number of startup frames during which the\n> filter speed is 1.0, meaning we apply instantly the exposure value\n> calculated and not a slower filtered one.\n\n\nTo me, it feels a 'because...' is coming at the end of the commit \nmessage. Perhaps, I don't have enough context, but I assume we want to \nconverge fast while startup hence we use speed == 1? If you could add \nthe explaination a bit, it would make more complete commit message for \nnaive reviewers like me ;-)\n\n>\n> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\nReviewed-by: Umang Jain <umang.jain@ideasonboard.com>\n\n> ---\n>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------\n>   1 file changed, 11 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 83aa3676..74bce7bb 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {\n>   \n>   LOG_DEFINE_CATEGORY(IPU3Agc)\n>   \n> -/* Number of frames to wait before calculating stats on minimum exposure */\n> -static constexpr uint32_t kInitialFrameMinAECount = 4;\n> -/* Number of frames to wait between new gain/shutter time estimations */\n> -static constexpr uint32_t kFrameSkipCount = 6;\n> -\n>   /* Limits for analogue gain values */\n>   static constexpr double kMinAnalogueGain = 1.0;\n>   static constexpr double kMaxAnalogueGain = 8.0;\n> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;\n>    */\n>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;\n>   \n> +/* Number of frames to wait before calculating stats on minimum exposure */\n> +static constexpr uint32_t kNumStartupFrames = 10;\n> +\n>   Agc::Agc()\n> -\t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> -\t  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n> -\t  currentExposure_(0s), prevExposureValue_(0s)\n> +\t: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),\n> +\t  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n> +\t  prevExposureValue_(0s)\n>   {\n>   }\n>   \n> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n>   void Agc::filterExposure()\n>   {\n>   \tdouble speed = 0.2;\n> +\n> +\t/* Adapt instantly if we are in startup phase */\n> +\tif (frameCount_ < kNumStartupFrames)\n> +\t\tspeed = 1.0;\n> +\n>   \tif (filteredExposure_ == 0s) {\n>   \t\t/* DG stands for digital gain.*/\n>   \t\tfilteredExposure_ = currentExposure_;\n> @@ -185,13 +188,6 @@ void Agc::filterExposure()\n>    */\n>   void Agc::computeExposure(IPAFrameContext &frameContext)\n>   {\n> -\t/* Algorithm initialization should wait for first valid frames */\n> -\t/* \\todo - have a number of frames given by DelayedControls ?\n> -\t * - implement a function for IIR */\n> -\tif ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))\n> -\t\treturn;\n> -\n> -\tlastFrame_ = frameCount_;\n>   \n>   \t/* Are we correctly exposed ? */\n>   \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 AB5CDBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Nov 2021 08:37:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F28B16035D;\n\tFri, 12 Nov 2021 09:37:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86BB560234\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Nov 2021 09:37:10 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.254])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AB70E2F3;\n\tFri, 12 Nov 2021 09:37:09 +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=\"s6VCX7dU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636706230;\n\tbh=SjxZcG8A0c9uwcKrUYm5wa/BPtuCvCaOQwK/FpyVTZg=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=s6VCX7dUglyXb1JFLNU7WmozVV87NABDBQ+fGjaYkZw26HBkbe0arhYL34YshMKVc\n\tJ8QkG27pWwENGZ5V0VqsZykOOPQnGa8qq79607YIpJ6OOgK6aLjVbhHROXNCDr7INU\n\tVby+gY5OWWB5ddrbl1hBKikrW+QIWmj1KRdVL4YI=","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211111140928.136111-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<37c9aa62-b910-1fa7-3d2b-57f2e72055e2@ideasonboard.com>","Date":"Fri, 12 Nov 2021 14:07:05 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20898,"web_url":"https://patchwork.libcamera.org/comment/20898/","msgid":"<73c537be-a582-9f34-2de8-921fbf32fc9e@ideasonboard.com>","date":"2021-11-12T08:47:39","subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang\n\nOn 12/11/2021 09:37, Umang Jain wrote:\n> Hi JM,\n> \n> Thank you for the patch.\n> \n> On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:\n>> Now that we have the real exposure applied at each frame, remove the\n>> early return based on a frame counter and compute the gain for each\n>> frame.\n>>\n>> Instead of that, introduce a number of startup frames during which the\n>> filter speed is 1.0, meaning we apply instantly the exposure value\n>> calculated and not a slower filtered one.\n> \n> \n> To me, it feels a 'because...' is coming at the end of the commit \n> message. Perhaps, I don't have enough context, but I assume we want to \n> converge fast while startup hence we use speed == 1? If you could add \n> the explaination a bit, it would make more complete commit message for \n> naive reviewers like me ;-)\n\nWould it help ?\n'''\nInstead of that, introduce a number of startup frames during which the\nfilter speed is 1.0, meaning we apply instantly the exposure value\ncalculated and not a slower filtered one. This is used to have a faster\nconvergence, and those frames may ultimately be dropped.\n'''\n\n> \n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> <jeanmichel.hautbois@ideasonboard.com>\n>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> \n> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>\n> \n>> ---\n>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------\n>>   1 file changed, 11 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp \n>> b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 83aa3676..74bce7bb 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {\n>>   LOG_DEFINE_CATEGORY(IPU3Agc)\n>> -/* Number of frames to wait before calculating stats on minimum \n>> exposure */\n>> -static constexpr uint32_t kInitialFrameMinAECount = 4;\n>> -/* Number of frames to wait between new gain/shutter time estimations */\n>> -static constexpr uint32_t kFrameSkipCount = 6;\n>> -\n>>   /* Limits for analogue gain values */\n>>   static constexpr double kMinAnalogueGain = 1.0;\n>>   static constexpr double kMaxAnalogueGain = 8.0;\n>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;\n>>    */\n>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;\n>> +/* Number of frames to wait before calculating stats on minimum \n>> exposure */\n>> +static constexpr uint32_t kNumStartupFrames = 10;\n>> +\n>>   Agc::Agc()\n>> -    : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>> -      minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n>> -      currentExposure_(0s), prevExposureValue_(0s)\n>> +    : frameCount_(0), iqMean_(0.0), lineDuration_(0s), \n>> minExposureLines_(0),\n>> +      maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n>> +      prevExposureValue_(0s)\n>>   {\n>>   }\n>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const \n>> ipu3_uapi_stats_3a *stats,\n>>   void Agc::filterExposure()\n>>   {\n>>       double speed = 0.2;\n>> +\n>> +    /* Adapt instantly if we are in startup phase */\n>> +    if (frameCount_ < kNumStartupFrames)\n>> +        speed = 1.0;\n>> +\n>>       if (filteredExposure_ == 0s) {\n>>           /* DG stands for digital gain.*/\n>>           filteredExposure_ = currentExposure_;\n>> @@ -185,13 +188,6 @@ void Agc::filterExposure()\n>>    */\n>>   void Agc::computeExposure(IPAFrameContext &frameContext)\n>>   {\n>> -    /* Algorithm initialization should wait for first valid frames */\n>> -    /* \\todo - have a number of frames given by DelayedControls ?\n>> -     * - implement a function for IIR */\n>> -    if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - \n>> lastFrame_ < kFrameSkipCount))\n>> -        return;\n>> -\n>> -    lastFrame_ = frameCount_;\n>>       /* Are we correctly exposed ? */\n>>       if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3DAC2BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Nov 2021 08:47:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFFC16033C;\n\tFri, 12 Nov 2021 09:47:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63C5460234\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Nov 2021 09:47:42 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:61e2:b805:836d:e891] (unknown\n\t[IPv6:2a01:e0a:169:7140:61e2:b805:836d:e891])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F333C4A6;\n\tFri, 12 Nov 2021 09:47:41 +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=\"jxKyvCf5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636706862;\n\tbh=AIqsr2I5EF9jmJ4tstm/B7Rdbme5rZxSvs/c6KPYFEs=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=jxKyvCf5FB0kD+ryFoQ2iOt9BhSMlB0sixUJoszxcktOD6ZDlHANcKKWyTE/fAImh\n\tBbIlY85Xm9ib07d2YGxaINc90YjRZO1kTVg5bUB1Cxdyzb3boRUMdF0kP1XkeAF8zk\n\t69hbbVPnjBiZG3x1Qn+vDNA663+9tosixnmIO12s=","Message-ID":"<73c537be-a582-9f34-2de8-921fbf32fc9e@ideasonboard.com>","Date":"Fri, 12 Nov 2021 09:47:39 +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":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211111140928.136111-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>\n\t<37c9aa62-b910-1fa7-3d2b-57f2e72055e2@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","In-Reply-To":"<37c9aa62-b910-1fa7-3d2b-57f2e72055e2@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20899,"web_url":"https://patchwork.libcamera.org/comment/20899/","msgid":"<00bd7c27-efd0-448d-5336-eaa30913de5b@ideasonboard.com>","date":"2021-11-12T08:48:44","subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM,\n\nOn 11/12/21 2:17 PM, Jean-Michel Hautbois wrote:\n> Hi Umang\n>\n> On 12/11/2021 09:37, Umang Jain wrote:\n>> Hi JM,\n>>\n>> Thank you for the patch.\n>>\n>> On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:\n>>> Now that we have the real exposure applied at each frame, remove the\n>>> early return based on a frame counter and compute the gain for each\n>>> frame.\n>>>\n>>> Instead of that, introduce a number of startup frames during which the\n>>> filter speed is 1.0, meaning we apply instantly the exposure value\n>>> calculated and not a slower filtered one.\n>>\n>>\n>> To me, it feels a 'because...' is coming at the end of the commit \n>> message. Perhaps, I don't have enough context, but I assume we want \n>> to converge fast while startup hence we use speed == 1? If you could \n>> add the explaination a bit, it would make more complete commit \n>> message for naive reviewers like me ;-)\n>\n> Would it help ?\n> '''\n> Instead of that, introduce a number of startup frames during which the\n> filter speed is 1.0, meaning we apply instantly the exposure value\n> calculated and not a slower filtered one. This is used to have a faster\n> convergence, and those frames may ultimately be dropped.\n> '''\n\n\nYes, this would help. Thank you.\n\n>\n>>\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> <jeanmichel.hautbois@ideasonboard.com>\n>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>\n>>\n>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>\n>>\n>>> ---\n>>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------\n>>>   1 file changed, 11 insertions(+), 15 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp \n>>> b/src/ipa/ipu3/algorithms/agc.cpp\n>>> index 83aa3676..74bce7bb 100644\n>>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {\n>>>   LOG_DEFINE_CATEGORY(IPU3Agc)\n>>> -/* Number of frames to wait before calculating stats on minimum \n>>> exposure */\n>>> -static constexpr uint32_t kInitialFrameMinAECount = 4;\n>>> -/* Number of frames to wait between new gain/shutter time \n>>> estimations */\n>>> -static constexpr uint32_t kFrameSkipCount = 6;\n>>> -\n>>>   /* Limits for analogue gain values */\n>>>   static constexpr double kMinAnalogueGain = 1.0;\n>>>   static constexpr double kMaxAnalogueGain = 8.0;\n>>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;\n>>>    */\n>>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;\n>>> +/* Number of frames to wait before calculating stats on minimum \n>>> exposure */\n>>> +static constexpr uint32_t kNumStartupFrames = 10;\n>>> +\n>>>   Agc::Agc()\n>>> -    : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>>> -      minExposureLines_(0), maxExposureLines_(0), \n>>> filteredExposure_(0s),\n>>> -      currentExposure_(0s), prevExposureValue_(0s)\n>>> +    : frameCount_(0), iqMean_(0.0), lineDuration_(0s), \n>>> minExposureLines_(0),\n>>> +      maxExposureLines_(0), filteredExposure_(0s), \n>>> currentExposure_(0s),\n>>> +      prevExposureValue_(0s)\n>>>   {\n>>>   }\n>>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const \n>>> ipu3_uapi_stats_3a *stats,\n>>>   void Agc::filterExposure()\n>>>   {\n>>>       double speed = 0.2;\n>>> +\n>>> +    /* Adapt instantly if we are in startup phase */\n>>> +    if (frameCount_ < kNumStartupFrames)\n>>> +        speed = 1.0;\n>>> +\n>>>       if (filteredExposure_ == 0s) {\n>>>           /* DG stands for digital gain.*/\n>>>           filteredExposure_ = currentExposure_;\n>>> @@ -185,13 +188,6 @@ void Agc::filterExposure()\n>>>    */\n>>>   void Agc::computeExposure(IPAFrameContext &frameContext)\n>>>   {\n>>> -    /* Algorithm initialization should wait for first valid frames */\n>>> -    /* \\todo - have a number of frames given by DelayedControls ?\n>>> -     * - implement a function for IIR */\n>>> -    if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - \n>>> lastFrame_ < kFrameSkipCount))\n>>> -        return;\n>>> -\n>>> -    lastFrame_ = frameCount_;\n>>>       /* Are we correctly exposed ? */\n>>>       if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7AE7FBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Nov 2021 08:48:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2EC3A6033C;\n\tFri, 12 Nov 2021 09:48:51 +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 33DD160234\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Nov 2021 09:48:49 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.254])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7492B4A6;\n\tFri, 12 Nov 2021 09:48: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=\"bqvQK6AB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636706929;\n\tbh=X9wWoW141zH1SqFI6ClWr2Tp8uigsfcrPdRJ5tcmplY=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=bqvQK6ABmfeb2zUe5N1InN2TYiePr4GPtDxkcrpVjXGfqfl5xbgjK2gRc1sUMLW50\n\t4cy1aiy9VMLHnaJsm2PnwDmet5amqM6ban3ZFlISvOAepZN455+H3QPeRRcuMUqPUZ\n\tweYOCFcspusZihBkeFZGkGdW+TlKeBF1PfnqdA1w=","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211111140928.136111-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>\n\t<37c9aa62-b910-1fa7-3d2b-57f2e72055e2@ideasonboard.com>\n\t<73c537be-a582-9f34-2de8-921fbf32fc9e@ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<00bd7c27-efd0-448d-5336-eaa30913de5b@ideasonboard.com>","Date":"Fri, 12 Nov 2021 14:18:44 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<73c537be-a582-9f34-2de8-921fbf32fc9e@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20904,"web_url":"https://patchwork.libcamera.org/comment/20904/","msgid":"<163671157485.2310770.9186913829362194992@Monstersaurus>","date":"2021-11-12T10:06:14","subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2021-11-12 08:48:44)\n> Hi JM,\n> \n> On 11/12/21 2:17 PM, Jean-Michel Hautbois wrote:\n> > Hi Umang\n> >\n> > On 12/11/2021 09:37, Umang Jain wrote:\n> >> Hi JM,\n> >>\n> >> Thank you for the patch.\n> >>\n> >> On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:\n> >>> Now that we have the real exposure applied at each frame, remove the\n> >>> early return based on a frame counter and compute the gain for each\n> >>> frame.\n> >>>\n> >>> Instead of that, introduce a number of startup frames during which the\n> >>> filter speed is 1.0, meaning we apply instantly the exposure value\n> >>> calculated and not a slower filtered one.\n> >>\n> >>\n> >> To me, it feels a 'because...' is coming at the end of the commit \n> >> message. Perhaps, I don't have enough context, but I assume we want \n> >> to converge fast while startup hence we use speed == 1? If you could \n> >> add the explaination a bit, it would make more complete commit \n> >> message for naive reviewers like me ;-)\n> >\n> > Would it help ?\n> > '''\n> > Instead of that, introduce a number of startup frames during which the\n> > filter speed is 1.0, meaning we apply instantly the exposure value\n> > calculated and not a slower filtered one. This is used to have a faster\n> > convergence, and those frames may ultimately be dropped.\n\nThat sounds like it might already exist in the code ... perhaps:\n\n\"\"\"\nand those frames may be dropped in a future development to hide the\nconvergance process from the viewer.\n\"\"\"\n\n> > '''\n> \n> \n> Yes, this would help. Thank you.\n> \n> >\n> >>\n> >>>\n> >>> Signed-off-by: Jean-Michel Hautbois \n> >>> <jeanmichel.hautbois@ideasonboard.com>\n> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>\n> >>\n> >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>\n> >>\n> >>> ---\n> >>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------\n> >>>   1 file changed, 11 insertions(+), 15 deletions(-)\n> >>>\n> >>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp \n> >>> b/src/ipa/ipu3/algorithms/agc.cpp\n> >>> index 83aa3676..74bce7bb 100644\n> >>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> >>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> >>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {\n> >>>   LOG_DEFINE_CATEGORY(IPU3Agc)\n> >>> -/* Number of frames to wait before calculating stats on minimum \n> >>> exposure */\n> >>> -static constexpr uint32_t kInitialFrameMinAECount = 4;\n> >>> -/* Number of frames to wait between new gain/shutter time \n> >>> estimations */\n> >>> -static constexpr uint32_t kFrameSkipCount = 6;\n> >>> -\n> >>>   /* Limits for analogue gain values */\n> >>>   static constexpr double kMinAnalogueGain = 1.0;\n> >>>   static constexpr double kMaxAnalogueGain = 8.0;\n> >>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;\n> >>>    */\n> >>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;\n> >>> +/* Number of frames to wait before calculating stats on minimum \n> >>> exposure */\n> >>> +static constexpr uint32_t kNumStartupFrames = 10;\n> >>> +\n> >>>   Agc::Agc()\n> >>> -    : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> >>> -      minExposureLines_(0), maxExposureLines_(0), \n> >>> filteredExposure_(0s),\n> >>> -      currentExposure_(0s), prevExposureValue_(0s)\n> >>> +    : frameCount_(0), iqMean_(0.0), lineDuration_(0s), \n> >>> minExposureLines_(0),\n> >>> +      maxExposureLines_(0), filteredExposure_(0s), \n> >>> currentExposure_(0s),\n> >>> +      prevExposureValue_(0s)\n> >>>   {\n> >>>   }\n> >>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const \n> >>> ipu3_uapi_stats_3a *stats,\n> >>>   void Agc::filterExposure()\n> >>>   {\n> >>>       double speed = 0.2;\n> >>> +\n> >>> +    /* Adapt instantly if we are in startup phase */\n> >>> +    if (frameCount_ < kNumStartupFrames)\n> >>> +        speed = 1.0;\n> >>> +\n> >>>       if (filteredExposure_ == 0s) {\n> >>>           /* DG stands for digital gain.*/\n> >>>           filteredExposure_ = currentExposure_;\n> >>> @@ -185,13 +188,6 @@ void Agc::filterExposure()\n> >>>    */\n> >>>   void Agc::computeExposure(IPAFrameContext &frameContext)\n> >>>   {\n> >>> -    /* Algorithm initialization should wait for first valid frames */\n> >>> -    /* \\todo - have a number of frames given by DelayedControls ?\n> >>> -     * - implement a function for IIR */\n> >>> -    if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - \n> >>> lastFrame_ < kFrameSkipCount))\n> >>> -        return;\n> >>> -\n> >>> -    lastFrame_ = frameCount_;\n> >>>       /* Are we correctly exposed ? */\n> >>>       if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D6D45BDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Nov 2021 10:06:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FB136035A;\n\tFri, 12 Nov 2021 11:06: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 C4FAD60234\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Nov 2021 11:06:17 +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 7047874C;\n\tFri, 12 Nov 2021 11:06: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=\"L2nr09td\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636711577;\n\tbh=X9MUWkXugGKRq1dCaaCQzljc4qwFaaeJOWZyipAkRvI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=L2nr09td6NpnLh9m6nOnZu3AcnqV9kBT/Pj02bjpNHw6x5lrPV8DFPmpmdL/HI6Uy\n\tvpcPgBDhjotJHXv+s5SRDkpwOnYozMZnDzl2Pn2Hokd00Q9DtZ7j4clF10I0pB4Iws\n\tIHAY244HvBEtWxKWoMXlAxuMi1ag8qeEa5QjNe5A=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<00bd7c27-efd0-448d-5336-eaa30913de5b@ideasonboard.com>","References":"<20211111140928.136111-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>\n\t<37c9aa62-b910-1fa7-3d2b-57f2e72055e2@ideasonboard.com>\n\t<73c537be-a582-9f34-2de8-921fbf32fc9e@ideasonboard.com>\n\t<00bd7c27-efd0-448d-5336-eaa30913de5b@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tUmang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 12 Nov 2021 10:06:14 +0000","Message-ID":"<163671157485.2310770.9186913829362194992@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20933,"web_url":"https://patchwork.libcamera.org/comment/20933/","msgid":"<20211112230509.GC8453@jade.amanokami.net>","date":"2021-11-12T23:05:09","subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Thu, Nov 11, 2021 at 03:09:19PM +0100, Jean-Michel Hautbois wrote:\n> Now that we have the real exposure applied at each frame, remove the\n> early return based on a frame counter and compute the gain for each\n> frame.\n\nThis is parsed as \"this patch removes the early return\" and \"this patch\ncomputes the gain for each frame\".\n\n> \n> Instead of that, introduce a number of startup frames during which the\n\nAnd this \"instead\" reads like it contradicts the two points above :/\n\nI think dropping \"instead of that\" or adding a bit more, like \"instead\nof the early return\" would make it easier to read.\n\n> filter speed is 1.0, meaning we apply instantly the exposure value\n> calculated and not a slower filtered one.\n> \n> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nWith those clarifications (and the elaboration that you sent to Umang),\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> ---\n>  src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------\n>  1 file changed, 11 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 83aa3676..74bce7bb 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {\n>  \n>  LOG_DEFINE_CATEGORY(IPU3Agc)\n>  \n> -/* Number of frames to wait before calculating stats on minimum exposure */\n> -static constexpr uint32_t kInitialFrameMinAECount = 4;\n> -/* Number of frames to wait between new gain/shutter time estimations */\n> -static constexpr uint32_t kFrameSkipCount = 6;\n> -\n>  /* Limits for analogue gain values */\n>  static constexpr double kMinAnalogueGain = 1.0;\n>  static constexpr double kMaxAnalogueGain = 8.0;\n> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;\n>   */\n>  static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;\n>  \n> +/* Number of frames to wait before calculating stats on minimum exposure */\n> +static constexpr uint32_t kNumStartupFrames = 10;\n> +\n>  Agc::Agc()\n> -\t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> -\t  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n> -\t  currentExposure_(0s), prevExposureValue_(0s)\n> +\t: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),\n> +\t  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n> +\t  prevExposureValue_(0s)\n>  {\n>  }\n>  \n> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n>  void Agc::filterExposure()\n>  {\n>  \tdouble speed = 0.2;\n> +\n> +\t/* Adapt instantly if we are in startup phase */\n> +\tif (frameCount_ < kNumStartupFrames)\n> +\t\tspeed = 1.0;\n> +\n>  \tif (filteredExposure_ == 0s) {\n>  \t\t/* DG stands for digital gain.*/\n>  \t\tfilteredExposure_ = currentExposure_;\n> @@ -185,13 +188,6 @@ void Agc::filterExposure()\n>   */\n>  void Agc::computeExposure(IPAFrameContext &frameContext)\n>  {\n> -\t/* Algorithm initialization should wait for first valid frames */\n> -\t/* \\todo - have a number of frames given by DelayedControls ?\n> -\t * - implement a function for IIR */\n> -\tif ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))\n> -\t\treturn;\n> -\n> -\tlastFrame_ = frameCount_;\n>  \n>  \t/* Are we correctly exposed ? */\n>  \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 BB5C7BDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Nov 2021 23:05:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 009926036B;\n\tSat, 13 Nov 2021 00:05:24 +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 3451E600B5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 13 Nov 2021 00:05:23 +0100 (CET)","from jade.amanokami.net (KD027085206038.au-net.ne.jp\n\t[27.85.206.38])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 004589CA;\n\tSat, 13 Nov 2021 00:05:20 +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=\"Hg3bkllN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636758322;\n\tbh=kf84TVfES327uKzfyywnbJhdP9OLNyVXtS7iO1j56/0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Hg3bkllN77uCXuhN5pLvDDIFWxBeAAyAy7x/tcwjGcFVLukmesOqBTgPRXButoJKz\n\tTIBNz4Aggpcg8bTxHZzzGGOiKqZgAO2dqCqC/j1NnYQi2GjNz4O/eFdNv61b4TLDtI\n\txyihCLuJlG0vf1wpQGBFYqqPiPMoCxVu8NLouV6A=","Date":"Sat, 13 Nov 2021 08:05:09 +0900","From":"Paul Elder <paul.elder@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<20211112230509.GC8453@jade.amanokami.net>","References":"<20211111140928.136111-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20944,"web_url":"https://patchwork.libcamera.org/comment/20944/","msgid":"<9082ea7f-c079-4bb0-4fb4-be3d9f366819@ideasonboard.com>","date":"2021-11-13T08:24:45","subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Paul,\n\nOn 13/11/2021 00:05, Paul Elder wrote:\n> Hi Jean-Michel,\n> \n> On Thu, Nov 11, 2021 at 03:09:19PM +0100, Jean-Michel Hautbois wrote:\n>> Now that we have the real exposure applied at each frame, remove the\n>> early return based on a frame counter and compute the gain for each\n>> frame.\n> \n> This is parsed as \"this patch removes the early return\" and \"this patch\n> computes the gain for each frame\".\n> \n>>\n>> Instead of that, introduce a number of startup frames during which the\n> \n> And this \"instead\" reads like it contradicts the two points above :/\n> \n> I think dropping \"instead of that\" or adding a bit more, like \"instead\n> of the early return\" would make it easier to read.\n\nI will remove \"Instead of that\", thanks !\n\n> \n>> filter speed is 1.0, meaning we apply instantly the exposure value\n>> calculated and not a slower filtered one.\n>>\n>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> With those clarifications (and the elaboration that you sent to Umang),\n> \n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> \n>> ---\n>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------\n>>   1 file changed, 11 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 83aa3676..74bce7bb 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {\n>>   \n>>   LOG_DEFINE_CATEGORY(IPU3Agc)\n>>   \n>> -/* Number of frames to wait before calculating stats on minimum exposure */\n>> -static constexpr uint32_t kInitialFrameMinAECount = 4;\n>> -/* Number of frames to wait between new gain/shutter time estimations */\n>> -static constexpr uint32_t kFrameSkipCount = 6;\n>> -\n>>   /* Limits for analogue gain values */\n>>   static constexpr double kMinAnalogueGain = 1.0;\n>>   static constexpr double kMaxAnalogueGain = 8.0;\n>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;\n>>    */\n>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;\n>>   \n>> +/* Number of frames to wait before calculating stats on minimum exposure */\n>> +static constexpr uint32_t kNumStartupFrames = 10;\n>> +\n>>   Agc::Agc()\n>> -\t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>> -\t  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n>> -\t  currentExposure_(0s), prevExposureValue_(0s)\n>> +\t: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),\n>> +\t  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n>> +\t  prevExposureValue_(0s)\n>>   {\n>>   }\n>>   \n>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n>>   void Agc::filterExposure()\n>>   {\n>>   \tdouble speed = 0.2;\n>> +\n>> +\t/* Adapt instantly if we are in startup phase */\n>> +\tif (frameCount_ < kNumStartupFrames)\n>> +\t\tspeed = 1.0;\n>> +\n>>   \tif (filteredExposure_ == 0s) {\n>>   \t\t/* DG stands for digital gain.*/\n>>   \t\tfilteredExposure_ = currentExposure_;\n>> @@ -185,13 +188,6 @@ void Agc::filterExposure()\n>>    */\n>>   void Agc::computeExposure(IPAFrameContext &frameContext)\n>>   {\n>> -\t/* Algorithm initialization should wait for first valid frames */\n>> -\t/* \\todo - have a number of frames given by DelayedControls ?\n>> -\t * - implement a function for IIR */\n>> -\tif ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))\n>> -\t\treturn;\n>> -\n>> -\tlastFrame_ = frameCount_;\n>>   \n>>   \t/* Are we correctly exposed ? */\n>>   \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n>> -- \n>> 2.32.0\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2A1E6BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 13 Nov 2021 08:24:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 564BD60368;\n\tSat, 13 Nov 2021 09:24:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBC4C60121\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 13 Nov 2021 09:24:48 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:4d35:9445:6881:4ffd] (unknown\n\t[IPv6:2a01:e0a:169:7140:4d35:9445:6881:4ffd])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8F57455C;\n\tSat, 13 Nov 2021 09:24: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=\"YZpAqwto\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636791888;\n\tbh=sqq4LYUWwyqb9dpjqg5LhFjh058izQTL0P/iSjv5i08=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=YZpAqwto7YOgUhxF88n4b96LCh99azJpx62vchxNl1uUiYxmUrUbhptAHFd22oZqz\n\txyGij8f3KsChXRh9iRYT9Vw25UClTmHU9q5sdh2tklvqGEmn5wwIyTeWJMpC0wZyyV\n\tIw6nMESAK23TJyxYU4nQnxvnEV9B45Ypu2UsR0oo=","Message-ID":"<9082ea7f-c079-4bb0-4fb4-be3d9f366819@ideasonboard.com>","Date":"Sat, 13 Nov 2021 09:24:45 +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":"Paul Elder <paul.elder@ideasonboard.com>","References":"<20211111140928.136111-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111140928.136111-6-jeanmichel.hautbois@ideasonboard.com>\n\t<20211112230509.GC8453@jade.amanokami.net>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","In-Reply-To":"<20211112230509.GC8453@jade.amanokami.net>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v4 05/14] ipa: ipu3: agc: Compute the\n\tgain for each frame","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]