[libcamera-devel,v4,2/4] ipa: raspberrypi: Switch ipa and cam_helper to use utils::Duration
diff mbox series

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

Commit Message

Naushir Patuck May 24, 2021, 8:48 a.m. UTC 
Switch the ipa and cam_helper code to use libcamera::utils::Duration for
all time based variables. This improves code readability and avoids
possible errors when converting between time bases.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp           | 19 +++---
 src/ipa/raspberrypi/cam_helper.hpp           | 12 ++--
 src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
 src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
 4 files changed, 56 insertions(+), 45 deletions(-)

Comments

Jacopo Mondi May 24, 2021, 12:19 p.m. UTC | #1 
Hi Naush,

On Mon, May 24, 2021 at 09:48:20AM +0100, Naushir Patuck wrote:
> Switch the ipa and cam_helper code to use libcamera::utils::Duration for
> all time based variables. This improves code readability and avoids
> possible errors when converting between time bases.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp           | 19 +++---
>  src/ipa/raspberrypi/cam_helper.hpp           | 12 ++--
>  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
>  src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
>  4 files changed, 56 insertions(+), 45 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 09917f3cc079..feb02d1806b2 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -61,20 +61,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
>  {
>  }
>
> -uint32_t CamHelper::ExposureLines(double exposure_us) const
> +uint32_t CamHelper::ExposureLines(const Duration &exposure) const
>  {
>  	assert(initialized_);
> -	return exposure_us * 1000.0 / mode_.line_length;
> +	return exposure / mode_.line_length;
>  }
>
> -double CamHelper::Exposure(uint32_t exposure_lines) const
> +Duration CamHelper::Exposure(uint32_t exposure_lines) const
>  {
>  	assert(initialized_);
> -	return exposure_lines * mode_.line_length / 1000.0;
> +	return exposure_lines * mode_.line_length;
>  }
>
> -uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
> -				 double maxFrameDuration) const
> +uint32_t CamHelper::GetVBlanking(Duration &exposure,
> +				 const Duration &minFrameDuration,
> +				 const Duration &maxFrameDuration) const
>  {
>  	uint32_t frameLengthMin, frameLengthMax, vblank;
>  	uint32_t exposureLines = ExposureLines(exposure);
> @@ -85,8 +86,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
>  	 * minFrameDuration and maxFrameDuration are clamped by the caller
>  	 * based on the limits for the active sensor mode.
>  	 */
> -	frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;
> -	frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;
> +	frameLengthMin = minFrameDuration / mode_.line_length;
> +	frameLengthMax = maxFrameDuration / mode_.line_length;
>
>  	/*
>  	 * Limit the exposure to the maximum frame duration requested, and
> @@ -182,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>  		return;
>  	}
>
> -	deviceStatus.shutter_speed = Exposure(exposureLines);
> +	deviceStatus.shutter_speed = Exposure(exposureLines).get<std::micro>();
>  	deviceStatus.analogue_gain = Gain(gainCode);
>
>  	LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> index a52f3f0b583c..71ee15463a71 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -15,10 +15,13 @@
>  #include "controller/metadata.hpp"
>  #include "md_parser.hpp"
>
> +#include "libcamera/internal/utils.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>
>  namespace RPiController {
>
> +using libcamera::utils::Duration;

usually, and you do the same below, we spell out the full namespace in
headers, and use 'using ...' in cpp files.

Up to you.

> +
>  // The CamHelper class provides a number of facilities that anyone trying
>  // to drive a camera will need to know, but which are not provided by the
>  // standard driver framework. Specifically, it provides:
> @@ -72,10 +75,11 @@ public:
>  	virtual void Prepare(libcamera::Span<const uint8_t> buffer,
>  			     Metadata &metadata);
>  	virtual void Process(StatisticsPtr &stats, Metadata &metadata);
> -	uint32_t ExposureLines(double exposure_us) const;
> -	double Exposure(uint32_t exposure_lines) const; // in us
> -	virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,
> -				      double maxFrameDuration) const;
> +	uint32_t ExposureLines(const Duration &exposure) const;
> +	Duration Exposure(uint32_t exposure_lines) const;
> +	virtual uint32_t GetVBlanking(Duration &exposure,
> +				      const Duration &minFrameDuration,
> +				      const Duration &maxFrameDuration) const;
>  	virtual uint32_t GainCode(double gain) const = 0;
>  	virtual double Gain(uint32_t gain_code) const = 0;
>  	virtual void GetDelays(int &exposure_delay, int &gain_delay,
> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h
> index 256438c931d9..2aa2335dcf90 100644
> --- a/src/ipa/raspberrypi/controller/camera_mode.h
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> @@ -8,6 +8,8 @@
>
>  #include <libcamera/transform.h>
>
> +#include "libcamera/internal/utils.h"
> +
>  // Description of a "camera mode", holding enough information for control
>  // algorithms to adapt their behaviour to the different modes of the camera,
>  // including binning, scaling, cropping etc.
> @@ -33,8 +35,8 @@ struct CameraMode {
>  	double scale_x, scale_y;
>  	// scaling of the noise compared to the native sensor mode
>  	double noise_factor;
> -	// line time in nanoseconds
> -	double line_length;
> +	// line time
> +	libcamera::utils::Duration line_length;
>  	// any camera transform *not* reflected already in the camera tuning
>  	libcamera::Transform transform;
>  	// minimum and maximum fame lengths in units of lines
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 52d91db282ea..4e02e94084f7 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -55,19 +55,22 @@
>
>  namespace libcamera {
>
> +using namespace std::literals::chrono_literals;
> +using utils::Duration;
> +
>  /* Configure the sensor with these values initially. */
>  constexpr double DefaultAnalogueGain = 1.0;
> -constexpr unsigned int DefaultExposureTime = 20000;
> -constexpr double defaultMinFrameDuration = 1e6 / 30.0;
> -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;
> +constexpr Duration DefaultExposureTime = 20.0ms;
> +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;
> +constexpr Duration defaultMaxFrameDuration = 100.0s;
>
>  /*
> - * Determine the minimum allowable inter-frame duration (in us) to run the
> - * controller algorithms. If the pipeline handler provider frames at a rate
> - * higher than this, we rate-limit the controller Prepare() and Process() calls
> - * to lower than or equal to this rate.
> + * Determine the minimum allowable inter-frame duration to run the controller
> + * algorithms. If the pipeline handler provider frames at a rate higher than this,
> + * we rate-limit the controller Prepare() and Process() calls to lower than or
> + * equal to this rate.
>   */
> -constexpr double controllerMinFrameDuration = 1e6 / 60.0;
> +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;
>
>  LOG_DEFINE_CATEGORY(IPARPI)
>
> @@ -111,7 +114,8 @@ private:
>  	void reportMetadata();
>  	void fillDeviceStatus(const ControlList &sensorControls);
>  	void processStats(unsigned int bufferId);
> -	void applyFrameDurations(double minFrameDuration, double maxFrameDuration);
> +	void applyFrameDurations(const Duration &minFrameDuration,
> +				 const Duration &maxFrameDuration);
>  	void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
>  	void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
>  	void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
> @@ -167,9 +171,9 @@ private:
>  	/* Distinguish the first camera start from others. */
>  	bool firstStart_;
>
> -	/* Frame duration (1/fps) limits, given in microseconds. */
> -	double minFrameDuration_;
> -	double maxFrameDuration_;
> +	/* Frame duration (1/fps) limits. */
> +	Duration minFrameDuration_;
> +	Duration maxFrameDuration_;
>  };
>
>  int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)
> @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  	mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);
>
>  	/*
> -	 * Calculate the line length in nanoseconds as the ratio between
> -	 * the line length in pixels and the pixel rate.
> +	 * Calculate the line length as the ratio between the line length in

I would keep the 'line length in nanoseconds' as two line lengths in

        mode.line_length = sensorInfo.lineLength

can be easily confused ? Although the mode.line_length type is a
duration. Would mode.line_duration be a better fit ?

> +	 * pixels and the pixel rate.
>  	 */
> -	mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;
> +	mode_.line_length = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);
>
>  	/*
>  	 * Set the frame length limits for the mode to ensure exposure and
> @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		/* Supply initial values for gain and exposure. */
>  		ControlList ctrls(sensorCtrls_);
>  		AgcStatus agcStatus;
> -		agcStatus.shutter_time = DefaultExposureTime;
> +		agcStatus.shutter_time = DefaultExposureTime.get<std::micro>();
>  		agcStatus.analogue_gain = DefaultAnalogueGain;
>  		applyAGC(&agcStatus, ctrls);
>
> @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>
>  		case controls::FRAME_DURATIONS: {
>  			auto frameDurations = ctrl.second.get<Span<const int64_t>>();
> -			applyFrameDurations(frameDurations[0], frameDurations[1]);
> +			applyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us);

It would be lovely to be able to avoid the 1.0us multiplication that
turns the integers frameDurations[] into an std::chrono:duration.

I wonder if a constructor that takes an integer would make sense. It
would be rather difficult to make sure the integer passed to the
constructor is diligently specified in nanoseconds.

Or, can we make controls like frameDurations being Durations ? We can
use libcamera types in controls definition after all...

>  			break;
>  		}
>
> @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>  		returnEmbeddedBuffer(data.embeddedBufferId);
>
>  	/* Allow a 10% margin on the comparison below. */
> -	constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;
> +	Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
>  	if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> -	    frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {
> +	    delta < controllerMinFrameDuration * 0.9) {
>  		/*
>  		 * Ensure we merge the previous frame's metadata with the current
>  		 * frame. This will not overwrite exposure/gain values for the
> @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
>  	int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>  	int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>
> -	deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> +	deviceStatus.shutter_speed = helper_->Exposure(exposureLines).get<std::micro>();
>  	deviceStatus.analogue_gain = helper_->Gain(gainCode);
>
>  	LOG(IPARPI, Debug) << "Metadata - Exposure : "
> @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  		  static_cast<int32_t>(awbStatus->gain_b * 1000));
>  }
>
> -void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)
> +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,
> +				 const Duration &maxFrameDuration)
>  {
> -	const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length;
> -	const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length;
> +	const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;
> +	const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
>
>  	/*
>  	 * This will only be applied once AGC recalculations occur.
> @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio
>
>  	/* Return the validated limits via metadata. */
>  	libcameraMetadata_.set(controls::FrameDurations,
> -			       { static_cast<int64_t>(minFrameDuration_),
> -				 static_cast<int64_t>(maxFrameDuration_) });
> +			       { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> +				 static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
>
>  	/*
>  	 * Calculate the maximum exposure time possible for the AGC to use.
>  	 * GetVBlanking() will update maxShutter with the largest exposure
>  	 * value possible.
>  	 */
> -	double maxShutter = std::numeric_limits<double>::max();
> +	Duration maxShutter = Duration::max();
>  	helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);
>
>  	RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>  		controller_.GetAlgorithm("agc"));
> -	agc->SetMaxShutter(maxShutter);
> +	agc->SetMaxShutter(maxShutter.get<std::micro>());
>  }
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
>
>  	/* GetVBlanking might clip exposure time to the fps limits. */
> -	double exposure = agcStatus->shutter_time;
> -	int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,
> -						  maxFrameDuration_);
> +	Duration exposure = agcStatus->shutter_time * 1.0us;
> +	int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);

Overall, very nice introduction, Duration will be really helpful.

One additional concern for sake of discussion. Do we need to specify
the time unit ? As in DurationNs ?

Thanks
  j

>  	int32_t exposureLines = helper_->ExposureLines(exposure);
>
>  	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> --
> 2.25.1
>
Naushir Patuck May 24, 2021, 12:52 p.m. UTC | #2 
Hi Jacopo,

Thank you for the review feedback.

On Mon, 24 May 2021 at 13:19, Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Naush,
>
> On Mon, May 24, 2021 at 09:48:20AM +0100, Naushir Patuck wrote:
> > Switch the ipa and cam_helper code to use libcamera::utils::Duration for
> > all time based variables. This improves code readability and avoids
> > possible errors when converting between time bases.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp           | 19 +++---
> >  src/ipa/raspberrypi/cam_helper.hpp           | 12 ++--
> >  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
> >  4 files changed, 56 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index 09917f3cc079..feb02d1806b2 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -61,20 +61,21 @@ void CamHelper::Process([[maybe_unused]]
> StatisticsPtr &stats,
> >  {
> >  }
> >
> > -uint32_t CamHelper::ExposureLines(double exposure_us) const
> > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const
> >  {
> >       assert(initialized_);
> > -     return exposure_us * 1000.0 / mode_.line_length;
> > +     return exposure / mode_.line_length;
> >  }
> >
> > -double CamHelper::Exposure(uint32_t exposure_lines) const
> > +Duration CamHelper::Exposure(uint32_t exposure_lines) const
> >  {
> >       assert(initialized_);
> > -     return exposure_lines * mode_.line_length / 1000.0;
> > +     return exposure_lines * mode_.line_length;
> >  }
> >
> > -uint32_t CamHelper::GetVBlanking(double &exposure, double
> minFrameDuration,
> > -                              double maxFrameDuration) const
> > +uint32_t CamHelper::GetVBlanking(Duration &exposure,
> > +                              const Duration &minFrameDuration,
> > +                              const Duration &maxFrameDuration) const
> >  {
> >       uint32_t frameLengthMin, frameLengthMax, vblank;
> >       uint32_t exposureLines = ExposureLines(exposure);
> > @@ -85,8 +86,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure,
> double minFrameDuration,
> >        * minFrameDuration and maxFrameDuration are clamped by the caller
> >        * based on the limits for the active sensor mode.
> >        */
> > -     frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;
> > -     frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;
> > +     frameLengthMin = minFrameDuration / mode_.line_length;
> > +     frameLengthMax = maxFrameDuration / mode_.line_length;
> >
> >       /*
> >        * Limit the exposure to the maximum frame duration requested, and
> > @@ -182,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> >               return;
> >       }
> >
> > -     deviceStatus.shutter_speed = Exposure(exposureLines);
> > +     deviceStatus.shutter_speed =
> Exposure(exposureLines).get<std::micro>();
> >       deviceStatus.analogue_gain = Gain(gainCode);
> >
> >       LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> b/src/ipa/raspberrypi/cam_helper.hpp
> > index a52f3f0b583c..71ee15463a71 100644
> > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > @@ -15,10 +15,13 @@
> >  #include "controller/metadata.hpp"
> >  #include "md_parser.hpp"
> >
> > +#include "libcamera/internal/utils.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> >
> >  namespace RPiController {
> >
> > +using libcamera::utils::Duration;
>
> usually, and you do the same below, we spell out the full namespace in
> headers, and use 'using ...' in cpp files.
>
> Up to you.
>

Agreed, to be consistent, I will remove the using... and expand the
namespace
below.


>
> > +
> >  // The CamHelper class provides a number of facilities that anyone
> trying
> >  // to drive a camera will need to know, but which are not provided by
> the
> >  // standard driver framework. Specifically, it provides:
> > @@ -72,10 +75,11 @@ public:
> >       virtual void Prepare(libcamera::Span<const uint8_t> buffer,
> >                            Metadata &metadata);
> >       virtual void Process(StatisticsPtr &stats, Metadata &metadata);
> > -     uint32_t ExposureLines(double exposure_us) const;
> > -     double Exposure(uint32_t exposure_lines) const; // in us
> > -     virtual uint32_t GetVBlanking(double &exposure_us, double
> minFrameDuration,
> > -                                   double maxFrameDuration) const;
> > +     uint32_t ExposureLines(const Duration &exposure) const;
> > +     Duration Exposure(uint32_t exposure_lines) const;
> > +     virtual uint32_t GetVBlanking(Duration &exposure,
> > +                                   const Duration &minFrameDuration,
> > +                                   const Duration &maxFrameDuration)
> const;
> >       virtual uint32_t GainCode(double gain) const = 0;
> >       virtual double Gain(uint32_t gain_code) const = 0;
> >       virtual void GetDelays(int &exposure_delay, int &gain_delay,
> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h
> b/src/ipa/raspberrypi/controller/camera_mode.h
> > index 256438c931d9..2aa2335dcf90 100644
> > --- a/src/ipa/raspberrypi/controller/camera_mode.h
> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> > @@ -8,6 +8,8 @@
> >
> >  #include <libcamera/transform.h>
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  // Description of a "camera mode", holding enough information for
> control
> >  // algorithms to adapt their behaviour to the different modes of the
> camera,
> >  // including binning, scaling, cropping etc.
> > @@ -33,8 +35,8 @@ struct CameraMode {
> >       double scale_x, scale_y;
> >       // scaling of the noise compared to the native sensor mode
> >       double noise_factor;
> > -     // line time in nanoseconds
> > -     double line_length;
> > +     // line time
> > +     libcamera::utils::Duration line_length;
> >       // any camera transform *not* reflected already in the camera
> tuning
> >       libcamera::Transform transform;
> >       // minimum and maximum fame lengths in units of lines
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 52d91db282ea..4e02e94084f7 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -55,19 +55,22 @@
> >
> >  namespace libcamera {
> >
> > +using namespace std::literals::chrono_literals;
> > +using utils::Duration;
> > +
> >  /* Configure the sensor with these values initially. */
> >  constexpr double DefaultAnalogueGain = 1.0;
> > -constexpr unsigned int DefaultExposureTime = 20000;
> > -constexpr double defaultMinFrameDuration = 1e6 / 30.0;
> > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;
> > +constexpr Duration DefaultExposureTime = 20.0ms;
> > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;
> > +constexpr Duration defaultMaxFrameDuration = 100.0s;
> >
> >  /*
> > - * Determine the minimum allowable inter-frame duration (in us) to run
> the
> > - * controller algorithms. If the pipeline handler provider frames at a
> rate
> > - * higher than this, we rate-limit the controller Prepare() and
> Process() calls
> > - * to lower than or equal to this rate.
> > + * Determine the minimum allowable inter-frame duration to run the
> controller
> > + * algorithms. If the pipeline handler provider frames at a rate higher
> than this,
> > + * we rate-limit the controller Prepare() and Process() calls to lower
> than or
> > + * equal to this rate.
> >   */
> > -constexpr double controllerMinFrameDuration = 1e6 / 60.0;
> > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;
> >
> >  LOG_DEFINE_CATEGORY(IPARPI)
> >
> > @@ -111,7 +114,8 @@ private:
> >       void reportMetadata();
> >       void fillDeviceStatus(const ControlList &sensorControls);
> >       void processStats(unsigned int bufferId);
> > -     void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
> > +     void applyFrameDurations(const Duration &minFrameDuration,
> > +                              const Duration &maxFrameDuration);
> >       void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> >       void applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls);
> >       void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
> > @@ -167,9 +171,9 @@ private:
> >       /* Distinguish the first camera start from others. */
> >       bool firstStart_;
> >
> > -     /* Frame duration (1/fps) limits, given in microseconds. */
> > -     double minFrameDuration_;
> > -     double maxFrameDuration_;
> > +     /* Frame duration (1/fps) limits. */
> > +     Duration minFrameDuration_;
> > +     Duration maxFrameDuration_;
> >  };
> >
> >  int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig
> *sensorConfig)
> > @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
> >       mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);
> >
> >       /*
> > -      * Calculate the line length in nanoseconds as the ratio between
> > -      * the line length in pixels and the pixel rate.
> > +      * Calculate the line length as the ratio between the line length
> in
>
> I would keep the 'line length in nanoseconds' as two line lengths in
>
>         mode.line_length = sensorInfo.lineLength
>
> can be easily confused ? Although the mode.line_length type is a
> duration. Would mode.line_duration be a better fit ?
>

I see your point here.  However, as you pointed out mode.line_length is a
Duration type, and
I did not want to specify a time unit in the comment, as Duration hides all
of that for you.
Perhaps I should update the comment like:

s/Calculate the line length as the ratio/Calculate the line length duration
as the ratio/

to be more explicit?  I would prefer not to rename mode.line_length if at
all possible, as that
is (afaik) standard sensor device terminology.


> > +      * pixels and the pixel rate.
> >        */
> > -     mode_.line_length = 1e9 * sensorInfo.lineLength /
> sensorInfo.pixelRate;
> > +     mode_.line_length = sensorInfo.lineLength * (1.0s /
> sensorInfo.pixelRate);
> >
> >       /*
> >        * Set the frame length limits for the mode to ensure exposure and
> > @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >               /* Supply initial values for gain and exposure. */
> >               ControlList ctrls(sensorCtrls_);
> >               AgcStatus agcStatus;
> > -             agcStatus.shutter_time = DefaultExposureTime;
> > +             agcStatus.shutter_time =
> DefaultExposureTime.get<std::micro>();
> >               agcStatus.analogue_gain = DefaultAnalogueGain;
> >               applyAGC(&agcStatus, ctrls);
> >
> > @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList
> &controls)
> >
> >               case controls::FRAME_DURATIONS: {
> >                       auto frameDurations = ctrl.second.get<Span<const
> int64_t>>();
> > -                     applyFrameDurations(frameDurations[0],
> frameDurations[1]);
> > +                     applyFrameDurations(frameDurations[0] * 1.0us,
> frameDurations[1] * 1.0us);
>
> It would be lovely to be able to avoid the 1.0us multiplication that
> turns the integers frameDurations[] into an std::chrono:duration.
>

The alternative to this is std::chrono::microseconds(frameDuration[0]).
I have a slight preference using "1.0us" over that though.  If you feel
it is better, I am happy to change.


>
> I wonder if a constructor that takes an integer would make sense. It
> would be rather difficult to make sure the integer passed to the
> constructor is diligently specified in nanoseconds.
>

This was my first thought as well, but it becomes a bit ambiguous.  It
required
the user to know that Duration is a nanosecond base, and frameDuration
is a microsecond base, and we must multiply by 1e3, and that becomes
error-prone.


> Or, can we make controls like frameDurations being Durations ? We can
> use libcamera types in controls definition after all...
>

I think this should certainly be the end goal of this work!  When all code
starts to
use Duration, none of these integer -> Duration conversion (apart from the
start/end
of the chain of course) will be needed.

I was a bit afraid of doing the wholesale change across all libcamera
without some feedback.
We can use the Raspberry Pi source code as a test-bed in the short term :-)


> >                       break;
> >               }
> >
> > @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig
> &data)
> >               returnEmbeddedBuffer(data.embeddedBufferId);
> >
> >       /* Allow a 10% margin on the comparison below. */
> > -     constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;
> > +     Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> >       if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > -         frameTimestamp - lastRunTimestamp_ + eps <
> controllerMinFrameDuration * 1e3) {
> > +         delta < controllerMinFrameDuration * 0.9) {
> >               /*
> >                * Ensure we merge the previous frame's metadata with the
> current
> >                * frame. This will not overwrite exposure/gain values for
> the
> > @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls)
> >       int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >       int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >
> > -     deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > +     deviceStatus.shutter_speed =
> helper_->Exposure(exposureLines).get<std::micro>();
> >       deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >
> >       LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus
> *awbStatus, ControlList &ctrls)
> >                 static_cast<int32_t>(awbStatus->gain_b * 1000));
> >  }
> >
> > -void IPARPi::applyFrameDurations(double minFrameDuration, double
> maxFrameDuration)
> > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,
> > +                              const Duration &maxFrameDuration)
> >  {
> > -     const double minSensorFrameDuration = 1e-3 *
> mode_.min_frame_length * mode_.line_length;
> > -     const double maxSensorFrameDuration = 1e-3 *
> mode_.max_frame_length * mode_.line_length;
> > +     const Duration minSensorFrameDuration = mode_.min_frame_length *
> mode_.line_length;
> > +     const Duration maxSensorFrameDuration = mode_.max_frame_length *
> mode_.line_length;
> >
> >       /*
> >        * This will only be applied once AGC recalculations occur.
> > @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double
> minFrameDuration, double maxFrameDuratio
> >
> >       /* Return the validated limits via metadata. */
> >       libcameraMetadata_.set(controls::FrameDurations,
> > -                            { static_cast<int64_t>(minFrameDuration_),
> > -                              static_cast<int64_t>(maxFrameDuration_)
> });
> > +                            {
> static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> > +
> static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
> >
> >       /*
> >        * Calculate the maximum exposure time possible for the AGC to use.
> >        * GetVBlanking() will update maxShutter with the largest exposure
> >        * value possible.
> >        */
> > -     double maxShutter = std::numeric_limits<double>::max();
> > +     Duration maxShutter = Duration::max();
> >       helper_->GetVBlanking(maxShutter, minFrameDuration_,
> maxFrameDuration_);
> >
> >       RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> >               controller_.GetAlgorithm("agc"));
> > -     agc->SetMaxShutter(maxShutter);
> > +     agc->SetMaxShutter(maxShutter.get<std::micro>());
> >  }
> >
> >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls)
> > @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> >       int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> >
> >       /* GetVBlanking might clip exposure time to the fps limits. */
> > -     double exposure = agcStatus->shutter_time;
> > -     int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_,
> > -                                               maxFrameDuration_);
> > +     Duration exposure = agcStatus->shutter_time * 1.0us;
> > +     int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
>
> Overall, very nice introduction, Duration will be really helpful.
>

> One additional concern for sake of discussion. Do we need to specify
> the time unit ? As in DurationNs ?
>

My preference would be not to put a unit to Duration.  The idea is to
ensure the user (as much
as is possible) does not worry about the internal representation, and let
the compiler worry
about it.  I don't think there would be any scenario where having us know
that Duration is
stored as ns would be helpful, since we have a Duration::get<T> to do any
conversions needed.

Regards,
Naush


>
> Thanks
>   j
>
> >       int32_t exposureLines = helper_->ExposureLines(exposure);
> >
> >       LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> > --
> > 2.25.1
> >
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index 09917f3cc079..feb02d1806b2 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -61,20 +61,21 @@  void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
 {
 }
 
-uint32_t CamHelper::ExposureLines(double exposure_us) const
+uint32_t CamHelper::ExposureLines(const Duration &exposure) const
 {
 	assert(initialized_);
-	return exposure_us * 1000.0 / mode_.line_length;
+	return exposure / mode_.line_length;
 }
 
-double CamHelper::Exposure(uint32_t exposure_lines) const
+Duration CamHelper::Exposure(uint32_t exposure_lines) const
 {
 	assert(initialized_);
-	return exposure_lines * mode_.line_length / 1000.0;
+	return exposure_lines * mode_.line_length;
 }
 
-uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
-				 double maxFrameDuration) const
+uint32_t CamHelper::GetVBlanking(Duration &exposure,
+				 const Duration &minFrameDuration,
+				 const Duration &maxFrameDuration) const
 {
 	uint32_t frameLengthMin, frameLengthMax, vblank;
 	uint32_t exposureLines = ExposureLines(exposure);
@@ -85,8 +86,8 @@  uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
 	 * minFrameDuration and maxFrameDuration are clamped by the caller
 	 * based on the limits for the active sensor mode.
 	 */
-	frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;
-	frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;
+	frameLengthMin = minFrameDuration / mode_.line_length;
+	frameLengthMax = maxFrameDuration / mode_.line_length;
 
 	/*
 	 * Limit the exposure to the maximum frame duration requested, and
@@ -182,7 +183,7 @@  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
 		return;
 	}
 
-	deviceStatus.shutter_speed = Exposure(exposureLines);
+	deviceStatus.shutter_speed = Exposure(exposureLines).get<std::micro>();
 	deviceStatus.analogue_gain = Gain(gainCode);
 
 	LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
index a52f3f0b583c..71ee15463a71 100644
--- a/src/ipa/raspberrypi/cam_helper.hpp
+++ b/src/ipa/raspberrypi/cam_helper.hpp
@@ -15,10 +15,13 @@ 
 #include "controller/metadata.hpp"
 #include "md_parser.hpp"
 
+#include "libcamera/internal/utils.h"
 #include "libcamera/internal/v4l2_videodevice.h"
 
 namespace RPiController {
 
+using libcamera::utils::Duration;
+
 // The CamHelper class provides a number of facilities that anyone trying
 // to drive a camera will need to know, but which are not provided by the
 // standard driver framework. Specifically, it provides:
@@ -72,10 +75,11 @@  public:
 	virtual void Prepare(libcamera::Span<const uint8_t> buffer,
 			     Metadata &metadata);
 	virtual void Process(StatisticsPtr &stats, Metadata &metadata);
-	uint32_t ExposureLines(double exposure_us) const;
-	double Exposure(uint32_t exposure_lines) const; // in us
-	virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,
-				      double maxFrameDuration) const;
+	uint32_t ExposureLines(const Duration &exposure) const;
+	Duration Exposure(uint32_t exposure_lines) const;
+	virtual uint32_t GetVBlanking(Duration &exposure,
+				      const Duration &minFrameDuration,
+				      const Duration &maxFrameDuration) const;
 	virtual uint32_t GainCode(double gain) const = 0;
 	virtual double Gain(uint32_t gain_code) const = 0;
 	virtual void GetDelays(int &exposure_delay, int &gain_delay,
diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h
index 256438c931d9..2aa2335dcf90 100644
--- a/src/ipa/raspberrypi/controller/camera_mode.h
+++ b/src/ipa/raspberrypi/controller/camera_mode.h
@@ -8,6 +8,8 @@ 
 
 #include <libcamera/transform.h>
 
+#include "libcamera/internal/utils.h"
+
 // Description of a "camera mode", holding enough information for control
 // algorithms to adapt their behaviour to the different modes of the camera,
 // including binning, scaling, cropping etc.
@@ -33,8 +35,8 @@  struct CameraMode {
 	double scale_x, scale_y;
 	// scaling of the noise compared to the native sensor mode
 	double noise_factor;
-	// line time in nanoseconds
-	double line_length;
+	// line time
+	libcamera::utils::Duration line_length;
 	// any camera transform *not* reflected already in the camera tuning
 	libcamera::Transform transform;
 	// minimum and maximum fame lengths in units of lines
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 52d91db282ea..4e02e94084f7 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -55,19 +55,22 @@ 
 
 namespace libcamera {
 
+using namespace std::literals::chrono_literals;
+using utils::Duration;
+
 /* Configure the sensor with these values initially. */
 constexpr double DefaultAnalogueGain = 1.0;
-constexpr unsigned int DefaultExposureTime = 20000;
-constexpr double defaultMinFrameDuration = 1e6 / 30.0;
-constexpr double defaultMaxFrameDuration = 1e6 / 0.01;
+constexpr Duration DefaultExposureTime = 20.0ms;
+constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;
+constexpr Duration defaultMaxFrameDuration = 100.0s;
 
 /*
- * Determine the minimum allowable inter-frame duration (in us) to run the
- * controller algorithms. If the pipeline handler provider frames at a rate
- * higher than this, we rate-limit the controller Prepare() and Process() calls
- * to lower than or equal to this rate.
+ * Determine the minimum allowable inter-frame duration to run the controller
+ * algorithms. If the pipeline handler provider frames at a rate higher than this,
+ * we rate-limit the controller Prepare() and Process() calls to lower than or
+ * equal to this rate.
  */
-constexpr double controllerMinFrameDuration = 1e6 / 60.0;
+constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;
 
 LOG_DEFINE_CATEGORY(IPARPI)
 
@@ -111,7 +114,8 @@  private:
 	void reportMetadata();
 	void fillDeviceStatus(const ControlList &sensorControls);
 	void processStats(unsigned int bufferId);
-	void applyFrameDurations(double minFrameDuration, double maxFrameDuration);
+	void applyFrameDurations(const Duration &minFrameDuration,
+				 const Duration &maxFrameDuration);
 	void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
 	void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
 	void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
@@ -167,9 +171,9 @@  private:
 	/* Distinguish the first camera start from others. */
 	bool firstStart_;
 
-	/* Frame duration (1/fps) limits, given in microseconds. */
-	double minFrameDuration_;
-	double maxFrameDuration_;
+	/* Frame duration (1/fps) limits. */
+	Duration minFrameDuration_;
+	Duration maxFrameDuration_;
 };
 
 int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)
@@ -311,10 +315,10 @@  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
 	mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);
 
 	/*
-	 * Calculate the line length in nanoseconds as the ratio between
-	 * the line length in pixels and the pixel rate.
+	 * Calculate the line length as the ratio between the line length in
+	 * pixels and the pixel rate.
 	 */
-	mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;
+	mode_.line_length = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);
 
 	/*
 	 * Set the frame length limits for the mode to ensure exposure and
@@ -387,7 +391,7 @@  int IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		/* Supply initial values for gain and exposure. */
 		ControlList ctrls(sensorCtrls_);
 		AgcStatus agcStatus;
-		agcStatus.shutter_time = DefaultExposureTime;
+		agcStatus.shutter_time = DefaultExposureTime.get<std::micro>();
 		agcStatus.analogue_gain = DefaultAnalogueGain;
 		applyAGC(&agcStatus, ctrls);
 
@@ -862,7 +866,7 @@  void IPARPi::queueRequest(const ControlList &controls)
 
 		case controls::FRAME_DURATIONS: {
 			auto frameDurations = ctrl.second.get<Span<const int64_t>>();
-			applyFrameDurations(frameDurations[0], frameDurations[1]);
+			applyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us);
 			break;
 		}
 
@@ -937,9 +941,9 @@  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
 		returnEmbeddedBuffer(data.embeddedBufferId);
 
 	/* Allow a 10% margin on the comparison below. */
-	constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;
+	Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
 	if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
-	    frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {
+	    delta < controllerMinFrameDuration * 0.9) {
 		/*
 		 * Ensure we merge the previous frame's metadata with the current
 		 * frame. This will not overwrite exposure/gain values for the
@@ -1012,7 +1016,7 @@  void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
 	int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
 	int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
 
-	deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
+	deviceStatus.shutter_speed = helper_->Exposure(exposureLines).get<std::micro>();
 	deviceStatus.analogue_gain = helper_->Gain(gainCode);
 
 	LOG(IPARPI, Debug) << "Metadata - Exposure : "
@@ -1057,10 +1061,11 @@  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 		  static_cast<int32_t>(awbStatus->gain_b * 1000));
 }
 
-void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)
+void IPARPi::applyFrameDurations(const Duration &minFrameDuration,
+				 const Duration &maxFrameDuration)
 {
-	const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length;
-	const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length;
+	const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;
+	const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
 
 	/*
 	 * This will only be applied once AGC recalculations occur.
@@ -1076,20 +1081,20 @@  void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio
 
 	/* Return the validated limits via metadata. */
 	libcameraMetadata_.set(controls::FrameDurations,
-			       { static_cast<int64_t>(minFrameDuration_),
-				 static_cast<int64_t>(maxFrameDuration_) });
+			       { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
+				 static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
 
 	/*
 	 * Calculate the maximum exposure time possible for the AGC to use.
 	 * GetVBlanking() will update maxShutter with the largest exposure
 	 * value possible.
 	 */
-	double maxShutter = std::numeric_limits<double>::max();
+	Duration maxShutter = Duration::max();
 	helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);
 
 	RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
 		controller_.GetAlgorithm("agc"));
-	agc->SetMaxShutter(maxShutter);
+	agc->SetMaxShutter(maxShutter.get<std::micro>());
 }
 
 void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
@@ -1097,9 +1102,8 @@  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
 
 	/* GetVBlanking might clip exposure time to the fps limits. */
-	double exposure = agcStatus->shutter_time;
-	int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,
-						  maxFrameDuration_);
+	Duration exposure = agcStatus->shutter_time * 1.0us;
+	int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);
 	int32_t exposureLines = helper_->ExposureLines(exposure);
 
 	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure