| Message ID | 20260817114349.994123-23-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Aug 17, 2026 at 01:43:23PM +0200, Barnabás Pőcze wrote: > If the auto mode is not possible (e.g. without statistics in raw mode), > then the controls should be adjusted to reflect that. > > Specifically, ensure that the `ExposureValue` control and the controls > from `AgcMeanLuminance` are only present if the auto mode is enabled. > > Furthermore, only advertise `{AnalogueGain,ExposureTime}ModeAuto` conditionally. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/libipa/agc.cpp | 38 +++++++++++++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp > index 3c6b12e452..1d12ec6b1f 100644 > --- a/src/ipa/libipa/agc.cpp > +++ b/src/ipa/libipa/agc.cpp > @@ -369,6 +369,11 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, > state.minFrameDuration = session.minFrameDuration; > state.maxFrameDuration = session.maxFrameDuration; > > + /* The IPA control maps keep their states, so the removal is necessary. */ I would record with a \todo that we should ensure that IPA modules do not retain controls between two different streaming sessions. But for now, that's the best we can do Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > + config.ctrlMap.erase(&controls::ExposureValue); > + for (const auto &[id, _] : impl_.controls()) > + config.ctrlMap.erase(id); > + > /* \todo Move this to the `Camera` class. */ > config.ctrlMap[&controls::AeEnable] = ControlInfo{ > false, > @@ -390,16 +395,31 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, > frameDurations[1], > Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, > }; > - config.ctrlMap[&controls::ExposureTimeMode] = ControlInfo{ > - {{ controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual }}, > - controls::ExposureTimeModeAuto, > - }; > - config.ctrlMap[&controls::AnalogueGainMode] = ControlInfo{ > - {{ controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual }}, > - controls::AnalogueGainModeAuto, > + > + const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) { > + std::array<ControlValue, 2> values; > + size_t count = 0; > + > + if (session.autoAllowed) > + values[count++] = ControlValue(automatic); > + > + values[count++] = ControlValue(manual); > + > + config.ctrlMap[&cid] = ControlInfo{ > + { values.data(), count }, > + ControlValue(session.autoAllowed ? automatic : manual), > + }; > }; > - config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); > - config.ctrlMap.merge(impl_.controls()); > + > + add(controls::ExposureTimeMode, > + controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual); > + add(controls::AnalogueGainMode, > + controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual); > + > + if (session.autoAllowed) { > + config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); > + config.ctrlMap.merge(impl_.controls()); > + } > > return 0; > } > -- > 2.55.0 >
Hi Barnabás, Quoting Barnabás Pőcze (2026-08-17 13:43:23) > If the auto mode is not possible (e.g. without statistics in raw mode), > then the controls should be adjusted to reflect that. > > Specifically, ensure that the `ExposureValue` control and the controls > from `AgcMeanLuminance` are only present if the auto mode is enabled. > > Furthermore, only advertise `{AnalogueGain,ExposureTime}ModeAuto` conditionally. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Looks good to me. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Best regards, Stefan > --- > src/ipa/libipa/agc.cpp | 38 +++++++++++++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp > index 3c6b12e452..1d12ec6b1f 100644 > --- a/src/ipa/libipa/agc.cpp > +++ b/src/ipa/libipa/agc.cpp > @@ -369,6 +369,11 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, > state.minFrameDuration = session.minFrameDuration; > state.maxFrameDuration = session.maxFrameDuration; > > + /* The IPA control maps keep their states, so the removal is necessary. */ > + config.ctrlMap.erase(&controls::ExposureValue); > + for (const auto &[id, _] : impl_.controls()) > + config.ctrlMap.erase(id); > + > /* \todo Move this to the `Camera` class. */ > config.ctrlMap[&controls::AeEnable] = ControlInfo{ > false, > @@ -390,16 +395,31 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, > frameDurations[1], > Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, > }; > - config.ctrlMap[&controls::ExposureTimeMode] = ControlInfo{ > - {{ controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual }}, > - controls::ExposureTimeModeAuto, > - }; > - config.ctrlMap[&controls::AnalogueGainMode] = ControlInfo{ > - {{ controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual }}, > - controls::AnalogueGainModeAuto, > + > + const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) { > + std::array<ControlValue, 2> values; > + size_t count = 0; > + > + if (session.autoAllowed) > + values[count++] = ControlValue(automatic); > + > + values[count++] = ControlValue(manual); > + > + config.ctrlMap[&cid] = ControlInfo{ > + { values.data(), count }, > + ControlValue(session.autoAllowed ? automatic : manual), > + }; > }; > - config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); > - config.ctrlMap.merge(impl_.controls()); > + > + add(controls::ExposureTimeMode, > + controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual); > + add(controls::AnalogueGainMode, > + controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual); > + > + if (session.autoAllowed) { > + config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); > + config.ctrlMap.merge(impl_.controls()); > + } > > return 0; > } > -- > 2.55.0 >
2026. 08. 17. 17:08 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Aug 17, 2026 at 01:43:23PM +0200, Barnabás Pőcze wrote: >> If the auto mode is not possible (e.g. without statistics in raw mode), >> then the controls should be adjusted to reflect that. >> >> Specifically, ensure that the `ExposureValue` control and the controls >> from `AgcMeanLuminance` are only present if the auto mode is enabled. >> >> Furthermore, only advertise `{AnalogueGain,ExposureTime}ModeAuto` conditionally. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/ipa/libipa/agc.cpp | 38 +++++++++++++++++++++++++++++--------- >> 1 file changed, 29 insertions(+), 9 deletions(-) >> >> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp >> index 3c6b12e452..1d12ec6b1f 100644 >> --- a/src/ipa/libipa/agc.cpp >> +++ b/src/ipa/libipa/agc.cpp >> @@ -369,6 +369,11 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, >> state.minFrameDuration = session.minFrameDuration; >> state.maxFrameDuration = session.maxFrameDuration; >> >> + /* The IPA control maps keep their states, so the removal is necessary. */ > > I would record with a \todo that we should ensure that IPA modules do > not retain controls between two different streaming sessions. This is already noted in an internal discussion point. I feel like that's a better location. > > But for now, that's the best we can do > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > >> + config.ctrlMap.erase(&controls::ExposureValue); >> + for (const auto &[id, _] : impl_.controls()) >> + config.ctrlMap.erase(id); >> + >> /* \todo Move this to the `Camera` class. */ >> config.ctrlMap[&controls::AeEnable] = ControlInfo{ >> false, >> @@ -390,16 +395,31 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, >> frameDurations[1], >> Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, >> }; >> - config.ctrlMap[&controls::ExposureTimeMode] = ControlInfo{ >> - {{ controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual }}, >> - controls::ExposureTimeModeAuto, >> - }; >> - config.ctrlMap[&controls::AnalogueGainMode] = ControlInfo{ >> - {{ controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual }}, >> - controls::AnalogueGainModeAuto, >> + >> + const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) { >> + std::array<ControlValue, 2> values; >> + size_t count = 0; >> + >> + if (session.autoAllowed) >> + values[count++] = ControlValue(automatic); >> + >> + values[count++] = ControlValue(manual); >> + >> + config.ctrlMap[&cid] = ControlInfo{ >> + { values.data(), count }, >> + ControlValue(session.autoAllowed ? automatic : manual), >> + }; >> }; >> - config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); >> - config.ctrlMap.merge(impl_.controls()); >> + >> + add(controls::ExposureTimeMode, >> + controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual); >> + add(controls::AnalogueGainMode, >> + controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual); >> + >> + if (session.autoAllowed) { >> + config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); >> + config.ctrlMap.merge(impl_.controls()); >> + } >> >> return 0; >> } >> -- >> 2.55.0 >>
diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp index 3c6b12e452..1d12ec6b1f 100644 --- a/src/ipa/libipa/agc.cpp +++ b/src/ipa/libipa/agc.cpp @@ -369,6 +369,11 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, state.minFrameDuration = session.minFrameDuration; state.maxFrameDuration = session.maxFrameDuration; + /* The IPA control maps keep their states, so the removal is necessary. */ + config.ctrlMap.erase(&controls::ExposureValue); + for (const auto &[id, _] : impl_.controls()) + config.ctrlMap.erase(id); + /* \todo Move this to the `Camera` class. */ config.ctrlMap[&controls::AeEnable] = ControlInfo{ false, @@ -390,16 +395,31 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state, frameDurations[1], Span<const int64_t, 2>{ { frameDurations[2], frameDurations[2] } }, }; - config.ctrlMap[&controls::ExposureTimeMode] = ControlInfo{ - {{ controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual }}, - controls::ExposureTimeModeAuto, - }; - config.ctrlMap[&controls::AnalogueGainMode] = ControlInfo{ - {{ controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual }}, - controls::AnalogueGainModeAuto, + + const auto add = [&](const ControlId &cid, const auto &automatic, const auto &manual) { + std::array<ControlValue, 2> values; + size_t count = 0; + + if (session.autoAllowed) + values[count++] = ControlValue(automatic); + + values[count++] = ControlValue(manual); + + config.ctrlMap[&cid] = ControlInfo{ + { values.data(), count }, + ControlValue(session.autoAllowed ? automatic : manual), + }; }; - config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); - config.ctrlMap.merge(impl_.controls()); + + add(controls::ExposureTimeMode, + controls::ExposureTimeModeAuto, controls::ExposureTimeModeManual); + add(controls::AnalogueGainMode, + controls::AnalogueGainModeAuto, controls::AnalogueGainModeManual); + + if (session.autoAllowed) { + config.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f); + config.ctrlMap.merge(impl_.controls()); + } return 0; }
If the auto mode is not possible (e.g. without statistics in raw mode), then the controls should be adjusted to reflect that. Specifically, ensure that the `ExposureValue` control and the controls from `AgcMeanLuminance` are only present if the auto mode is enabled. Furthermore, only advertise `{AnalogueGain,ExposureTime}ModeAuto` conditionally. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/libipa/agc.cpp | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-)