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

Message ID 20201207180121.6374-3-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi AGC: initial frame drop count
Related show

Commit Message

David Plowman Dec. 7, 2020, 6:01 p.m. UTC 
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@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(+)

Comments

Naushir Patuck Dec. 8, 2020, 10:07 a.m. UTC | #1 
Hi David,

Thank you for your patch.

On Mon, 7 Dec 2020 at 18:02, David Plowman <david.plowman@raspberrypi.com>
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@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,

Reviewed-by: Naushir Patuck <naush@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
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Dec. 8, 2020, 11:35 a.m. UTC | #2 
Hi David,

Thank you for the patch.

On Mon, Dec 07, 2020 at 06:01:17PM +0000, 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@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;

As you need the same feature for AWB, would it make sense to add this
virtual function to the Algorithm class instead ?

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

Supplying the convergence value from the algorithm instead of the
CamHelper is really nice.

Possibly with GetConvergenceFrames() moved to Algorithm,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	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;
> +}
> +
>  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
Laurent Pinchart Dec. 8, 2020, 11:38 a.m. UTC | #3 
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@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@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
David Plowman Dec. 8, 2020, 11:50 a.m. UTC | #4 
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@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@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@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
Laurent Pinchart Dec. 8, 2020, 11:55 a.m. UTC | #5 
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@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@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

Patch
diff mbox series 

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;
+}
+
 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