[libcamera-devel] [PATCH v5 3/4] ipa: raspberrypi: Switch AgcAlgorithm API to use utils::Duration

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Tue, May 25, 2021 at 12:42:40PM +0100, Naushir Patuck wrote:
> Switch the AgcAlgorithm API functions to use utils::Duration for all
> time based variables.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/ipa/raspberrypi/controller/agc_algorithm.hpp |  7 ++++---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp       | 13 +++++++------
>  src/ipa/raspberrypi/controller/rpi/agc.hpp       |  6 +++---
>  src/ipa/raspberrypi/raspberrypi.cpp              |  6 +++---
>  4 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> index f97deb0fca59..579ee2c91fa8 100644
> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> @@ -6,6 +6,7 @@
>   */
>  #pragma once
>  
> +#include "libcamera/internal/utils.h"
>  #include "algorithm.hpp"
>  
>  namespace RPiController {
> @@ -17,9 +18,9 @@ public:
>  	// An AGC algorithm must provide the following:
>  	virtual unsigned int GetConvergenceFrames() const = 0;
>  	virtual void SetEv(double ev) = 0;
> -	virtual void SetFlickerPeriod(double flicker_period) = 0;
> -	virtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds
> -	virtual void SetMaxShutter(double max_shutter) = 0; // microseconds
> +	virtual void SetFlickerPeriod(const libcamera::utils::Duration &flicker_period) = 0;
> +	virtual void SetFixedShutter(const libcamera::utils::Duration &fixed_shutter) = 0;
> +	virtual void SetMaxShutter(const libcamera::utils::Duration &max_shutter) = 0;
>  	virtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;
>  	virtual void SetMeteringMode(std::string const &metering_mode_name) = 0;
>  	virtual void SetExposureMode(std::string const &exposure_mode_name) = 0;
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index f4cd5d26fb4e..4e3318445cf3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -21,6 +21,7 @@
>  
>  using namespace RPiController;
>  using namespace libcamera;
> +using libcamera::utils::Duration;

I would have used utils::Duration instead of Duration below, but I don't
mind.

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

>  
>  LOG_DEFINE_CATEGORY(RPiAgc)
>  
> @@ -222,19 +223,19 @@ void Agc::SetEv(double ev)
>  	ev_ = ev;
>  }
>  
> -void Agc::SetFlickerPeriod(double flicker_period)
> +void Agc::SetFlickerPeriod(const Duration &flicker_period)
>  {
> -	flicker_period_ = flicker_period;
> +	flicker_period_ = flicker_period.get<std::micro>();
>  }
>  
> -void Agc::SetMaxShutter(double max_shutter)
> +void Agc::SetMaxShutter(const Duration &max_shutter)
>  {
> -	max_shutter_ = max_shutter;
> +	max_shutter_ = max_shutter.get<std::micro>();
>  }
>  
> -void Agc::SetFixedShutter(double fixed_shutter)
> +void Agc::SetFixedShutter(const Duration &fixed_shutter)
>  {
> -	fixed_shutter_ = fixed_shutter;
> +	fixed_shutter_ = fixed_shutter.get<std::micro>();
>  	// Set this in case someone calls Pause() straight after.
>  	status_.shutter_time = clipShutter(fixed_shutter_);
>  }
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 0427fb59ec1b..2b39fcabada3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -77,9 +77,9 @@ public:
>  	void Resume() override;
>  	unsigned int GetConvergenceFrames() const override;
>  	void SetEv(double ev) override;
> -	void SetFlickerPeriod(double flicker_period) override;
> -	void SetMaxShutter(double max_shutter) override; // microseconds
> -	void SetFixedShutter(double fixed_shutter) override; // microseconds
> +	void SetFlickerPeriod(const libcamera::utils::Duration &flicker_period) override;
> +	void SetMaxShutter(const libcamera::utils::Duration &max_shutter) override;
> +	void SetFixedShutter(const libcamera::utils::Duration &fixed_shutter) override;
>  	void SetFixedAnalogueGain(double fixed_analogue_gain) override;
>  	void SetMeteringMode(std::string const &metering_mode_name) override;
>  	void SetExposureMode(std::string const &exposure_mode_name) override;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 4e02e94084f7..e083f6c254cc 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -640,8 +640,8 @@ void IPARPi::queueRequest(const ControlList &controls)
>  				break;
>  			}
>  
> -			/* This expects units of micro-seconds. */
> -			agc->SetFixedShutter(ctrl.second.get<int32_t>());
> +			/* The control provides units of microseconds. */
> +			agc->SetFixedShutter(ctrl.second.get<int32_t>() * 1.0us);
>  
>  			libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
>  			break;
> @@ -1094,7 +1094,7 @@ void IPARPi::applyFrameDurations(const Duration &minFrameDuration,
>  
>  	RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  		controller_.GetAlgorithm("agc"));
> -	agc->SetMaxShutter(maxShutter.get<std::micro>());
> +	agc->SetMaxShutter(maxShutter);
>  }
>  
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)

-- 
Regards,

Laurent Pinchart