[libcamera-devel,3/4] ipa: raspberrypi: Switch AgcAlgorithm API to use RPiController::Duration
diff mbox series

Message ID 20210518100706.578526-4-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Switch RaspberryPi IPA to use std::chrono::duration
Related show

Commit Message

Naushir Patuck May 18, 2021, 10:07 a.m. UTC 
Switch the AgcAlgorithm API functions to use RPiController::Duration
for all time based variables.

Signed-off-by: Naushir Patuck <naush@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(-)

Comments

David Plowman May 19, 2021, 2:21 p.m. UTC | #1 
Hi Naush

Thanks for this work!

On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Switch the AgcAlgorithm API functions to use RPiController::Duration
> for all time based variables.
>
> Signed-off-by: Naushir Patuck <naush@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@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
>
Naushir Patuck May 19, 2021, 2:27 p.m. UTC | #2 
Hi David,

Thank you for your feedback.

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

> Hi Naush
>
> Thanks for this work!
>
> On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com>
> wrote:
> >
> > Switch the AgcAlgorithm API functions to use RPiController::Duration
> > for all time based variables.
> >
> > Signed-off-by: Naushir Patuck <naush@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@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
> >
>

Patch
diff mbox series 

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);
 }
 
-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);
 
 			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)