[{"id":35713,"web_url":"https://patchwork.libcamera.org/comment/35713/","msgid":"<175706898003.1787083.11203466010457288826@neptunite.rasen.tech>","date":"2025-09-05T10:43:00","subject":"Re: [PATCH v3 04/19] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-08-15 19:29:24)\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> Signed-off-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Daniel Scally \n> \n> ---\n> \n> Changes in v3:\n> - Collected tags\n> - Renamed lineLength to lineDuration\n> - Remove the \"no functional changes\" sentence, as the returned\n> exposureTime/gain are now quantized, which happened outside of this\n> class before. So that is actually a functional change.\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..b18c30a6291c 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> + : lineDuration_(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] lineDuration 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\ns/this is not/this has not been/\n\ns/is is/it is/\n\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\"or the next call to configure\" is a bit confusing. Do you mean that the\nsensorHelper must be valid until the next configure() call or configure() must\nbe called again if ExposureModeHelper dies?\n\n\nPaul\n\n> + */\n> +void ExposureModeHelper::configure(utils::Duration lineDuration,\n> + const CameraSensorHelper *sensorHelper)\n> +{\n> + lineDuration_ = lineDuration;\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 / lineDuration_) * lineDuration_;\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..ac7e8da95c6c 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 lineDuration_;\n> utils::Duration minExposureTime_;\n> utils::Duration maxExposureTime_;\n> double minGain_;\n> double maxGain_;\n> + const CameraSensorHelper *sensorHelper_;\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 1D905BEFBE\n\tfor ;\n\tFri, 5 Sep 2025 10:43:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6456169361;\n\tFri, 5 Sep 2025 12:43:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F24ED69362\n\tfor ;\n\tFri, 5 Sep 2025 12:43:09 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:dbfb:387c:7405:52bc])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id C9B0E16C5; \n\tFri, 5 Sep 2025 12:41:58 +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=\"JA6Zz+fR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1757068919;\n\tbh=Dm/sfpMvb3GJHMGbzZ5LE/OVVdp+jNZgYh9BrgkLIdk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JA6Zz+fRJutI1MsvQI7INZstEjvrEQc9wM0XSewFSSMeMs6F9XCwskJGSDYnWe2xu\n\tu+A/ylXgAuc1af+jWenNoJwvyphNSkPuYMG5j4xv06JqbzBVRxKEbicjAelLTJVmcR\n\txVcYs+TkCBJLzfS/DCZ3IMeav88pK6QG255ozMgI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250815102945.1602071-5-stefan.klug@ideasonboard.com>","References":"<20250815102945.1602071-1-stefan.klug@ideasonboard.com>\n\t<20250815102945.1602071-5-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v3 04/19] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","From":"Paul Elder ","Cc":"Stefan Klug ,\n\tKieran Bingham ,\n\tDaniel Scally ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 05 Sep 2025 19:43:00 +0900","Message-ID":"<175706898003.1787083.11203466010457288826@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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":35900,"web_url":"https://patchwork.libcamera.org/comment/35900/","msgid":"<175820184956.17312.12436927482996369029@localhost>","date":"2025-09-18T13:24:09","subject":"Re: [PATCH v3 04/19] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the review.\n\nQuoting Paul Elder (2025-09-05 12:43:00)\n> Quoting Stefan Klug (2025-08-15 19:29:24)\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> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Daniel Scally \n> > \n> > ---\n> > \n> > Changes in v3:\n> > - Collected tags\n> > - Renamed lineLength to lineDuration\n> > - Remove the \"no functional changes\" sentence, as the returned\n> > exposureTime/gain are now quantized, which happened outside of this\n> > class before. So that is actually a functional change.\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..b18c30a6291c 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> > + : lineDuration_(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] lineDuration 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> \n> s/this is not/this has not been/\n> \n> s/is is/it is/\n> \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> \"or the next call to configure\" is a bit confusing. Do you mean that the\n> sensorHelper must be valid until the next configure() call or configure() must\n> be called again if ExposureModeHelper dies?\n\nI really don't like that part. I replaced it now with\n\"ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the\ncaller has to ensure that sensorHelper is valid until the next call to\nconfigure().\"\n\nBest regards,\nStefan\n\n> \n> \n> Paul\n> \n> > + */\n> > +void ExposureModeHelper::configure(utils::Duration lineDuration,\n> > + const CameraSensorHelper *sensorHelper)\n> > +{\n> > + lineDuration_ = lineDuration;\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 / lineDuration_) * lineDuration_;\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..ac7e8da95c6c 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 lineDuration_;\n> > utils::Duration minExposureTime_;\n> > utils::Duration maxExposureTime_;\n> > double minGain_;\n> > double maxGain_;\n> > + const CameraSensorHelper *sensorHelper_;\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 1D0C5C328C\n\tfor ;\n\tThu, 18 Sep 2025 13:24:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0EEE6936F;\n\tThu, 18 Sep 2025 15:24:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FB0562C3B\n\tfor ;\n\tThu, 18 Sep 2025 15:24:12 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:ace3:d2c2:5eff:9cc5])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id DD5E355A;\n\tThu, 18 Sep 2025 15:22:52 +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=\"Fu6IS02u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758201773;\n\tbh=NMlAPF7OHhc/b8sOzqO4HVRhnHlyTTBDjPx7umP4iUg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Fu6IS02uAmnlNJLI4eLo4bcRNPPJu227OnLjrBEEc3r8T4dhkhpLawTOic2zOinTe\n\tXf6E688rRNyzs/DbQVePfMJFBakf2EF+wKidSUuYq5T90Wvzbbqf3ON0yhv6SVIcRN\n\tnivUAvY9rQWlQfl+YhkcaxsxCBs7Z8wlN5q824NQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175706898003.1787083.11203466010457288826@neptunite.rasen.tech>","References":"<20250815102945.1602071-1-stefan.klug@ideasonboard.com>\n\t<20250815102945.1602071-5-stefan.klug@ideasonboard.com>\n\t<175706898003.1787083.11203466010457288826@neptunite.rasen.tech>","Subject":"Re: [PATCH v3 04/19] libipa: exposure_mode_helper: Take\n\texposure/gain quantization into account","From":"Stefan Klug ","Cc":"Kieran Bingham ,\n\tDaniel Scally ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 18 Sep 2025 15:24:09 +0200","Message-ID":"<175820184956.17312.12436927482996369029@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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\" "}}]