[libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add GetDropFrames method to AGC base class

[Laurent Pinchart]

Hi David and Naush,

On Fri, Dec 04, 2020 at 03:57:25PM +0000, Naushir Patuck wrote:
> On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:
> 
> > We add a GetDropFrames 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       | 11 +++++++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp       |  2 ++
> >  3 files changed, 14 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > index b4ea54fb..bfc9743f 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 GetDropFrames() 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..94c02d47 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);
> > +       drop_frames = params.get<unsigned int>("drop_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,16 @@ void Agc::Resume()
> >         fixed_analogue_gain_ = 0;
> >  }
> >
> > +unsigned int Agc::GetDropFrames() const
> > +{
> > +       // If shutter and gain have been explicitly set, there is no
> > +       // convergence to happen, so no need to drop any frames.
> > +       if (fixed_shutter_ && fixed_analogue_gain_)
> > +               return 0;
> > +       else
> > +               return config_.drop_frames;
> > +}
> > +
> 
> One little nit on this one.  The method name,  GetDropFrames, doesn't sit
> right with me.  Could we rename this to something like, say,
> GetConvergenceFrames, or something.  I feel this would better convey what
> this number is.  What do you think?

I think that's better, but, as commented in another patch, I wonder if
we actually need this, or if we should base the decision on the AeLocked
value.

I'm also wondering how one would pick the right value to set in the
tuning file. And don't we need a patch for CTT to set a value in the
file it generates ?

>  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..1de4d505 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 drop_frames;
> >
> 
> Similarly, instead of calling it drop_frames, we could call it
> 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 GetDropFrames() const override;
> >         void SetEv(double ev) override;
> >         void SetFlickerPeriod(double flicker_period) override;
> >         void SetFixedShutter(double fixed_shutter) override; // microseconds

-- 
Regards,

Laurent Pinchart