ipa: simple: Derive AGC minimum gain step from sensor granularity

28080 diff mbox series

The minimum gain step is computed as 1% of the sensor's gain range. For a
sensor with a wide gain range that is far larger than the hardware can
resolve: the OV01A10 spans 1-63.9961, giving a step of 0.629961, which is
63% at unity gain where the sensor resolves 1/256 = 0.0039.

updateExposure() substitutes againMinStep whenever the proportional
correction is smaller than it. Once the exposure saturates and gain is
the only remaining control, the AGC therefore applies a 63% change every
time it computes a 2% one, and never settles:

  again 1.00000 exposureMSV 1.99 error +0.51 factor 1.020 -> 1.62996
  again 1.62996 exposureMSV 2.90 error -0.41 factor 0.984 -> 1.00000

That is 0.7 EV of brightness flicker at frame rate, observed on a Dell
XPS 13 9315 (IPU6, simple pipeline and software ISP).

This is not specific to one sensor. Without a CameraSensorHelper the gain
values are raw register codes, where a step of 1.0 means one code and is
harmless; adding a helper switches the units and the same formula starts
producing most of a stop. Any sensor with a wide gain range is exposed to
this once it gains a helper.

Sample the step at both ends of the gain range and take the smaller one,
so that non-uniform gain models are covered as well. This yields
0.00390625 for the OV01A10, and the gain then tracks continuously
instead of alternating between two values.

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=2483190

Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Signed-off-by: Emilio Miranda <gemilio@gmail.com>
---
Based on v0.7.2 (base-commit below) rather than master, as discussed in
the thread, for a potential 0.7.3.

This implements the min-of-both-ends form Barnabás suggested. Checked
against every gain helper in tree: all 28 yield a non-zero step, and the
two ends of the range differ by up to ~1000x in both directions
(Vd55g1/Vd56g3 are finest at the top, Imx283 at the bottom), so sampling
only one end would have been wrong. AnalogueGainExp has no users in tree
at present.

Tested on the OV01A10 at 1280x720, where the exposure saturates and gain
becomes the only remaining control, against an unpatched build in the
same scene: the smallest correction the AGC can make drops from ~15% of
current gain to ~1.5%, median 19.4% -> 2.6%. Unpatched, the absolute
gain step has an interquartile range of exactly 162-323 codes, i.e. one
and two times againMinStep; patched, the step scales with the gain
level as it should.

Disclosure: I investigated this with an AI assistant (Claude). The
measurements are from my own hardware and I have verified them myself.

 src/ipa/simple/soft_simple.cpp | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)


base-commit: 191e202178f02430b5942397c70d215cdd2056fa

2026. 08. 25. 12:50 keltezéssel, Emilio Miranda írta:
> The minimum gain step is computed as 1% of the sensor's gain range. For a
> sensor with a wide gain range that is far larger than the hardware can
> resolve: the OV01A10 spans 1-63.9961, giving a step of 0.629961, which is
> 63% at unity gain where the sensor resolves 1/256 = 0.0039.
> 
> updateExposure() substitutes againMinStep whenever the proportional
> correction is smaller than it. Once the exposure saturates and gain is
> the only remaining control, the AGC therefore applies a 63% change every
> time it computes a 2% one, and never settles:
> 
>    again 1.00000 exposureMSV 1.99 error +0.51 factor 1.020 -> 1.62996
>    again 1.62996 exposureMSV 2.90 error -0.41 factor 0.984 -> 1.00000
> 
> That is 0.7 EV of brightness flicker at frame rate, observed on a Dell
> XPS 13 9315 (IPU6, simple pipeline and software ISP).
> 
> This is not specific to one sensor. Without a CameraSensorHelper the gain
> values are raw register codes, where a step of 1.0 means one code and is
> harmless; adding a helper switches the units and the same formula starts

I'd replace the "is harmless" part with "is by definition the smallest
possible change in gain" or something similar.


> producing most of a stop. Any sensor with a wide gain range is exposed to

  Could you clarify what is meant by "most of a stop" ?


> this once it gains a helper.
> 
> Sample the step at both ends of the gain range and take the smaller one,
> so that non-uniform gain models are covered as well. This yields
> 0.00390625 for the OV01A10, and the gain then tracks continuously
> instead of alternating between two values.
> 
> Reported at https://bugzilla.redhat.com/show_bug.cgi?id=2483190

You can add this as a trailer: `Link: https://bugzilla.redhat.com/show_bug.cgi?id=2483190`


> 
> Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Signed-off-by: Emilio Miranda <gemilio@gmail.com>
> ---
> Based on v0.7.2 (base-commit below) rather than master, as discussed in
> the thread, for a potential 0.7.3.
> 
> This implements the min-of-both-ends form Barnabás suggested. Checked
> against every gain helper in tree: all 28 yield a non-zero step, and the
> two ends of the range differ by up to ~1000x in both directions
> (Vd55g1/Vd56g3 are finest at the top, Imx283 at the bottom), so sampling
> only one end would have been wrong. AnalogueGainExp has no users in tree
> at present.

What do you mean `AnalogueGainExp` has no users?


> 
> Tested on the OV01A10 at 1280x720, where the exposure saturates and gain
> becomes the only remaining control, against an unpatched build in the
> same scene: the smallest correction the AGC can make drops from ~15% of
> current gain to ~1.5%, median 19.4% -> 2.6%. Unpatched, the absolute
> gain step has an interquartile range of exactly 162-323 codes, i.e. one
> and two times againMinStep; patched, the step scales with the gain
> level as it should.
> 
> Disclosure: I investigated this with an AI assistant (Claude). The
> measurements are from my own hardware and I have verified them myself.
> 
>   src/ipa/simple/soft_simple.cpp | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 629e1a32d..300806948 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <chrono>
> +#include <cmath>
>   #include <stdint.h>
>   #include <sys/mman.h>
>   
> @@ -228,10 +229,23 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>   		context_.configuration.agc.againMin = camHelper_->gain(againMin);
>   		context_.configuration.agc.againMax = camHelper_->gain(againMax);
>   		context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0);
> +		/*
> +		 * The minimum gain step must reflect what the sensor can
> +		 * actually resolve. Deriving it from a fraction of the gain
> +		 * range yields a huge step for sensors with a wide range: the
> +		 * OV01A10 spans 1-63.9961, giving 0.63 at unity gain where the
> +		 * hardware resolves 1/256. The AGC can then make no correction
> +		 * smaller than 63%, and oscillates around the target once the
> +		 * exposure saturates and gain is the only remaining control.
> +		 *
> +		 * Sample the step at both ends of the range and take the
> +		 * smaller one, so that non-uniform gain models are covered.
> +		 */
>   		context_.configuration.agc.againMinStep =
> -			(context_.configuration.agc.againMax -
> -			 context_.configuration.agc.againMin) /
> -			100.0;
> +			std::min(std::fabs(camHelper_->gain(againMin + 1) -
> +					   camHelper_->gain(againMin)),
> +				 std::fabs(camHelper_->gain(againMax) -
> +					   camHelper_->gain(againMax - 1)));

`<cmath>` provides `std::abs()` overloads for floating point types, so just `std::abs()` is fine.

Also, could you maybe break the lines up as follows:

		context_.configuration.agc.againMinStep = std::min(
			std::fabs(camHelper_->gain(againMin + 1) - camHelper_->gain(againMin),
			std::fabs(camHelper_->gain(againMax) - camHelper_->gain(againMax - 1));

It looks better in my opinion.


>   		if (camHelper_->blackLevel().has_value()) {
>   			/*
>   			 * The black level from camHelper_ is a 16 bit value, software ISP
> 
> base-commit: 191e202178f02430b5942397c70d215cdd2056fa

Thanks for the review.

> I'd replace the "is harmless" part with "is by definition the smallest
> possible change in gain" or something similar.

Done.

> Could you clarify what is meant by "most of a stop" ?

0.63 in linear gain at unity, so log2(1.63) = 0.70 EV. I have dropped the
phrasing rather than explain it.

> You can add this as a trailer: `Link: https://bugzilla.redhat.com/...`

Done.

> What do you mean `AnalogueGainExp` has no users?

I was wrong - my scan only matched numeric literals and the exponential
helpers are written as expGainDb(0.3), so they did not match. At v0.7.2 there
are five: Imx290, Imx296, Imx335, Imx415, Imx678.

They are handled: for an exponential model the step grows with the code, so
min() takes the bottom of the range and it is always non-zero. For Imx290
(0.3 dB steps) that is 0.035 at the bottom against ~135 at the top, so the
min-of-both-ends form matters more for those than for the rational ones.

> `<cmath>` provides `std::abs()` overloads for floating point types

Done.

> Also, could you maybe break the lines up as follows

Done. clang-format leaves it as written (ColumnLimit is 0), so it stays in
that shape.