Message ID | 20211013154125.133419-14-jeanmichel.hautbois@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Quoting Jean-Michel Hautbois (2021-10-13 16:41:25) > We currently control the exposure value by the shutter speed and the > analogue gain. We can't use the digital gain to have more than the > maximum exposure value calculated because we are not controlling it. Do you mean, because we are not setting the digital gain in the ISP? > Remove unused code associated with this digital gain. If it's unused, I'm happy to see it go, and the code simplified ;-) Unless you think we should be adding the plumbing to allow setting the digital gain on the ISP: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 19 +++---------------- > src/ipa/ipu3/algorithms/agc.h | 2 -- > 2 files changed, 3 insertions(+), 18 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 81eaf436..0177bb92 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -46,8 +46,7 @@ static constexpr double kEvGainTarget = 0.5; > Agc::Agc() > : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), > minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), > - filteredExposureNoDg_(0s), currentExposure_(0s), > - currentExposureNoDg_(0s), prevExposureValue(0s) > + currentExposure_(0s), prevExposureValue(0s) > { > } > > @@ -96,7 +95,6 @@ void Agc::filterExposure() > if (filteredExposure_ == 0s) { > /* DG stands for digital gain.*/ > filteredExposure_ = currentExposure_; > - filteredExposureNoDg_ = currentExposureNoDg_; > } else { > /* > * If we are close to the desired result, go faster to avoid making > @@ -109,18 +107,8 @@ void Agc::filterExposure() > > filteredExposure_ = speed * currentExposure_ + > filteredExposure_ * (1.0 - speed); > - filteredExposureNoDg_ = speed * currentExposureNoDg_ + > - filteredExposureNoDg_ * (1.0 - speed); > } > - /* > - * We can't let the no_dg exposure deviate too far below the > - * total exposure, as there might not be enough digital gain available > - * in the ISP to hide it (which will cause nasty oscillation). > - */ > - double fastReduceThreshold = 0.4; > - if (filteredExposureNoDg_ < > - filteredExposure_ * fastReduceThreshold) > - filteredExposureNoDg_ = filteredExposure_ * fastReduceThreshold; > + > LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_; > } > > @@ -136,8 +124,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain) > > /* extracted from Rpi::Agc::computeTargetExposure */ > Duration currentShutter = exposure * lineDuration_; > - currentExposureNoDg_ = currentShutter * analogueGain; > - LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_ > + LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain > << " Shutter speed " << currentShutter > << " Gain " << analogueGain > << " Needed ev gain " << evGain; > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index 32817c4f..5b34c013 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -48,9 +48,7 @@ private: > uint32_t maxExposureLines_; > > Duration filteredExposure_; > - Duration filteredExposureNoDg_; > Duration currentExposure_; > - Duration currentExposureNoDg_; > Duration prevExposureValue; > > uint32_t stride_; > -- > 2.30.2 >
On Thu, Oct 14, 2021 at 12:40:12PM +0100, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-10-13 16:41:25) > > We currently control the exposure value by the shutter speed and the > > analogue gain. We can't use the digital gain to have more than the > > maximum exposure value calculated because we are not controlling it. > > Do you mean, because we are not setting the digital gain in the ISP? > > > Remove unused code associated with this digital gain. > > If it's unused, I'm happy to see it go, and the code simplified ;-) > > Unless you think we should be adding the plumbing to allow setting the > digital gain on the ISP: I think we should do so eventually, yes. We can simplify the code in the meantime, but it will be an important improvement for low-light conditions. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/ipa/ipu3/algorithms/agc.cpp | 19 +++---------------- > > src/ipa/ipu3/algorithms/agc.h | 2 -- > > 2 files changed, 3 insertions(+), 18 deletions(-) > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > index 81eaf436..0177bb92 100644 > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > @@ -46,8 +46,7 @@ static constexpr double kEvGainTarget = 0.5; > > Agc::Agc() > > : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), > > minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), > > - filteredExposureNoDg_(0s), currentExposure_(0s), > > - currentExposureNoDg_(0s), prevExposureValue(0s) > > + currentExposure_(0s), prevExposureValue(0s) > > { > > } > > > > @@ -96,7 +95,6 @@ void Agc::filterExposure() > > if (filteredExposure_ == 0s) { > > /* DG stands for digital gain.*/ > > filteredExposure_ = currentExposure_; > > - filteredExposureNoDg_ = currentExposureNoDg_; > > } else { > > /* > > * If we are close to the desired result, go faster to avoid making > > @@ -109,18 +107,8 @@ void Agc::filterExposure() > > > > filteredExposure_ = speed * currentExposure_ + > > filteredExposure_ * (1.0 - speed); > > - filteredExposureNoDg_ = speed * currentExposureNoDg_ + > > - filteredExposureNoDg_ * (1.0 - speed); > > } > > - /* > > - * We can't let the no_dg exposure deviate too far below the > > - * total exposure, as there might not be enough digital gain available > > - * in the ISP to hide it (which will cause nasty oscillation). > > - */ > > - double fastReduceThreshold = 0.4; > > - if (filteredExposureNoDg_ < > > - filteredExposure_ * fastReduceThreshold) > > - filteredExposureNoDg_ = filteredExposure_ * fastReduceThreshold; > > + > > LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_; > > } > > > > @@ -136,8 +124,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain) > > > > /* extracted from Rpi::Agc::computeTargetExposure */ > > Duration currentShutter = exposure * lineDuration_; > > - currentExposureNoDg_ = currentShutter * analogueGain; > > - LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_ > > + LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain > > << " Shutter speed " << currentShutter > > << " Gain " << analogueGain > > << " Needed ev gain " << evGain; > > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > > index 32817c4f..5b34c013 100644 > > --- a/src/ipa/ipu3/algorithms/agc.h > > +++ b/src/ipa/ipu3/algorithms/agc.h > > @@ -48,9 +48,7 @@ private: > > uint32_t maxExposureLines_; > > > > Duration filteredExposure_; > > - Duration filteredExposureNoDg_; > > Duration currentExposure_; > > - Duration currentExposureNoDg_; > > Duration prevExposureValue; > > > > uint32_t stride_;
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 81eaf436..0177bb92 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -46,8 +46,7 @@ static constexpr double kEvGainTarget = 0.5; Agc::Agc() : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s), - filteredExposureNoDg_(0s), currentExposure_(0s), - currentExposureNoDg_(0s), prevExposureValue(0s) + currentExposure_(0s), prevExposureValue(0s) { } @@ -96,7 +95,6 @@ void Agc::filterExposure() if (filteredExposure_ == 0s) { /* DG stands for digital gain.*/ filteredExposure_ = currentExposure_; - filteredExposureNoDg_ = currentExposureNoDg_; } else { /* * If we are close to the desired result, go faster to avoid making @@ -109,18 +107,8 @@ void Agc::filterExposure() filteredExposure_ = speed * currentExposure_ + filteredExposure_ * (1.0 - speed); - filteredExposureNoDg_ = speed * currentExposureNoDg_ + - filteredExposureNoDg_ * (1.0 - speed); } - /* - * We can't let the no_dg exposure deviate too far below the - * total exposure, as there might not be enough digital gain available - * in the ISP to hide it (which will cause nasty oscillation). - */ - double fastReduceThreshold = 0.4; - if (filteredExposureNoDg_ < - filteredExposure_ * fastReduceThreshold) - filteredExposureNoDg_ = filteredExposure_ * fastReduceThreshold; + LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_; } @@ -136,8 +124,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain) /* extracted from Rpi::Agc::computeTargetExposure */ Duration currentShutter = exposure * lineDuration_; - currentExposureNoDg_ = currentShutter * analogueGain; - LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_ + LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain << " Shutter speed " << currentShutter << " Gain " << analogueGain << " Needed ev gain " << evGain; diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h index 32817c4f..5b34c013 100644 --- a/src/ipa/ipu3/algorithms/agc.h +++ b/src/ipa/ipu3/algorithms/agc.h @@ -48,9 +48,7 @@ private: uint32_t maxExposureLines_; Duration filteredExposure_; - Duration filteredExposureNoDg_; Duration currentExposure_; - Duration currentExposureNoDg_; Duration prevExposureValue; uint32_t stride_;
We currently control the exposure value by the shutter speed and the analogue gain. We can't use the digital gain to have more than the maximum exposure value calculated because we are not controlling it. Remove unused code associated with this digital gain. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 19 +++---------------- src/ipa/ipu3/algorithms/agc.h | 2 -- 2 files changed, 3 insertions(+), 18 deletions(-)