[{"id":33169,"web_url":"https://patchwork.libcamera.org/comment/33169/","msgid":"","date":"2025-01-24T22:51:12","subject":"Re: [PATCH v2 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jan 23, 2025 at 02:07:26PM +0000, Daniel Scally wrote:\n> Update the ExposureModeHelper class to compensate for flickering\n> light sources in the ExposureModeHelper::splitExposure() function.\n> The adjustment simply caps exposure time at a multiple of the given\n> flicker period and compensates for any loss in the effective exposure\n> value by increasing analogue and then digital gain.\n> \n> Initially in the one call-site for this function, a std::nullopt is\n> passed to make this a no-op.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> Changes in v2:\n> \n> \t- Added a function to perform all the exposure time functionality in a\n> \t single place so we're not repeating ourselves\n> \t- Called that function in all the return sites rather than just one so\n> \t the flicker mitigation takes effect using exposure from the stages\n> \t list\n> \t- Switched the flickerPeriod input to a std::optional\n> \t- Clamped the calculated exposure time to guarantee it can't go beneath\n> \t the configured minima\n> \n> src/ipa/libipa/agc_mean_luminance.cpp | 3 +-\n> src/ipa/libipa/exposure_mode_helper.cpp | 37 ++++++++++++++++++++++---\n> src/ipa/libipa/exposure_mode_helper.h | 6 +++-\n> 3 files changed, 40 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 02555a44..273ec4e5 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -8,6 +8,7 @@\n> #include \"agc_mean_luminance.h\"\n> \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -560,7 +561,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> \tnewExposureValue = filterExposure(newExposureValue);\n> \n> \tframeCount_++;\n> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n> +\treturn exposureModeHelper->splitExposure(newExposureValue, std::nullopt);\n> }\n> \n> /**\n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> index f235316d..4e1ba943 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -7,6 +7,7 @@\n> #include \"exposure_mode_helper.h\"\n> \n> #include \n> +#include \n> \n> #include \n> \n> @@ -118,9 +119,31 @@ double ExposureModeHelper::clampGain(double gain) const\n> \treturn std::clamp(gain, minGain_, maxGain_);\n> }\n> \n> +utils::Duration\n> +ExposureModeHelper::calculateExposureTime(utils::Duration exposure, double stageGain,\n> +\t\t\t\t\t std::optional flickerPeriod) const\n> +{\n> +\tutils::Duration exposureTime;\n> +\n> +\texposureTime = clampExposureTime(exposure / stageGain);\n> +\n> +\t/*\n> +\t * If we haven't been given a flicker period to adjust for or if it's\n> +\t * longer than the exposure time that we need to set then there's not\n> +\t * much we can do to compensate.\n> +\t */\n> +\tif (!flickerPeriod.has_value() || flickerPeriod.value() >= exposureTime)\n> +\t\treturn exposureTime;\n> +\n> +\tunsigned int flickerPeriods = exposureTime / flickerPeriod.value();\n> +\n> +\treturn clampExposureTime(flickerPeriods * flickerPeriod.value());\n\nSomething doesn't seem right to me...?\n\nPremise:\n- exposure * stageGain gives us sufficient total effective exposure\n- flickerPeriod.has_value() and flickerPeriod < exposureTime\n\nScenario:\n- flickerPeriods * flickerPeriod < minExposureTime_\n - Which is possible afaiu if flickerPeriod < exposureTime = minExposureTime_\n- Thus we would end up with exposureTime = minExposureTime_ at a value\n that is not a multiple of flickerPeriod\n\nFor example:\n- Let:\n - exposureTime = minExposureTime_\n - flickerPeriod = minExposureTime_ * 0.8\n- Then:\n - double dFlickerPeriods = exposureTime / flickerPeriod\n = minExposureTime_ / (minExposureTime_ * 0.8)\n\t\t\t = 1.25\n - unsigned int flickerPeriods = (unsigned int)dFlickerPeriods\n = 1\n- Thus:\n - ret = flickerPeriods * flickerPeriod\n = 1 * minExposureTime_ * 0.8\n - Since ret < minExposureTime_, it will get clamped to\n minExposureTime_, and is not a multiple of flickerPeriod\n\nResult:\n- The exposure time doesn't adjust for flicker, since it's not a\n multiple of flickerPeriod\n\nExpected result:\n- We go up one more stage in the splitExposure() loop and run\n calculateExposureTime() on that next stage\n- exposureTime would be high enough that we have leeway to lower\n exposureTime to flicker-adjust\n\n\nOr did I miss something...?\n\n\nPaul\n\n> +}\n> +\n> /**\n> * \\brief Split exposure into exposure time and gain\n> * \\param[in] exposure Exposure value\n> + * \\param[in] flickerPeriod The period of a flickering light source\n> *\n> * This function divides a given exposure into exposure time, analogue and\n> * digital gain by iterating through stages of exposure time and gain limits.\n> @@ -147,10 +170,15 @@ double ExposureModeHelper::clampGain(double gain) const\n> * required exposure, the helper falls-back to simply maximising the exposure\n> * time first, followed by analogue gain, followed by digital gain.\n> *\n> + * Once the exposure time has been determined from the modes, an adjustment is\n> + * made to compensate for a flickering light source by fixing the exposure time\n> + * to an exact multiple of the flicker period. Any effective exposure value that\n> + * is lost is added back via analogue and digital gain.\n> + *\n> * \\return Tuple of exposure time, analogue gain, and digital gain\n> */\n> std::tuple\n> -ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> +ExposureModeHelper::splitExposure(utils::Duration exposure, std::optional flickerPeriod) const\n> {\n> \tASSERT(maxExposureTime_);\n> \tASSERT(maxGain_);\n> @@ -183,14 +211,14 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> \t\t */\n> \n> \t\tif (stageExposureTime * lastStageGain >= exposure) {\n> -\t\t\texposureTime = clampExposureTime(exposure / clampGain(lastStageGain));\n> +\t\t\texposureTime = calculateExposureTime(exposure, clampGain(lastStageGain), flickerPeriod);\n> \t\t\tgain = clampGain(exposure / exposureTime);\n> \n> \t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> \t\t}\n> \n> \t\tif (stageExposureTime * stageGain >= exposure) {\n> -\t\t\texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n> +\t\t\texposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod);\n> \t\t\tgain = clampGain(exposure / exposureTime);\n> \n> \t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> @@ -204,7 +232,8 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> \t * stages to use then the default stageGain of 1.0 is used so that\n> \t * exposure time is maxed before gain is touched at all.\n> \t */\n> -\texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n> +\texposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod);\n> +\n> \tgain = clampGain(exposure / exposureTime);\n> \n> \treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> index c5be1b67..a1d8c6bf 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -7,6 +7,7 @@\n> \n> #pragma once\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -28,7 +29,7 @@ public:\n> \t\t double minGain, double maxGain);\n> \n> \tstd::tuple\n> -\tsplitExposure(utils::Duration exposure) const;\n> +\tsplitExposure(utils::Duration exposure, std::optional flickerPeriod) const;\n> \n> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }\n> @@ -38,6 +39,9 @@ public:\n> private:\n> \tutils::Duration clampExposureTime(utils::Duration exposureTime) const;\n> \tdouble clampGain(double gain) const;\n> +\tutils::Duration\n> +\tcalculateExposureTime(utils::Duration exposureTime, double stageGain,\n> +\t\t\t std::optional flickerPeriod) const;\n> \n> \tstd::vector exposureTimes_;\n> \tstd::vector gains_;\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 072CAC31E9\n\tfor ;\n\tFri, 24 Jan 2025 22:51:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CFD66855D;\n\tFri, 24 Jan 2025 23:51:21 +0100 (CET)","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 5540C68556\n\tfor ;\n\tFri, 24 Jan 2025 23:51:19 +0100 (CET)","from pyrite.rasen.tech (unknown [IPv6:2603:6081:63f0:60f0::17f2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0748221C;\n\tFri, 24 Jan 2025 23:50:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Aq6uTWv5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737759014;\n\tbh=uuQkriAN2skJPTk0vfZ47t/0zDUJWqCx76OTpZYddvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Aq6uTWv5YJlxPvaEznZ1bvRFZid+KIKTSbh5tk3GRRUMESH/cGzSe+YpraqebDEzK\n\tXJLDqkLfatHQFV+7IjMRJPg6+5iRJAm0HGye1BXmr7vi/riU5XPCvvPAzSH4JyXk9G\n\tiFUPn62HC5kxLyUOfcEo4qbO2hgPt+G0tAzEonhA=","Date":"Fri, 24 Jan 2025 17:51:12 -0500","From":"Paul Elder ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","Message-ID":"","References":"<20250123140727.458567-1-dan.scally@ideasonboard.com>\n\t<20250123140727.458567-3-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250123140727.458567-3-dan.scally@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":33170,"web_url":"https://patchwork.libcamera.org/comment/33170/","msgid":"<20250124230243.GB1805@pendragon.ideasonboard.com>","date":"2025-01-24T23:02:43","subject":"Re: [PATCH v2 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dan,\n\nThank you for the patch.\n\nOn Thu, Jan 23, 2025 at 02:07:26PM +0000, Daniel Scally wrote:\n> Update the ExposureModeHelper class to compensate for flickering\n> light sources in the ExposureModeHelper::splitExposure() function.\n> The adjustment simply caps exposure time at a multiple of the given\n> flicker period and compensates for any loss in the effective exposure\n> value by increasing analogue and then digital gain.\n> \n> Initially in the one call-site for this function, a std::nullopt is\n> passed to make this a no-op.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> Changes in v2:\n> \n> \t- Added a function to perform all the exposure time functionality in a\n> \t single place so we're not repeating ourselves\n> \t- Called that function in all the return sites rather than just one so\n> \t the flicker mitigation takes effect using exposure from the stages\n> \t list\n> \t- Switched the flickerPeriod input to a std::optional\n> \t- Clamped the calculated exposure time to guarantee it can't go beneath\n> \t the configured minima\n> \n> src/ipa/libipa/agc_mean_luminance.cpp | 3 +-\n> src/ipa/libipa/exposure_mode_helper.cpp | 37 ++++++++++++++++++++++---\n> src/ipa/libipa/exposure_mode_helper.h | 6 +++-\n> 3 files changed, 40 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 02555a44..273ec4e5 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -8,6 +8,7 @@\n> #include \"agc_mean_luminance.h\"\n> \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -560,7 +561,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> \tnewExposureValue = filterExposure(newExposureValue);\n> \n> \tframeCount_++;\n> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n> +\treturn exposureModeHelper->splitExposure(newExposureValue, std::nullopt);\n> }\n> \n> /**\n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> index f235316d..4e1ba943 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -7,6 +7,7 @@\n> #include \"exposure_mode_helper.h\"\n> \n> #include \n> +#include \n> \n> #include \n> \n> @@ -118,9 +119,31 @@ double ExposureModeHelper::clampGain(double gain) const\n> \treturn std::clamp(gain, minGain_, maxGain_);\n> }\n> \n> +utils::Duration\n> +ExposureModeHelper::calculateExposureTime(utils::Duration exposure, double stageGain,\n> +\t\t\t\t\t std::optional flickerPeriod) const\n> +{\n> +\tutils::Duration exposureTime;\n> +\n\nLet's assume that the [minExposureTime_, maxExposureTime_] interval\ncontains exposure/stageGain and exposure/stageGain rounded up to the\nnext multiple of the flicker period, but not rounded down.\n\n> +\texposureTime = clampExposureTime(exposure / stageGain);\n\nexposureTime will be exactly exposure/stageGain.\n\n> +\n> +\t/*\n> +\t * If we haven't been given a flicker period to adjust for or if it's\n> +\t * longer than the exposure time that we need to set then there's not\n> +\t * much we can do to compensate.\n> +\t */\n> +\tif (!flickerPeriod.has_value() || flickerPeriod.value() >= exposureTime)\n> +\t\treturn exposureTime;\n> +\n> +\tunsigned int flickerPeriods = exposureTime / flickerPeriod.value();\n> +\n> +\treturn clampExposureTime(flickerPeriods * flickerPeriod.value());\n\nThis will round exposureTime down to a multiple of flickerPeriod and\nclamp it. The returned value will be minExposureTime_, which is not a\nmultiple of flickerPeriod, while the [min, max] interval contains a\nvalid multiple of flickerPeriod.\n\nThe preconditions and postconditions of this function should be\ndocumented. \n\n> +}\n> +\n> /**\n> * \\brief Split exposure into exposure time and gain\n> * \\param[in] exposure Exposure value\n> + * \\param[in] flickerPeriod The period of a flickering light source\n> *\n> * This function divides a given exposure into exposure time, analogue and\n> * digital gain by iterating through stages of exposure time and gain limits.\n> @@ -147,10 +170,15 @@ double ExposureModeHelper::clampGain(double gain) const\n> * required exposure, the helper falls-back to simply maximising the exposure\n> * time first, followed by analogue gain, followed by digital gain.\n> *\n> + * Once the exposure time has been determined from the modes, an adjustment is\n> + * made to compensate for a flickering light source by fixing the exposure time\n> + * to an exact multiple of the flicker period. Any effective exposure value that\n> + * is lost is added back via analogue and digital gain.\n> + *\n> * \\return Tuple of exposure time, analogue gain, and digital gain\n> */\n> std::tuple\n> -ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> +ExposureModeHelper::splitExposure(utils::Duration exposure, std::optional flickerPeriod) const\n\nLet's keep lines below 100 characters at least. Same below.\n\n> {\n> \tASSERT(maxExposureTime_);\n> \tASSERT(maxGain_);\n> @@ -183,14 +211,14 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> \t\t */\n> \n> \t\tif (stageExposureTime * lastStageGain >= exposure) {\n> -\t\t\texposureTime = clampExposureTime(exposure / clampGain(lastStageGain));\n> +\t\t\texposureTime = calculateExposureTime(exposure, clampGain(lastStageGain), flickerPeriod);\n\nYou always pass clampGain(...) to the calculateExposureTime() function.\nShould the clampGain() call be moved there ? I suppose it depends on how\ncalculateExposureTime() is defined. Even if it's a private function, I\nthink documenting it would help understand the code flow.\n\n> \t\t\tgain = clampGain(exposure / exposureTime);\n> \n> \t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> \t\t}\n> \n> \t\tif (stageExposureTime * stageGain >= exposure) {\n> -\t\t\texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n> +\t\t\texposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod);\n> \t\t\tgain = clampGain(exposure / exposureTime);\n> \n> \t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n\nI have a bit of trouble following the impact of rounding the exposure at\nevery stage. It may be fine, but I think an explanation in the commit\nmessage of why it's fine will help.\n\n\n> @@ -204,7 +232,8 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> \t * stages to use then the default stageGain of 1.0 is used so that\n> \t * exposure time is maxed before gain is touched at all.\n> \t */\n> -\texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n> +\texposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod);\n> +\n\nThe code here tries to maximize the exposure time. Can it also suffer\nfrom calculateExposureTime() rounding down ?\n\n> \tgain = clampGain(exposure / exposureTime);\n> \n> \treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> index c5be1b67..a1d8c6bf 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -7,6 +7,7 @@\n> \n> #pragma once\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -28,7 +29,7 @@ public:\n> \t\t double minGain, double maxGain);\n> \n> \tstd::tuple\n> -\tsplitExposure(utils::Duration exposure) const;\n> +\tsplitExposure(utils::Duration exposure, std::optional flickerPeriod) const;\n> \n> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }\n> @@ -38,6 +39,9 @@ public:\n> private:\n> \tutils::Duration clampExposureTime(utils::Duration exposureTime) const;\n> \tdouble clampGain(double gain) const;\n> +\tutils::Duration\n> +\tcalculateExposureTime(utils::Duration exposureTime, double stageGain,\n> +\t\t\t std::optional flickerPeriod) const;\n> \n> \tstd::vector exposureTimes_;\n> \tstd::vector gains_;","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 BA840BD78E\n\tfor ;\n\tFri, 24 Jan 2025 23:02:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E9E16855D;\n\tSat, 25 Jan 2025 00:02:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A199E61878\n\tfor ;\n\tSat, 25 Jan 2025 00:02:54 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F25C8134C;\n\tSat, 25 Jan 2025 00:01:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fNBH73bw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737759710;\n\tbh=7EF3EPSqMlJljUsUgqiI/mow3YME7/6MEGqEs8aMR9g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fNBH73bwWv9ZnBPpuLN5asX2gMpqhaMaptt9oe17gRx4CegUYHqh+iGZR3OuNJOny\n\tu9jsLiEJc4Ie/SX2D1mV6P2zuAHLJ/giRhmG+fLMlQLRJD5Wr0AHkpB6LzYwicbxzU\n\tCDRogrIoVOz0EtUQuwmZZzygiy/psGU2H9LtDtYI=","Date":"Sat, 25 Jan 2025 01:02:43 +0200","From":"Laurent Pinchart ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","Message-ID":"<20250124230243.GB1805@pendragon.ideasonboard.com>","References":"<20250123140727.458567-1-dan.scally@ideasonboard.com>\n\t<20250123140727.458567-3-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250123140727.458567-3-dan.scally@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":33588,"web_url":"https://patchwork.libcamera.org/comment/33588/","msgid":"<448386c1-fb02-4b27-9897-67247b5391f8@ideasonboard.com>","date":"2025-03-07T13:52:56","subject":"Re: [PATCH v2 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul, thanks for the very in-depth review\n\nOn 24/01/2025 22:51, Paul Elder wrote:\n> On Thu, Jan 23, 2025 at 02:07:26PM +0000, Daniel Scally wrote:\n>> Update the ExposureModeHelper class to compensate for flickering\n>> light sources in the ExposureModeHelper::splitExposure() function.\n>> The adjustment simply caps exposure time at a multiple of the given\n>> flicker period and compensates for any loss in the effective exposure\n>> value by increasing analogue and then digital gain.\n>>\n>> Initially in the one call-site for this function, a std::nullopt is\n>> passed to make this a no-op.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> Changes in v2:\n>>\n>> \t- Added a function to perform all the exposure time functionality in a\n>> \t single place so we're not repeating ourselves\n>> \t- Called that function in all the return sites rather than just one so\n>> \t the flicker mitigation takes effect using exposure from the stages\n>> \t list\n>> \t- Switched the flickerPeriod input to a std::optional\n>> \t- Clamped the calculated exposure time to guarantee it can't go beneath\n>> \t the configured minima\n>>\n>> src/ipa/libipa/agc_mean_luminance.cpp | 3 +-\n>> src/ipa/libipa/exposure_mode_helper.cpp | 37 ++++++++++++++++++++++---\n>> src/ipa/libipa/exposure_mode_helper.h | 6 +++-\n>> 3 files changed, 40 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>> index 02555a44..273ec4e5 100644\n>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>> @@ -8,6 +8,7 @@\n>> #include \"agc_mean_luminance.h\"\n>> \n>> #include \n>> +#include \n>> \n>> #include \n>> #include \n>> @@ -560,7 +561,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n>> \tnewExposureValue = filterExposure(newExposureValue);\n>> \n>> \tframeCount_++;\n>> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n>> +\treturn exposureModeHelper->splitExposure(newExposureValue, std::nullopt);\n>> }\n>> \n>> /**\n>> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n>> index f235316d..4e1ba943 100644\n>> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n>> @@ -7,6 +7,7 @@\n>> #include \"exposure_mode_helper.h\"\n>> \n>> #include \n>> +#include \n>> \n>> #include \n>> \n>> @@ -118,9 +119,31 @@ double ExposureModeHelper::clampGain(double gain) const\n>> \treturn std::clamp(gain, minGain_, maxGain_);\n>> }\n>> \n>> +utils::Duration\n>> +ExposureModeHelper::calculateExposureTime(utils::Duration exposure, double stageGain,\n>> +\t\t\t\t\t std::optional flickerPeriod) const\n>> +{\n>> +\tutils::Duration exposureTime;\n>> +\n>> +\texposureTime = clampExposureTime(exposure / stageGain);\n>> +\n>> +\t/*\n>> +\t * If we haven't been given a flicker period to adjust for or if it's\n>> +\t * longer than the exposure time that we need to set then there's not\n>> +\t * much we can do to compensate.\n>> +\t */\n>> +\tif (!flickerPeriod.has_value() || flickerPeriod.value() >= exposureTime)\n>> +\t\treturn exposureTime;\n>> +\n>> +\tunsigned int flickerPeriods = exposureTime / flickerPeriod.value();\n>> +\n>> +\treturn clampExposureTime(flickerPeriods * flickerPeriod.value());\n> Something doesn't seem right to me...?\n>\n> Premise:\n> - exposure * stageGain gives us sufficient total effective exposure\n> - flickerPeriod.has_value() and flickerPeriod < exposureTime\n>\n> Scenario:\n> - flickerPeriods * flickerPeriod < minExposureTime_\n> - Which is possible afaiu if flickerPeriod < exposureTime = minExposureTime_\n> - Thus we would end up with exposureTime = minExposureTime_ at a value\n> that is not a multiple of flickerPeriod\n>\n> For example:\n> - Let:\n> - exposureTime = minExposureTime_\n> - flickerPeriod = minExposureTime_ * 0.8\n> - Then:\n> - double dFlickerPeriods = exposureTime / flickerPeriod\n> = minExposureTime_ / (minExposureTime_ * 0.8)\n> \t\t\t = 1.25\n> - unsigned int flickerPeriods = (unsigned int)dFlickerPeriods\n> = 1\n> - Thus:\n> - ret = flickerPeriods * flickerPeriod\n> = 1 * minExposureTime_ * 0.8\n> - Since ret < minExposureTime_, it will get clamped to\n> minExposureTime_, and is not a multiple of flickerPeriod\n>\n> Result:\n> - The exposure time doesn't adjust for flicker, since it's not a\n> multiple of flickerPeriod\n>\n> Expected result:\n> - We go up one more stage in the splitExposure() loop and run\n> calculateExposureTime() on that next stage\n> - exposureTime would be high enough that we have leeway to lower\n> exposureTime to flicker-adjust\n>\n>\n> Or did I miss something...?\n\n\nThis made my brain hurt, but no I don't think so. I think I've accounted for this by moving the \nclamping to a multiple of the flicker period inside clampExposureTime() itself, which means that \nwhen the loop evaluates whether stageExposureTime multiplied by either lastStageGain or stageGain is \nsufficient to meet the requirements, it's doing so based on an exposure time that's already fixed to \na multiple of flickerPeriod (if possible).\n\n\n>\n>\n> Paul\n>\n>> +}\n>> +\n>> /**\n>> * \\brief Split exposure into exposure time and gain\n>> * \\param[in] exposure Exposure value\n>> + * \\param[in] flickerPeriod The period of a flickering light source\n>> *\n>> * This function divides a given exposure into exposure time, analogue and\n>> * digital gain by iterating through stages of exposure time and gain limits.\n>> @@ -147,10 +170,15 @@ double ExposureModeHelper::clampGain(double gain) const\n>> * required exposure, the helper falls-back to simply maximising the exposure\n>> * time first, followed by analogue gain, followed by digital gain.\n>> *\n>> + * Once the exposure time has been determined from the modes, an adjustment is\n>> + * made to compensate for a flickering light source by fixing the exposure time\n>> + * to an exact multiple of the flicker period. Any effective exposure value that\n>> + * is lost is added back via analogue and digital gain.\n>> + *\n>> * \\return Tuple of exposure time, analogue gain, and digital gain\n>> */\n>> std::tuple\n>> -ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>> +ExposureModeHelper::splitExposure(utils::Duration exposure, std::optional flickerPeriod) const\n>> {\n>> \tASSERT(maxExposureTime_);\n>> \tASSERT(maxGain_);\n>> @@ -183,14 +211,14 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>> \t\t */\n>> \n>> \t\tif (stageExposureTime * lastStageGain >= exposure) {\n>> -\t\t\texposureTime = clampExposureTime(exposure / clampGain(lastStageGain));\n>> +\t\t\texposureTime = calculateExposureTime(exposure, clampGain(lastStageGain), flickerPeriod);\n>> \t\t\tgain = clampGain(exposure / exposureTime);\n>> \n>> \t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n>> \t\t}\n>> \n>> \t\tif (stageExposureTime * stageGain >= exposure) {\n>> -\t\t\texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n>> +\t\t\texposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod);\n>> \t\t\tgain = clampGain(exposure / exposureTime);\n>> \n>> \t\t\treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n>> @@ -204,7 +232,8 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>> \t * stages to use then the default stageGain of 1.0 is used so that\n>> \t * exposure time is maxed before gain is touched at all.\n>> \t */\n>> -\texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n>> +\texposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod);\n>> +\n>> \tgain = clampGain(exposure / exposureTime);\n>> \n>> \treturn { exposureTime, gain, exposure / (exposureTime * gain) };\n>> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n>> index c5be1b67..a1d8c6bf 100644\n>> --- a/src/ipa/libipa/exposure_mode_helper.h\n>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n>> @@ -7,6 +7,7 @@\n>> \n>> #pragma once\n>> \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -28,7 +29,7 @@ public:\n>> \t\t double minGain, double maxGain);\n>> \n>> \tstd::tuple\n>> -\tsplitExposure(utils::Duration exposure) const;\n>> +\tsplitExposure(utils::Duration exposure, std::optional flickerPeriod) const;\n>> \n>> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n>> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }\n>> @@ -38,6 +39,9 @@ public:\n>> private:\n>> \tutils::Duration clampExposureTime(utils::Duration exposureTime) const;\n>> \tdouble clampGain(double gain) const;\n>> +\tutils::Duration\n>> +\tcalculateExposureTime(utils::Duration exposureTime, double stageGain,\n>> +\t\t\t std::optional flickerPeriod) const;\n>> \n>> \tstd::vector exposureTimes_;\n>> \tstd::vector gains_;\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 6F642C32DE\n\tfor ;\n\tFri, 7 Mar 2025 13:53:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E1F0687F0;\n\tFri, 7 Mar 2025 14:53:03 +0100 (CET)","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 106DC68779\n\tfor ;\n\tFri, 7 Mar 2025 14:53:00 +0100 (CET)","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 88B8E63B;\n\tFri, 7 Mar 2025 14:51:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"O2w1fcg8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741355485;\n\tbh=8eT5mUfpZ8SRKhupFgn8rrYWSE8V23qaoaxDKGr2Sig=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=O2w1fcg8vMEhaFlg3lsk/eEJIwBVbj/6ciCD+E3LQg2aOhoP9UU9TafDA0/hJWhGA\n\tVKf64yToct9nV+9iOAOre1QOD7GfhK9p4HFE0m1eg/YDcTALi4X5LM73CjX/LVEvTM\n\t8uxKYVOxfOYZ6ngvBKGbuPsIxPovuMfVxBRwuACE=","Message-ID":"<448386c1-fb02-4b27-9897-67247b5391f8@ideasonboard.com>","Date":"Fri, 7 Mar 2025 13:52:56 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250123140727.458567-1-dan.scally@ideasonboard.com>\n\t<20250123140727.458567-3-dan.scally@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","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\" "}}]