| Message ID | 20260825120632.167143-1-gemilio@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series |
|
| Related | show |
Quoting Emilio Miranda (2026-08-25 13:06:30) > 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 is by definition the > smallest possible change in gain; adding a helper switches the units to > real multipliers, and 1% of the range then bears no relation to what the > sensor can resolve. 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. > > 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> > --- > Changes since v1: > - use std::abs, and reflow the expression as suggested > - reword the "is harmless" sentence; drop "most of a stop" > - Link: trailer instead of the inline URL > > Base and testing unchanged from v1: v0.7.2, tested on the OV01A10 at > 1280x720 against an unpatched build in the same scene. > > 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, 16 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 629e1a32d..8bffd2a44 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,21 @@ 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); > - context_.configuration.agc.againMinStep = > - (context_.configuration.agc.againMax - > - context_.configuration.agc.againMin) / > - 100.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 = std::min( > + std::abs(camHelper_->gain(againMin + 1) - camHelper_->gain(againMin)), > + std::abs(camHelper_->gain(againMax) - camHelper_->gain(againMax - 1))); Does this work when there's no camHelper ? Do we default to a camHelper with a 1:1 gain model now, or otherwise guarantee we have a valid camHelper_ here ? > if (camHelper_->blackLevel().has_value()) { Of course looking here, we 'must' have a camHelper_ ? -- Kieran > /* > * The black level from camHelper_ is a 16 bit value, software ISP > > base-commit: 191e202178f02430b5942397c70d215cdd2056fa > -- > 2.55.0 >
2026. 08. 25. 14:26 keltezéssel, Kieran Bingham írta: > Quoting Emilio Miranda (2026-08-25 13:06:30) >> 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 is by definition the >> smallest possible change in gain; adding a helper switches the units to >> real multipliers, and 1% of the range then bears no relation to what the >> sensor can resolve. 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. >> >> 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> >> --- >> Changes since v1: >> - use std::abs, and reflow the expression as suggested >> - reword the "is harmless" sentence; drop "most of a stop" >> - Link: trailer instead of the inline URL >> >> Base and testing unchanged from v1: v0.7.2, tested on the OV01A10 at >> 1280x720 against an unpatched build in the same scene. >> >> 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, 16 insertions(+), 4 deletions(-) >> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index 629e1a32d..8bffd2a44 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,21 @@ 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); >> - context_.configuration.agc.againMinStep = >> - (context_.configuration.agc.againMax - >> - context_.configuration.agc.againMin) / >> - 100.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 = std::min( >> + std::abs(camHelper_->gain(againMin + 1) - camHelper_->gain(againMin)), >> + std::abs(camHelper_->gain(againMax) - camHelper_->gain(againMax - 1))); > > > Does this work when there's no camHelper ? Do we default to a camHelper > with a 1:1 gain model now, or otherwise guarantee we have a valid > camHelper_ here ? This whole block is only entered `if (camHelper_) {`. The unknown sensor case - handled in the `else` block below - will use gain codes directly with 1 as the minimum step. > > >> if (camHelper_->blackLevel().has_value()) { > > Of course looking here, we 'must' have a camHelper_ ? > > -- > Kieran > >> /* >> * The black level from camHelper_ is a 16 bit value, software ISP >> >> base-commit: 191e202178f02430b5942397c70d215cdd2056fa >> -- >> 2.55.0 >>
Hi, thanks for the patch! In the softISP call we just came to the conclusion that there probably won't be a 0.7.3 release and that distros should just cherry-pick the patch on top of 0.7.2. Milan said he'd make a release for Fedora 44 and 45 and I'll file an issue for Ubuntu. Barnabás, are you fine with the patch as is (would you give a R-B or A-B)? Best regards, Robert On 25.08.26 14:06, Emilio Miranda wrote: > 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 is by definition the > smallest possible change in gain; adding a helper switches the units to > real multipliers, and 1% of the range then bears no relation to what the > sensor can resolve. 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. > > 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> > --- > Changes since v1: > - use std::abs, and reflow the expression as suggested > - reword the "is harmless" sentence; drop "most of a stop" > - Link: trailer instead of the inline URL > > Base and testing unchanged from v1: v0.7.2, tested on the OV01A10 at > 1280x720 against an unpatched build in the same scene. > > 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, 16 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 629e1a32d..8bffd2a44 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,21 @@ 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); > - context_.configuration.agc.againMinStep = > - (context_.configuration.agc.againMax - > - context_.configuration.agc.againMin) / > - 100.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 = std::min( > + std::abs(camHelper_->gain(againMin + 1) - camHelper_->gain(againMin)), > + std::abs(camHelper_->gain(againMax) - camHelper_->gain(againMax - 1))); > if (camHelper_->blackLevel().has_value()) { > /* > * The black level from camHelper_ is a 16 bit value, software ISP > > base-commit: 191e202178f02430b5942397c70d215cdd2056fa
> Barnabás, are you fine with the patch as is (would you give a R-B or A-B)? Before you spend time on that, please don't - I would like to withdraw the patch. I saw Laurent's reply in the OV32C4 thread about the AI policy being drafted. My patch carries an AI disclosure too: the diagnosis and every measurement are mine, but the code and the commit message were drafted with an assistant. I am not a C++ developer, and I am not going to resubmit it as hand-written work when I would not be comfortable defending it line by line. The fix itself is Barnabás's anyway - he specified it in the v1 review, and it is three lines. Anyone on the list can write it properly. I have no interest in authorship here. What I can offer is the hardware. As far as I know this is the only OV01A10 in the conversation, and I am happy to test whatever anyone writes: the failure reproduces reliably at 1280x720, where the exposure saturates and gain becomes the only remaining control. The bug report stands on its own, and I will keep the local build running here in the meantime. That said, sorry for the churn - and for what it's worth, I now have a working webcam instead of a disco party. Best regards, Emilio El mié, 26 ago 2026 a las 13:04, Robert Mader (<robert.mader@collabora.com>) escribió: > > Hi, thanks for the patch! > > In the softISP call we just came to the conclusion that there probably > won't be a 0.7.3 release and that distros should just cherry-pick the > patch on top of 0.7.2. Milan said he'd make a release for Fedora 44 and > 45 and I'll file an issue for Ubuntu. > > Barnabás, are you fine with the patch as is (would you give a R-B or A-B)? > > Best regards, > > Robert > > On 25.08.26 14:06, Emilio Miranda wrote: > > 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 is by definition the > > smallest possible change in gain; adding a helper switches the units to > > real multipliers, and 1% of the range then bears no relation to what the > > sensor can resolve. 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. > > > > 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> > > --- > > Changes since v1: > > - use std::abs, and reflow the expression as suggested > > - reword the "is harmless" sentence; drop "most of a stop" > > - Link: trailer instead of the inline URL > > > > Base and testing unchanged from v1: v0.7.2, tested on the OV01A10 at > > 1280x720 against an unpatched build in the same scene. > > > > 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, 16 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > > index 629e1a32d..8bffd2a44 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,21 @@ 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); > > - context_.configuration.agc.againMinStep = > > - (context_.configuration.agc.againMax - > > - context_.configuration.agc.againMin) / > > - 100.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 = std::min( > > + std::abs(camHelper_->gain(againMin + 1) - camHelper_->gain(againMin)), > > + std::abs(camHelper_->gain(againMax) - camHelper_->gain(againMax - 1))); > > if (camHelper_->blackLevel().has_value()) { > > /* > > * The black level from camHelper_ is a 16 bit value, software ISP > > > > base-commit: 191e202178f02430b5942397c70d215cdd2056fa
Quoting Emilio Miranda (2026-08-27 12:49:21) > > Barnabás, are you fine with the patch as is (would you give a R-B or A-B)? > > Before you spend time on that, please don't - I would like to withdraw the > patch. > > I saw Laurent's reply in the OV32C4 thread about the AI policy being drafted. > My patch carries an AI disclosure too: the diagnosis and every measurement are > mine, but the code and the commit message were drafted with an assistant. I am > not a C++ developer, and I am not going to resubmit it as hand-written work > when I would not be comfortable defending it line by line. Don't worry I don't think you need to. The aim here is that this patch will be potentially collected by the distributions (Fedora/Debian) to track on their libcamera-0.7.2 release as a single/short 'fix'. We're not aiming to apply this patch to mainline libcamera as we have just replaced the AGC algorithm, and the code impacted here with the new libipa rework. Please do test on mainline libcamera to see if this has also resolved your issue! A tag from Barnabas is potentially helpful to confirm to the distributions that the patch is a valid fix, but it's up to them to decide if they apply it or not. -- Kieran > > The fix itself is Barnabás's anyway - he specified it in the v1 review, and it > is three lines. Anyone on the list can write it properly. I have no interest in > authorship here. > > What I can offer is the hardware. As far as I know this is the only OV01A10 in > the conversation, and I am happy to test whatever anyone writes: the failure > reproduces reliably at 1280x720, where the exposure saturates and gain becomes > the only remaining control. > > The bug report stands on its own, and I will keep the local build running here > in the meantime. > > That said, sorry for the churn - and for what it's worth, I now have a working > webcam instead of a disco party. > > Best regards, > > Emilio > > El mié, 26 ago 2026 a las 13:04, Robert Mader > (<robert.mader@collabora.com>) escribió: > > > > Hi, thanks for the patch! > > > > In the softISP call we just came to the conclusion that there probably > > won't be a 0.7.3 release and that distros should just cherry-pick the > > patch on top of 0.7.2. Milan said he'd make a release for Fedora 44 and > > 45 and I'll file an issue for Ubuntu. > > > > Barnabás, are you fine with the patch as is (would you give a R-B or A-B)? > > > > Best regards, > > > > Robert > > > > On 25.08.26 14:06, Emilio Miranda wrote: > > > 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 is by definition the > > > smallest possible change in gain; adding a helper switches the units to > > > real multipliers, and 1% of the range then bears no relation to what the > > > sensor can resolve. 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. > > > > > > 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> > > > --- > > > Changes since v1: > > > - use std::abs, and reflow the expression as suggested > > > - reword the "is harmless" sentence; drop "most of a stop" > > > - Link: trailer instead of the inline URL > > > > > > Base and testing unchanged from v1: v0.7.2, tested on the OV01A10 at > > > 1280x720 against an unpatched build in the same scene. > > > > > > 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, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > > > index 629e1a32d..8bffd2a44 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,21 @@ 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); > > > - context_.configuration.agc.againMinStep = > > > - (context_.configuration.agc.againMax - > > > - context_.configuration.agc.againMin) / > > > - 100.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 = std::min( > > > + std::abs(camHelper_->gain(againMin + 1) - camHelper_->gain(againMin)), > > > + std::abs(camHelper_->gain(againMax) - camHelper_->gain(againMax - 1))); > > > if (camHelper_->blackLevel().has_value()) { > > > /* > > > * The black level from camHelper_ is a 16 bit value, software ISP > > > > > > base-commit: 191e202178f02430b5942397c70d215cdd2056fa > > > > -- > Urci, 31 de Junio del año 239 a.C.
Thanks for clarifying - I was worried I had made unnecessary work for everyone. > Please do test on mainline libcamera to see if this has also resolved > your issue! Tested on master at 63f9b4e5d (v0.7.2+120), built with -Dipas=softisp, on the OV01A10. It resolves it. Method: forced 1280x720 so the exposure saturates and gain becomes the only remaining control, which is the condition that triggers the problem - at 1284x812 there is exposure headroom and nothing looks wrong. Sampled V4L2_CID_ANALOGUE_GAIN at 20 Hz for 30 s while covering and uncovering the lens, against the same measurement on master before the rework. steps <2% corrections at the 161-code quantum master before the rework 189 4.2% 50.8% master with the rework 224 81.7% 0.0% master with the rework 200 3.5% 1.0% 161 codes is the old againMinStep (0.629961) in register units. Before the rework half of every gain movement was that quantum or a multiple of it, and the smallest correction the loop could make was ~15% of current gain. It is gone. The two runs of the reworked build differ because I perturbed the second one harder: it spends most of its time recovering from a fully covered lens, so the steps are large by necessity, and the gain climbs geometrically (1009 -> 1172 -> 1385 -> 1637 -> 1906 -> 2223 ...) rather than in quantised jumps. The first run is closer to a settled scene, where 82% of corrections are under 2% of gain and the smallest is a single gain code. Two notes on what this does and does not cover. Master has no ov01a10.yaml, so it falls back to uncalibrated.yaml - this tests the AGC and not my local tuning. And the "No sensor delays found in static properties" warning is still there, since ov01a10 has no entry in camera_sensor_properties.cpp; it does not appear to affect this. Happy to run anything else on this hardware.
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index 629e1a32d..8bffd2a44 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,21 @@ 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); - context_.configuration.agc.againMinStep = - (context_.configuration.agc.againMax - - context_.configuration.agc.againMin) / - 100.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 = std::min( + std::abs(camHelper_->gain(againMin + 1) - camHelper_->gain(againMin)), + std::abs(camHelper_->gain(againMax) - camHelper_->gain(againMax - 1))); if (camHelper_->blackLevel().has_value()) { /* * The black level from camHelper_ is a 16 bit value, software ISP
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 is by definition the smallest possible change in gain; adding a helper switches the units to real multipliers, and 1% of the range then bears no relation to what the sensor can resolve. 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. 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> --- Changes since v1: - use std::abs, and reflow the expression as suggested - reword the "is harmless" sentence; drop "most of a stop" - Link: trailer instead of the inline URL Base and testing unchanged from v1: v0.7.2, tested on the OV01A10 at 1280x720 against an unpatched build in the same scene. 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, 16 insertions(+), 4 deletions(-) base-commit: 191e202178f02430b5942397c70d215cdd2056fa