[{"id":18762,"web_url":"https://patchwork.libcamera.org/comment/18762/","msgid":"","date":"2021-08-13T10:06:11","subject":"Re: [libcamera-devel] [PATCH v2 08/10] ipa: ipu3: Remove unneeded\n\tfunction calls in AGC","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 12/08/2021 17:52, Jean-Michel Hautbois wrote:\n> We never use converged_ so remove its declaration.\n> The controls may not need to be updated at each call, but it should be\n> decided on the context side and not by a specific call.\n\nGood, I see how this helps move towards making it more modular.\nI guess the short term effect is that the controls will be set for every\nframe, but if the values don't change, then that's likely a no-op in the\nkernel anyway so I dont' think that's an issue.\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 6 ++----\n> src/ipa/ipu3/ipu3_agc.cpp | 10 ++--------\n> src/ipa/ipu3/ipu3_agc.h | 5 -----\n> 3 files changed, 4 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 2a8225a0..27765aa6 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -309,8 +309,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> \tfor (auto const &algo : algorithms_)\n> \t\talgo->prepare(context_, params_);\n> \n> -\tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->prepare(context_, params_);\n> +\tawbAlgo_->prepare(context_, params_);\n> \n> \t*params = params_;\n> \n> @@ -338,8 +337,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> \n> \tawbAlgo_->process(context_, stats);\n> \n> -\tif (agcAlgo_->updateControls())\n> -\t\tsetControls(frame);\n> +\tsetControls(frame);\n> \n> \t/* \\todo Use VBlank value calculated from each frame exposure. */\n> \tint64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /\n> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> index 0cc35b9f..0d0584a8 100644\n> --- a/src/ipa/ipu3/ipu3_agc.cpp\n> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n> @@ -51,9 +51,8 @@ static constexpr double kEvGainTarget = 0.5;\n> static constexpr uint8_t kCellSize = 8;\n> \n> IPU3Agc::IPU3Agc()\n> -\t: frameCount_(0), lastFrame_(0), converged_(false),\n> -\t updateControls_(false), iqMean_(0.0),\n> -\t lineDuration_(0s), maxExposureTime_(0s),\n> +\t: frameCount_(0), lastFrame_(0),\n> +\t iqMean_(0.0), lineDuration_(0s), maxExposureTime_(0s),\n> \t prevExposure_(0s), prevExposureNoDg_(0s),\n> \t currentExposure_(0s), currentExposureNoDg_(0s)\n> {\n> @@ -146,8 +145,6 @@ void IPU3Agc::filterExposure()\n> \n> void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> {\n> -\tupdateControls_ = false;\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> @@ -157,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> \t/* Are we correctly exposed ? */\n> \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> \t\tLOG(IPU3Agc, Debug) << \"!!! Good exposure with iqMean = \" << iqMean_;\n> -\t\tconverged_ = true;\n> \t} else {\n> \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> @@ -180,12 +176,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n> \t\t\tnewExposure = currentExposure_ / exposure;\n> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n> -\t\t\tupdateControls_ = true;\n> \t\t} else if (currentShutter >= maxExposureTime_) {\n> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);\n> \t\t\tnewExposure = currentExposure_ / gain;\n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);\n> -\t\t\tupdateControls_ = true;\n> \t\t}\n> \t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n> \t}\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index e096648b..0e922664 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -31,8 +31,6 @@ public:\n> \n> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> \tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> -\tbool converged() { return converged_; }\n> -\tbool updateControls() { return updateControls_; }\n> \n> private:\n> \tvoid processBrightness(const ipu3_uapi_stats_3a *stats);\n> @@ -44,9 +42,6 @@ private:\n> \tuint64_t frameCount_;\n> \tuint64_t lastFrame_;\n> \n> -\tbool converged_;\n> -\tbool updateControls_;\n> -\n> \tdouble iqMean_;\n> \n> \tDuration lineDuration_;\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 70B7DBD87D\n\tfor ;\n\tFri, 13 Aug 2021 10:06:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4B216888D;\n\tFri, 13 Aug 2021 12:06:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CBC22687FA\n\tfor ;\n\tFri, 13 Aug 2021 12:06:14 +0200 (CEST)","from [192.168.0.20]\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 3BE8EEE;\n\tFri, 13 Aug 2021 12:06: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=\"fSGkeRLb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628849174;\n\tbh=/I7miRB8WkwYg1JyEtSO8Ktm5jcyLC0hPDUYjhb08yc=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=fSGkeRLby2duO0Nnzw+1wl5+4r/OT1iRZmmq4j5J+iv3mUVvWVCILiwFokSPlfuX9\n\tj1Pl7oeCUvxEhYUB8V0KBArn0ld6aH+Ppaos+BAMUWt9oVi/6MnrNM5QbBmOgouinE\n\thRbl96AKi1VloOtxTObRVByRaOVQjO1TCSBF0UXw=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-9-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Fri, 13 Aug 2021 11:06:11 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210812165243.276977-9-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 08/10] ipa: ipu3: Remove unneeded\n\tfunction calls in AGC","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":18809,"web_url":"https://patchwork.libcamera.org/comment/18809/","msgid":"","date":"2021-08-15T23:59:10","subject":"Re: [libcamera-devel] [PATCH v2 08/10] ipa: ipu3: Remove unneeded\n\tfunction calls in AGC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Fri, Aug 13, 2021 at 11:06:11AM +0100, Kieran Bingham wrote:\n> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:\n> > We never use converged_ so remove its declaration.\n> > The controls may not need to be updated at each call, but it should be\n> > decided on the context side and not by a specific call.\n\nCould you elaborate on this ? Specifically, the comment message seems to\nimply that this is a useful feature, but that it should be implemented\ndifferently. How do you plan to implement it ?\n\n> Good, I see how this helps move towards making it more modular.\n> I guess the short term effect is that the controls will be set for every\n> frame, but if the values don't change, then that's likely a no-op in the\n> kernel anyway so I dont' think that's an issue.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/ipu3.cpp | 6 ++----\n> > src/ipa/ipu3/ipu3_agc.cpp | 10 ++--------\n> > src/ipa/ipu3/ipu3_agc.h | 5 -----\n> > 3 files changed, 4 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 2a8225a0..27765aa6 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -309,8 +309,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > \tfor (auto const &algo : algorithms_)\n> > \t\talgo->prepare(context_, params_);\n> > \n> > -\tif (agcAlgo_->updateControls())\n> > -\t\tawbAlgo_->prepare(context_, params_);\n> > +\tawbAlgo_->prepare(context_, params_);\n> > \n> > \t*params = params_;\n> > \n> > @@ -338,8 +337,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > \n> > \tawbAlgo_->process(context_, stats);\n> > \n> > -\tif (agcAlgo_->updateControls())\n> > -\t\tsetControls(frame);\n> > +\tsetControls(frame);\n> > \n> > \t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > \tint64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /\n> > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> > index 0cc35b9f..0d0584a8 100644\n> > --- a/src/ipa/ipu3/ipu3_agc.cpp\n> > +++ b/src/ipa/ipu3/ipu3_agc.cpp\n> > @@ -51,9 +51,8 @@ static constexpr double kEvGainTarget = 0.5;\n> > static constexpr uint8_t kCellSize = 8;\n> > \n> > IPU3Agc::IPU3Agc()\n> > -\t: frameCount_(0), lastFrame_(0), converged_(false),\n> > -\t updateControls_(false), iqMean_(0.0),\n> > -\t lineDuration_(0s), maxExposureTime_(0s),\n> > +\t: frameCount_(0), lastFrame_(0),\n> > +\t iqMean_(0.0), lineDuration_(0s), maxExposureTime_(0s),\n> > \t prevExposure_(0s), prevExposureNoDg_(0s),\n> > \t currentExposure_(0s), currentExposureNoDg_(0s)\n\nYou can reflow this as\n\n\t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n\t maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),\n \t currentExposure_(0s), currentExposureNoDg_(0s)\n\n> > {\n> > @@ -146,8 +145,6 @@ void IPU3Agc::filterExposure()\n> > \n> > void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> > {\n> > -\tupdateControls_ = false;\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> > @@ -157,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> > \t/* Are we correctly exposed ? */\n> > \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> > \t\tLOG(IPU3Agc, Debug) << \"!!! Good exposure with iqMean = \" << iqMean_;\n> > -\t\tconverged_ = true;\n> > \t} else {\n> > \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> > \n> > @@ -180,12 +176,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> > \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n> > \t\t\tnewExposure = currentExposure_ / exposure;\n> > \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n> > -\t\t\tupdateControls_ = true;\n> > \t\t} else if (currentShutter >= maxExposureTime_) {\n> > \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);\n> > \t\t\tnewExposure = currentExposure_ / gain;\n> > \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);\n> > -\t\t\tupdateControls_ = true;\n> > \t\t}\n> > \t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n> > \t}\n> > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> > index e096648b..0e922664 100644\n> > --- a/src/ipa/ipu3/ipu3_agc.h\n> > +++ b/src/ipa/ipu3/ipu3_agc.h\n> > @@ -31,8 +31,6 @@ public:\n> > \n> > \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> > \tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> > -\tbool converged() { return converged_; }\n> > -\tbool updateControls() { return updateControls_; }\n> > \n> > private:\n> > \tvoid processBrightness(const ipu3_uapi_stats_3a *stats);\n> > @@ -44,9 +42,6 @@ private:\n> > \tuint64_t frameCount_;\n> > \tuint64_t lastFrame_;\n> > \n> > -\tbool converged_;\n> > -\tbool updateControls_;\n> > -\n> > \tdouble iqMean_;\n> > \n> > \tDuration lineDuration_;","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 8A126BD87C\n\tfor ;\n\tSun, 15 Aug 2021 23:59:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6CA268890;\n\tMon, 16 Aug 2021 01:59:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27BA660261\n\tfor ;\n\tMon, 16 Aug 2021 01:59:16 +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 964AB2C5;\n\tMon, 16 Aug 2021 01:59:15 +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=\"t0W2cVbE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629071955;\n\tbh=qU6lMI+tPL5cQ/bJg2o+L4660tQ3A4c1J0Uqg2Yqdi8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t0W2cVbE9ujm0P7HHQK/L9DReVBrG7F/xR2izpC05JS/GjplPad69Gxhr96OJPbc6\n\tHV6MPXW/VgiigKRcvJp9vZo+ntzKEhz0iPJrpdGncqTkfJ7HvjT5NPI+Ph5HwtEvZW\n\t9ekSXgjdK5GRpNYPSMDnU7f/m46RyW95upgflHPs=","Date":"Mon, 16 Aug 2021 02:59:10 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-9-jeanmichel.hautbois@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 08/10] ipa: ipu3: Remove unneeded\n\tfunction calls in AGC","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\" "}}]