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

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

Commit Message

Naushir Patuck May 25, 2021, 11:42 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           | 20 +++---
 src/ipa/raspberrypi/cam_helper.hpp           | 10 +--
 src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
 src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
 4 files changed, 55 insertions(+), 45 deletions(-)

Comments

Naushir Patuck June 2, 2021, 1:11 p.m. UTC | #1
Hi all,

Gentle ping to have some feedback for this patch.

All other patches in this series have 2x R-b tags, so once this has another
review, I think
the series can be merged.

Regards,
Naush


On Tue, 25 May 2021 at 12:42, Naushir Patuck <naush@raspberrypi.com> 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           | 20 +++---
>  src/ipa/raspberrypi/cam_helper.hpp           | 10 +--
>  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
>  src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
>  4 files changed, 55 insertions(+), 45 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index 09917f3cc079..7b4331a87a42 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -18,6 +18,7 @@
>
>  using namespace RPiController;
>  using namespace libcamera;
> +using libcamera::utils::Duration;
>
>  namespace libcamera {
>  LOG_DECLARE_CATEGORY(IPARPI)
> @@ -61,20 +62,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 +87,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 +184,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..07481e4174a7 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -15,6 +15,7 @@
>  #include "controller/metadata.hpp"
>  #include "md_parser.hpp"
>
> +#include "libcamera/internal/utils.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>
>  namespace RPiController {
> @@ -72,10 +73,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 libcamera::utils::Duration &exposure)
> const;
> +       libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;
> +       virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,
> +                                     const libcamera::utils::Duration
> &minFrameDuration,
> +                                     const libcamera::utils::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
> --
> 2.25.1
>
>
Laurent Pinchart June 8, 2021, 1:16 a.m. UTC | #2
Hi Naush,

Thank you for the patch.

On Tue, May 25, 2021 at 12:42:39PM +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           | 20 +++---
>  src/ipa/raspberrypi/cam_helper.hpp           | 10 +--
>  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
>  src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
>  4 files changed, 55 insertions(+), 45 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 09917f3cc079..7b4331a87a42 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -18,6 +18,7 @@
>  
>  using namespace RPiController;
>  using namespace libcamera;
> +using libcamera::utils::Duration;

I would have used utils::Duration below, but I don't mind.

>  
>  namespace libcamera {
>  LOG_DECLARE_CATEGORY(IPARPI)
> @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
>  {
>  }
>  
> -uint32_t CamHelper::ExposureLines(double exposure_us) const
> +uint32_t CamHelper::ExposureLines(const Duration &exposure) const

Given that the duration only stores a double value, you could pass it by
value instead of by reference. The comment applies to the whole series.
That would be my preference, but it's up to you.

Overall the code is really much nicer !

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  {
>  	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 +87,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 +184,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..07481e4174a7 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -15,6 +15,7 @@
>  #include "controller/metadata.hpp"
>  #include "md_parser.hpp"
>  
> +#include "libcamera/internal/utils.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
>  namespace RPiController {
> @@ -72,10 +73,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 libcamera::utils::Duration &exposure) const;
> +	libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;
> +	virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,
> +				      const libcamera::utils::Duration &minFrameDuration,
> +				      const libcamera::utils::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
Naushir Patuck June 8, 2021, 9:47 a.m. UTC | #3
Hi Laurent,

Thank you for your review feedback.

On Tue, 8 Jun 2021 at 02:17, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Tue, May 25, 2021 at 12:42:39PM +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           | 20 +++---
> >  src/ipa/raspberrypi/cam_helper.hpp           | 10 +--
> >  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
> >  4 files changed, 55 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index 09917f3cc079..7b4331a87a42 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -18,6 +18,7 @@
> >
> >  using namespace RPiController;
> >  using namespace libcamera;
> > +using libcamera::utils::Duration;
>
> I would have used utils::Duration below, but I don't mind.
>
> >
> >  namespace libcamera {
> >  LOG_DECLARE_CATEGORY(IPARPI)
> > @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]]
> StatisticsPtr &stats,
> >  {
> >  }
> >
> > -uint32_t CamHelper::ExposureLines(double exposure_us) const
> > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const
>
> Given that the duration only stores a double value, you could pass it by
> value instead of by reference. The comment applies to the whole series.
> That would be my preference, but it's up to you.
>

Agreed, I will change all functions to pass by value in the next revision.


>
> Overall the code is really much nicer !
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

Thanks.  Yes, this change does make time based variables so much easier
to work with!  At some point, we should change libcamera controls to use
this :-)

Regards,
Naush


> >  {
> >       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 +87,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 +184,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..07481e4174a7 100644
> > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > @@ -15,6 +15,7 @@
> >  #include "controller/metadata.hpp"
> >  #include "md_parser.hpp"
> >
> > +#include "libcamera/internal/utils.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> >
> >  namespace RPiController {
> > @@ -72,10 +73,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 libcamera::utils::Duration &exposure)
> const;
> > +     libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;
> > +     virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,
> > +                                   const libcamera::utils::Duration
> &minFrameDuration,
> > +                                   const libcamera::utils::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
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart June 8, 2021, 9:53 a.m. UTC | #4
Hi Naush,

On Tue, Jun 08, 2021 at 10:47:47AM +0100, Naushir Patuck wrote:
> On Tue, 8 Jun 2021 at 02:17, Laurent Pinchart wrote:
> > On Tue, May 25, 2021 at 12:42:39PM +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           | 20 +++---
> > >  src/ipa/raspberrypi/cam_helper.hpp           | 10 +--
> > >  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
> > >  src/ipa/raspberrypi/raspberrypi.cpp          | 64 +++++++++++---------
> > >  4 files changed, 55 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > > index 09917f3cc079..7b4331a87a42 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > @@ -18,6 +18,7 @@
> > >
> > >  using namespace RPiController;
> > >  using namespace libcamera;
> > > +using libcamera::utils::Duration;
> >
> > I would have used utils::Duration below, but I don't mind.
> >
> > >
> > >  namespace libcamera {
> > >  LOG_DECLARE_CATEGORY(IPARPI)
> > > @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
> > >  {
> > >  }
> > >
> > > -uint32_t CamHelper::ExposureLines(double exposure_us) const
> > > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const
> >
> > Given that the duration only stores a double value, you could pass it by
> > value instead of by reference. The comment applies to the whole series.
> > That would be my preference, but it's up to you.
> 
> Agreed, I will change all functions to pass by value in the next revision.
> 
> > Overall the code is really much nicer !
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks.  Yes, this change does make time based variables so much easier
> to work with!  At some point, we should change libcamera controls to use
> this :-)

That's on my todo list :-)

By the way, would it be possible to add a test case for the Duration
class in the next version ?

> > >  {
> > >       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 +87,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 +184,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..07481e4174a7 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > @@ -15,6 +15,7 @@
> > >  #include "controller/metadata.hpp"
> > >  #include "md_parser.hpp"
> > >
> > > +#include "libcamera/internal/utils.h"
> > >  #include "libcamera/internal/v4l2_videodevice.h"
> > >
> > >  namespace RPiController {
> > > @@ -72,10 +73,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 libcamera::utils::Duration &exposure) const;
> > > +     libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;
> > > +     virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,
> > > +                                   const libcamera::utils::Duration &minFrameDuration,
> > > +                                   const libcamera::utils::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
Naushir Patuck June 8, 2021, 9:58 a.m. UTC | #5
On Tue, 8 Jun 2021 at 10:53, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> On Tue, Jun 08, 2021 at 10:47:47AM +0100, Naushir Patuck wrote:
> > On Tue, 8 Jun 2021 at 02:17, Laurent Pinchart wrote:
> > > On Tue, May 25, 2021 at 12:42:39PM +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           | 20 +++---
> > > >  src/ipa/raspberrypi/cam_helper.hpp           | 10 +--
> > > >  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-
> > > >  src/ipa/raspberrypi/raspberrypi.cpp          | 64
> +++++++++++---------
> > > >  4 files changed, 55 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > > > index 09917f3cc079..7b4331a87a42 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > @@ -18,6 +18,7 @@
> > > >
> > > >  using namespace RPiController;
> > > >  using namespace libcamera;
> > > > +using libcamera::utils::Duration;
> > >
> > > I would have used utils::Duration below, but I don't mind.
> > >
> > > >
> > > >  namespace libcamera {
> > > >  LOG_DECLARE_CATEGORY(IPARPI)
> > > > @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]]
> StatisticsPtr &stats,
> > > >  {
> > > >  }
> > > >
> > > > -uint32_t CamHelper::ExposureLines(double exposure_us) const
> > > > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const
> > >
> > > Given that the duration only stores a double value, you could pass it
> by
> > > value instead of by reference. The comment applies to the whole series.
> > > That would be my preference, but it's up to you.
> >
> > Agreed, I will change all functions to pass by value in the next
> revision.
> >
> > > Overall the code is really much nicer !
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Thanks.  Yes, this change does make time based variables so much easier
> > to work with!  At some point, we should change libcamera controls to use
> > this :-)
>
> That's on my todo list :-)
>
> By the way, would it be possible to add a test case for the Duration
> class in the next version ?
>

Sure, I will add some unit tests in the next revision.

Naush



>
> > > >  {
> > > >       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 +87,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 +184,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..07481e4174a7 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > > @@ -15,6 +15,7 @@
> > > >  #include "controller/metadata.hpp"
> > > >  #include "md_parser.hpp"
> > > >
> > > > +#include "libcamera/internal/utils.h"
> > > >  #include "libcamera/internal/v4l2_videodevice.h"
> > > >
> > > >  namespace RPiController {
> > > > @@ -72,10 +73,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 libcamera::utils::Duration
> &exposure) const;
> > > > +     libcamera::utils::Duration Exposure(uint32_t exposure_lines)
> const;
> > > > +     virtual uint32_t GetVBlanking(libcamera::utils::Duration
> &exposure,
> > > > +                                   const libcamera::utils::Duration
> &minFrameDuration,
> > > > +                                   const libcamera::utils::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
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index 09917f3cc079..7b4331a87a42 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -18,6 +18,7 @@ 
 
 using namespace RPiController;
 using namespace libcamera;
+using libcamera::utils::Duration;
 
 namespace libcamera {
 LOG_DECLARE_CATEGORY(IPARPI)
@@ -61,20 +62,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 +87,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 +184,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..07481e4174a7 100644
--- a/src/ipa/raspberrypi/cam_helper.hpp
+++ b/src/ipa/raspberrypi/cam_helper.hpp
@@ -15,6 +15,7 @@ 
 #include "controller/metadata.hpp"
 #include "md_parser.hpp"
 
+#include "libcamera/internal/utils.h"
 #include "libcamera/internal/v4l2_videodevice.h"
 
 namespace RPiController {
@@ -72,10 +73,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 libcamera::utils::Duration &exposure) const;
+	libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;
+	virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,
+				      const libcamera::utils::Duration &minFrameDuration,
+				      const libcamera::utils::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