[{"id":35340,"web_url":"https://patchwork.libcamera.org/comment/35340/","msgid":"<175492279590.1641235.15355460662043213684@ping.linuxembedded.co.uk>","date":"2025-08-11T14:33:15","subject":"Re: [PATCH v2 04/16] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-08-08 15:12:42)\n> In ExposureModeHelper::splitExposure() the quantization of exposure time\n> and gain is not taken into account. This can lead to visible flicker\n> when the quantization steps are too big. As a preparation to fixing\n> that, add a function to set the sensor line length and the current\n> sensor mode helper and extend the clampXXX functions to return the\n> quantization error.\n> \n> By default the exposure time quantization is assumed to be 1us and gain\n> is assumed to not be quantized at all.\n> \n> This patch doesn't introduce any functional changes.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/exposure_mode_helper.cpp | 49 ++++++++++++++++++++-----\n> src/ipa/libipa/exposure_mode_helper.h | 10 ++++-\n> 2 files changed, 48 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> index 0c1e99e31a47..d0a61908c966 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -70,18 +70,37 @@ namespace ipa {\n> * the runtime limits set through setLimits() instead.\n> */\n> ExposureModeHelper::ExposureModeHelper(const Span> stages)\n> + : lineLength_(1us), minExposureTime_(0us), maxExposureTime_(0us),\n> + minGain_(0), maxGain_(0), sensorHelper_(nullptr)\n> {\n> - minExposureTime_ = 0us;\n> - maxExposureTime_ = 0us;\n> - minGain_ = 0;\n> - maxGain_ = 0;\n> -\n> for (const auto &[s, g] : stages) {\n> exposureTimes_.push_back(s);\n> gains_.push_back(g);\n> }\n> }\n> \n> +/**\n> + * \\brief Configure sensor details\n> + * \\param[in] lineLength The current line length of the sensor\n> + * \\param[in] sensorHelper The sensor helper\n> + *\n> + * This function sets the line length and sensor helper. These are used in\n> + * splitExposure() to take the quantization of the exposure and gain into\n> + * account.\n> + *\n> + * When this is not called, is is assumed that exposure is in micro second\n> + * granularity and gain has no quantization at all.\n> + *\n> + * The caller has to ensure that sensorHelper is valid for the lifetime of\n> + * ExposureModeHelper or the next call to configure.\n> + */\n> +void ExposureModeHelper::configure(utils::Duration lineLength,\n> + const CameraSensorHelper *sensorHelper)\n> +{\n> + lineLength_ = lineLength;\n> + sensorHelper_ = sensorHelper;\n> +}\n> +\n> /**\n> * \\brief Set the exposure time and gain limits\n> * \\param[in] minExposureTime The minimum exposure time supported\n> @@ -108,14 +127,26 @@ void ExposureModeHelper::setLimits(utils::Duration minExposureTime,\n> maxGain_ = maxGain;\n> }\n> \n> -utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime) const\n> +utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime,\n> + double *quantizationGain) const\n> {\n> - return std::clamp(exposureTime, minExposureTime_, maxExposureTime_);\n> + utils::Duration clamped;\n> + utils::Duration exp;\n> +\n> + clamped = std::clamp(exposureTime, minExposureTime_, maxExposureTime_);\n> + exp = static_cast(clamped / lineLength_) * lineLength_;\n> + if (quantizationGain)\n> + *quantizationGain = clamped / exp;\n> +\n> + return exp;\n> }\n> \n> -double ExposureModeHelper::clampGain(double gain) const\n> +double ExposureModeHelper::clampGain(double gain, double *quantizationGain) const\n> {\n> - return std::clamp(gain, minGain_, maxGain_);\n> + double clamped = std::clamp(gain, minGain_, maxGain_);\n> + if (!sensorHelper_)\n> + return clamped;\n> + return sensorHelper_->quantizeGain(clamped, quantizationGain);\n> }\n> \n> /**\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> index c5be1b6703a8..8701fae9a26e 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -14,6 +14,8 @@\n> #include \n> #include \n> \n> +#include \"camera_sensor_helper.h\"\n> +\n> namespace libcamera {\n> \n> namespace ipa {\n> @@ -24,6 +26,7 @@ public:\n> ExposureModeHelper(const Span> stages);\n> ~ExposureModeHelper() = default;\n> \n> + void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> double minGain, double maxGain);\n> \n> @@ -36,16 +39,19 @@ public:\n> double maxGain() const { return maxGain_; }\n> \n> private:\n> - utils::Duration clampExposureTime(utils::Duration exposureTime) const;\n> - double clampGain(double gain) const;\n> + utils::Duration clampExposureTime(utils::Duration exposureTime,\n> + double *quantizationGain = nullptr) const;\n> + double clampGain(double gain, double *quantizationGain = nullptr) const;\n> \n> std::vector exposureTimes_;\n> std::vector gains_;\n> \n> + utils::Duration lineLength_;\n> utils::Duration minExposureTime_;\n> utils::Duration maxExposureTime_;\n> double minGain_;\n> double maxGain_;\n\n\nIt seems like we're storing this sensor configuration information in\nmultiple places. I've seen it recently also stored in the rkisp1 agc\ncontext data, which makes me wonder if there's something better to do\naround this topic.\n\nBut for now - This patch say's there's no functional change and it helps\ndevelopment so I'm fine with it.\n\n\nReviewed-by: Kieran Bingham \n\n\n> + const CameraSensorHelper *sensorHelper_;\n\n> };\n> \n> } /* namespace ipa */\n> -- \n> 2.48.1\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 296EDBEFBE\n\tfor ;\n\tMon, 11 Aug 2025 14:33:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 391EE69240;\n\tMon, 11 Aug 2025 16:33:20 +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 0EA7169233\n\tfor ;\n\tMon, 11 Aug 2025 16:33:19 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E307982A;\n\tMon, 11 Aug 2025 16:32:26 +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=\"EPKbspIo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754922747;\n\tbh=FuWsmleyw4oHJvMM0QuiI18z0QrJMI3IrthciII5lAo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=EPKbspIoKJJU+onTRMUX+J/XA3mhorc6tQWvi89zH+AI+6vFNA1SHODJAyz6nYlWu\n\tWakVoxQTCEmkXkBBc0qcOPPQ4iddQbzuyjNGi7pz9I0W5TXeX8hX1u0kaK35dGUsTb\n\t1w8o5dSy1RitBYlIXZkaanxt3LeOO9Jp0F11H4Uk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250808141315.413839-5-stefan.klug@ideasonboard.com>","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-5-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 04/16] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 11 Aug 2025 15:33:15 +0100","Message-ID":"<175492279590.1641235.15355460662043213684@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":35341,"web_url":"https://patchwork.libcamera.org/comment/35341/","msgid":"","date":"2025-08-11T14:36:24","subject":"Re: [PATCH v2 04/16] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 08/08/2025 15:12, Stefan Klug wrote:\n> In ExposureModeHelper::splitExposure() the quantization of exposure time\n> and gain is not taken into account. This can lead to visible flicker\n> when the quantization steps are too big. As a preparation to fixing\n> that, add a function to set the sensor line length and the current\n> sensor mode helper and extend the clampXXX functions to return the\n> quantization error.\n> \n> By default the exposure time quantization is assumed to be 1usand gain\n> is assumed to not be quantized at all.\n> \n> This patch doesn't introduce any functional changes.\n> \n> Signed-off-by: Stefan Klug \n\nReviewed-by: Daniel Scally \n\n> ---\n> src/ipa/libipa/exposure_mode_helper.cpp | 49 ++++++++++++++++++++-----\n> src/ipa/libipa/exposure_mode_helper.h | 10 ++++-\n> 2 files changed, 48 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> index 0c1e99e31a47..d0a61908c966 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -70,18 +70,37 @@ namespace ipa {\n> * the runtime limits set through setLimits() instead.\n> */\n> ExposureModeHelper::ExposureModeHelper(const Span> stages)\n> +\t: lineLength_(1us), minExposureTime_(0us), maxExposureTime_(0us),\n> +\t minGain_(0), maxGain_(0), sensorHelper_(nullptr)\n> {\n> -\tminExposureTime_ = 0us;\n> -\tmaxExposureTime_ = 0us;\n> -\tminGain_ = 0;\n> -\tmaxGain_ = 0;\n> -\n> \tfor (const auto &[s, g] : stages) {\n> \t\texposureTimes_.push_back(s);\n> \t\tgains_.push_back(g);\n> \t}\n> }\n> \n> +/**\n> + * \\brief Configure sensor details\n> + * \\param[in] lineLength The current line length of the sensor\n> + * \\param[in] sensorHelper The sensor helper\n> + *\n> + * This function sets the line length and sensor helper. These are used in\n> + * splitExposure() to take the quantization of the exposure and gain into\n> + * account.\n> + *\n> + * When this is not called, is is assumed that exposure is in micro second\n> + * granularity and gain has no quantization at all.\n> + *\n> + * The caller has to ensure that sensorHelper is valid for the lifetime of\n> + * ExposureModeHelper or the next call to configure.\n> + */\n> +void ExposureModeHelper::configure(utils::Duration lineLength,\n> +\t\t\t\t const CameraSensorHelper *sensorHelper)\n> +{\n> +\tlineLength_ = lineLength;\n> +\tsensorHelper_ = sensorHelper;\n> +}\n> +\n> /**\n> * \\brief Set the exposure time and gain limits\n> * \\param[in] minExposureTime The minimum exposure time supported\n> @@ -108,14 +127,26 @@ void ExposureModeHelper::setLimits(utils::Duration minExposureTime,\n> \tmaxGain_ = maxGain;\n> }\n> \n> -utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime) const\n> +utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime,\n> +\t\t\t\t\t\t double *quantizationGain) const\n> {\n> -\treturn std::clamp(exposureTime, minExposureTime_, maxExposureTime_);\n> +\tutils::Duration clamped;\n> +\tutils::Duration exp;\n> +\n> +\tclamped = std::clamp(exposureTime, minExposureTime_, maxExposureTime_);\n> +\texp = static_cast(clamped / lineLength_) * lineLength_;\n> +\tif (quantizationGain)\n> +\t\t*quantizationGain = clamped / exp;\n> +\n> +\treturn exp;\n> }\n> \n> -double ExposureModeHelper::clampGain(double gain) const\n> +double ExposureModeHelper::clampGain(double gain, double *quantizationGain) const\n> {\n> -\treturn std::clamp(gain, minGain_, maxGain_);\n> +\tdouble clamped = std::clamp(gain, minGain_, maxGain_);\n> +\tif (!sensorHelper_)\n> +\t\treturn clamped;\n> +\treturn sensorHelper_->quantizeGain(clamped, quantizationGain);\n> }\n> \n> /**\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> index c5be1b6703a8..8701fae9a26e 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -14,6 +14,8 @@\n> #include \n> #include \n> \n> +#include \"camera_sensor_helper.h\"\n> +\n> namespace libcamera {\n> \n> namespace ipa {\n> @@ -24,6 +26,7 @@ public:\n> \tExposureModeHelper(const Span> stages);\n> \t~ExposureModeHelper() = default;\n> \n> +\tvoid configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> \tvoid setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> \t\t double minGain, double maxGain);\n> \n> @@ -36,16 +39,19 @@ public:\n> \tdouble maxGain() const { return maxGain_; }\n> \n> private:\n> -\tutils::Duration clampExposureTime(utils::Duration exposureTime) const;\n> -\tdouble clampGain(double gain) const;\n> +\tutils::Duration clampExposureTime(utils::Duration exposureTime,\n> +\t\t\t\t\t double *quantizationGain = nullptr) const;\n> +\tdouble clampGain(double gain, double *quantizationGain = nullptr) const;\n> \n> \tstd::vector exposureTimes_;\n> \tstd::vector gains_;\n> \n> +\tutils::Duration lineLength_;\n> \tutils::Duration minExposureTime_;\n> \tutils::Duration maxExposureTime_;\n> \tdouble minGain_;\n> \tdouble maxGain_;\n> +\tconst CameraSensorHelper *sensorHelper_;\n> };\n> \n> } /* namespace ipa */","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 F1BE5BDCC1\n\tfor ;\n\tMon, 11 Aug 2025 14:36:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21FB469236;\n\tMon, 11 Aug 2025 16:36:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F19169233\n\tfor ;\n\tMon, 11 Aug 2025 16:36:27 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09B7C82A\n\tfor ;\n\tMon, 11 Aug 2025 16:35:34 +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=\"DefM+ck5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754922935;\n\tbh=Moy6S/a6xPk+68SfPk0/Jifvc9uH8MXZPOZJIbStC3Q=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=DefM+ck5iQVQTQwXjMz1BciRsTLsZ/cUAnLeKN5xV3WVnxlzXOlKzcVktiWP5nAdN\n\tNk4MG8d7zfYRxQBHy77R9191MXeYqCO4yIyBcw34A6HNGwhqUFQ0TBjCB0MokFG+mN\n\tJ1gOpbyni9AOk7jM9UX6yh0OKj5qusr7Y2BanFYk=","Message-ID":"","Date":"Mon, 11 Aug 2025 15:36:24 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 04/16] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","To":"libcamera-devel@lists.libcamera.org","References":"<20250808141315.413839-1-stefan.klug@ideasonboard.com>\n\t<20250808141315.413839-5-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<20250808141315.413839-5-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]