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

[David Plowman]

Hi Laurent, Naush,

Thanks for the comments. I think there are some good points there. To
bring the two together, I think Naush's suggest would be to avoid
passing mistrust_frames into the GetConvergenceFrames method, and in
the raspberrypi.cpp IPA file have something like:

unsigned int convergenceFrames = agc->GetConvergenceFrames();
if (convergenceFrames)
        convergenceFrames += mistrustFrames;

I can see that leaves the Algorithms looking simpler, which is nice.

Moving onto Laurent's point, this would mean that there's an in-built
assumption that convergence is only to do with statistics (which,
you're right, is what "mistrusted frames" are all about). But I don't
really see any other sort of convergence, at least certainly not in
our pipeline.

So I'm feeling like the Naush approach is probably the way to go...?

Thanks
David

On Tue, 8 Dec 2020 at 11:38, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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