[{"id":39873,"web_url":"https://patchwork.libcamera.org/comment/39873/","msgid":"","date":"2026-07-27T13:54:42","subject":"Re: [RFC PATCH v2 22/43] ipa: libipa: agc: Clamp exposure value","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Jul 23, 2026 at 05:43:05PM +0200, Barnabás Pőcze wrote:\n> It is possible that the minimum exposure time in microseconds is not an\n> integer. In that case the minimum `ExposureTime` in the `ControlInfo` will\n> therefore be actually lower than the real minimum. This can cause issues:\n> if the minimum published exposure time is set, the division when setting\n> `agc.manual.exposure` might produce a value in [0;1), leading to the manual\n> exposure being set to 0, leading to an assertion failure in\n\nI'm not sure how can this go to 0 as the min exposure is usually some\nnumber of lines.\n\nBut I understand it's certainly possible to go below the minimum due\nto rounding errors\n\n> `ExposureModeHelper::splitExposure()`.\n>\n> Note that this is theoretically possible even when the exposure time is\n> determined automatically by `AgcMeanLuminance`.\n>\n> Store the true integer limits of the exposure as well, and ensure that\n> the exposure (time) -> exposure (line) conversions always clamp.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/ipa/libipa/agc.cpp | 27 ++++++++++++++++++++++++---\n> src/ipa/libipa/agc.h | 2 ++\n> 2 files changed, 26 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n> index e16a02fdde..8a2525ec98 100644\n> --- a/src/ipa/libipa/agc.cpp\n> +++ b/src/ipa/libipa/agc.cpp\n> @@ -38,6 +38,12 @@ LOG_DEFINE_CATEGORY(Agc)\n> * \\struct agc::Session\n> * \\brief Session configuration for AgcAlgorithm\n> *\n> + * \\var agc::Session::minExposure\n> + * \\brief Minimum exposure (in lines) supported with the configured sensor\n> + *\n> + * \\var agc::Session::maxExposure\n> + * \\brief Maximum exposure (in lines) supported with the configured sensor\n> + *\n> * \\var agc::Session::minExposureTime\n> * \\brief Minimum exposure time supported with the configured sensor\n> *\n> @@ -223,6 +229,19 @@ LOG_DEFINE_CATEGORY(Agc)\n> * \\brief Effective lux value of the frame\n> */\n>\n> +namespace {\n> +\n> +[[nodiscard]] uint32_t clampExposure(utils::Duration exposureTime, const agc::Session &session)\n> +{\n> +\treturn std::clamp(\n> +\t\texposureTime / session.lineDuration,\n> +\t\tsession.minExposure,\n> +\t\tsession.maxExposure\n> +\t);\n> +}\n> +\n> +} /* namespace */\n> +\n> /**\n> * \\brief Load tuning data\n> */\n> @@ -317,6 +336,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons\n> \t *\n> \t * \\todo take VBLANK into account for maximum exposure time\n> \t */\n> +\tsession.minExposure = minExposure;\n> +\tsession.maxExposure = maxExposure;\n> \tsession.minExposureTime = minExposure * session.lineDuration;\n> \tsession.maxExposureTime = maxExposure * session.lineDuration;\n> \tsession.minAnalogueGain = minGain;\n> @@ -331,7 +352,7 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons\n> \t/* Configure the default exposure and gain. */\n> \tstate = {};\n> \tstate.automatic.gain = session.minAnalogueGain;\n> -\tstate.automatic.exposure = 10ms / session.lineDuration;\n> +\tstate.automatic.exposure = clampExposure(10ms, session);\n> \tstate.automatic.quantizationGain = 1;\n> \tstate.automatic.yTarget = impl_.effectiveYTarget(0, 1);\n> \tstate.manual.gain = state.automatic.gain;\n> @@ -437,7 +458,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s\n>\n> \tconst auto &exposure = controls.get(controls::ExposureTime);\n> \tif (exposure && !state.autoExposureEnabled) {\n> -\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n> +\t\tstate.manual.exposure = clampExposure(*exposure * 1us, session);\n>\n> \t\tLOG(Agc, Debug)\n> \t\t\t<< \"Set exposure to \" << state.manual.exposure;\n> @@ -599,7 +620,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n> \t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n>\n> \t\t/* Update the estimated exposure and gain. */\n> -\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n> +\t\tstate.automatic.exposure = clampExposure(newEv.exposureTime, session);\n> \t\tstate.automatic.gain = newEv.analogueGain;\n> \t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n> \t\tstate.automatic.yTarget = newEv.yTarget;\n> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n> index 1eace12908..c11dbf80cd 100644\n> --- a/src/ipa/libipa/agc.h\n> +++ b/src/ipa/libipa/agc.h\n> @@ -49,6 +49,8 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n> }\n>\n> struct Session {\n> +\tuint32_t minExposure;\n> +\tuint32_t maxExposure;\n> \tutils::Duration minExposureTime;\n> \tutils::Duration maxExposureTime;\n> \tdouble minAnalogueGain;\n\nReviewed-by: Jacopo Mondi \n\n> --\n> 2.55.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 50731BDE4C\n\tfor ;\n\tMon, 27 Jul 2026 13:54:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A2CE67F76;\n\tMon, 27 Jul 2026 15:54:47 +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 6827367EB2\n\tfor ;\n\tMon, 27 Jul 2026 15:54:45 +0200 (CEST)","from ideasonboard.com (mob-5-90-50-102.net.vodafone.it\n\t[5.90.50.102])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E023492;\n\tMon, 27 Jul 2026 15:53:41 +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=\"CFbC2/4h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785160421;\n\tbh=2VJHzC7SZYifbPeOVdBKWPUNDHIFpSRLTfgw37agEmw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CFbC2/4hyAVjGN7HebNd3+GJGX/9qTGL/YpiwH+CYbMcoSQJpAu+a5bQ3begq1I+1\n\tthyXBXGGNgmpC+0FrbdJPtUrknpc75mGQiECGawUkp8sno/W4yP3ZHePwoAcCXddl1\n\tUkhIBxLuccjw2oCky44IBnzJaYu6UcuHWvDvJhFk=","Date":"Mon, 27 Jul 2026 15:54:42 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 22/43] ipa: libipa: agc: Clamp exposure value","Message-ID":"","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-23-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260723154327.1357866-23-barnabas.pocze@ideasonboard.com>","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":39882,"web_url":"https://patchwork.libcamera.org/comment/39882/","msgid":"","date":"2026-07-27T14:49:06","subject":"Re: [RFC PATCH v2 22/43] ipa: libipa: agc: Clamp exposure value","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 07. 27. 15:54 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Thu, Jul 23, 2026 at 05:43:05PM +0200, Barnabás Pőcze wrote:\n>> It is possible that the minimum exposure time in microseconds is not an\n>> integer. In that case the minimum `ExposureTime` in the `ControlInfo` will\n>> therefore be actually lower than the real minimum. This can cause issues:\n>> if the minimum published exposure time is set, the division when setting\n>> `agc.manual.exposure` might produce a value in [0;1), leading to the manual\n>> exposure being set to 0, leading to an assertion failure in\n> \n> I'm not sure how can this go to 0 as the min exposure is usually some\n> number of lines.\n\nThe \"problematic part\" is\n\n state.manual.exposure = *exposure * 1.0us / session.lineDuration;\n\nFirstly, if the application does not obey the `ExposureTime` limits,\nit can set it to 0us, trivially leading to the issue.\n\nHowever, even if it adheres to the limits, the issues is that the minimum\nexposure time is determined by division, and then float->integer truncation,\nessentially leading to rounding down.\n\nSo it could be that the true min exposure time is 13.36us, but that will\ngo into the `ControlInfo` as 13. So setting an exposure of 13:\n\n (*exposure * 1.0us / session.lineDuration) == 0.X\n\nand then due to the truncation causes `state.manual.exposure == 0`.\n\nSo rounding-up for the minimum seems like a sensible choice in any case,\nbut that's not implemented here.\n\nAnd maybe the `clampGain()` below should do proper rounding before the clamping.\n\n> \n> But I understand it's certainly possible to go below the minimum due\n> to rounding errors\n> \n>> `ExposureModeHelper::splitExposure()`.\n>>\n>> Note that this is theoretically possible even when the exposure time is\n>> determined automatically by `AgcMeanLuminance`.\n>>\n>> Store the true integer limits of the exposure as well, and ensure that\n>> the exposure (time) -> exposure (line) conversions always clamp.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/ipa/libipa/agc.cpp | 27 ++++++++++++++++++++++++---\n>> src/ipa/libipa/agc.h | 2 ++\n>> 2 files changed, 26 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp\n>> index e16a02fdde..8a2525ec98 100644\n>> --- a/src/ipa/libipa/agc.cpp\n>> +++ b/src/ipa/libipa/agc.cpp\n>> @@ -38,6 +38,12 @@ LOG_DEFINE_CATEGORY(Agc)\n>> * \\struct agc::Session\n>> * \\brief Session configuration for AgcAlgorithm\n>> *\n>> + * \\var agc::Session::minExposure\n>> + * \\brief Minimum exposure (in lines) supported with the configured sensor\n>> + *\n>> + * \\var agc::Session::maxExposure\n>> + * \\brief Maximum exposure (in lines) supported with the configured sensor\n>> + *\n>> * \\var agc::Session::minExposureTime\n>> * \\brief Minimum exposure time supported with the configured sensor\n>> *\n>> @@ -223,6 +229,19 @@ LOG_DEFINE_CATEGORY(Agc)\n>> * \\brief Effective lux value of the frame\n>> */\n>>\n>> +namespace {\n>> +\n>> +[[nodiscard]] uint32_t clampExposure(utils::Duration exposureTime, const agc::Session &session)\n>> +{\n>> +\treturn std::clamp(\n>> +\t\texposureTime / session.lineDuration,\n>> +\t\tsession.minExposure,\n>> +\t\tsession.maxExposure\n>> +\t);\n>> +}\n>> +\n>> +} /* namespace */\n>> +\n>> /**\n>> * \\brief Load tuning data\n>> */\n>> @@ -317,6 +336,8 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons\n>> \t *\n>> \t * \\todo take VBLANK into account for maximum exposure time\n>> \t */\n>> +\tsession.minExposure = minExposure;\n>> +\tsession.maxExposure = maxExposure;\n>> \tsession.minExposureTime = minExposure * session.lineDuration;\n>> \tsession.maxExposureTime = maxExposure * session.lineDuration;\n>> \tsession.minAnalogueGain = minGain;\n>> @@ -331,7 +352,7 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, cons\n>> \t/* Configure the default exposure and gain. */\n>> \tstate = {};\n>> \tstate.automatic.gain = session.minAnalogueGain;\n>> -\tstate.automatic.exposure = 10ms / session.lineDuration;\n>> +\tstate.automatic.exposure = clampExposure(10ms, session);\n>> \tstate.automatic.quantizationGain = 1;\n>> \tstate.automatic.yTarget = impl_.effectiveYTarget(0, 1);\n>> \tstate.manual.gain = state.automatic.gain;\n>> @@ -437,7 +458,7 @@ void AgcAlgorithm::queueRequest(const agc::Session &session, agc::ActiveState &s\n>>\n>> \tconst auto &exposure = controls.get(controls::ExposureTime);\n>> \tif (exposure && !state.autoExposureEnabled) {\n>> -\t\tstate.manual.exposure = *exposure * 1.0us / session.lineDuration;\n>> +\t\tstate.manual.exposure = clampExposure(*exposure * 1us, session);\n>>\n>> \t\tLOG(Agc, Debug)\n>> \t\t\t<< \"Set exposure to \" << state.manual.exposure;\n>> @@ -599,7 +620,7 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,\n>> \t\t\t<< \", \" << newEv.quantizationGain << \" and \" << newEv.digitalGain;\n>>\n>> \t\t/* Update the estimated exposure and gain. */\n>> -\t\tstate.automatic.exposure = newEv.exposureTime / lineDuration;\n>> +\t\tstate.automatic.exposure = clampExposure(newEv.exposureTime, session);\n>> \t\tstate.automatic.gain = newEv.analogueGain;\n>> \t\tstate.automatic.quantizationGain = newEv.quantizationGain;\n>> \t\tstate.automatic.yTarget = newEv.yTarget;\n>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h\n>> index 1eace12908..c11dbf80cd 100644\n>> --- a/src/ipa/libipa/agc.h\n>> +++ b/src/ipa/libipa/agc.h\n>> @@ -49,6 +49,8 @@ prepareControls(ControlList &controls, const CameraSensorHelper *sensor,\n>> }\n>>\n>> struct Session {\n>> +\tuint32_t minExposure;\n>> +\tuint32_t maxExposure;\n>> \tutils::Duration minExposureTime;\n>> \tutils::Duration maxExposureTime;\n>> \tdouble minAnalogueGain;\n> \n> Reviewed-by: Jacopo Mondi \n> \n>> --\n>> 2.55.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 92353BE080\n\tfor ;\n\tMon, 27 Jul 2026 14:49:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6EA6E67F89;\n\tMon, 27 Jul 2026 16:49:11 +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 A91AE67EB2\n\tfor ;\n\tMon, 27 Jul 2026 16:49:09 +0200 (CEST)","from [192.168.33.46] (185.182.215.156.nat.pool.zt.hu\n\t[185.182.215.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BF212492;\n\tMon, 27 Jul 2026 16:48:05 +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=\"pkK3A64Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785163685;\n\tbh=MMbAXIyhbHv9Y9vnTCjiuyH5w9/kvbb3o2eURp/khD0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=pkK3A64Qoli4wS6ca573aycGyw+RG1xatozIb7dhIc7KVTrfQMUEf92rP6wMp9iN+\n\t58cFkAb33wexwcwSqJFqpXmyTQvy1WTGnbrOMZvrqZ2L8U/Gk+h0T4qn2WfiPRLY2K\n\t9DBaYNv7BYkOGjQ9fNr76MsA0XAD43lPWmahQIQA=","Message-ID":"","Date":"Mon, 27 Jul 2026 16:49:06 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 22/43] ipa: libipa: agc: Clamp exposure value","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-23-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]