| Message ID | 20250923190657.21453-6-hansg@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, thank you for the patch. Hans de Goede <hansg@kernel.org> writes: > Due to a combination of not having correct control-delay information for > various sensors as well as the generic nature of the simple-pipeline + > software-ISP meaning that any pipeline delays are unknown The issue is that we cannot be sure at which frame the pipeline applies exposure changes and that the drivers don't report exposure/gain in metadata? It might be useful to mention the reasons in the commit message. > it is impossible to get reliable control-delay values. > > Wrong control-delay values can lead to pretty bad oscilation. Since atm > it is not possible to fix the wrong control-delay values, stop using > the old sensor values reported by the delayed-controlled mechanism. > > Instead remember the gain and exposures set the last time the algorithm > runs (initializing the cached values with the actual sensor values on > the first frame), combined with skipping a fixed number of frames after > changing values. A question is whether we, now when we do it less frequently, should adjust the exposure/gain in larger increments, possibly based on the deviation from the optimal exposure and/or recent adjustments. Just a thought for followup stuff, not suggesting addressing it here. > Signed-off-by: Hans de Goede <hansg@kernel.org> > --- > src/ipa/simple/algorithms/agc.cpp | 30 +++++++++++++++++++++++++----- > src/ipa/simple/algorithms/agc.h | 3 ++- > src/ipa/simple/ipa_context.h | 6 ++++++ > src/ipa/simple/soft_simple.cpp | 4 ++-- > 4 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index cdde56ba2..3c0e20ddc 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -41,7 +41,15 @@ Agc::Agc() > { > } > > -void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV) > +int Agc::configure(IPAContext &context, > + [[maybe_unused]] const IPAConfigInfo &configInfo) > +{ > + context.activeState.agc.skipFrames = 0; > + > + return 0; > +} > + > +void Agc::updateExposure(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, double exposureMSV) > { > /* > * kExpDenominator of 10 gives ~10% increment/decrement; > @@ -51,8 +59,18 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1; > static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1; > > - int32_t &exposure = frameContext.sensor.exposure; > - double &again = frameContext.sensor.gain; > + int32_t &skipFrames = context.activeState.agc.skipFrames; > + int32_t &exposure = context.activeState.agc.exposure; > + double &again = context.activeState.agc.again; > + > + /* Set initial-gain values from sensor on first frame */ > + if (frame == 0) { > + exposure = frameContext.sensor.exposure; > + again = frameContext.sensor.gain; > + } > + > + if (skipFrames && --skipFrames) > + return; I think something like if (skipFrames > 1) { skipFrames--; return; } would be much clearer. > if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { > if (exposure < context.configuration.agc.exposureMax) { > @@ -68,6 +86,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > else > again = next; > } > + skipFrames = 3; Perhaps the value of `3' deserves a declared constant, with an explanation why 3 is a good value. > } > > if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { > @@ -84,6 +103,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > else > exposure = next; > } > + skipFrames = 3; > } > > exposure = std::clamp(exposure, context.configuration.agc.exposureMin, > @@ -97,7 +117,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > } > > void Agc::process(IPAContext &context, > - [[maybe_unused]] const uint32_t frame, > + const uint32_t frame, > IPAFrameContext &frameContext, > const SwIspStats *stats, > ControlList &metadata) > @@ -135,7 +155,7 @@ void Agc::process(IPAContext &context, > } > > float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom); > - updateExposure(context, frameContext, exposureMSV); > + updateExposure(context, frame, frameContext, exposureMSV); > } > > REGISTER_IPA_ALGORITHM(Agc, "Agc") > diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/simple/algorithms/agc.h > index 112d9f5a1..ef387664f 100644 > --- a/src/ipa/simple/algorithms/agc.h > +++ b/src/ipa/simple/algorithms/agc.h > @@ -19,13 +19,14 @@ public: > Agc(); > ~Agc() = default; > > + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > void process(IPAContext &context, const uint32_t frame, > IPAFrameContext &frameContext, > const SwIspStats *stats, > ControlList &metadata) override; > > private: > - void updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV); > + void updateExposure(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, double exposureMSV); > }; > > } /* namespace ipa::soft::algorithms */ > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index ba525a881..cdab980a1 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -37,6 +37,12 @@ struct IPASessionConfiguration { > }; > > struct IPAActiveState { > + struct { > + int32_t skipFrames; Unsigned? > + int32_t exposure; > + double again; > + } agc; > + > struct { > uint8_t level; > int32_t lastExposure; > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index f0764ef46..f8c6291e2 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -327,8 +327,8 @@ void IPASoftSimple::processStats(const uint32_t frame, > > ControlList ctrls(sensorInfoMap_); > > - auto &againNew = frameContext.sensor.gain; > - ctrls.set(V4L2_CID_EXPOSURE, frameContext.sensor.exposure); > + auto &againNew = context_.activeState.agc.again; > + ctrls.set(V4L2_CID_EXPOSURE, context_.activeState.agc.exposure); > ctrls.set(V4L2_CID_ANALOGUE_GAIN, > static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(againNew) : againNew));
diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index cdde56ba2..3c0e20ddc 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -41,7 +41,15 @@ Agc::Agc() { } -void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV) +int Agc::configure(IPAContext &context, + [[maybe_unused]] const IPAConfigInfo &configInfo) +{ + context.activeState.agc.skipFrames = 0; + + return 0; +} + +void Agc::updateExposure(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, double exposureMSV) { /* * kExpDenominator of 10 gives ~10% increment/decrement; @@ -51,8 +59,18 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1; static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1; - int32_t &exposure = frameContext.sensor.exposure; - double &again = frameContext.sensor.gain; + int32_t &skipFrames = context.activeState.agc.skipFrames; + int32_t &exposure = context.activeState.agc.exposure; + double &again = context.activeState.agc.again; + + /* Set initial-gain values from sensor on first frame */ + if (frame == 0) { + exposure = frameContext.sensor.exposure; + again = frameContext.sensor.gain; + } + + if (skipFrames && --skipFrames) + return; if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { if (exposure < context.configuration.agc.exposureMax) { @@ -68,6 +86,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou else again = next; } + skipFrames = 3; } if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { @@ -84,6 +103,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou else exposure = next; } + skipFrames = 3; } exposure = std::clamp(exposure, context.configuration.agc.exposureMin, @@ -97,7 +117,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou } void Agc::process(IPAContext &context, - [[maybe_unused]] const uint32_t frame, + const uint32_t frame, IPAFrameContext &frameContext, const SwIspStats *stats, ControlList &metadata) @@ -135,7 +155,7 @@ void Agc::process(IPAContext &context, } float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom); - updateExposure(context, frameContext, exposureMSV); + updateExposure(context, frame, frameContext, exposureMSV); } REGISTER_IPA_ALGORITHM(Agc, "Agc") diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/simple/algorithms/agc.h index 112d9f5a1..ef387664f 100644 --- a/src/ipa/simple/algorithms/agc.h +++ b/src/ipa/simple/algorithms/agc.h @@ -19,13 +19,14 @@ public: Agc(); ~Agc() = default; + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; void process(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, const SwIspStats *stats, ControlList &metadata) override; private: - void updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV); + void updateExposure(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, double exposureMSV); }; } /* namespace ipa::soft::algorithms */ diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index ba525a881..cdab980a1 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -37,6 +37,12 @@ struct IPASessionConfiguration { }; struct IPAActiveState { + struct { + int32_t skipFrames; + int32_t exposure; + double again; + } agc; + struct { uint8_t level; int32_t lastExposure; diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index f0764ef46..f8c6291e2 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -327,8 +327,8 @@ void IPASoftSimple::processStats(const uint32_t frame, ControlList ctrls(sensorInfoMap_); - auto &againNew = frameContext.sensor.gain; - ctrls.set(V4L2_CID_EXPOSURE, frameContext.sensor.exposure); + auto &againNew = context_.activeState.agc.again; + ctrls.set(V4L2_CID_EXPOSURE, context_.activeState.agc.exposure); ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(againNew) : againNew));
Due to a combination of not having correct control-delay information for various sensors as well as the generic nature of the simple-pipeline + software-ISP meaning that any pipeline delays are unknown it is impossible to get reliable control-delay values. Wrong control-delay values can lead to pretty bad oscilation. Since atm it is not possible to fix the wrong control-delay values, stop using the old sensor values reported by the delayed-controlled mechanism. Instead remember the gain and exposures set the last time the algorithm runs (initializing the cached values with the actual sensor values on the first frame), combined with skipping a fixed number of frames after changing values. Signed-off-by: Hans de Goede <hansg@kernel.org> --- src/ipa/simple/algorithms/agc.cpp | 30 +++++++++++++++++++++++++----- src/ipa/simple/algorithms/agc.h | 3 ++- src/ipa/simple/ipa_context.h | 6 ++++++ src/ipa/simple/soft_simple.cpp | 4 ++-- 4 files changed, 35 insertions(+), 8 deletions(-)