[v5,1/3] ipa: simple: agc: Replace bang-bang controller with proportional
diff mbox series

Message ID 20260506221942.55C4F1EA006B@mailuser.phl.internal
State Accepted
Headers show
Series
  • ipa: simple: Proportional AGC and statistics normalization
Related show

Commit Message

Javier Tia May 6, 2026, 9:46 p.m. UTC 
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 step is
clamped to +/-15% to bound corrections when the scene changes
dramatically. 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>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/simple/algorithms/agc.cpp | 73 +++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 24 deletions(-)

Comments

Hans de Goede May 7, 2026, 1:16 p.m. UTC | #1 
Hi,

On 6-May-26 23:46, Javier Tia wrote:
> 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 step is
> clamped to +/-15% to bound corrections when the scene changes
> dramatically. 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>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  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;
>  }
>
Laurent Pinchart May 7, 2026, 2:33 p.m. UTC | #2 
Hi Javier,

Thank you for the patch.

On Wed, May 06, 2026 at 03:46:01PM -0600, Javier Tia wrote:
> 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 step is
> clamped to +/-15% to bound corrections when the scene changes
> dramatically. 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.

I'm fine with this, but I'd like to reiterate that Kieran is working on
moving the soft ISP IPA to use the AGC algorithm from libipa. That may
then cause a regression when this code will be replaced.

Kieran, could you try and remember to CC Javier when you will post that
series, to ensure it gets tested properly ?

> Signed-off-by: Javier Tia <floss@jetm.me>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  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;
>  }
>

Patch
diff mbox series 

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;
 }