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

[Naushir Patuck]

Hi David,

Thank you for your feedback.

On Wed, 19 May 2021 at 15:21, David Plowman <david.plowman at raspberrypi.com>
wrote:

> 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...!
>

Yes!


>
> >  }
> >
> > -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.
>

You can either use the above or the constructor form Duration(x).  I find
the
above a bit more intuitive, as you do not need to know the time units of the
Duration object.

Regards,
Naush



>
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210519/c454ccce/attachment.htm>