[{"id":21235,"web_url":"https://patchwork.libcamera.org/comment/21235/","msgid":"<163784101086.3059017.17081389453082894176@Monstersaurus>","date":"2021-11-25T11:50:10","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: ipu3: Return filtered 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-11-25 10:21:40)\n> When the current exposure value is calculated, it is cached and used by\n> filterExposure(). Use private filteredExposure_ and pass currentExposure\n> as a member variable.\n> \n> In order to limit the use of filteredExposure_, return the value from\n> filterExposure().\n> \n> While at it, remove a stale comment.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 31 ++++++++++++++++---------------\n> src/ipa/ipu3/algorithms/agc.h | 3 +--\n> 2 files changed, 17 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 582f0ae1..2945a138 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -71,7 +71,7 @@ static constexpr double kRelativeLuminanceTarget = 0.16;\n> \n> Agc::Agc()\n> : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s),\n> - maxShutterSpeed_(0s), filteredExposure_(0s), currentExposure_(0s)\n> + maxShutterSpeed_(0s), filteredExposure_(0s)\n> {\n> }\n> \n> @@ -143,7 +143,7 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n> /**\n> * \\brief Apply a filter on the exposure value to limit the speed of changes\n> */\n> -void Agc::filterExposure()\n> +utils::Duration Agc::filterExposure(utils::Duration currentExposure)\n> {\n> double speed = 0.2;\n> \n> @@ -152,23 +152,24 @@ void Agc::filterExposure()\n> speed = 1.0;\n> \n> if (filteredExposure_ == 0s) {\n> - /* DG stands for digital gain.*/\n> - filteredExposure_ = currentExposure_;\n> + filteredExposure_ = currentExposure;\n> } else {\n> /*\n> * If we are close to the desired result, go faster to avoid making\n> * multiple micro-adjustments.\n> * \\todo Make this customisable?\n> */\n> - if (filteredExposure_ < 1.2 * currentExposure_ &&\n> - filteredExposure_ > 0.8 * currentExposure_)\n> + if (filteredExposure_ < 1.2 * currentExposure &&\n> + filteredExposure_ > 0.8 * currentExposure)\n> speed = sqrt(speed);\n> \n> - filteredExposure_ = speed * currentExposure_ +\n> + filteredExposure_ = speed * currentExposure +\n> filteredExposure_ * (1.0 - speed);\n> }\n> \n> LOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << filteredExposure_;\n> +\n> + return filteredExposure_;\n> }\n> \n> /**\n> @@ -212,19 +213,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,\n> * Calculate the current exposure value for the scene as the latest\n> * exposure value applied multiplied by the new estimated gain.\n> */\n> - currentExposure_ = effectiveExposureValue * evGain;\n> + utils::Duration currentExposure = effectiveExposureValue * evGain;\n> \n> /* Clamp the exposure value to the min and max authorized */\n> utils::Duration maxTotalExposure = maxShutterSpeed_ * maxAnalogueGain_;\n> - currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> - LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> + currentExposure = std::min(currentExposure, maxTotalExposure);\n> + LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure\n> << \", maximum is \" << maxTotalExposure;\n> \n> - /* \\todo: estimate if we need to desaturate */\n> - filterExposure();\n> -\n> - /* Divide the exposure value as new exposure and gain values */\n> - utils::Duration exposureValue = filteredExposure_;\n> + /*\n> + * Divide the exposure value as new exposure and gain values\n> + * \\todo: estimate if we need to desaturate\n\nWhile refactoring these, this might be a good time to look at those\ncomments. I'm not entirely sure what they mean?\n\n\"Divide the exposure value as new exposure and gain values\"\nIs that directly related to the filter? Or is that more about somethign\nthat will happen further down?\n\nFiltering isn't dividing as a new exposure and gain ...?\n\nAnd the:\n \"todo: estimate if we need to desaturate\"\n\nDoesn't make sense in the context of the filter either?\nThe filter won't ever saturate will it? it simply slows the\nresponse/change rate of the exposure value....\n\n\nThat said, refactoring those can be done as a separate patch.\n/this/ patch is making the filter itself clearer, and it's only moving\nexisting comments.\n\nReviewed-by: Kieran Bingham \n\n\n> + */\n> + utils::Duration exposureValue = filterExposure(currentExposure);\n> utils::Duration shutterTime;\n> \n> /*\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 96ec7005..84bfe045 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -33,7 +33,7 @@ public:\n> private:\n> double measureBrightness(const ipu3_uapi_stats_3a *stats,\n> const ipu3_uapi_grid_config &grid) const;\n> - void filterExposure();\n> + utils::Duration filterExposure(utils::Duration currentExposure);\n> void computeExposure(IPAFrameContext &frameContext, double yGain,\n> double iqMeanGain);\n> double estimateLuminance(IPAFrameContext &frameContext,\n> @@ -51,7 +51,6 @@ private:\n> double maxAnalogueGain_;\n> \n> utils::Duration filteredExposure_;\n> - utils::Duration currentExposure_;\n> \n> uint32_t stride_;\n> };\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 94386BF415\n\tfor ;\n\tThu, 25 Nov 2021 11:50:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8B1060231;\n\tThu, 25 Nov 2021 12:50:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C848C60231\n\tfor ;\n\tThu, 25 Nov 2021 12:50:13 +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 5CF2D90E;\n\tThu, 25 Nov 2021 12:50:13 +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=\"P/iYbzgZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637841013;\n\tbh=ySv9FE50PLH52gS44In/cLpB8MPaDWjfLR8D++uHXvE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=P/iYbzgZ2MMl/utotwjCCAU01iNcTX3pOaApLLc6NLTkpFBK4bNm9kj5YNnGn/EHb\n\tYgF47q9Vl+kXGgoXvq4Tol2OYUsSyr5J0wiSvxm+09REpclpndtYmkAMWUUO2MEl1F\n\tdiT90zmS5Xb/cUYcpQcH94CNK40dmo5MMp9BqZOc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211125102143.52556-2-jeanmichel.hautbois@ideasonboard.com>","References":"<20211125102143.52556-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211125102143.52556-2-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 25 Nov 2021 11:50:10 +0000","Message-ID":"<163784101086.3059017.17081389453082894176@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: ipu3: Return filtered 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":21240,"web_url":"https://patchwork.libcamera.org/comment/21240/","msgid":"","date":"2021-11-25T12:05:01","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: ipu3: Return filtered value","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Nov 25, 2021 at 11:50:10AM +0000, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-11-25 10:21:40)\n> > When the current exposure value is calculated, it is cached and used by\n> > filterExposure(). Use private filteredExposure_ and pass currentExposure\n> > as a member variable.\n\ns/member variable/parameter/\n\n> > In order to limit the use of filteredExposure_, return the value from\n> > filterExposure().\n> > \n> > While at it, remove a stale comment.\n> > \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 31 ++++++++++++++++---------------\n> > src/ipa/ipu3/algorithms/agc.h | 3 +--\n> > 2 files changed, 17 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index 582f0ae1..2945a138 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -71,7 +71,7 @@ static constexpr double kRelativeLuminanceTarget = 0.16;\n> > \n> > Agc::Agc()\n> > : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s),\n> > - maxShutterSpeed_(0s), filteredExposure_(0s), currentExposure_(0s)\n> > + maxShutterSpeed_(0s), filteredExposure_(0s)\n> > {\n> > }\n> > \n> > @@ -143,7 +143,7 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n> > /**\n> > * \\brief Apply a filter on the exposure value to limit the speed of changes\n\nMissing \\param and \\return. Kieran has proposed a good documentation for\nthis function in the RkISP1 IPA series.\n\n> > */\n> > -void Agc::filterExposure()\n> > +utils::Duration Agc::filterExposure(utils::Duration currentExposure)\n> > {\n> > double speed = 0.2;\n> > \n> > @@ -152,23 +152,24 @@ void Agc::filterExposure()\n> > speed = 1.0;\n> > \n> > if (filteredExposure_ == 0s) {\n> > - /* DG stands for digital gain.*/\n> > - filteredExposure_ = currentExposure_;\n> > + filteredExposure_ = currentExposure;\n> > } else {\n> > /*\n> > * If we are close to the desired result, go faster to avoid making\n> > * multiple micro-adjustments.\n> > * \\todo Make this customisable?\n> > */\n> > - if (filteredExposure_ < 1.2 * currentExposure_ &&\n> > - filteredExposure_ > 0.8 * currentExposure_)\n> > + if (filteredExposure_ < 1.2 * currentExposure &&\n> > + filteredExposure_ > 0.8 * currentExposure)\n> > speed = sqrt(speed);\n> > \n> > - filteredExposure_ = speed * currentExposure_ +\n> > + filteredExposure_ = speed * currentExposure +\n> > filteredExposure_ * (1.0 - speed);\n> > }\n> > \n> > LOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << filteredExposure_;\n> > +\n> > + return filteredExposure_;\n> > }\n> > \n> > /**\n> > @@ -212,19 +213,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,\n> > * Calculate the current exposure value for the scene as the latest\n> > * exposure value applied multiplied by the new estimated gain.\n> > */\n> > - currentExposure_ = effectiveExposureValue * evGain;\n> > + utils::Duration currentExposure = effectiveExposureValue * evGain;\n> > \n> > /* Clamp the exposure value to the min and max authorized */\n> > utils::Duration maxTotalExposure = maxShutterSpeed_ * maxAnalogueGain_;\n> > - currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> > - LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> > + currentExposure = std::min(currentExposure, maxTotalExposure);\n> > + LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure\n> > << \", maximum is \" << maxTotalExposure;\n> > \n> > - /* \\todo: estimate if we need to desaturate */\n> > - filterExposure();\n> > -\n> > - /* Divide the exposure value as new exposure and gain values */\n> > - utils::Duration exposureValue = filteredExposure_;\n> > + /*\n> > + * Divide the exposure value as new exposure and gain values\n> > + * \\todo: estimate if we need to desaturate\n> \n> While refactoring these, this might be a good time to look at those\n> comments. I'm not entirely sure what they mean?\n> \n> \"Divide the exposure value as new exposure and gain values\"\n> Is that directly related to the filter? Or is that more about somethign\n> that will happen further down?\n> \n> Filtering isn't dividing as a new exposure and gain ...?\n> \n> And the:\n> \"todo: estimate if we need to desaturate\"\n> \n> Doesn't make sense in the context of the filter either?\n> The filter won't ever saturate will it? it simply slows the\n> response/change rate of the exposure value....\n\nThe first comment is about the process that happens after filtering. I'd\nwrite\n\n\tutils::Duration exposureValue = effectiveExposureValue * evGain;\n\n\t/* Clamp the exposure value to the min and max authorized */\n\tutils::Duration maxTotalExposure = maxShutterSpeed_ * maxAnalogueGain_;\n\texposureValue = std::min(exposureValue, maxTotalExposure);\n\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << exposureValue\n\t\t\t << \", maximum is \" << maxTotalExposure;\n\n\t/*\n\t * Filter the exposure.\n\t * \\todo: estimate if we need to desaturate\n\t */\n\texposureValue = filterExposure(exposureValue);\n\n\t/*\n\t * Divide the exposure value as new exposure and gain values.\n\t *\n\t * Push the shutter time up to the maximum first, and only then\n\t * increase the gain.\n\t */\n\tutils::Duration shutterTime =\n\t\tstd::clamp(exposureValue / minAnalogueGain_,\n\t\t\t\t\t minShutterSpeed_, maxShutterSpeed_);\n\n(this removes the name \"currentExposure\" which wasn't actually\n\"current\".\n\nReviewed-by: Laurent Pinchart \n\n> That said, refactoring those can be done as a separate patch.\n> /this/ patch is making the filter itself clearer, and it's only moving\n> existing comments.\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> > + */\n> > + utils::Duration exposureValue = filterExposure(currentExposure);\n> > utils::Duration shutterTime;\n> > \n> > /*\n> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> > index 96ec7005..84bfe045 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.h\n> > +++ b/src/ipa/ipu3/algorithms/agc.h\n> > @@ -33,7 +33,7 @@ public:\n> > private:\n> > double measureBrightness(const ipu3_uapi_stats_3a *stats,\n> > const ipu3_uapi_grid_config &grid) const;\n> > - void filterExposure();\n> > + utils::Duration filterExposure(utils::Duration currentExposure);\n> > void computeExposure(IPAFrameContext &frameContext, double yGain,\n> > double iqMeanGain);\n> > double estimateLuminance(IPAFrameContext &frameContext,\n> > @@ -51,7 +51,6 @@ private:\n> > double maxAnalogueGain_;\n> > \n> > utils::Duration filteredExposure_;\n> > - utils::Duration currentExposure_;\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 DB8C7BF415\n\tfor ;\n\tThu, 25 Nov 2021 12:05:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F1F26036F;\n\tThu, 25 Nov 2021 13:05:26 +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 7CAA760231\n\tfor ;\n\tThu, 25 Nov 2021 13:05:24 +0100 (CET)","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 0DF9E90E;\n\tThu, 25 Nov 2021 13:05:23 +0100 (CET)"],"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=\"D0w/3A5k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637841924;\n\tbh=/TQ7u5nfAHIZgHQPCnDa2UGcZheCnYfabexqfsXj9+A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D0w/3A5kNc4qpX1KiCF0laevd6FLC0lZu8x98OuOeJeQ83k7zGG5qNThhUEvoDVRE\n\tQEiKDMfinbB5yiAgEW4rz0dF1n1IWbr8X+ynWmXaCb2DM42gIRjL17ZtoM4iwY5dsE\n\tWOHl+xCdaniJgjo1O+vvrR3SoQ9rM0vT/2XQOGHQ=","Date":"Thu, 25 Nov 2021 14:05:01 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211125102143.52556-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211125102143.52556-2-jeanmichel.hautbois@ideasonboard.com>\n\t<163784101086.3059017.17081389453082894176@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163784101086.3059017.17081389453082894176@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: ipu3: Return filtered 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\" "}}]