[{"id":33124,"web_url":"https://patchwork.libcamera.org/comment/33124/","msgid":"<20250121162741.GQ9249@pendragon.ideasonboard.com>","date":"2025-01-21T16:27:41","subject":"Re: [PATCH 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 Fri, Jan 17, 2025 at 02:34:09PM +0000, Daniel Scally wrote:\n> Update the ExposureModeHelper class to compensate for flickering\n> light sources in the ::splitExposure() function. The adjustment\n\nExposureModeHelper::splitExposure() or just splitExposure()\n\n> simply caps exposure time at a multiple of the given flicker period\n> and compensates for any loss in the effective exposure value by\n> increasing analogue and then digital gain.\n> \n> Initially in the one call-site for this function, a flicker period\n> of 0us is passed, making this a no-op.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 2 +-\n> src/ipa/libipa/exposure_mode_helper.cpp | 20 +++++++++++++++++++-\n> src/ipa/libipa/exposure_mode_helper.h | 2 +-\n> 3 files changed, 21 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 02555a44..b5e6afe3 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -560,7 +560,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> \tnewExposureValue = filterExposure(newExposureValue);\n> \n> \tframeCount_++;\n> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n> +\treturn exposureModeHelper->splitExposure(newExposureValue, 0us);\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..038aa33c 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -121,6 +121,7 @@ double ExposureModeHelper::clampGain(double gain) const\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 +148,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, utils::Duration flickerPeriod) const\n\nWe could avoid magic values by passing a std::optional.\nWhat do you think ?\n\n> {\n> \tASSERT(maxExposureTime_);\n> \tASSERT(maxGain_);\n> @@ -205,6 +211,18 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> \t * exposure time is maxed before gain is touched at all.\n> \t */\n> \texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n> +\n> +\t/*\n> +\t * Finally, if we have been given a flicker period we need to reduce the\n> +\t * exposure time to be a multiple of that period (if possible). The\n> +\t * effect of this should be to hide the flicker.\n> +\t */\n> +\tif (flickerPeriod > 0us && flickerPeriod < exposureTime) {\n> +\t\tunsigned int flickerPeriods = exposureTime / flickerPeriod;\n> +\t\tif (flickerPeriods)\n> +\t\t\texposureTime = flickerPeriods * flickerPeriod;\n> +\t}\n\nThis means that the exposure time could become lower than\nminExposureTime_. Is that an issue ? Should we define what controls take\nprecedence in that case ? For instance, when manual exposure time is\nenabled, should the flicker period be ignored ?\n\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..a5fcc366 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -28,7 +28,7 @@ public:\n> \t\t double minGain, double maxGain);\n> \n> \tstd::tuple\n> -\tsplitExposure(utils::Duration exposure) const;\n> +\tsplitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const;\n> \n> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }","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 922F0BD160\n\tfor ;\n\tTue, 21 Jan 2025 16:27:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFB9568550;\n\tTue, 21 Jan 2025 17:27:50 +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 502FF68503\n\tfor ;\n\tTue, 21 Jan 2025 17:27:49 +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 CEA6555A;\n\tTue, 21 Jan 2025 17:26:46 +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=\"KwMpxFZO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737476807;\n\tbh=fEgMm9RMD+O3SYARMa1LYBgURj/SeJoWB5dGdww5QsY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KwMpxFZOtoyemIblHL5MsewSSA0VGMlMwPjx6OXn8R57pV//Sjl/Px0uam0gCFxTI\n\tJXHf55Au5eGcelEHPM91qfpAJWesfvo4S/P+8Rh2JjwtYmnRvqrIU/BENV877683IM\n\tz9bO1bTJs2zEoeeW0zv/xUtGnk/sCHvenwXTqGlY=","Date":"Tue, 21 Jan 2025 18:27:41 +0200","From":"Laurent Pinchart ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","Message-ID":"<20250121162741.GQ9249@pendragon.ideasonboard.com>","References":"<20250117143410.20363-1-dan.scally@ideasonboard.com>\n\t<20250117143410.20363-3-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250117143410.20363-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":33125,"web_url":"https://patchwork.libcamera.org/comment/33125/","msgid":"","date":"2025-01-21T16:30:19","subject":"Re: [PATCH 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 Laurent\n\nOn 21/01/2025 16:27, Laurent Pinchart wrote:\n> Hi Dan,\n>\n> Thank you for the patch.\n>\n> On Fri, Jan 17, 2025 at 02:34:09PM +0000, Daniel Scally wrote:\n>> Update the ExposureModeHelper class to compensate for flickering\n>> light sources in the ::splitExposure() function. The adjustment\n> ExposureModeHelper::splitExposure() or just splitExposure()\nThe former :)\n>\n>> simply caps exposure time at a multiple of the given flicker period\n>> and compensates for any loss in the effective exposure value by\n>> increasing analogue and then digital gain.\n>>\n>> Initially in the one call-site for this function, a flicker period\n>> of 0us is passed, making this a no-op.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> src/ipa/libipa/agc_mean_luminance.cpp | 2 +-\n>> src/ipa/libipa/exposure_mode_helper.cpp | 20 +++++++++++++++++++-\n>> src/ipa/libipa/exposure_mode_helper.h | 2 +-\n>> 3 files changed, 21 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>> index 02555a44..b5e6afe3 100644\n>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>> @@ -560,7 +560,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n>> \tnewExposureValue = filterExposure(newExposureValue);\n>> \n>> \tframeCount_++;\n>> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n>> +\treturn exposureModeHelper->splitExposure(newExposureValue, 0us);\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..038aa33c 100644\n>> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n>> @@ -121,6 +121,7 @@ double ExposureModeHelper::clampGain(double gain) const\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 +148,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, utils::Duration flickerPeriod) const\n> We could avoid magic values by passing a std::optional.\n> What do you think ?\nGood idea\n>\n>> {\n>> \tASSERT(maxExposureTime_);\n>> \tASSERT(maxGain_);\n>> @@ -205,6 +211,18 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>> \t * exposure time is maxed before gain is touched at all.\n>> \t */\n>> \texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n>> +\n>> +\t/*\n>> +\t * Finally, if we have been given a flicker period we need to reduce the\n>> +\t * exposure time to be a multiple of that period (if possible). The\n>> +\t * effect of this should be to hide the flicker.\n>> +\t */\n>> +\tif (flickerPeriod > 0us && flickerPeriod < exposureTime) {\n>> +\t\tunsigned int flickerPeriods = exposureTime / flickerPeriod;\n>> +\t\tif (flickerPeriods)\n>> +\t\t\texposureTime = flickerPeriods * flickerPeriod;\n>> +\t}\n> This means that the exposure time could become lower than\n> minExposureTime_. Is that an issue ?\nI doubt it given likely exposure times and flicker periods, but I can extend it to guarantee that \ndoesn't happen.\n> Should we define what controls take\n> precedence in that case ? For instance, when manual exposure time is\n> enabled, should the flicker period be ignored ?\n\nYes, and is. This only takes effect in automatic exposure mode.\n\n>\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..a5fcc366 100644\n>> --- a/src/ipa/libipa/exposure_mode_helper.h\n>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n>> @@ -28,7 +28,7 @@ public:\n>> \t\t double minGain, double maxGain);\n>> \n>> \tstd::tuple\n>> -\tsplitExposure(utils::Duration exposure) const;\n>> +\tsplitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const;\n>> \n>> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n>> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }","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 E007FBD16B\n\tfor ;\n\tTue, 21 Jan 2025 16:30:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CF5E68553;\n\tTue, 21 Jan 2025 17:30:23 +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 34D6968543\n\tfor ;\n\tTue, 21 Jan 2025 17:30:22 +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 DB33755A;\n\tTue, 21 Jan 2025 17:29:19 +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=\"cConkd/X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737476960;\n\tbh=HrTLRXBR08dIDve1ujxJ48raKp8MRe8vxcl9n2hR+UU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=cConkd/XGesekeLC8nTrJ/UjTCJ0/BPIjMOF3e94X4oga7yk1cC/2GJTusXeRnPVv\n\tmMexVSiggWEtHxidbb8HIXBxaKKmM/iNShDpxEHgmAjGAvs0jcox/vrVwvcvIxB5Cd\n\tbTbTLJZqpuHCI7zQ1dXECDY64oRB46Qpfe1ttImw=","Message-ID":"","Date":"Tue, 21 Jan 2025 16:30:19 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250117143410.20363-1-dan.scally@ideasonboard.com>\n\t<20250117143410.20363-3-dan.scally@ideasonboard.com>\n\t<20250121162741.GQ9249@pendragon.ideasonboard.com>","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":"<20250121162741.GQ9249@pendragon.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\" "}},{"id":33127,"web_url":"https://patchwork.libcamera.org/comment/33127/","msgid":"<20250121171737.GR9249@pendragon.ideasonboard.com>","date":"2025-01-21T17:17:37","subject":"Re: [PATCH 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\nOn Tue, Jan 21, 2025 at 04:30:19PM +0000, Daniel Scally wrote:\n> On 21/01/2025 16:27, Laurent Pinchart wrote:\n> > On Fri, Jan 17, 2025 at 02:34:09PM +0000, Daniel Scally wrote:\n> >> Update the ExposureModeHelper class to compensate for flickering\n> >> light sources in the ::splitExposure() function. The adjustment\n> > ExposureModeHelper::splitExposure() or just splitExposure()\n> The former :)\n> >\n> >> simply caps exposure time at a multiple of the given flicker period\n> >> and compensates for any loss in the effective exposure value by\n> >> increasing analogue and then digital gain.\n> >>\n> >> Initially in the one call-site for this function, a flicker period\n> >> of 0us is passed, making this a no-op.\n> >>\n> >> Signed-off-by: Daniel Scally \n> >> ---\n> >> src/ipa/libipa/agc_mean_luminance.cpp | 2 +-\n> >> src/ipa/libipa/exposure_mode_helper.cpp | 20 +++++++++++++++++++-\n> >> src/ipa/libipa/exposure_mode_helper.h | 2 +-\n> >> 3 files changed, 21 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> >> index 02555a44..b5e6afe3 100644\n> >> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> >> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> >> @@ -560,7 +560,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> >> \tnewExposureValue = filterExposure(newExposureValue);\n> >> \n> >> \tframeCount_++;\n> >> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n> >> +\treturn exposureModeHelper->splitExposure(newExposureValue, 0us);\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..038aa33c 100644\n> >> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> >> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> >> @@ -121,6 +121,7 @@ double ExposureModeHelper::clampGain(double gain) const\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 +148,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, utils::Duration flickerPeriod) const\n> > We could avoid magic values by passing a std::optional.\n> > What do you think ?\n> Good idea\n> >\n> >> {\n> >> \tASSERT(maxExposureTime_);\n> >> \tASSERT(maxGain_);\n> >> @@ -205,6 +211,18 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> >> \t * exposure time is maxed before gain is touched at all.\n> >> \t */\n> >> \texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n> >> +\n> >> +\t/*\n> >> +\t * Finally, if we have been given a flicker period we need to reduce the\n> >> +\t * exposure time to be a multiple of that period (if possible). The\n> >> +\t * effect of this should be to hide the flicker.\n> >> +\t */\n> >> +\tif (flickerPeriod > 0us && flickerPeriod < exposureTime) {\n> >> +\t\tunsigned int flickerPeriods = exposureTime / flickerPeriod;\n> >> +\t\tif (flickerPeriods)\n> >> +\t\t\texposureTime = flickerPeriods * flickerPeriod;\n> >> +\t}\n> >\n> > This means that the exposure time could become lower than\n> > minExposureTime_. Is that an issue ?\n>\n> I doubt it given likely exposure times and flicker periods, but I can\n> extend it to guarantee that doesn't happen.\n>\n> > Should we define what controls take\n> > precedence in that case ? For instance, when manual exposure time is\n> > enabled, should the flicker period be ignored ?\n> \n> Yes, and is. This only takes effect in automatic exposure mode.\n\nIs it ? If exposure time is manual and gain is automatic, the above code\nstill gets run as far as I can tell.\n\nThe interactions between the flicker and AE controls also need to be\ndocumented in the definition of the appropriate controls.\n\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..a5fcc366 100644\n> >> --- a/src/ipa/libipa/exposure_mode_helper.h\n> >> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> >> @@ -28,7 +28,7 @@ public:\n> >> \t\t double minGain, double maxGain);\n> >> \n> >> \tstd::tuple\n> >> -\tsplitExposure(utils::Duration exposure) const;\n> >> +\tsplitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const;\n> >> \n> >> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n> >> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }","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 92C3DBD16B\n\tfor ;\n\tTue, 21 Jan 2025 17:17:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD6BE68550;\n\tTue, 21 Jan 2025 18:17:47 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 952E568503\n\tfor ;\n\tTue, 21 Jan 2025 18:17:45 +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 39BEF788;\n\tTue, 21 Jan 2025 18:16:43 +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=\"U2DDTpqD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737479803;\n\tbh=D4mAJihINA0obd/iisTZeGw1//RzMpUJx9A/ot513Ss=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U2DDTpqD1IIXH+VKk12zma0Ou2wbAKeGq+F8sguMPt6tfI1Mw9brvwSwaiTJQ0biu\n\tEFrXodqwhSqD9txcg0wINz7eXtSnKCvFUfPjHj/1FGvdvjGr2tRSWcN3mabUS8DUvw\n\tgaJBdXajx7bcN8GBv4pxy7A7l2NLsOLFPG2uW+jE=","Date":"Tue, 21 Jan 2025 19:17:37 +0200","From":"Laurent Pinchart ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","Message-ID":"<20250121171737.GR9249@pendragon.ideasonboard.com>","References":"<20250117143410.20363-1-dan.scally@ideasonboard.com>\n\t<20250117143410.20363-3-dan.scally@ideasonboard.com>\n\t<20250121162741.GQ9249@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":33128,"web_url":"https://patchwork.libcamera.org/comment/33128/","msgid":"<181ee622-b420-43a6-8ef1-06ddd2e9f680@ideasonboard.com>","date":"2025-01-21T21:29:01","subject":"Re: [PATCH 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 Laurent\n\nOn 21/01/2025 17:17, Laurent Pinchart wrote:\n> Hi Dan,\n>\n> On Tue, Jan 21, 2025 at 04:30:19PM +0000, Daniel Scally wrote:\n>> On 21/01/2025 16:27, Laurent Pinchart wrote:\n>>> On Fri, Jan 17, 2025 at 02:34:09PM +0000, Daniel Scally wrote:\n>>>> Update the ExposureModeHelper class to compensate for flickering\n>>>> light sources in the ::splitExposure() function. The adjustment\n>>> ExposureModeHelper::splitExposure() or just splitExposure()\n>> The former :)\n>>>> simply caps exposure time at a multiple of the given flicker period\n>>>> and compensates for any loss in the effective exposure value by\n>>>> increasing analogue and then digital gain.\n>>>>\n>>>> Initially in the one call-site for this function, a flicker period\n>>>> of 0us is passed, making this a no-op.\n>>>>\n>>>> Signed-off-by: Daniel Scally \n>>>> ---\n>>>> src/ipa/libipa/agc_mean_luminance.cpp | 2 +-\n>>>> src/ipa/libipa/exposure_mode_helper.cpp | 20 +++++++++++++++++++-\n>>>> src/ipa/libipa/exposure_mode_helper.h | 2 +-\n>>>> 3 files changed, 21 insertions(+), 3 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n>>>> index 02555a44..b5e6afe3 100644\n>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n>>>> @@ -560,7 +560,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n>>>> \tnewExposureValue = filterExposure(newExposureValue);\n>>>> \n>>>> \tframeCount_++;\n>>>> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n>>>> +\treturn exposureModeHelper->splitExposure(newExposureValue, 0us);\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..038aa33c 100644\n>>>> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n>>>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n>>>> @@ -121,6 +121,7 @@ double ExposureModeHelper::clampGain(double gain) const\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 +148,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, utils::Duration flickerPeriod) const\n>>> We could avoid magic values by passing a std::optional.\n>>> What do you think ?\n>> Good idea\n>>>> {\n>>>> \tASSERT(maxExposureTime_);\n>>>> \tASSERT(maxGain_);\n>>>> @@ -205,6 +211,18 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>>>> \t * exposure time is maxed before gain is touched at all.\n>>>> \t */\n>>>> \texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n>>>> +\n>>>> +\t/*\n>>>> +\t * Finally, if we have been given a flicker period we need to reduce the\n>>>> +\t * exposure time to be a multiple of that period (if possible). The\n>>>> +\t * effect of this should be to hide the flicker.\n>>>> +\t */\n>>>> +\tif (flickerPeriod > 0us && flickerPeriod < exposureTime) {\n>>>> +\t\tunsigned int flickerPeriods = exposureTime / flickerPeriod;\n>>>> +\t\tif (flickerPeriods)\n>>>> +\t\t\texposureTime = flickerPeriods * flickerPeriod;\n>>>> +\t}\n>>> This means that the exposure time could become lower than\n>>> minExposureTime_. Is that an issue ?\n>> I doubt it given likely exposure times and flicker periods, but I can\n>> extend it to guarantee that doesn't happen.\n>>\n>>> Should we define what controls take\n>>> precedence in that case ? For instance, when manual exposure time is\n>>> enabled, should the flicker period be ignored ?\n>> Yes, and is. This only takes effect in automatic exposure mode.\n> Is it ? If exposure time is manual and gain is automatic, the above code\n> still gets run as far as I can tell.\nThis calculation still runs as is, but the rkisp1 and mali-c55 IPAs both take exposure time from a \ndifferent field in the IPA context if they are running in manual mode than the field this value is \nset to, and the IPU3 IPA has no handling of controls for AGC at all at the moment.\n>\n> The interactions between the flicker and AE controls also need to be\n> documented in the definition of the appropriate controls.\n>\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..a5fcc366 100644\n>>>> --- a/src/ipa/libipa/exposure_mode_helper.h\n>>>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n>>>> @@ -28,7 +28,7 @@ public:\n>>>> \t\t double minGain, double maxGain);\n>>>> \n>>>> \tstd::tuple\n>>>> -\tsplitExposure(utils::Duration exposure) const;\n>>>> +\tsplitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const;\n>>>> \n>>>> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n>>>> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }","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 9D072C3213\n\tfor ;\n\tTue, 21 Jan 2025 21:29:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A8F068558;\n\tTue, 21 Jan 2025 22:29:06 +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 E0E6360380\n\tfor ;\n\tTue, 21 Jan 2025 22:29:04 +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 461EFF0C;\n\tTue, 21 Jan 2025 22:28:02 +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=\"XaI3tqoC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737494882;\n\tbh=zDVkFLBu7l549IFfAJS21u/WRbGJVbTwNgldo/QS9rE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=XaI3tqoCKYjliEoe95weXxHKcAEeD4HQ/aaNnyDibo4wfr6HitR4Gtctxm1pikX2T\n\tFPf7KUE/BEFm0T4qpCwG03Ut2YtErHJpgVsp+JJkVOYn2UD5g+altfES+30tgKSrGr\n\txNwvXVbfYygxhjcMTl5WjlG6muhjFAwk+Pwe6CNg=","Message-ID":"<181ee622-b420-43a6-8ef1-06ddd2e9f680@ideasonboard.com>","Date":"Tue, 21 Jan 2025 21:29:01 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250117143410.20363-1-dan.scally@ideasonboard.com>\n\t<20250117143410.20363-3-dan.scally@ideasonboard.com>\n\t<20250121162741.GQ9249@pendragon.ideasonboard.com>\n\t\n\t<20250121171737.GR9249@pendragon.ideasonboard.com>","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":"<20250121171737.GR9249@pendragon.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\" "}},{"id":33147,"web_url":"https://patchwork.libcamera.org/comment/33147/","msgid":"<20250124003536.GB5963@pendragon.ideasonboard.com>","date":"2025-01-24T00:35:36","subject":"Re: [PATCH 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":"On Wed, Jan 22, 2025 at 11:31:29PM -0500, Paul Elder wrote:\n> On Wed, Jan 22, 2025 at 03:22:11PM +0200, Laurent Pinchart wrote:\n> > On Wed, Jan 22, 2025 at 01:15:43PM +0000, Daniel Scally wrote:\n> > > On 22/01/2025 13:09, Laurent Pinchart wrote:\n> > > > On Tue, Jan 21, 2025 at 09:29:01PM +0000, Daniel Scally wrote:\n> > > >> On 21/01/2025 17:17, Laurent Pinchart wrote:\n> > > >>> On Tue, Jan 21, 2025 at 04:30:19PM +0000, Daniel Scally wrote:\n> > > >>>> On 21/01/2025 16:27, Laurent Pinchart wrote:\n> > > >>>>> On Fri, Jan 17, 2025 at 02:34:09PM +0000, Daniel Scally wrote:\n> > > >>>>>> Update the ExposureModeHelper class to compensate for flickering\n> > > >>>>>> light sources in the ::splitExposure() function. The adjustment\n> > > >>>>> \n> > > >>>>> ExposureModeHelper::splitExposure() or just splitExposure()\n> > > >>>> \n> > > >>>> The former :)\n> > > >>>>\n> > > >>>>>> simply caps exposure time at a multiple of the given flicker period\n> > > >>>>>> and compensates for any loss in the effective exposure value by\n> > > >>>>>> increasing analogue and then digital gain.\n> > > >>>>>>\n> > > >>>>>> Initially in the one call-site for this function, a flicker period\n> > > >>>>>> of 0us is passed, making this a no-op.\n> > > >>>>>>\n> > > >>>>>> Signed-off-by: Daniel Scally \n> > > >>>>>> ---\n> > > >>>>>> src/ipa/libipa/agc_mean_luminance.cpp | 2 +-\n> > > >>>>>> src/ipa/libipa/exposure_mode_helper.cpp | 20 +++++++++++++++++++-\n> > > >>>>>> src/ipa/libipa/exposure_mode_helper.h | 2 +-\n> > > >>>>>> 3 files changed, 21 insertions(+), 3 deletions(-)\n> > > >>>>>>\n> > > >>>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > >>>>>> index 02555a44..b5e6afe3 100644\n> > > >>>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > >>>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > >>>>>> @@ -560,7 +560,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,\n> > > >>>>>> \tnewExposureValue = filterExposure(newExposureValue);\n> > > >>>>>> \n> > > >>>>>> \tframeCount_++;\n> > > >>>>>> -\treturn exposureModeHelper->splitExposure(newExposureValue);\n> > > >>>>>> +\treturn exposureModeHelper->splitExposure(newExposureValue, 0us);\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..038aa33c 100644\n> > > >>>>>> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> > > >>>>>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > > >>>>>> @@ -121,6 +121,7 @@ double ExposureModeHelper::clampGain(double gain) const\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 +148,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, utils::Duration flickerPeriod) const\n> > > >>>>> \n> > > >>>>> We could avoid magic values by passing a std::optional.\n> > > >>>>> What do you think ?\n> > > >>>> \n> > > >>>> Good idea\n> > > >>>>\n> > > >>>>>> {\n> > > >>>>>> \tASSERT(maxExposureTime_);\n> > > >>>>>> \tASSERT(maxGain_);\n> > > >>>>>> @@ -205,6 +211,18 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> > > >>>>>> \t * exposure time is maxed before gain is touched at all.\n> > > >>>>>> \t */\n> > > >>>>>> \texposureTime = clampExposureTime(exposure / clampGain(stageGain));\n> > > >>>>>> +\n> > > >>>>>> +\t/*\n> > > >>>>>> +\t * Finally, if we have been given a flicker period we need to reduce the\n> > > >>>>>> +\t * exposure time to be a multiple of that period (if possible). The\n> > > >>>>>> +\t * effect of this should be to hide the flicker.\n> > > >>>>>> +\t */\n> > > >>>>>> +\tif (flickerPeriod > 0us && flickerPeriod < exposureTime) {\n> > > >>>>>> +\t\tunsigned int flickerPeriods = exposureTime / flickerPeriod;\n> > > >>>>>> +\t\tif (flickerPeriods)\n> > > >>>>>> +\t\t\texposureTime = flickerPeriods * flickerPeriod;\n> > > >>>>>> +\t}\n> > > >>>>> \n> > > >>>>> This means that the exposure time could become lower than\n> > > >>>>> minExposureTime_. Is that an issue ?\n> > > >>>> \n> > > >>>> I doubt it given likely exposure times and flicker periods, but I can\n> > > >>>> extend it to guarantee that doesn't happen.\n> \n> I would also be more comfortable with a clamp.\n> \n> Also, do we not have to worry about cameras that have really high\n> framerate?\n\nIt depends what you mean by \"very high\". At the moment we assume that\nlibcamera can run all IPA algorithms per frame. That limits the frame\nrate. For high frames rates we'll have to reconsider that, and also\nlikely implement a batch API in V4L2.\n\n> > > >>>>> Should we define what controls take\n> > > >>>>> precedence in that case ? For instance, when manual exposure time is\n> > > >>>>> enabled, should the flicker period be ignored ?\n> > > >>>> \n> > > >>>> Yes, and is. This only takes effect in automatic exposure mode.\n> > > >>> \n> > > >>> Is it ? If exposure time is manual and gain is automatic, the above code\n> > > >>> still gets run as far as I can tell.\n> > > >> \n> > > >> This calculation still runs as is, but the rkisp1 and mali-c55 IPAs both take exposure time from a\n> > > >> different field in the IPA context if they are running in manual mode than the field this value is\n> > > >> set to, and the IPU3 IPA has no handling of controls for AGC at all at the moment.\n> > > > \n> > > > That doesn't seem right. It means the helpers will split exposure in\n> > > > exposure time and gain without knowing that one of the two will be\n> > > > overridden. It's not an issue with this patch, but I think it needs to\n> > > > be fixed.\n> > > \n> > > Sorry - I should have been clearer - analogue and digital gain are treated the same way. Either all \n> > > three sensor controls / ISP parameters are set from the manual values passed in through libcamera \n> > > controls, or all three values are set from the automatic values calculated here. I don't think there \n> > > would be a situation in which only one of the values calculated here was overridden.\n> > \n> > With the AEGC series that we merged recently, the exposure time mode can\n> > be set to manual and the gain mode to automatic.\n> > \n> > > > Paul, what do you think ?\n> \n> Ah, I see the problem, I forgot to plumb those sub-controls into the\n> exposure mode helper.\n> \n> I don't think this has to be addressed in this series though, and we can\n> do it on top (otherwise that fix and this series will start\n> conflicting).\n\nFine with me. Could you please coordinate with Dan to implement this in\nthe near future ?\n\n> > > >\n> > > >>> The interactions between the flicker and AE controls also need to be\n> > > >>> documented in the definition of the appropriate controls.\n> > > >>>\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..a5fcc366 100644\n> > > >>>>>> --- a/src/ipa/libipa/exposure_mode_helper.h\n> > > >>>>>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > > >>>>>> @@ -28,7 +28,7 @@ public:\n> > > >>>>>> \t\t double minGain, double maxGain);\n> > > >>>>>> \n> > > >>>>>> \tstd::tuple\n> > > >>>>>> -\tsplitExposure(utils::Duration exposure) const;\n> > > >>>>>> +\tsplitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const;\n> > > >>>>>> \n> > > >>>>>> \tutils::Duration minExposureTime() const { return minExposureTime_; }\n> > > >>>>>> \tutils::Duration maxExposureTime() const { return maxExposureTime_; }","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 DC15EC3200\n\tfor ;\n\tFri, 24 Jan 2025 00:35:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 052526855B;\n\tFri, 24 Jan 2025 01:35:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6991360353\n\tfor ;\n\tFri, 24 Jan 2025 01:35:45 +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 4822CC62;\n\tFri, 24 Jan 2025 01:34:41 +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=\"KSow/FC7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737678881;\n\tbh=8Wp74RpL4cZUnW3C21V4mo4dT4MWiy9V8DOb27mK8Z4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KSow/FC7ZOhg4pcAYdYbVQgLC23oizd1+hA/mniEaaTl6FgEh2iTET49BBkBqncd+\n\t33VMKStFvrpPsRLzPd0Sq9PxcIHXAjPEMHfSWrsiwfwgdVIqiUOM00U4w9jDXgjdPQ\n\t+qzhl7l0Dd5Gq9TSWKylqgKEImqhOpzXokWQH9Ns=","Date":"Fri, 24 Jan 2025 02:35:36 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Dan Scally ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] ipa: libipa: Adjust for flicker in\n\tExposureModeHelper","Message-ID":"<20250124003536.GB5963@pendragon.ideasonboard.com>","References":"<20250117143410.20363-1-dan.scally@ideasonboard.com>\n\t<20250117143410.20363-3-dan.scally@ideasonboard.com>\n\t<20250121162741.GQ9249@pendragon.ideasonboard.com>\n\t\n\t<20250121171737.GR9249@pendragon.ideasonboard.com>\n\t<181ee622-b420-43a6-8ef1-06ddd2e9f680@ideasonboard.com>\n\t<20250122130929.GD7077@pendragon.ideasonboard.com>\n\t\n\t<20250122132211.GE7077@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]