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

Message ID 20260305-agc-proportional-v3-1-25abc1bfacca@jetm.me
State Superseded
Headers show
Series
  • Simple pipeline: proportional AGC, AWB stats fix, OV2740 black level
Related show

Commit Message

Javier Tia March 5, 2026, 8:10 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 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>
---
 src/ipa/simple/algorithms/agc.cpp | 73 ++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 24 deletions(-)

Comments

Barnabás Pőcze March 6, 2026, 3:21 p.m. UTC | #1 
Hi

2026. 03. 05. 21:10 keltezéssel, Javier Tia írta:
> 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>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.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;

I suppose this is very subjective, but in my testing I felt that this was a bit slow.
In any case, it's probably fine, especially for "web camera" usage.


> +
> +/*
> + * Maximum multiplicative step per frame, to bound the correction when the
> + * scene changes dramatically.
> + */
> +static constexpr float kExpMaxStep = 0.15;

2.5 * 0.04 = 0.1, which is smaller than `kExpMaxStep`. Is this necessary?


Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # ThinkPad X1 Yoga Gen 7 + ov2740

> +
>   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;
>   }
>   
>
Milan Zamazal March 6, 2026, 3:39 p.m. UTC | #2 
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2026. 03. 05. 21:10 keltezéssel, Javier Tia írta:
>> 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>
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.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;
>
> I suppose this is very subjective, but in my testing I felt that this was a bit slow.

I have the same impression.

Well, changing the constant any time later is easy, a harder part might
be to agree on a good value.

> In any case, it's probably fine, especially for "web camera" usage.
>
>
>> +
>> +/*
>> + * Maximum multiplicative step per frame, to bound the correction when the
>> + * scene changes dramatically.
>> + */
>> +static constexpr float kExpMaxStep = 0.15;
>
> 2.5 * 0.04 = 0.1, which is smaller than `kExpMaxStep`. Is this necessary?
>
>
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # ThinkPad X1 Yoga Gen 7 + ov2740
>
>> +
>>   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;
 }