[libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc: Make AGC handle Pause/Resume for itself

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Thu, Nov 26, 2020 at 12:32:01PM +0000, David Plowman wrote:
> AGC, when paused, sets the last exposure/gain it wrote to be its
> "fixed" values and will therefore continue to return them. When
> resumed, we clear them so that both will float again.
> 
> This approach is better because AGC can be paused and we can
> subsequently change (for example) the exposure and the gain won't
> float again.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++
>  src/ipa/raspberrypi/controller/rpi/agc.hpp |  5 +++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 30a1c1c1..9da18c31 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const &params)
>  	exposure_mode_ = &config_.exposure_modes[exposure_mode_name_];
>  	constraint_mode_name_ = config_.default_constraint_mode;
>  	constraint_mode_ = &config_.constraint_modes[constraint_mode_name_];
> +	// Set up the "last shutter/gain" values, in case AGC starts "disabled".
> +	status_.shutter_time = config_.default_exposure_time;
> +	status_.analogue_gain = config_.default_analogue_gain;
> +}
> +
> +bool Agc::IsPaused() const
> +{
> +	return false;
> +}
> +
> +void Agc::Pause()
> +{
> +	fixed_shutter_ = status_.shutter_time;
> +	fixed_analogue_gain_ = status_.analogue_gain;
> +}
> +
> +void Agc::Resume()
> +{
> +	fixed_shutter_ = 0;
> +	fixed_analogue_gain_ = 0;
>  }
>  
>  void Agc::SetEv(double ev)
> @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)
>  void Agc::SetFixedShutter(double fixed_shutter)
>  {
>  	fixed_shutter_ = fixed_shutter;

Maybe there's something I'm missing, but when a request is queued with
an exposure time, Agc::SetFixedShutter is called regardless of whether
AGC is enabled or disabled. fixed_shutter_ is thus set to a non-zero
value, which will be copied to status.fixed_shutter in
Agc::housekeepConfig(), called in Agc::Process(). As the core code
doesn't see the AGC being paused anymore, it will call Process() for
every frame. Won't the fixed shutter take precedence, even when AGC is
enabled ?

> +	// Set this in case someone calls Pause() straight after.
> +	status_.shutter_time = fixed_shutter;
>  }
>  
>  void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
>  {
>  	fixed_analogue_gain_ = fixed_analogue_gain;
> +	// Set this in case someone calls Pause() straight after.
> +	status_.analogue_gain = fixed_analogue_gain;
>  }
>  
>  void Agc::SetMeteringMode(std::string const &metering_mode_name)
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 47ebb324..8a1a20e6 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -70,6 +70,11 @@ public:
>  	Agc(Controller *controller);
>  	char const *Name() const override;
>  	void Read(boost::property_tree::ptree const &params) override;
> +	// AGC handles "pausing" for itself.
> +	bool IsPaused() const;
> +	void Pause();
> +	void Resume();

Could you add "override" for those three functions ?

> +	unsigned int GetFrameDrops() const override;
>  	void SetEv(double ev) override;
>  	void SetFlickerPeriod(double flicker_period) override;
>  	void SetFixedShutter(double fixed_shutter) override; // microseconds

-- 
Regards,

Laurent Pinchart