[{"id":20203,"web_url":"https://patchwork.libcamera.org/comment/20203/","msgid":"<163421083187.3829429.11814980684329089722@Monstersaurus>","date":"2021-10-14T11:27:11","subject":"Re: [libcamera-devel] [PATCH 10/13] ipa: ipu3: agc: Introduce\n\tprevious exposure value","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-10-13 16:41:22)\n> We need to calculate the gain on the previous exposure value calculated.\n> This value needs to be updated after each process call, initialised to\n> 0s.\n> \n\nSeems resonable.\n\nChecking lockExposureGain() looks like it could be refactored to indent\nthat whole exposure calculation one level, but lets deal with that after\nthis series.\n\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 8 ++++++--\n> src/ipa/ipu3/algorithms/agc.h | 1 +\n> 2 files changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index bd28998a..7efe0907 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -46,7 +46,8 @@ static constexpr double kEvGainTarget = 0.5;\n> Agc::Agc()\n> : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n> - filteredExposureNoDg_(0s), currentExposure_(0s), currentExposureNoDg_(0s)\n> + filteredExposureNoDg_(0s), currentExposure_(0s),\n> + currentExposureNoDg_(0s), prevExposureValue(0s)\n> {\n> }\n> \n> @@ -145,7 +146,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> << \" Gain \" << analogueGain\n> << \" Needed ev gain \" << evGain;\n> \n> - currentExposure_ = currentExposureNoDg_ * evGain;\n> + currentExposure_ = prevExposureValue * evGain;\n> Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> @@ -182,6 +183,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> \n> exposure = shutterTime / lineDuration_;\n> analogueGain = stepGain;\n> +\n> + /* Update the exposure value for the next process call */\n> + prevExposureValue = shutterTime * analogueGain;\n> }\n> lastFrame_ = frameCount_;\n> }\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 7605ab39..32817c4f 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -51,6 +51,7 @@ private:\n> Duration filteredExposureNoDg_;\n> Duration currentExposure_;\n> Duration currentExposureNoDg_;\n> + Duration prevExposureValue;\n\nIt looks like this should have an underscore at the end.\n\nWith that,\n\nReviewed-by: Kieran Bingham \n\n> \n> uint32_t stride_;\n> };\n> -- \n> 2.30.2\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 5BC14C323E\n\tfor ;\n\tThu, 14 Oct 2021 11:27:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B87D568F4F;\n\tThu, 14 Oct 2021 13:27:16 +0200 (CEST)","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 C758F68541\n\tfor ;\n\tThu, 14 Oct 2021 13:27:14 +0200 (CEST)","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 569EC2F3;\n\tThu, 14 Oct 2021 13:27:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"B2bDeqgq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634210834;\n\tbh=RQVHq4T0fyxd64YQSRdNJh1nsrGUTGAGOGOgC1baQl4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=B2bDeqgq2AiPc8BLKfcIwq5ev/lihFvG2S7ad/dT7IXiIX3TLviQDCuWIZRTnTwA/\n\to6O23TV2bl8l9o+JBzs8p+4fVVyaEWzCfGLSECDOfuQ2SrikGJd332relo6DUJw0GS\n\tEAwlR/TtbRf6cM4fE3DJK3Jp5XM+T9uNzevy2Suk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211013154125.133419-11-jeanmichel.hautbois@ideasonboard.com>","References":"<20211013154125.133419-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211013154125.133419-11-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 14 Oct 2021 12:27:11 +0100","Message-ID":"<163421083187.3829429.11814980684329089722@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 10/13] ipa: ipu3: agc: Introduce\n\tprevious exposure value","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":20220,"web_url":"https://patchwork.libcamera.org/comment/20220/","msgid":"","date":"2021-10-15T01:27:13","subject":"Re: [libcamera-devel] [PATCH 10/13] ipa: ipu3: agc: Introduce\n\tprevious exposure value","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Oct 14, 2021 at 12:27:11PM +0100, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-10-13 16:41:22)\n> > We need to calculate the gain on the previous exposure value calculated.\n> > This value needs to be updated after each process call, initialised to\n> > 0s.\n> \n> Seems resonable.\n> \n> Checking lockExposureGain() looks like it could be refactored to indent\n> that whole exposure calculation one level, but lets deal with that after\n> this series.\n> \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 8 ++++++--\n> > src/ipa/ipu3/algorithms/agc.h | 1 +\n> > 2 files changed, 7 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index bd28998a..7efe0907 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -46,7 +46,8 @@ static constexpr double kEvGainTarget = 0.5;\n> > Agc::Agc()\n> > : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> > minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n> > - filteredExposureNoDg_(0s), currentExposure_(0s), currentExposureNoDg_(0s)\n> > + filteredExposureNoDg_(0s), currentExposure_(0s),\n> > + currentExposureNoDg_(0s), prevExposureValue(0s)\n> > {\n> > }\n> > \n> > @@ -145,7 +146,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> > << \" Gain \" << analogueGain\n> > << \" Needed ev gain \" << evGain;\n> > \n> > - currentExposure_ = currentExposureNoDg_ * evGain;\n> > + currentExposure_ = prevExposureValue * evGain;\n\nSo on the first run this will be 0, which means that you'll apply the\nminimum gain and shutter, instead of starting from the values that were\nused for the first frame. Doesn't this slow down convergence ?\n\nBeside, currentExposureNoDg_ contains the correct exposure that was used\nfor the previous frame, that's how it's calculated a few lines above.\nI'm failing to see what this patch fixes. The commit message doesn't\nactually mention it fixes something, so maybe it's just a refactoring ?\nIn either case, it seems to introduce an issue, and the commit message\ndoesn't explain what benefit it brings.\n\n> > Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> > currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> > LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> > @@ -182,6 +183,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> > \n> > exposure = shutterTime / lineDuration_;\n> > analogueGain = stepGain;\n> > +\n> > + /* Update the exposure value for the next process call */\n> > + prevExposureValue = shutterTime * analogueGain;\n> > }\n> > lastFrame_ = frameCount_;\n> > }\n> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> > index 7605ab39..32817c4f 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.h\n> > +++ b/src/ipa/ipu3/algorithms/agc.h\n> > @@ -51,6 +51,7 @@ private:\n> > Duration filteredExposureNoDg_;\n> > Duration currentExposure_;\n> > Duration currentExposureNoDg_;\n> > + Duration prevExposureValue;\n> \n> It looks like this should have an underscore at the end.\n> \n> With that,\n> \n> Reviewed-by: Kieran Bingham \n> \n> > \n> > uint32_t stride_;\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 A600EC323E\n\tfor ;\n\tFri, 15 Oct 2021 01:27:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40C0268F4E;\n\tFri, 15 Oct 2021 03:27:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3851A68F4D\n\tfor ;\n\tFri, 15 Oct 2021 03:27:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9709B2E3;\n\tFri, 15 Oct 2021 03:27:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HKeEG/KE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634261248;\n\tbh=EqGn6EUxIJI63T5y1HzswXPHDvidCw6v5kWV4W40OZA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HKeEG/KEsparcDxBNIsA8TiqsQXpSmAT/pvxcx4rzRs6Z3VPYCC1kzUUblBCTgANJ\n\tG0LO+bLz1WyysNAiNOqf7B8pHK6Ma59MoDChOQq2kbgBdmxNXd9GhS4Gp1HKoJpBFR\n\tXOP5bvoSiudbc3zDmfctNZO0UibbJJw2xvMQdY8w=","Date":"Fri, 15 Oct 2021 04:27:13 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211013154125.133419-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211013154125.133419-11-jeanmichel.hautbois@ideasonboard.com>\n\t<163421083187.3829429.11814980684329089722@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163421083187.3829429.11814980684329089722@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 10/13] ipa: ipu3: agc: Introduce\n\tprevious exposure value","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]