[v4,1/3] ipa: simple: agc: Replace bang-bang controller with proportional

26267 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 + error * 0.04

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>
Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/simple/algorithms/agc.cpp | 65 ++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 24 deletions(-)

Milan, Hans,

If you could check my thoughts below - if you think this is still ok
without a correct gain model I'll be ok merging this (pending Javier's
rebase)



Quoting Javier Tia (2026-03-06 18:46:40)
> 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 + error * 0.04
> 
> 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>
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/simple/algorithms/agc.cpp | 65 ++++++++++++++++++++++++---------------
>  1 file changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> index 2f7e040c..ac977d5f 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,66 @@ 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;
> +
>  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 factor = 1.0f + static_cast<float>(error) * kExpProportionalGain;
> +
> +       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;

Does this still work when we don't have a gain model for the device ?

Supporting missing gain models (where the gain code is just an arbitrary
register value, and not a linear gain value) is the only reason the
simple pipeline handler has a separate AGC implementation from the one
in libipa ...

I'm happy enough to merge this - as a step towards unifying things
though, but it would be helpful if someone with a device that doesn't
have a gain model could test and make sure this isn't just fixing one
set of devices by breaking the actual target use case for keeping this
as a separate algorithm...



>                         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 +112,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>  
>         LOG(IPASoftExposure, Debug)
>                 << "exposureMSV " << exposureMSV
> +               << " error " << error << " factor " << factor
>                 << " exp " << exposure << " again " << again;
>  }
>  
> 
> -- 
> 2.53.0
>

Hi Kieran, Milan, Hans,

On the gain model question: the proportional step `again * factor`
has the same linearity assumption as the old fixed step
`again * 11/10`. Both multiply the current register value by a
scalar. For devices without a calibrated gain model, the register
value was already being used as a linear proxy in the original code.
This change only affects how aggressively the step adjusts -
narrower steps near target - not the direction logic or the
underlying gain model assumption.

Devices without a gain model should behave the same or better:
smaller steps near the target reduce the risk of overshoot, which is
exactly the problem the proportional controller was designed to fix.

v5 has been sent:
https://lists.libcamera.org/pipermail/libcamera-devel/2026-May/058560.html

Best,
Javier