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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart