[{"id":35925,"web_url":"https://patchwork.libcamera.org/comment/35925/","msgid":"<175828149458.3174118.6828517597900437960@neptunite.rasen.tech>","date":"2025-09-19T11:31:34","subject":"Re: [PATCH v5 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":"Hi Stefan,\n\nThanks for the patch.\n\nQuoting Stefan Klug (2025-09-19 18:40:19)\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\nLooks good to me.\n\nReviewed-by: Paul Elder \n\n> \n> ---\n> \n> Changes in v4:\n> - Improved documentation\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..b776a031b441 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 has not been called, it is assumed that exposure is in micro second\n> + * granularity and gain has no quantization at all.\n> + *\n> + * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller\n> + * has to ensure that sensorHelper is valid until the next call to configure().\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 17878BE173\n\tfor ;\n\tFri, 19 Sep 2025 11:31:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DE3F6B59A;\n\tFri, 19 Sep 2025 13:31:42 +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 8509862C35\n\tfor ;\n\tFri, 19 Sep 2025 13:31:40 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:ae7e:60b0:e249:fbe5])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B13BBD7;\n\tFri, 19 Sep 2025 13:30:19 +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=\"J8PCBnDb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758281420;\n\tbh=2xoRwCwd37+MkR522vfgepagtBfmjHPshPZOfw6Y+74=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=J8PCBnDbF2HGseNVWQ/09xFrYEDrgvg7NcYjwFA+lS0y23k0zgaKTti060R0ZlSjF\n\tNmeJ6s13UBRrlmhGkoV0vDLHDzsZH0J4iOkypoz75S8iTmCAyrd8irUJGeYYrATHJT\n\tsbdjTDcNunaVfMCWgD4503bavjUGJy3rX3tKm6Mg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250919094041.183031-5-stefan.klug@ideasonboard.com>","References":"<20250919094041.183031-1-stefan.klug@ideasonboard.com>\n\t<20250919094041.183031-5-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v5 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, 19 Sep 2025 20:31:34 +0900","Message-ID":"<175828149458.3174118.6828517597900437960@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\" "}}]