[{"id":20830,"web_url":"https://patchwork.libcamera.org/comment/20830/","msgid":"<163658360325.2121661.1593649466684710043@Monstersaurus>","date":"2021-11-10T22:33:23","subject":"Re: [libcamera-devel] [PATCH v2 08/14] ipa: ipu3: agc: Update\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-11-10 19:58:55)\n> Previously, the exposure value was calculated based on the estimated\n> shutter time and gain applied. Now that we have the real values for the\n> current frame, use those before estimating the next one and rename the\n> variable accordingly.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 32 +++++++++++---------------------\n> src/ipa/ipu3/algorithms/agc.h | 2 +-\n> 2 files changed, 12 insertions(+), 22 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index ee37a9d5..38667e61 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -73,7 +73,7 @@ static constexpr uint32_t kMaxLuminance = 255;\n> Agc::Agc()\n> : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),\n> maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n> - prevExposureValue_(0s)\n> + effectiveExposureValue_(0s)\n> {\n> }\n> \n> @@ -104,9 +104,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> context.frameContext.agc.gain = minAnalogueGain_;\n> context.frameContext.agc.exposure = minExposureLines_;\n> \n> - prevExposureValue_ = context.frameContext.agc.gain\n> - * context.frameContext.agc.exposure\n> - * lineDuration_;\n> + effectiveExposureValue_ = context.frameContext.agc.gain\n\nAhhh nice. 'Effective' sounds so much clearer than 'Previous' in this\ncontext.\n\n> + * context.frameContext.agc.exposure\n> + * lineDuration_;\n> \n> return 0;\n> }\n> @@ -201,16 +201,16 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\n> */\n> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> - if (std::abs(evGain - 1.0) < 0.01) {\n> - LOG(IPU3Agc, Debug) << \"We are well exposed (iqMean = \"\n> - << iqMean_ << \")\";\n> - return;\n> - }\n\nIs this intentional? I thought we only just added this in this series?\n\n> -\n> /* extracted from Rpi::Agc::computeTargetExposure */\n> \n> /* Calculate the shutter time in seconds */\n> utils::Duration currentShutter = exposure * lineDuration_;\n\nI'd have an empty line here.\n\n> + /*\n> + * Update the exposure value for the next computation using the values\n> + * of exposure and gain really used by the sensor.\n> + */\n> + effectiveExposureValue_ = currentShutter * analogueGain;\n> +\n> LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentShutter * analogueGain\n> << \" Shutter speed \" << currentShutter\n> << \" Gain \" << analogueGain\n> @@ -228,7 +228,7 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\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_ = prevExposureValue_ * evGain;\n> + currentExposure_ = effectiveExposureValue_ * evGain;\n> utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;\n> utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;\n> \n> @@ -259,16 +259,6 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\n> \n> exposure = shutterTime / lineDuration_;\n> analogueGain = stepGain;\n> -\n> - /*\n> - * Update the exposure value for the next process call.\n> - *\n> - * \\todo Obtain the values of the exposure time and analog gain\n> - * that were actually used by the sensor, either from embedded\n> - * data when available, or from the delayed controls\n> - * infrastructure in case a slow down caused a mismatch.\n> - */\n> - prevExposureValue_ = shutterTime * analogueGain;\n> }\n> \n> /**\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 0a9152a9..51174639 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -54,7 +54,7 @@ private:\n> \n> utils::Duration filteredExposure_;\n> utils::Duration currentExposure_;\n> - utils::Duration prevExposureValue_;\n> + utils::Duration effectiveExposureValue_;\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 EC613BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 22:33:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43F6A60361;\n\tWed, 10 Nov 2021 23:33:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 42EF76033C\n\tfor ;\n\tWed, 10 Nov 2021 23:33:26 +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 C757E8B6;\n\tWed, 10 Nov 2021 23:33:25 +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=\"tMJEotle\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636583606;\n\tbh=WCuj7IGF8xOmlMffmtH9SQwZUO8Edk9tIwSnbwhJhU8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=tMJEotleMZcTcWUS9cpU0sDZQgzdY7DtJdA6430xpN/BCBPyxnxwK+cSvGMrlFwQY\n\tyKCdPzUkkr73SKzSVPwd/g0+wkW5ap6g4zMCUWRSKWXxNSBa3ujIngaSN4bfdFmxzF\n\tDCDdulc1vKGILLaZ09RugY6rHb6E4m0BVTpoeBR4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211110195901.85597-9-jeanmichel.hautbois@ideasonboard.com>","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-9-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 10 Nov 2021 22:33:23 +0000","Message-ID":"<163658360325.2121661.1593649466684710043@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 08/14] ipa: ipu3: agc: Update\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":20851,"web_url":"https://patchwork.libcamera.org/comment/20851/","msgid":"","date":"2021-11-11T10:16:49","subject":"Re: [libcamera-devel] [PATCH v2 08/14] ipa: ipu3: agc: Update\n\tprevious exposure value","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 10/11/2021 23:33, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-11-10 19:58:55)\n>> Previously, the exposure value was calculated based on the estimated\n>> shutter time and gain applied. Now that we have the real values for the\n>> current frame, use those before estimating the next one and rename the\n>> variable accordingly.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 32 +++++++++++---------------------\n>> src/ipa/ipu3/algorithms/agc.h | 2 +-\n>> 2 files changed, 12 insertions(+), 22 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index ee37a9d5..38667e61 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -73,7 +73,7 @@ static constexpr uint32_t kMaxLuminance = 255;\n>> Agc::Agc()\n>> : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),\n>> maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),\n>> - prevExposureValue_(0s)\n>> + effectiveExposureValue_(0s)\n>> {\n>> }\n>> \n>> @@ -104,9 +104,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>> context.frameContext.agc.gain = minAnalogueGain_;\n>> context.frameContext.agc.exposure = minExposureLines_;\n>> \n>> - prevExposureValue_ = context.frameContext.agc.gain\n>> - * context.frameContext.agc.exposure\n>> - * lineDuration_;\n>> + effectiveExposureValue_ = context.frameContext.agc.gain\n> \n> Ahhh nice. 'Effective' sounds so much clearer than 'Previous' in this\n> context.\n> \n>> + * context.frameContext.agc.exposure\n>> + * lineDuration_;\n>> \n>> return 0;\n>> }\n>> @@ -201,16 +201,16 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\n>> */\n>> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n>> \n>> - if (std::abs(evGain - 1.0) < 0.01) {\n>> - LOG(IPU3Agc, Debug) << \"We are well exposed (iqMean = \"\n>> - << iqMean_ << \")\";\n>> - return;\n>> - }\n> \n> Is this intentional? I thought we only just added this in this series?\n\nIt is not :-/\n\n> \n>> -\n>> /* extracted from Rpi::Agc::computeTargetExposure */\n>> \n>> /* Calculate the shutter time in seconds */\n>> utils::Duration currentShutter = exposure * lineDuration_;\n> \n> I'd have an empty line here.\n> \n>> + /*\n>> + * Update the exposure value for the next computation using the values\n>> + * of exposure and gain really used by the sensor.\n>> + */\n>> + effectiveExposureValue_ = currentShutter * analogueGain;\n>> +\n>> LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentShutter * analogueGain\n>> << \" Shutter speed \" << currentShutter\n>> << \" Gain \" << analogueGain\n>> @@ -228,7 +228,7 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\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_ = prevExposureValue_ * evGain;\n>> + currentExposure_ = effectiveExposureValue_ * evGain;\n>> utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;\n>> utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;\n>> \n>> @@ -259,16 +259,6 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\n>> \n>> exposure = shutterTime / lineDuration_;\n>> analogueGain = stepGain;\n>> -\n>> - /*\n>> - * Update the exposure value for the next process call.\n>> - *\n>> - * \\todo Obtain the values of the exposure time and analog gain\n>> - * that were actually used by the sensor, either from embedded\n>> - * data when available, or from the delayed controls\n>> - * infrastructure in case a slow down caused a mismatch.\n>> - */\n>> - prevExposureValue_ = shutterTime * analogueGain;\n>> }\n>> \n>> /**\n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index 0a9152a9..51174639 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -54,7 +54,7 @@ private:\n>> \n>> utils::Duration filteredExposure_;\n>> utils::Duration currentExposure_;\n>> - utils::Duration prevExposureValue_;\n>> + utils::Duration effectiveExposureValue_;\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 83D66BDB1C\n\tfor ;\n\tThu, 11 Nov 2021 10:16:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE3696035A;\n\tThu, 11 Nov 2021 11:16:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9A0F6032C\n\tfor ;\n\tThu, 11 Nov 2021 11:16:51 +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 8899FE51;\n\tThu, 11 Nov 2021 11:16:51 +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=\"XV/p4vad\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636625811;\n\tbh=RnL8bqik+jz0F6cBYUYON0VYMSXFboKJFIKvc1ESIOk=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=XV/p4vadr1Nu5Z0LtNpFzCPHan5PBkzgkchD8PRgdaWSUvxNNbXEadIiOfmwlCchl\n\tDqVJaHsDr6DX2uIDBtTBoxdVtGsMWEdac6WjxKrw/2H110dyN7ojlHWTUHBHOab3ef\n\tC0nkQXjdQpFmZS72x24S77u5YVr5wthy9SLXPB/8=","Message-ID":"","Date":"Thu, 11 Nov 2021 11:16:49 +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-9-jeanmichel.hautbois@ideasonboard.com>\n\t<163658360325.2121661.1593649466684710043@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163658360325.2121661.1593649466684710043@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 08/14] ipa: ipu3: agc: Update\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\" "}}]