[{"id":20825,"web_url":"https://patchwork.libcamera.org/comment/20825/","msgid":"<163658172027.2121661.2589918574403026541@Monstersaurus>","date":"2021-11-10T22:02:00","subject":"Re: [libcamera-devel] [PATCH v2 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 Jean-Michel Hautbois (2021-11-10 19:58:52)\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> Signed-off-by: Jean-Michel Hautbois \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 2bf68e04..133f5931 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> - : 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), minExposureLines_(0),\n\nI see frameCount_ is initialised to zero here...\n\n\n> + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n> + 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> double speed = 0.2;\n> +\n> + /* Adapt instantly if we are in startup phase */\n> + if (frameCount_ < kNumStartupFrames)\n> + speed = 1.0;\n\nAnd it's used to determine how long into startup we are.\n\nIs it ever set to 0 on start/stop/configure operations that would\nnecessitate re-evaluating the whole startup procedure like this?\n\nI can't see anything resetting it - and we don't destroy and reconstruct\nthe objects so it isn't going to be re-initialised.\n\nThis may be a distinctly separate (and pre-existing) bug, and likely\nwarrant a patch or fix of its own.\n\nI wonder if we should instead be passing in the frame/sequence count for\nall algorithm operations. That (should?) be reset to zero when\nstarting/stopping I think ...\n\nEven with that limitation, I'm tempted to say this should still go in\nand the count can be fixed on top. I think this is still an improvement.\n\nReviewed-by: Kieran Bingham \n\n> +\n> if (filteredExposure_ == 0s) {\n> /* DG stands for digital gain.*/\n> filteredExposure_ = currentExposure_;\n> @@ -186,13 +189,6 @@ void Agc::filterExposure()\n> */\n> void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\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_ - lastFrame_ < kFrameSkipCount))\n> - return;\n> -\n> - lastFrame_ = frameCount_;\n> \n> /* Are we correctly exposed ? */\n> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> -- \n> 2.32.0\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 4C9C6BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 22:02:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8784E6035D;\n\tWed, 10 Nov 2021 23:02:04 +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 6B3D76033C\n\tfor ;\n\tWed, 10 Nov 2021 23:02:03 +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 0D9798B6;\n\tWed, 10 Nov 2021 23:02:03 +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=\"MY6GGBiB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636581723;\n\tbh=1+jZb+M9xAadqdd5hJYaCOLUEMiuQvliuxKpFRbSi2g=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=MY6GGBiBqOYAVWiGkbACFJES/shHNLY1l9o8TdtwAnBALhWsBZ5qJVKr0f0+6M/S8\n\t4W9jmpwDPal/wAyx2vuSQKDEAEXtM88I3ctPFP7dMDcBcuCqPpKqcFKUirrv8NKZiX\n\tO4t5lbD859MKQe9yW8ZQOLfRkb65tD4DMFYFWZqw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211110195901.85597-6-jeanmichel.hautbois@ideasonboard.com>","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-6-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 10 Nov 2021 22:02:00 +0000","Message-ID":"<163658172027.2121661.2589918574403026541@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","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":20828,"web_url":"https://patchwork.libcamera.org/comment/20828/","msgid":"<53e02ecc-a011-1c6d-0157-0682f1cedfd4@ideasonboard.com>","date":"2021-11-10T22:12:50","subject":"Re: [libcamera-devel] [PATCH v2 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 Kieran,\n\nOn 10/11/2021 23:02, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-11-10 19:58:52)\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>> Signed-off-by: Jean-Michel Hautbois \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 2bf68e04..133f5931 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>> - : 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), minExposureLines_(0),\n> \n> I see frameCount_ is initialised to zero here...\n> \n> \n>> + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n>> + 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>> double speed = 0.2;\n>> +\n>> + /* Adapt instantly if we are in startup phase */\n>> + if (frameCount_ < kNumStartupFrames)\n>> + speed = 1.0;\n> \n> And it's used to determine how long into startup we are.\n> \n> Is it ever set to 0 on start/stop/configure operations that would\n> necessitate re-evaluating the whole startup procedure like this?\n> \n> I can't see anything resetting it - and we don't destroy and reconstruct\n> the objects so it isn't going to be re-initialised.\n> \n> This may be a distinctly separate (and pre-existing) bug, and likely\n> warrant a patch or fix of its own.\n> \n> I wonder if we should instead be passing in the frame/sequence count for\n> all algorithm operations. That (should?) be reset to zero when\n> starting/stopping I think ...\n> \n\nWe could add the frameId in the frameContext, and fill it in \nEventStatReady. That would solve the issue and remove the frameCount_ at \nthe same time ?\n\n> Even with that limitation, I'm tempted to say this should still go in\n> and the count can be fixed on top. I think this is still an improvement.\n> \n> Reviewed-by: Kieran Bingham \n> \n>> +\n>> if (filteredExposure_ == 0s) {\n>> /* DG stands for digital gain.*/\n>> filteredExposure_ = currentExposure_;\n>> @@ -186,13 +189,6 @@ void Agc::filterExposure()\n>> */\n>> void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\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_ - lastFrame_ < kFrameSkipCount))\n>> - return;\n>> -\n>> - lastFrame_ = frameCount_;\n>> \n>> /* Are we correctly exposed ? */\n>> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n>> -- \n>> 2.32.0\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 9B1C9BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 22:12:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3E9F6035D;\n\tWed, 10 Nov 2021 23:12:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C4E76033C\n\tfor ;\n\tWed, 10 Nov 2021 23:12:53 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:7713:3465:89e6:c5cd] (unknown\n\t[IPv6:2a01:e0a:169:7140:7713:3465:89e6:c5cd])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A61C8B6;\n\tWed, 10 Nov 2021 23:12:53 +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=\"iWvxB1To\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636582373;\n\tbh=5mrbJ9BYhcsgHyFM4tE1h+akTJiCLQBNAIZpHUlLB84=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=iWvxB1ToBsnrgcDQKC5ePfB90HSbw7TEKO7aNe4KD1Z0aRvynudHtoxty/OSXpcqu\n\tX5AdqBW2qVN73QpNFz/NIuOHS7mRjhPYeQMTKxj2iZW3yfWDvCiXU/x7n4JQ2S9ojb\n\tR3AtL+We7NJ8JPIxebMKQu6SY3IJLsJzO9mRfqaQ=","Message-ID":"<53e02ecc-a011-1c6d-0157-0682f1cedfd4@ideasonboard.com>","Date":"Wed, 10 Nov 2021 23:12:50 +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":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-6-jeanmichel.hautbois@ideasonboard.com>\n\t<163658172027.2121661.2589918574403026541@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163658172027.2121661.2589918574403026541@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","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":20837,"web_url":"https://patchwork.libcamera.org/comment/20837/","msgid":"<163658931287.2121661.17418781646917536858@Monstersaurus>","date":"2021-11-11T00:08:32","subject":"Re: [libcamera-devel] [PATCH v2 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 Jean-Michel Hautbois (2021-11-10 22:12:50)\n> Hi Kieran,\n> \n> On 10/11/2021 23:02, Kieran Bingham wrote:\n> > Quoting Jean-Michel Hautbois (2021-11-10 19:58:52)\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> >> Signed-off-by: Jean-Michel Hautbois \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 2bf68e04..133f5931 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> >> - : 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), minExposureLines_(0),\n> > \n> > I see frameCount_ is initialised to zero here...\n> > \n> > \n> >> + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n> >> + 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> >> double speed = 0.2;\n> >> +\n> >> + /* Adapt instantly if we are in startup phase */\n> >> + if (frameCount_ < kNumStartupFrames)\n> >> + speed = 1.0;\n> > \n> > And it's used to determine how long into startup we are.\n> > \n> > Is it ever set to 0 on start/stop/configure operations that would\n> > necessitate re-evaluating the whole startup procedure like this?\n> > \n> > I can't see anything resetting it - and we don't destroy and reconstruct\n> > the objects so it isn't going to be re-initialised.\n> > \n> > This may be a distinctly separate (and pre-existing) bug, and likely\n> > warrant a patch or fix of its own.\n> > \n> > I wonder if we should instead be passing in the frame/sequence count for\n> > all algorithm operations. That (should?) be reset to zero when\n> > starting/stopping I think ...\n> > \n> \n> We could add the frameId in the frameContext, and fill it in \n> EventStatReady. That would solve the issue and remove the frameCount_ at \n> the same time ?\n\n\nAh yes, I think you had something like that already? Anyway, it can be\nsolved on top of this series. I thought we'd have shrunk the /22 down a\nbit more ;-) but seems there's still plenty of change in play.\n\n--\nKieran\n\n\n\n> \n> > Even with that limitation, I'm tempted to say this should still go in\n> > and the count can be fixed on top. I think this is still an improvement.\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> >> +\n> >> if (filteredExposure_ == 0s) {\n> >> /* DG stands for digital gain.*/\n> >> filteredExposure_ = currentExposure_;\n> >> @@ -186,13 +189,6 @@ void Agc::filterExposure()\n> >> */\n> >> void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\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_ - lastFrame_ < kFrameSkipCount))\n> >> - return;\n> >> -\n> >> - lastFrame_ = frameCount_;\n> >> \n> >> /* Are we correctly exposed ? */\n> >> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> >> -- \n> >> 2.32.0\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 929D1BF415\n\tfor ;\n\tThu, 11 Nov 2021 00:08:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6B1E6035A;\n\tThu, 11 Nov 2021 01:08:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6179600BF\n\tfor ;\n\tThu, 11 Nov 2021 01:08:35 +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 516CC55C;\n\tThu, 11 Nov 2021 01:08:35 +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=\"nHA3Vsvo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636589315;\n\tbh=J5gXNl+SKrnshhOM5RDYX4L6IU+a2E+Z8SHkonlN3jM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=nHA3VsvoiX6EsuwJesm4on19yZQySeyCH/XX9slslf2RXf55s6sQ8Ire3AjqNNCjy\n\tSyLDjYrMUn08Twpvsl9uj4yJnblLdDjsrLgpNQpzaJEcObg3KyYvSDPsoLuiAYn6b1\n\tSyN7PJV/DIm0Koum/00Y+ZTEU6lqCleDuLEzq20k=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<53e02ecc-a011-1c6d-0157-0682f1cedfd4@ideasonboard.com>","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-6-jeanmichel.hautbois@ideasonboard.com>\n\t<163658172027.2121661.2589918574403026541@Monstersaurus>\n\t<53e02ecc-a011-1c6d-0157-0682f1cedfd4@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 11 Nov 2021 00:08:32 +0000","Message-ID":"<163658931287.2121661.17418781646917536858@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","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":20850,"web_url":"https://patchwork.libcamera.org/comment/20850/","msgid":"","date":"2021-11-11T10:06:52","subject":"Re: [libcamera-devel] [PATCH v2 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":"On 11/11/2021 01:08, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-11-10 22:12:50)\n>> Hi Kieran,\n>>\n>> On 10/11/2021 23:02, Kieran Bingham wrote:\n>>> Quoting Jean-Michel Hautbois (2021-11-10 19:58:52)\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>>>> Signed-off-by: Jean-Michel Hautbois \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 2bf68e04..133f5931 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>>>> - : 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), minExposureLines_(0),\n>>>\n>>> I see frameCount_ is initialised to zero here...\n>>>\n>>>\n>>>> + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n>>>> + 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>>>> double speed = 0.2;\n>>>> +\n>>>> + /* Adapt instantly if we are in startup phase */\n>>>> + if (frameCount_ < kNumStartupFrames)\n>>>> + speed = 1.0;\n>>>\n>>> And it's used to determine how long into startup we are.\n>>>\n>>> Is it ever set to 0 on start/stop/configure operations that would\n>>> necessitate re-evaluating the whole startup procedure like this?\n>>>\n>>> I can't see anything resetting it - and we don't destroy and reconstruct\n>>> the objects so it isn't going to be re-initialised.\n>>>\n>>> This may be a distinctly separate (and pre-existing) bug, and likely\n>>> warrant a patch or fix of its own.\n>>>\n>>> I wonder if we should instead be passing in the frame/sequence count for\n>>> all algorithm operations. That (should?) be reset to zero when\n>>> starting/stopping I think ...\n>>>\n>>\n>> We could add the frameId in the frameContext, and fill it in\n>> EventStatReady. That would solve the issue and remove the frameCount_ at\n>> the same time ?\n> \n> \n> Ah yes, I think you had something like that already? Anyway, it can be\n> solved on top of this series. I thought we'd have shrunk the /22 down a\n> bit more ;-) but seems there's still plenty of change in play.\n> \n\nI can cherry-pick the frameId introduction and use it in a patch \nremoving frameCount_, we would then have 15 instead of 22 patches :-).\n\n> --\n> Kieran\n> \n> \n> \n>>\n>>> Even with that limitation, I'm tempted to say this should still go in\n>>> and the count can be fixed on top. I think this is still an improvement.\n>>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>>> +\n>>>> if (filteredExposure_ == 0s) {\n>>>> /* DG stands for digital gain.*/\n>>>> filteredExposure_ = currentExposure_;\n>>>> @@ -186,13 +189,6 @@ void Agc::filterExposure()\n>>>> */\n>>>> void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\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_ - lastFrame_ < kFrameSkipCount))\n>>>> - return;\n>>>> -\n>>>> - lastFrame_ = frameCount_;\n>>>> \n>>>> /* Are we correctly exposed ? */\n>>>> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n>>>> -- \n>>>> 2.32.0\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 49F4CBF415\n\tfor ;\n\tThu, 11 Nov 2021 10:06:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B09B60345;\n\tThu, 11 Nov 2021 11:06:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05BF06032C\n\tfor ;\n\tThu, 11 Nov 2021 11:06:54 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:e627:8337:a781:d98] (unknown\n\t[IPv6:2a01:e0a:169:7140:e627:8337:a781:d98])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A8DBE51;\n\tThu, 11 Nov 2021 11:06:54 +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=\"qJGSNOKV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636625214;\n\tbh=aTD8jtNn+5bHdZ//njYzrL11A3iAoOEs6QumnKi8cF0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=qJGSNOKVnoSSS3akrairhZICHbQ1mKP2ViK3hE/8/pLq9wmajPCuSyM2H/8sfS2IB\n\tyGPIJQmUAPphoGX8kINKpUURnrzl6JBLWDCGku5DxVYIXxI3CLeolhahZE+nyzmOBW\n\tjuOP7Msq/e6XBAVG2AtlfXW50oMsV7lWgBOuu238=","Message-ID":"","Date":"Thu, 11 Nov 2021 11:06:52 +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":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-6-jeanmichel.hautbois@ideasonboard.com>\n\t<163658172027.2121661.2589918574403026541@Monstersaurus>\n\t<53e02ecc-a011-1c6d-0157-0682f1cedfd4@ideasonboard.com>\n\t<163658931287.2121661.17418781646917536858@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163658931287.2121661.17418781646917536858@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","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\" "}}]