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

[Jacopo Mondi]

Hi Naush,

On Mon, May 24, 2021 at 09:48:21AM +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>
> ---
>  src/ipa/raspberrypi/controller/agc_algorithm.hpp |  9 ++++++---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp       | 12 ++++++------
>  src/ipa/raspberrypi/controller/rpi/agc.hpp       |  6 +++---
>  src/ipa/raspberrypi/raspberrypi.cpp              |  6 +++---
>  4 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> index f97deb0fca59..dabb814ea3dd 100644
> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> @@ -6,10 +6,13 @@
>   */
>  #pragma once
>
> +#include "libcamera/internal/utils.h"
>  #include "algorithm.hpp"
>
>  namespace RPiController {
>
> +using libcamera::utils::Duration;
> +

Same comment about importing namespaces in header files. At least for
the HAL those are spelled out in full. IPA are kind of weird beasts,
part of the core library but not really, so the rule might not apply
here.

>  class AgcAlgorithm : public Algorithm
>  {
>  public:
> @@ -17,9 +20,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 Duration &flicker_period) = 0;
> +	virtual void SetFixedShutter(const Duration &fixed_shutter) = 0;
> +	virtual void SetMaxShutter(const 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..482eb3ef962d 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -222,19 +222,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..16a65959d90e 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 Duration &flicker_period) override;
> +	void SetMaxShutter(const Duration &max_shutter) override;
> +	void SetFixedShutter(const 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);

Looks good
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  }
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> --
> 2.25.1
>