[libcamera-devel,10/13] ipa: ipu3: agc: Introduce previous exposure value

14126 diff mbox series

We need to calculate the gain on the previous exposure value calculated.
This value needs to be updated after each process call, initialised to
0s.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 8 ++++++--
 src/ipa/ipu3/algorithms/agc.h   | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

Quoting Jean-Michel Hautbois (2021-10-13 16:41:22)
> We need to calculate the gain on the previous exposure value calculated.
> This value needs to be updated after each process call, initialised to
> 0s.
> 

Seems resonable.

Checking lockExposureGain() looks like it could be refactored to indent
that whole exposure calculation one level, but lets deal with that after
this series.


> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 8 ++++++--
>  src/ipa/ipu3/algorithms/agc.h   | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index bd28998a..7efe0907 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -46,7 +46,8 @@ 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)
> +         filteredExposureNoDg_(0s), currentExposure_(0s),
> +         currentExposureNoDg_(0s), prevExposureValue(0s)
>  {
>  }
>  
> @@ -145,7 +146,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>                                     << " Gain " << analogueGain
>                                     << " Needed ev gain " << evGain;
>  
> -               currentExposure_ = currentExposureNoDg_ * evGain;
> +               currentExposure_ = prevExposureValue * evGain;
>                 Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
>                 currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>                 LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> @@ -182,6 +183,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  
>                 exposure = shutterTime / lineDuration_;
>                 analogueGain = stepGain;
> +
> +               /* Update the exposure value for the next process call */
> +               prevExposureValue = shutterTime * analogueGain;
>         }
>         lastFrame_ = frameCount_;
>  }
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 7605ab39..32817c4f 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -51,6 +51,7 @@ private:
>         Duration filteredExposureNoDg_;
>         Duration currentExposure_;
>         Duration currentExposureNoDg_;
> +       Duration prevExposureValue;

It looks like this should have an underscore at the end.

With that,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>         uint32_t stride_;
>  };
> -- 
> 2.30.2
>

On Thu, Oct 14, 2021 at 12:27:11PM +0100, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-13 16:41:22)
> > We need to calculate the gain on the previous exposure value calculated.
> > This value needs to be updated after each process call, initialised to
> > 0s.
> 
> Seems resonable.
> 
> Checking lockExposureGain() looks like it could be refactored to indent
> that whole exposure calculation one level, but lets deal with that after
> this series.
> 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp | 8 ++++++--
> >  src/ipa/ipu3/algorithms/agc.h   | 1 +
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index bd28998a..7efe0907 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -46,7 +46,8 @@ 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)
> > +         filteredExposureNoDg_(0s), currentExposure_(0s),
> > +         currentExposureNoDg_(0s), prevExposureValue(0s)
> >  {
> >  }
> >  
> > @@ -145,7 +146,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> >                                     << " Gain " << analogueGain
> >                                     << " Needed ev gain " << evGain;
> >  
> > -               currentExposure_ = currentExposureNoDg_ * evGain;
> > +               currentExposure_ = prevExposureValue * evGain;

So on the first run this will be 0, which means that you'll apply the
minimum gain and shutter, instead of starting from the values that were
used for the first frame. Doesn't this slow down convergence ?

Beside, currentExposureNoDg_ contains the correct exposure that was used
for the previous frame, that's how it's calculated a few lines above.
I'm failing to see what this patch fixes. The commit message doesn't
actually mention it fixes something, so maybe it's just a refactoring ?
In either case, it seems to introduce an issue, and the commit message
doesn't explain what benefit it brings.

> >                 Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
> >                 currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> >                 LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> > @@ -182,6 +183,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> >  
> >                 exposure = shutterTime / lineDuration_;
> >                 analogueGain = stepGain;
> > +
> > +               /* Update the exposure value for the next process call */
> > +               prevExposureValue = shutterTime * analogueGain;
> >         }
> >         lastFrame_ = frameCount_;
> >  }
> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > index 7605ab39..32817c4f 100644
> > --- a/src/ipa/ipu3/algorithms/agc.h
> > +++ b/src/ipa/ipu3/algorithms/agc.h
> > @@ -51,6 +51,7 @@ private:
> >         Duration filteredExposureNoDg_;
> >         Duration currentExposure_;
> >         Duration currentExposureNoDg_;
> > +       Duration prevExposureValue;
> 
> It looks like this should have an underscore at the end.
> 
> With that,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >  
> >         uint32_t stride_;
> >  };