| Message ID | 20250923190657.21453-4-hansg@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, thank you for the patch. Hans de Goede <hansg@kernel.org> writes: > Raise either exposure or gain; not both at the same time. I think there can still be a reason to raise both, although differently than in the original code, see below. > Before this change when the last exposure raise is done, hitting > exposure-max, gain would be increased at the same time. > > Signed-off-by: Hans de Goede <hansg@kernel.org> > --- > src/ipa/simple/algorithms/agc.cpp | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index 94961f9fe..230616e62 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -56,12 +56,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > double &again = frameContext.sensor.gain; > > if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { > - next = exposure * kExpNumeratorUp / kExpDenominator; > - if (next - exposure < 1) > - exposure += 1; > - else > - exposure = next; > - if (exposure >= context.configuration.agc.exposureMax) { > + if (exposure < context.configuration.agc.exposureMax) { > + next = exposure * kExpNumeratorUp / kExpDenominator; > + if (next - exposure < 1) > + exposure += 1; > + else > + exposure = next; What if next > context.configuration.agc.exposureMax? Shouldn't exposure be limited to context.configuration.agc.exposureMax and gain still increased proportionally, for the missing part? > + } else { > next = again * kExpNumeratorUp / kExpDenominator; > if (next - again < context.configuration.agc.againMinStep) > again += context.configuration.agc.againMinStep;
Hi Milan, On 24-Sep-25 10:09, Milan Zamazal wrote: > Hi Hans, > > thank you for the patch. > > Hans de Goede <hansg@kernel.org> writes: > >> Raise either exposure or gain; not both at the same time. > > I think there can still be a reason to raise both, although differently > than in the original code, see below. > >> Before this change when the last exposure raise is done, hitting >> exposure-max, gain would be increased at the same time. >> >> Signed-off-by: Hans de Goede <hansg@kernel.org> >> --- >> src/ipa/simple/algorithms/agc.cpp | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index 94961f9fe..230616e62 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -56,12 +56,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >> double &again = frameContext.sensor.gain; >> >> if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { >> - next = exposure * kExpNumeratorUp / kExpDenominator; >> - if (next - exposure < 1) >> - exposure += 1; >> - else >> - exposure = next; >> - if (exposure >= context.configuration.agc.exposureMax) { >> + if (exposure < context.configuration.agc.exposureMax) { >> + next = exposure * kExpNumeratorUp / kExpDenominator; >> + if (next - exposure < 1) >> + exposure += 1; >> + else >> + exposure = next; > > What if next > context.configuration.agc.exposureMax? Shouldn't > exposure be limited to context.configuration.agc.exposureMax and gain > still increased proportionally, for the missing part? I assume you mean that if when trying to raise 10% we only end up raising say 4%, then raise gain by the remaining 6%. I'm not in favor of doing that for 2 reasons: 1. It seems to me that then can cause some instabilities / oscillations due to the step ending up bigger then expected, esp. for sensors without sensor-helper code where we do not know the gain model so a 5% increase of the register value may lead to a bigger step then 5% 2. This will add a bunch of non trivial code just to avoid a possible slowdown of hitting the brightness target by a single control-algo run, which just does not seem worth it. Regards, Hans > >> + } else { >> next = again * kExpNumeratorUp / kExpDenominator; >> if (next - again < context.configuration.agc.againMinStep) >> again += context.configuration.agc.againMinStep; >
Hans de Goede <hansg@kernel.org> writes: > Hi Milan, > > On 24-Sep-25 10:09, Milan Zamazal wrote: >> Hi Hans, >> >> thank you for the patch. >> >> Hans de Goede <hansg@kernel.org> writes: >> >>> Raise either exposure or gain; not both at the same time. >> >> I think there can still be a reason to raise both, although differently >> than in the original code, see below. >> >>> Before this change when the last exposure raise is done, hitting >>> exposure-max, gain would be increased at the same time. >>> >>> Signed-off-by: Hans de Goede <hansg@kernel.org> >>> --- >>> src/ipa/simple/algorithms/agc.cpp | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >>> index 94961f9fe..230616e62 100644 >>> --- a/src/ipa/simple/algorithms/agc.cpp >>> +++ b/src/ipa/simple/algorithms/agc.cpp >>> @@ -56,12 +56,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >>> double &again = frameContext.sensor.gain; >>> >>> if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { >>> - next = exposure * kExpNumeratorUp / kExpDenominator; >>> - if (next - exposure < 1) >>> - exposure += 1; >>> - else >>> - exposure = next; >>> - if (exposure >= context.configuration.agc.exposureMax) { >>> + if (exposure < context.configuration.agc.exposureMax) { >>> + next = exposure * kExpNumeratorUp / kExpDenominator; >>> + if (next - exposure < 1) >>> + exposure += 1; >>> + else >>> + exposure = next; >> >> What if next > context.configuration.agc.exposureMax? Shouldn't >> exposure be limited to context.configuration.agc.exposureMax and gain >> still increased proportionally, for the missing part? > > I assume you mean that if when trying to raise 10% we only end up raising > say 4%, then raise gain by the remaining 6%. Yes. > I'm not in favor of doing that for 2 reasons: > > 1. It seems to me that then can cause some instabilities / oscillations > due to the step ending up bigger then expected, esp. for sensors without > sensor-helper code where we do not know the gain model so a 5% increase > of the register value may lead to a bigger step then 5% But this already applies when the exposure is at max and we have to change the gain, doesn't it? > 2. This will add a bunch of non trivial code just to avoid a possible slowdown > of hitting the brightness target by a single control-algo run, which just > does not seem worth it. That's a fair point. Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > Regards, > > Hans > > > > >> >>> + } else { >>> next = again * kExpNumeratorUp / kExpDenominator; >>> if (next - again < context.configuration.agc.againMinStep) >>> again += context.configuration.agc.againMinStep; >>
diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index 94961f9fe..230616e62 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -56,12 +56,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou double &again = frameContext.sensor.gain; if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { - next = exposure * kExpNumeratorUp / kExpDenominator; - if (next - exposure < 1) - exposure += 1; - else - exposure = next; - if (exposure >= context.configuration.agc.exposureMax) { + if (exposure < context.configuration.agc.exposureMax) { + next = exposure * kExpNumeratorUp / kExpDenominator; + if (next - exposure < 1) + exposure += 1; + else + exposure = next; + } else { next = again * kExpNumeratorUp / kExpDenominator; if (next - again < context.configuration.agc.againMinStep) again += context.configuration.agc.againMinStep;
Raise either exposure or gain; not both at the same time. Before this change when the last exposure raise is done, hitting exposure-max, gain would be increased at the same time. Signed-off-by: Hans de Goede <hansg@kernel.org> --- src/ipa/simple/algorithms/agc.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)