[1/2] ipa: simple: agc: Replace bang-bang controller with proportional

26238 diff mbox series

The AGC's updateExposure() uses a fixed ~10% step per frame regardless
of how far the current exposure is from optimal. With a hysteresis dead
band of only +/-4%, the controller overshoots when the correct value
falls within one step, causing visible brightness oscillation (flicker).

Replace the fixed-step bang-bang controller with a proportional one
where the correction factor scales linearly with the MSV error:

  factor = 1.0 + clamp(error * 0.04, -0.15, +0.15)

At maximum error (~2.5), this gives the same ~10% step as before. Near
the target, steps shrink to <1%, eliminating overshoot. The existing
hysteresis (kExposureSatisfactory) still prevents hunting on noise.

Tested on OV2740 behind Intel IPU6 ISYS (ThinkPad X1 Carbon Gen 10)
where the old controller produced continuous brightness flicker. The
proportional controller converges in ~3 seconds from cold start with
no visible oscillation.

Signed-off-by: Javier Tia <floss@jetm.me>
---
 src/ipa/simple/algorithms/agc.cpp | 73 +++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 24 deletions(-)

Hi Javier,

thank you for the patch.

Javier Tia <floss@jetm.me> writes:

> The AGC's updateExposure() uses a fixed ~10% step per frame regardless
> of how far the current exposure is from optimal. With a hysteresis dead
> band of only +/-4%, the controller overshoots when the correct value
> falls within one step, causing visible brightness oscillation (flicker).
>
> Replace the fixed-step bang-bang controller with a proportional one
> where the correction factor scales linearly with the MSV error:
>
>   factor = 1.0 + clamp(error * 0.04, -0.15, +0.15)
>
> At maximum error (~2.5), this gives the same ~10% step as before. Near
> the target, steps shrink to <1%, eliminating overshoot. The existing
> hysteresis (kExposureSatisfactory) still prevents hunting on noise.
>
> Tested on OV2740 behind Intel IPU6 ISYS (ThinkPad X1 Carbon Gen 10)
> where the old controller produced continuous brightness flicker. The
> proportional controller converges in ~3 seconds from cold start with
> no visible oscillation.

Nice change.  The patch looks good to me, let me test it in my
environment (later).

> Signed-off-by: Javier Tia <floss@jetm.me>
> ---
>  src/ipa/simple/algorithms/agc.cpp | 73 +++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 24 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> index 2f7e040c..a13a7552 100644
> --- a/src/ipa/simple/algorithms/agc.cpp
> +++ b/src/ipa/simple/algorithms/agc.cpp
> @@ -7,6 +7,8 @@
>  
>  #include "agc.h"
>  
> +#include <algorithm>
> +#include <cmath>
>  #include <stdint.h>
>  
>  #include <libcamera/base/log.h>
> @@ -37,52 +39,74 @@ static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>   */
>  static constexpr float kExposureSatisfactory = 0.2;
>  
> +/*
> + * Proportional gain for exposure/gain adjustment. Maps the MSV error to a
> + * multiplicative correction factor:
> + *
> + *   factor = 1.0 + kExpProportionalGain * error
> + *
> + * With kExpProportionalGain = 0.04:
> + *   - max error ~2.5 -> factor 1.10 (~10% step, same as before)
> + *   - error 1.0      -> factor 1.04 (~4% step)
> + *   - error 0.3      -> factor 1.012 (~1.2% step)
> + *
> + * This replaces the fixed 10% bang-bang step with a proportional correction
> + * that converges smoothly and avoids overshooting near the target.
> + */
> +static constexpr float kExpProportionalGain = 0.04;
> +
> +/*
> + * Maximum multiplicative step per frame, to bound the correction when the
> + * scene changes dramatically.
> + */
> +static constexpr float kExpMaxStep = 0.15;
> +
>  Agc::Agc()
>  {
>  }
>  
>  void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV)
>  {
> -	/*
> -	 * kExpDenominator of 10 gives ~10% increment/decrement;
> -	 * kExpDenominator of 5 - about ~20%
> -	 */
> -	static constexpr uint8_t kExpDenominator = 10;
> -	static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> -	static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
> -
>  	int32_t &exposure = frameContext.sensor.exposure;
>  	double &again = frameContext.sensor.gain;
>  
> -	if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
> +	double error = kExposureOptimal - exposureMSV;
> +
> +	if (std::abs(error) <= kExposureSatisfactory)
> +		return;
> +
> +	/*
> +	 * Compute a proportional correction factor. The sign of the error
> +	 * determines the direction: positive error means too dark (increase),
> +	 * negative means too bright (decrease).
> +	 */
> +	float step = std::clamp(static_cast<float>(error) * kExpProportionalGain,
> +				-kExpMaxStep, kExpMaxStep);
> +	float factor = 1.0f + step;
> +
> +	if (factor > 1.0f) {
> +		/* Scene too dark: increase exposure first, then gain. */
>  		if (exposure < context.configuration.agc.exposureMax) {
> -			int32_t next = exposure * kExpNumeratorUp / kExpDenominator;
> -			if (next - exposure < 1)
> -				exposure += 1;
> -			else
> -				exposure = next;
> +			int32_t next = static_cast<int32_t>(exposure * factor);
> +			exposure = std::max(next, exposure + 1);
>  		} else {
> -			double next = again * kExpNumeratorUp / kExpDenominator;
> +			double next = again * factor;
>  			if (next - again < context.configuration.agc.againMinStep)
>  				again += context.configuration.agc.againMinStep;
>  			else
>  				again = next;
>  		}
> -	}
> -
> -	if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
> +	} else {
> +		/* Scene too bright: decrease gain first, then exposure. */
>  		if (again > context.configuration.agc.again10) {
> -			double next = again * kExpNumeratorDown / kExpDenominator;
> +			double next = again * factor;
>  			if (again - next < context.configuration.agc.againMinStep)
>  				again -= context.configuration.agc.againMinStep;
>  			else
>  				again = next;
>  		} else {
> -			int32_t next = exposure * kExpNumeratorDown / kExpDenominator;
> -			if (exposure - next < 1)
> -				exposure -= 1;
> -			else
> -				exposure = next;
> +			int32_t next = static_cast<int32_t>(exposure * factor);
> +			exposure = std::min(next, exposure - 1);
>  		}
>  	}
>  
> @@ -96,6 +120,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>  
>  	LOG(IPASoftExposure, Debug)
>  		<< "exposureMSV " << exposureMSV
> +		<< " error " << error << " factor " << factor
>  		<< " exp " << exposure << " again " << again;
>  }