[PATCH] ipa: libipa: Fix bug in ExposureModeHelper that leads to oscillations in AEGC

[Dan Scally]

Hi Stefan

On 27/02/2025 19:58, Stefan Klug wrote:
> The ExposureModeHelper::splitExposures() runs through the configured
> stages to find the best gain/exposure time pair. It first raises the
> exposure time until it reaches the limit of the current stage. Then it
> raises the gain until that also reaches the limit of the current stage.
> After that it continues with the next stage until a match is found.
>
> Due to a slight mistake in the initial code, the second step doesn't
> work as expected because the exposure time gets divided by the gain of
> the current stage, effectively leading to a jump of the gain value from
> the maximum gain of the last stage to the maximum gain of the current
> stage instead of gradually increasing the gain value.
>
> Depending on the tuning file this leads to very visible oscillations and
> jumps in the brightness.
>
> Fix by clamping the exposure time in the second step to the maximum
> exposure time of the current stage.
Great catch!
> While at it, add two comments for easier understanding.


Honestly those two make the whole thing so much clearer...and so does directly specifying 
stageExposureTime.


Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>

>
> Fixes: 34c9ab62827b ("ipa: libipa: Add ExposureModeHelper")
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>   src/ipa/libipa/exposure_mode_helper.cpp | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> index f235316d3539..0c1e99e31a47 100644
> --- a/src/ipa/libipa/exposure_mode_helper.cpp
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -182,6 +182,7 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>   		 * the stage limits are initialised.
>   		 */
>   
> +		/* Clamp the gain to lastStageGain and regulate exposureTime. */
>   		if (stageExposureTime * lastStageGain >= exposure) {
>   			exposureTime = clampExposureTime(exposure / clampGain(lastStageGain));
>   			gain = clampGain(exposure / exposureTime);
> @@ -189,8 +190,9 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>   			return { exposureTime, gain, exposure / (exposureTime * gain) };
>   		}
>   
> +		/* Clamp the exposureTime to stageExposureTime and regulate gain. */
>   		if (stageExposureTime * stageGain >= exposure) {
> -			exposureTime = clampExposureTime(exposure / clampGain(stageGain));
> +			exposureTime = clampExposureTime(stageExposureTime);
>   			gain = clampGain(exposure / exposureTime);
>   
>   			return { exposureTime, gain, exposure / (exposureTime * gain) };