| Message ID | 20260810103846.1075936-48-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Quoting Barnabás Pőcze (2026-08-10 12:38:43) > The `frameContext` is of an already completed frame, its exposure/gain/etc > is of no real concern for future frames, so take the most recent settings > from the active state. I think it was already discussed elsewhere that we should postpone that change. Overall you are (mostly) right. The problem is that the current logic uses the current frame context (of a frame that was already produced) to create values which are sent to the sensor for some upcoming frame. Just using active state is in some cases better, but not necessarily in all cases right. Assume you queued in x requests with manual gain and alternating gain values of 1.0 and 3.0 on even/odd frames. In that case, the active state will give you the values of the last request queued in, which is not what was requested. The way I want to solve it in the regulation rework is to change the semantics as follows: process() only updates the active state/regulation of the algorithms and fills metadata from the frame context. prepare() is called with a lookahead, so that prepare(N) is actually called early enough so that the values set in the frame context are actually sent to the sensor in time for frame N. Prepare() uses the values from active state to calculate the sensor controls. This way the system becomes easier to mentally follow. There will still be some edge cases like exposureValue which is handled inside process() even though in theory it should be handled in prepare(). The thing there is that process is allowed to take more time than prepare() which is expected to run super fast. But these are edge cases we can improve on later. Best regards, Stefan > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/libipa/agc.cpp | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp > index bc57179d26..eefd7ef712 100644 > --- a/src/ipa/libipa/agc.cpp > +++ b/src/ipa/libipa/agc.cpp > @@ -623,22 +623,22 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state, > double minAnalogueGain; > double maxAnalogueGain; > > - if (frameContext.autoExposureEnabled) { > + if (state.autoExposureEnabled) { > minExposureTime = session.minExposureTime; > - maxExposureTime = std::clamp(frameContext.maxFrameDuration, > + maxExposureTime = std::clamp(state.maxFrameDuration, > session.minExposureTime, > session.maxExposureTime); > } else { > - minExposureTime = lineDuration * frameContext.exposure; > + minExposureTime = lineDuration * state.manual.exposure; > maxExposureTime = minExposureTime; > } > > - if (frameContext.autoGainEnabled) { > + if (state.autoGainEnabled) { > minAnalogueGain = session.minAnalogueGain; > maxAnalogueGain = session.maxAnalogueGain; > } else { > - minAnalogueGain = frameContext.gain; > - maxAnalogueGain = frameContext.gain; > + minAnalogueGain = state.manual.gain; > + maxAnalogueGain = state.manual.gain; > } > > std::visit(utils::overloaded{ > @@ -683,10 +683,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state, > .traits = params->traits, > .yHist = params->yHist, > .effectiveExposureValue = effectiveExposureValue, > - .constraintModeIndex = frameContext.constraintMode, > - .exposureModeIndex = frameContext.exposureMode, > + .constraintModeIndex = state.constraintMode, > + .exposureModeIndex = state.exposureMode, > .lux = params->lux, > - .exposureCompensation = pow(2.0, frameContext.exposureValue), > + .exposureCompensation = pow(2.0, state.exposureValue), > }); > > /* Update the estimated exposure and gain. */ > -- > 2.55.0 >
2026. 08. 12. 11:49 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Quoting Barnabás Pőcze (2026-08-10 12:38:43) >> The `frameContext` is of an already completed frame, its exposure/gain/etc >> is of no real concern for future frames, so take the most recent settings >> from the active state. > > I think it was already discussed elsewhere that we should postpone that Yes. > change. Overall you are (mostly) right. The problem is that the current > logic uses the current frame context (of a frame that was already > produced) to create values which are sent to the sensor for some > upcoming frame. Just using active state is in some cases better, but not > necessarily in all cases right. Assume you queued in x requests with > manual gain and alternating gain values of 1.0 and 3.0 on even/odd > frames. In that case, the active state will give you the values of the > last request queued in, which is not what was requested. > > The way I want to solve it in the regulation rework is to change the > semantics as follows: > > process() only updates the active state/regulation of the algorithms and > fills metadata from the frame context. Yes, although at this point I don't exactly see how only updating the active state in `process()` will work out. I would argue that conceptually the algorithm has to run for frame X only after the limits and other manual parameters have been finalized for frame X. It cannot run before that because then by definition it might not use the e.g. the expected frame duration limits, constraint mode, etc. Anyways, this is a sidenote. > > prepare() is called with a lookahead, so that prepare(N) is actually > called early enough so that the values set in the frame context are > actually sent to the sensor in time for frame N. Prepare() uses the values > from active state to calculate the sensor controls. > > This way the system becomes easier to mentally follow. > > There will still be some edge cases like exposureValue which is handled > inside process() even though in theory it should be handled in prepare(). > The thing there is that process is allowed to take more time than > prepare() which is expected to run super fast. But these are edge cases > we can improve on later. > > Best regards, > Stefan > > >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/ipa/libipa/agc.cpp | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp >> index bc57179d26..eefd7ef712 100644 >> --- a/src/ipa/libipa/agc.cpp >> +++ b/src/ipa/libipa/agc.cpp >> @@ -623,22 +623,22 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state, >> double minAnalogueGain; >> double maxAnalogueGain; >> >> - if (frameContext.autoExposureEnabled) { >> + if (state.autoExposureEnabled) { >> minExposureTime = session.minExposureTime; >> - maxExposureTime = std::clamp(frameContext.maxFrameDuration, >> + maxExposureTime = std::clamp(state.maxFrameDuration, >> session.minExposureTime, >> session.maxExposureTime); >> } else { >> - minExposureTime = lineDuration * frameContext.exposure; >> + minExposureTime = lineDuration * state.manual.exposure; >> maxExposureTime = minExposureTime; >> } >> >> - if (frameContext.autoGainEnabled) { >> + if (state.autoGainEnabled) { >> minAnalogueGain = session.minAnalogueGain; >> maxAnalogueGain = session.maxAnalogueGain; >> } else { >> - minAnalogueGain = frameContext.gain; >> - maxAnalogueGain = frameContext.gain; >> + minAnalogueGain = state.manual.gain; >> + maxAnalogueGain = state.manual.gain; >> } >> >> std::visit(utils::overloaded{ >> @@ -683,10 +683,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state, >> .traits = params->traits, >> .yHist = params->yHist, >> .effectiveExposureValue = effectiveExposureValue, >> - .constraintModeIndex = frameContext.constraintMode, >> - .exposureModeIndex = frameContext.exposureMode, >> + .constraintModeIndex = state.constraintMode, >> + .exposureModeIndex = state.exposureMode, >> .lux = params->lux, >> - .exposureCompensation = pow(2.0, frameContext.exposureValue), >> + .exposureCompensation = pow(2.0, state.exposureValue), >> }); >> >> /* Update the estimated exposure and gain. */ >> -- >> 2.55.0 >>
diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp index bc57179d26..eefd7ef712 100644 --- a/src/ipa/libipa/agc.cpp +++ b/src/ipa/libipa/agc.cpp @@ -623,22 +623,22 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state, double minAnalogueGain; double maxAnalogueGain; - if (frameContext.autoExposureEnabled) { + if (state.autoExposureEnabled) { minExposureTime = session.minExposureTime; - maxExposureTime = std::clamp(frameContext.maxFrameDuration, + maxExposureTime = std::clamp(state.maxFrameDuration, session.minExposureTime, session.maxExposureTime); } else { - minExposureTime = lineDuration * frameContext.exposure; + minExposureTime = lineDuration * state.manual.exposure; maxExposureTime = minExposureTime; } - if (frameContext.autoGainEnabled) { + if (state.autoGainEnabled) { minAnalogueGain = session.minAnalogueGain; maxAnalogueGain = session.maxAnalogueGain; } else { - minAnalogueGain = frameContext.gain; - maxAnalogueGain = frameContext.gain; + minAnalogueGain = state.manual.gain; + maxAnalogueGain = state.manual.gain; } std::visit(utils::overloaded{ @@ -683,10 +683,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state, .traits = params->traits, .yHist = params->yHist, .effectiveExposureValue = effectiveExposureValue, - .constraintModeIndex = frameContext.constraintMode, - .exposureModeIndex = frameContext.exposureMode, + .constraintModeIndex = state.constraintMode, + .exposureModeIndex = state.exposureMode, .lux = params->lux, - .exposureCompensation = pow(2.0, frameContext.exposureValue), + .exposureCompensation = pow(2.0, state.exposureValue), }); /* Update the estimated exposure and gain. */
The `frameContext` is of an already completed frame, its exposure/gain/etc is of no real concern for future frames, so take the most recent settings from the active state. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/libipa/agc.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)