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

[Laurent Pinchart]

Hi David,

On Sat, Dec 05, 2020 at 11:39:28PM +0000, David Plowman wrote:
> On Sat, 5 Dec 2020 at 20:03, Laurent Pinchart wrote:
> > 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 ?
> 
> The value just gets assigned a default, according to the person who
> implemented the algorithm. My advice would be to use that, so nothing
> extra is required in the json file (nor in the CTT therefore). Having
> said that, there are always people who like to fiddle ("I want to see
> every frame even while everything is still converging!") which is why
> I have given them the option to customise it.

Sounds good to me. Maybe this could become a runtime API in the future
if a need arises, but that's definitely for later.

> > >  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