[libcamera-devel] [PATCH v2 2/6] src: ipa: raspberrypi: agc: Add GetConvergenceFrames method to AGC base class

[Laurent Pinchart]

Hi Naush,

On Tue, Dec 08, 2020 at 10:07:19AM +0000, Naushir Patuck wrote:
> On Mon, 7 Dec 2020 at 18:02, David Plowman wrote:
> > We add a GetConvergenceFrames method to the AgcAlgorithm class which
> > can be called when the AGC is started from scratch. It suggests how
> > many frames should be dropped before displaying any (while the AGC
> > converges).
> >
> > The Raspberry Pi specific implementation makes this customisable from
> > the tuning file.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/agc_algorithm.hpp |  1 +
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp       | 13 +++++++++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp       |  2 ++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > index b4ea54fb..85fc6084 100644
> > --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > @@ -15,6 +15,7 @@ class AgcAlgorithm : public Algorithm
> >  public:
> >         AgcAlgorithm(Controller *controller) : Algorithm(controller) {}
> >         // An AGC algorithm must provide the following:
> > +       virtual unsigned int GetConvergenceFrames(unsigned int mistrust_frames) const = 0;
> >
> >         virtual void SetEv(double ev) = 0;
> >         virtual void SetFlickerPeriod(double flicker_period) = 0;
> >         virtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 9da18c31..787918cc 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -142,6 +142,7 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
> >         Y_target.Read(params.get_child("y_target"));
> >         speed = params.get<double>("speed", 0.2);
> >         startup_frames = params.get<uint16_t>("startup_frames", 10);
> > +       convergence_frames = params.get<unsigned int>("convergence_frames", 6);
> >         fast_reduce_threshold =
> >                 params.get<double>("fast_reduce_threshold", 0.4);
> >         base_ev = params.get<double>("base_ev", 1.0);
> > @@ -206,6 +207,18 @@ void Agc::Resume()
> >         fixed_analogue_gain_ = 0;
> >  }
> >
> > +unsigned int Agc::GetConvergenceFrames(unsigned int mistrust_frames) const
> > +{
> > +       // If shutter and gain have been explicitly set, there is no
> > +       // convergence to happen, so no need to drop any frames - return zero.
> > +       // Otherwise, any frames for which we have been told not to trust the
> > +       // statistics must be added to our own count.
> > +       if (fixed_shutter_ && fixed_analogue_gain_)
> > +               return 0;
> > +       else
> > +               return config_.convergence_frames + mistrust_frames;
> > +}
> > +
> 
> Minor nitpick, but why do you pass mistrust_frames into
> GetConvergenceFrames?  Could you not leave it out and do the addition of
> mistrust frames in the IPA? Either way,

Could it be that the effect of the mistrusted frames could depend on the
algorithm's implementation ? I could imagine some algorithms not using
metadata, in which case the number of mistrusted frames could be
ignored (at least if mistrusted frames always means mistrusted
metadata).

> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> 
> >  void Agc::SetEv(double ev)
> >  {
> >         ev_ = ev;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 95db1812..7d41608a 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -52,6 +52,7 @@ struct AgcConfig {
> >         Pwl Y_target;
> >         double speed;
> >         uint16_t startup_frames;
> > +       unsigned int convergence_frames;
> >         double max_change;
> >         double min_change;
> >         double fast_reduce_threshold;
> > @@ -74,6 +75,7 @@ public:
> >         bool IsPaused() const override;
> >         void Pause() override;
> >         void Resume() override;
> > +       unsigned int GetConvergenceFrames(unsigned int mistrust_frames) const override;
> >         void SetEv(double ev) override;
> >         void SetFlickerPeriod(double flicker_period) override;
> >         void SetFixedShutter(double fixed_shutter) override; // microseconds

-- 
Regards,

Laurent Pinchart