[libcamera-devel] [PATCH 13/13] ipa: ipu3: agc: Remove unused variables

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at 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_;

-- 
Regards,

Laurent Pinchart