[{"id":20199,"web_url":"https://patchwork.libcamera.org/comment/20199/","msgid":"<163420918769.3829429.14886583337927608496@Monstersaurus>","date":"2021-10-14T10:59:47","subject":"Re: [libcamera-devel] [PATCH 06/13] ipa: ipu3: agc: Change exposure\n\tlimits","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:18)\n> The exposure limits are set for one sensor until now, in a number of\n> lines. We should not use those, but exposure limits in a time unit.\n> \n> Introduce default limits which with a default minimum of 20ms. This\n> value should be given by the IPAIPU3. Use cached values expressed as a\n> number of lines in the configure() call.\n> \n> Adapt the process() call accordingly.\n\nReviewed-by: Kieran Bingham \n \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 30 +++++++++++++++++-------------\n> src/ipa/ipu3/algorithms/agc.h | 3 ++-\n> 2 files changed, 19 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 7c2d4201..2dd5c5c1 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -40,8 +40,8 @@ static constexpr uint32_t kMinGain = kMinISO / 100;\n> static constexpr uint32_t kMaxGain = kMaxISO / 100;\n> \n> /* \\todo use calculated value based on sensor */\n> -static constexpr uint32_t kMinExposure = 1;\n> -static constexpr uint32_t kMaxExposure = 1976;\n> +static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us;\n> +static constexpr libcamera::utils::Duration kMaxShutterSpeed = 20ms;\n\nI guess these limits impose restrictions on long exposure support somehow?\nBut we don't need to worry about that for now.\n\n> \n> /* Histogram constants */\n> static constexpr uint32_t knumHistogramBins = 256;\n> @@ -49,8 +49,8 @@ static constexpr double kEvGainTarget = 0.5;\n> \n> Agc::Agc()\n> : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> - maxExposureTime_(0s), filteredExposure_(0s), filteredExposureNoDg_(0s),\n> - currentExposure_(0s), currentExposureNoDg_(0s)\n> + minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n> + filteredExposureNoDg_(0s), currentExposure_(0s), currentExposureNoDg_(0s)\n> {\n> }\n> \n> @@ -60,7 +60,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> \n> lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> / configInfo.sensorInfo.pixelRate;\n> - maxExposureTime_ = kMaxExposure * lineDuration_;\n> +\n> + minExposureLines_ = kMinShutterSpeed / lineDuration_;\n> + maxExposureLines_ = kMaxShutterSpeed / lineDuration_;\n> \n> return 0;\n> }\n> @@ -140,28 +142,30 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> double newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> /* extracted from Rpi::Agc::computeTargetExposure */\n> - libcamera::utils::Duration currentShutter = exposure * lineDuration_;\n> + Duration currentShutter = exposure * lineDuration_;\n> currentExposureNoDg_ = currentShutter * gain;\n> LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> << \" Shutter speed \" << currentShutter\n> << \" Gain \" << gain;\n> +\n> currentExposure_ = currentExposureNoDg_ * newGain;\n> - libcamera::utils::Duration maxTotalExposure = maxExposureTime_ * kMaxGain;\n> + Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> - LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\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> - libcamera::utils::Duration newExposure = 0.0s;\n> - if (currentShutter < maxExposureTime_) {\n> - exposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n> + Duration newExposure = 0.0s;\n> + if (currentShutter < kMaxShutterSpeed) {\n> + exposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_);\n> newExposure = currentExposure_ / exposure;\n> gain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n> - } else if (currentShutter >= maxExposureTime_) {\n> + } else if (currentShutter >= kMaxShutterSpeed) {\n> gain = std::clamp(static_cast(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);\n> newExposure = currentExposure_ / gain;\n> - exposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);\n> + exposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_);\n> }\n> LOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n> }\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index cd4d4855..7605ab39 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -44,7 +44,8 @@ private:\n> double iqMean_;\n> \n> Duration lineDuration_;\n> - Duration maxExposureTime_;\n> + uint32_t minExposureLines_;\n> + uint32_t maxExposureLines_;\n> \n> Duration filteredExposure_;\n> Duration filteredExposureNoDg_;\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 41863C323E\n\tfor ;\n\tThu, 14 Oct 2021 10:59:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C826968F4D;\n\tThu, 14 Oct 2021 12:59:51 +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 3B0D368541\n\tfor ;\n\tThu, 14 Oct 2021 12:59:50 +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 C35972F3;\n\tThu, 14 Oct 2021 12:59:49 +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=\"VzkOTROa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634209189;\n\tbh=HQilRVY19WqG0UbU5EQHuVhRZjuDWIpsVJOpHnB3F6c=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=VzkOTROaAFgnGMnAP4dxcupAIGi3RSDysJTEa3ThF0dHWk5feH7hRavNdwCsfuG+k\n\tNlfo8DMJErsyDFmhn37zTI95JYITEotuKS7J3SY7Qi/hAwxy0TLzRELm64IS2iHZ0e\n\tcVT8QJ4gdTW+Qem0bLKpT6rDYZymUwSXcLefVUvA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211013154125.133419-7-jeanmichel.hautbois@ideasonboard.com>","References":"<20211013154125.133419-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211013154125.133419-7-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 14 Oct 2021 11:59:47 +0100","Message-ID":"<163420918769.3829429.14886583337927608496@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 06/13] ipa: ipu3: agc: Change exposure\n\tlimits","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":20216,"web_url":"https://patchwork.libcamera.org/comment/20216/","msgid":"","date":"2021-10-15T00:34:48","subject":"Re: [libcamera-devel] [PATCH 06/13] ipa: ipu3: agc: Change exposure\n\tlimits","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\n(CC'ing David for a question below)\n\nThank you for the patch.\n\nOn Wed, Oct 13, 2021 at 05:41:18PM +0200, Jean-Michel Hautbois wrote:\n> The exposure limits are set for one sensor until now, in a number of\n> lines. We should not use those, but exposure limits in a time unit.\n> \n> Introduce default limits which with a default minimum of 20ms. This\n\nThat's a default maximum, right ?\n\nIsn't 20ms a bit low ?\n\n> value should be given by the IPAIPU3. Use cached values expressed as a\n> number of lines in the configure() call.\n> \n> Adapt the process() call accordingly.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 30 +++++++++++++++++-------------\n> src/ipa/ipu3/algorithms/agc.h | 3 ++-\n> 2 files changed, 19 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 7c2d4201..2dd5c5c1 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -40,8 +40,8 @@ static constexpr uint32_t kMinGain = kMinISO / 100;\n> static constexpr uint32_t kMaxGain = kMaxISO / 100;\n> \n> /* \\todo use calculated value based on sensor */\n> -static constexpr uint32_t kMinExposure = 1;\n> -static constexpr uint32_t kMaxExposure = 1976;\n> +static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us;\n> +static constexpr libcamera::utils::Duration kMaxShutterSpeed = 20ms;\n\nI'd like to rename shutter speed to integration time (in a separate\npatch). What do you think ? \"exposure time\" may also be a candidate, but\nit is often shortened to \"exposure\" which is then easily confused with\nthe photographic exposure.\n\n> /* Histogram constants */\n> static constexpr uint32_t knumHistogramBins = 256;\n> @@ -49,8 +49,8 @@ static constexpr double kEvGainTarget = 0.5;\n> \n> Agc::Agc()\n> \t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> -\t maxExposureTime_(0s), filteredExposure_(0s), filteredExposureNoDg_(0s),\n> -\t currentExposure_(0s), currentExposureNoDg_(0s)\n> +\t minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n> +\t filteredExposureNoDg_(0s), currentExposure_(0s), currentExposureNoDg_(0s)\n> {\n> }\n> \n> @@ -60,7 +60,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> \n> \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> \t\t / configInfo.sensorInfo.pixelRate;\n> -\tmaxExposureTime_ = kMaxExposure * lineDuration_;\n\nRemoving the maxExposureTime_ member variable is fine as it's currently\na constant, but that won't always be the case. Shouldn't we keep it to\nprepare for calculation of the maximum exposure time from the sensor's\nlimits ?\n\n> +\n> +\tminExposureLines_ = kMinShutterSpeed / lineDuration_;\n> +\tmaxExposureLines_ = kMaxShutterSpeed / lineDuration_;\n\nI wonder if this goes in the right direction. Wouldn't it be better for\nthe algorithm to operate on a time, with conversion to/from lines being\nhandled outside (in ipu3.cpp, or possibly even in the pipeline handler)\n? David, what's your opinion on this ?\n\n> \n> \treturn 0;\n> }\n> @@ -140,28 +142,30 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> \t\t/* extracted from Rpi::Agc::computeTargetExposure */\n> -\t\tlibcamera::utils::Duration currentShutter = exposure * lineDuration_;\n> +\t\tDuration currentShutter = exposure * lineDuration_;\n\nThis compiles because there's a \"using utils::Duration\" in agc.h. This\nis harmful, I'll send a patch to drop it, so you will need\nutils::duration here (the libcamera:: prefix can be dropped as we're in\nthe libcamera namespace).\n\n> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> \t\t\t\t << \" Shutter speed \" << currentShutter\n> \t\t\t\t << \" Gain \" << gain;\n> +\n> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n> -\t\tlibcamera::utils::Duration maxTotalExposure = maxExposureTime_ * kMaxGain;\n> +\t\tDuration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n> \t\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> -\t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n> +\t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> +\t\t\t\t << \" maximum is \" << maxTotalExposure;\n\n\t\t\t\t << \", maximum is \" << maxTotalExposure;\n\n> \n> \t\t/* \\todo: estimate if we need to desaturate */\n> \t\tfilterExposure();\n> \n> -\t\tlibcamera::utils::Duration newExposure = 0.0s;\n> -\t\tif (currentShutter < maxExposureTime_) {\n> -\t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n> +\t\tDuration newExposure = 0.0s;\n> +\t\tif (currentShutter < kMaxShutterSpeed) {\n> +\t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_);\n\nThis is a very good occasion to wrap lines:\n\n\t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_),\n\t\t\t\t\t minExposureLines_, maxExposureLines_);\n\n> \t\t\tnewExposure = currentExposure_ / exposure;\n> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n> -\t\t} else if (currentShutter >= maxExposureTime_) {\n> +\t\t} else if (currentShutter >= kMaxShutterSpeed) {\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\texposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_);\n\nSame here.\n\n> \t\t}\n> \t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n> \t}\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index cd4d4855..7605ab39 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -44,7 +44,8 @@ private:\n> \tdouble iqMean_;\n> \n> \tDuration lineDuration_;\n> -\tDuration maxExposureTime_;\n> +\tuint32_t minExposureLines_;\n> +\tuint32_t maxExposureLines_;\n> \n> \tDuration filteredExposure_;\n> \tDuration filteredExposureNoDg_;","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 0D268C323E\n\tfor ;\n\tFri, 15 Oct 2021 00:35:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9047868F4E;\n\tFri, 15 Oct 2021 02:35:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E479968F4D\n\tfor ;\n\tFri, 15 Oct 2021 02:35:04 +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 4918A268;\n\tFri, 15 Oct 2021 02:35:04 +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=\"PNi5TMWG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634258104;\n\tbh=HgS3+5gQ4yPd2TTVoNfXy8C2WKaxoHQmX5j1+aJI3CQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PNi5TMWGSgrruKUu1EVbumRIBrb5mce6c3xoysZucXNk+QjZ4Kx6RPhvaSCA5NG77\n\tXtsCNVdHYAIITkoH4qQRLJMt0I/TWmKNpH3cBYmOFL/uUSgbmd1/F1YiWxpdHD43mz\n\tAYCRK4x6PxI7+4paoRZMu/hKprovlSzGIZJysJvw=","Date":"Fri, 15 Oct 2021 03:34:48 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ,\n\tDavid Plowman ","Message-ID":"","References":"<20211013154125.133419-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211013154125.133419-7-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211013154125.133419-7-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 06/13] ipa: ipu3: agc: Change exposure\n\tlimits","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\" "}},{"id":20233,"web_url":"https://patchwork.libcamera.org/comment/20233/","msgid":"","date":"2021-10-15T05:49:45","subject":"Re: [libcamera-devel] [PATCH 06/13] ipa: ipu3: agc: Change exposure\n\tlimits","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 15/10/2021 02:34, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> (CC'ing David for a question below)\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 13, 2021 at 05:41:18PM +0200, Jean-Michel Hautbois wrote:\n>> The exposure limits are set for one sensor until now, in a number of\n>> lines. We should not use those, but exposure limits in a time unit.\n>>\n>> Introduce default limits which with a default minimum of 20ms. This\n> \n> That's a default maximum, right ?\n> \n> Isn't 20ms a bit low ?\n> \n\nIt is not meant to stay like that, the value should come from IPAIPU3\nbased on what the sensor can do, maybe with a local maximum of the\nmaximum if we want (60ms sounds better). It also means we need to change\nexposure and VBLANK in the setControls() call. For that, we need to pass\na full exposure time and not only the exposure in a number of lines.\n\n>> value should be given by the IPAIPU3. Use cached values expressed as a\n>> number of lines in the configure() call.\n>>\n>> Adapt the process() call accordingly.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 30 +++++++++++++++++-------------\n>> src/ipa/ipu3/algorithms/agc.h | 3 ++-\n>> 2 files changed, 19 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 7c2d4201..2dd5c5c1 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -40,8 +40,8 @@ static constexpr uint32_t kMinGain = kMinISO / 100;\n>> static constexpr uint32_t kMaxGain = kMaxISO / 100;\n>> \n>> /* \\todo use calculated value based on sensor */\n>> -static constexpr uint32_t kMinExposure = 1;\n>> -static constexpr uint32_t kMaxExposure = 1976;\n>> +static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us;\n>> +static constexpr libcamera::utils::Duration kMaxShutterSpeed = 20ms;\n> \n> I'd like to rename shutter speed to integration time (in a separate\n> patch). What do you think ? \"exposure time\" may also be a candidate, but\n> it is often shortened to \"exposure\" which is then easily confused with\n> the photographic exposure.\n> \n\nI hesitated, between the three. I think \"integration time\" is digital\nphotography term, while shutter speed is more general, but maybe David\nand others have a different feeling ?\n\n>> /* Histogram constants */\n>> static constexpr uint32_t knumHistogramBins = 256;\n>> @@ -49,8 +49,8 @@ static constexpr double kEvGainTarget = 0.5;\n>> \n>> Agc::Agc()\n>> \t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>> -\t maxExposureTime_(0s), filteredExposure_(0s), filteredExposureNoDg_(0s),\n>> -\t currentExposure_(0s), currentExposureNoDg_(0s)\n>> +\t minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),\n>> +\t filteredExposureNoDg_(0s), currentExposure_(0s), currentExposureNoDg_(0s)\n>> {\n>> }\n>> \n>> @@ -60,7 +60,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>> \n>> \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n>> \t\t / configInfo.sensorInfo.pixelRate;\n>> -\tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> \n> Removing the maxExposureTime_ member variable is fine as it's currently\n> a constant, but that won't always be the case. Shouldn't we keep it to\n> prepare for calculation of the maximum exposure time from the sensor's\n> limits ?\n> \n>> +\n>> +\tminExposureLines_ = kMinShutterSpeed / lineDuration_;\n>> +\tmaxExposureLines_ = kMaxShutterSpeed / lineDuration_;\n> \n> I wonder if this goes in the right direction. Wouldn't it be better for\n> the algorithm to operate on a time, with conversion to/from lines being\n> handled outside (in ipu3.cpp, or possibly even in the pipeline handler)\n> ? David, what's your opinion on this ?\n\nYes, we should do that. I could keep the previous maxExposureTime, and\nuse local variables in lockExposureGain() for the moment...\n\n> \n>> \n>> \treturn 0;\n>> }\n>> @@ -140,28 +142,30 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)\n>> \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n>> \n>> \t\t/* extracted from Rpi::Agc::computeTargetExposure */\n>> -\t\tlibcamera::utils::Duration currentShutter = exposure * lineDuration_;\n>> +\t\tDuration currentShutter = exposure * lineDuration_;\n> \n> This compiles because there's a \"using utils::Duration\" in agc.h. This\n> is harmful, I'll send a patch to drop it, so you will need\n> utils::duration here (the libcamera:: prefix can be dropped as we're in\n> the libcamera namespace).\n\nOK, thanks.\n\n> \n>> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n>> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n>> \t\t\t\t << \" Shutter speed \" << currentShutter\n>> \t\t\t\t << \" Gain \" << gain;\n>> +\n>> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n>> -\t\tlibcamera::utils::Duration maxTotalExposure = maxExposureTime_ * kMaxGain;\n>> +\t\tDuration maxTotalExposure = kMaxShutterSpeed * kMaxGain;\n>> \t\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n>> -\t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n>> +\t\tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n>> +\t\t\t\t << \" maximum is \" << maxTotalExposure;\n> \n> \t\t\t\t << \", maximum is \" << maxTotalExposure;\n> \n>> \n>> \t\t/* \\todo: estimate if we need to desaturate */\n>> \t\tfilterExposure();\n>> \n>> -\t\tlibcamera::utils::Duration newExposure = 0.0s;\n>> -\t\tif (currentShutter < maxExposureTime_) {\n>> -\t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n>> +\t\tDuration newExposure = 0.0s;\n>> +\t\tif (currentShutter < kMaxShutterSpeed) {\n>> +\t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_);\n> \n> This is a very good occasion to wrap lines:\n> \n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_),\n> \t\t\t\t\t minExposureLines_, maxExposureLines_);\n> \n>> \t\t\tnewExposure = currentExposure_ / exposure;\n>> \t\t\tgain = std::clamp(static_cast(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);\n>> -\t\t} else if (currentShutter >= maxExposureTime_) {\n>> +\t\t} else if (currentShutter >= kMaxShutterSpeed) {\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\texposure = std::clamp(static_cast(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_);\n> \n> Same here.\n> \n>> \t\t}\n>> \t\tLOG(IPU3Agc, Debug) << \"Adjust exposure \" << exposure * lineDuration_ << \" and gain \" << gain;\n>> \t}\n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index cd4d4855..7605ab39 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -44,7 +44,8 @@ private:\n>> \tdouble iqMean_;\n>> \n>> \tDuration lineDuration_;\n>> -\tDuration maxExposureTime_;\n>> +\tuint32_t minExposureLines_;\n>> +\tuint32_t maxExposureLines_;\n>> \n>> \tDuration filteredExposure_;\n>> \tDuration filteredExposureNoDg_;\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 D4E8CC324C\n\tfor ;\n\tFri, 15 Oct 2021 05:49:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 448CA68F52;\n\tFri, 15 Oct 2021 07:49:50 +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 2538B60239\n\tfor ;\n\tFri, 15 Oct 2021 07:49:48 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:ae2a:a11b:484b:c825])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A2FD92E3;\n\tFri, 15 Oct 2021 07:49:47 +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=\"g4cFDZsd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634276987;\n\tbh=DM7hJHkd23db9+LeZ+aDXlV/wUYusjmXmWBq1gS7UQs=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=g4cFDZsdFTjCt0MI1C/76Cq04yPRoY+G6AYEA6N/Uz0ixeOcP4fqbCL00IVxqIzCJ\n\tSFGd2PTDJQ/NxAhfObR2IrcAUACI+kiCPRORRXUqESNB4x8R9BoIsWZHh2axZaJaRF\n\tFogpTD/HQi7ZtN5ohIAci2F7Hp5rPN7TV8uDlPaA=","To":"Laurent Pinchart ,\n\tDavid Plowman ","References":"<20211013154125.133419-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211013154125.133419-7-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Fri, 15 Oct 2021 07:49:45 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 06/13] ipa: ipu3: agc: Change exposure\n\tlimits","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\" "}}]