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

[Laurent Pinchart]

Hi David,

On Tue, Dec 08, 2020 at 11:50:22AM +0000, David Plowman wrote:
> 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

Do you mean sensor metadata, or statistics ?

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

I'm fine with that. Given that the mistrusted frames cause
Controller::Process() to be skipped altogether, the algorithms can't see
those frames anyway, so there will always be an additional convergence
delay. If some algorithms could later work with mistrusted frames, we
would need a more intrusive rework anyway.

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