[libcamera-devel,v3,4/4] ipa: raspberrypi: Switch the AGC/Lux code to use utils::Duration

12336 diff mbox series

Convert the core AGC and Lux controller code to use
utils::Duration for all exposure time related variables and
calculations.

Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
to use utils::Duration.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
 src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
 .../raspberrypi/controller/device_status.h    |  6 +-
 src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
 src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
 src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
 src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
 src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
 src/libcamera/utils.cpp                       |  9 +-
 9 files changed, 105 insertions(+), 77 deletions(-)

Hi Naush

Thanks for all this work!

On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Convert the core AGC and Lux controller code to use
> utils::Duration for all exposure time related variables and
> calculations.
>
> Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
> to use utils::Duration.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
>  src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
>  .../raspberrypi/controller/device_status.h    |  6 +-
>  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
>  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
>  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
>  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
>  src/libcamera/utils.cpp                       |  9 +-
>  9 files changed, 105 insertions(+), 77 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index feb02d1806b2..968fae13bd5e 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>                 return;
>         }
>
> -       deviceStatus.shutter_speed = Exposure(exposureLines).get<std::micro>();
> +       deviceStatus.shutter_speed = Exposure(exposureLines);
>         deviceStatus.analogue_gain = Gain(gainCode);
>
>         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> diff --git a/src/ipa/raspberrypi/controller/agc_status.h b/src/ipa/raspberrypi/controller/agc_status.h
> index 10381c90a313..c893715af588 100644
> --- a/src/ipa/raspberrypi/controller/agc_status.h
> +++ b/src/ipa/raspberrypi/controller/agc_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>
> +#include "libcamera/internal/utils.h"
> +
>  // The AGC algorithm should post the following structure into the image's
>  // "agc.status" metadata.
>
> @@ -17,18 +19,20 @@ extern "C" {
>  // seen statistics and calculated meaningful values. The contents should be
>  // ignored until then.
>
> +using libcamera::utils::Duration;
> +
>  struct AgcStatus {
> -       double total_exposure_value; // value for all exposure and gain for this image
> -       double target_exposure_value; // (unfiltered) target total exposure AGC is aiming for
> -       double shutter_time;
> +       Duration total_exposure_value; // value for all exposure and gain for this image
> +       Duration target_exposure_value; // (unfiltered) target total exposure AGC is aiming for
> +       Duration shutter_time;
>         double analogue_gain;
>         char exposure_mode[32];
>         char constraint_mode[32];
>         char metering_mode[32];
>         double ev;
> -       double flicker_period;
> +       Duration flicker_period;
>         int floating_region_enable;
> -       double fixed_shutter;
> +       Duration fixed_shutter;
>         double fixed_analogue_gain;
>         double digital_gain;
>         int locked;
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> index aa08608b5d40..131b4cd344ee 100644
> --- a/src/ipa/raspberrypi/controller/device_status.h
> +++ b/src/ipa/raspberrypi/controller/device_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>
> +#include "libcamera/internal/utils.h"
> +
>  // Definition of "device metadata" which stores things like shutter time and
>  // analogue gain that downstream control algorithms will want to know.
>
> @@ -14,8 +16,8 @@ extern "C" {
>  #endif
>
>  struct DeviceStatus {
> -       // time shutter is open, in microseconds
> -       double shutter_speed;
> +       // time shutter is open
> +       libcamera::utils::Duration shutter_speed;
>         double analogue_gain;
>         // 1.0/distance-in-metres, or 0 if unknown
>         double lens_position;
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 482eb3ef962d..46742d845034 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -21,6 +21,7 @@
>
>  using namespace RPiController;
>  using namespace libcamera;
> +using utils::operator<<;

Still can't say that I like this... just because someone wants to use
Durations, it seems like an awkward thing to make them remember. But
unless anyone has anything better....

>
>  LOG_DEFINE_CATEGORY(RPiAgc)
>
> @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string, AgcMeteringMode> &metering_modes,
>         return first;
>  }
>
> -static int read_double_list(std::vector<double> &list,
> -                           boost::property_tree::ptree const &params)
> +static int read_list(std::vector<double> &list,
> +                    boost::property_tree::ptree const &params)
>  {
>         for (auto &p : params)
>                 list.push_back(p.second.get_value<double>());
>         return list.size();
>  }
>
> +static int read_list(std::vector<Duration> &list,
> +                    boost::property_tree::ptree const &params)
> +{
> +       for (auto &p : params)
> +               list.push_back(p.second.get_value<double>() * 1us);
> +       return list.size();
> +}
> +
>  void AgcExposureMode::Read(boost::property_tree::ptree const &params)
>  {
>         int num_shutters =
> -               read_double_list(shutter, params.get_child("shutter"));
> -       int num_ags = read_double_list(gain, params.get_child("gain"));
> +               read_list(shutter, params.get_child("shutter"));
> +       int num_ags = read_list(gain, params.get_child("gain"));
>         if (num_shutters < 2 || num_ags < 2)
>                 throw std::runtime_error(
>                         "AgcConfig: must have at least two entries in exposure profile");
> @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
>                 params.get<double>("fast_reduce_threshold", 0.4);
>         base_ev = params.get<double>("base_ev", 1.0);
>         // Start with quite a low value as ramping up is easier than ramping down.
> -       default_exposure_time = params.get<double>("default_exposure_time", 1000);
> +       default_exposure_time = params.get<double>("default_exposure_time", 1000) * 1us;
>         default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
>  }
>
> @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)
>         : AgcAlgorithm(controller), metering_mode_(nullptr),
>           exposure_mode_(nullptr), constraint_mode_(nullptr),
>           frame_count_(0), lock_count_(0),
> -         last_target_exposure_(0.0),
> -         ev_(1.0), flicker_period_(0.0),
> -         max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> +         last_target_exposure_(0s),
> +         ev_(1.0), flicker_period_(0s),
> +         max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)
>  {
>         memset(&awb_, 0, sizeof(awb_));
>         // Setting status_.total_exposure_value_ to zero initially tells us
> @@ -203,7 +212,7 @@ void Agc::Pause()
>
>  void Agc::Resume()
>  {
> -       fixed_shutter_ = 0;
> +       fixed_shutter_ = 0s;
>         fixed_analogue_gain_ = 0;
>  }
>
> @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)
>
>  void Agc::SetFlickerPeriod(const Duration &flicker_period)
>  {
> -       flicker_period_ = flicker_period.get<std::micro>();
> +       flicker_period_ = flicker_period;
>  }
>
>  void Agc::SetMaxShutter(const Duration &max_shutter)
>  {
> -       max_shutter_ = max_shutter.get<std::micro>();
> +       max_shutter_ = max_shutter;
>  }
>
>  void Agc::SetFixedShutter(const Duration &fixed_shutter)
>  {
> -       fixed_shutter_ = fixed_shutter.get<std::micro>();
> +       fixed_shutter_ = fixed_shutter;
>         // Set this in case someone calls Pause() straight after.
>         status_.shutter_time = clipShutter(fixed_shutter_);
>  }
> @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
>  {
>         housekeepConfig();
>
> -       double fixed_shutter = clipShutter(fixed_shutter_);
> -       if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> +       Duration fixed_shutter = clipShutter(fixed_shutter_);
> +       if (fixed_shutter && fixed_analogue_gain_) {
>                 // We're going to reset the algorithm here with these fixed values.
>
>                 fetchAwbStatus(metadata);
> @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)
>                 // Process has run, so we have meaningful values.
>                 DeviceStatus device_status;
>                 if (image_metadata->Get("device.status", device_status) == 0) {
> -                       double actual_exposure = device_status.shutter_speed *
> -                                                device_status.analogue_gain;
> +                       Duration actual_exposure = device_status.shutter_speed *
> +                                                  device_status.analogue_gain;
>                         if (actual_exposure) {
>                                 status_.digital_gain =
>                                         status_.total_exposure_value /
> @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)
>                                         std::min(status_.digital_gain, 4.0));
>                                 LOG(RPiAgc, Debug) << "Actual exposure " << actual_exposure;
>                                 LOG(RPiAgc, Debug) << "Use digital_gain " << status_.digital_gain;
> -                               LOG(RPiAgc, Debug) << "Effective exposure " << actual_exposure * status_.digital_gain;
> +                               LOG(RPiAgc, Debug) << "Effective exposure "
> +                                                  << actual_exposure * status_.digital_gain;
>                                 // Decide whether AEC/AGC has converged.
>                                 updateLockStatus(device_status);
>                         }
> @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const &device_status)
>         const double RESET_MARGIN = 1.5;
>
>         // Add 200us to the exposure time error to allow for line quantisation.
> -       double exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200;
> +       Duration exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200us;
>         double gain_error = last_device_status_.analogue_gain * ERROR_FACTOR;
> -       double target_error = last_target_exposure_ * ERROR_FACTOR;
> +       Duration target_error = last_target_exposure_ * ERROR_FACTOR;
>
>         // Note that we don't know the exposure/gain limits of the sensor, so
>         // the values we keep requesting may be unachievable. For this reason
> @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata *image_metadata)
>         current_.analogue_gain = device_status->analogue_gain;
>         AgcStatus *agc_status =
>                 image_metadata->GetLocked<AgcStatus>("agc.status");
> -       current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0;
> +       current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0s;
>         current_.total_exposure_no_dg = current_.shutter * current_.analogue_gain;
>  }
>
> @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *image_metadata,
>
>  void Agc::computeTargetExposure(double gain)
>  {
> -       if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain != 0.0) {
> +       if (status_.fixed_shutter && status_.fixed_analogue_gain) {
>                 // When ag and shutter are both fixed, we need to drive the
>                 // total exposure so that we end up with a digital gain of at least
>                 // 1/min_colour_gain. Otherwise we'd desaturate channels causing
> @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)
>                 target_.total_exposure = current_.total_exposure_no_dg * gain;
>                 // The final target exposure is also limited to what the exposure
>                 // mode allows.
> -               double max_shutter = status_.fixed_shutter != 0.0
> +               Duration max_shutter = status_.fixed_shutter
>                                    ? status_.fixed_shutter
>                                    : exposure_mode_->shutter.back();
>                 max_shutter = clipShutter(max_shutter);
> -               double max_total_exposure =
> +               Duration max_total_exposure =
>                         max_shutter *
>                         (status_.fixed_analogue_gain != 0.0
>                                  ? status_.fixed_analogue_gain
> @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)
>         if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
>             frame_count_ <= config_.startup_frames)
>                 speed = 1.0;
> -       if (filtered_.total_exposure == 0.0) {
> +       if (!filtered_.total_exposure) {
>                 filtered_.total_exposure = target_.total_exposure;
>                 filtered_.total_exposure_no_dg = target_.total_exposure_no_dg;
>         } else {
> @@ -674,9 +684,10 @@ void Agc::divideUpExposure()
>         // Sending the fixed shutter/gain cases through the same code may seem
>         // unnecessary, but it will make more sense when extend this to cover
>         // variable aperture.
> -       double exposure_value = filtered_.total_exposure_no_dg;
> -       double shutter_time, analogue_gain;
> -       shutter_time = status_.fixed_shutter != 0.0
> +       Duration exposure_value = filtered_.total_exposure_no_dg;
> +       Duration shutter_time;
> +       double analogue_gain;
> +       shutter_time = status_.fixed_shutter
>                                ? status_.fixed_shutter
>                                : exposure_mode_->shutter[0];
>         shutter_time = clipShutter(shutter_time);
> @@ -686,8 +697,8 @@ void Agc::divideUpExposure()
>         if (shutter_time * analogue_gain < exposure_value) {
>                 for (unsigned int stage = 1;
>                      stage < exposure_mode_->gain.size(); stage++) {
> -                       if (status_.fixed_shutter == 0.0) {
> -                               double stage_shutter =
> +                       if (!status_.fixed_shutter) {
> +                               Duration stage_shutter =
>                                         clipShutter(exposure_mode_->shutter[stage]);
>                                 if (stage_shutter * analogue_gain >=
>                                     exposure_value) {
> @@ -713,12 +724,11 @@ void Agc::divideUpExposure()
>                            << analogue_gain;
>         // Finally adjust shutter time for flicker avoidance (require both
>         // shutter and gain not to be fixed).
> -       if (status_.fixed_shutter == 0.0 &&
> -           status_.fixed_analogue_gain == 0.0 &&
> -           status_.flicker_period != 0.0) {
> -               int flicker_periods = shutter_time / status_.flicker_period;
> -               if (flicker_periods > 0) {
> -                       double new_shutter_time = flicker_periods * status_.flicker_period;
> +       if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
> +           status_.flicker_period) {
> +               double flicker_periods = shutter_time / status_.flicker_period;
> +               if (flicker_periods > 0.0) {
> +                       Duration new_shutter_time = flicker_periods * status_.flicker_period;

Just wondering if this will behave the same now that flicker_periods
is a double. Won't it now hold fractional values with the result that
new_shutter_time isn't an exact multiple of the period?

>                         analogue_gain *= shutter_time / new_shutter_time;
>                         // We should still not allow the ag to go over the
>                         // largest value in the exposure mode. Note that this
> @@ -738,7 +748,7 @@ void Agc::divideUpExposure()
>  void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
>  {
>         status_.total_exposure_value = filtered_.total_exposure;
> -       status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg;
> +       status_.target_exposure_value = desaturate ? 0s : target_.total_exposure_no_dg;
>         status_.shutter_time = filtered_.shutter;
>         status_.analogue_gain = filtered_.analogue_gain;
>         // Write to metadata as well, in case anyone wants to update the camera
> @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
>                            << " analogue gain " << filtered_.analogue_gain;
>  }
>
> -double Agc::clipShutter(double shutter)
> +Duration Agc::clipShutter(const Duration &shutter)
>  {
> +       Duration clip_shutter = shutter;

Given that we copy it anyway I wonder if we might have left this as
call-by-value, resulting in fewer changes. But it's only the teeniest
of nit-picks!

If you can put my mind at rest over the flicker period thing:

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

Thanks!
David

>         if (max_shutter_)
> -               shutter = std::min(shutter, max_shutter_);
> -       return shutter;
> +               clip_shutter = std::min(clip_shutter, max_shutter_);
> +       return clip_shutter;
>  }
>
>  // Register algorithm with the system.
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 16a65959d90e..24a6610096c2 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -9,6 +9,8 @@
>  #include <vector>
>  #include <mutex>
>
> +#include "libcamera/internal/utils.h"
> +
>  #include "../agc_algorithm.hpp"
>  #include "../agc_status.h"
>  #include "../pwl.hpp"
> @@ -22,13 +24,15 @@
>
>  namespace RPiController {
>
> +using namespace std::literals::chrono_literals;
> +
>  struct AgcMeteringMode {
>         double weights[AGC_STATS_SIZE];
>         void Read(boost::property_tree::ptree const &params);
>  };
>
>  struct AgcExposureMode {
> -       std::vector<double> shutter;
> +       std::vector<Duration> shutter;
>         std::vector<double> gain;
>         void Read(boost::property_tree::ptree const &params);
>  };
> @@ -61,7 +65,7 @@ struct AgcConfig {
>         std::string default_exposure_mode;
>         std::string default_constraint_mode;
>         double base_ev;
> -       double default_exposure_time;
> +       Duration default_exposure_time;
>         double default_analogue_gain;
>  };
>
> @@ -101,19 +105,19 @@ private:
>         void filterExposure(bool desaturate);
>         void divideUpExposure();
>         void writeAndFinish(Metadata *image_metadata, bool desaturate);
> -       double clipShutter(double shutter);
> +       Duration clipShutter(const Duration &shutter);
>         AgcMeteringMode *metering_mode_;
>         AgcExposureMode *exposure_mode_;
>         AgcConstraintMode *constraint_mode_;
>         uint64_t frame_count_;
>         AwbStatus awb_;
>         struct ExposureValues {
> -               ExposureValues() : shutter(0), analogue_gain(0),
> -                                  total_exposure(0), total_exposure_no_dg(0) {}
> -               double shutter;
> +               ExposureValues() : shutter(0s), analogue_gain(0),
> +                                  total_exposure(0s), total_exposure_no_dg(0s) {}
> +               Duration shutter;
>                 double analogue_gain;
> -               double total_exposure;
> -               double total_exposure_no_dg; // without digital gain
> +               Duration total_exposure;
> +               Duration total_exposure_no_dg; // without digital gain
>         };
>         ExposureValues current_;  // values for the current frame
>         ExposureValues target_;   // calculate the values we want here
> @@ -121,15 +125,15 @@ private:
>         AgcStatus status_;
>         int lock_count_;
>         DeviceStatus last_device_status_;
> -       double last_target_exposure_;
> +       Duration last_target_exposure_;
>         // Below here the "settings" that applications can change.
>         std::string metering_mode_name_;
>         std::string exposure_mode_name_;
>         std::string constraint_mode_name_;
>         double ev_;
> -       double flicker_period_;
> -       double max_shutter_;
> -       double fixed_shutter_;
> +       Duration flicker_period_;
> +       Duration max_shutter_;
> +       Duration fixed_shutter_;
>         double fixed_analogue_gain_;
>  };
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> index f74381cab2b4..0fa6457c1376 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -16,6 +16,7 @@
>
>  using namespace RPiController;
>  using namespace libcamera;
> +using namespace std::literals::chrono_literals;
>
>  LOG_DEFINE_CATEGORY(RPiLux)
>
> @@ -38,7 +39,7 @@ char const *Lux::Name() const
>  void Lux::Read(boost::property_tree::ptree const &params)
>  {
>         reference_shutter_speed_ =
> -               params.get<double>("reference_shutter_speed");
> +               params.get<double>("reference_shutter_speed") * 1.0us;
>         reference_gain_ = params.get<double>("reference_gain");
>         reference_aperture_ = params.get<double>("reference_aperture", 1.0);
>         reference_Y_ = params.get<double>("reference_Y");
> @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
>  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
>  {
>         // set some initial values to shut the compiler up
> -       DeviceStatus device_status =
> -               { .shutter_speed = 1.0,
> -                 .analogue_gain = 1.0,
> -                 .lens_position = 0.0,
> -                 .aperture = 0.0,
> -                 .flash_intensity = 0.0 };
> +       DeviceStatus device_status = { .shutter_speed = 1.0ms,
> +                                      .analogue_gain = 1.0,
> +                                      .lens_position = 0.0,
> +                                      .aperture = 0.0,
> +                                      .flash_intensity = 0.0 };
>         if (image_metadata->Get("device.status", device_status) == 0) {
>                 double current_gain = device_status.analogue_gain;
> -               double current_shutter_speed = device_status.shutter_speed;
>                 double current_aperture = device_status.aperture;
>                 if (current_aperture == 0)
>                         current_aperture = current_aperture_;
> @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
>                 double current_Y = sum / (double)num + .5;
>                 double gain_ratio = reference_gain_ / current_gain;
>                 double shutter_speed_ratio =
> -                       reference_shutter_speed_ / current_shutter_speed;
> +                       reference_shutter_speed_ / device_status.shutter_speed;
>                 double aperture_ratio = reference_aperture_ / current_aperture;
>                 double Y_ratio = current_Y * (65536 / num_bins) / reference_Y_;
>                 double estimated_lux = shutter_speed_ratio * gain_ratio *
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> index f9090484a136..45c844393e62 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> @@ -8,6 +8,8 @@
>
>  #include <mutex>
>
> +#include "libcamera/internal/utils.h"
> +
>  #include "../lux_status.h"
>  #include "../algorithm.hpp"
>
> @@ -28,7 +30,7 @@ public:
>  private:
>         // These values define the conditions of the reference image, against
>         // which we compare the new image.
> -       double reference_shutter_speed_; // in micro-seconds
> +       libcamera::utils::Duration reference_shutter_speed_;
>         double reference_gain_;
>         double reference_aperture_; // units of 1/f
>         double reference_Y_; // out of 65536
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index f8f36420b076..b77230ded591 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
>
>         /* SwitchMode may supply updated exposure/gain values to use. */
>         AgcStatus agcStatus;
> -       agcStatus.shutter_time = 0.0;
> +       agcStatus.shutter_time = 0.0s;
>         agcStatus.analogue_gain = 0.0;
>
>         metadata.Get("agc.status", agcStatus);
> -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> +       if (agcStatus.shutter_time && agcStatus.analogue_gain) {
>                 ControlList ctrls(sensorCtrls_);
>                 applyAGC(&agcStatus, ctrls);
>                 startConfig->controls = std::move(ctrls);
> @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,
>                 /* Supply initial values for gain and exposure. */
>                 ControlList ctrls(sensorCtrls_);
>                 AgcStatus agcStatus;
> -               agcStatus.shutter_time = DefaultExposureTime.get<std::micro>();
> +               agcStatus.shutter_time = DefaultExposureTime;
>                 agcStatus.analogue_gain = DefaultAnalogueGain;
>                 applyAGC(&agcStatus, ctrls);
>
> @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()
>          */
>         DeviceStatus *deviceStatus = rpiMetadata_.GetLocked<DeviceStatus>("device.status");
>         if (deviceStatus) {
> -               libcameraMetadata_.set(controls::ExposureTime, deviceStatus->shutter_speed);
> +               libcameraMetadata_.set(controls::ExposureTime,
> +                                      deviceStatus->shutter_speed.get<std::micro>());
>                 libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain);
>         }
>
> @@ -1017,7 +1018,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).get<std::micro>();
> +       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>         deviceStatus.analogue_gain = helper_->Gain(gainCode);
>
>         LOG(IPARPI, Debug) << "Metadata - Exposure : "
> @@ -1103,7 +1104,7 @@ 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. */
> -       Duration exposure = agcStatus->shutter_time * 1.0us;
> +       Duration exposure = agcStatus->shutter_time;
>         int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);
>         int32_t exposureLines = helper_->ExposureLines(exposure);
>
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 3131aa2d6666..5fa6f8366f3d 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -506,9 +506,14 @@ std::string libcameraSourcePath()
>   * loop, iterates over an indexed view of the \a iterable
>   */
>
> +/**
> + * \typedef BaseDuration
> + * \brief Base struct for the \a Duration helper class.
> + */
> +
>  /**
>   * \class Duration
> - * \brief Helper for std::chrono::duration types. Derived from \a BaseDuration
> + * \brief Helper class for std::chrono::duration types.
>   */
>
>  /**
> @@ -517,7 +522,7 @@ std::string libcameraSourcePath()
>   * \param[in] d The std::chrono::duration object to convert from.
>   *
>   * Constructs a \a Duration object from a \a std::chrono::duration object,
> - * converting the representation to \a BaseDuration type.
> + * internally converting the representation to \a BaseDuration type.
>   */
>
>  /**
> --
> 2.25.1
>

On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com> wrote:

> Convert the core AGC and Lux controller code to use
> utils::Duration for all exposure time related variables and
> calculations.
>
> Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
> to use utils::Duration.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
>  src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
>  .../raspberrypi/controller/device_status.h    |  6 +-
>  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
>  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
>  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
>  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
>  src/libcamera/utils.cpp                       |  9 +-
>  9 files changed, 105 insertions(+), 77 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index feb02d1806b2..968fae13bd5e 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t>
> buffer,
>                 return;
>         }
>
> -       deviceStatus.shutter_speed =
> Exposure(exposureLines).get<std::micro>();
> +       deviceStatus.shutter_speed = Exposure(exposureLines);
>         deviceStatus.analogue_gain = Gain(gainCode);
>
>         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> diff --git a/src/ipa/raspberrypi/controller/agc_status.h
> b/src/ipa/raspberrypi/controller/agc_status.h
> index 10381c90a313..c893715af588 100644
> --- a/src/ipa/raspberrypi/controller/agc_status.h
> +++ b/src/ipa/raspberrypi/controller/agc_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>
> +#include "libcamera/internal/utils.h"
> +
>  // The AGC algorithm should post the following structure into the image's
>  // "agc.status" metadata.
>
> @@ -17,18 +19,20 @@ extern "C" {
>  // seen statistics and calculated meaningful values. The contents should
> be
>  // ignored until then.
>
> +using libcamera::utils::Duration;
> +
>  struct AgcStatus {
> -       double total_exposure_value; // value for all exposure and gain
> for this image
> -       double target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
> -       double shutter_time;
> +       Duration total_exposure_value; // value for all exposure and gain
> for this image
> +       Duration target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
> +       Duration shutter_time;
>         double analogue_gain;
>         char exposure_mode[32];
>         char constraint_mode[32];
>         char metering_mode[32];
>         double ev;
> -       double flicker_period;
> +       Duration flicker_period;
>         int floating_region_enable;
> -       double fixed_shutter;
> +       Duration fixed_shutter;
>         double fixed_analogue_gain;
>         double digital_gain;
>         int locked;
> diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> index aa08608b5d40..131b4cd344ee 100644
> --- a/src/ipa/raspberrypi/controller/device_status.h
> +++ b/src/ipa/raspberrypi/controller/device_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>
> +#include "libcamera/internal/utils.h"
> +
>  // Definition of "device metadata" which stores things like shutter time
> and
>  // analogue gain that downstream control algorithms will want to know.
>
> @@ -14,8 +16,8 @@ extern "C" {
>  #endif
>
>  struct DeviceStatus {
> -       // time shutter is open, in microseconds
> -       double shutter_speed;
> +       // time shutter is open
> +       libcamera::utils::Duration shutter_speed;
>         double analogue_gain;
>         // 1.0/distance-in-metres, or 0 if unknown
>         double lens_position;
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 482eb3ef962d..46742d845034 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -21,6 +21,7 @@
>
>  using namespace RPiController;
>  using namespace libcamera;
> +using utils::operator<<;
>
>  LOG_DEFINE_CATEGORY(RPiAgc)
>
> @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string,
> AgcMeteringMode> &metering_modes,
>         return first;
>  }
>
> -static int read_double_list(std::vector<double> &list,
> -                           boost::property_tree::ptree const &params)
> +static int read_list(std::vector<double> &list,
> +                    boost::property_tree::ptree const &params)
>  {
>         for (auto &p : params)
>                 list.push_back(p.second.get_value<double>());
>         return list.size();
>  }
>
> +static int read_list(std::vector<Duration> &list,
> +                    boost::property_tree::ptree const &params)
> +{
> +       for (auto &p : params)
> +               list.push_back(p.second.get_value<double>() * 1us);
> +       return list.size();
> +}
> +
>  void AgcExposureMode::Read(boost::property_tree::ptree const &params)
>  {
>         int num_shutters =
> -               read_double_list(shutter, params.get_child("shutter"));
> -       int num_ags = read_double_list(gain, params.get_child("gain"));
> +               read_list(shutter, params.get_child("shutter"));
> +       int num_ags = read_list(gain, params.get_child("gain"));
>         if (num_shutters < 2 || num_ags < 2)
>                 throw std::runtime_error(
>                         "AgcConfig: must have at least two entries in
> exposure profile");
> @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const
> &params)
>                 params.get<double>("fast_reduce_threshold", 0.4);
>         base_ev = params.get<double>("base_ev", 1.0);
>         // Start with quite a low value as ramping up is easier than
> ramping down.
> -       default_exposure_time =
> params.get<double>("default_exposure_time", 1000);
> +       default_exposure_time =
> params.get<double>("default_exposure_time", 1000) * 1us;
>         default_analogue_gain =
> params.get<double>("default_analogue_gain", 1.0);
>  }
>
> @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)
>         : AgcAlgorithm(controller), metering_mode_(nullptr),
>           exposure_mode_(nullptr), constraint_mode_(nullptr),
>           frame_count_(0), lock_count_(0),
> -         last_target_exposure_(0.0),
> -         ev_(1.0), flicker_period_(0.0),
> -         max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> +         last_target_exposure_(0s),
> +         ev_(1.0), flicker_period_(0s),
> +         max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)
>  {
>         memset(&awb_, 0, sizeof(awb_));
>         // Setting status_.total_exposure_value_ to zero initially tells us
> @@ -203,7 +212,7 @@ void Agc::Pause()
>
>  void Agc::Resume()
>  {
> -       fixed_shutter_ = 0;
> +       fixed_shutter_ = 0s;
>         fixed_analogue_gain_ = 0;
>  }
>
> @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)
>
>  void Agc::SetFlickerPeriod(const Duration &flicker_period)
>  {
> -       flicker_period_ = flicker_period.get<std::micro>();
> +       flicker_period_ = flicker_period;
>  }
>
>  void Agc::SetMaxShutter(const Duration &max_shutter)
>  {
> -       max_shutter_ = max_shutter.get<std::micro>();
> +       max_shutter_ = max_shutter;
>  }
>
>  void Agc::SetFixedShutter(const Duration &fixed_shutter)
>  {
> -       fixed_shutter_ = fixed_shutter.get<std::micro>();
> +       fixed_shutter_ = fixed_shutter;
>         // Set this in case someone calls Pause() straight after.
>         status_.shutter_time = clipShutter(fixed_shutter_);
>  }
> @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const
> &camera_mode,
>  {
>         housekeepConfig();
>
> -       double fixed_shutter = clipShutter(fixed_shutter_);
> -       if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> +       Duration fixed_shutter = clipShutter(fixed_shutter_);
> +       if (fixed_shutter && fixed_analogue_gain_) {
>                 // We're going to reset the algorithm here with these
> fixed values.
>
>                 fetchAwbStatus(metadata);
> @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)
>                 // Process has run, so we have meaningful values.
>                 DeviceStatus device_status;
>                 if (image_metadata->Get("device.status", device_status) ==
> 0) {
> -                       double actual_exposure =
> device_status.shutter_speed *
> -
> device_status.analogue_gain;
> +                       Duration actual_exposure =
> device_status.shutter_speed *
> +
> device_status.analogue_gain;
>                         if (actual_exposure) {
>                                 status_.digital_gain =
>                                         status_.total_exposure_value /
> @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)
>                                         std::min(status_.digital_gain,
> 4.0));
>                                 LOG(RPiAgc, Debug) << "Actual exposure "
> << actual_exposure;
>                                 LOG(RPiAgc, Debug) << "Use digital_gain "
> << status_.digital_gain;
> -                               LOG(RPiAgc, Debug) << "Effective exposure
> " << actual_exposure * status_.digital_gain;
> +                               LOG(RPiAgc, Debug) << "Effective exposure "
> +                                                  << actual_exposure *
> status_.digital_gain;
>                                 // Decide whether AEC/AGC has converged.
>                                 updateLockStatus(device_status);
>                         }
> @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const
> &device_status)
>         const double RESET_MARGIN = 1.5;
>
>         // Add 200us to the exposure time error to allow for line
> quantisation.
> -       double exposure_error = last_device_status_.shutter_speed *
> ERROR_FACTOR + 200;
> +       Duration exposure_error = last_device_status_.shutter_speed *
> ERROR_FACTOR + 200us;
>         double gain_error = last_device_status_.analogue_gain *
> ERROR_FACTOR;
> -       double target_error = last_target_exposure_ * ERROR_FACTOR;
> +       Duration target_error = last_target_exposure_ * ERROR_FACTOR;
>
>         // Note that we don't know the exposure/gain limits of the sensor,
> so
>         // the values we keep requesting may be unachievable. For this
> reason
> @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata
> *image_metadata)
>         current_.analogue_gain = device_status->analogue_gain;
>         AgcStatus *agc_status =
>                 image_metadata->GetLocked<AgcStatus>("agc.status");
> -       current_.total_exposure = agc_status ?
> agc_status->total_exposure_value : 0;
> +       current_.total_exposure = agc_status ?
> agc_status->total_exposure_value : 0s;
>         current_.total_exposure_no_dg = current_.shutter *
> current_.analogue_gain;
>  }
>
> @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics,
> Metadata *image_metadata,
>
>  void Agc::computeTargetExposure(double gain)
>  {
> -       if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain !=
> 0.0) {
> +       if (status_.fixed_shutter && status_.fixed_analogue_gain) {
>                 // When ag and shutter are both fixed, we need to drive the
>                 // total exposure so that we end up with a digital gain of
> at least
>                 // 1/min_colour_gain. Otherwise we'd desaturate channels
> causing
> @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)
>                 target_.total_exposure = current_.total_exposure_no_dg *
> gain;
>                 // The final target exposure is also limited to what the
> exposure
>                 // mode allows.
> -               double max_shutter = status_.fixed_shutter != 0.0
> +               Duration max_shutter = status_.fixed_shutter
>                                    ? status_.fixed_shutter
>                                    : exposure_mode_->shutter.back();
>                 max_shutter = clipShutter(max_shutter);
> -               double max_total_exposure =
> +               Duration max_total_exposure =
>                         max_shutter *
>                         (status_.fixed_analogue_gain != 0.0
>                                  ? status_.fixed_analogue_gain
> @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)
>         if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
>             frame_count_ <= config_.startup_frames)
>                 speed = 1.0;
> -       if (filtered_.total_exposure == 0.0) {
> +       if (!filtered_.total_exposure) {
>                 filtered_.total_exposure = target_.total_exposure;
>                 filtered_.total_exposure_no_dg =
> target_.total_exposure_no_dg;
>         } else {
> @@ -674,9 +684,10 @@ void Agc::divideUpExposure()
>         // Sending the fixed shutter/gain cases through the same code may
> seem
>         // unnecessary, but it will make more sense when extend this to
> cover
>         // variable aperture.
> -       double exposure_value = filtered_.total_exposure_no_dg;
> -       double shutter_time, analogue_gain;
> -       shutter_time = status_.fixed_shutter != 0.0
> +       Duration exposure_value = filtered_.total_exposure_no_dg;
> +       Duration shutter_time;
> +       double analogue_gain;
> +       shutter_time = status_.fixed_shutter
>                                ? status_.fixed_shutter
>                                : exposure_mode_->shutter[0];
>         shutter_time = clipShutter(shutter_time);
> @@ -686,8 +697,8 @@ void Agc::divideUpExposure()
>         if (shutter_time * analogue_gain < exposure_value) {
>                 for (unsigned int stage = 1;
>                      stage < exposure_mode_->gain.size(); stage++) {
> -                       if (status_.fixed_shutter == 0.0) {
> -                               double stage_shutter =
> +                       if (!status_.fixed_shutter) {
> +                               Duration stage_shutter =
>
> clipShutter(exposure_mode_->shutter[stage]);
>                                 if (stage_shutter * analogue_gain >=
>                                     exposure_value) {
> @@ -713,12 +724,11 @@ void Agc::divideUpExposure()
>                            << analogue_gain;
>         // Finally adjust shutter time for flicker avoidance (require both
>         // shutter and gain not to be fixed).
> -       if (status_.fixed_shutter == 0.0 &&
> -           status_.fixed_analogue_gain == 0.0 &&
> -           status_.flicker_period != 0.0) {
> -               int flicker_periods = shutter_time /
> status_.flicker_period;
> -               if (flicker_periods > 0) {
> -                       double new_shutter_time = flicker_periods *
> status_.flicker_period;
> +       if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
> +           status_.flicker_period) {
> +               double flicker_periods = shutter_time /
> status_.flicker_period;
> +               if (flicker_periods > 0.0) {
> +                       Duration new_shutter_time = flicker_periods *
> status_.flicker_period;
>                         analogue_gain *= shutter_time / new_shutter_time;
>                         // We should still not allow the ag to go over the
>                         // largest value in the exposure mode. Note that
> this
> @@ -738,7 +748,7 @@ void Agc::divideUpExposure()
>  void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
>  {
>         status_.total_exposure_value = filtered_.total_exposure;
> -       status_.target_exposure_value = desaturate ? 0 :
> target_.total_exposure_no_dg;
> +       status_.target_exposure_value = desaturate ? 0s :
> target_.total_exposure_no_dg;
>         status_.shutter_time = filtered_.shutter;
>         status_.analogue_gain = filtered_.analogue_gain;
>         // Write to metadata as well, in case anyone wants to update the
> camera
> @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata,
> bool desaturate)
>                            << " analogue gain " << filtered_.analogue_gain;
>  }
>
> -double Agc::clipShutter(double shutter)
> +Duration Agc::clipShutter(const Duration &shutter)
>  {
> +       Duration clip_shutter = shutter;
>         if (max_shutter_)
> -               shutter = std::min(shutter, max_shutter_);
> -       return shutter;
> +               clip_shutter = std::min(clip_shutter, max_shutter_);
> +       return clip_shutter;
>  }
>
>  // Register algorithm with the system.
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 16a65959d90e..24a6610096c2 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -9,6 +9,8 @@
>  #include <vector>
>  #include <mutex>
>
> +#include "libcamera/internal/utils.h"
> +
>  #include "../agc_algorithm.hpp"
>  #include "../agc_status.h"
>  #include "../pwl.hpp"
> @@ -22,13 +24,15 @@
>
>  namespace RPiController {
>
> +using namespace std::literals::chrono_literals;
> +
>  struct AgcMeteringMode {
>         double weights[AGC_STATS_SIZE];
>         void Read(boost::property_tree::ptree const &params);
>  };
>
>  struct AgcExposureMode {
> -       std::vector<double> shutter;
> +       std::vector<Duration> shutter;
>         std::vector<double> gain;
>         void Read(boost::property_tree::ptree const &params);
>  };
> @@ -61,7 +65,7 @@ struct AgcConfig {
>         std::string default_exposure_mode;
>         std::string default_constraint_mode;
>         double base_ev;
> -       double default_exposure_time;
> +       Duration default_exposure_time;
>         double default_analogue_gain;
>  };
>
> @@ -101,19 +105,19 @@ private:
>         void filterExposure(bool desaturate);
>         void divideUpExposure();
>         void writeAndFinish(Metadata *image_metadata, bool desaturate);
> -       double clipShutter(double shutter);
> +       Duration clipShutter(const Duration &shutter);
>         AgcMeteringMode *metering_mode_;
>         AgcExposureMode *exposure_mode_;
>         AgcConstraintMode *constraint_mode_;
>         uint64_t frame_count_;
>         AwbStatus awb_;
>         struct ExposureValues {
> -               ExposureValues() : shutter(0), analogue_gain(0),
> -                                  total_exposure(0),
> total_exposure_no_dg(0) {}
> -               double shutter;
> +               ExposureValues() : shutter(0s), analogue_gain(0),
> +                                  total_exposure(0s),
> total_exposure_no_dg(0s) {}
> +               Duration shutter;
>                 double analogue_gain;
> -               double total_exposure;
> -               double total_exposure_no_dg; // without digital gain
> +               Duration total_exposure;
> +               Duration total_exposure_no_dg; // without digital gain
>         };
>         ExposureValues current_;  // values for the current frame
>         ExposureValues target_;   // calculate the values we want here
> @@ -121,15 +125,15 @@ private:
>         AgcStatus status_;
>         int lock_count_;
>         DeviceStatus last_device_status_;
> -       double last_target_exposure_;
> +       Duration last_target_exposure_;
>         // Below here the "settings" that applications can change.
>         std::string metering_mode_name_;
>         std::string exposure_mode_name_;
>         std::string constraint_mode_name_;
>         double ev_;
> -       double flicker_period_;
> -       double max_shutter_;
> -       double fixed_shutter_;
> +       Duration flicker_period_;
> +       Duration max_shutter_;
> +       Duration fixed_shutter_;
>         double fixed_analogue_gain_;
>  };
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> index f74381cab2b4..0fa6457c1376 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -16,6 +16,7 @@
>
>  using namespace RPiController;
>  using namespace libcamera;
> +using namespace std::literals::chrono_literals;
>
>  LOG_DEFINE_CATEGORY(RPiLux)
>
> @@ -38,7 +39,7 @@ char const *Lux::Name() const
>  void Lux::Read(boost::property_tree::ptree const &params)
>  {
>         reference_shutter_speed_ =
> -               params.get<double>("reference_shutter_speed");
> +               params.get<double>("reference_shutter_speed") * 1.0us;
>         reference_gain_ = params.get<double>("reference_gain");
>         reference_aperture_ = params.get<double>("reference_aperture",
> 1.0);
>         reference_Y_ = params.get<double>("reference_Y");
> @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
>  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
>  {
>         // set some initial values to shut the compiler up
> -       DeviceStatus device_status =
> -               { .shutter_speed = 1.0,
> -                 .analogue_gain = 1.0,
> -                 .lens_position = 0.0,
> -                 .aperture = 0.0,
> -                 .flash_intensity = 0.0 };
> +       DeviceStatus device_status = { .shutter_speed = 1.0ms,
> +                                      .analogue_gain = 1.0,
> +                                      .lens_position = 0.0,
> +                                      .aperture = 0.0,
> +                                      .flash_intensity = 0.0 };
>         if (image_metadata->Get("device.status", device_status) == 0) {
>                 double current_gain = device_status.analogue_gain;
> -               double current_shutter_speed = device_status.shutter_speed;
>                 double current_aperture = device_status.aperture;
>                 if (current_aperture == 0)
>                         current_aperture = current_aperture_;
> @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata
> *image_metadata)
>                 double current_Y = sum / (double)num + .5;
>                 double gain_ratio = reference_gain_ / current_gain;
>                 double shutter_speed_ratio =
> -                       reference_shutter_speed_ / current_shutter_speed;
> +                       reference_shutter_speed_ /
> device_status.shutter_speed;
>                 double aperture_ratio = reference_aperture_ /
> current_aperture;
>                 double Y_ratio = current_Y * (65536 / num_bins) /
> reference_Y_;
>                 double estimated_lux = shutter_speed_ratio * gain_ratio *
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> index f9090484a136..45c844393e62 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> @@ -8,6 +8,8 @@
>
>  #include <mutex>
>
> +#include "libcamera/internal/utils.h"
> +
>  #include "../lux_status.h"
>  #include "../algorithm.hpp"
>
> @@ -28,7 +30,7 @@ public:
>  private:
>         // These values define the conditions of the reference image,
> against
>         // which we compare the new image.
> -       double reference_shutter_speed_; // in micro-seconds
> +       libcamera::utils::Duration reference_shutter_speed_;
>         double reference_gain_;
>         double reference_aperture_; // units of 1/f
>         double reference_Y_; // out of 65536
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index f8f36420b076..b77230ded591 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,
> ipa::RPi::StartConfig *startConf
>
>         /* SwitchMode may supply updated exposure/gain values to use. */
>         AgcStatus agcStatus;
> -       agcStatus.shutter_time = 0.0;
> +       agcStatus.shutter_time = 0.0s;
>         agcStatus.analogue_gain = 0.0;
>
>         metadata.Get("agc.status", agcStatus);
> -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=
> 0.0) {
> +       if (agcStatus.shutter_time && agcStatus.analogue_gain) {
>                 ControlList ctrls(sensorCtrls_);
>                 applyAGC(&agcStatus, ctrls);
>                 startConfig->controls = std::move(ctrls);
> @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>                 /* Supply initial values for gain and exposure. */
>                 ControlList ctrls(sensorCtrls_);
>                 AgcStatus agcStatus;
> -               agcStatus.shutter_time =
> DefaultExposureTime.get<std::micro>();
> +               agcStatus.shutter_time = DefaultExposureTime;
>                 agcStatus.analogue_gain = DefaultAnalogueGain;
>                 applyAGC(&agcStatus, ctrls);
>
> @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()
>          */
>         DeviceStatus *deviceStatus =
> rpiMetadata_.GetLocked<DeviceStatus>("device.status");
>         if (deviceStatus) {
> -               libcameraMetadata_.set(controls::ExposureTime,
> deviceStatus->shutter_speed);
> +               libcameraMetadata_.set(controls::ExposureTime,
> +
> deviceStatus->shutter_speed.get<std::micro>());
>                 libcameraMetadata_.set(controls::AnalogueGain,
> deviceStatus->analogue_gain);
>         }
>
> @@ -1017,7 +1018,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).get<std::micro>();
> +       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>         deviceStatus.analogue_gain = helper_->Gain(gainCode);
>
>         LOG(IPARPI, Debug) << "Metadata - Exposure : "
> @@ -1103,7 +1104,7 @@ 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. */
> -       Duration exposure = agcStatus->shutter_time * 1.0us;
> +       Duration exposure = agcStatus->shutter_time;
>         int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
>         int32_t exposureLines = helper_->ExposureLines(exposure);
>
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 3131aa2d6666..5fa6f8366f3d 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -506,9 +506,14 @@ std::string libcameraSourcePath()
>   * loop, iterates over an indexed view of the \a iterable
>   */
>
> +/**
> + * \typedef BaseDuration
> + * \brief Base struct for the \a Duration helper class.
> + */
> +
>  /**
>   * \class Duration
> - * \brief Helper for std::chrono::duration types. Derived from \a
> BaseDuration
> + * \brief Helper class for std::chrono::duration types.
>   */
>
>  /**
> @@ -517,7 +522,7 @@ std::string libcameraSourcePath()
>   * \param[in] d The std::chrono::duration object to convert from.
>   *
>   * Constructs a \a Duration object from a \a std::chrono::duration object,
> - * converting the representation to \a BaseDuration type.
> + * internally converting the representation to \a BaseDuration type.
>   */
>

Hmmm, this does not belong in patch 4/4.  It should be in 1/4 instead.
Will fix in the next revision.  Sorry about that!


>
>  /**
> --
> 2.25.1
>
>

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

> Hi Naush
>
> Thanks for all this work!
>
> On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com>
> wrote:
> >
> > Convert the core AGC and Lux controller code to use
> > utils::Duration for all exposure time related variables and
> > calculations.
> >
> > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
> > to use utils::Duration.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
> >  src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
> >  .../raspberrypi/controller/device_status.h    |  6 +-
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
> >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
> >  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
> >  src/libcamera/utils.cpp                       |  9 +-
> >  9 files changed, 105 insertions(+), 77 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index feb02d1806b2..968fae13bd5e 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> >                 return;
> >         }
> >
> > -       deviceStatus.shutter_speed =
> Exposure(exposureLines).get<std::micro>();
> > +       deviceStatus.shutter_speed = Exposure(exposureLines);
> >         deviceStatus.analogue_gain = Gain(gainCode);
> >
> >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > diff --git a/src/ipa/raspberrypi/controller/agc_status.h
> b/src/ipa/raspberrypi/controller/agc_status.h
> > index 10381c90a313..c893715af588 100644
> > --- a/src/ipa/raspberrypi/controller/agc_status.h
> > +++ b/src/ipa/raspberrypi/controller/agc_status.h
> > @@ -6,6 +6,8 @@
> >   */
> >  #pragma once
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  // The AGC algorithm should post the following structure into the
> image's
> >  // "agc.status" metadata.
> >
> > @@ -17,18 +19,20 @@ extern "C" {
> >  // seen statistics and calculated meaningful values. The contents
> should be
> >  // ignored until then.
> >
> > +using libcamera::utils::Duration;
> > +
> >  struct AgcStatus {
> > -       double total_exposure_value; // value for all exposure and gain
> for this image
> > -       double target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
> > -       double shutter_time;
> > +       Duration total_exposure_value; // value for all exposure and
> gain for this image
> > +       Duration target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
> > +       Duration shutter_time;
> >         double analogue_gain;
> >         char exposure_mode[32];
> >         char constraint_mode[32];
> >         char metering_mode[32];
> >         double ev;
> > -       double flicker_period;
> > +       Duration flicker_period;
> >         int floating_region_enable;
> > -       double fixed_shutter;
> > +       Duration fixed_shutter;
> >         double fixed_analogue_gain;
> >         double digital_gain;
> >         int locked;
> > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> > index aa08608b5d40..131b4cd344ee 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.h
> > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > @@ -6,6 +6,8 @@
> >   */
> >  #pragma once
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  // Definition of "device metadata" which stores things like shutter
> time and
> >  // analogue gain that downstream control algorithms will want to know.
> >
> > @@ -14,8 +16,8 @@ extern "C" {
> >  #endif
> >
> >  struct DeviceStatus {
> > -       // time shutter is open, in microseconds
> > -       double shutter_speed;
> > +       // time shutter is open
> > +       libcamera::utils::Duration shutter_speed;
> >         double analogue_gain;
> >         // 1.0/distance-in-metres, or 0 if unknown
> >         double lens_position;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 482eb3ef962d..46742d845034 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -21,6 +21,7 @@
> >
> >  using namespace RPiController;
> >  using namespace libcamera;
> > +using utils::operator<<;
>
> Still can't say that I like this... just because someone wants to use
> Durations, it seems like an awkward thing to make them remember. But
> unless anyone has anything better....
>
> >
> >  LOG_DEFINE_CATEGORY(RPiAgc)
> >
> > @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string,
> AgcMeteringMode> &metering_modes,
> >         return first;
> >  }
> >
> > -static int read_double_list(std::vector<double> &list,
> > -                           boost::property_tree::ptree const &params)
> > +static int read_list(std::vector<double> &list,
> > +                    boost::property_tree::ptree const &params)
> >  {
> >         for (auto &p : params)
> >                 list.push_back(p.second.get_value<double>());
> >         return list.size();
> >  }
> >
> > +static int read_list(std::vector<Duration> &list,
> > +                    boost::property_tree::ptree const &params)
> > +{
> > +       for (auto &p : params)
> > +               list.push_back(p.second.get_value<double>() * 1us);
> > +       return list.size();
> > +}
> > +
> >  void AgcExposureMode::Read(boost::property_tree::ptree const &params)
> >  {
> >         int num_shutters =
> > -               read_double_list(shutter, params.get_child("shutter"));
> > -       int num_ags = read_double_list(gain, params.get_child("gain"));
> > +               read_list(shutter, params.get_child("shutter"));
> > +       int num_ags = read_list(gain, params.get_child("gain"));
> >         if (num_shutters < 2 || num_ags < 2)
> >                 throw std::runtime_error(
> >                         "AgcConfig: must have at least two entries in
> exposure profile");
> > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree
> const &params)
> >                 params.get<double>("fast_reduce_threshold", 0.4);
> >         base_ev = params.get<double>("base_ev", 1.0);
> >         // Start with quite a low value as ramping up is easier than
> ramping down.
> > -       default_exposure_time =
> params.get<double>("default_exposure_time", 1000);
> > +       default_exposure_time =
> params.get<double>("default_exposure_time", 1000) * 1us;
> >         default_analogue_gain =
> params.get<double>("default_analogue_gain", 1.0);
> >  }
> >
> > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)
> >         : AgcAlgorithm(controller), metering_mode_(nullptr),
> >           exposure_mode_(nullptr), constraint_mode_(nullptr),
> >           frame_count_(0), lock_count_(0),
> > -         last_target_exposure_(0.0),
> > -         ev_(1.0), flicker_period_(0.0),
> > -         max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> > +         last_target_exposure_(0s),
> > +         ev_(1.0), flicker_period_(0s),
> > +         max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)
> >  {
> >         memset(&awb_, 0, sizeof(awb_));
> >         // Setting status_.total_exposure_value_ to zero initially tells
> us
> > @@ -203,7 +212,7 @@ void Agc::Pause()
> >
> >  void Agc::Resume()
> >  {
> > -       fixed_shutter_ = 0;
> > +       fixed_shutter_ = 0s;
> >         fixed_analogue_gain_ = 0;
> >  }
> >
> > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)
> >
> >  void Agc::SetFlickerPeriod(const Duration &flicker_period)
> >  {
> > -       flicker_period_ = flicker_period.get<std::micro>();
> > +       flicker_period_ = flicker_period;
> >  }
> >
> >  void Agc::SetMaxShutter(const Duration &max_shutter)
> >  {
> > -       max_shutter_ = max_shutter.get<std::micro>();
> > +       max_shutter_ = max_shutter;
> >  }
> >
> >  void Agc::SetFixedShutter(const Duration &fixed_shutter)
> >  {
> > -       fixed_shutter_ = fixed_shutter.get<std::micro>();
> > +       fixed_shutter_ = fixed_shutter;
> >         // Set this in case someone calls Pause() straight after.
> >         status_.shutter_time = clipShutter(fixed_shutter_);
> >  }
> > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
> const &camera_mode,
> >  {
> >         housekeepConfig();
> >
> > -       double fixed_shutter = clipShutter(fixed_shutter_);
> > -       if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> > +       Duration fixed_shutter = clipShutter(fixed_shutter_);
> > +       if (fixed_shutter && fixed_analogue_gain_) {
> >                 // We're going to reset the algorithm here with these
> fixed values.
> >
> >                 fetchAwbStatus(metadata);
> > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)
> >                 // Process has run, so we have meaningful values.
> >                 DeviceStatus device_status;
> >                 if (image_metadata->Get("device.status", device_status)
> == 0) {
> > -                       double actual_exposure =
> device_status.shutter_speed *
> > -
> device_status.analogue_gain;
> > +                       Duration actual_exposure =
> device_status.shutter_speed *
> > +
> device_status.analogue_gain;
> >                         if (actual_exposure) {
> >                                 status_.digital_gain =
> >                                         status_.total_exposure_value /
> > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)
> >                                         std::min(status_.digital_gain,
> 4.0));
> >                                 LOG(RPiAgc, Debug) << "Actual exposure "
> << actual_exposure;
> >                                 LOG(RPiAgc, Debug) << "Use digital_gain
> " << status_.digital_gain;
> > -                               LOG(RPiAgc, Debug) << "Effective
> exposure " << actual_exposure * status_.digital_gain;
> > +                               LOG(RPiAgc, Debug) << "Effective
> exposure "
> > +                                                  << actual_exposure *
> status_.digital_gain;
> >                                 // Decide whether AEC/AGC has converged.
> >                                 updateLockStatus(device_status);
> >                         }
> > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const
> &device_status)
> >         const double RESET_MARGIN = 1.5;
> >
> >         // Add 200us to the exposure time error to allow for line
> quantisation.
> > -       double exposure_error = last_device_status_.shutter_speed *
> ERROR_FACTOR + 200;
> > +       Duration exposure_error = last_device_status_.shutter_speed *
> ERROR_FACTOR + 200us;
> >         double gain_error = last_device_status_.analogue_gain *
> ERROR_FACTOR;
> > -       double target_error = last_target_exposure_ * ERROR_FACTOR;
> > +       Duration target_error = last_target_exposure_ * ERROR_FACTOR;
> >
> >         // Note that we don't know the exposure/gain limits of the
> sensor, so
> >         // the values we keep requesting may be unachievable. For this
> reason
> > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata
> *image_metadata)
> >         current_.analogue_gain = device_status->analogue_gain;
> >         AgcStatus *agc_status =
> >                 image_metadata->GetLocked<AgcStatus>("agc.status");
> > -       current_.total_exposure = agc_status ?
> agc_status->total_exposure_value : 0;
> > +       current_.total_exposure = agc_status ?
> agc_status->total_exposure_value : 0s;
> >         current_.total_exposure_no_dg = current_.shutter *
> current_.analogue_gain;
> >  }
> >
> > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics,
> Metadata *image_metadata,
> >
> >  void Agc::computeTargetExposure(double gain)
> >  {
> > -       if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain
> != 0.0) {
> > +       if (status_.fixed_shutter && status_.fixed_analogue_gain) {
> >                 // When ag and shutter are both fixed, we need to drive
> the
> >                 // total exposure so that we end up with a digital gain
> of at least
> >                 // 1/min_colour_gain. Otherwise we'd desaturate channels
> causing
> > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)
> >                 target_.total_exposure = current_.total_exposure_no_dg *
> gain;
> >                 // The final target exposure is also limited to what the
> exposure
> >                 // mode allows.
> > -               double max_shutter = status_.fixed_shutter != 0.0
> > +               Duration max_shutter = status_.fixed_shutter
> >                                    ? status_.fixed_shutter
> >                                    : exposure_mode_->shutter.back();
> >                 max_shutter = clipShutter(max_shutter);
> > -               double max_total_exposure =
> > +               Duration max_total_exposure =
> >                         max_shutter *
> >                         (status_.fixed_analogue_gain != 0.0
> >                                  ? status_.fixed_analogue_gain
> > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)
> >         if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
> >             frame_count_ <= config_.startup_frames)
> >                 speed = 1.0;
> > -       if (filtered_.total_exposure == 0.0) {
> > +       if (!filtered_.total_exposure) {
> >                 filtered_.total_exposure = target_.total_exposure;
> >                 filtered_.total_exposure_no_dg =
> target_.total_exposure_no_dg;
> >         } else {
> > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()
> >         // Sending the fixed shutter/gain cases through the same code
> may seem
> >         // unnecessary, but it will make more sense when extend this to
> cover
> >         // variable aperture.
> > -       double exposure_value = filtered_.total_exposure_no_dg;
> > -       double shutter_time, analogue_gain;
> > -       shutter_time = status_.fixed_shutter != 0.0
> > +       Duration exposure_value = filtered_.total_exposure_no_dg;
> > +       Duration shutter_time;
> > +       double analogue_gain;
> > +       shutter_time = status_.fixed_shutter
> >                                ? status_.fixed_shutter
> >                                : exposure_mode_->shutter[0];
> >         shutter_time = clipShutter(shutter_time);
> > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()
> >         if (shutter_time * analogue_gain < exposure_value) {
> >                 for (unsigned int stage = 1;
> >                      stage < exposure_mode_->gain.size(); stage++) {
> > -                       if (status_.fixed_shutter == 0.0) {
> > -                               double stage_shutter =
> > +                       if (!status_.fixed_shutter) {
> > +                               Duration stage_shutter =
> >
>  clipShutter(exposure_mode_->shutter[stage]);
> >                                 if (stage_shutter * analogue_gain >=
> >                                     exposure_value) {
> > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()
> >                            << analogue_gain;
> >         // Finally adjust shutter time for flicker avoidance (require
> both
> >         // shutter and gain not to be fixed).
> > -       if (status_.fixed_shutter == 0.0 &&
> > -           status_.fixed_analogue_gain == 0.0 &&
> > -           status_.flicker_period != 0.0) {
> > -               int flicker_periods = shutter_time /
> status_.flicker_period;
> > -               if (flicker_periods > 0) {
> > -                       double new_shutter_time = flicker_periods *
> status_.flicker_period;
> > +       if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
> > +           status_.flicker_period) {
> > +               double flicker_periods = shutter_time /
> status_.flicker_period;
> > +               if (flicker_periods > 0.0) {
> > +                       Duration new_shutter_time = flicker_periods *
> status_.flicker_period;
>
> Just wondering if this will behave the same now that flicker_periods
> is a double. Won't it now hold fractional values with the result that
> new_shutter_time isn't an exact multiple of the period?
>

Quite right, it can be fractional!  I should just keep flicker_periods as
an int to avoid
this.  Thanks for pointing that out.


> >                         analogue_gain *= shutter_time / new_shutter_time;
> >                         // We should still not allow the ag to go over
> the
> >                         // largest value in the exposure mode. Note that
> this
> > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()
> >  void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
> >  {
> >         status_.total_exposure_value = filtered_.total_exposure;
> > -       status_.target_exposure_value = desaturate ? 0 :
> target_.total_exposure_no_dg;
> > +       status_.target_exposure_value = desaturate ? 0s :
> target_.total_exposure_no_dg;
> >         status_.shutter_time = filtered_.shutter;
> >         status_.analogue_gain = filtered_.analogue_gain;
> >         // Write to metadata as well, in case anyone wants to update the
> camera
> > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata,
> bool desaturate)
> >                            << " analogue gain " <<
> filtered_.analogue_gain;
> >  }
> >
> > -double Agc::clipShutter(double shutter)
> > +Duration Agc::clipShutter(const Duration &shutter)
> >  {
> > +       Duration clip_shutter = shutter;
>
> Given that we copy it anyway I wonder if we might have left this as
> call-by-value, resulting in fewer changes. But it's only the teeniest
> of nit-picks!
>
> If you can put my mind at rest over the flicker period thing:
>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>
> Thanks!
> David
>
> >         if (max_shutter_)
> > -               shutter = std::min(shutter, max_shutter_);
> > -       return shutter;
> > +               clip_shutter = std::min(clip_shutter, max_shutter_);
> > +       return clip_shutter;
> >  }
> >
> >  // Register algorithm with the system.
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 16a65959d90e..24a6610096c2 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -9,6 +9,8 @@
> >  #include <vector>
> >  #include <mutex>
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  #include "../agc_algorithm.hpp"
> >  #include "../agc_status.h"
> >  #include "../pwl.hpp"
> > @@ -22,13 +24,15 @@
> >
> >  namespace RPiController {
> >
> > +using namespace std::literals::chrono_literals;
> > +
> >  struct AgcMeteringMode {
> >         double weights[AGC_STATS_SIZE];
> >         void Read(boost::property_tree::ptree const &params);
> >  };
> >
> >  struct AgcExposureMode {
> > -       std::vector<double> shutter;
> > +       std::vector<Duration> shutter;
> >         std::vector<double> gain;
> >         void Read(boost::property_tree::ptree const &params);
> >  };
> > @@ -61,7 +65,7 @@ struct AgcConfig {
> >         std::string default_exposure_mode;
> >         std::string default_constraint_mode;
> >         double base_ev;
> > -       double default_exposure_time;
> > +       Duration default_exposure_time;
> >         double default_analogue_gain;
> >  };
> >
> > @@ -101,19 +105,19 @@ private:
> >         void filterExposure(bool desaturate);
> >         void divideUpExposure();
> >         void writeAndFinish(Metadata *image_metadata, bool desaturate);
> > -       double clipShutter(double shutter);
> > +       Duration clipShutter(const Duration &shutter);
> >         AgcMeteringMode *metering_mode_;
> >         AgcExposureMode *exposure_mode_;
> >         AgcConstraintMode *constraint_mode_;
> >         uint64_t frame_count_;
> >         AwbStatus awb_;
> >         struct ExposureValues {
> > -               ExposureValues() : shutter(0), analogue_gain(0),
> > -                                  total_exposure(0),
> total_exposure_no_dg(0) {}
> > -               double shutter;
> > +               ExposureValues() : shutter(0s), analogue_gain(0),
> > +                                  total_exposure(0s),
> total_exposure_no_dg(0s) {}
> > +               Duration shutter;
> >                 double analogue_gain;
> > -               double total_exposure;
> > -               double total_exposure_no_dg; // without digital gain
> > +               Duration total_exposure;
> > +               Duration total_exposure_no_dg; // without digital gain
> >         };
> >         ExposureValues current_;  // values for the current frame
> >         ExposureValues target_;   // calculate the values we want here
> > @@ -121,15 +125,15 @@ private:
> >         AgcStatus status_;
> >         int lock_count_;
> >         DeviceStatus last_device_status_;
> > -       double last_target_exposure_;
> > +       Duration last_target_exposure_;
> >         // Below here the "settings" that applications can change.
> >         std::string metering_mode_name_;
> >         std::string exposure_mode_name_;
> >         std::string constraint_mode_name_;
> >         double ev_;
> > -       double flicker_period_;
> > -       double max_shutter_;
> > -       double fixed_shutter_;
> > +       Duration flicker_period_;
> > +       Duration max_shutter_;
> > +       Duration fixed_shutter_;
> >         double fixed_analogue_gain_;
> >  };
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > index f74381cab2b4..0fa6457c1376 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > @@ -16,6 +16,7 @@
> >
> >  using namespace RPiController;
> >  using namespace libcamera;
> > +using namespace std::literals::chrono_literals;
> >
> >  LOG_DEFINE_CATEGORY(RPiLux)
> >
> > @@ -38,7 +39,7 @@ char const *Lux::Name() const
> >  void Lux::Read(boost::property_tree::ptree const &params)
> >  {
> >         reference_shutter_speed_ =
> > -               params.get<double>("reference_shutter_speed");
> > +               params.get<double>("reference_shutter_speed") * 1.0us;
> >         reference_gain_ = params.get<double>("reference_gain");
> >         reference_aperture_ = params.get<double>("reference_aperture",
> 1.0);
> >         reference_Y_ = params.get<double>("reference_Y");
> > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
> >  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> >  {
> >         // set some initial values to shut the compiler up
> > -       DeviceStatus device_status =
> > -               { .shutter_speed = 1.0,
> > -                 .analogue_gain = 1.0,
> > -                 .lens_position = 0.0,
> > -                 .aperture = 0.0,
> > -                 .flash_intensity = 0.0 };
> > +       DeviceStatus device_status = { .shutter_speed = 1.0ms,
> > +                                      .analogue_gain = 1.0,
> > +                                      .lens_position = 0.0,
> > +                                      .aperture = 0.0,
> > +                                      .flash_intensity = 0.0 };
> >         if (image_metadata->Get("device.status", device_status) == 0) {
> >                 double current_gain = device_status.analogue_gain;
> > -               double current_shutter_speed =
> device_status.shutter_speed;
> >                 double current_aperture = device_status.aperture;
> >                 if (current_aperture == 0)
> >                         current_aperture = current_aperture_;
> > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata
> *image_metadata)
> >                 double current_Y = sum / (double)num + .5;
> >                 double gain_ratio = reference_gain_ / current_gain;
> >                 double shutter_speed_ratio =
> > -                       reference_shutter_speed_ / current_shutter_speed;
> > +                       reference_shutter_speed_ /
> device_status.shutter_speed;
> >                 double aperture_ratio = reference_aperture_ /
> current_aperture;
> >                 double Y_ratio = current_Y * (65536 / num_bins) /
> reference_Y_;
> >                 double estimated_lux = shutter_speed_ratio * gain_ratio *
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > index f9090484a136..45c844393e62 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > @@ -8,6 +8,8 @@
> >
> >  #include <mutex>
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  #include "../lux_status.h"
> >  #include "../algorithm.hpp"
> >
> > @@ -28,7 +30,7 @@ public:
> >  private:
> >         // These values define the conditions of the reference image,
> against
> >         // which we compare the new image.
> > -       double reference_shutter_speed_; // in micro-seconds
> > +       libcamera::utils::Duration reference_shutter_speed_;
> >         double reference_gain_;
> >         double reference_aperture_; // units of 1/f
> >         double reference_Y_; // out of 65536
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f8f36420b076..b77230ded591 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,
> ipa::RPi::StartConfig *startConf
> >
> >         /* SwitchMode may supply updated exposure/gain values to use. */
> >         AgcStatus agcStatus;
> > -       agcStatus.shutter_time = 0.0;
> > +       agcStatus.shutter_time = 0.0s;
> >         agcStatus.analogue_gain = 0.0;
> >
> >         metadata.Get("agc.status", agcStatus);
> > -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=
> 0.0) {
> > +       if (agcStatus.shutter_time && agcStatus.analogue_gain) {
> >                 ControlList ctrls(sensorCtrls_);
> >                 applyAGC(&agcStatus, ctrls);
> >                 startConfig->controls = std::move(ctrls);
> > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >                 /* Supply initial values for gain and exposure. */
> >                 ControlList ctrls(sensorCtrls_);
> >                 AgcStatus agcStatus;
> > -               agcStatus.shutter_time =
> DefaultExposureTime.get<std::micro>();
> > +               agcStatus.shutter_time = DefaultExposureTime;
> >                 agcStatus.analogue_gain = DefaultAnalogueGain;
> >                 applyAGC(&agcStatus, ctrls);
> >
> > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()
> >          */
> >         DeviceStatus *deviceStatus =
> rpiMetadata_.GetLocked<DeviceStatus>("device.status");
> >         if (deviceStatus) {
> > -               libcameraMetadata_.set(controls::ExposureTime,
> deviceStatus->shutter_speed);
> > +               libcameraMetadata_.set(controls::ExposureTime,
> > +
> deviceStatus->shutter_speed.get<std::micro>());
> >                 libcameraMetadata_.set(controls::AnalogueGain,
> deviceStatus->analogue_gain);
> >         }
> >
> > @@ -1017,7 +1018,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).get<std::micro>();
> > +       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> >         deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >
> >         LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > @@ -1103,7 +1104,7 @@ 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. */
> > -       Duration exposure = agcStatus->shutter_time * 1.0us;
> > +       Duration exposure = agcStatus->shutter_time;
> >         int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
> >         int32_t exposureLines = helper_->ExposureLines(exposure);
> >
> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > index 3131aa2d6666..5fa6f8366f3d 100644
> > --- a/src/libcamera/utils.cpp
> > +++ b/src/libcamera/utils.cpp
> > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()
> >   * loop, iterates over an indexed view of the \a iterable
> >   */
> >
> > +/**
> > + * \typedef BaseDuration
> > + * \brief Base struct for the \a Duration helper class.
> > + */
> > +
> >  /**
> >   * \class Duration
> > - * \brief Helper for std::chrono::duration types. Derived from \a
> BaseDuration
> > + * \brief Helper class for std::chrono::duration types.
> >   */
> >
> >  /**
> > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()
> >   * \param[in] d The std::chrono::duration object to convert from.
> >   *
> >   * Constructs a \a Duration object from a \a std::chrono::duration
> object,
> > - * converting the representation to \a BaseDuration type.
> > + * internally converting the representation to \a BaseDuration type.
> >   */
> >
> >  /**
> > --
> > 2.25.1
> >
>

On Fri, 21 May 2021 at 12:36, Naushir Patuck <naush@raspberrypi.com> wrote:

> On Fri, 21 May 2021 at 12:19, David Plowman <david.plowman@raspberrypi.com>
> wrote:
>
>> Hi Naush
>>
>> Thanks for all this work!
>>
>> On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com>
>> wrote:
>> >
>> > Convert the core AGC and Lux controller code to use
>> > utils::Duration for all exposure time related variables and
>> > calculations.
>> >
>> > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
>> > to use utils::Duration.
>> >
>> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>> > ---
>> >  src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
>> >  src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
>> >  .../raspberrypi/controller/device_status.h    |  6 +-
>> >  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
>> >  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
>> >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
>> >  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
>> >  src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
>> >  src/libcamera/utils.cpp                       |  9 +-
>> >  9 files changed, 105 insertions(+), 77 deletions(-)
>> >
>> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
>> b/src/ipa/raspberrypi/cam_helper.cpp
>> > index feb02d1806b2..968fae13bd5e 100644
>> > --- a/src/ipa/raspberrypi/cam_helper.cpp
>> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
>> > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const
>> uint8_t> buffer,
>> >                 return;
>> >         }
>> >
>> > -       deviceStatus.shutter_speed =
>> Exposure(exposureLines).get<std::micro>();
>> > +       deviceStatus.shutter_speed = Exposure(exposureLines);
>> >         deviceStatus.analogue_gain = Gain(gainCode);
>> >
>> >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
>> > diff --git a/src/ipa/raspberrypi/controller/agc_status.h
>> b/src/ipa/raspberrypi/controller/agc_status.h
>> > index 10381c90a313..c893715af588 100644
>> > --- a/src/ipa/raspberrypi/controller/agc_status.h
>> > +++ b/src/ipa/raspberrypi/controller/agc_status.h
>> > @@ -6,6 +6,8 @@
>> >   */
>> >  #pragma once
>> >
>> > +#include "libcamera/internal/utils.h"
>> > +
>> >  // The AGC algorithm should post the following structure into the
>> image's
>> >  // "agc.status" metadata.
>> >
>> > @@ -17,18 +19,20 @@ extern "C" {
>> >  // seen statistics and calculated meaningful values. The contents
>> should be
>> >  // ignored until then.
>> >
>> > +using libcamera::utils::Duration;
>> > +
>> >  struct AgcStatus {
>> > -       double total_exposure_value; // value for all exposure and gain
>> for this image
>> > -       double target_exposure_value; // (unfiltered) target total
>> exposure AGC is aiming for
>> > -       double shutter_time;
>> > +       Duration total_exposure_value; // value for all exposure and
>> gain for this image
>> > +       Duration target_exposure_value; // (unfiltered) target total
>> exposure AGC is aiming for
>> > +       Duration shutter_time;
>> >         double analogue_gain;
>> >         char exposure_mode[32];
>> >         char constraint_mode[32];
>> >         char metering_mode[32];
>> >         double ev;
>> > -       double flicker_period;
>> > +       Duration flicker_period;
>> >         int floating_region_enable;
>> > -       double fixed_shutter;
>> > +       Duration fixed_shutter;
>> >         double fixed_analogue_gain;
>> >         double digital_gain;
>> >         int locked;
>> > diff --git a/src/ipa/raspberrypi/controller/device_status.h
>> b/src/ipa/raspberrypi/controller/device_status.h
>> > index aa08608b5d40..131b4cd344ee 100644
>> > --- a/src/ipa/raspberrypi/controller/device_status.h
>> > +++ b/src/ipa/raspberrypi/controller/device_status.h
>> > @@ -6,6 +6,8 @@
>> >   */
>> >  #pragma once
>> >
>> > +#include "libcamera/internal/utils.h"
>> > +
>> >  // Definition of "device metadata" which stores things like shutter
>> time and
>> >  // analogue gain that downstream control algorithms will want to know.
>> >
>> > @@ -14,8 +16,8 @@ extern "C" {
>> >  #endif
>> >
>> >  struct DeviceStatus {
>> > -       // time shutter is open, in microseconds
>> > -       double shutter_speed;
>> > +       // time shutter is open
>> > +       libcamera::utils::Duration shutter_speed;
>> >         double analogue_gain;
>> >         // 1.0/distance-in-metres, or 0 if unknown
>> >         double lens_position;
>> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> > index 482eb3ef962d..46742d845034 100644
>> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> > @@ -21,6 +21,7 @@
>> >
>> >  using namespace RPiController;
>> >  using namespace libcamera;
>> > +using utils::operator<<;
>>
>> Still can't say that I like this... just because someone wants to use
>> Durations, it seems like an awkward thing to make them remember. But
>> unless anyone has anything better....
>>
>
Sorry, I forgot to add this comment in the last email.
One option to make things cleaner is to move the operator<< into the
libcamera namespace,
avoiding the need to add the above using directive everywhere.


>
>> >
>> >  LOG_DEFINE_CATEGORY(RPiAgc)
>> >
>> > @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string,
>> AgcMeteringMode> &metering_modes,
>> >         return first;
>> >  }
>> >
>> > -static int read_double_list(std::vector<double> &list,
>> > -                           boost::property_tree::ptree const &params)
>> > +static int read_list(std::vector<double> &list,
>> > +                    boost::property_tree::ptree const &params)
>> >  {
>> >         for (auto &p : params)
>> >                 list.push_back(p.second.get_value<double>());
>> >         return list.size();
>> >  }
>> >
>> > +static int read_list(std::vector<Duration> &list,
>> > +                    boost::property_tree::ptree const &params)
>> > +{
>> > +       for (auto &p : params)
>> > +               list.push_back(p.second.get_value<double>() * 1us);
>> > +       return list.size();
>> > +}
>> > +
>> >  void AgcExposureMode::Read(boost::property_tree::ptree const &params)
>> >  {
>> >         int num_shutters =
>> > -               read_double_list(shutter, params.get_child("shutter"));
>> > -       int num_ags = read_double_list(gain, params.get_child("gain"));
>> > +               read_list(shutter, params.get_child("shutter"));
>> > +       int num_ags = read_list(gain, params.get_child("gain"));
>> >         if (num_shutters < 2 || num_ags < 2)
>> >                 throw std::runtime_error(
>> >                         "AgcConfig: must have at least two entries in
>> exposure profile");
>> > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree
>> const &params)
>> >                 params.get<double>("fast_reduce_threshold", 0.4);
>> >         base_ev = params.get<double>("base_ev", 1.0);
>> >         // Start with quite a low value as ramping up is easier than
>> ramping down.
>> > -       default_exposure_time =
>> params.get<double>("default_exposure_time", 1000);
>> > +       default_exposure_time =
>> params.get<double>("default_exposure_time", 1000) * 1us;
>> >         default_analogue_gain =
>> params.get<double>("default_analogue_gain", 1.0);
>> >  }
>> >
>> > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)
>> >         : AgcAlgorithm(controller), metering_mode_(nullptr),
>> >           exposure_mode_(nullptr), constraint_mode_(nullptr),
>> >           frame_count_(0), lock_count_(0),
>> > -         last_target_exposure_(0.0),
>> > -         ev_(1.0), flicker_period_(0.0),
>> > -         max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
>> > +         last_target_exposure_(0s),
>> > +         ev_(1.0), flicker_period_(0s),
>> > +         max_shutter_(0s), fixed_shutter_(0s),
>> fixed_analogue_gain_(0.0)
>> >  {
>> >         memset(&awb_, 0, sizeof(awb_));
>> >         // Setting status_.total_exposure_value_ to zero initially
>> tells us
>> > @@ -203,7 +212,7 @@ void Agc::Pause()
>> >
>> >  void Agc::Resume()
>> >  {
>> > -       fixed_shutter_ = 0;
>> > +       fixed_shutter_ = 0s;
>> >         fixed_analogue_gain_ = 0;
>> >  }
>> >
>> > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)
>> >
>> >  void Agc::SetFlickerPeriod(const Duration &flicker_period)
>> >  {
>> > -       flicker_period_ = flicker_period.get<std::micro>();
>> > +       flicker_period_ = flicker_period;
>> >  }
>> >
>> >  void Agc::SetMaxShutter(const Duration &max_shutter)
>> >  {
>> > -       max_shutter_ = max_shutter.get<std::micro>();
>> > +       max_shutter_ = max_shutter;
>> >  }
>> >
>> >  void Agc::SetFixedShutter(const Duration &fixed_shutter)
>> >  {
>> > -       fixed_shutter_ = fixed_shutter.get<std::micro>();
>> > +       fixed_shutter_ = fixed_shutter;
>> >         // Set this in case someone calls Pause() straight after.
>> >         status_.shutter_time = clipShutter(fixed_shutter_);
>> >  }
>> > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
>> const &camera_mode,
>> >  {
>> >         housekeepConfig();
>> >
>> > -       double fixed_shutter = clipShutter(fixed_shutter_);
>> > -       if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
>> > +       Duration fixed_shutter = clipShutter(fixed_shutter_);
>> > +       if (fixed_shutter && fixed_analogue_gain_) {
>> >                 // We're going to reset the algorithm here with these
>> fixed values.
>> >
>> >                 fetchAwbStatus(metadata);
>> > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)
>> >                 // Process has run, so we have meaningful values.
>> >                 DeviceStatus device_status;
>> >                 if (image_metadata->Get("device.status", device_status)
>> == 0) {
>> > -                       double actual_exposure =
>> device_status.shutter_speed *
>> > -
>> device_status.analogue_gain;
>> > +                       Duration actual_exposure =
>> device_status.shutter_speed *
>> > +
>> device_status.analogue_gain;
>> >                         if (actual_exposure) {
>> >                                 status_.digital_gain =
>> >                                         status_.total_exposure_value /
>> > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)
>> >                                         std::min(status_.digital_gain,
>> 4.0));
>> >                                 LOG(RPiAgc, Debug) << "Actual exposure
>> " << actual_exposure;
>> >                                 LOG(RPiAgc, Debug) << "Use digital_gain
>> " << status_.digital_gain;
>> > -                               LOG(RPiAgc, Debug) << "Effective
>> exposure " << actual_exposure * status_.digital_gain;
>> > +                               LOG(RPiAgc, Debug) << "Effective
>> exposure "
>> > +                                                  << actual_exposure *
>> status_.digital_gain;
>> >                                 // Decide whether AEC/AGC has converged.
>> >                                 updateLockStatus(device_status);
>> >                         }
>> > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const
>> &device_status)
>> >         const double RESET_MARGIN = 1.5;
>> >
>> >         // Add 200us to the exposure time error to allow for line
>> quantisation.
>> > -       double exposure_error = last_device_status_.shutter_speed *
>> ERROR_FACTOR + 200;
>> > +       Duration exposure_error = last_device_status_.shutter_speed *
>> ERROR_FACTOR + 200us;
>> >         double gain_error = last_device_status_.analogue_gain *
>> ERROR_FACTOR;
>> > -       double target_error = last_target_exposure_ * ERROR_FACTOR;
>> > +       Duration target_error = last_target_exposure_ * ERROR_FACTOR;
>> >
>> >         // Note that we don't know the exposure/gain limits of the
>> sensor, so
>> >         // the values we keep requesting may be unachievable. For this
>> reason
>> > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata
>> *image_metadata)
>> >         current_.analogue_gain = device_status->analogue_gain;
>> >         AgcStatus *agc_status =
>> >                 image_metadata->GetLocked<AgcStatus>("agc.status");
>> > -       current_.total_exposure = agc_status ?
>> agc_status->total_exposure_value : 0;
>> > +       current_.total_exposure = agc_status ?
>> agc_status->total_exposure_value : 0s;
>> >         current_.total_exposure_no_dg = current_.shutter *
>> current_.analogue_gain;
>> >  }
>> >
>> > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats
>> *statistics, Metadata *image_metadata,
>> >
>> >  void Agc::computeTargetExposure(double gain)
>> >  {
>> > -       if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain
>> != 0.0) {
>> > +       if (status_.fixed_shutter && status_.fixed_analogue_gain) {
>> >                 // When ag and shutter are both fixed, we need to drive
>> the
>> >                 // total exposure so that we end up with a digital gain
>> of at least
>> >                 // 1/min_colour_gain. Otherwise we'd desaturate
>> channels causing
>> > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)
>> >                 target_.total_exposure = current_.total_exposure_no_dg
>> * gain;
>> >                 // The final target exposure is also limited to what
>> the exposure
>> >                 // mode allows.
>> > -               double max_shutter = status_.fixed_shutter != 0.0
>> > +               Duration max_shutter = status_.fixed_shutter
>> >                                    ? status_.fixed_shutter
>> >                                    : exposure_mode_->shutter.back();
>> >                 max_shutter = clipShutter(max_shutter);
>> > -               double max_total_exposure =
>> > +               Duration max_total_exposure =
>> >                         max_shutter *
>> >                         (status_.fixed_analogue_gain != 0.0
>> >                                  ? status_.fixed_analogue_gain
>> > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)
>> >         if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
>> >             frame_count_ <= config_.startup_frames)
>> >                 speed = 1.0;
>> > -       if (filtered_.total_exposure == 0.0) {
>> > +       if (!filtered_.total_exposure) {
>> >                 filtered_.total_exposure = target_.total_exposure;
>> >                 filtered_.total_exposure_no_dg =
>> target_.total_exposure_no_dg;
>> >         } else {
>> > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()
>> >         // Sending the fixed shutter/gain cases through the same code
>> may seem
>> >         // unnecessary, but it will make more sense when extend this to
>> cover
>> >         // variable aperture.
>> > -       double exposure_value = filtered_.total_exposure_no_dg;
>> > -       double shutter_time, analogue_gain;
>> > -       shutter_time = status_.fixed_shutter != 0.0
>> > +       Duration exposure_value = filtered_.total_exposure_no_dg;
>> > +       Duration shutter_time;
>> > +       double analogue_gain;
>> > +       shutter_time = status_.fixed_shutter
>> >                                ? status_.fixed_shutter
>> >                                : exposure_mode_->shutter[0];
>> >         shutter_time = clipShutter(shutter_time);
>> > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()
>> >         if (shutter_time * analogue_gain < exposure_value) {
>> >                 for (unsigned int stage = 1;
>> >                      stage < exposure_mode_->gain.size(); stage++) {
>> > -                       if (status_.fixed_shutter == 0.0) {
>> > -                               double stage_shutter =
>> > +                       if (!status_.fixed_shutter) {
>> > +                               Duration stage_shutter =
>> >
>>  clipShutter(exposure_mode_->shutter[stage]);
>> >                                 if (stage_shutter * analogue_gain >=
>> >                                     exposure_value) {
>> > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()
>> >                            << analogue_gain;
>> >         // Finally adjust shutter time for flicker avoidance (require
>> both
>> >         // shutter and gain not to be fixed).
>> > -       if (status_.fixed_shutter == 0.0 &&
>> > -           status_.fixed_analogue_gain == 0.0 &&
>> > -           status_.flicker_period != 0.0) {
>> > -               int flicker_periods = shutter_time /
>> status_.flicker_period;
>> > -               if (flicker_periods > 0) {
>> > -                       double new_shutter_time = flicker_periods *
>> status_.flicker_period;
>> > +       if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
>> > +           status_.flicker_period) {
>> > +               double flicker_periods = shutter_time /
>> status_.flicker_period;
>> > +               if (flicker_periods > 0.0) {
>> > +                       Duration new_shutter_time = flicker_periods *
>> status_.flicker_period;
>>
>> Just wondering if this will behave the same now that flicker_periods
>> is a double. Won't it now hold fractional values with the result that
>> new_shutter_time isn't an exact multiple of the period?
>>
>
> Quite right, it can be fractional!  I should just keep flicker_periods as
> an int to avoid
> this.  Thanks for pointing that out.
>
>
>> >                         analogue_gain *= shutter_time /
>> new_shutter_time;
>> >                         // We should still not allow the ag to go over
>> the
>> >                         // largest value in the exposure mode. Note
>> that this
>> > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()
>> >  void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
>> >  {
>> >         status_.total_exposure_value = filtered_.total_exposure;
>> > -       status_.target_exposure_value = desaturate ? 0 :
>> target_.total_exposure_no_dg;
>> > +       status_.target_exposure_value = desaturate ? 0s :
>> target_.total_exposure_no_dg;
>> >         status_.shutter_time = filtered_.shutter;
>> >         status_.analogue_gain = filtered_.analogue_gain;
>> >         // Write to metadata as well, in case anyone wants to update
>> the camera
>> > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata
>> *image_metadata, bool desaturate)
>> >                            << " analogue gain " <<
>> filtered_.analogue_gain;
>> >  }
>> >
>> > -double Agc::clipShutter(double shutter)
>> > +Duration Agc::clipShutter(const Duration &shutter)
>> >  {
>> > +       Duration clip_shutter = shutter;
>>
>> Given that we copy it anyway I wonder if we might have left this as
>> call-by-value, resulting in fewer changes. But it's only the teeniest
>> of nit-picks!
>>
>> If you can put my mind at rest over the flicker period thing:
>>
>> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>>
>> Thanks!
>> David
>>
>> >         if (max_shutter_)
>> > -               shutter = std::min(shutter, max_shutter_);
>> > -       return shutter;
>> > +               clip_shutter = std::min(clip_shutter, max_shutter_);
>> > +       return clip_shutter;
>> >  }
>> >
>> >  // Register algorithm with the system.
>> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
>> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
>> > index 16a65959d90e..24a6610096c2 100644
>> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
>> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
>> > @@ -9,6 +9,8 @@
>> >  #include <vector>
>> >  #include <mutex>
>> >
>> > +#include "libcamera/internal/utils.h"
>> > +
>> >  #include "../agc_algorithm.hpp"
>> >  #include "../agc_status.h"
>> >  #include "../pwl.hpp"
>> > @@ -22,13 +24,15 @@
>> >
>> >  namespace RPiController {
>> >
>> > +using namespace std::literals::chrono_literals;
>> > +
>> >  struct AgcMeteringMode {
>> >         double weights[AGC_STATS_SIZE];
>> >         void Read(boost::property_tree::ptree const &params);
>> >  };
>> >
>> >  struct AgcExposureMode {
>> > -       std::vector<double> shutter;
>> > +       std::vector<Duration> shutter;
>> >         std::vector<double> gain;
>> >         void Read(boost::property_tree::ptree const &params);
>> >  };
>> > @@ -61,7 +65,7 @@ struct AgcConfig {
>> >         std::string default_exposure_mode;
>> >         std::string default_constraint_mode;
>> >         double base_ev;
>> > -       double default_exposure_time;
>> > +       Duration default_exposure_time;
>> >         double default_analogue_gain;
>> >  };
>> >
>> > @@ -101,19 +105,19 @@ private:
>> >         void filterExposure(bool desaturate);
>> >         void divideUpExposure();
>> >         void writeAndFinish(Metadata *image_metadata, bool desaturate);
>> > -       double clipShutter(double shutter);
>> > +       Duration clipShutter(const Duration &shutter);
>> >         AgcMeteringMode *metering_mode_;
>> >         AgcExposureMode *exposure_mode_;
>> >         AgcConstraintMode *constraint_mode_;
>> >         uint64_t frame_count_;
>> >         AwbStatus awb_;
>> >         struct ExposureValues {
>> > -               ExposureValues() : shutter(0), analogue_gain(0),
>> > -                                  total_exposure(0),
>> total_exposure_no_dg(0) {}
>> > -               double shutter;
>> > +               ExposureValues() : shutter(0s), analogue_gain(0),
>> > +                                  total_exposure(0s),
>> total_exposure_no_dg(0s) {}
>> > +               Duration shutter;
>> >                 double analogue_gain;
>> > -               double total_exposure;
>> > -               double total_exposure_no_dg; // without digital gain
>> > +               Duration total_exposure;
>> > +               Duration total_exposure_no_dg; // without digital gain
>> >         };
>> >         ExposureValues current_;  // values for the current frame
>> >         ExposureValues target_;   // calculate the values we want here
>> > @@ -121,15 +125,15 @@ private:
>> >         AgcStatus status_;
>> >         int lock_count_;
>> >         DeviceStatus last_device_status_;
>> > -       double last_target_exposure_;
>> > +       Duration last_target_exposure_;
>> >         // Below here the "settings" that applications can change.
>> >         std::string metering_mode_name_;
>> >         std::string exposure_mode_name_;
>> >         std::string constraint_mode_name_;
>> >         double ev_;
>> > -       double flicker_period_;
>> > -       double max_shutter_;
>> > -       double fixed_shutter_;
>> > +       Duration flicker_period_;
>> > +       Duration max_shutter_;
>> > +       Duration fixed_shutter_;
>> >         double fixed_analogue_gain_;
>> >  };
>> >
>> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> > index f74381cab2b4..0fa6457c1376 100644
>> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> > @@ -16,6 +16,7 @@
>> >
>> >  using namespace RPiController;
>> >  using namespace libcamera;
>> > +using namespace std::literals::chrono_literals;
>> >
>> >  LOG_DEFINE_CATEGORY(RPiLux)
>> >
>> > @@ -38,7 +39,7 @@ char const *Lux::Name() const
>> >  void Lux::Read(boost::property_tree::ptree const &params)
>> >  {
>> >         reference_shutter_speed_ =
>> > -               params.get<double>("reference_shutter_speed");
>> > +               params.get<double>("reference_shutter_speed") * 1.0us;
>> >         reference_gain_ = params.get<double>("reference_gain");
>> >         reference_aperture_ = params.get<double>("reference_aperture",
>> 1.0);
>> >         reference_Y_ = params.get<double>("reference_Y");
>> > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
>> >  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
>> >  {
>> >         // set some initial values to shut the compiler up
>> > -       DeviceStatus device_status =
>> > -               { .shutter_speed = 1.0,
>> > -                 .analogue_gain = 1.0,
>> > -                 .lens_position = 0.0,
>> > -                 .aperture = 0.0,
>> > -                 .flash_intensity = 0.0 };
>> > +       DeviceStatus device_status = { .shutter_speed = 1.0ms,
>> > +                                      .analogue_gain = 1.0,
>> > +                                      .lens_position = 0.0,
>> > +                                      .aperture = 0.0,
>> > +                                      .flash_intensity = 0.0 };
>> >         if (image_metadata->Get("device.status", device_status) == 0) {
>> >                 double current_gain = device_status.analogue_gain;
>> > -               double current_shutter_speed =
>> device_status.shutter_speed;
>> >                 double current_aperture = device_status.aperture;
>> >                 if (current_aperture == 0)
>> >                         current_aperture = current_aperture_;
>> > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata
>> *image_metadata)
>> >                 double current_Y = sum / (double)num + .5;
>> >                 double gain_ratio = reference_gain_ / current_gain;
>> >                 double shutter_speed_ratio =
>> > -                       reference_shutter_speed_ /
>> current_shutter_speed;
>> > +                       reference_shutter_speed_ /
>> device_status.shutter_speed;
>> >                 double aperture_ratio = reference_aperture_ /
>> current_aperture;
>> >                 double Y_ratio = current_Y * (65536 / num_bins) /
>> reference_Y_;
>> >                 double estimated_lux = shutter_speed_ratio * gain_ratio
>> *
>> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp
>> b/src/ipa/raspberrypi/controller/rpi/lux.hpp
>> > index f9090484a136..45c844393e62 100644
>> > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
>> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
>> > @@ -8,6 +8,8 @@
>> >
>> >  #include <mutex>
>> >
>> > +#include "libcamera/internal/utils.h"
>> > +
>> >  #include "../lux_status.h"
>> >  #include "../algorithm.hpp"
>> >
>> > @@ -28,7 +30,7 @@ public:
>> >  private:
>> >         // These values define the conditions of the reference image,
>> against
>> >         // which we compare the new image.
>> > -       double reference_shutter_speed_; // in micro-seconds
>> > +       libcamera::utils::Duration reference_shutter_speed_;
>> >         double reference_gain_;
>> >         double reference_aperture_; // units of 1/f
>> >         double reference_Y_; // out of 65536
>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>> b/src/ipa/raspberrypi/raspberrypi.cpp
>> > index f8f36420b076..b77230ded591 100644
>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,
>> ipa::RPi::StartConfig *startConf
>> >
>> >         /* SwitchMode may supply updated exposure/gain values to use. */
>> >         AgcStatus agcStatus;
>> > -       agcStatus.shutter_time = 0.0;
>> > +       agcStatus.shutter_time = 0.0s;
>> >         agcStatus.analogue_gain = 0.0;
>> >
>> >         metadata.Get("agc.status", agcStatus);
>> > -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=
>> 0.0) {
>> > +       if (agcStatus.shutter_time && agcStatus.analogue_gain) {
>> >                 ControlList ctrls(sensorCtrls_);
>> >                 applyAGC(&agcStatus, ctrls);
>> >                 startConfig->controls = std::move(ctrls);
>> > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo
>> &sensorInfo,
>> >                 /* Supply initial values for gain and exposure. */
>> >                 ControlList ctrls(sensorCtrls_);
>> >                 AgcStatus agcStatus;
>> > -               agcStatus.shutter_time =
>> DefaultExposureTime.get<std::micro>();
>> > +               agcStatus.shutter_time = DefaultExposureTime;
>> >                 agcStatus.analogue_gain = DefaultAnalogueGain;
>> >                 applyAGC(&agcStatus, ctrls);
>> >
>> > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()
>> >          */
>> >         DeviceStatus *deviceStatus =
>> rpiMetadata_.GetLocked<DeviceStatus>("device.status");
>> >         if (deviceStatus) {
>> > -               libcameraMetadata_.set(controls::ExposureTime,
>> deviceStatus->shutter_speed);
>> > +               libcameraMetadata_.set(controls::ExposureTime,
>> > +
>> deviceStatus->shutter_speed.get<std::micro>());
>> >                 libcameraMetadata_.set(controls::AnalogueGain,
>> deviceStatus->analogue_gain);
>> >         }
>> >
>> > @@ -1017,7 +1018,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).get<std::micro>();
>> > +       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>> >         deviceStatus.analogue_gain = helper_->Gain(gainCode);
>> >
>> >         LOG(IPARPI, Debug) << "Metadata - Exposure : "
>> > @@ -1103,7 +1104,7 @@ 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. */
>> > -       Duration exposure = agcStatus->shutter_time * 1.0us;
>> > +       Duration exposure = agcStatus->shutter_time;
>> >         int32_t vblanking = helper_->GetVBlanking(exposure,
>> minFrameDuration_, maxFrameDuration_);
>> >         int32_t exposureLines = helper_->ExposureLines(exposure);
>> >
>> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
>> > index 3131aa2d6666..5fa6f8366f3d 100644
>> > --- a/src/libcamera/utils.cpp
>> > +++ b/src/libcamera/utils.cpp
>> > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()
>> >   * loop, iterates over an indexed view of the \a iterable
>> >   */
>> >
>> > +/**
>> > + * \typedef BaseDuration
>> > + * \brief Base struct for the \a Duration helper class.
>> > + */
>> > +
>> >  /**
>> >   * \class Duration
>> > - * \brief Helper for std::chrono::duration types. Derived from \a
>> BaseDuration
>> > + * \brief Helper class for std::chrono::duration types.
>> >   */
>> >
>> >  /**
>> > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()
>> >   * \param[in] d The std::chrono::duration object to convert from.
>> >   *
>> >   * Constructs a \a Duration object from a \a std::chrono::duration
>> object,
>> > - * converting the representation to \a BaseDuration type.
>> > + * internally converting the representation to \a BaseDuration type.
>> >   */
>> >
>> >  /**
>> > --
>> > 2.25.1
>> >
>>
>

Hello,

On Fri, May 21, 2021 at 12:18:55PM +0100, David Plowman wrote:
> Hi Naush
>
> Thanks for all this work!
>
> On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com> wrote:
> >
> > Convert the core AGC and Lux controller code to use
> > utils::Duration for all exposure time related variables and
> > calculations.
> >
> > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
> > to use utils::Duration.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
> >  src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
> >  .../raspberrypi/controller/device_status.h    |  6 +-
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
> >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
> >  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
> >  src/libcamera/utils.cpp                       |  9 +-
> >  9 files changed, 105 insertions(+), 77 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > index feb02d1806b2..968fae13bd5e 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> >                 return;
> >         }
> >
> > -       deviceStatus.shutter_speed = Exposure(exposureLines).get<std::micro>();
> > +       deviceStatus.shutter_speed = Exposure(exposureLines);
> >         deviceStatus.analogue_gain = Gain(gainCode);
> >
> >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > diff --git a/src/ipa/raspberrypi/controller/agc_status.h b/src/ipa/raspberrypi/controller/agc_status.h
> > index 10381c90a313..c893715af588 100644
> > --- a/src/ipa/raspberrypi/controller/agc_status.h
> > +++ b/src/ipa/raspberrypi/controller/agc_status.h
> > @@ -6,6 +6,8 @@
> >   */
> >  #pragma once
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  // The AGC algorithm should post the following structure into the image's
> >  // "agc.status" metadata.
> >
> > @@ -17,18 +19,20 @@ extern "C" {
> >  // seen statistics and calculated meaningful values. The contents should be
> >  // ignored until then.
> >
> > +using libcamera::utils::Duration;
> > +
> >  struct AgcStatus {
> > -       double total_exposure_value; // value for all exposure and gain for this image
> > -       double target_exposure_value; // (unfiltered) target total exposure AGC is aiming for
> > -       double shutter_time;
> > +       Duration total_exposure_value; // value for all exposure and gain for this image
> > +       Duration target_exposure_value; // (unfiltered) target total exposure AGC is aiming for
> > +       Duration shutter_time;
> >         double analogue_gain;
> >         char exposure_mode[32];
> >         char constraint_mode[32];
> >         char metering_mode[32];
> >         double ev;
> > -       double flicker_period;
> > +       Duration flicker_period;
> >         int floating_region_enable;
> > -       double fixed_shutter;
> > +       Duration fixed_shutter;
> >         double fixed_analogue_gain;
> >         double digital_gain;
> >         int locked;
> > diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> > index aa08608b5d40..131b4cd344ee 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.h
> > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > @@ -6,6 +6,8 @@
> >   */
> >  #pragma once
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  // Definition of "device metadata" which stores things like shutter time and
> >  // analogue gain that downstream control algorithms will want to know.
> >
> > @@ -14,8 +16,8 @@ extern "C" {
> >  #endif
> >
> >  struct DeviceStatus {
> > -       // time shutter is open, in microseconds
> > -       double shutter_speed;
> > +       // time shutter is open
> > +       libcamera::utils::Duration shutter_speed;
> >         double analogue_gain;
> >         // 1.0/distance-in-metres, or 0 if unknown
> >         double lens_position;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 482eb3ef962d..46742d845034 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -21,6 +21,7 @@
> >
> >  using namespace RPiController;
> >  using namespace libcamera;
> > +using utils::operator<<;
>
> Still can't say that I like this... just because someone wants to use
> Durations, it seems like an awkward thing to make them remember. But
> unless anyone has anything better....

I got the same feeling while reading the rest of the patches.
I can't say toString() is that better though, but probably is less
implicit that this using statment, which if read in isolation does not
immediately relates to the Duration class ?

>
> >
> >  LOG_DEFINE_CATEGORY(RPiAgc)
> >
> > @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string, AgcMeteringMode> &metering_modes,
> >         return first;
> >  }
> >
> > -static int read_double_list(std::vector<double> &list,
> > -                           boost::property_tree::ptree const &params)
> > +static int read_list(std::vector<double> &list,
> > +                    boost::property_tree::ptree const &params)
> >  {
> >         for (auto &p : params)
> >                 list.push_back(p.second.get_value<double>());
> >         return list.size();
> >  }
> >
> > +static int read_list(std::vector<Duration> &list,
> > +                    boost::property_tree::ptree const &params)
> > +{
> > +       for (auto &p : params)
> > +               list.push_back(p.second.get_value<double>() * 1us);
> > +       return list.size();
> > +}
> > +
> >  void AgcExposureMode::Read(boost::property_tree::ptree const &params)
> >  {
> >         int num_shutters =
> > -               read_double_list(shutter, params.get_child("shutter"));
> > -       int num_ags = read_double_list(gain, params.get_child("gain"));
> > +               read_list(shutter, params.get_child("shutter"));
> > +       int num_ags = read_list(gain, params.get_child("gain"));
> >         if (num_shutters < 2 || num_ags < 2)
> >                 throw std::runtime_error(
> >                         "AgcConfig: must have at least two entries in exposure profile");
> > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
> >                 params.get<double>("fast_reduce_threshold", 0.4);
> >         base_ev = params.get<double>("base_ev", 1.0);
> >         // Start with quite a low value as ramping up is easier than ramping down.
> > -       default_exposure_time = params.get<double>("default_exposure_time", 1000);
> > +       default_exposure_time = params.get<double>("default_exposure_time", 1000) * 1us;
> >         default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
> >  }
> >
> > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)
> >         : AgcAlgorithm(controller), metering_mode_(nullptr),
> >           exposure_mode_(nullptr), constraint_mode_(nullptr),
> >           frame_count_(0), lock_count_(0),
> > -         last_target_exposure_(0.0),
> > -         ev_(1.0), flicker_period_(0.0),
> > -         max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> > +         last_target_exposure_(0s),
> > +         ev_(1.0), flicker_period_(0s),
> > +         max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)
> >  {
> >         memset(&awb_, 0, sizeof(awb_));
> >         // Setting status_.total_exposure_value_ to zero initially tells us
> > @@ -203,7 +212,7 @@ void Agc::Pause()
> >
> >  void Agc::Resume()
> >  {
> > -       fixed_shutter_ = 0;
> > +       fixed_shutter_ = 0s;
> >         fixed_analogue_gain_ = 0;
> >  }
> >
> > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)
> >
> >  void Agc::SetFlickerPeriod(const Duration &flicker_period)
> >  {
> > -       flicker_period_ = flicker_period.get<std::micro>();
> > +       flicker_period_ = flicker_period;
> >  }
> >
> >  void Agc::SetMaxShutter(const Duration &max_shutter)
> >  {
> > -       max_shutter_ = max_shutter.get<std::micro>();
> > +       max_shutter_ = max_shutter;
> >  }
> >
> >  void Agc::SetFixedShutter(const Duration &fixed_shutter)
> >  {
> > -       fixed_shutter_ = fixed_shutter.get<std::micro>();
> > +       fixed_shutter_ = fixed_shutter;
> >         // Set this in case someone calls Pause() straight after.
> >         status_.shutter_time = clipShutter(fixed_shutter_);
> >  }
> > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
> >  {
> >         housekeepConfig();
> >
> > -       double fixed_shutter = clipShutter(fixed_shutter_);
> > -       if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> > +       Duration fixed_shutter = clipShutter(fixed_shutter_);
> > +       if (fixed_shutter && fixed_analogue_gain_) {
> >                 // We're going to reset the algorithm here with these fixed values.
> >
> >                 fetchAwbStatus(metadata);
> > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)
> >                 // Process has run, so we have meaningful values.
> >                 DeviceStatus device_status;
> >                 if (image_metadata->Get("device.status", device_status) == 0) {
> > -                       double actual_exposure = device_status.shutter_speed *
> > -                                                device_status.analogue_gain;
> > +                       Duration actual_exposure = device_status.shutter_speed *
> > +                                                  device_status.analogue_gain;
> >                         if (actual_exposure) {
> >                                 status_.digital_gain =
> >                                         status_.total_exposure_value /
> > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)
> >                                         std::min(status_.digital_gain, 4.0));
> >                                 LOG(RPiAgc, Debug) << "Actual exposure " << actual_exposure;
> >                                 LOG(RPiAgc, Debug) << "Use digital_gain " << status_.digital_gain;
> > -                               LOG(RPiAgc, Debug) << "Effective exposure " << actual_exposure * status_.digital_gain;
> > +                               LOG(RPiAgc, Debug) << "Effective exposure "
> > +                                                  << actual_exposure * status_.digital_gain;
> >                                 // Decide whether AEC/AGC has converged.
> >                                 updateLockStatus(device_status);
> >                         }
> > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const &device_status)
> >         const double RESET_MARGIN = 1.5;
> >
> >         // Add 200us to the exposure time error to allow for line quantisation.
> > -       double exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200;
> > +       Duration exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200us;
> >         double gain_error = last_device_status_.analogue_gain * ERROR_FACTOR;
> > -       double target_error = last_target_exposure_ * ERROR_FACTOR;
> > +       Duration target_error = last_target_exposure_ * ERROR_FACTOR;
> >
> >         // Note that we don't know the exposure/gain limits of the sensor, so
> >         // the values we keep requesting may be unachievable. For this reason
> > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata *image_metadata)
> >         current_.analogue_gain = device_status->analogue_gain;
> >         AgcStatus *agc_status =
> >                 image_metadata->GetLocked<AgcStatus>("agc.status");
> > -       current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0;
> > +       current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0s;
> >         current_.total_exposure_no_dg = current_.shutter * current_.analogue_gain;
> >  }
> >
> > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *image_metadata,
> >
> >  void Agc::computeTargetExposure(double gain)
> >  {
> > -       if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain != 0.0) {
> > +       if (status_.fixed_shutter && status_.fixed_analogue_gain) {
> >                 // When ag and shutter are both fixed, we need to drive the
> >                 // total exposure so that we end up with a digital gain of at least
> >                 // 1/min_colour_gain. Otherwise we'd desaturate channels causing
> > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)
> >                 target_.total_exposure = current_.total_exposure_no_dg * gain;
> >                 // The final target exposure is also limited to what the exposure
> >                 // mode allows.
> > -               double max_shutter = status_.fixed_shutter != 0.0
> > +               Duration max_shutter = status_.fixed_shutter
> >                                    ? status_.fixed_shutter
> >                                    : exposure_mode_->shutter.back();
> >                 max_shutter = clipShutter(max_shutter);
> > -               double max_total_exposure =
> > +               Duration max_total_exposure =
> >                         max_shutter *
> >                         (status_.fixed_analogue_gain != 0.0
> >                                  ? status_.fixed_analogue_gain
> > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)
> >         if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
> >             frame_count_ <= config_.startup_frames)
> >                 speed = 1.0;
> > -       if (filtered_.total_exposure == 0.0) {
> > +       if (!filtered_.total_exposure) {
> >                 filtered_.total_exposure = target_.total_exposure;
> >                 filtered_.total_exposure_no_dg = target_.total_exposure_no_dg;
> >         } else {
> > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()
> >         // Sending the fixed shutter/gain cases through the same code may seem
> >         // unnecessary, but it will make more sense when extend this to cover
> >         // variable aperture.
> > -       double exposure_value = filtered_.total_exposure_no_dg;
> > -       double shutter_time, analogue_gain;
> > -       shutter_time = status_.fixed_shutter != 0.0
> > +       Duration exposure_value = filtered_.total_exposure_no_dg;
> > +       Duration shutter_time;
> > +       double analogue_gain;
> > +       shutter_time = status_.fixed_shutter
> >                                ? status_.fixed_shutter
> >                                : exposure_mode_->shutter[0];
> >         shutter_time = clipShutter(shutter_time);
> > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()
> >         if (shutter_time * analogue_gain < exposure_value) {
> >                 for (unsigned int stage = 1;
> >                      stage < exposure_mode_->gain.size(); stage++) {
> > -                       if (status_.fixed_shutter == 0.0) {
> > -                               double stage_shutter =
> > +                       if (!status_.fixed_shutter) {
> > +                               Duration stage_shutter =
> >                                         clipShutter(exposure_mode_->shutter[stage]);
> >                                 if (stage_shutter * analogue_gain >=
> >                                     exposure_value) {
> > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()
> >                            << analogue_gain;
> >         // Finally adjust shutter time for flicker avoidance (require both
> >         // shutter and gain not to be fixed).
> > -       if (status_.fixed_shutter == 0.0 &&
> > -           status_.fixed_analogue_gain == 0.0 &&
> > -           status_.flicker_period != 0.0) {
> > -               int flicker_periods = shutter_time / status_.flicker_period;
> > -               if (flicker_periods > 0) {
> > -                       double new_shutter_time = flicker_periods * status_.flicker_period;
> > +       if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
> > +           status_.flicker_period) {
> > +               double flicker_periods = shutter_time / status_.flicker_period;
> > +               if (flicker_periods > 0.0) {
> > +                       Duration new_shutter_time = flicker_periods * status_.flicker_period;
>
> Just wondering if this will behave the same now that flicker_periods
> is a double. Won't it now hold fractional values with the result that
> new_shutter_time isn't an exact multiple of the period?
>
> >                         analogue_gain *= shutter_time / new_shutter_time;
> >                         // We should still not allow the ag to go over the
> >                         // largest value in the exposure mode. Note that this
> > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()
> >  void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
> >  {
> >         status_.total_exposure_value = filtered_.total_exposure;
> > -       status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg;
> > +       status_.target_exposure_value = desaturate ? 0s : target_.total_exposure_no_dg;
> >         status_.shutter_time = filtered_.shutter;
> >         status_.analogue_gain = filtered_.analogue_gain;
> >         // Write to metadata as well, in case anyone wants to update the camera
> > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
> >                            << " analogue gain " << filtered_.analogue_gain;
> >  }
> >
> > -double Agc::clipShutter(double shutter)
> > +Duration Agc::clipShutter(const Duration &shutter)
> >  {
> > +       Duration clip_shutter = shutter;
>
> Given that we copy it anyway I wonder if we might have left this as
> call-by-value, resulting in fewer changes. But it's only the teeniest
> of nit-picks!
>
> If you can put my mind at rest over the flicker period thing:
>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>
> Thanks!
> David
>
> >         if (max_shutter_)
> > -               shutter = std::min(shutter, max_shutter_);
> > -       return shutter;
> > +               clip_shutter = std::min(clip_shutter, max_shutter_);
> > +       return clip_shutter;
> >  }
> >
> >  // Register algorithm with the system.
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 16a65959d90e..24a6610096c2 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -9,6 +9,8 @@
> >  #include <vector>
> >  #include <mutex>
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  #include "../agc_algorithm.hpp"
> >  #include "../agc_status.h"
> >  #include "../pwl.hpp"
> > @@ -22,13 +24,15 @@
> >
> >  namespace RPiController {
> >
> > +using namespace std::literals::chrono_literals;
> > +
> >  struct AgcMeteringMode {
> >         double weights[AGC_STATS_SIZE];
> >         void Read(boost::property_tree::ptree const &params);
> >  };
> >
> >  struct AgcExposureMode {
> > -       std::vector<double> shutter;
> > +       std::vector<Duration> shutter;
> >         std::vector<double> gain;
> >         void Read(boost::property_tree::ptree const &params);
> >  };
> > @@ -61,7 +65,7 @@ struct AgcConfig {
> >         std::string default_exposure_mode;
> >         std::string default_constraint_mode;
> >         double base_ev;
> > -       double default_exposure_time;
> > +       Duration default_exposure_time;
> >         double default_analogue_gain;
> >  };
> >
> > @@ -101,19 +105,19 @@ private:
> >         void filterExposure(bool desaturate);
> >         void divideUpExposure();
> >         void writeAndFinish(Metadata *image_metadata, bool desaturate);
> > -       double clipShutter(double shutter);
> > +       Duration clipShutter(const Duration &shutter);
> >         AgcMeteringMode *metering_mode_;
> >         AgcExposureMode *exposure_mode_;
> >         AgcConstraintMode *constraint_mode_;
> >         uint64_t frame_count_;
> >         AwbStatus awb_;
> >         struct ExposureValues {
> > -               ExposureValues() : shutter(0), analogue_gain(0),
> > -                                  total_exposure(0), total_exposure_no_dg(0) {}
> > -               double shutter;
> > +               ExposureValues() : shutter(0s), analogue_gain(0),
> > +                                  total_exposure(0s), total_exposure_no_dg(0s) {}
> > +               Duration shutter;
> >                 double analogue_gain;
> > -               double total_exposure;
> > -               double total_exposure_no_dg; // without digital gain
> > +               Duration total_exposure;
> > +               Duration total_exposure_no_dg; // without digital gain
> >         };
> >         ExposureValues current_;  // values for the current frame
> >         ExposureValues target_;   // calculate the values we want here
> > @@ -121,15 +125,15 @@ private:
> >         AgcStatus status_;
> >         int lock_count_;
> >         DeviceStatus last_device_status_;
> > -       double last_target_exposure_;
> > +       Duration last_target_exposure_;
> >         // Below here the "settings" that applications can change.
> >         std::string metering_mode_name_;
> >         std::string exposure_mode_name_;
> >         std::string constraint_mode_name_;
> >         double ev_;
> > -       double flicker_period_;
> > -       double max_shutter_;
> > -       double fixed_shutter_;
> > +       Duration flicker_period_;
> > +       Duration max_shutter_;
> > +       Duration fixed_shutter_;
> >         double fixed_analogue_gain_;
> >  };
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > index f74381cab2b4..0fa6457c1376 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > @@ -16,6 +16,7 @@
> >
> >  using namespace RPiController;
> >  using namespace libcamera;
> > +using namespace std::literals::chrono_literals;
> >
> >  LOG_DEFINE_CATEGORY(RPiLux)
> >
> > @@ -38,7 +39,7 @@ char const *Lux::Name() const
> >  void Lux::Read(boost::property_tree::ptree const &params)
> >  {
> >         reference_shutter_speed_ =
> > -               params.get<double>("reference_shutter_speed");
> > +               params.get<double>("reference_shutter_speed") * 1.0us;
> >         reference_gain_ = params.get<double>("reference_gain");
> >         reference_aperture_ = params.get<double>("reference_aperture", 1.0);
> >         reference_Y_ = params.get<double>("reference_Y");
> > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
> >  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> >  {
> >         // set some initial values to shut the compiler up
> > -       DeviceStatus device_status =
> > -               { .shutter_speed = 1.0,
> > -                 .analogue_gain = 1.0,
> > -                 .lens_position = 0.0,
> > -                 .aperture = 0.0,
> > -                 .flash_intensity = 0.0 };
> > +       DeviceStatus device_status = { .shutter_speed = 1.0ms,
> > +                                      .analogue_gain = 1.0,
> > +                                      .lens_position = 0.0,
> > +                                      .aperture = 0.0,
> > +                                      .flash_intensity = 0.0 };
> >         if (image_metadata->Get("device.status", device_status) == 0) {
> >                 double current_gain = device_status.analogue_gain;
> > -               double current_shutter_speed = device_status.shutter_speed;
> >                 double current_aperture = device_status.aperture;
> >                 if (current_aperture == 0)
> >                         current_aperture = current_aperture_;
> > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> >                 double current_Y = sum / (double)num + .5;
> >                 double gain_ratio = reference_gain_ / current_gain;
> >                 double shutter_speed_ratio =
> > -                       reference_shutter_speed_ / current_shutter_speed;
> > +                       reference_shutter_speed_ / device_status.shutter_speed;
> >                 double aperture_ratio = reference_aperture_ / current_aperture;
> >                 double Y_ratio = current_Y * (65536 / num_bins) / reference_Y_;
> >                 double estimated_lux = shutter_speed_ratio * gain_ratio *
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > index f9090484a136..45c844393e62 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > @@ -8,6 +8,8 @@
> >
> >  #include <mutex>
> >
> > +#include "libcamera/internal/utils.h"
> > +
> >  #include "../lux_status.h"
> >  #include "../algorithm.hpp"
> >
> > @@ -28,7 +30,7 @@ public:
> >  private:
> >         // These values define the conditions of the reference image, against
> >         // which we compare the new image.
> > -       double reference_shutter_speed_; // in micro-seconds
> > +       libcamera::utils::Duration reference_shutter_speed_;
> >         double reference_gain_;
> >         double reference_aperture_; // units of 1/f
> >         double reference_Y_; // out of 65536
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f8f36420b076..b77230ded591 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
> >
> >         /* SwitchMode may supply updated exposure/gain values to use. */
> >         AgcStatus agcStatus;
> > -       agcStatus.shutter_time = 0.0;
> > +       agcStatus.shutter_time = 0.0s;
> >         agcStatus.analogue_gain = 0.0;
> >
> >         metadata.Get("agc.status", agcStatus);
> > -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > +       if (agcStatus.shutter_time && agcStatus.analogue_gain) {
> >                 ControlList ctrls(sensorCtrls_);
> >                 applyAGC(&agcStatus, ctrls);
> >                 startConfig->controls = std::move(ctrls);
> > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >                 /* Supply initial values for gain and exposure. */
> >                 ControlList ctrls(sensorCtrls_);
> >                 AgcStatus agcStatus;
> > -               agcStatus.shutter_time = DefaultExposureTime.get<std::micro>();
> > +               agcStatus.shutter_time = DefaultExposureTime;
> >                 agcStatus.analogue_gain = DefaultAnalogueGain;
> >                 applyAGC(&agcStatus, ctrls);
> >
> > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()
> >          */
> >         DeviceStatus *deviceStatus = rpiMetadata_.GetLocked<DeviceStatus>("device.status");
> >         if (deviceStatus) {
> > -               libcameraMetadata_.set(controls::ExposureTime, deviceStatus->shutter_speed);
> > +               libcameraMetadata_.set(controls::ExposureTime,
> > +                                      deviceStatus->shutter_speed.get<std::micro>());
> >                 libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain);
> >         }
> >
> > @@ -1017,7 +1018,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).get<std::micro>();
> > +       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> >         deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >
> >         LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > @@ -1103,7 +1104,7 @@ 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. */
> > -       Duration exposure = agcStatus->shutter_time * 1.0us;
> > +       Duration exposure = agcStatus->shutter_time;
> >         int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);
> >         int32_t exposureLines = helper_->ExposureLines(exposure);
> >
> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > index 3131aa2d6666..5fa6f8366f3d 100644
> > --- a/src/libcamera/utils.cpp
> > +++ b/src/libcamera/utils.cpp
> > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()
> >   * loop, iterates over an indexed view of the \a iterable
> >   */
> >
> > +/**
> > + * \typedef BaseDuration
> > + * \brief Base struct for the \a Duration helper class.
> > + */
> > +
> >  /**
> >   * \class Duration
> > - * \brief Helper for std::chrono::duration types. Derived from \a BaseDuration
> > + * \brief Helper class for std::chrono::duration types.
> >   */
> >
> >  /**
> > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()
> >   * \param[in] d The std::chrono::duration object to convert from.
> >   *
> >   * Constructs a \a Duration object from a \a std::chrono::duration object,
> > - * converting the representation to \a BaseDuration type.
> > + * internally converting the representation to \a BaseDuration type.
> >   */
> >
> >  /**
> > --
> > 2.25.1
> >

On Fri, 21 May 2021 at 13:37, Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hello,
>
> On Fri, May 21, 2021 at 12:18:55PM +0100, David Plowman wrote:
> > Hi Naush
> >
> > Thanks for all this work!
> >
> > On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com>
> wrote:
> > >
> > > Convert the core AGC and Lux controller code to use
> > > utils::Duration for all exposure time related variables and
> > > calculations.
> > >
> > > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
> > > to use utils::Duration.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
> > >  src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
> > >  .../raspberrypi/controller/device_status.h    |  6 +-
> > >  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
> > >  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
> > >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
> > >  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
> > >  src/libcamera/utils.cpp                       |  9 +-
> > >  9 files changed, 105 insertions(+), 77 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > > index feb02d1806b2..968fae13bd5e 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> > >                 return;
> > >         }
> > >
> > > -       deviceStatus.shutter_speed =
> Exposure(exposureLines).get<std::micro>();
> > > +       deviceStatus.shutter_speed = Exposure(exposureLines);
> > >         deviceStatus.analogue_gain = Gain(gainCode);
> > >
> > >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > > diff --git a/src/ipa/raspberrypi/controller/agc_status.h
> b/src/ipa/raspberrypi/controller/agc_status.h
> > > index 10381c90a313..c893715af588 100644
> > > --- a/src/ipa/raspberrypi/controller/agc_status.h
> > > +++ b/src/ipa/raspberrypi/controller/agc_status.h
> > > @@ -6,6 +6,8 @@
> > >   */
> > >  #pragma once
> > >
> > > +#include "libcamera/internal/utils.h"
> > > +
> > >  // The AGC algorithm should post the following structure into the
> image's
> > >  // "agc.status" metadata.
> > >
> > > @@ -17,18 +19,20 @@ extern "C" {
> > >  // seen statistics and calculated meaningful values. The contents
> should be
> > >  // ignored until then.
> > >
> > > +using libcamera::utils::Duration;
> > > +
> > >  struct AgcStatus {
> > > -       double total_exposure_value; // value for all exposure and
> gain for this image
> > > -       double target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
> > > -       double shutter_time;
> > > +       Duration total_exposure_value; // value for all exposure and
> gain for this image
> > > +       Duration target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
> > > +       Duration shutter_time;
> > >         double analogue_gain;
> > >         char exposure_mode[32];
> > >         char constraint_mode[32];
> > >         char metering_mode[32];
> > >         double ev;
> > > -       double flicker_period;
> > > +       Duration flicker_period;
> > >         int floating_region_enable;
> > > -       double fixed_shutter;
> > > +       Duration fixed_shutter;
> > >         double fixed_analogue_gain;
> > >         double digital_gain;
> > >         int locked;
> > > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> > > index aa08608b5d40..131b4cd344ee 100644
> > > --- a/src/ipa/raspberrypi/controller/device_status.h
> > > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > > @@ -6,6 +6,8 @@
> > >   */
> > >  #pragma once
> > >
> > > +#include "libcamera/internal/utils.h"
> > > +
> > >  // Definition of "device metadata" which stores things like shutter
> time and
> > >  // analogue gain that downstream control algorithms will want to know.
> > >
> > > @@ -14,8 +16,8 @@ extern "C" {
> > >  #endif
> > >
> > >  struct DeviceStatus {
> > > -       // time shutter is open, in microseconds
> > > -       double shutter_speed;
> > > +       // time shutter is open
> > > +       libcamera::utils::Duration shutter_speed;
> > >         double analogue_gain;
> > >         // 1.0/distance-in-metres, or 0 if unknown
> > >         double lens_position;
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > index 482eb3ef962d..46742d845034 100644
> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > @@ -21,6 +21,7 @@
> > >
> > >  using namespace RPiController;
> > >  using namespace libcamera;
> > > +using utils::operator<<;
> >
> > Still can't say that I like this... just because someone wants to use
> > Durations, it seems like an awkward thing to make them remember. But
> > unless anyone has anything better....
>
> I got the same feeling while reading the rest of the patches.
> I can't say toString() is that better though, but probably is less
> implicit that this using statment, which if read in isolation does not
> immediately relates to the Duration class ?
>

I do agree with these points, and I don't like this as well.  I can see two
alternatives without
using toString():

1) Move the Duration class into its own namespace under utils, so then the
above statement
will look a bit more informative:

using utils::Duration::operator<<;

2) Move the operator() out of the utils namespace.  This way we eliminate
the need for the
statement completely.

Regards,
Naush


>
> >
> > >
> > >  LOG_DEFINE_CATEGORY(RPiAgc)
> > >
> > > @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string,
> AgcMeteringMode> &metering_modes,
> > >         return first;
> > >  }
> > >
> > > -static int read_double_list(std::vector<double> &list,
> > > -                           boost::property_tree::ptree const &params)
> > > +static int read_list(std::vector<double> &list,
> > > +                    boost::property_tree::ptree const &params)
> > >  {
> > >         for (auto &p : params)
> > >                 list.push_back(p.second.get_value<double>());
> > >         return list.size();
> > >  }
> > >
> > > +static int read_list(std::vector<Duration> &list,
> > > +                    boost::property_tree::ptree const &params)
> > > +{
> > > +       for (auto &p : params)
> > > +               list.push_back(p.second.get_value<double>() * 1us);
> > > +       return list.size();
> > > +}
> > > +
> > >  void AgcExposureMode::Read(boost::property_tree::ptree const &params)
> > >  {
> > >         int num_shutters =
> > > -               read_double_list(shutter, params.get_child("shutter"));
> > > -       int num_ags = read_double_list(gain, params.get_child("gain"));
> > > +               read_list(shutter, params.get_child("shutter"));
> > > +       int num_ags = read_list(gain, params.get_child("gain"));
> > >         if (num_shutters < 2 || num_ags < 2)
> > >                 throw std::runtime_error(
> > >                         "AgcConfig: must have at least two entries in
> exposure profile");
> > > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree
> const &params)
> > >                 params.get<double>("fast_reduce_threshold", 0.4);
> > >         base_ev = params.get<double>("base_ev", 1.0);
> > >         // Start with quite a low value as ramping up is easier than
> ramping down.
> > > -       default_exposure_time =
> params.get<double>("default_exposure_time", 1000);
> > > +       default_exposure_time =
> params.get<double>("default_exposure_time", 1000) * 1us;
> > >         default_analogue_gain =
> params.get<double>("default_analogue_gain", 1.0);
> > >  }
> > >
> > > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)
> > >         : AgcAlgorithm(controller), metering_mode_(nullptr),
> > >           exposure_mode_(nullptr), constraint_mode_(nullptr),
> > >           frame_count_(0), lock_count_(0),
> > > -         last_target_exposure_(0.0),
> > > -         ev_(1.0), flicker_period_(0.0),
> > > -         max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> > > +         last_target_exposure_(0s),
> > > +         ev_(1.0), flicker_period_(0s),
> > > +         max_shutter_(0s), fixed_shutter_(0s),
> fixed_analogue_gain_(0.0)
> > >  {
> > >         memset(&awb_, 0, sizeof(awb_));
> > >         // Setting status_.total_exposure_value_ to zero initially
> tells us
> > > @@ -203,7 +212,7 @@ void Agc::Pause()
> > >
> > >  void Agc::Resume()
> > >  {
> > > -       fixed_shutter_ = 0;
> > > +       fixed_shutter_ = 0s;
> > >         fixed_analogue_gain_ = 0;
> > >  }
> > >
> > > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)
> > >
> > >  void Agc::SetFlickerPeriod(const Duration &flicker_period)
> > >  {
> > > -       flicker_period_ = flicker_period.get<std::micro>();
> > > +       flicker_period_ = flicker_period;
> > >  }
> > >
> > >  void Agc::SetMaxShutter(const Duration &max_shutter)
> > >  {
> > > -       max_shutter_ = max_shutter.get<std::micro>();
> > > +       max_shutter_ = max_shutter;
> > >  }
> > >
> > >  void Agc::SetFixedShutter(const Duration &fixed_shutter)
> > >  {
> > > -       fixed_shutter_ = fixed_shutter.get<std::micro>();
> > > +       fixed_shutter_ = fixed_shutter;
> > >         // Set this in case someone calls Pause() straight after.
> > >         status_.shutter_time = clipShutter(fixed_shutter_);
> > >  }
> > > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
> const &camera_mode,
> > >  {
> > >         housekeepConfig();
> > >
> > > -       double fixed_shutter = clipShutter(fixed_shutter_);
> > > -       if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> > > +       Duration fixed_shutter = clipShutter(fixed_shutter_);
> > > +       if (fixed_shutter && fixed_analogue_gain_) {
> > >                 // We're going to reset the algorithm here with these
> fixed values.
> > >
> > >                 fetchAwbStatus(metadata);
> > > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)
> > >                 // Process has run, so we have meaningful values.
> > >                 DeviceStatus device_status;
> > >                 if (image_metadata->Get("device.status",
> device_status) == 0) {
> > > -                       double actual_exposure =
> device_status.shutter_speed *
> > > -
> device_status.analogue_gain;
> > > +                       Duration actual_exposure =
> device_status.shutter_speed *
> > > +
> device_status.analogue_gain;
> > >                         if (actual_exposure) {
> > >                                 status_.digital_gain =
> > >                                         status_.total_exposure_value /
> > > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)
> > >                                         std::min(status_.digital_gain,
> 4.0));
> > >                                 LOG(RPiAgc, Debug) << "Actual exposure
> " << actual_exposure;
> > >                                 LOG(RPiAgc, Debug) << "Use
> digital_gain " << status_.digital_gain;
> > > -                               LOG(RPiAgc, Debug) << "Effective
> exposure " << actual_exposure * status_.digital_gain;
> > > +                               LOG(RPiAgc, Debug) << "Effective
> exposure "
> > > +                                                  << actual_exposure
> * status_.digital_gain;
> > >                                 // Decide whether AEC/AGC has
> converged.
> > >                                 updateLockStatus(device_status);
> > >                         }
> > > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const
> &device_status)
> > >         const double RESET_MARGIN = 1.5;
> > >
> > >         // Add 200us to the exposure time error to allow for line
> quantisation.
> > > -       double exposure_error = last_device_status_.shutter_speed *
> ERROR_FACTOR + 200;
> > > +       Duration exposure_error = last_device_status_.shutter_speed *
> ERROR_FACTOR + 200us;
> > >         double gain_error = last_device_status_.analogue_gain *
> ERROR_FACTOR;
> > > -       double target_error = last_target_exposure_ * ERROR_FACTOR;
> > > +       Duration target_error = last_target_exposure_ * ERROR_FACTOR;
> > >
> > >         // Note that we don't know the exposure/gain limits of the
> sensor, so
> > >         // the values we keep requesting may be unachievable. For this
> reason
> > > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata
> *image_metadata)
> > >         current_.analogue_gain = device_status->analogue_gain;
> > >         AgcStatus *agc_status =
> > >                 image_metadata->GetLocked<AgcStatus>("agc.status");
> > > -       current_.total_exposure = agc_status ?
> agc_status->total_exposure_value : 0;
> > > +       current_.total_exposure = agc_status ?
> agc_status->total_exposure_value : 0s;
> > >         current_.total_exposure_no_dg = current_.shutter *
> current_.analogue_gain;
> > >  }
> > >
> > > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats
> *statistics, Metadata *image_metadata,
> > >
> > >  void Agc::computeTargetExposure(double gain)
> > >  {
> > > -       if (status_.fixed_shutter != 0.0 &&
> status_.fixed_analogue_gain != 0.0) {
> > > +       if (status_.fixed_shutter && status_.fixed_analogue_gain) {
> > >                 // When ag and shutter are both fixed, we need to
> drive the
> > >                 // total exposure so that we end up with a digital
> gain of at least
> > >                 // 1/min_colour_gain. Otherwise we'd desaturate
> channels causing
> > > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)
> > >                 target_.total_exposure = current_.total_exposure_no_dg
> * gain;
> > >                 // The final target exposure is also limited to what
> the exposure
> > >                 // mode allows.
> > > -               double max_shutter = status_.fixed_shutter != 0.0
> > > +               Duration max_shutter = status_.fixed_shutter
> > >                                    ? status_.fixed_shutter
> > >                                    : exposure_mode_->shutter.back();
> > >                 max_shutter = clipShutter(max_shutter);
> > > -               double max_total_exposure =
> > > +               Duration max_total_exposure =
> > >                         max_shutter *
> > >                         (status_.fixed_analogue_gain != 0.0
> > >                                  ? status_.fixed_analogue_gain
> > > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)
> > >         if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
> > >             frame_count_ <= config_.startup_frames)
> > >                 speed = 1.0;
> > > -       if (filtered_.total_exposure == 0.0) {
> > > +       if (!filtered_.total_exposure) {
> > >                 filtered_.total_exposure = target_.total_exposure;
> > >                 filtered_.total_exposure_no_dg =
> target_.total_exposure_no_dg;
> > >         } else {
> > > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()
> > >         // Sending the fixed shutter/gain cases through the same code
> may seem
> > >         // unnecessary, but it will make more sense when extend this
> to cover
> > >         // variable aperture.
> > > -       double exposure_value = filtered_.total_exposure_no_dg;
> > > -       double shutter_time, analogue_gain;
> > > -       shutter_time = status_.fixed_shutter != 0.0
> > > +       Duration exposure_value = filtered_.total_exposure_no_dg;
> > > +       Duration shutter_time;
> > > +       double analogue_gain;
> > > +       shutter_time = status_.fixed_shutter
> > >                                ? status_.fixed_shutter
> > >                                : exposure_mode_->shutter[0];
> > >         shutter_time = clipShutter(shutter_time);
> > > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()
> > >         if (shutter_time * analogue_gain < exposure_value) {
> > >                 for (unsigned int stage = 1;
> > >                      stage < exposure_mode_->gain.size(); stage++) {
> > > -                       if (status_.fixed_shutter == 0.0) {
> > > -                               double stage_shutter =
> > > +                       if (!status_.fixed_shutter) {
> > > +                               Duration stage_shutter =
> > >
>  clipShutter(exposure_mode_->shutter[stage]);
> > >                                 if (stage_shutter * analogue_gain >=
> > >                                     exposure_value) {
> > > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()
> > >                            << analogue_gain;
> > >         // Finally adjust shutter time for flicker avoidance (require
> both
> > >         // shutter and gain not to be fixed).
> > > -       if (status_.fixed_shutter == 0.0 &&
> > > -           status_.fixed_analogue_gain == 0.0 &&
> > > -           status_.flicker_period != 0.0) {
> > > -               int flicker_periods = shutter_time /
> status_.flicker_period;
> > > -               if (flicker_periods > 0) {
> > > -                       double new_shutter_time = flicker_periods *
> status_.flicker_period;
> > > +       if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
> > > +           status_.flicker_period) {
> > > +               double flicker_periods = shutter_time /
> status_.flicker_period;
> > > +               if (flicker_periods > 0.0) {
> > > +                       Duration new_shutter_time = flicker_periods *
> status_.flicker_period;
> >
> > Just wondering if this will behave the same now that flicker_periods
> > is a double. Won't it now hold fractional values with the result that
> > new_shutter_time isn't an exact multiple of the period?
> >
> > >                         analogue_gain *= shutter_time /
> new_shutter_time;
> > >                         // We should still not allow the ag to go over
> the
> > >                         // largest value in the exposure mode. Note
> that this
> > > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()
> > >  void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
> > >  {
> > >         status_.total_exposure_value = filtered_.total_exposure;
> > > -       status_.target_exposure_value = desaturate ? 0 :
> target_.total_exposure_no_dg;
> > > +       status_.target_exposure_value = desaturate ? 0s :
> target_.total_exposure_no_dg;
> > >         status_.shutter_time = filtered_.shutter;
> > >         status_.analogue_gain = filtered_.analogue_gain;
> > >         // Write to metadata as well, in case anyone wants to update
> the camera
> > > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata
> *image_metadata, bool desaturate)
> > >                            << " analogue gain " <<
> filtered_.analogue_gain;
> > >  }
> > >
> > > -double Agc::clipShutter(double shutter)
> > > +Duration Agc::clipShutter(const Duration &shutter)
> > >  {
> > > +       Duration clip_shutter = shutter;
> >
> > Given that we copy it anyway I wonder if we might have left this as
> > call-by-value, resulting in fewer changes. But it's only the teeniest
> > of nit-picks!
> >
> > If you can put my mind at rest over the flicker period thing:
> >
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> >
> > Thanks!
> > David
> >
> > >         if (max_shutter_)
> > > -               shutter = std::min(shutter, max_shutter_);
> > > -       return shutter;
> > > +               clip_shutter = std::min(clip_shutter, max_shutter_);
> > > +       return clip_shutter;
> > >  }
> > >
> > >  // Register algorithm with the system.
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > index 16a65959d90e..24a6610096c2 100644
> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > @@ -9,6 +9,8 @@
> > >  #include <vector>
> > >  #include <mutex>
> > >
> > > +#include "libcamera/internal/utils.h"
> > > +
> > >  #include "../agc_algorithm.hpp"
> > >  #include "../agc_status.h"
> > >  #include "../pwl.hpp"
> > > @@ -22,13 +24,15 @@
> > >
> > >  namespace RPiController {
> > >
> > > +using namespace std::literals::chrono_literals;
> > > +
> > >  struct AgcMeteringMode {
> > >         double weights[AGC_STATS_SIZE];
> > >         void Read(boost::property_tree::ptree const &params);
> > >  };
> > >
> > >  struct AgcExposureMode {
> > > -       std::vector<double> shutter;
> > > +       std::vector<Duration> shutter;
> > >         std::vector<double> gain;
> > >         void Read(boost::property_tree::ptree const &params);
> > >  };
> > > @@ -61,7 +65,7 @@ struct AgcConfig {
> > >         std::string default_exposure_mode;
> > >         std::string default_constraint_mode;
> > >         double base_ev;
> > > -       double default_exposure_time;
> > > +       Duration default_exposure_time;
> > >         double default_analogue_gain;
> > >  };
> > >
> > > @@ -101,19 +105,19 @@ private:
> > >         void filterExposure(bool desaturate);
> > >         void divideUpExposure();
> > >         void writeAndFinish(Metadata *image_metadata, bool desaturate);
> > > -       double clipShutter(double shutter);
> > > +       Duration clipShutter(const Duration &shutter);
> > >         AgcMeteringMode *metering_mode_;
> > >         AgcExposureMode *exposure_mode_;
> > >         AgcConstraintMode *constraint_mode_;
> > >         uint64_t frame_count_;
> > >         AwbStatus awb_;
> > >         struct ExposureValues {
> > > -               ExposureValues() : shutter(0), analogue_gain(0),
> > > -                                  total_exposure(0),
> total_exposure_no_dg(0) {}
> > > -               double shutter;
> > > +               ExposureValues() : shutter(0s), analogue_gain(0),
> > > +                                  total_exposure(0s),
> total_exposure_no_dg(0s) {}
> > > +               Duration shutter;
> > >                 double analogue_gain;
> > > -               double total_exposure;
> > > -               double total_exposure_no_dg; // without digital gain
> > > +               Duration total_exposure;
> > > +               Duration total_exposure_no_dg; // without digital gain
> > >         };
> > >         ExposureValues current_;  // values for the current frame
> > >         ExposureValues target_;   // calculate the values we want here
> > > @@ -121,15 +125,15 @@ private:
> > >         AgcStatus status_;
> > >         int lock_count_;
> > >         DeviceStatus last_device_status_;
> > > -       double last_target_exposure_;
> > > +       Duration last_target_exposure_;
> > >         // Below here the "settings" that applications can change.
> > >         std::string metering_mode_name_;
> > >         std::string exposure_mode_name_;
> > >         std::string constraint_mode_name_;
> > >         double ev_;
> > > -       double flicker_period_;
> > > -       double max_shutter_;
> > > -       double fixed_shutter_;
> > > +       Duration flicker_period_;
> > > +       Duration max_shutter_;
> > > +       Duration fixed_shutter_;
> > >         double fixed_analogue_gain_;
> > >  };
> > >
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > index f74381cab2b4..0fa6457c1376 100644
> > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > @@ -16,6 +16,7 @@
> > >
> > >  using namespace RPiController;
> > >  using namespace libcamera;
> > > +using namespace std::literals::chrono_literals;
> > >
> > >  LOG_DEFINE_CATEGORY(RPiLux)
> > >
> > > @@ -38,7 +39,7 @@ char const *Lux::Name() const
> > >  void Lux::Read(boost::property_tree::ptree const &params)
> > >  {
> > >         reference_shutter_speed_ =
> > > -               params.get<double>("reference_shutter_speed");
> > > +               params.get<double>("reference_shutter_speed") * 1.0us;
> > >         reference_gain_ = params.get<double>("reference_gain");
> > >         reference_aperture_ = params.get<double>("reference_aperture",
> 1.0);
> > >         reference_Y_ = params.get<double>("reference_Y");
> > > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
> > >  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> > >  {
> > >         // set some initial values to shut the compiler up
> > > -       DeviceStatus device_status =
> > > -               { .shutter_speed = 1.0,
> > > -                 .analogue_gain = 1.0,
> > > -                 .lens_position = 0.0,
> > > -                 .aperture = 0.0,
> > > -                 .flash_intensity = 0.0 };
> > > +       DeviceStatus device_status = { .shutter_speed = 1.0ms,
> > > +                                      .analogue_gain = 1.0,
> > > +                                      .lens_position = 0.0,
> > > +                                      .aperture = 0.0,
> > > +                                      .flash_intensity = 0.0 };
> > >         if (image_metadata->Get("device.status", device_status) == 0) {
> > >                 double current_gain = device_status.analogue_gain;
> > > -               double current_shutter_speed =
> device_status.shutter_speed;
> > >                 double current_aperture = device_status.aperture;
> > >                 if (current_aperture == 0)
> > >                         current_aperture = current_aperture_;
> > > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata
> *image_metadata)
> > >                 double current_Y = sum / (double)num + .5;
> > >                 double gain_ratio = reference_gain_ / current_gain;
> > >                 double shutter_speed_ratio =
> > > -                       reference_shutter_speed_ /
> current_shutter_speed;
> > > +                       reference_shutter_speed_ /
> device_status.shutter_speed;
> > >                 double aperture_ratio = reference_aperture_ /
> current_aperture;
> > >                 double Y_ratio = current_Y * (65536 / num_bins) /
> reference_Y_;
> > >                 double estimated_lux = shutter_speed_ratio *
> gain_ratio *
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > > index f9090484a136..45c844393e62 100644
> > > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > > @@ -8,6 +8,8 @@
> > >
> > >  #include <mutex>
> > >
> > > +#include "libcamera/internal/utils.h"
> > > +
> > >  #include "../lux_status.h"
> > >  #include "../algorithm.hpp"
> > >
> > > @@ -28,7 +30,7 @@ public:
> > >  private:
> > >         // These values define the conditions of the reference image,
> against
> > >         // which we compare the new image.
> > > -       double reference_shutter_speed_; // in micro-seconds
> > > +       libcamera::utils::Duration reference_shutter_speed_;
> > >         double reference_gain_;
> > >         double reference_aperture_; // units of 1/f
> > >         double reference_Y_; // out of 65536
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index f8f36420b076..b77230ded591 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,
> ipa::RPi::StartConfig *startConf
> > >
> > >         /* SwitchMode may supply updated exposure/gain values to use.
> */
> > >         AgcStatus agcStatus;
> > > -       agcStatus.shutter_time = 0.0;
> > > +       agcStatus.shutter_time = 0.0s;
> > >         agcStatus.analogue_gain = 0.0;
> > >
> > >         metadata.Get("agc.status", agcStatus);
> > > -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain
> != 0.0) {
> > > +       if (agcStatus.shutter_time && agcStatus.analogue_gain) {
> > >                 ControlList ctrls(sensorCtrls_);
> > >                 applyAGC(&agcStatus, ctrls);
> > >                 startConfig->controls = std::move(ctrls);
> > > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > >                 /* Supply initial values for gain and exposure. */
> > >                 ControlList ctrls(sensorCtrls_);
> > >                 AgcStatus agcStatus;
> > > -               agcStatus.shutter_time =
> DefaultExposureTime.get<std::micro>();
> > > +               agcStatus.shutter_time = DefaultExposureTime;
> > >                 agcStatus.analogue_gain = DefaultAnalogueGain;
> > >                 applyAGC(&agcStatus, ctrls);
> > >
> > > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()
> > >          */
> > >         DeviceStatus *deviceStatus =
> rpiMetadata_.GetLocked<DeviceStatus>("device.status");
> > >         if (deviceStatus) {
> > > -               libcameraMetadata_.set(controls::ExposureTime,
> deviceStatus->shutter_speed);
> > > +               libcameraMetadata_.set(controls::ExposureTime,
> > > +
> deviceStatus->shutter_speed.get<std::micro>());
> > >                 libcameraMetadata_.set(controls::AnalogueGain,
> deviceStatus->analogue_gain);
> > >         }
> > >
> > > @@ -1017,7 +1018,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).get<std::micro>();
> > > +       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > >         deviceStatus.analogue_gain = helper_->Gain(gainCode);
> > >
> > >         LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > > @@ -1103,7 +1104,7 @@ 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. */
> > > -       Duration exposure = agcStatus->shutter_time * 1.0us;
> > > +       Duration exposure = agcStatus->shutter_time;
> > >         int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
> > >         int32_t exposureLines = helper_->ExposureLines(exposure);
> > >
> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > > index 3131aa2d6666..5fa6f8366f3d 100644
> > > --- a/src/libcamera/utils.cpp
> > > +++ b/src/libcamera/utils.cpp
> > > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()
> > >   * loop, iterates over an indexed view of the \a iterable
> > >   */
> > >
> > > +/**
> > > + * \typedef BaseDuration
> > > + * \brief Base struct for the \a Duration helper class.
> > > + */
> > > +
> > >  /**
> > >   * \class Duration
> > > - * \brief Helper for std::chrono::duration types. Derived from \a
> BaseDuration
> > > + * \brief Helper class for std::chrono::duration types.
> > >   */
> > >
> > >  /**
> > > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()
> > >   * \param[in] d The std::chrono::duration object to convert from.
> > >   *
> > >   * Constructs a \a Duration object from a \a std::chrono::duration
> object,
> > > - * converting the representation to \a BaseDuration type.
> > > + * internally converting the representation to \a BaseDuration type.
> > >   */
> > >
> > >  /**
> > > --
> > > 2.25.1
> > >
>

Hi Naush,

On Fri, May 21, 2021 at 01:49:30PM +0100, Naushir Patuck wrote:
> On Fri, 21 May 2021 at 13:37, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> > Hello,
> >
> > On Fri, May 21, 2021 at 12:18:55PM +0100, David Plowman wrote:
> > > Hi Naush
> > >
> > > Thanks for all this work!
> > >
> > > On Fri, 21 May 2021 at 09:05, Naushir Patuck <naush@raspberrypi.com>
> > wrote:
> > > >
> > > > Convert the core AGC and Lux controller code to use
> > > > utils::Duration for all exposure time related variables and
> > > > calculations.
> > > >
> > > > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
> > > > to use utils::Duration.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > ---
> > > >  src/ipa/raspberrypi/cam_helper.cpp            |  2 +-
> > > >  src/ipa/raspberrypi/controller/agc_status.h   | 14 +--
> > > >  .../raspberrypi/controller/device_status.h    |  6 +-
> > > >  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 89 +++++++++++--------
> > > >  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 28 +++---
> > > >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 17 ++--
> > > >  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  4 +-
> > > >  src/ipa/raspberrypi/raspberrypi.cpp           | 13 +--
> > > >  src/libcamera/utils.cpp                       |  9 +-
> > > >  9 files changed, 105 insertions(+), 77 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> > b/src/ipa/raspberrypi/cam_helper.cpp
> > > > index feb02d1806b2..968fae13bd5e 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const
> > uint8_t> buffer,
> > > >                 return;
> > > >         }
> > > >
> > > > -       deviceStatus.shutter_speed =
> > Exposure(exposureLines).get<std::micro>();
> > > > +       deviceStatus.shutter_speed = Exposure(exposureLines);
> > > >         deviceStatus.analogue_gain = Gain(gainCode);
> > > >
> > > >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > > > diff --git a/src/ipa/raspberrypi/controller/agc_status.h
> > b/src/ipa/raspberrypi/controller/agc_status.h
> > > > index 10381c90a313..c893715af588 100644
> > > > --- a/src/ipa/raspberrypi/controller/agc_status.h
> > > > +++ b/src/ipa/raspberrypi/controller/agc_status.h
> > > > @@ -6,6 +6,8 @@
> > > >   */
> > > >  #pragma once
> > > >
> > > > +#include "libcamera/internal/utils.h"
> > > > +
> > > >  // The AGC algorithm should post the following structure into the
> > image's
> > > >  // "agc.status" metadata.
> > > >
> > > > @@ -17,18 +19,20 @@ extern "C" {
> > > >  // seen statistics and calculated meaningful values. The contents
> > should be
> > > >  // ignored until then.
> > > >
> > > > +using libcamera::utils::Duration;
> > > > +
> > > >  struct AgcStatus {
> > > > -       double total_exposure_value; // value for all exposure and
> > gain for this image
> > > > -       double target_exposure_value; // (unfiltered) target total
> > exposure AGC is aiming for
> > > > -       double shutter_time;
> > > > +       Duration total_exposure_value; // value for all exposure and
> > gain for this image
> > > > +       Duration target_exposure_value; // (unfiltered) target total
> > exposure AGC is aiming for
> > > > +       Duration shutter_time;
> > > >         double analogue_gain;
> > > >         char exposure_mode[32];
> > > >         char constraint_mode[32];
> > > >         char metering_mode[32];
> > > >         double ev;
> > > > -       double flicker_period;
> > > > +       Duration flicker_period;
> > > >         int floating_region_enable;
> > > > -       double fixed_shutter;
> > > > +       Duration fixed_shutter;
> > > >         double fixed_analogue_gain;
> > > >         double digital_gain;
> > > >         int locked;
> > > > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> > b/src/ipa/raspberrypi/controller/device_status.h
> > > > index aa08608b5d40..131b4cd344ee 100644
> > > > --- a/src/ipa/raspberrypi/controller/device_status.h
> > > > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > > > @@ -6,6 +6,8 @@
> > > >   */
> > > >  #pragma once
> > > >
> > > > +#include "libcamera/internal/utils.h"
> > > > +
> > > >  // Definition of "device metadata" which stores things like shutter
> > time and
> > > >  // analogue gain that downstream control algorithms will want to know.
> > > >
> > > > @@ -14,8 +16,8 @@ extern "C" {
> > > >  #endif
> > > >
> > > >  struct DeviceStatus {
> > > > -       // time shutter is open, in microseconds
> > > > -       double shutter_speed;
> > > > +       // time shutter is open
> > > > +       libcamera::utils::Duration shutter_speed;
> > > >         double analogue_gain;
> > > >         // 1.0/distance-in-metres, or 0 if unknown
> > > >         double lens_position;
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > > index 482eb3ef962d..46742d845034 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > > @@ -21,6 +21,7 @@
> > > >
> > > >  using namespace RPiController;
> > > >  using namespace libcamera;
> > > > +using utils::operator<<;
> > >
> > > Still can't say that I like this... just because someone wants to use
> > > Durations, it seems like an awkward thing to make them remember. But
> > > unless anyone has anything better....
> >
> > I got the same feeling while reading the rest of the patches.
> > I can't say toString() is that better though, but probably is less
> > implicit that this using statment, which if read in isolation does not
> > immediately relates to the Duration class ?
> >
>
> I do agree with these points, and I don't like this as well.  I can see two
> alternatives without
> using toString():
>
> 1) Move the Duration class into its own namespace under utils, so then the
> above statement
> will look a bit more informative:
>
> using utils::Duration::operator<<;
>
> 2) Move the operator() out of the utils namespace.  This way we eliminate
> the need for the
> statement completely.

I vote for 2 if it can be done nicely!

>
> Regards,
> Naush
>
>
> >
> > >
> > > >
> > > >  LOG_DEFINE_CATEGORY(RPiAgc)
> > > >
> > > > @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string,
> > AgcMeteringMode> &metering_modes,
> > > >         return first;
> > > >  }
> > > >
> > > > -static int read_double_list(std::vector<double> &list,
> > > > -                           boost::property_tree::ptree const &params)
> > > > +static int read_list(std::vector<double> &list,
> > > > +                    boost::property_tree::ptree const &params)
> > > >  {
> > > >         for (auto &p : params)
> > > >                 list.push_back(p.second.get_value<double>());
> > > >         return list.size();
> > > >  }
> > > >
> > > > +static int read_list(std::vector<Duration> &list,
> > > > +                    boost::property_tree::ptree const &params)
> > > > +{
> > > > +       for (auto &p : params)
> > > > +               list.push_back(p.second.get_value<double>() * 1us);
> > > > +       return list.size();
> > > > +}
> > > > +
> > > >  void AgcExposureMode::Read(boost::property_tree::ptree const &params)
> > > >  {
> > > >         int num_shutters =
> > > > -               read_double_list(shutter, params.get_child("shutter"));
> > > > -       int num_ags = read_double_list(gain, params.get_child("gain"));
> > > > +               read_list(shutter, params.get_child("shutter"));
> > > > +       int num_ags = read_list(gain, params.get_child("gain"));
> > > >         if (num_shutters < 2 || num_ags < 2)
> > > >                 throw std::runtime_error(
> > > >                         "AgcConfig: must have at least two entries in
> > exposure profile");
> > > > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree
> > const &params)
> > > >                 params.get<double>("fast_reduce_threshold", 0.4);
> > > >         base_ev = params.get<double>("base_ev", 1.0);
> > > >         // Start with quite a low value as ramping up is easier than
> > ramping down.
> > > > -       default_exposure_time =
> > params.get<double>("default_exposure_time", 1000);
> > > > +       default_exposure_time =
> > params.get<double>("default_exposure_time", 1000) * 1us;
> > > >         default_analogue_gain =
> > params.get<double>("default_analogue_gain", 1.0);
> > > >  }
> > > >
> > > > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)
> > > >         : AgcAlgorithm(controller), metering_mode_(nullptr),
> > > >           exposure_mode_(nullptr), constraint_mode_(nullptr),
> > > >           frame_count_(0), lock_count_(0),
> > > > -         last_target_exposure_(0.0),
> > > > -         ev_(1.0), flicker_period_(0.0),
> > > > -         max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> > > > +         last_target_exposure_(0s),
> > > > +         ev_(1.0), flicker_period_(0s),
> > > > +         max_shutter_(0s), fixed_shutter_(0s),
> > fixed_analogue_gain_(0.0)
> > > >  {
> > > >         memset(&awb_, 0, sizeof(awb_));
> > > >         // Setting status_.total_exposure_value_ to zero initially
> > tells us
> > > > @@ -203,7 +212,7 @@ void Agc::Pause()
> > > >
> > > >  void Agc::Resume()
> > > >  {
> > > > -       fixed_shutter_ = 0;
> > > > +       fixed_shutter_ = 0s;
> > > >         fixed_analogue_gain_ = 0;
> > > >  }
> > > >
> > > > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)
> > > >
> > > >  void Agc::SetFlickerPeriod(const Duration &flicker_period)
> > > >  {
> > > > -       flicker_period_ = flicker_period.get<std::micro>();
> > > > +       flicker_period_ = flicker_period;
> > > >  }
> > > >
> > > >  void Agc::SetMaxShutter(const Duration &max_shutter)
> > > >  {
> > > > -       max_shutter_ = max_shutter.get<std::micro>();
> > > > +       max_shutter_ = max_shutter;
> > > >  }
> > > >
> > > >  void Agc::SetFixedShutter(const Duration &fixed_shutter)
> > > >  {
> > > > -       fixed_shutter_ = fixed_shutter.get<std::micro>();
> > > > +       fixed_shutter_ = fixed_shutter;
> > > >         // Set this in case someone calls Pause() straight after.
> > > >         status_.shutter_time = clipShutter(fixed_shutter_);
> > > >  }
> > > > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
> > const &camera_mode,
> > > >  {
> > > >         housekeepConfig();
> > > >
> > > > -       double fixed_shutter = clipShutter(fixed_shutter_);
> > > > -       if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> > > > +       Duration fixed_shutter = clipShutter(fixed_shutter_);
> > > > +       if (fixed_shutter && fixed_analogue_gain_) {
> > > >                 // We're going to reset the algorithm here with these
> > fixed values.
> > > >
> > > >                 fetchAwbStatus(metadata);
> > > > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)
> > > >                 // Process has run, so we have meaningful values.
> > > >                 DeviceStatus device_status;
> > > >                 if (image_metadata->Get("device.status",
> > device_status) == 0) {
> > > > -                       double actual_exposure =
> > device_status.shutter_speed *
> > > > -
> > device_status.analogue_gain;
> > > > +                       Duration actual_exposure =
> > device_status.shutter_speed *
> > > > +
> > device_status.analogue_gain;
> > > >                         if (actual_exposure) {
> > > >                                 status_.digital_gain =
> > > >                                         status_.total_exposure_value /
> > > > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)
> > > >                                         std::min(status_.digital_gain,
> > 4.0));
> > > >                                 LOG(RPiAgc, Debug) << "Actual exposure
> > " << actual_exposure;
> > > >                                 LOG(RPiAgc, Debug) << "Use
> > digital_gain " << status_.digital_gain;
> > > > -                               LOG(RPiAgc, Debug) << "Effective
> > exposure " << actual_exposure * status_.digital_gain;
> > > > +                               LOG(RPiAgc, Debug) << "Effective
> > exposure "
> > > > +                                                  << actual_exposure
> > * status_.digital_gain;
> > > >                                 // Decide whether AEC/AGC has
> > converged.
> > > >                                 updateLockStatus(device_status);
> > > >                         }
> > > > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const
> > &device_status)
> > > >         const double RESET_MARGIN = 1.5;
> > > >
> > > >         // Add 200us to the exposure time error to allow for line
> > quantisation.
> > > > -       double exposure_error = last_device_status_.shutter_speed *
> > ERROR_FACTOR + 200;
> > > > +       Duration exposure_error = last_device_status_.shutter_speed *
> > ERROR_FACTOR + 200us;
> > > >         double gain_error = last_device_status_.analogue_gain *
> > ERROR_FACTOR;
> > > > -       double target_error = last_target_exposure_ * ERROR_FACTOR;
> > > > +       Duration target_error = last_target_exposure_ * ERROR_FACTOR;
> > > >
> > > >         // Note that we don't know the exposure/gain limits of the
> > sensor, so
> > > >         // the values we keep requesting may be unachievable. For this
> > reason
> > > > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata
> > *image_metadata)
> > > >         current_.analogue_gain = device_status->analogue_gain;
> > > >         AgcStatus *agc_status =
> > > >                 image_metadata->GetLocked<AgcStatus>("agc.status");
> > > > -       current_.total_exposure = agc_status ?
> > agc_status->total_exposure_value : 0;
> > > > +       current_.total_exposure = agc_status ?
> > agc_status->total_exposure_value : 0s;
> > > >         current_.total_exposure_no_dg = current_.shutter *
> > current_.analogue_gain;
> > > >  }
> > > >
> > > > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats
> > *statistics, Metadata *image_metadata,
> > > >
> > > >  void Agc::computeTargetExposure(double gain)
> > > >  {
> > > > -       if (status_.fixed_shutter != 0.0 &&
> > status_.fixed_analogue_gain != 0.0) {
> > > > +       if (status_.fixed_shutter && status_.fixed_analogue_gain) {
> > > >                 // When ag and shutter are both fixed, we need to
> > drive the
> > > >                 // total exposure so that we end up with a digital
> > gain of at least
> > > >                 // 1/min_colour_gain. Otherwise we'd desaturate
> > channels causing
> > > > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)
> > > >                 target_.total_exposure = current_.total_exposure_no_dg
> > * gain;
> > > >                 // The final target exposure is also limited to what
> > the exposure
> > > >                 // mode allows.
> > > > -               double max_shutter = status_.fixed_shutter != 0.0
> > > > +               Duration max_shutter = status_.fixed_shutter
> > > >                                    ? status_.fixed_shutter
> > > >                                    : exposure_mode_->shutter.back();
> > > >                 max_shutter = clipShutter(max_shutter);
> > > > -               double max_total_exposure =
> > > > +               Duration max_total_exposure =
> > > >                         max_shutter *
> > > >                         (status_.fixed_analogue_gain != 0.0
> > > >                                  ? status_.fixed_analogue_gain
> > > > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)
> > > >         if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
> > > >             frame_count_ <= config_.startup_frames)
> > > >                 speed = 1.0;
> > > > -       if (filtered_.total_exposure == 0.0) {
> > > > +       if (!filtered_.total_exposure) {
> > > >                 filtered_.total_exposure = target_.total_exposure;
> > > >                 filtered_.total_exposure_no_dg =
> > target_.total_exposure_no_dg;
> > > >         } else {
> > > > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()
> > > >         // Sending the fixed shutter/gain cases through the same code
> > may seem
> > > >         // unnecessary, but it will make more sense when extend this
> > to cover
> > > >         // variable aperture.
> > > > -       double exposure_value = filtered_.total_exposure_no_dg;
> > > > -       double shutter_time, analogue_gain;
> > > > -       shutter_time = status_.fixed_shutter != 0.0
> > > > +       Duration exposure_value = filtered_.total_exposure_no_dg;
> > > > +       Duration shutter_time;
> > > > +       double analogue_gain;
> > > > +       shutter_time = status_.fixed_shutter
> > > >                                ? status_.fixed_shutter
> > > >                                : exposure_mode_->shutter[0];
> > > >         shutter_time = clipShutter(shutter_time);
> > > > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()
> > > >         if (shutter_time * analogue_gain < exposure_value) {
> > > >                 for (unsigned int stage = 1;
> > > >                      stage < exposure_mode_->gain.size(); stage++) {
> > > > -                       if (status_.fixed_shutter == 0.0) {
> > > > -                               double stage_shutter =
> > > > +                       if (!status_.fixed_shutter) {
> > > > +                               Duration stage_shutter =
> > > >
> >  clipShutter(exposure_mode_->shutter[stage]);
> > > >                                 if (stage_shutter * analogue_gain >=
> > > >                                     exposure_value) {
> > > > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()
> > > >                            << analogue_gain;
> > > >         // Finally adjust shutter time for flicker avoidance (require
> > both
> > > >         // shutter and gain not to be fixed).
> > > > -       if (status_.fixed_shutter == 0.0 &&
> > > > -           status_.fixed_analogue_gain == 0.0 &&
> > > > -           status_.flicker_period != 0.0) {
> > > > -               int flicker_periods = shutter_time /
> > status_.flicker_period;
> > > > -               if (flicker_periods > 0) {
> > > > -                       double new_shutter_time = flicker_periods *
> > status_.flicker_period;
> > > > +       if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
> > > > +           status_.flicker_period) {
> > > > +               double flicker_periods = shutter_time /
> > status_.flicker_period;
> > > > +               if (flicker_periods > 0.0) {
> > > > +                       Duration new_shutter_time = flicker_periods *
> > status_.flicker_period;
> > >
> > > Just wondering if this will behave the same now that flicker_periods
> > > is a double. Won't it now hold fractional values with the result that
> > > new_shutter_time isn't an exact multiple of the period?
> > >
> > > >                         analogue_gain *= shutter_time /
> > new_shutter_time;
> > > >                         // We should still not allow the ag to go over
> > the
> > > >                         // largest value in the exposure mode. Note
> > that this
> > > > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()
> > > >  void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)
> > > >  {
> > > >         status_.total_exposure_value = filtered_.total_exposure;
> > > > -       status_.target_exposure_value = desaturate ? 0 :
> > target_.total_exposure_no_dg;
> > > > +       status_.target_exposure_value = desaturate ? 0s :
> > target_.total_exposure_no_dg;
> > > >         status_.shutter_time = filtered_.shutter;
> > > >         status_.analogue_gain = filtered_.analogue_gain;
> > > >         // Write to metadata as well, in case anyone wants to update
> > the camera
> > > > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata
> > *image_metadata, bool desaturate)
> > > >                            << " analogue gain " <<
> > filtered_.analogue_gain;
> > > >  }
> > > >
> > > > -double Agc::clipShutter(double shutter)
> > > > +Duration Agc::clipShutter(const Duration &shutter)
> > > >  {
> > > > +       Duration clip_shutter = shutter;
> > >
> > > Given that we copy it anyway I wonder if we might have left this as
> > > call-by-value, resulting in fewer changes. But it's only the teeniest
> > > of nit-picks!
> > >
> > > If you can put my mind at rest over the flicker period thing:
> > >
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > >
> > > Thanks!
> > > David
> > >
> > > >         if (max_shutter_)
> > > > -               shutter = std::min(shutter, max_shutter_);
> > > > -       return shutter;
> > > > +               clip_shutter = std::min(clip_shutter, max_shutter_);
> > > > +       return clip_shutter;
> > > >  }
> > > >
> > > >  // Register algorithm with the system.
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > > index 16a65959d90e..24a6610096c2 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > > @@ -9,6 +9,8 @@
> > > >  #include <vector>
> > > >  #include <mutex>
> > > >
> > > > +#include "libcamera/internal/utils.h"
> > > > +
> > > >  #include "../agc_algorithm.hpp"
> > > >  #include "../agc_status.h"
> > > >  #include "../pwl.hpp"
> > > > @@ -22,13 +24,15 @@
> > > >
> > > >  namespace RPiController {
> > > >
> > > > +using namespace std::literals::chrono_literals;
> > > > +
> > > >  struct AgcMeteringMode {
> > > >         double weights[AGC_STATS_SIZE];
> > > >         void Read(boost::property_tree::ptree const &params);
> > > >  };
> > > >
> > > >  struct AgcExposureMode {
> > > > -       std::vector<double> shutter;
> > > > +       std::vector<Duration> shutter;
> > > >         std::vector<double> gain;
> > > >         void Read(boost::property_tree::ptree const &params);
> > > >  };
> > > > @@ -61,7 +65,7 @@ struct AgcConfig {
> > > >         std::string default_exposure_mode;
> > > >         std::string default_constraint_mode;
> > > >         double base_ev;
> > > > -       double default_exposure_time;
> > > > +       Duration default_exposure_time;
> > > >         double default_analogue_gain;
> > > >  };
> > > >
> > > > @@ -101,19 +105,19 @@ private:
> > > >         void filterExposure(bool desaturate);
> > > >         void divideUpExposure();
> > > >         void writeAndFinish(Metadata *image_metadata, bool desaturate);
> > > > -       double clipShutter(double shutter);
> > > > +       Duration clipShutter(const Duration &shutter);
> > > >         AgcMeteringMode *metering_mode_;
> > > >         AgcExposureMode *exposure_mode_;
> > > >         AgcConstraintMode *constraint_mode_;
> > > >         uint64_t frame_count_;
> > > >         AwbStatus awb_;
> > > >         struct ExposureValues {
> > > > -               ExposureValues() : shutter(0), analogue_gain(0),
> > > > -                                  total_exposure(0),
> > total_exposure_no_dg(0) {}
> > > > -               double shutter;
> > > > +               ExposureValues() : shutter(0s), analogue_gain(0),
> > > > +                                  total_exposure(0s),
> > total_exposure_no_dg(0s) {}
> > > > +               Duration shutter;
> > > >                 double analogue_gain;
> > > > -               double total_exposure;
> > > > -               double total_exposure_no_dg; // without digital gain
> > > > +               Duration total_exposure;
> > > > +               Duration total_exposure_no_dg; // without digital gain
> > > >         };
> > > >         ExposureValues current_;  // values for the current frame
> > > >         ExposureValues target_;   // calculate the values we want here
> > > > @@ -121,15 +125,15 @@ private:
> > > >         AgcStatus status_;
> > > >         int lock_count_;
> > > >         DeviceStatus last_device_status_;
> > > > -       double last_target_exposure_;
> > > > +       Duration last_target_exposure_;
> > > >         // Below here the "settings" that applications can change.
> > > >         std::string metering_mode_name_;
> > > >         std::string exposure_mode_name_;
> > > >         std::string constraint_mode_name_;
> > > >         double ev_;
> > > > -       double flicker_period_;
> > > > -       double max_shutter_;
> > > > -       double fixed_shutter_;
> > > > +       Duration flicker_period_;
> > > > +       Duration max_shutter_;
> > > > +       Duration fixed_shutter_;
> > > >         double fixed_analogue_gain_;
> > > >  };
> > > >
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > > index f74381cab2b4..0fa6457c1376 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > > @@ -16,6 +16,7 @@
> > > >
> > > >  using namespace RPiController;
> > > >  using namespace libcamera;
> > > > +using namespace std::literals::chrono_literals;
> > > >
> > > >  LOG_DEFINE_CATEGORY(RPiLux)
> > > >
> > > > @@ -38,7 +39,7 @@ char const *Lux::Name() const
> > > >  void Lux::Read(boost::property_tree::ptree const &params)
> > > >  {
> > > >         reference_shutter_speed_ =
> > > > -               params.get<double>("reference_shutter_speed");
> > > > +               params.get<double>("reference_shutter_speed") * 1.0us;
> > > >         reference_gain_ = params.get<double>("reference_gain");
> > > >         reference_aperture_ = params.get<double>("reference_aperture",
> > 1.0);
> > > >         reference_Y_ = params.get<double>("reference_Y");
> > > > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
> > > >  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> > > >  {
> > > >         // set some initial values to shut the compiler up
> > > > -       DeviceStatus device_status =
> > > > -               { .shutter_speed = 1.0,
> > > > -                 .analogue_gain = 1.0,
> > > > -                 .lens_position = 0.0,
> > > > -                 .aperture = 0.0,
> > > > -                 .flash_intensity = 0.0 };
> > > > +       DeviceStatus device_status = { .shutter_speed = 1.0ms,
> > > > +                                      .analogue_gain = 1.0,
> > > > +                                      .lens_position = 0.0,
> > > > +                                      .aperture = 0.0,
> > > > +                                      .flash_intensity = 0.0 };
> > > >         if (image_metadata->Get("device.status", device_status) == 0) {
> > > >                 double current_gain = device_status.analogue_gain;
> > > > -               double current_shutter_speed =
> > device_status.shutter_speed;
> > > >                 double current_aperture = device_status.aperture;
> > > >                 if (current_aperture == 0)
> > > >                         current_aperture = current_aperture_;
> > > > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata
> > *image_metadata)
> > > >                 double current_Y = sum / (double)num + .5;
> > > >                 double gain_ratio = reference_gain_ / current_gain;
> > > >                 double shutter_speed_ratio =
> > > > -                       reference_shutter_speed_ /
> > current_shutter_speed;
> > > > +                       reference_shutter_speed_ /
> > device_status.shutter_speed;
> > > >                 double aperture_ratio = reference_aperture_ /
> > current_aperture;
> > > >                 double Y_ratio = current_Y * (65536 / num_bins) /
> > reference_Y_;
> > > >                 double estimated_lux = shutter_speed_ratio *
> > gain_ratio *
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > > > index f9090484a136..45c844393e62 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > > > @@ -8,6 +8,8 @@
> > > >
> > > >  #include <mutex>
> > > >
> > > > +#include "libcamera/internal/utils.h"
> > > > +
> > > >  #include "../lux_status.h"
> > > >  #include "../algorithm.hpp"
> > > >
> > > > @@ -28,7 +30,7 @@ public:
> > > >  private:
> > > >         // These values define the conditions of the reference image,
> > against
> > > >         // which we compare the new image.
> > > > -       double reference_shutter_speed_; // in micro-seconds
> > > > +       libcamera::utils::Duration reference_shutter_speed_;
> > > >         double reference_gain_;
> > > >         double reference_aperture_; // units of 1/f
> > > >         double reference_Y_; // out of 65536
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index f8f36420b076..b77230ded591 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,
> > ipa::RPi::StartConfig *startConf
> > > >
> > > >         /* SwitchMode may supply updated exposure/gain values to use.
> > */
> > > >         AgcStatus agcStatus;
> > > > -       agcStatus.shutter_time = 0.0;
> > > > +       agcStatus.shutter_time = 0.0s;
> > > >         agcStatus.analogue_gain = 0.0;
> > > >
> > > >         metadata.Get("agc.status", agcStatus);
> > > > -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain
> > != 0.0) {
> > > > +       if (agcStatus.shutter_time && agcStatus.analogue_gain) {
> > > >                 ControlList ctrls(sensorCtrls_);
> > > >                 applyAGC(&agcStatus, ctrls);
> > > >                 startConfig->controls = std::move(ctrls);
> > > > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo
> > &sensorInfo,
> > > >                 /* Supply initial values for gain and exposure. */
> > > >                 ControlList ctrls(sensorCtrls_);
> > > >                 AgcStatus agcStatus;
> > > > -               agcStatus.shutter_time =
> > DefaultExposureTime.get<std::micro>();
> > > > +               agcStatus.shutter_time = DefaultExposureTime;
> > > >                 agcStatus.analogue_gain = DefaultAnalogueGain;
> > > >                 applyAGC(&agcStatus, ctrls);
> > > >
> > > > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()
> > > >          */
> > > >         DeviceStatus *deviceStatus =
> > rpiMetadata_.GetLocked<DeviceStatus>("device.status");
> > > >         if (deviceStatus) {
> > > > -               libcameraMetadata_.set(controls::ExposureTime,
> > deviceStatus->shutter_speed);
> > > > +               libcameraMetadata_.set(controls::ExposureTime,
> > > > +
> > deviceStatus->shutter_speed.get<std::micro>());
> > > >                 libcameraMetadata_.set(controls::AnalogueGain,
> > deviceStatus->analogue_gain);
> > > >         }
> > > >
> > > > @@ -1017,7 +1018,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).get<std::micro>();
> > > > +       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > > >         deviceStatus.analogue_gain = helper_->Gain(gainCode);
> > > >
> > > >         LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > > > @@ -1103,7 +1104,7 @@ 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. */
> > > > -       Duration exposure = agcStatus->shutter_time * 1.0us;
> > > > +       Duration exposure = agcStatus->shutter_time;
> > > >         int32_t vblanking = helper_->GetVBlanking(exposure,
> > minFrameDuration_, maxFrameDuration_);
> > > >         int32_t exposureLines = helper_->ExposureLines(exposure);
> > > >
> > > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > > > index 3131aa2d6666..5fa6f8366f3d 100644
> > > > --- a/src/libcamera/utils.cpp
> > > > +++ b/src/libcamera/utils.cpp
> > > > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()
> > > >   * loop, iterates over an indexed view of the \a iterable
> > > >   */
> > > >
> > > > +/**
> > > > + * \typedef BaseDuration
> > > > + * \brief Base struct for the \a Duration helper class.
> > > > + */
> > > > +
> > > >  /**
> > > >   * \class Duration
> > > > - * \brief Helper for std::chrono::duration types. Derived from \a
> > BaseDuration
> > > > + * \brief Helper class for std::chrono::duration types.
> > > >   */
> > > >
> > > >  /**
> > > > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()
> > > >   * \param[in] d The std::chrono::duration object to convert from.
> > > >   *
> > > >   * Constructs a \a Duration object from a \a std::chrono::duration
> > object,
> > > > - * converting the representation to \a BaseDuration type.
> > > > + * internally converting the representation to \a BaseDuration type.
> > > >   */
> > > >
> > > >  /**
> > > > --
> > > > 2.25.1
> > > >
> >