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

[David Plowman]

Hi Naush

Thanks for this work!

On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Switch the AgcAlgorithm API functions to use RPiController::Duration
> for all time based variables.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/agc_algorithm.hpp |  7 ++++---
>  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, 16 insertions(+), 15 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> index f97deb0fca59..668912c47388 100644
> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> @@ -7,6 +7,7 @@
>  #pragma once
>
>  #include "algorithm.hpp"
> +#include "duration.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(Duration flicker_period) = 0;
> +       virtual void SetFixedShutter(Duration fixed_shutter) = 0;
> +       virtual void SetMaxShutter(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..1cb4472b2691 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(Duration flicker_period)
>  {
> -       flicker_period_ = flicker_period;
> +       flicker_period_ = DurationValue<std::micro>(flicker_period);

I assume these will all get nicer again in the next patch...!

>  }
>
> -void Agc::SetMaxShutter(double max_shutter)
> +void Agc::SetMaxShutter(Duration max_shutter)
>  {
> -       max_shutter_ = max_shutter;
> +       max_shutter_ = DurationValue<std::micro>(max_shutter);
>  }
>
> -void Agc::SetFixedShutter(double fixed_shutter)
> +void Agc::SetFixedShutter(Duration fixed_shutter)
>  {
> -       fixed_shutter_ = fixed_shutter;
> +       fixed_shutter_ = DurationValue<std::micro>(fixed_shutter);
>         // 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..cb79bf61ba42 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(Duration flicker_period) override;
> +       void SetMaxShutter(Duration max_shutter) override;
> +       void SetFixedShutter(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 994fb7e057a9..f080f2e53bac 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -643,8 +643,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);

I suppose I just wanted to ask if this is the "proper" way to convert
to a Duration - it certainly seems quite clear and tidy.

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks
David

>
>                         libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
>                         break;
> @@ -1097,7 +1097,7 @@ void IPARPi::applyFrameDurations(const Duration &minFrameDuration,
>
>         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>                 controller_.GetAlgorithm("agc"));
> -       agc->SetMaxShutter(DurationValue<std::micro>(maxShutter));
> +       agc->SetMaxShutter(maxShutter);
>  }
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> --
> 2.25.1
>