| Message ID | 20211110195901.85597-6-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-11-10 19:58:52) > Now that we have the real exposure applied at each frame, remove the > early return based on a frame counter and compute the gain for each > frame. > > Instead of that, introduce a number of startup frames during which the > filter speed is 1.0, meaning we apply instantly the exposure value > calculated and not a slower filtered one. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++--------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 2bf68e04..133f5931 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms { > > LOG_DEFINE_CATEGORY(IPU3Agc) > > -/* Number of frames to wait before calculating stats on minimum exposure */ > -static constexpr uint32_t kInitialFrameMinAECount = 4; > -/* Number of frames to wait between new gain/shutter time estimations */ > -static constexpr uint32_t kFrameSkipCount = 6; > - > /* Limits for analogue gain values */ > static constexpr double kMinAnalogueGain = 1.0; > static constexpr double kMaxAnalogueGain = 8.0; > @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5; > */ > static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; > > +/* Number of frames to wait before calculating stats on minimum exposure */ > +static constexpr uint32_t kNumStartupFrames = 10; > + > Agc::Agc() > - : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), > - minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), > - currentExposure_(0s), prevExposureValue_(0s) > + : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0), I see frameCount_ is initialised to zero here... > + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s), > + prevExposureValue_(0s) > { > } > > @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, > void Agc::filterExposure() > { > double speed = 0.2; > + > + /* Adapt instantly if we are in startup phase */ > + if (frameCount_ < kNumStartupFrames) > + speed = 1.0; And it's used to determine how long into startup we are. Is it ever set to 0 on start/stop/configure operations that would necessitate re-evaluating the whole startup procedure like this? I can't see anything resetting it - and we don't destroy and reconstruct the objects so it isn't going to be re-initialised. This may be a distinctly separate (and pre-existing) bug, and likely warrant a patch or fix of its own. I wonder if we should instead be passing in the frame/sequence count for all algorithm operations. That (should?) be reset to zero when starting/stopping I think ... Even with that limitation, I'm tempted to say this should still go in and the count can be fixed on top. I think this is still an improvement. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > if (filteredExposure_ == 0s) { > /* DG stands for digital gain.*/ > filteredExposure_ = currentExposure_; > @@ -186,13 +189,6 @@ void Agc::filterExposure() > */ > void Agc::computeExposure(uint32_t &exposure, double &analogueGain) > { > - /* Algorithm initialization should wait for first valid frames */ > - /* \todo - have a number of frames given by DelayedControls ? > - * - implement a function for IIR */ > - if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount)) > - return; > - > - lastFrame_ = frameCount_; > > /* Are we correctly exposed ? */ > if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) { > -- > 2.32.0 >
Hi Kieran, On 10/11/2021 23:02, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-11-10 19:58:52) >> Now that we have the real exposure applied at each frame, remove the >> early return based on a frame counter and compute the gain for each >> frame. >> >> Instead of that, introduce a number of startup frames during which the >> filter speed is 1.0, meaning we apply instantly the exposure value >> calculated and not a slower filtered one. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++--------------- >> 1 file changed, 11 insertions(+), 15 deletions(-) >> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >> index 2bf68e04..133f5931 100644 >> --- a/src/ipa/ipu3/algorithms/agc.cpp >> +++ b/src/ipa/ipu3/algorithms/agc.cpp >> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms { >> >> LOG_DEFINE_CATEGORY(IPU3Agc) >> >> -/* Number of frames to wait before calculating stats on minimum exposure */ >> -static constexpr uint32_t kInitialFrameMinAECount = 4; >> -/* Number of frames to wait between new gain/shutter time estimations */ >> -static constexpr uint32_t kFrameSkipCount = 6; >> - >> /* Limits for analogue gain values */ >> static constexpr double kMinAnalogueGain = 1.0; >> static constexpr double kMaxAnalogueGain = 8.0; >> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5; >> */ >> static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; >> >> +/* Number of frames to wait before calculating stats on minimum exposure */ >> +static constexpr uint32_t kNumStartupFrames = 10; >> + >> Agc::Agc() >> - : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), >> - minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), >> - currentExposure_(0s), prevExposureValue_(0s) >> + : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0), > > I see frameCount_ is initialised to zero here... > > >> + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s), >> + prevExposureValue_(0s) >> { >> } >> >> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, >> void Agc::filterExposure() >> { >> double speed = 0.2; >> + >> + /* Adapt instantly if we are in startup phase */ >> + if (frameCount_ < kNumStartupFrames) >> + speed = 1.0; > > And it's used to determine how long into startup we are. > > Is it ever set to 0 on start/stop/configure operations that would > necessitate re-evaluating the whole startup procedure like this? > > I can't see anything resetting it - and we don't destroy and reconstruct > the objects so it isn't going to be re-initialised. > > This may be a distinctly separate (and pre-existing) bug, and likely > warrant a patch or fix of its own. > > I wonder if we should instead be passing in the frame/sequence count for > all algorithm operations. That (should?) be reset to zero when > starting/stopping I think ... > We could add the frameId in the frameContext, and fill it in EventStatReady. That would solve the issue and remove the frameCount_ at the same time ? > Even with that limitation, I'm tempted to say this should still go in > and the count can be fixed on top. I think this is still an improvement. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> + >> if (filteredExposure_ == 0s) { >> /* DG stands for digital gain.*/ >> filteredExposure_ = currentExposure_; >> @@ -186,13 +189,6 @@ void Agc::filterExposure() >> */ >> void Agc::computeExposure(uint32_t &exposure, double &analogueGain) >> { >> - /* Algorithm initialization should wait for first valid frames */ >> - /* \todo - have a number of frames given by DelayedControls ? >> - * - implement a function for IIR */ >> - if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount)) >> - return; >> - >> - lastFrame_ = frameCount_; >> >> /* Are we correctly exposed ? */ >> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) { >> -- >> 2.32.0 >>
Quoting Jean-Michel Hautbois (2021-11-10 22:12:50) > Hi Kieran, > > On 10/11/2021 23:02, Kieran Bingham wrote: > > Quoting Jean-Michel Hautbois (2021-11-10 19:58:52) > >> Now that we have the real exposure applied at each frame, remove the > >> early return based on a frame counter and compute the gain for each > >> frame. > >> > >> Instead of that, introduce a number of startup frames during which the > >> filter speed is 1.0, meaning we apply instantly the exposure value > >> calculated and not a slower filtered one. > >> > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- > >> src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++--------------- > >> 1 file changed, 11 insertions(+), 15 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > >> index 2bf68e04..133f5931 100644 > >> --- a/src/ipa/ipu3/algorithms/agc.cpp > >> +++ b/src/ipa/ipu3/algorithms/agc.cpp > >> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms { > >> > >> LOG_DEFINE_CATEGORY(IPU3Agc) > >> > >> -/* Number of frames to wait before calculating stats on minimum exposure */ > >> -static constexpr uint32_t kInitialFrameMinAECount = 4; > >> -/* Number of frames to wait between new gain/shutter time estimations */ > >> -static constexpr uint32_t kFrameSkipCount = 6; > >> - > >> /* Limits for analogue gain values */ > >> static constexpr double kMinAnalogueGain = 1.0; > >> static constexpr double kMaxAnalogueGain = 8.0; > >> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5; > >> */ > >> static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; > >> > >> +/* Number of frames to wait before calculating stats on minimum exposure */ > >> +static constexpr uint32_t kNumStartupFrames = 10; > >> + > >> Agc::Agc() > >> - : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), > >> - minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), > >> - currentExposure_(0s), prevExposureValue_(0s) > >> + : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0), > > > > I see frameCount_ is initialised to zero here... > > > > > >> + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s), > >> + prevExposureValue_(0s) > >> { > >> } > >> > >> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, > >> void Agc::filterExposure() > >> { > >> double speed = 0.2; > >> + > >> + /* Adapt instantly if we are in startup phase */ > >> + if (frameCount_ < kNumStartupFrames) > >> + speed = 1.0; > > > > And it's used to determine how long into startup we are. > > > > Is it ever set to 0 on start/stop/configure operations that would > > necessitate re-evaluating the whole startup procedure like this? > > > > I can't see anything resetting it - and we don't destroy and reconstruct > > the objects so it isn't going to be re-initialised. > > > > This may be a distinctly separate (and pre-existing) bug, and likely > > warrant a patch or fix of its own. > > > > I wonder if we should instead be passing in the frame/sequence count for > > all algorithm operations. That (should?) be reset to zero when > > starting/stopping I think ... > > > > We could add the frameId in the frameContext, and fill it in > EventStatReady. That would solve the issue and remove the frameCount_ at > the same time ? Ah yes, I think you had something like that already? Anyway, it can be solved on top of this series. I thought we'd have shrunk the /22 down a bit more ;-) but seems there's still plenty of change in play. -- Kieran > > > Even with that limitation, I'm tempted to say this should still go in > > and the count can be fixed on top. I think this is still an improvement. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> + > >> if (filteredExposure_ == 0s) { > >> /* DG stands for digital gain.*/ > >> filteredExposure_ = currentExposure_; > >> @@ -186,13 +189,6 @@ void Agc::filterExposure() > >> */ > >> void Agc::computeExposure(uint32_t &exposure, double &analogueGain) > >> { > >> - /* Algorithm initialization should wait for first valid frames */ > >> - /* \todo - have a number of frames given by DelayedControls ? > >> - * - implement a function for IIR */ > >> - if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount)) > >> - return; > >> - > >> - lastFrame_ = frameCount_; > >> > >> /* Are we correctly exposed ? */ > >> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) { > >> -- > >> 2.32.0 > >>
On 11/11/2021 01:08, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-11-10 22:12:50) >> Hi Kieran, >> >> On 10/11/2021 23:02, Kieran Bingham wrote: >>> Quoting Jean-Michel Hautbois (2021-11-10 19:58:52) >>>> Now that we have the real exposure applied at each frame, remove the >>>> early return based on a frame counter and compute the gain for each >>>> frame. >>>> >>>> Instead of that, introduce a number of startup frames during which the >>>> filter speed is 1.0, meaning we apply instantly the exposure value >>>> calculated and not a slower filtered one. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++--------------- >>>> 1 file changed, 11 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >>>> index 2bf68e04..133f5931 100644 >>>> --- a/src/ipa/ipu3/algorithms/agc.cpp >>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp >>>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms { >>>> >>>> LOG_DEFINE_CATEGORY(IPU3Agc) >>>> >>>> -/* Number of frames to wait before calculating stats on minimum exposure */ >>>> -static constexpr uint32_t kInitialFrameMinAECount = 4; >>>> -/* Number of frames to wait between new gain/shutter time estimations */ >>>> -static constexpr uint32_t kFrameSkipCount = 6; >>>> - >>>> /* Limits for analogue gain values */ >>>> static constexpr double kMinAnalogueGain = 1.0; >>>> static constexpr double kMaxAnalogueGain = 8.0; >>>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5; >>>> */ >>>> static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; >>>> >>>> +/* Number of frames to wait before calculating stats on minimum exposure */ >>>> +static constexpr uint32_t kNumStartupFrames = 10; >>>> + >>>> Agc::Agc() >>>> - : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), >>>> - minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), >>>> - currentExposure_(0s), prevExposureValue_(0s) >>>> + : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0), >>> >>> I see frameCount_ is initialised to zero here... >>> >>> >>>> + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s), >>>> + prevExposureValue_(0s) >>>> { >>>> } >>>> >>>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, >>>> void Agc::filterExposure() >>>> { >>>> double speed = 0.2; >>>> + >>>> + /* Adapt instantly if we are in startup phase */ >>>> + if (frameCount_ < kNumStartupFrames) >>>> + speed = 1.0; >>> >>> And it's used to determine how long into startup we are. >>> >>> Is it ever set to 0 on start/stop/configure operations that would >>> necessitate re-evaluating the whole startup procedure like this? >>> >>> I can't see anything resetting it - and we don't destroy and reconstruct >>> the objects so it isn't going to be re-initialised. >>> >>> This may be a distinctly separate (and pre-existing) bug, and likely >>> warrant a patch or fix of its own. >>> >>> I wonder if we should instead be passing in the frame/sequence count for >>> all algorithm operations. That (should?) be reset to zero when >>> starting/stopping I think ... >>> >> >> We could add the frameId in the frameContext, and fill it in >> EventStatReady. That would solve the issue and remove the frameCount_ at >> the same time ? > > > Ah yes, I think you had something like that already? Anyway, it can be > solved on top of this series. I thought we'd have shrunk the /22 down a > bit more ;-) but seems there's still plenty of change in play. > I can cherry-pick the frameId introduction and use it in a patch removing frameCount_, we would then have 15 instead of 22 patches :-). > -- > Kieran > > > >> >>> Even with that limitation, I'm tempted to say this should still go in >>> and the count can be fixed on top. I think this is still an improvement. >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> + >>>> if (filteredExposure_ == 0s) { >>>> /* DG stands for digital gain.*/ >>>> filteredExposure_ = currentExposure_; >>>> @@ -186,13 +189,6 @@ void Agc::filterExposure() >>>> */ >>>> void Agc::computeExposure(uint32_t &exposure, double &analogueGain) >>>> { >>>> - /* Algorithm initialization should wait for first valid frames */ >>>> - /* \todo - have a number of frames given by DelayedControls ? >>>> - * - implement a function for IIR */ >>>> - if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount)) >>>> - return; >>>> - >>>> - lastFrame_ = frameCount_; >>>> >>>> /* Are we correctly exposed ? */ >>>> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) { >>>> -- >>>> 2.32.0 >>>>
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 2bf68e04..133f5931 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms { LOG_DEFINE_CATEGORY(IPU3Agc) -/* Number of frames to wait before calculating stats on minimum exposure */ -static constexpr uint32_t kInitialFrameMinAECount = 4; -/* Number of frames to wait between new gain/shutter time estimations */ -static constexpr uint32_t kFrameSkipCount = 6; - /* Limits for analogue gain values */ static constexpr double kMinAnalogueGain = 1.0; static constexpr double kMaxAnalogueGain = 8.0; @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5; */ static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; +/* Number of frames to wait before calculating stats on minimum exposure */ +static constexpr uint32_t kNumStartupFrames = 10; + Agc::Agc() - : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), - minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), - currentExposure_(0s), prevExposureValue_(0s) + : frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0), + maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s), + prevExposureValue_(0s) { } @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, void Agc::filterExposure() { double speed = 0.2; + + /* Adapt instantly if we are in startup phase */ + if (frameCount_ < kNumStartupFrames) + speed = 1.0; + if (filteredExposure_ == 0s) { /* DG stands for digital gain.*/ filteredExposure_ = currentExposure_; @@ -186,13 +189,6 @@ void Agc::filterExposure() */ void Agc::computeExposure(uint32_t &exposure, double &analogueGain) { - /* Algorithm initialization should wait for first valid frames */ - /* \todo - have a number of frames given by DelayedControls ? - * - implement a function for IIR */ - if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount)) - return; - - lastFrame_ = frameCount_; /* Are we correctly exposed ? */ if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
Now that we have the real exposure applied at each frame, remove the early return based on a frame counter and compute the gain for each frame. Instead of that, introduce a number of startup frames during which the filter speed is 1.0, meaning we apply instantly the exposure value calculated and not a slower filtered one. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)