[{"id":17085,"web_url":"https://patchwork.libcamera.org/comment/17085/","msgid":"","date":"2021-05-21T11:18:55","subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for all this work!\n\nOn Fri, 21 May 2021 at 09:05, Naushir Patuck wrote:\n>\n> Convert the core AGC and Lux controller code to use\n> utils::Duration for all exposure time related variables and\n> calculations.\n>\n> Convert the exposure/shutter time fields in AgcStatus and DeviceStatus\n> to use utils::Duration.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 2 +-\n> src/ipa/raspberrypi/controller/agc_status.h | 14 +--\n> .../raspberrypi/controller/device_status.h | 6 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 +++++++++++--------\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---\n> src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--\n> src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 13 +--\n> src/libcamera/utils.cpp | 9 +-\n> 9 files changed, 105 insertions(+), 77 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index feb02d1806b2..968fae13bd5e 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> return;\n> }\n>\n> - deviceStatus.shutter_speed = Exposure(exposureLines).get();\n> + deviceStatus.shutter_speed = Exposure(exposureLines);\n> deviceStatus.analogue_gain = Gain(gainCode);\n>\n> LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> diff --git a/src/ipa/raspberrypi/controller/agc_status.h b/src/ipa/raspberrypi/controller/agc_status.h\n> index 10381c90a313..c893715af588 100644\n> --- a/src/ipa/raspberrypi/controller/agc_status.h\n> +++ b/src/ipa/raspberrypi/controller/agc_status.h\n> @@ -6,6 +6,8 @@\n> */\n> #pragma once\n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> // The AGC algorithm should post the following structure into the image's\n> // \"agc.status\" metadata.\n>\n> @@ -17,18 +19,20 @@ extern \"C\" {\n> // seen statistics and calculated meaningful values. The contents should be\n> // ignored until then.\n>\n> +using libcamera::utils::Duration;\n> +\n> struct AgcStatus {\n> - double total_exposure_value; // value for all exposure and gain for this image\n> - double target_exposure_value; // (unfiltered) target total exposure AGC is aiming for\n> - double shutter_time;\n> + Duration total_exposure_value; // value for all exposure and gain for this image\n> + Duration target_exposure_value; // (unfiltered) target total exposure AGC is aiming for\n> + Duration shutter_time;\n> double analogue_gain;\n> char exposure_mode[32];\n> char constraint_mode[32];\n> char metering_mode[32];\n> double ev;\n> - double flicker_period;\n> + Duration flicker_period;\n> int floating_region_enable;\n> - double fixed_shutter;\n> + Duration fixed_shutter;\n> double fixed_analogue_gain;\n> double digital_gain;\n> int locked;\n> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h\n> index aa08608b5d40..131b4cd344ee 100644\n> --- a/src/ipa/raspberrypi/controller/device_status.h\n> +++ b/src/ipa/raspberrypi/controller/device_status.h\n> @@ -6,6 +6,8 @@\n> */\n> #pragma once\n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> // Definition of \"device metadata\" which stores things like shutter time and\n> // analogue gain that downstream control algorithms will want to know.\n>\n> @@ -14,8 +16,8 @@ extern \"C\" {\n> #endif\n>\n> struct DeviceStatus {\n> - // time shutter is open, in microseconds\n> - double shutter_speed;\n> + // time shutter is open\n> + libcamera::utils::Duration shutter_speed;\n> double analogue_gain;\n> // 1.0/distance-in-metres, or 0 if unknown\n> double lens_position;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 482eb3ef962d..46742d845034 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -21,6 +21,7 @@\n>\n> using namespace RPiController;\n> using namespace libcamera;\n> +using utils::operator<<;\n\nStill can't say that I like this... just because someone wants to use\nDurations, it seems like an awkward thing to make them remember. But\nunless anyone has anything better....\n\n>\n> LOG_DEFINE_CATEGORY(RPiAgc)\n>\n> @@ -55,19 +56,27 @@ read_metering_modes(std::map &metering_modes,\n> return first;\n> }\n>\n> -static int read_double_list(std::vector &list,\n> - boost::property_tree::ptree const ¶ms)\n> +static int read_list(std::vector &list,\n> + boost::property_tree::ptree const ¶ms)\n> {\n> for (auto &p : params)\n> list.push_back(p.second.get_value());\n> return list.size();\n> }\n>\n> +static int read_list(std::vector &list,\n> + boost::property_tree::ptree const ¶ms)\n> +{\n> + for (auto &p : params)\n> + list.push_back(p.second.get_value() * 1us);\n> + return list.size();\n> +}\n> +\n> void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms)\n> {\n> int num_shutters =\n> - read_double_list(shutter, params.get_child(\"shutter\"));\n> - int num_ags = read_double_list(gain, params.get_child(\"gain\"));\n> + read_list(shutter, params.get_child(\"shutter\"));\n> + int num_ags = read_list(gain, params.get_child(\"gain\"));\n> if (num_shutters < 2 || num_ags < 2)\n> throw std::runtime_error(\n> \"AgcConfig: must have at least two entries in exposure profile\");\n> @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const ¶ms)\n> params.get(\"fast_reduce_threshold\", 0.4);\n> base_ev = params.get(\"base_ev\", 1.0);\n> // Start with quite a low value as ramping up is easier than ramping down.\n> - default_exposure_time = params.get(\"default_exposure_time\", 1000);\n> + default_exposure_time = params.get(\"default_exposure_time\", 1000) * 1us;\n> default_analogue_gain = params.get(\"default_analogue_gain\", 1.0);\n> }\n>\n> @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)\n> : AgcAlgorithm(controller), metering_mode_(nullptr),\n> exposure_mode_(nullptr), constraint_mode_(nullptr),\n> frame_count_(0), lock_count_(0),\n> - last_target_exposure_(0.0),\n> - ev_(1.0), flicker_period_(0.0),\n> - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> + last_target_exposure_(0s),\n> + ev_(1.0), flicker_period_(0s),\n> + max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)\n> {\n> memset(&awb_, 0, sizeof(awb_));\n> // Setting status_.total_exposure_value_ to zero initially tells us\n> @@ -203,7 +212,7 @@ void Agc::Pause()\n>\n> void Agc::Resume()\n> {\n> - fixed_shutter_ = 0;\n> + fixed_shutter_ = 0s;\n> fixed_analogue_gain_ = 0;\n> }\n>\n> @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)\n>\n> void Agc::SetFlickerPeriod(const Duration &flicker_period)\n> {\n> - flicker_period_ = flicker_period.get();\n> + flicker_period_ = flicker_period;\n> }\n>\n> void Agc::SetMaxShutter(const Duration &max_shutter)\n> {\n> - max_shutter_ = max_shutter.get();\n> + max_shutter_ = max_shutter;\n> }\n>\n> void Agc::SetFixedShutter(const Duration &fixed_shutter)\n> {\n> - fixed_shutter_ = fixed_shutter.get();\n> + fixed_shutter_ = fixed_shutter;\n> // Set this in case someone calls Pause() straight after.\n> status_.shutter_time = clipShutter(fixed_shutter_);\n> }\n> @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> {\n> housekeepConfig();\n>\n> - double fixed_shutter = clipShutter(fixed_shutter_);\n> - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> + Duration fixed_shutter = clipShutter(fixed_shutter_);\n> + if (fixed_shutter && fixed_analogue_gain_) {\n> // We're going to reset the algorithm here with these fixed values.\n>\n> fetchAwbStatus(metadata);\n> @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> // Process has run, so we have meaningful values.\n> DeviceStatus device_status;\n> if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> - double actual_exposure = device_status.shutter_speed *\n> - device_status.analogue_gain;\n> + Duration actual_exposure = device_status.shutter_speed *\n> + device_status.analogue_gain;\n> if (actual_exposure) {\n> status_.digital_gain =\n> status_.total_exposure_value /\n> @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> std::min(status_.digital_gain, 4.0));\n> LOG(RPiAgc, Debug) << \"Actual exposure \" << actual_exposure;\n> LOG(RPiAgc, Debug) << \"Use digital_gain \" << status_.digital_gain;\n> - LOG(RPiAgc, Debug) << \"Effective exposure \" << actual_exposure * status_.digital_gain;\n> + LOG(RPiAgc, Debug) << \"Effective exposure \"\n> + << actual_exposure * status_.digital_gain;\n> // Decide whether AEC/AGC has converged.\n> updateLockStatus(device_status);\n> }\n> @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const &device_status)\n> const double RESET_MARGIN = 1.5;\n>\n> // Add 200us to the exposure time error to allow for line quantisation.\n> - double exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200;\n> + Duration exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200us;\n> double gain_error = last_device_status_.analogue_gain * ERROR_FACTOR;\n> - double target_error = last_target_exposure_ * ERROR_FACTOR;\n> + Duration target_error = last_target_exposure_ * ERROR_FACTOR;\n>\n> // Note that we don't know the exposure/gain limits of the sensor, so\n> // the values we keep requesting may be unachievable. For this reason\n> @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata *image_metadata)\n> current_.analogue_gain = device_status->analogue_gain;\n> AgcStatus *agc_status =\n> image_metadata->GetLocked(\"agc.status\");\n> - current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0;\n> + current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0s;\n> current_.total_exposure_no_dg = current_.shutter * current_.analogue_gain;\n> }\n>\n> @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *image_metadata,\n>\n> void Agc::computeTargetExposure(double gain)\n> {\n> - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain != 0.0) {\n> + if (status_.fixed_shutter && status_.fixed_analogue_gain) {\n> // When ag and shutter are both fixed, we need to drive the\n> // total exposure so that we end up with a digital gain of at least\n> // 1/min_colour_gain. Otherwise we'd desaturate channels causing\n> @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)\n> target_.total_exposure = current_.total_exposure_no_dg * gain;\n> // The final target exposure is also limited to what the exposure\n> // mode allows.\n> - double max_shutter = status_.fixed_shutter != 0.0\n> + Duration max_shutter = status_.fixed_shutter\n> ? status_.fixed_shutter\n> : exposure_mode_->shutter.back();\n> max_shutter = clipShutter(max_shutter);\n> - double max_total_exposure =\n> + Duration max_total_exposure =\n> max_shutter *\n> (status_.fixed_analogue_gain != 0.0\n> ? status_.fixed_analogue_gain\n> @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)\n> if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||\n> frame_count_ <= config_.startup_frames)\n> speed = 1.0;\n> - if (filtered_.total_exposure == 0.0) {\n> + if (!filtered_.total_exposure) {\n> filtered_.total_exposure = target_.total_exposure;\n> filtered_.total_exposure_no_dg = target_.total_exposure_no_dg;\n> } else {\n> @@ -674,9 +684,10 @@ void Agc::divideUpExposure()\n> // Sending the fixed shutter/gain cases through the same code may seem\n> // unnecessary, but it will make more sense when extend this to cover\n> // variable aperture.\n> - double exposure_value = filtered_.total_exposure_no_dg;\n> - double shutter_time, analogue_gain;\n> - shutter_time = status_.fixed_shutter != 0.0\n> + Duration exposure_value = filtered_.total_exposure_no_dg;\n> + Duration shutter_time;\n> + double analogue_gain;\n> + shutter_time = status_.fixed_shutter\n> ? status_.fixed_shutter\n> : exposure_mode_->shutter[0];\n> shutter_time = clipShutter(shutter_time);\n> @@ -686,8 +697,8 @@ void Agc::divideUpExposure()\n> if (shutter_time * analogue_gain < exposure_value) {\n> for (unsigned int stage = 1;\n> stage < exposure_mode_->gain.size(); stage++) {\n> - if (status_.fixed_shutter == 0.0) {\n> - double stage_shutter =\n> + if (!status_.fixed_shutter) {\n> + Duration stage_shutter =\n> clipShutter(exposure_mode_->shutter[stage]);\n> if (stage_shutter * analogue_gain >=\n> exposure_value) {\n> @@ -713,12 +724,11 @@ void Agc::divideUpExposure()\n> << analogue_gain;\n> // Finally adjust shutter time for flicker avoidance (require both\n> // shutter and gain not to be fixed).\n> - if (status_.fixed_shutter == 0.0 &&\n> - status_.fixed_analogue_gain == 0.0 &&\n> - status_.flicker_period != 0.0) {\n> - int flicker_periods = shutter_time / status_.flicker_period;\n> - if (flicker_periods > 0) {\n> - double new_shutter_time = flicker_periods * status_.flicker_period;\n> + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&\n> + status_.flicker_period) {\n> + double flicker_periods = shutter_time / status_.flicker_period;\n> + if (flicker_periods > 0.0) {\n> + Duration new_shutter_time = flicker_periods * status_.flicker_period;\n\nJust wondering if this will behave the same now that flicker_periods\nis a double. Won't it now hold fractional values with the result that\nnew_shutter_time isn't an exact multiple of the period?\n\n> analogue_gain *= shutter_time / new_shutter_time;\n> // We should still not allow the ag to go over the\n> // largest value in the exposure mode. Note that this\n> @@ -738,7 +748,7 @@ void Agc::divideUpExposure()\n> void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> {\n> status_.total_exposure_value = filtered_.total_exposure;\n> - status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg;\n> + status_.target_exposure_value = desaturate ? 0s : target_.total_exposure_no_dg;\n> status_.shutter_time = filtered_.shutter;\n> status_.analogue_gain = filtered_.analogue_gain;\n> // Write to metadata as well, in case anyone wants to update the camera\n> @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> << \" analogue gain \" << filtered_.analogue_gain;\n> }\n>\n> -double Agc::clipShutter(double shutter)\n> +Duration Agc::clipShutter(const Duration &shutter)\n> {\n> + Duration clip_shutter = shutter;\n\nGiven that we copy it anyway I wonder if we might have left this as\ncall-by-value, resulting in fewer changes. But it's only the teeniest\nof nit-picks!\n\nIf you can put my mind at rest over the flicker period thing:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> if (max_shutter_)\n> - shutter = std::min(shutter, max_shutter_);\n> - return shutter;\n> + clip_shutter = std::min(clip_shutter, max_shutter_);\n> + return clip_shutter;\n> }\n>\n> // Register algorithm with the system.\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 16a65959d90e..24a6610096c2 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -9,6 +9,8 @@\n> #include \n> #include \n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> #include \"../agc_algorithm.hpp\"\n> #include \"../agc_status.h\"\n> #include \"../pwl.hpp\"\n> @@ -22,13 +24,15 @@\n>\n> namespace RPiController {\n>\n> +using namespace std::literals::chrono_literals;\n> +\n> struct AgcMeteringMode {\n> double weights[AGC_STATS_SIZE];\n> void Read(boost::property_tree::ptree const ¶ms);\n> };\n>\n> struct AgcExposureMode {\n> - std::vector shutter;\n> + std::vector shutter;\n> std::vector gain;\n> void Read(boost::property_tree::ptree const ¶ms);\n> };\n> @@ -61,7 +65,7 @@ struct AgcConfig {\n> std::string default_exposure_mode;\n> std::string default_constraint_mode;\n> double base_ev;\n> - double default_exposure_time;\n> + Duration default_exposure_time;\n> double default_analogue_gain;\n> };\n>\n> @@ -101,19 +105,19 @@ private:\n> void filterExposure(bool desaturate);\n> void divideUpExposure();\n> void writeAndFinish(Metadata *image_metadata, bool desaturate);\n> - double clipShutter(double shutter);\n> + Duration clipShutter(const Duration &shutter);\n> AgcMeteringMode *metering_mode_;\n> AgcExposureMode *exposure_mode_;\n> AgcConstraintMode *constraint_mode_;\n> uint64_t frame_count_;\n> AwbStatus awb_;\n> struct ExposureValues {\n> - ExposureValues() : shutter(0), analogue_gain(0),\n> - total_exposure(0), total_exposure_no_dg(0) {}\n> - double shutter;\n> + ExposureValues() : shutter(0s), analogue_gain(0),\n> + total_exposure(0s), total_exposure_no_dg(0s) {}\n> + Duration shutter;\n> double analogue_gain;\n> - double total_exposure;\n> - double total_exposure_no_dg; // without digital gain\n> + Duration total_exposure;\n> + Duration total_exposure_no_dg; // without digital gain\n> };\n> ExposureValues current_; // values for the current frame\n> ExposureValues target_; // calculate the values we want here\n> @@ -121,15 +125,15 @@ private:\n> AgcStatus status_;\n> int lock_count_;\n> DeviceStatus last_device_status_;\n> - double last_target_exposure_;\n> + Duration last_target_exposure_;\n> // Below here the \"settings\" that applications can change.\n> std::string metering_mode_name_;\n> std::string exposure_mode_name_;\n> std::string constraint_mode_name_;\n> double ev_;\n> - double flicker_period_;\n> - double max_shutter_;\n> - double fixed_shutter_;\n> + Duration flicker_period_;\n> + Duration max_shutter_;\n> + Duration fixed_shutter_;\n> double fixed_analogue_gain_;\n> };\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> index f74381cab2b4..0fa6457c1376 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> @@ -16,6 +16,7 @@\n>\n> using namespace RPiController;\n> using namespace libcamera;\n> +using namespace std::literals::chrono_literals;\n>\n> LOG_DEFINE_CATEGORY(RPiLux)\n>\n> @@ -38,7 +39,7 @@ char const *Lux::Name() const\n> void Lux::Read(boost::property_tree::ptree const ¶ms)\n> {\n> reference_shutter_speed_ =\n> - params.get(\"reference_shutter_speed\");\n> + params.get(\"reference_shutter_speed\") * 1.0us;\n> reference_gain_ = params.get(\"reference_gain\");\n> reference_aperture_ = params.get(\"reference_aperture\", 1.0);\n> reference_Y_ = params.get(\"reference_Y\");\n> @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)\n> void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> {\n> // set some initial values to shut the compiler up\n> - DeviceStatus device_status =\n> - { .shutter_speed = 1.0,\n> - .analogue_gain = 1.0,\n> - .lens_position = 0.0,\n> - .aperture = 0.0,\n> - .flash_intensity = 0.0 };\n> + DeviceStatus device_status = { .shutter_speed = 1.0ms,\n> + .analogue_gain = 1.0,\n> + .lens_position = 0.0,\n> + .aperture = 0.0,\n> + .flash_intensity = 0.0 };\n> if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> double current_gain = device_status.analogue_gain;\n> - double current_shutter_speed = device_status.shutter_speed;\n> double current_aperture = device_status.aperture;\n> if (current_aperture == 0)\n> current_aperture = current_aperture_;\n> @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> double current_Y = sum / (double)num + .5;\n> double gain_ratio = reference_gain_ / current_gain;\n> double shutter_speed_ratio =\n> - reference_shutter_speed_ / current_shutter_speed;\n> + reference_shutter_speed_ / device_status.shutter_speed;\n> double aperture_ratio = reference_aperture_ / current_aperture;\n> double Y_ratio = current_Y * (65536 / num_bins) / reference_Y_;\n> double estimated_lux = shutter_speed_ratio * gain_ratio *\n> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> index f9090484a136..45c844393e62 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> @@ -8,6 +8,8 @@\n>\n> #include \n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> #include \"../lux_status.h\"\n> #include \"../algorithm.hpp\"\n>\n> @@ -28,7 +30,7 @@ public:\n> private:\n> // These values define the conditions of the reference image, against\n> // which we compare the new image.\n> - double reference_shutter_speed_; // in micro-seconds\n> + libcamera::utils::Duration reference_shutter_speed_;\n> double reference_gain_;\n> double reference_aperture_; // units of 1/f\n> double reference_Y_; // out of 65536\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f8f36420b076..b77230ded591 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf\n>\n> /* SwitchMode may supply updated exposure/gain values to use. */\n> AgcStatus agcStatus;\n> - agcStatus.shutter_time = 0.0;\n> + agcStatus.shutter_time = 0.0s;\n> agcStatus.analogue_gain = 0.0;\n>\n> metadata.Get(\"agc.status\", agcStatus);\n> - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> + if (agcStatus.shutter_time && agcStatus.analogue_gain) {\n> ControlList ctrls(sensorCtrls_);\n> applyAGC(&agcStatus, ctrls);\n> startConfig->controls = std::move(ctrls);\n> @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> /* Supply initial values for gain and exposure. */\n> ControlList ctrls(sensorCtrls_);\n> AgcStatus agcStatus;\n> - agcStatus.shutter_time = DefaultExposureTime.get();\n> + agcStatus.shutter_time = DefaultExposureTime;\n> agcStatus.analogue_gain = DefaultAnalogueGain;\n> applyAGC(&agcStatus, ctrls);\n>\n> @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()\n> */\n> DeviceStatus *deviceStatus = rpiMetadata_.GetLocked(\"device.status\");\n> if (deviceStatus) {\n> - libcameraMetadata_.set(controls::ExposureTime, deviceStatus->shutter_speed);\n> + libcameraMetadata_.set(controls::ExposureTime,\n> + deviceStatus->shutter_speed.get());\n> libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain);\n> }\n>\n> @@ -1017,7 +1018,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>\n> - deviceStatus.shutter_speed = helper_->Exposure(exposureLines).get();\n> + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>\n> LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> @@ -1103,7 +1104,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n>\n> /* GetVBlanking might clip exposure time to the fps limits. */\n> - Duration exposure = agcStatus->shutter_time * 1.0us;\n> + Duration exposure = agcStatus->shutter_time;\n> int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);\n> int32_t exposureLines = helper_->ExposureLines(exposure);\n>\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 3131aa2d6666..5fa6f8366f3d 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -506,9 +506,14 @@ std::string libcameraSourcePath()\n> * loop, iterates over an indexed view of the \\a iterable\n> */\n>\n> +/**\n> + * \\typedef BaseDuration\n> + * \\brief Base struct for the \\a Duration helper class.\n> + */\n> +\n> /**\n> * \\class Duration\n> - * \\brief Helper for std::chrono::duration types. Derived from \\a BaseDuration\n> + * \\brief Helper class for std::chrono::duration types.\n> */\n>\n> /**\n> @@ -517,7 +522,7 @@ std::string libcameraSourcePath()\n> * \\param[in] d The std::chrono::duration object to convert from.\n> *\n> * Constructs a \\a Duration object from a \\a std::chrono::duration object,\n> - * converting the representation to \\a BaseDuration type.\n> + * internally converting the representation to \\a BaseDuration type.\n> */\n>\n> /**\n> --\n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C6022C3200\n\tfor ;\n\tFri, 21 May 2021 11:19:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F010F6891D;\n\tFri, 21 May 2021 13:19:10 +0200 (CEST)","from mail-wr1-x430.google.com (mail-wr1-x430.google.com\n\t[IPv6:2a00:1450:4864:20::430])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E09EE68911\n\tfor ;\n\tFri, 21 May 2021 13:19:08 +0200 (CEST)","by mail-wr1-x430.google.com with SMTP id j14so18954843wrq.5\n\tfor ;\n\tFri, 21 May 2021 04:19:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"beT/yuSV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=MEbJ3DyNtjLLMBkdQyBbeYcTo54agcj3ngjIfSRPpdM=;\n\tb=beT/yuSVHxcupAC2AAr5Arpmy/O0vfZt90RTybEi0OaNT+C3y+Igj8cdcDWaZlVc/9\n\thAcdfl6o6BXTldVfsUIEFxg/2+5cnAbKNI942nECuDbGmXxMeOmXi/1+aCWuxn5gahjz\n\ttR6HkCc9CqVruh6plkzQNnUxEAxdDJKSd/kbVXNJyEQ1F/vHtV9w5hD/50JeioiyN22H\n\tHjRlqqq9EQsQz2bp6aeqDW5PFUcGzPrk7yzeZ6TP9tPv8MQgEwO5fodFhEsxHtOkmGHY\n\tovBsEBg6fE2c66AaYMtJcDudjXjLRGJPKmvMbgVdLzjnGnMfnq/CDn8s3CLbDlZym/ha\n\tuXZQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=MEbJ3DyNtjLLMBkdQyBbeYcTo54agcj3ngjIfSRPpdM=;\n\tb=U03f3y8NJAvM6ncg9CNzqxvkPKI6VpooL4REbEE6YlJ/MQQvYXsPehZARpD4q6t7sh\n\tFyB5hsIxH1DFJGuJG4D4BOEtfcYAxclKHkjnwKlUJ1veZL511iz5mJtVdph5LL8aJLBY\n\t1T2kwJfM7K0LIr9XIkUwPNiqKj23bOBBDgF6TCogKCXifWFzA0M7eSrVuPjkMAEivJ4x\n\t9avpfGO5v3t6ApKn6hz2qZtQlurGIRGVdFPiid8meSzsM3Ykt8mPrHDCu+m0fgAVhh9R\n\tSwBWYg0H46giliQrR4OuI0IEZydONZkOIP+ZBFe2gllY7+tiu0nxyqQMIG72LeA/tn+/\n\tfdUw==","X-Gm-Message-State":"AOAM531R5t97jfxT5j7K/v45tag+Mgk3YXbXCw9xSc6gsiMU59loj8MT\n\tfJIOjY287jfarca4PmIkYO6yeA9KgQOi3CzDPbBZEqGagno=","X-Google-Smtp-Source":"ABdhPJwxln6oC6dQKpphwcDyJv7Zz+EdaBwt4ghdQncG3mFDW+8p3g6l3/HoOLBKoKEnonPXxGSzp3l5jWioGb/1wEQ=","X-Received":"by 2002:a5d:408f:: with SMTP id o15mr8724599wrp.89.1621595948209;\n\tFri, 21 May 2021 04:19:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-5-naush@raspberrypi.com>","In-Reply-To":"<20210521080530.37591-5-naush@raspberrypi.com>","From":"David Plowman ","Date":"Fri, 21 May 2021 12:18:55 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17087,"web_url":"https://patchwork.libcamera.org/comment/17087/","msgid":"","date":"2021-05-21T11:33:03","subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 21 May 2021 at 09:05, Naushir Patuck wrote:\n\n> Convert the core AGC and Lux controller code to use\n> utils::Duration for all exposure time related variables and\n> calculations.\n>\n> Convert the exposure/shutter time fields in AgcStatus and DeviceStatus\n> to use utils::Duration.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 2 +-\n> src/ipa/raspberrypi/controller/agc_status.h | 14 +--\n> .../raspberrypi/controller/device_status.h | 6 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 +++++++++++--------\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---\n> src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--\n> src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 13 +--\n> src/libcamera/utils.cpp | 9 +-\n> 9 files changed, 105 insertions(+), 77 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index feb02d1806b2..968fae13bd5e 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span\n> buffer,\n> return;\n> }\n>\n> - deviceStatus.shutter_speed =\n> Exposure(exposureLines).get();\n> + deviceStatus.shutter_speed = Exposure(exposureLines);\n> deviceStatus.analogue_gain = Gain(gainCode);\n>\n> LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> diff --git a/src/ipa/raspberrypi/controller/agc_status.h\n> b/src/ipa/raspberrypi/controller/agc_status.h\n> index 10381c90a313..c893715af588 100644\n> --- a/src/ipa/raspberrypi/controller/agc_status.h\n> +++ b/src/ipa/raspberrypi/controller/agc_status.h\n> @@ -6,6 +6,8 @@\n> */\n> #pragma once\n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> // The AGC algorithm should post the following structure into the image's\n> // \"agc.status\" metadata.\n>\n> @@ -17,18 +19,20 @@ extern \"C\" {\n> // seen statistics and calculated meaningful values. The contents should\n> be\n> // ignored until then.\n>\n> +using libcamera::utils::Duration;\n> +\n> struct AgcStatus {\n> - double total_exposure_value; // value for all exposure and gain\n> for this image\n> - double target_exposure_value; // (unfiltered) target total\n> exposure AGC is aiming for\n> - double shutter_time;\n> + Duration total_exposure_value; // value for all exposure and gain\n> for this image\n> + Duration target_exposure_value; // (unfiltered) target total\n> exposure AGC is aiming for\n> + Duration shutter_time;\n> double analogue_gain;\n> char exposure_mode[32];\n> char constraint_mode[32];\n> char metering_mode[32];\n> double ev;\n> - double flicker_period;\n> + Duration flicker_period;\n> int floating_region_enable;\n> - double fixed_shutter;\n> + Duration fixed_shutter;\n> double fixed_analogue_gain;\n> double digital_gain;\n> int locked;\n> diff --git a/src/ipa/raspberrypi/controller/device_status.h\n> b/src/ipa/raspberrypi/controller/device_status.h\n> index aa08608b5d40..131b4cd344ee 100644\n> --- a/src/ipa/raspberrypi/controller/device_status.h\n> +++ b/src/ipa/raspberrypi/controller/device_status.h\n> @@ -6,6 +6,8 @@\n> */\n> #pragma once\n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> // Definition of \"device metadata\" which stores things like shutter time\n> and\n> // analogue gain that downstream control algorithms will want to know.\n>\n> @@ -14,8 +16,8 @@ extern \"C\" {\n> #endif\n>\n> struct DeviceStatus {\n> - // time shutter is open, in microseconds\n> - double shutter_speed;\n> + // time shutter is open\n> + libcamera::utils::Duration shutter_speed;\n> double analogue_gain;\n> // 1.0/distance-in-metres, or 0 if unknown\n> double lens_position;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 482eb3ef962d..46742d845034 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -21,6 +21,7 @@\n>\n> using namespace RPiController;\n> using namespace libcamera;\n> +using utils::operator<<;\n>\n> LOG_DEFINE_CATEGORY(RPiAgc)\n>\n> @@ -55,19 +56,27 @@ read_metering_modes(std::map AgcMeteringMode> &metering_modes,\n> return first;\n> }\n>\n> -static int read_double_list(std::vector &list,\n> - boost::property_tree::ptree const ¶ms)\n> +static int read_list(std::vector &list,\n> + boost::property_tree::ptree const ¶ms)\n> {\n> for (auto &p : params)\n> list.push_back(p.second.get_value());\n> return list.size();\n> }\n>\n> +static int read_list(std::vector &list,\n> + boost::property_tree::ptree const ¶ms)\n> +{\n> + for (auto &p : params)\n> + list.push_back(p.second.get_value() * 1us);\n> + return list.size();\n> +}\n> +\n> void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms)\n> {\n> int num_shutters =\n> - read_double_list(shutter, params.get_child(\"shutter\"));\n> - int num_ags = read_double_list(gain, params.get_child(\"gain\"));\n> + read_list(shutter, params.get_child(\"shutter\"));\n> + int num_ags = read_list(gain, params.get_child(\"gain\"));\n> if (num_shutters < 2 || num_ags < 2)\n> throw std::runtime_error(\n> \"AgcConfig: must have at least two entries in\n> exposure profile\");\n> @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const\n> ¶ms)\n> params.get(\"fast_reduce_threshold\", 0.4);\n> base_ev = params.get(\"base_ev\", 1.0);\n> // Start with quite a low value as ramping up is easier than\n> ramping down.\n> - default_exposure_time =\n> params.get(\"default_exposure_time\", 1000);\n> + default_exposure_time =\n> params.get(\"default_exposure_time\", 1000) * 1us;\n> default_analogue_gain =\n> params.get(\"default_analogue_gain\", 1.0);\n> }\n>\n> @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)\n> : AgcAlgorithm(controller), metering_mode_(nullptr),\n> exposure_mode_(nullptr), constraint_mode_(nullptr),\n> frame_count_(0), lock_count_(0),\n> - last_target_exposure_(0.0),\n> - ev_(1.0), flicker_period_(0.0),\n> - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> + last_target_exposure_(0s),\n> + ev_(1.0), flicker_period_(0s),\n> + max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)\n> {\n> memset(&awb_, 0, sizeof(awb_));\n> // Setting status_.total_exposure_value_ to zero initially tells us\n> @@ -203,7 +212,7 @@ void Agc::Pause()\n>\n> void Agc::Resume()\n> {\n> - fixed_shutter_ = 0;\n> + fixed_shutter_ = 0s;\n> fixed_analogue_gain_ = 0;\n> }\n>\n> @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)\n>\n> void Agc::SetFlickerPeriod(const Duration &flicker_period)\n> {\n> - flicker_period_ = flicker_period.get();\n> + flicker_period_ = flicker_period;\n> }\n>\n> void Agc::SetMaxShutter(const Duration &max_shutter)\n> {\n> - max_shutter_ = max_shutter.get();\n> + max_shutter_ = max_shutter;\n> }\n>\n> void Agc::SetFixedShutter(const Duration &fixed_shutter)\n> {\n> - fixed_shutter_ = fixed_shutter.get();\n> + fixed_shutter_ = fixed_shutter;\n> // Set this in case someone calls Pause() straight after.\n> status_.shutter_time = clipShutter(fixed_shutter_);\n> }\n> @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const\n> &camera_mode,\n> {\n> housekeepConfig();\n>\n> - double fixed_shutter = clipShutter(fixed_shutter_);\n> - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> + Duration fixed_shutter = clipShutter(fixed_shutter_);\n> + if (fixed_shutter && fixed_analogue_gain_) {\n> // We're going to reset the algorithm here with these\n> fixed values.\n>\n> fetchAwbStatus(metadata);\n> @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> // Process has run, so we have meaningful values.\n> DeviceStatus device_status;\n> if (image_metadata->Get(\"device.status\", device_status) ==\n> 0) {\n> - double actual_exposure =\n> device_status.shutter_speed *\n> -\n> device_status.analogue_gain;\n> + Duration actual_exposure =\n> device_status.shutter_speed *\n> +\n> device_status.analogue_gain;\n> if (actual_exposure) {\n> status_.digital_gain =\n> status_.total_exposure_value /\n> @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> std::min(status_.digital_gain,\n> 4.0));\n> LOG(RPiAgc, Debug) << \"Actual exposure \"\n> << actual_exposure;\n> LOG(RPiAgc, Debug) << \"Use digital_gain \"\n> << status_.digital_gain;\n> - LOG(RPiAgc, Debug) << \"Effective exposure\n> \" << actual_exposure * status_.digital_gain;\n> + LOG(RPiAgc, Debug) << \"Effective exposure \"\n> + << actual_exposure *\n> status_.digital_gain;\n> // Decide whether AEC/AGC has converged.\n> updateLockStatus(device_status);\n> }\n> @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const\n> &device_status)\n> const double RESET_MARGIN = 1.5;\n>\n> // Add 200us to the exposure time error to allow for line\n> quantisation.\n> - double exposure_error = last_device_status_.shutter_speed *\n> ERROR_FACTOR + 200;\n> + Duration exposure_error = last_device_status_.shutter_speed *\n> ERROR_FACTOR + 200us;\n> double gain_error = last_device_status_.analogue_gain *\n> ERROR_FACTOR;\n> - double target_error = last_target_exposure_ * ERROR_FACTOR;\n> + Duration target_error = last_target_exposure_ * ERROR_FACTOR;\n>\n> // Note that we don't know the exposure/gain limits of the sensor,\n> so\n> // the values we keep requesting may be unachievable. For this\n> reason\n> @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata\n> *image_metadata)\n> current_.analogue_gain = device_status->analogue_gain;\n> AgcStatus *agc_status =\n> image_metadata->GetLocked(\"agc.status\");\n> - current_.total_exposure = agc_status ?\n> agc_status->total_exposure_value : 0;\n> + current_.total_exposure = agc_status ?\n> agc_status->total_exposure_value : 0s;\n> current_.total_exposure_no_dg = current_.shutter *\n> current_.analogue_gain;\n> }\n>\n> @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics,\n> Metadata *image_metadata,\n>\n> void Agc::computeTargetExposure(double gain)\n> {\n> - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain !=\n> 0.0) {\n> + if (status_.fixed_shutter && status_.fixed_analogue_gain) {\n> // When ag and shutter are both fixed, we need to drive the\n> // total exposure so that we end up with a digital gain of\n> at least\n> // 1/min_colour_gain. Otherwise we'd desaturate channels\n> causing\n> @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)\n> target_.total_exposure = current_.total_exposure_no_dg *\n> gain;\n> // The final target exposure is also limited to what the\n> exposure\n> // mode allows.\n> - double max_shutter = status_.fixed_shutter != 0.0\n> + Duration max_shutter = status_.fixed_shutter\n> ? status_.fixed_shutter\n> : exposure_mode_->shutter.back();\n> max_shutter = clipShutter(max_shutter);\n> - double max_total_exposure =\n> + Duration max_total_exposure =\n> max_shutter *\n> (status_.fixed_analogue_gain != 0.0\n> ? status_.fixed_analogue_gain\n> @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)\n> if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||\n> frame_count_ <= config_.startup_frames)\n> speed = 1.0;\n> - if (filtered_.total_exposure == 0.0) {\n> + if (!filtered_.total_exposure) {\n> filtered_.total_exposure = target_.total_exposure;\n> filtered_.total_exposure_no_dg =\n> target_.total_exposure_no_dg;\n> } else {\n> @@ -674,9 +684,10 @@ void Agc::divideUpExposure()\n> // Sending the fixed shutter/gain cases through the same code may\n> seem\n> // unnecessary, but it will make more sense when extend this to\n> cover\n> // variable aperture.\n> - double exposure_value = filtered_.total_exposure_no_dg;\n> - double shutter_time, analogue_gain;\n> - shutter_time = status_.fixed_shutter != 0.0\n> + Duration exposure_value = filtered_.total_exposure_no_dg;\n> + Duration shutter_time;\n> + double analogue_gain;\n> + shutter_time = status_.fixed_shutter\n> ? status_.fixed_shutter\n> : exposure_mode_->shutter[0];\n> shutter_time = clipShutter(shutter_time);\n> @@ -686,8 +697,8 @@ void Agc::divideUpExposure()\n> if (shutter_time * analogue_gain < exposure_value) {\n> for (unsigned int stage = 1;\n> stage < exposure_mode_->gain.size(); stage++) {\n> - if (status_.fixed_shutter == 0.0) {\n> - double stage_shutter =\n> + if (!status_.fixed_shutter) {\n> + Duration stage_shutter =\n>\n> clipShutter(exposure_mode_->shutter[stage]);\n> if (stage_shutter * analogue_gain >=\n> exposure_value) {\n> @@ -713,12 +724,11 @@ void Agc::divideUpExposure()\n> << analogue_gain;\n> // Finally adjust shutter time for flicker avoidance (require both\n> // shutter and gain not to be fixed).\n> - if (status_.fixed_shutter == 0.0 &&\n> - status_.fixed_analogue_gain == 0.0 &&\n> - status_.flicker_period != 0.0) {\n> - int flicker_periods = shutter_time /\n> status_.flicker_period;\n> - if (flicker_periods > 0) {\n> - double new_shutter_time = flicker_periods *\n> status_.flicker_period;\n> + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&\n> + status_.flicker_period) {\n> + double flicker_periods = shutter_time /\n> status_.flicker_period;\n> + if (flicker_periods > 0.0) {\n> + Duration new_shutter_time = flicker_periods *\n> status_.flicker_period;\n> analogue_gain *= shutter_time / new_shutter_time;\n> // We should still not allow the ag to go over the\n> // largest value in the exposure mode. Note that\n> this\n> @@ -738,7 +748,7 @@ void Agc::divideUpExposure()\n> void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> {\n> status_.total_exposure_value = filtered_.total_exposure;\n> - status_.target_exposure_value = desaturate ? 0 :\n> target_.total_exposure_no_dg;\n> + status_.target_exposure_value = desaturate ? 0s :\n> target_.total_exposure_no_dg;\n> status_.shutter_time = filtered_.shutter;\n> status_.analogue_gain = filtered_.analogue_gain;\n> // Write to metadata as well, in case anyone wants to update the\n> camera\n> @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata,\n> bool desaturate)\n> << \" analogue gain \" << filtered_.analogue_gain;\n> }\n>\n> -double Agc::clipShutter(double shutter)\n> +Duration Agc::clipShutter(const Duration &shutter)\n> {\n> + Duration clip_shutter = shutter;\n> if (max_shutter_)\n> - shutter = std::min(shutter, max_shutter_);\n> - return shutter;\n> + clip_shutter = std::min(clip_shutter, max_shutter_);\n> + return clip_shutter;\n> }\n>\n> // Register algorithm with the system.\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 16a65959d90e..24a6610096c2 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -9,6 +9,8 @@\n> #include \n> #include \n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> #include \"../agc_algorithm.hpp\"\n> #include \"../agc_status.h\"\n> #include \"../pwl.hpp\"\n> @@ -22,13 +24,15 @@\n>\n> namespace RPiController {\n>\n> +using namespace std::literals::chrono_literals;\n> +\n> struct AgcMeteringMode {\n> double weights[AGC_STATS_SIZE];\n> void Read(boost::property_tree::ptree const ¶ms);\n> };\n>\n> struct AgcExposureMode {\n> - std::vector shutter;\n> + std::vector shutter;\n> std::vector gain;\n> void Read(boost::property_tree::ptree const ¶ms);\n> };\n> @@ -61,7 +65,7 @@ struct AgcConfig {\n> std::string default_exposure_mode;\n> std::string default_constraint_mode;\n> double base_ev;\n> - double default_exposure_time;\n> + Duration default_exposure_time;\n> double default_analogue_gain;\n> };\n>\n> @@ -101,19 +105,19 @@ private:\n> void filterExposure(bool desaturate);\n> void divideUpExposure();\n> void writeAndFinish(Metadata *image_metadata, bool desaturate);\n> - double clipShutter(double shutter);\n> + Duration clipShutter(const Duration &shutter);\n> AgcMeteringMode *metering_mode_;\n> AgcExposureMode *exposure_mode_;\n> AgcConstraintMode *constraint_mode_;\n> uint64_t frame_count_;\n> AwbStatus awb_;\n> struct ExposureValues {\n> - ExposureValues() : shutter(0), analogue_gain(0),\n> - total_exposure(0),\n> total_exposure_no_dg(0) {}\n> - double shutter;\n> + ExposureValues() : shutter(0s), analogue_gain(0),\n> + total_exposure(0s),\n> total_exposure_no_dg(0s) {}\n> + Duration shutter;\n> double analogue_gain;\n> - double total_exposure;\n> - double total_exposure_no_dg; // without digital gain\n> + Duration total_exposure;\n> + Duration total_exposure_no_dg; // without digital gain\n> };\n> ExposureValues current_; // values for the current frame\n> ExposureValues target_; // calculate the values we want here\n> @@ -121,15 +125,15 @@ private:\n> AgcStatus status_;\n> int lock_count_;\n> DeviceStatus last_device_status_;\n> - double last_target_exposure_;\n> + Duration last_target_exposure_;\n> // Below here the \"settings\" that applications can change.\n> std::string metering_mode_name_;\n> std::string exposure_mode_name_;\n> std::string constraint_mode_name_;\n> double ev_;\n> - double flicker_period_;\n> - double max_shutter_;\n> - double fixed_shutter_;\n> + Duration flicker_period_;\n> + Duration max_shutter_;\n> + Duration fixed_shutter_;\n> double fixed_analogue_gain_;\n> };\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> index f74381cab2b4..0fa6457c1376 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> @@ -16,6 +16,7 @@\n>\n> using namespace RPiController;\n> using namespace libcamera;\n> +using namespace std::literals::chrono_literals;\n>\n> LOG_DEFINE_CATEGORY(RPiLux)\n>\n> @@ -38,7 +39,7 @@ char const *Lux::Name() const\n> void Lux::Read(boost::property_tree::ptree const ¶ms)\n> {\n> reference_shutter_speed_ =\n> - params.get(\"reference_shutter_speed\");\n> + params.get(\"reference_shutter_speed\") * 1.0us;\n> reference_gain_ = params.get(\"reference_gain\");\n> reference_aperture_ = params.get(\"reference_aperture\",\n> 1.0);\n> reference_Y_ = params.get(\"reference_Y\");\n> @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)\n> void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> {\n> // set some initial values to shut the compiler up\n> - DeviceStatus device_status =\n> - { .shutter_speed = 1.0,\n> - .analogue_gain = 1.0,\n> - .lens_position = 0.0,\n> - .aperture = 0.0,\n> - .flash_intensity = 0.0 };\n> + DeviceStatus device_status = { .shutter_speed = 1.0ms,\n> + .analogue_gain = 1.0,\n> + .lens_position = 0.0,\n> + .aperture = 0.0,\n> + .flash_intensity = 0.0 };\n> if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> double current_gain = device_status.analogue_gain;\n> - double current_shutter_speed = device_status.shutter_speed;\n> double current_aperture = device_status.aperture;\n> if (current_aperture == 0)\n> current_aperture = current_aperture_;\n> @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata\n> *image_metadata)\n> double current_Y = sum / (double)num + .5;\n> double gain_ratio = reference_gain_ / current_gain;\n> double shutter_speed_ratio =\n> - reference_shutter_speed_ / current_shutter_speed;\n> + reference_shutter_speed_ /\n> device_status.shutter_speed;\n> double aperture_ratio = reference_aperture_ /\n> current_aperture;\n> double Y_ratio = current_Y * (65536 / num_bins) /\n> reference_Y_;\n> double estimated_lux = shutter_speed_ratio * gain_ratio *\n> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> index f9090484a136..45c844393e62 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> @@ -8,6 +8,8 @@\n>\n> #include \n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> #include \"../lux_status.h\"\n> #include \"../algorithm.hpp\"\n>\n> @@ -28,7 +30,7 @@ public:\n> private:\n> // These values define the conditions of the reference image,\n> against\n> // which we compare the new image.\n> - double reference_shutter_speed_; // in micro-seconds\n> + libcamera::utils::Duration reference_shutter_speed_;\n> double reference_gain_;\n> double reference_aperture_; // units of 1/f\n> double reference_Y_; // out of 65536\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f8f36420b076..b77230ded591 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,\n> ipa::RPi::StartConfig *startConf\n>\n> /* SwitchMode may supply updated exposure/gain values to use. */\n> AgcStatus agcStatus;\n> - agcStatus.shutter_time = 0.0;\n> + agcStatus.shutter_time = 0.0s;\n> agcStatus.analogue_gain = 0.0;\n>\n> metadata.Get(\"agc.status\", agcStatus);\n> - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=\n> 0.0) {\n> + if (agcStatus.shutter_time && agcStatus.analogue_gain) {\n> ControlList ctrls(sensorCtrls_);\n> applyAGC(&agcStatus, ctrls);\n> startConfig->controls = std::move(ctrls);\n> @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> /* Supply initial values for gain and exposure. */\n> ControlList ctrls(sensorCtrls_);\n> AgcStatus agcStatus;\n> - agcStatus.shutter_time =\n> DefaultExposureTime.get();\n> + agcStatus.shutter_time = DefaultExposureTime;\n> agcStatus.analogue_gain = DefaultAnalogueGain;\n> applyAGC(&agcStatus, ctrls);\n>\n> @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()\n> */\n> DeviceStatus *deviceStatus =\n> rpiMetadata_.GetLocked(\"device.status\");\n> if (deviceStatus) {\n> - libcameraMetadata_.set(controls::ExposureTime,\n> deviceStatus->shutter_speed);\n> + libcameraMetadata_.set(controls::ExposureTime,\n> +\n> deviceStatus->shutter_speed.get());\n> libcameraMetadata_.set(controls::AnalogueGain,\n> deviceStatus->analogue_gain);\n> }\n>\n> @@ -1017,7 +1018,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls)\n> int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>\n> - deviceStatus.shutter_speed =\n> helper_->Exposure(exposureLines).get();\n> + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>\n> LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> @@ -1103,7 +1104,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n>\n> /* GetVBlanking might clip exposure time to the fps limits. */\n> - Duration exposure = agcStatus->shutter_time * 1.0us;\n> + Duration exposure = agcStatus->shutter_time;\n> int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> int32_t exposureLines = helper_->ExposureLines(exposure);\n>\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 3131aa2d6666..5fa6f8366f3d 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -506,9 +506,14 @@ std::string libcameraSourcePath()\n> * loop, iterates over an indexed view of the \\a iterable\n> */\n>\n> +/**\n> + * \\typedef BaseDuration\n> + * \\brief Base struct for the \\a Duration helper class.\n> + */\n> +\n> /**\n> * \\class Duration\n> - * \\brief Helper for std::chrono::duration types. Derived from \\a\n> BaseDuration\n> + * \\brief Helper class for std::chrono::duration types.\n> */\n>\n> /**\n> @@ -517,7 +522,7 @@ std::string libcameraSourcePath()\n> * \\param[in] d The std::chrono::duration object to convert from.\n> *\n> * Constructs a \\a Duration object from a \\a std::chrono::duration object,\n> - * converting the representation to \\a BaseDuration type.\n> + * internally converting the representation to \\a BaseDuration type.\n> */\n>\n\nHmmm, this does not belong in patch 4/4. It should be in 1/4 instead.\nWill fix in the next revision. Sorry about that!\n\n\n>\n> /**\n> --\n> 2.25.1\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 64829C3200\n\tfor ;\n\tFri, 21 May 2021 11:33:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF84B6891F;\n\tFri, 21 May 2021 13:33:22 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26DC968911\n\tfor ;\n\tFri, 21 May 2021 13:33:21 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id 131so23559286ljj.3\n\tfor ;\n\tFri, 21 May 2021 04:33:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ZcCQaLne\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=cZNhbdy51w7fdNeUyOB2BYEgOR6RmJB9QgaZGY5vKoQ=;\n\tb=ZcCQaLnemn8z/wd4EAjxoGyRq+BR45nImwk5rUpxJp0WADpJmK08QkRntaVf42TLLK\n\tSwDnvDTwun8m4FVUT2vshd98YkNoIcOqAYfoURBe0t6zthgvIbxPNgyLQUWxLkIFPrWB\n\tGpowQ6iwwhQOnJJSskkSVjuFnFmRW+0iFZxG2jDKZrAKcohMViNy6LCaHpLSd+iR43u8\n\t3OgcQj3h62VLEAo7tDq88KrvNg8Qdbt/OK7dEj/w5TRmOHXh+xTnvHCaStZoPJi1Gqq9\n\tLQUpMFApe/4dT5pBVug+js928jaWYFnt/Yl2jb5Q6Fx3bRq0L8dRAuDeKdCASpJcN3Qt\n\to/lw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=cZNhbdy51w7fdNeUyOB2BYEgOR6RmJB9QgaZGY5vKoQ=;\n\tb=pn0H9SJzOOscC6WjBedgZSlkJd2KQPBynzfwLW3dCwG9Y0lGxlY5mQfEAc+4Nql0OZ\n\tahG1q37/NdEdBo0hQjmXc6ZZi++YbGrBP+ErGsQqadbH4jO6I/l1NK/JG71fNEyfsPTK\n\tPHqt2C6VSmvhkyEvMX8ShIaoHM4RqHaw72sQgzgUGLNelKLt1b8G2zoMj48aZGwNhKKz\n\tEis/Aupp4LBiQiiKbW/XSdLDT7hvpQc8EBItu8xEb1gJJ0Nxqid8r5ueu11DUt5slwJd\n\tdweV6ZOMhoCRxNalxua/wk9ueC2cq5XcTKLtvIP0+BviMPFsw2G7PA0MHfv72pdkiyIh\n\t897Q==","X-Gm-Message-State":"AOAM531hsc5J+lIQUTz7KO7s15A3saWFXGoO4kKvz7OpnEgdMCiFc5R8\n\tOgbhLhlrGAWKufTNUUdKewz4eSMI/DeTZciF9yzm7LiuZx8=","X-Google-Smtp-Source":"ABdhPJwPYq0BuhGWrBqixXwfXQAvLN1Ppxc6tfVWlvv0mKzqhDDFIKtkPf2SbnQj9rP5KuS+9hWazJ/n8Rf9bvW7yAo=","X-Received":"by 2002:a2e:7203:: with SMTP id n3mr5314977ljc.499.1621596799863;\n\tFri, 21 May 2021 04:33:19 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-5-naush@raspberrypi.com>","In-Reply-To":"<20210521080530.37591-5-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Fri, 21 May 2021 12:33:03 +0100","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"00000000000039a3b205c2d56d53\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17089,"web_url":"https://patchwork.libcamera.org/comment/17089/","msgid":"","date":"2021-05-21T11:36:52","subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 21 May 2021 at 12:19, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for all this work!\n>\n> On Fri, 21 May 2021 at 09:05, Naushir Patuck \n> wrote:\n> >\n> > Convert the core AGC and Lux controller code to use\n> > utils::Duration for all exposure time related variables and\n> > calculations.\n> >\n> > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus\n> > to use utils::Duration.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 2 +-\n> > src/ipa/raspberrypi/controller/agc_status.h | 14 +--\n> > .../raspberrypi/controller/device_status.h | 6 +-\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 +++++++++++--------\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---\n> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--\n> > src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 13 +--\n> > src/libcamera/utils.cpp | 9 +-\n> > 9 files changed, 105 insertions(+), 77 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index feb02d1806b2..968fae13bd5e 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > return;\n> > }\n> >\n> > - deviceStatus.shutter_speed =\n> Exposure(exposureLines).get();\n> > + deviceStatus.shutter_speed = Exposure(exposureLines);\n> > deviceStatus.analogue_gain = Gain(gainCode);\n> >\n> > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > diff --git a/src/ipa/raspberrypi/controller/agc_status.h\n> b/src/ipa/raspberrypi/controller/agc_status.h\n> > index 10381c90a313..c893715af588 100644\n> > --- a/src/ipa/raspberrypi/controller/agc_status.h\n> > +++ b/src/ipa/raspberrypi/controller/agc_status.h\n> > @@ -6,6 +6,8 @@\n> > */\n> > #pragma once\n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > // The AGC algorithm should post the following structure into the\n> image's\n> > // \"agc.status\" metadata.\n> >\n> > @@ -17,18 +19,20 @@ extern \"C\" {\n> > // seen statistics and calculated meaningful values. The contents\n> should be\n> > // ignored until then.\n> >\n> > +using libcamera::utils::Duration;\n> > +\n> > struct AgcStatus {\n> > - double total_exposure_value; // value for all exposure and gain\n> for this image\n> > - double target_exposure_value; // (unfiltered) target total\n> exposure AGC is aiming for\n> > - double shutter_time;\n> > + Duration total_exposure_value; // value for all exposure and\n> gain for this image\n> > + Duration target_exposure_value; // (unfiltered) target total\n> exposure AGC is aiming for\n> > + Duration shutter_time;\n> > double analogue_gain;\n> > char exposure_mode[32];\n> > char constraint_mode[32];\n> > char metering_mode[32];\n> > double ev;\n> > - double flicker_period;\n> > + Duration flicker_period;\n> > int floating_region_enable;\n> > - double fixed_shutter;\n> > + Duration fixed_shutter;\n> > double fixed_analogue_gain;\n> > double digital_gain;\n> > int locked;\n> > diff --git a/src/ipa/raspberrypi/controller/device_status.h\n> b/src/ipa/raspberrypi/controller/device_status.h\n> > index aa08608b5d40..131b4cd344ee 100644\n> > --- a/src/ipa/raspberrypi/controller/device_status.h\n> > +++ b/src/ipa/raspberrypi/controller/device_status.h\n> > @@ -6,6 +6,8 @@\n> > */\n> > #pragma once\n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > // Definition of \"device metadata\" which stores things like shutter\n> time and\n> > // analogue gain that downstream control algorithms will want to know.\n> >\n> > @@ -14,8 +16,8 @@ extern \"C\" {\n> > #endif\n> >\n> > struct DeviceStatus {\n> > - // time shutter is open, in microseconds\n> > - double shutter_speed;\n> > + // time shutter is open\n> > + libcamera::utils::Duration shutter_speed;\n> > double analogue_gain;\n> > // 1.0/distance-in-metres, or 0 if unknown\n> > double lens_position;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 482eb3ef962d..46742d845034 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -21,6 +21,7 @@\n> >\n> > using namespace RPiController;\n> > using namespace libcamera;\n> > +using utils::operator<<;\n>\n> Still can't say that I like this... just because someone wants to use\n> Durations, it seems like an awkward thing to make them remember. But\n> unless anyone has anything better....\n>\n> >\n> > LOG_DEFINE_CATEGORY(RPiAgc)\n> >\n> > @@ -55,19 +56,27 @@ read_metering_modes(std::map AgcMeteringMode> &metering_modes,\n> > return first;\n> > }\n> >\n> > -static int read_double_list(std::vector &list,\n> > - boost::property_tree::ptree const ¶ms)\n> > +static int read_list(std::vector &list,\n> > + boost::property_tree::ptree const ¶ms)\n> > {\n> > for (auto &p : params)\n> > list.push_back(p.second.get_value());\n> > return list.size();\n> > }\n> >\n> > +static int read_list(std::vector &list,\n> > + boost::property_tree::ptree const ¶ms)\n> > +{\n> > + for (auto &p : params)\n> > + list.push_back(p.second.get_value() * 1us);\n> > + return list.size();\n> > +}\n> > +\n> > void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms)\n> > {\n> > int num_shutters =\n> > - read_double_list(shutter, params.get_child(\"shutter\"));\n> > - int num_ags = read_double_list(gain, params.get_child(\"gain\"));\n> > + read_list(shutter, params.get_child(\"shutter\"));\n> > + int num_ags = read_list(gain, params.get_child(\"gain\"));\n> > if (num_shutters < 2 || num_ags < 2)\n> > throw std::runtime_error(\n> > \"AgcConfig: must have at least two entries in\n> exposure profile\");\n> > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree\n> const ¶ms)\n> > params.get(\"fast_reduce_threshold\", 0.4);\n> > base_ev = params.get(\"base_ev\", 1.0);\n> > // Start with quite a low value as ramping up is easier than\n> ramping down.\n> > - default_exposure_time =\n> params.get(\"default_exposure_time\", 1000);\n> > + default_exposure_time =\n> params.get(\"default_exposure_time\", 1000) * 1us;\n> > default_analogue_gain =\n> params.get(\"default_analogue_gain\", 1.0);\n> > }\n> >\n> > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)\n> > : AgcAlgorithm(controller), metering_mode_(nullptr),\n> > exposure_mode_(nullptr), constraint_mode_(nullptr),\n> > frame_count_(0), lock_count_(0),\n> > - last_target_exposure_(0.0),\n> > - ev_(1.0), flicker_period_(0.0),\n> > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> > + last_target_exposure_(0s),\n> > + ev_(1.0), flicker_period_(0s),\n> > + max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)\n> > {\n> > memset(&awb_, 0, sizeof(awb_));\n> > // Setting status_.total_exposure_value_ to zero initially tells\n> us\n> > @@ -203,7 +212,7 @@ void Agc::Pause()\n> >\n> > void Agc::Resume()\n> > {\n> > - fixed_shutter_ = 0;\n> > + fixed_shutter_ = 0s;\n> > fixed_analogue_gain_ = 0;\n> > }\n> >\n> > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)\n> >\n> > void Agc::SetFlickerPeriod(const Duration &flicker_period)\n> > {\n> > - flicker_period_ = flicker_period.get();\n> > + flicker_period_ = flicker_period;\n> > }\n> >\n> > void Agc::SetMaxShutter(const Duration &max_shutter)\n> > {\n> > - max_shutter_ = max_shutter.get();\n> > + max_shutter_ = max_shutter;\n> > }\n> >\n> > void Agc::SetFixedShutter(const Duration &fixed_shutter)\n> > {\n> > - fixed_shutter_ = fixed_shutter.get();\n> > + fixed_shutter_ = fixed_shutter;\n> > // Set this in case someone calls Pause() straight after.\n> > status_.shutter_time = clipShutter(fixed_shutter_);\n> > }\n> > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n> const &camera_mode,\n> > {\n> > housekeepConfig();\n> >\n> > - double fixed_shutter = clipShutter(fixed_shutter_);\n> > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> > + Duration fixed_shutter = clipShutter(fixed_shutter_);\n> > + if (fixed_shutter && fixed_analogue_gain_) {\n> > // We're going to reset the algorithm here with these\n> fixed values.\n> >\n> > fetchAwbStatus(metadata);\n> > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > // Process has run, so we have meaningful values.\n> > DeviceStatus device_status;\n> > if (image_metadata->Get(\"device.status\", device_status)\n> == 0) {\n> > - double actual_exposure =\n> device_status.shutter_speed *\n> > -\n> device_status.analogue_gain;\n> > + Duration actual_exposure =\n> device_status.shutter_speed *\n> > +\n> device_status.analogue_gain;\n> > if (actual_exposure) {\n> > status_.digital_gain =\n> > status_.total_exposure_value /\n> > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > std::min(status_.digital_gain,\n> 4.0));\n> > LOG(RPiAgc, Debug) << \"Actual exposure \"\n> << actual_exposure;\n> > LOG(RPiAgc, Debug) << \"Use digital_gain\n> \" << status_.digital_gain;\n> > - LOG(RPiAgc, Debug) << \"Effective\n> exposure \" << actual_exposure * status_.digital_gain;\n> > + LOG(RPiAgc, Debug) << \"Effective\n> exposure \"\n> > + << actual_exposure *\n> status_.digital_gain;\n> > // Decide whether AEC/AGC has converged.\n> > updateLockStatus(device_status);\n> > }\n> > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const\n> &device_status)\n> > const double RESET_MARGIN = 1.5;\n> >\n> > // Add 200us to the exposure time error to allow for line\n> quantisation.\n> > - double exposure_error = last_device_status_.shutter_speed *\n> ERROR_FACTOR + 200;\n> > + Duration exposure_error = last_device_status_.shutter_speed *\n> ERROR_FACTOR + 200us;\n> > double gain_error = last_device_status_.analogue_gain *\n> ERROR_FACTOR;\n> > - double target_error = last_target_exposure_ * ERROR_FACTOR;\n> > + Duration target_error = last_target_exposure_ * ERROR_FACTOR;\n> >\n> > // Note that we don't know the exposure/gain limits of the\n> sensor, so\n> > // the values we keep requesting may be unachievable. For this\n> reason\n> > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata\n> *image_metadata)\n> > current_.analogue_gain = device_status->analogue_gain;\n> > AgcStatus *agc_status =\n> > image_metadata->GetLocked(\"agc.status\");\n> > - current_.total_exposure = agc_status ?\n> agc_status->total_exposure_value : 0;\n> > + current_.total_exposure = agc_status ?\n> agc_status->total_exposure_value : 0s;\n> > current_.total_exposure_no_dg = current_.shutter *\n> current_.analogue_gain;\n> > }\n> >\n> > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics,\n> Metadata *image_metadata,\n> >\n> > void Agc::computeTargetExposure(double gain)\n> > {\n> > - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain\n> != 0.0) {\n> > + if (status_.fixed_shutter && status_.fixed_analogue_gain) {\n> > // When ag and shutter are both fixed, we need to drive\n> the\n> > // total exposure so that we end up with a digital gain\n> of at least\n> > // 1/min_colour_gain. Otherwise we'd desaturate channels\n> causing\n> > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)\n> > target_.total_exposure = current_.total_exposure_no_dg *\n> gain;\n> > // The final target exposure is also limited to what the\n> exposure\n> > // mode allows.\n> > - double max_shutter = status_.fixed_shutter != 0.0\n> > + Duration max_shutter = status_.fixed_shutter\n> > ? status_.fixed_shutter\n> > : exposure_mode_->shutter.back();\n> > max_shutter = clipShutter(max_shutter);\n> > - double max_total_exposure =\n> > + Duration max_total_exposure =\n> > max_shutter *\n> > (status_.fixed_analogue_gain != 0.0\n> > ? status_.fixed_analogue_gain\n> > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)\n> > if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||\n> > frame_count_ <= config_.startup_frames)\n> > speed = 1.0;\n> > - if (filtered_.total_exposure == 0.0) {\n> > + if (!filtered_.total_exposure) {\n> > filtered_.total_exposure = target_.total_exposure;\n> > filtered_.total_exposure_no_dg =\n> target_.total_exposure_no_dg;\n> > } else {\n> > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()\n> > // Sending the fixed shutter/gain cases through the same code\n> may seem\n> > // unnecessary, but it will make more sense when extend this to\n> cover\n> > // variable aperture.\n> > - double exposure_value = filtered_.total_exposure_no_dg;\n> > - double shutter_time, analogue_gain;\n> > - shutter_time = status_.fixed_shutter != 0.0\n> > + Duration exposure_value = filtered_.total_exposure_no_dg;\n> > + Duration shutter_time;\n> > + double analogue_gain;\n> > + shutter_time = status_.fixed_shutter\n> > ? status_.fixed_shutter\n> > : exposure_mode_->shutter[0];\n> > shutter_time = clipShutter(shutter_time);\n> > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()\n> > if (shutter_time * analogue_gain < exposure_value) {\n> > for (unsigned int stage = 1;\n> > stage < exposure_mode_->gain.size(); stage++) {\n> > - if (status_.fixed_shutter == 0.0) {\n> > - double stage_shutter =\n> > + if (!status_.fixed_shutter) {\n> > + Duration stage_shutter =\n> >\n> clipShutter(exposure_mode_->shutter[stage]);\n> > if (stage_shutter * analogue_gain >=\n> > exposure_value) {\n> > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()\n> > << analogue_gain;\n> > // Finally adjust shutter time for flicker avoidance (require\n> both\n> > // shutter and gain not to be fixed).\n> > - if (status_.fixed_shutter == 0.0 &&\n> > - status_.fixed_analogue_gain == 0.0 &&\n> > - status_.flicker_period != 0.0) {\n> > - int flicker_periods = shutter_time /\n> status_.flicker_period;\n> > - if (flicker_periods > 0) {\n> > - double new_shutter_time = flicker_periods *\n> status_.flicker_period;\n> > + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&\n> > + status_.flicker_period) {\n> > + double flicker_periods = shutter_time /\n> status_.flicker_period;\n> > + if (flicker_periods > 0.0) {\n> > + Duration new_shutter_time = flicker_periods *\n> status_.flicker_period;\n>\n> Just wondering if this will behave the same now that flicker_periods\n> is a double. Won't it now hold fractional values with the result that\n> new_shutter_time isn't an exact multiple of the period?\n>\n\nQuite right, it can be fractional! I should just keep flicker_periods as\nan int to avoid\nthis. Thanks for pointing that out.\n\n\n> > analogue_gain *= shutter_time / new_shutter_time;\n> > // We should still not allow the ag to go over\n> the\n> > // largest value in the exposure mode. Note that\n> this\n> > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()\n> > void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> > {\n> > status_.total_exposure_value = filtered_.total_exposure;\n> > - status_.target_exposure_value = desaturate ? 0 :\n> target_.total_exposure_no_dg;\n> > + status_.target_exposure_value = desaturate ? 0s :\n> target_.total_exposure_no_dg;\n> > status_.shutter_time = filtered_.shutter;\n> > status_.analogue_gain = filtered_.analogue_gain;\n> > // Write to metadata as well, in case anyone wants to update the\n> camera\n> > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata,\n> bool desaturate)\n> > << \" analogue gain \" <<\n> filtered_.analogue_gain;\n> > }\n> >\n> > -double Agc::clipShutter(double shutter)\n> > +Duration Agc::clipShutter(const Duration &shutter)\n> > {\n> > + Duration clip_shutter = shutter;\n>\n> Given that we copy it anyway I wonder if we might have left this as\n> call-by-value, resulting in fewer changes. But it's only the teeniest\n> of nit-picks!\n>\n> If you can put my mind at rest over the flicker period thing:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n>\n> > if (max_shutter_)\n> > - shutter = std::min(shutter, max_shutter_);\n> > - return shutter;\n> > + clip_shutter = std::min(clip_shutter, max_shutter_);\n> > + return clip_shutter;\n> > }\n> >\n> > // Register algorithm with the system.\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index 16a65959d90e..24a6610096c2 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -9,6 +9,8 @@\n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > #include \"../agc_algorithm.hpp\"\n> > #include \"../agc_status.h\"\n> > #include \"../pwl.hpp\"\n> > @@ -22,13 +24,15 @@\n> >\n> > namespace RPiController {\n> >\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > struct AgcMeteringMode {\n> > double weights[AGC_STATS_SIZE];\n> > void Read(boost::property_tree::ptree const ¶ms);\n> > };\n> >\n> > struct AgcExposureMode {\n> > - std::vector shutter;\n> > + std::vector shutter;\n> > std::vector gain;\n> > void Read(boost::property_tree::ptree const ¶ms);\n> > };\n> > @@ -61,7 +65,7 @@ struct AgcConfig {\n> > std::string default_exposure_mode;\n> > std::string default_constraint_mode;\n> > double base_ev;\n> > - double default_exposure_time;\n> > + Duration default_exposure_time;\n> > double default_analogue_gain;\n> > };\n> >\n> > @@ -101,19 +105,19 @@ private:\n> > void filterExposure(bool desaturate);\n> > void divideUpExposure();\n> > void writeAndFinish(Metadata *image_metadata, bool desaturate);\n> > - double clipShutter(double shutter);\n> > + Duration clipShutter(const Duration &shutter);\n> > AgcMeteringMode *metering_mode_;\n> > AgcExposureMode *exposure_mode_;\n> > AgcConstraintMode *constraint_mode_;\n> > uint64_t frame_count_;\n> > AwbStatus awb_;\n> > struct ExposureValues {\n> > - ExposureValues() : shutter(0), analogue_gain(0),\n> > - total_exposure(0),\n> total_exposure_no_dg(0) {}\n> > - double shutter;\n> > + ExposureValues() : shutter(0s), analogue_gain(0),\n> > + total_exposure(0s),\n> total_exposure_no_dg(0s) {}\n> > + Duration shutter;\n> > double analogue_gain;\n> > - double total_exposure;\n> > - double total_exposure_no_dg; // without digital gain\n> > + Duration total_exposure;\n> > + Duration total_exposure_no_dg; // without digital gain\n> > };\n> > ExposureValues current_; // values for the current frame\n> > ExposureValues target_; // calculate the values we want here\n> > @@ -121,15 +125,15 @@ private:\n> > AgcStatus status_;\n> > int lock_count_;\n> > DeviceStatus last_device_status_;\n> > - double last_target_exposure_;\n> > + Duration last_target_exposure_;\n> > // Below here the \"settings\" that applications can change.\n> > std::string metering_mode_name_;\n> > std::string exposure_mode_name_;\n> > std::string constraint_mode_name_;\n> > double ev_;\n> > - double flicker_period_;\n> > - double max_shutter_;\n> > - double fixed_shutter_;\n> > + Duration flicker_period_;\n> > + Duration max_shutter_;\n> > + Duration fixed_shutter_;\n> > double fixed_analogue_gain_;\n> > };\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > index f74381cab2b4..0fa6457c1376 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > @@ -16,6 +16,7 @@\n> >\n> > using namespace RPiController;\n> > using namespace libcamera;\n> > +using namespace std::literals::chrono_literals;\n> >\n> > LOG_DEFINE_CATEGORY(RPiLux)\n> >\n> > @@ -38,7 +39,7 @@ char const *Lux::Name() const\n> > void Lux::Read(boost::property_tree::ptree const ¶ms)\n> > {\n> > reference_shutter_speed_ =\n> > - params.get(\"reference_shutter_speed\");\n> > + params.get(\"reference_shutter_speed\") * 1.0us;\n> > reference_gain_ = params.get(\"reference_gain\");\n> > reference_aperture_ = params.get(\"reference_aperture\",\n> 1.0);\n> > reference_Y_ = params.get(\"reference_Y\");\n> > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)\n> > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> > {\n> > // set some initial values to shut the compiler up\n> > - DeviceStatus device_status =\n> > - { .shutter_speed = 1.0,\n> > - .analogue_gain = 1.0,\n> > - .lens_position = 0.0,\n> > - .aperture = 0.0,\n> > - .flash_intensity = 0.0 };\n> > + DeviceStatus device_status = { .shutter_speed = 1.0ms,\n> > + .analogue_gain = 1.0,\n> > + .lens_position = 0.0,\n> > + .aperture = 0.0,\n> > + .flash_intensity = 0.0 };\n> > if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> > double current_gain = device_status.analogue_gain;\n> > - double current_shutter_speed =\n> device_status.shutter_speed;\n> > double current_aperture = device_status.aperture;\n> > if (current_aperture == 0)\n> > current_aperture = current_aperture_;\n> > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata\n> *image_metadata)\n> > double current_Y = sum / (double)num + .5;\n> > double gain_ratio = reference_gain_ / current_gain;\n> > double shutter_speed_ratio =\n> > - reference_shutter_speed_ / current_shutter_speed;\n> > + reference_shutter_speed_ /\n> device_status.shutter_speed;\n> > double aperture_ratio = reference_aperture_ /\n> current_aperture;\n> > double Y_ratio = current_Y * (65536 / num_bins) /\n> reference_Y_;\n> > double estimated_lux = shutter_speed_ratio * gain_ratio *\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > index f9090484a136..45c844393e62 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > @@ -8,6 +8,8 @@\n> >\n> > #include \n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > #include \"../lux_status.h\"\n> > #include \"../algorithm.hpp\"\n> >\n> > @@ -28,7 +30,7 @@ public:\n> > private:\n> > // These values define the conditions of the reference image,\n> against\n> > // which we compare the new image.\n> > - double reference_shutter_speed_; // in micro-seconds\n> > + libcamera::utils::Duration reference_shutter_speed_;\n> > double reference_gain_;\n> > double reference_aperture_; // units of 1/f\n> > double reference_Y_; // out of 65536\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f8f36420b076..b77230ded591 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,\n> ipa::RPi::StartConfig *startConf\n> >\n> > /* SwitchMode may supply updated exposure/gain values to use. */\n> > AgcStatus agcStatus;\n> > - agcStatus.shutter_time = 0.0;\n> > + agcStatus.shutter_time = 0.0s;\n> > agcStatus.analogue_gain = 0.0;\n> >\n> > metadata.Get(\"agc.status\", agcStatus);\n> > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=\n> 0.0) {\n> > + if (agcStatus.shutter_time && agcStatus.analogue_gain) {\n> > ControlList ctrls(sensorCtrls_);\n> > applyAGC(&agcStatus, ctrls);\n> > startConfig->controls = std::move(ctrls);\n> > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > AgcStatus agcStatus;\n> > - agcStatus.shutter_time =\n> DefaultExposureTime.get();\n> > + agcStatus.shutter_time = DefaultExposureTime;\n> > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > applyAGC(&agcStatus, ctrls);\n> >\n> > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()\n> > */\n> > DeviceStatus *deviceStatus =\n> rpiMetadata_.GetLocked(\"device.status\");\n> > if (deviceStatus) {\n> > - libcameraMetadata_.set(controls::ExposureTime,\n> deviceStatus->shutter_speed);\n> > + libcameraMetadata_.set(controls::ExposureTime,\n> > +\n> deviceStatus->shutter_speed.get());\n> > libcameraMetadata_.set(controls::AnalogueGain,\n> deviceStatus->analogue_gain);\n> > }\n> >\n> > @@ -1017,7 +1018,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls)\n> > int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >\n> > - deviceStatus.shutter_speed =\n> helper_->Exposure(exposureLines).get();\n> > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >\n> > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > @@ -1103,7 +1104,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> >\n> > /* GetVBlanking might clip exposure time to the fps limits. */\n> > - Duration exposure = agcStatus->shutter_time * 1.0us;\n> > + Duration exposure = agcStatus->shutter_time;\n> > int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> > int32_t exposureLines = helper_->ExposureLines(exposure);\n> >\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 3131aa2d6666..5fa6f8366f3d 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()\n> > * loop, iterates over an indexed view of the \\a iterable\n> > */\n> >\n> > +/**\n> > + * \\typedef BaseDuration\n> > + * \\brief Base struct for the \\a Duration helper class.\n> > + */\n> > +\n> > /**\n> > * \\class Duration\n> > - * \\brief Helper for std::chrono::duration types. Derived from \\a\n> BaseDuration\n> > + * \\brief Helper class for std::chrono::duration types.\n> > */\n> >\n> > /**\n> > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()\n> > * \\param[in] d The std::chrono::duration object to convert from.\n> > *\n> > * Constructs a \\a Duration object from a \\a std::chrono::duration\n> object,\n> > - * converting the representation to \\a BaseDuration type.\n> > + * internally converting the representation to \\a BaseDuration type.\n> > */\n> >\n> > /**\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1464CC31FF\n\tfor ;\n\tFri, 21 May 2021 11:37:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67EDF6891E;\n\tFri, 21 May 2021 13:37:10 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D24B68911\n\tfor ;\n\tFri, 21 May 2021 13:37:09 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id m11so29229093lfg.3\n\tfor ;\n\tFri, 21 May 2021 04:37:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"F4pmoFwp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=H5pYQG4J8Pok/LGPqvBd0GMPESu1cgRdtpEYAFRkUvo=;\n\tb=F4pmoFwp0WxrhgvoS27kLv0II4/qqnBL5v/jot1YI+rKifXUpwgtStu2G05JCqHLED\n\t7LjBXm1t+uaGFxGz1FmiIQvAy8sLKbM9gVm9xUn6bDDtbVZbg8M2LnXxxzL8iwJwCQJe\n\tF8ZINmA3+asnaqZurmU9DMyA23t3Smer2mwS6aZ5ahDXJD+gNypyjOvbTAKkGRT7Q2bg\n\tWlenfU4Ygggj7fi3/9+vO95rGvvfIv2JhENlmHF0adXkw/TgOup85l4onox/KpWxIKFl\n\tQexcTc0OsDYhi8UDgA7fIkB654G6ouwLRavxdGh+2C69xJ+rwx7mhPbpTOTUMc9Di77r\n\tuzFg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=H5pYQG4J8Pok/LGPqvBd0GMPESu1cgRdtpEYAFRkUvo=;\n\tb=ELwkcS7P/vOPCPySDCFsQUPBtV1VvSJWAoZ9CZuL5wQ1j08UjXlGMSxADjAw8isM1V\n\tC/kNQry4mbXXfZsGLmfWjlu6ut8a5IZ4Cxe/pViISv3S2J4aBdHx/j6ERu73H0lU6gD5\n\tr9TGh0PDVz88zwZV2Sl2jaJHL5OBbb107RxA7xqyylUXYaOsRRq1YZXPwPDbPfA5vdOf\n\taUI4m8TWz46/NABeQCiMIlvVE51ClOvR3/JKk38T/WiAJSRo4NB/SdFPiA6GP12WZRyb\n\tti8KhKI3DVtidumQq0mv2n8Y7ey4JAVzEhvUfuNwaGamS1j/byPXemgWXHJEsBLDVqlt\n\tf4GQ==","X-Gm-Message-State":"AOAM530hGQdhSlnrJEr1390JXeLKydhAARgEpK04F77lHMYBsDYWxHe/\n\t6Xu35xyM0BCXRjtwKYe0zYn4DIPrIADdCCgZoHmTa1jg2qU=","X-Google-Smtp-Source":"ABdhPJwHE3ZreSpWAP+uFn6DBDcr4997VreQIGxPr3am9obOngx2/FcTrLvC0ngnOUt4E0sWhE/WtHvE8pB5MXe6NWE=","X-Received":"by 2002:ac2:544e:: with SMTP id\n\td14mr1921969lfn.567.1621597028404; \n\tFri, 21 May 2021 04:37:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 21 May 2021 12:36:52 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000d8e0b805c2d57a9e\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17095,"web_url":"https://patchwork.libcamera.org/comment/17095/","msgid":"","date":"2021-05-21T11:58:57","subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 21 May 2021 at 12:36, Naushir Patuck wrote:\n\n> On Fri, 21 May 2021 at 12:19, David Plowman \n> wrote:\n>\n>> Hi Naush\n>>\n>> Thanks for all this work!\n>>\n>> On Fri, 21 May 2021 at 09:05, Naushir Patuck \n>> wrote:\n>> >\n>> > Convert the core AGC and Lux controller code to use\n>> > utils::Duration for all exposure time related variables and\n>> > calculations.\n>> >\n>> > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus\n>> > to use utils::Duration.\n>> >\n>> > Signed-off-by: Naushir Patuck \n>> > ---\n>> > src/ipa/raspberrypi/cam_helper.cpp | 2 +-\n>> > src/ipa/raspberrypi/controller/agc_status.h | 14 +--\n>> > .../raspberrypi/controller/device_status.h | 6 +-\n>> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 +++++++++++--------\n>> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---\n>> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--\n>> > src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-\n>> > src/ipa/raspberrypi/raspberrypi.cpp | 13 +--\n>> > src/libcamera/utils.cpp | 9 +-\n>> > 9 files changed, 105 insertions(+), 77 deletions(-)\n>> >\n>> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n>> b/src/ipa/raspberrypi/cam_helper.cpp\n>> > index feb02d1806b2..968fae13bd5e 100644\n>> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n>> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n>> > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span> uint8_t> buffer,\n>> > return;\n>> > }\n>> >\n>> > - deviceStatus.shutter_speed =\n>> Exposure(exposureLines).get();\n>> > + deviceStatus.shutter_speed = Exposure(exposureLines);\n>> > deviceStatus.analogue_gain = Gain(gainCode);\n>> >\n>> > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n>> > diff --git a/src/ipa/raspberrypi/controller/agc_status.h\n>> b/src/ipa/raspberrypi/controller/agc_status.h\n>> > index 10381c90a313..c893715af588 100644\n>> > --- a/src/ipa/raspberrypi/controller/agc_status.h\n>> > +++ b/src/ipa/raspberrypi/controller/agc_status.h\n>> > @@ -6,6 +6,8 @@\n>> > */\n>> > #pragma once\n>> >\n>> > +#include \"libcamera/internal/utils.h\"\n>> > +\n>> > // The AGC algorithm should post the following structure into the\n>> image's\n>> > // \"agc.status\" metadata.\n>> >\n>> > @@ -17,18 +19,20 @@ extern \"C\" {\n>> > // seen statistics and calculated meaningful values. The contents\n>> should be\n>> > // ignored until then.\n>> >\n>> > +using libcamera::utils::Duration;\n>> > +\n>> > struct AgcStatus {\n>> > - double total_exposure_value; // value for all exposure and gain\n>> for this image\n>> > - double target_exposure_value; // (unfiltered) target total\n>> exposure AGC is aiming for\n>> > - double shutter_time;\n>> > + Duration total_exposure_value; // value for all exposure and\n>> gain for this image\n>> > + Duration target_exposure_value; // (unfiltered) target total\n>> exposure AGC is aiming for\n>> > + Duration shutter_time;\n>> > double analogue_gain;\n>> > char exposure_mode[32];\n>> > char constraint_mode[32];\n>> > char metering_mode[32];\n>> > double ev;\n>> > - double flicker_period;\n>> > + Duration flicker_period;\n>> > int floating_region_enable;\n>> > - double fixed_shutter;\n>> > + Duration fixed_shutter;\n>> > double fixed_analogue_gain;\n>> > double digital_gain;\n>> > int locked;\n>> > diff --git a/src/ipa/raspberrypi/controller/device_status.h\n>> b/src/ipa/raspberrypi/controller/device_status.h\n>> > index aa08608b5d40..131b4cd344ee 100644\n>> > --- a/src/ipa/raspberrypi/controller/device_status.h\n>> > +++ b/src/ipa/raspberrypi/controller/device_status.h\n>> > @@ -6,6 +6,8 @@\n>> > */\n>> > #pragma once\n>> >\n>> > +#include \"libcamera/internal/utils.h\"\n>> > +\n>> > // Definition of \"device metadata\" which stores things like shutter\n>> time and\n>> > // analogue gain that downstream control algorithms will want to know.\n>> >\n>> > @@ -14,8 +16,8 @@ extern \"C\" {\n>> > #endif\n>> >\n>> > struct DeviceStatus {\n>> > - // time shutter is open, in microseconds\n>> > - double shutter_speed;\n>> > + // time shutter is open\n>> > + libcamera::utils::Duration shutter_speed;\n>> > double analogue_gain;\n>> > // 1.0/distance-in-metres, or 0 if unknown\n>> > double lens_position;\n>> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n>> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n>> > index 482eb3ef962d..46742d845034 100644\n>> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n>> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n>> > @@ -21,6 +21,7 @@\n>> >\n>> > using namespace RPiController;\n>> > using namespace libcamera;\n>> > +using utils::operator<<;\n>>\n>> Still can't say that I like this... just because someone wants to use\n>> Durations, it seems like an awkward thing to make them remember. But\n>> unless anyone has anything better....\n>>\n>\nSorry, I forgot to add this comment in the last email.\nOne option to make things cleaner is to move the operator<< into the\nlibcamera namespace,\navoiding the need to add the above using directive everywhere.\n\n\n>\n>> >\n>> > LOG_DEFINE_CATEGORY(RPiAgc)\n>> >\n>> > @@ -55,19 +56,27 @@ read_metering_modes(std::map> AgcMeteringMode> &metering_modes,\n>> > return first;\n>> > }\n>> >\n>> > -static int read_double_list(std::vector &list,\n>> > - boost::property_tree::ptree const ¶ms)\n>> > +static int read_list(std::vector &list,\n>> > + boost::property_tree::ptree const ¶ms)\n>> > {\n>> > for (auto &p : params)\n>> > list.push_back(p.second.get_value());\n>> > return list.size();\n>> > }\n>> >\n>> > +static int read_list(std::vector &list,\n>> > + boost::property_tree::ptree const ¶ms)\n>> > +{\n>> > + for (auto &p : params)\n>> > + list.push_back(p.second.get_value() * 1us);\n>> > + return list.size();\n>> > +}\n>> > +\n>> > void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms)\n>> > {\n>> > int num_shutters =\n>> > - read_double_list(shutter, params.get_child(\"shutter\"));\n>> > - int num_ags = read_double_list(gain, params.get_child(\"gain\"));\n>> > + read_list(shutter, params.get_child(\"shutter\"));\n>> > + int num_ags = read_list(gain, params.get_child(\"gain\"));\n>> > if (num_shutters < 2 || num_ags < 2)\n>> > throw std::runtime_error(\n>> > \"AgcConfig: must have at least two entries in\n>> exposure profile\");\n>> > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree\n>> const ¶ms)\n>> > params.get(\"fast_reduce_threshold\", 0.4);\n>> > base_ev = params.get(\"base_ev\", 1.0);\n>> > // Start with quite a low value as ramping up is easier than\n>> ramping down.\n>> > - default_exposure_time =\n>> params.get(\"default_exposure_time\", 1000);\n>> > + default_exposure_time =\n>> params.get(\"default_exposure_time\", 1000) * 1us;\n>> > default_analogue_gain =\n>> params.get(\"default_analogue_gain\", 1.0);\n>> > }\n>> >\n>> > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)\n>> > : AgcAlgorithm(controller), metering_mode_(nullptr),\n>> > exposure_mode_(nullptr), constraint_mode_(nullptr),\n>> > frame_count_(0), lock_count_(0),\n>> > - last_target_exposure_(0.0),\n>> > - ev_(1.0), flicker_period_(0.0),\n>> > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n>> > + last_target_exposure_(0s),\n>> > + ev_(1.0), flicker_period_(0s),\n>> > + max_shutter_(0s), fixed_shutter_(0s),\n>> fixed_analogue_gain_(0.0)\n>> > {\n>> > memset(&awb_, 0, sizeof(awb_));\n>> > // Setting status_.total_exposure_value_ to zero initially\n>> tells us\n>> > @@ -203,7 +212,7 @@ void Agc::Pause()\n>> >\n>> > void Agc::Resume()\n>> > {\n>> > - fixed_shutter_ = 0;\n>> > + fixed_shutter_ = 0s;\n>> > fixed_analogue_gain_ = 0;\n>> > }\n>> >\n>> > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)\n>> >\n>> > void Agc::SetFlickerPeriod(const Duration &flicker_period)\n>> > {\n>> > - flicker_period_ = flicker_period.get();\n>> > + flicker_period_ = flicker_period;\n>> > }\n>> >\n>> > void Agc::SetMaxShutter(const Duration &max_shutter)\n>> > {\n>> > - max_shutter_ = max_shutter.get();\n>> > + max_shutter_ = max_shutter;\n>> > }\n>> >\n>> > void Agc::SetFixedShutter(const Duration &fixed_shutter)\n>> > {\n>> > - fixed_shutter_ = fixed_shutter.get();\n>> > + fixed_shutter_ = fixed_shutter;\n>> > // Set this in case someone calls Pause() straight after.\n>> > status_.shutter_time = clipShutter(fixed_shutter_);\n>> > }\n>> > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n>> const &camera_mode,\n>> > {\n>> > housekeepConfig();\n>> >\n>> > - double fixed_shutter = clipShutter(fixed_shutter_);\n>> > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n>> > + Duration fixed_shutter = clipShutter(fixed_shutter_);\n>> > + if (fixed_shutter && fixed_analogue_gain_) {\n>> > // We're going to reset the algorithm here with these\n>> fixed values.\n>> >\n>> > fetchAwbStatus(metadata);\n>> > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)\n>> > // Process has run, so we have meaningful values.\n>> > DeviceStatus device_status;\n>> > if (image_metadata->Get(\"device.status\", device_status)\n>> == 0) {\n>> > - double actual_exposure =\n>> device_status.shutter_speed *\n>> > -\n>> device_status.analogue_gain;\n>> > + Duration actual_exposure =\n>> device_status.shutter_speed *\n>> > +\n>> device_status.analogue_gain;\n>> > if (actual_exposure) {\n>> > status_.digital_gain =\n>> > status_.total_exposure_value /\n>> > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)\n>> > std::min(status_.digital_gain,\n>> 4.0));\n>> > LOG(RPiAgc, Debug) << \"Actual exposure\n>> \" << actual_exposure;\n>> > LOG(RPiAgc, Debug) << \"Use digital_gain\n>> \" << status_.digital_gain;\n>> > - LOG(RPiAgc, Debug) << \"Effective\n>> exposure \" << actual_exposure * status_.digital_gain;\n>> > + LOG(RPiAgc, Debug) << \"Effective\n>> exposure \"\n>> > + << actual_exposure *\n>> status_.digital_gain;\n>> > // Decide whether AEC/AGC has converged.\n>> > updateLockStatus(device_status);\n>> > }\n>> > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const\n>> &device_status)\n>> > const double RESET_MARGIN = 1.5;\n>> >\n>> > // Add 200us to the exposure time error to allow for line\n>> quantisation.\n>> > - double exposure_error = last_device_status_.shutter_speed *\n>> ERROR_FACTOR + 200;\n>> > + Duration exposure_error = last_device_status_.shutter_speed *\n>> ERROR_FACTOR + 200us;\n>> > double gain_error = last_device_status_.analogue_gain *\n>> ERROR_FACTOR;\n>> > - double target_error = last_target_exposure_ * ERROR_FACTOR;\n>> > + Duration target_error = last_target_exposure_ * ERROR_FACTOR;\n>> >\n>> > // Note that we don't know the exposure/gain limits of the\n>> sensor, so\n>> > // the values we keep requesting may be unachievable. For this\n>> reason\n>> > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata\n>> *image_metadata)\n>> > current_.analogue_gain = device_status->analogue_gain;\n>> > AgcStatus *agc_status =\n>> > image_metadata->GetLocked(\"agc.status\");\n>> > - current_.total_exposure = agc_status ?\n>> agc_status->total_exposure_value : 0;\n>> > + current_.total_exposure = agc_status ?\n>> agc_status->total_exposure_value : 0s;\n>> > current_.total_exposure_no_dg = current_.shutter *\n>> current_.analogue_gain;\n>> > }\n>> >\n>> > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats\n>> *statistics, Metadata *image_metadata,\n>> >\n>> > void Agc::computeTargetExposure(double gain)\n>> > {\n>> > - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain\n>> != 0.0) {\n>> > + if (status_.fixed_shutter && status_.fixed_analogue_gain) {\n>> > // When ag and shutter are both fixed, we need to drive\n>> the\n>> > // total exposure so that we end up with a digital gain\n>> of at least\n>> > // 1/min_colour_gain. Otherwise we'd desaturate\n>> channels causing\n>> > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)\n>> > target_.total_exposure = current_.total_exposure_no_dg\n>> * gain;\n>> > // The final target exposure is also limited to what\n>> the exposure\n>> > // mode allows.\n>> > - double max_shutter = status_.fixed_shutter != 0.0\n>> > + Duration max_shutter = status_.fixed_shutter\n>> > ? status_.fixed_shutter\n>> > : exposure_mode_->shutter.back();\n>> > max_shutter = clipShutter(max_shutter);\n>> > - double max_total_exposure =\n>> > + Duration max_total_exposure =\n>> > max_shutter *\n>> > (status_.fixed_analogue_gain != 0.0\n>> > ? status_.fixed_analogue_gain\n>> > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)\n>> > if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||\n>> > frame_count_ <= config_.startup_frames)\n>> > speed = 1.0;\n>> > - if (filtered_.total_exposure == 0.0) {\n>> > + if (!filtered_.total_exposure) {\n>> > filtered_.total_exposure = target_.total_exposure;\n>> > filtered_.total_exposure_no_dg =\n>> target_.total_exposure_no_dg;\n>> > } else {\n>> > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()\n>> > // Sending the fixed shutter/gain cases through the same code\n>> may seem\n>> > // unnecessary, but it will make more sense when extend this to\n>> cover\n>> > // variable aperture.\n>> > - double exposure_value = filtered_.total_exposure_no_dg;\n>> > - double shutter_time, analogue_gain;\n>> > - shutter_time = status_.fixed_shutter != 0.0\n>> > + Duration exposure_value = filtered_.total_exposure_no_dg;\n>> > + Duration shutter_time;\n>> > + double analogue_gain;\n>> > + shutter_time = status_.fixed_shutter\n>> > ? status_.fixed_shutter\n>> > : exposure_mode_->shutter[0];\n>> > shutter_time = clipShutter(shutter_time);\n>> > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()\n>> > if (shutter_time * analogue_gain < exposure_value) {\n>> > for (unsigned int stage = 1;\n>> > stage < exposure_mode_->gain.size(); stage++) {\n>> > - if (status_.fixed_shutter == 0.0) {\n>> > - double stage_shutter =\n>> > + if (!status_.fixed_shutter) {\n>> > + Duration stage_shutter =\n>> >\n>> clipShutter(exposure_mode_->shutter[stage]);\n>> > if (stage_shutter * analogue_gain >=\n>> > exposure_value) {\n>> > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()\n>> > << analogue_gain;\n>> > // Finally adjust shutter time for flicker avoidance (require\n>> both\n>> > // shutter and gain not to be fixed).\n>> > - if (status_.fixed_shutter == 0.0 &&\n>> > - status_.fixed_analogue_gain == 0.0 &&\n>> > - status_.flicker_period != 0.0) {\n>> > - int flicker_periods = shutter_time /\n>> status_.flicker_period;\n>> > - if (flicker_periods > 0) {\n>> > - double new_shutter_time = flicker_periods *\n>> status_.flicker_period;\n>> > + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&\n>> > + status_.flicker_period) {\n>> > + double flicker_periods = shutter_time /\n>> status_.flicker_period;\n>> > + if (flicker_periods > 0.0) {\n>> > + Duration new_shutter_time = flicker_periods *\n>> status_.flicker_period;\n>>\n>> Just wondering if this will behave the same now that flicker_periods\n>> is a double. Won't it now hold fractional values with the result that\n>> new_shutter_time isn't an exact multiple of the period?\n>>\n>\n> Quite right, it can be fractional! I should just keep flicker_periods as\n> an int to avoid\n> this. Thanks for pointing that out.\n>\n>\n>> > analogue_gain *= shutter_time /\n>> new_shutter_time;\n>> > // We should still not allow the ag to go over\n>> the\n>> > // largest value in the exposure mode. Note\n>> that this\n>> > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()\n>> > void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n>> > {\n>> > status_.total_exposure_value = filtered_.total_exposure;\n>> > - status_.target_exposure_value = desaturate ? 0 :\n>> target_.total_exposure_no_dg;\n>> > + status_.target_exposure_value = desaturate ? 0s :\n>> target_.total_exposure_no_dg;\n>> > status_.shutter_time = filtered_.shutter;\n>> > status_.analogue_gain = filtered_.analogue_gain;\n>> > // Write to metadata as well, in case anyone wants to update\n>> the camera\n>> > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata\n>> *image_metadata, bool desaturate)\n>> > << \" analogue gain \" <<\n>> filtered_.analogue_gain;\n>> > }\n>> >\n>> > -double Agc::clipShutter(double shutter)\n>> > +Duration Agc::clipShutter(const Duration &shutter)\n>> > {\n>> > + Duration clip_shutter = shutter;\n>>\n>> Given that we copy it anyway I wonder if we might have left this as\n>> call-by-value, resulting in fewer changes. But it's only the teeniest\n>> of nit-picks!\n>>\n>> If you can put my mind at rest over the flicker period thing:\n>>\n>> Reviewed-by: David Plowman \n>>\n>> Thanks!\n>> David\n>>\n>> > if (max_shutter_)\n>> > - shutter = std::min(shutter, max_shutter_);\n>> > - return shutter;\n>> > + clip_shutter = std::min(clip_shutter, max_shutter_);\n>> > + return clip_shutter;\n>> > }\n>> >\n>> > // Register algorithm with the system.\n>> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n>> b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n>> > index 16a65959d90e..24a6610096c2 100644\n>> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n>> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n>> > @@ -9,6 +9,8 @@\n>> > #include \n>> > #include \n>> >\n>> > +#include \"libcamera/internal/utils.h\"\n>> > +\n>> > #include \"../agc_algorithm.hpp\"\n>> > #include \"../agc_status.h\"\n>> > #include \"../pwl.hpp\"\n>> > @@ -22,13 +24,15 @@\n>> >\n>> > namespace RPiController {\n>> >\n>> > +using namespace std::literals::chrono_literals;\n>> > +\n>> > struct AgcMeteringMode {\n>> > double weights[AGC_STATS_SIZE];\n>> > void Read(boost::property_tree::ptree const ¶ms);\n>> > };\n>> >\n>> > struct AgcExposureMode {\n>> > - std::vector shutter;\n>> > + std::vector shutter;\n>> > std::vector gain;\n>> > void Read(boost::property_tree::ptree const ¶ms);\n>> > };\n>> > @@ -61,7 +65,7 @@ struct AgcConfig {\n>> > std::string default_exposure_mode;\n>> > std::string default_constraint_mode;\n>> > double base_ev;\n>> > - double default_exposure_time;\n>> > + Duration default_exposure_time;\n>> > double default_analogue_gain;\n>> > };\n>> >\n>> > @@ -101,19 +105,19 @@ private:\n>> > void filterExposure(bool desaturate);\n>> > void divideUpExposure();\n>> > void writeAndFinish(Metadata *image_metadata, bool desaturate);\n>> > - double clipShutter(double shutter);\n>> > + Duration clipShutter(const Duration &shutter);\n>> > AgcMeteringMode *metering_mode_;\n>> > AgcExposureMode *exposure_mode_;\n>> > AgcConstraintMode *constraint_mode_;\n>> > uint64_t frame_count_;\n>> > AwbStatus awb_;\n>> > struct ExposureValues {\n>> > - ExposureValues() : shutter(0), analogue_gain(0),\n>> > - total_exposure(0),\n>> total_exposure_no_dg(0) {}\n>> > - double shutter;\n>> > + ExposureValues() : shutter(0s), analogue_gain(0),\n>> > + total_exposure(0s),\n>> total_exposure_no_dg(0s) {}\n>> > + Duration shutter;\n>> > double analogue_gain;\n>> > - double total_exposure;\n>> > - double total_exposure_no_dg; // without digital gain\n>> > + Duration total_exposure;\n>> > + Duration total_exposure_no_dg; // without digital gain\n>> > };\n>> > ExposureValues current_; // values for the current frame\n>> > ExposureValues target_; // calculate the values we want here\n>> > @@ -121,15 +125,15 @@ private:\n>> > AgcStatus status_;\n>> > int lock_count_;\n>> > DeviceStatus last_device_status_;\n>> > - double last_target_exposure_;\n>> > + Duration last_target_exposure_;\n>> > // Below here the \"settings\" that applications can change.\n>> > std::string metering_mode_name_;\n>> > std::string exposure_mode_name_;\n>> > std::string constraint_mode_name_;\n>> > double ev_;\n>> > - double flicker_period_;\n>> > - double max_shutter_;\n>> > - double fixed_shutter_;\n>> > + Duration flicker_period_;\n>> > + Duration max_shutter_;\n>> > + Duration fixed_shutter_;\n>> > double fixed_analogue_gain_;\n>> > };\n>> >\n>> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n>> b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n>> > index f74381cab2b4..0fa6457c1376 100644\n>> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n>> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n>> > @@ -16,6 +16,7 @@\n>> >\n>> > using namespace RPiController;\n>> > using namespace libcamera;\n>> > +using namespace std::literals::chrono_literals;\n>> >\n>> > LOG_DEFINE_CATEGORY(RPiLux)\n>> >\n>> > @@ -38,7 +39,7 @@ char const *Lux::Name() const\n>> > void Lux::Read(boost::property_tree::ptree const ¶ms)\n>> > {\n>> > reference_shutter_speed_ =\n>> > - params.get(\"reference_shutter_speed\");\n>> > + params.get(\"reference_shutter_speed\") * 1.0us;\n>> > reference_gain_ = params.get(\"reference_gain\");\n>> > reference_aperture_ = params.get(\"reference_aperture\",\n>> 1.0);\n>> > reference_Y_ = params.get(\"reference_Y\");\n>> > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)\n>> > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n>> > {\n>> > // set some initial values to shut the compiler up\n>> > - DeviceStatus device_status =\n>> > - { .shutter_speed = 1.0,\n>> > - .analogue_gain = 1.0,\n>> > - .lens_position = 0.0,\n>> > - .aperture = 0.0,\n>> > - .flash_intensity = 0.0 };\n>> > + DeviceStatus device_status = { .shutter_speed = 1.0ms,\n>> > + .analogue_gain = 1.0,\n>> > + .lens_position = 0.0,\n>> > + .aperture = 0.0,\n>> > + .flash_intensity = 0.0 };\n>> > if (image_metadata->Get(\"device.status\", device_status) == 0) {\n>> > double current_gain = device_status.analogue_gain;\n>> > - double current_shutter_speed =\n>> device_status.shutter_speed;\n>> > double current_aperture = device_status.aperture;\n>> > if (current_aperture == 0)\n>> > current_aperture = current_aperture_;\n>> > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata\n>> *image_metadata)\n>> > double current_Y = sum / (double)num + .5;\n>> > double gain_ratio = reference_gain_ / current_gain;\n>> > double shutter_speed_ratio =\n>> > - reference_shutter_speed_ /\n>> current_shutter_speed;\n>> > + reference_shutter_speed_ /\n>> device_status.shutter_speed;\n>> > double aperture_ratio = reference_aperture_ /\n>> current_aperture;\n>> > double Y_ratio = current_Y * (65536 / num_bins) /\n>> reference_Y_;\n>> > double estimated_lux = shutter_speed_ratio * gain_ratio\n>> *\n>> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n>> b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n>> > index f9090484a136..45c844393e62 100644\n>> > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n>> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n>> > @@ -8,6 +8,8 @@\n>> >\n>> > #include \n>> >\n>> > +#include \"libcamera/internal/utils.h\"\n>> > +\n>> > #include \"../lux_status.h\"\n>> > #include \"../algorithm.hpp\"\n>> >\n>> > @@ -28,7 +30,7 @@ public:\n>> > private:\n>> > // These values define the conditions of the reference image,\n>> against\n>> > // which we compare the new image.\n>> > - double reference_shutter_speed_; // in micro-seconds\n>> > + libcamera::utils::Duration reference_shutter_speed_;\n>> > double reference_gain_;\n>> > double reference_aperture_; // units of 1/f\n>> > double reference_Y_; // out of 65536\n>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > index f8f36420b076..b77230ded591 100644\n>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,\n>> ipa::RPi::StartConfig *startConf\n>> >\n>> > /* SwitchMode may supply updated exposure/gain values to use. */\n>> > AgcStatus agcStatus;\n>> > - agcStatus.shutter_time = 0.0;\n>> > + agcStatus.shutter_time = 0.0s;\n>> > agcStatus.analogue_gain = 0.0;\n>> >\n>> > metadata.Get(\"agc.status\", agcStatus);\n>> > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=\n>> 0.0) {\n>> > + if (agcStatus.shutter_time && agcStatus.analogue_gain) {\n>> > ControlList ctrls(sensorCtrls_);\n>> > applyAGC(&agcStatus, ctrls);\n>> > startConfig->controls = std::move(ctrls);\n>> > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo\n>> &sensorInfo,\n>> > /* Supply initial values for gain and exposure. */\n>> > ControlList ctrls(sensorCtrls_);\n>> > AgcStatus agcStatus;\n>> > - agcStatus.shutter_time =\n>> DefaultExposureTime.get();\n>> > + agcStatus.shutter_time = DefaultExposureTime;\n>> > agcStatus.analogue_gain = DefaultAnalogueGain;\n>> > applyAGC(&agcStatus, ctrls);\n>> >\n>> > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()\n>> > */\n>> > DeviceStatus *deviceStatus =\n>> rpiMetadata_.GetLocked(\"device.status\");\n>> > if (deviceStatus) {\n>> > - libcameraMetadata_.set(controls::ExposureTime,\n>> deviceStatus->shutter_speed);\n>> > + libcameraMetadata_.set(controls::ExposureTime,\n>> > +\n>> deviceStatus->shutter_speed.get());\n>> > libcameraMetadata_.set(controls::AnalogueGain,\n>> deviceStatus->analogue_gain);\n>> > }\n>> >\n>> > @@ -1017,7 +1018,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n>> &sensorControls)\n>> > int32_t exposureLines =\n>> sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> > int32_t gainCode =\n>> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> >\n>> > - deviceStatus.shutter_speed =\n>> helper_->Exposure(exposureLines).get();\n>> > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n>> > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>> >\n>> > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n>> > @@ -1103,7 +1104,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n>> *agcStatus, ControlList &ctrls)\n>> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n>> >\n>> > /* GetVBlanking might clip exposure time to the fps limits. */\n>> > - Duration exposure = agcStatus->shutter_time * 1.0us;\n>> > + Duration exposure = agcStatus->shutter_time;\n>> > int32_t vblanking = helper_->GetVBlanking(exposure,\n>> minFrameDuration_, maxFrameDuration_);\n>> > int32_t exposureLines = helper_->ExposureLines(exposure);\n>> >\n>> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n>> > index 3131aa2d6666..5fa6f8366f3d 100644\n>> > --- a/src/libcamera/utils.cpp\n>> > +++ b/src/libcamera/utils.cpp\n>> > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()\n>> > * loop, iterates over an indexed view of the \\a iterable\n>> > */\n>> >\n>> > +/**\n>> > + * \\typedef BaseDuration\n>> > + * \\brief Base struct for the \\a Duration helper class.\n>> > + */\n>> > +\n>> > /**\n>> > * \\class Duration\n>> > - * \\brief Helper for std::chrono::duration types. Derived from \\a\n>> BaseDuration\n>> > + * \\brief Helper class for std::chrono::duration types.\n>> > */\n>> >\n>> > /**\n>> > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()\n>> > * \\param[in] d The std::chrono::duration object to convert from.\n>> > *\n>> > * Constructs a \\a Duration object from a \\a std::chrono::duration\n>> object,\n>> > - * converting the representation to \\a BaseDuration type.\n>> > + * internally converting the representation to \\a BaseDuration type.\n>> > */\n>> >\n>> > /**\n>> > --\n>> > 2.25.1\n>> >\n>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0BC94C31FF\n\tfor ;\n\tFri, 21 May 2021 11:59:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 631DD6891F;\n\tFri, 21 May 2021 13:59:16 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A448B68911\n\tfor ;\n\tFri, 21 May 2021 13:59:14 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id t17so6920879ljd.9\n\tfor ;\n\tFri, 21 May 2021 04:59:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"HTgijYr7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=1JqecmLXXT19a7VedCzexuhkXsqYldcYGt31iPNK6p4=;\n\tb=HTgijYr7YnvIGdNNKdQetjap96nDSnVtQFO4uLor5wXQAMx2DUbKzPr9b1SbKcIjpg\n\tmqF25m9JH3AU/JyC1nV4u5FdZpDpvGQmHG4pS/7R0VYD7zdkci2pgcP7N4E0fbkKEdN8\n\toGbKgtgINiNb8LMNkPYQAEcqQG8ZzCO9RlAgymJbhJRjMUTBv/LBtlKZQC7PSkDuRgg2\n\t4LSdMfevjpqIynquFwePxCrpHs31ZQynZ9jral5J4+yLy56891CvSsHdNZIBwnbsTM9O\n\tsikyEYuBOryRrW3bkquaR27LDKc0AL5JH51gsBREuUvR/hQMa8yDwUuizQnhTTFft6Wh\n\tfzpw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=1JqecmLXXT19a7VedCzexuhkXsqYldcYGt31iPNK6p4=;\n\tb=j+53zBFKOtov4iotfJtr8HNaQ0gCEuiYY+BIFkaGfEfEXdLLBlFHlNSucprtT7ZERT\n\tMsu6ux/dWn3aPbcHwD/I0gTqmixPQObrnSUC+S5D8Ta6RGv8yk5Pd18cy20pBaKQ24q4\n\t/qYSpmbqSa2aTQFcR7v767YXmypYZe0YlCH1SwPQxXeZ3FDMGm3efeDqlS8m1aFrX17F\n\tjSOe1zMoPwyeiYiWhwffx2ng3xMdxg7yY8D1K7lMRrf3OxHUNSbC/dhIZ80z0SEoYBiA\n\tT5GfiVHIXBWydFj02CnAAb5+kci4bgflPL3N4tKevJk+aqW3poK4OZU6X3S0XjGCjsCs\n\tyXFw==","X-Gm-Message-State":"AOAM532fG8PuS73TlKyUxcHIfsZP+ze2MQj41AV0BgmcSlvP8+k5SV7L\n\tS9NRzqLLunfsYThcjhPzq/JRY/U/rwFmjt5nza8zdcFMEm8=","X-Google-Smtp-Source":"ABdhPJzm0v00h5k4vXyCmi/cTHAtz4peZhINPLYaKopl91DU1bdKZqjOMr3F7hxJTryeOf56wf0rtdT9PrmwJjWvJPY=","X-Received":"by 2002:a05:651c:20e:: with SMTP id\n\ty14mr3728807ljn.253.1621598353969; \n\tFri, 21 May 2021 04:59:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-5-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 21 May 2021 12:58:57 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000db691f05c2d5c9b4\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17098,"web_url":"https://patchwork.libcamera.org/comment/17098/","msgid":"<20210521123800.7yktaq6ek4cgfw4i@uno.localdomain>","date":"2021-05-21T12:38:00","subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Fri, May 21, 2021 at 12:18:55PM +0100, David Plowman wrote:\n> Hi Naush\n>\n> Thanks for all this work!\n>\n> On Fri, 21 May 2021 at 09:05, Naushir Patuck wrote:\n> >\n> > Convert the core AGC and Lux controller code to use\n> > utils::Duration for all exposure time related variables and\n> > calculations.\n> >\n> > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus\n> > to use utils::Duration.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 2 +-\n> > src/ipa/raspberrypi/controller/agc_status.h | 14 +--\n> > .../raspberrypi/controller/device_status.h | 6 +-\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 +++++++++++--------\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---\n> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--\n> > src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 13 +--\n> > src/libcamera/utils.cpp | 9 +-\n> > 9 files changed, 105 insertions(+), 77 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > index feb02d1806b2..968fae13bd5e 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> > return;\n> > }\n> >\n> > - deviceStatus.shutter_speed = Exposure(exposureLines).get();\n> > + deviceStatus.shutter_speed = Exposure(exposureLines);\n> > deviceStatus.analogue_gain = Gain(gainCode);\n> >\n> > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > diff --git a/src/ipa/raspberrypi/controller/agc_status.h b/src/ipa/raspberrypi/controller/agc_status.h\n> > index 10381c90a313..c893715af588 100644\n> > --- a/src/ipa/raspberrypi/controller/agc_status.h\n> > +++ b/src/ipa/raspberrypi/controller/agc_status.h\n> > @@ -6,6 +6,8 @@\n> > */\n> > #pragma once\n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > // The AGC algorithm should post the following structure into the image's\n> > // \"agc.status\" metadata.\n> >\n> > @@ -17,18 +19,20 @@ extern \"C\" {\n> > // seen statistics and calculated meaningful values. The contents should be\n> > // ignored until then.\n> >\n> > +using libcamera::utils::Duration;\n> > +\n> > struct AgcStatus {\n> > - double total_exposure_value; // value for all exposure and gain for this image\n> > - double target_exposure_value; // (unfiltered) target total exposure AGC is aiming for\n> > - double shutter_time;\n> > + Duration total_exposure_value; // value for all exposure and gain for this image\n> > + Duration target_exposure_value; // (unfiltered) target total exposure AGC is aiming for\n> > + Duration shutter_time;\n> > double analogue_gain;\n> > char exposure_mode[32];\n> > char constraint_mode[32];\n> > char metering_mode[32];\n> > double ev;\n> > - double flicker_period;\n> > + Duration flicker_period;\n> > int floating_region_enable;\n> > - double fixed_shutter;\n> > + Duration fixed_shutter;\n> > double fixed_analogue_gain;\n> > double digital_gain;\n> > int locked;\n> > diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h\n> > index aa08608b5d40..131b4cd344ee 100644\n> > --- a/src/ipa/raspberrypi/controller/device_status.h\n> > +++ b/src/ipa/raspberrypi/controller/device_status.h\n> > @@ -6,6 +6,8 @@\n> > */\n> > #pragma once\n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > // Definition of \"device metadata\" which stores things like shutter time and\n> > // analogue gain that downstream control algorithms will want to know.\n> >\n> > @@ -14,8 +16,8 @@ extern \"C\" {\n> > #endif\n> >\n> > struct DeviceStatus {\n> > - // time shutter is open, in microseconds\n> > - double shutter_speed;\n> > + // time shutter is open\n> > + libcamera::utils::Duration shutter_speed;\n> > double analogue_gain;\n> > // 1.0/distance-in-metres, or 0 if unknown\n> > double lens_position;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 482eb3ef962d..46742d845034 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -21,6 +21,7 @@\n> >\n> > using namespace RPiController;\n> > using namespace libcamera;\n> > +using utils::operator<<;\n>\n> Still can't say that I like this... just because someone wants to use\n> Durations, it seems like an awkward thing to make them remember. But\n> unless anyone has anything better....\n\nI got the same feeling while reading the rest of the patches.\nI can't say toString() is that better though, but probably is less\nimplicit that this using statment, which if read in isolation does not\nimmediately relates to the Duration class ?\n\n>\n> >\n> > LOG_DEFINE_CATEGORY(RPiAgc)\n> >\n> > @@ -55,19 +56,27 @@ read_metering_modes(std::map &metering_modes,\n> > return first;\n> > }\n> >\n> > -static int read_double_list(std::vector &list,\n> > - boost::property_tree::ptree const ¶ms)\n> > +static int read_list(std::vector &list,\n> > + boost::property_tree::ptree const ¶ms)\n> > {\n> > for (auto &p : params)\n> > list.push_back(p.second.get_value());\n> > return list.size();\n> > }\n> >\n> > +static int read_list(std::vector &list,\n> > + boost::property_tree::ptree const ¶ms)\n> > +{\n> > + for (auto &p : params)\n> > + list.push_back(p.second.get_value() * 1us);\n> > + return list.size();\n> > +}\n> > +\n> > void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms)\n> > {\n> > int num_shutters =\n> > - read_double_list(shutter, params.get_child(\"shutter\"));\n> > - int num_ags = read_double_list(gain, params.get_child(\"gain\"));\n> > + read_list(shutter, params.get_child(\"shutter\"));\n> > + int num_ags = read_list(gain, params.get_child(\"gain\"));\n> > if (num_shutters < 2 || num_ags < 2)\n> > throw std::runtime_error(\n> > \"AgcConfig: must have at least two entries in exposure profile\");\n> > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const ¶ms)\n> > params.get(\"fast_reduce_threshold\", 0.4);\n> > base_ev = params.get(\"base_ev\", 1.0);\n> > // Start with quite a low value as ramping up is easier than ramping down.\n> > - default_exposure_time = params.get(\"default_exposure_time\", 1000);\n> > + default_exposure_time = params.get(\"default_exposure_time\", 1000) * 1us;\n> > default_analogue_gain = params.get(\"default_analogue_gain\", 1.0);\n> > }\n> >\n> > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)\n> > : AgcAlgorithm(controller), metering_mode_(nullptr),\n> > exposure_mode_(nullptr), constraint_mode_(nullptr),\n> > frame_count_(0), lock_count_(0),\n> > - last_target_exposure_(0.0),\n> > - ev_(1.0), flicker_period_(0.0),\n> > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> > + last_target_exposure_(0s),\n> > + ev_(1.0), flicker_period_(0s),\n> > + max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)\n> > {\n> > memset(&awb_, 0, sizeof(awb_));\n> > // Setting status_.total_exposure_value_ to zero initially tells us\n> > @@ -203,7 +212,7 @@ void Agc::Pause()\n> >\n> > void Agc::Resume()\n> > {\n> > - fixed_shutter_ = 0;\n> > + fixed_shutter_ = 0s;\n> > fixed_analogue_gain_ = 0;\n> > }\n> >\n> > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)\n> >\n> > void Agc::SetFlickerPeriod(const Duration &flicker_period)\n> > {\n> > - flicker_period_ = flicker_period.get();\n> > + flicker_period_ = flicker_period;\n> > }\n> >\n> > void Agc::SetMaxShutter(const Duration &max_shutter)\n> > {\n> > - max_shutter_ = max_shutter.get();\n> > + max_shutter_ = max_shutter;\n> > }\n> >\n> > void Agc::SetFixedShutter(const Duration &fixed_shutter)\n> > {\n> > - fixed_shutter_ = fixed_shutter.get();\n> > + fixed_shutter_ = fixed_shutter;\n> > // Set this in case someone calls Pause() straight after.\n> > status_.shutter_time = clipShutter(fixed_shutter_);\n> > }\n> > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> > {\n> > housekeepConfig();\n> >\n> > - double fixed_shutter = clipShutter(fixed_shutter_);\n> > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> > + Duration fixed_shutter = clipShutter(fixed_shutter_);\n> > + if (fixed_shutter && fixed_analogue_gain_) {\n> > // We're going to reset the algorithm here with these fixed values.\n> >\n> > fetchAwbStatus(metadata);\n> > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > // Process has run, so we have meaningful values.\n> > DeviceStatus device_status;\n> > if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> > - double actual_exposure = device_status.shutter_speed *\n> > - device_status.analogue_gain;\n> > + Duration actual_exposure = device_status.shutter_speed *\n> > + device_status.analogue_gain;\n> > if (actual_exposure) {\n> > status_.digital_gain =\n> > status_.total_exposure_value /\n> > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > std::min(status_.digital_gain, 4.0));\n> > LOG(RPiAgc, Debug) << \"Actual exposure \" << actual_exposure;\n> > LOG(RPiAgc, Debug) << \"Use digital_gain \" << status_.digital_gain;\n> > - LOG(RPiAgc, Debug) << \"Effective exposure \" << actual_exposure * status_.digital_gain;\n> > + LOG(RPiAgc, Debug) << \"Effective exposure \"\n> > + << actual_exposure * status_.digital_gain;\n> > // Decide whether AEC/AGC has converged.\n> > updateLockStatus(device_status);\n> > }\n> > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const &device_status)\n> > const double RESET_MARGIN = 1.5;\n> >\n> > // Add 200us to the exposure time error to allow for line quantisation.\n> > - double exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200;\n> > + Duration exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200us;\n> > double gain_error = last_device_status_.analogue_gain * ERROR_FACTOR;\n> > - double target_error = last_target_exposure_ * ERROR_FACTOR;\n> > + Duration target_error = last_target_exposure_ * ERROR_FACTOR;\n> >\n> > // Note that we don't know the exposure/gain limits of the sensor, so\n> > // the values we keep requesting may be unachievable. For this reason\n> > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata *image_metadata)\n> > current_.analogue_gain = device_status->analogue_gain;\n> > AgcStatus *agc_status =\n> > image_metadata->GetLocked(\"agc.status\");\n> > - current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0;\n> > + current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0s;\n> > current_.total_exposure_no_dg = current_.shutter * current_.analogue_gain;\n> > }\n> >\n> > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *image_metadata,\n> >\n> > void Agc::computeTargetExposure(double gain)\n> > {\n> > - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain != 0.0) {\n> > + if (status_.fixed_shutter && status_.fixed_analogue_gain) {\n> > // When ag and shutter are both fixed, we need to drive the\n> > // total exposure so that we end up with a digital gain of at least\n> > // 1/min_colour_gain. Otherwise we'd desaturate channels causing\n> > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)\n> > target_.total_exposure = current_.total_exposure_no_dg * gain;\n> > // The final target exposure is also limited to what the exposure\n> > // mode allows.\n> > - double max_shutter = status_.fixed_shutter != 0.0\n> > + Duration max_shutter = status_.fixed_shutter\n> > ? status_.fixed_shutter\n> > : exposure_mode_->shutter.back();\n> > max_shutter = clipShutter(max_shutter);\n> > - double max_total_exposure =\n> > + Duration max_total_exposure =\n> > max_shutter *\n> > (status_.fixed_analogue_gain != 0.0\n> > ? status_.fixed_analogue_gain\n> > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)\n> > if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||\n> > frame_count_ <= config_.startup_frames)\n> > speed = 1.0;\n> > - if (filtered_.total_exposure == 0.0) {\n> > + if (!filtered_.total_exposure) {\n> > filtered_.total_exposure = target_.total_exposure;\n> > filtered_.total_exposure_no_dg = target_.total_exposure_no_dg;\n> > } else {\n> > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()\n> > // Sending the fixed shutter/gain cases through the same code may seem\n> > // unnecessary, but it will make more sense when extend this to cover\n> > // variable aperture.\n> > - double exposure_value = filtered_.total_exposure_no_dg;\n> > - double shutter_time, analogue_gain;\n> > - shutter_time = status_.fixed_shutter != 0.0\n> > + Duration exposure_value = filtered_.total_exposure_no_dg;\n> > + Duration shutter_time;\n> > + double analogue_gain;\n> > + shutter_time = status_.fixed_shutter\n> > ? status_.fixed_shutter\n> > : exposure_mode_->shutter[0];\n> > shutter_time = clipShutter(shutter_time);\n> > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()\n> > if (shutter_time * analogue_gain < exposure_value) {\n> > for (unsigned int stage = 1;\n> > stage < exposure_mode_->gain.size(); stage++) {\n> > - if (status_.fixed_shutter == 0.0) {\n> > - double stage_shutter =\n> > + if (!status_.fixed_shutter) {\n> > + Duration stage_shutter =\n> > clipShutter(exposure_mode_->shutter[stage]);\n> > if (stage_shutter * analogue_gain >=\n> > exposure_value) {\n> > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()\n> > << analogue_gain;\n> > // Finally adjust shutter time for flicker avoidance (require both\n> > // shutter and gain not to be fixed).\n> > - if (status_.fixed_shutter == 0.0 &&\n> > - status_.fixed_analogue_gain == 0.0 &&\n> > - status_.flicker_period != 0.0) {\n> > - int flicker_periods = shutter_time / status_.flicker_period;\n> > - if (flicker_periods > 0) {\n> > - double new_shutter_time = flicker_periods * status_.flicker_period;\n> > + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&\n> > + status_.flicker_period) {\n> > + double flicker_periods = shutter_time / status_.flicker_period;\n> > + if (flicker_periods > 0.0) {\n> > + Duration new_shutter_time = flicker_periods * status_.flicker_period;\n>\n> Just wondering if this will behave the same now that flicker_periods\n> is a double. Won't it now hold fractional values with the result that\n> new_shutter_time isn't an exact multiple of the period?\n>\n> > analogue_gain *= shutter_time / new_shutter_time;\n> > // We should still not allow the ag to go over the\n> > // largest value in the exposure mode. Note that this\n> > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()\n> > void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> > {\n> > status_.total_exposure_value = filtered_.total_exposure;\n> > - status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg;\n> > + status_.target_exposure_value = desaturate ? 0s : target_.total_exposure_no_dg;\n> > status_.shutter_time = filtered_.shutter;\n> > status_.analogue_gain = filtered_.analogue_gain;\n> > // Write to metadata as well, in case anyone wants to update the camera\n> > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> > << \" analogue gain \" << filtered_.analogue_gain;\n> > }\n> >\n> > -double Agc::clipShutter(double shutter)\n> > +Duration Agc::clipShutter(const Duration &shutter)\n> > {\n> > + Duration clip_shutter = shutter;\n>\n> Given that we copy it anyway I wonder if we might have left this as\n> call-by-value, resulting in fewer changes. But it's only the teeniest\n> of nit-picks!\n>\n> If you can put my mind at rest over the flicker period thing:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n>\n> > if (max_shutter_)\n> > - shutter = std::min(shutter, max_shutter_);\n> > - return shutter;\n> > + clip_shutter = std::min(clip_shutter, max_shutter_);\n> > + return clip_shutter;\n> > }\n> >\n> > // Register algorithm with the system.\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index 16a65959d90e..24a6610096c2 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -9,6 +9,8 @@\n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > #include \"../agc_algorithm.hpp\"\n> > #include \"../agc_status.h\"\n> > #include \"../pwl.hpp\"\n> > @@ -22,13 +24,15 @@\n> >\n> > namespace RPiController {\n> >\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > struct AgcMeteringMode {\n> > double weights[AGC_STATS_SIZE];\n> > void Read(boost::property_tree::ptree const ¶ms);\n> > };\n> >\n> > struct AgcExposureMode {\n> > - std::vector shutter;\n> > + std::vector shutter;\n> > std::vector gain;\n> > void Read(boost::property_tree::ptree const ¶ms);\n> > };\n> > @@ -61,7 +65,7 @@ struct AgcConfig {\n> > std::string default_exposure_mode;\n> > std::string default_constraint_mode;\n> > double base_ev;\n> > - double default_exposure_time;\n> > + Duration default_exposure_time;\n> > double default_analogue_gain;\n> > };\n> >\n> > @@ -101,19 +105,19 @@ private:\n> > void filterExposure(bool desaturate);\n> > void divideUpExposure();\n> > void writeAndFinish(Metadata *image_metadata, bool desaturate);\n> > - double clipShutter(double shutter);\n> > + Duration clipShutter(const Duration &shutter);\n> > AgcMeteringMode *metering_mode_;\n> > AgcExposureMode *exposure_mode_;\n> > AgcConstraintMode *constraint_mode_;\n> > uint64_t frame_count_;\n> > AwbStatus awb_;\n> > struct ExposureValues {\n> > - ExposureValues() : shutter(0), analogue_gain(0),\n> > - total_exposure(0), total_exposure_no_dg(0) {}\n> > - double shutter;\n> > + ExposureValues() : shutter(0s), analogue_gain(0),\n> > + total_exposure(0s), total_exposure_no_dg(0s) {}\n> > + Duration shutter;\n> > double analogue_gain;\n> > - double total_exposure;\n> > - double total_exposure_no_dg; // without digital gain\n> > + Duration total_exposure;\n> > + Duration total_exposure_no_dg; // without digital gain\n> > };\n> > ExposureValues current_; // values for the current frame\n> > ExposureValues target_; // calculate the values we want here\n> > @@ -121,15 +125,15 @@ private:\n> > AgcStatus status_;\n> > int lock_count_;\n> > DeviceStatus last_device_status_;\n> > - double last_target_exposure_;\n> > + Duration last_target_exposure_;\n> > // Below here the \"settings\" that applications can change.\n> > std::string metering_mode_name_;\n> > std::string exposure_mode_name_;\n> > std::string constraint_mode_name_;\n> > double ev_;\n> > - double flicker_period_;\n> > - double max_shutter_;\n> > - double fixed_shutter_;\n> > + Duration flicker_period_;\n> > + Duration max_shutter_;\n> > + Duration fixed_shutter_;\n> > double fixed_analogue_gain_;\n> > };\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > index f74381cab2b4..0fa6457c1376 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > @@ -16,6 +16,7 @@\n> >\n> > using namespace RPiController;\n> > using namespace libcamera;\n> > +using namespace std::literals::chrono_literals;\n> >\n> > LOG_DEFINE_CATEGORY(RPiLux)\n> >\n> > @@ -38,7 +39,7 @@ char const *Lux::Name() const\n> > void Lux::Read(boost::property_tree::ptree const ¶ms)\n> > {\n> > reference_shutter_speed_ =\n> > - params.get(\"reference_shutter_speed\");\n> > + params.get(\"reference_shutter_speed\") * 1.0us;\n> > reference_gain_ = params.get(\"reference_gain\");\n> > reference_aperture_ = params.get(\"reference_aperture\", 1.0);\n> > reference_Y_ = params.get(\"reference_Y\");\n> > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)\n> > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> > {\n> > // set some initial values to shut the compiler up\n> > - DeviceStatus device_status =\n> > - { .shutter_speed = 1.0,\n> > - .analogue_gain = 1.0,\n> > - .lens_position = 0.0,\n> > - .aperture = 0.0,\n> > - .flash_intensity = 0.0 };\n> > + DeviceStatus device_status = { .shutter_speed = 1.0ms,\n> > + .analogue_gain = 1.0,\n> > + .lens_position = 0.0,\n> > + .aperture = 0.0,\n> > + .flash_intensity = 0.0 };\n> > if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> > double current_gain = device_status.analogue_gain;\n> > - double current_shutter_speed = device_status.shutter_speed;\n> > double current_aperture = device_status.aperture;\n> > if (current_aperture == 0)\n> > current_aperture = current_aperture_;\n> > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> > double current_Y = sum / (double)num + .5;\n> > double gain_ratio = reference_gain_ / current_gain;\n> > double shutter_speed_ratio =\n> > - reference_shutter_speed_ / current_shutter_speed;\n> > + reference_shutter_speed_ / device_status.shutter_speed;\n> > double aperture_ratio = reference_aperture_ / current_aperture;\n> > double Y_ratio = current_Y * (65536 / num_bins) / reference_Y_;\n> > double estimated_lux = shutter_speed_ratio * gain_ratio *\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > index f9090484a136..45c844393e62 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > @@ -8,6 +8,8 @@\n> >\n> > #include \n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > #include \"../lux_status.h\"\n> > #include \"../algorithm.hpp\"\n> >\n> > @@ -28,7 +30,7 @@ public:\n> > private:\n> > // These values define the conditions of the reference image, against\n> > // which we compare the new image.\n> > - double reference_shutter_speed_; // in micro-seconds\n> > + libcamera::utils::Duration reference_shutter_speed_;\n> > double reference_gain_;\n> > double reference_aperture_; // units of 1/f\n> > double reference_Y_; // out of 65536\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f8f36420b076..b77230ded591 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf\n> >\n> > /* SwitchMode may supply updated exposure/gain values to use. */\n> > AgcStatus agcStatus;\n> > - agcStatus.shutter_time = 0.0;\n> > + agcStatus.shutter_time = 0.0s;\n> > agcStatus.analogue_gain = 0.0;\n> >\n> > metadata.Get(\"agc.status\", agcStatus);\n> > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > + if (agcStatus.shutter_time && agcStatus.analogue_gain) {\n> > ControlList ctrls(sensorCtrls_);\n> > applyAGC(&agcStatus, ctrls);\n> > startConfig->controls = std::move(ctrls);\n> > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > AgcStatus agcStatus;\n> > - agcStatus.shutter_time = DefaultExposureTime.get();\n> > + agcStatus.shutter_time = DefaultExposureTime;\n> > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > applyAGC(&agcStatus, ctrls);\n> >\n> > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()\n> > */\n> > DeviceStatus *deviceStatus = rpiMetadata_.GetLocked(\"device.status\");\n> > if (deviceStatus) {\n> > - libcameraMetadata_.set(controls::ExposureTime, deviceStatus->shutter_speed);\n> > + libcameraMetadata_.set(controls::ExposureTime,\n> > + deviceStatus->shutter_speed.get());\n> > libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain);\n> > }\n> >\n> > @@ -1017,7 +1018,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> > int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >\n> > - deviceStatus.shutter_speed = helper_->Exposure(exposureLines).get();\n> > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >\n> > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > @@ -1103,7 +1104,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> >\n> > /* GetVBlanking might clip exposure time to the fps limits. */\n> > - Duration exposure = agcStatus->shutter_time * 1.0us;\n> > + Duration exposure = agcStatus->shutter_time;\n> > int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);\n> > int32_t exposureLines = helper_->ExposureLines(exposure);\n> >\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 3131aa2d6666..5fa6f8366f3d 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()\n> > * loop, iterates over an indexed view of the \\a iterable\n> > */\n> >\n> > +/**\n> > + * \\typedef BaseDuration\n> > + * \\brief Base struct for the \\a Duration helper class.\n> > + */\n> > +\n> > /**\n> > * \\class Duration\n> > - * \\brief Helper for std::chrono::duration types. Derived from \\a BaseDuration\n> > + * \\brief Helper class for std::chrono::duration types.\n> > */\n> >\n> > /**\n> > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()\n> > * \\param[in] d The std::chrono::duration object to convert from.\n> > *\n> > * Constructs a \\a Duration object from a \\a std::chrono::duration object,\n> > - * converting the representation to \\a BaseDuration type.\n> > + * internally converting the representation to \\a BaseDuration type.\n> > */\n> >\n> > /**\n> > --\n> > 2.25.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5E4ECC3200\n\tfor ;\n\tFri, 21 May 2021 12:37:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B32186891F;\n\tFri, 21 May 2021 14:37:16 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9BD4968911\n\tfor ;\n\tFri, 21 May 2021 14:37:15 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id C1029E0006;\n\tFri, 21 May 2021 12:37:14 +0000 (UTC)"],"Date":"Fri, 21 May 2021 14:38:00 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210521123800.7yktaq6ek4cgfw4i@uno.localdomain>","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-5-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17100,"web_url":"https://patchwork.libcamera.org/comment/17100/","msgid":"","date":"2021-05-21T12:49:30","subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 21 May 2021 at 13:37, Jacopo Mondi wrote:\n\n> Hello,\n>\n> On Fri, May 21, 2021 at 12:18:55PM +0100, David Plowman wrote:\n> > Hi Naush\n> >\n> > Thanks for all this work!\n> >\n> > On Fri, 21 May 2021 at 09:05, Naushir Patuck \n> wrote:\n> > >\n> > > Convert the core AGC and Lux controller code to use\n> > > utils::Duration for all exposure time related variables and\n> > > calculations.\n> > >\n> > > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus\n> > > to use utils::Duration.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 2 +-\n> > > src/ipa/raspberrypi/controller/agc_status.h | 14 +--\n> > > .../raspberrypi/controller/device_status.h | 6 +-\n> > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 +++++++++++--------\n> > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---\n> > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--\n> > > src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 13 +--\n> > > src/libcamera/utils.cpp | 9 +-\n> > > 9 files changed, 105 insertions(+), 77 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index feb02d1806b2..968fae13bd5e 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > > return;\n> > > }\n> > >\n> > > - deviceStatus.shutter_speed =\n> Exposure(exposureLines).get();\n> > > + deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > deviceStatus.analogue_gain = Gain(gainCode);\n> > >\n> > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > diff --git a/src/ipa/raspberrypi/controller/agc_status.h\n> b/src/ipa/raspberrypi/controller/agc_status.h\n> > > index 10381c90a313..c893715af588 100644\n> > > --- a/src/ipa/raspberrypi/controller/agc_status.h\n> > > +++ b/src/ipa/raspberrypi/controller/agc_status.h\n> > > @@ -6,6 +6,8 @@\n> > > */\n> > > #pragma once\n> > >\n> > > +#include \"libcamera/internal/utils.h\"\n> > > +\n> > > // The AGC algorithm should post the following structure into the\n> image's\n> > > // \"agc.status\" metadata.\n> > >\n> > > @@ -17,18 +19,20 @@ extern \"C\" {\n> > > // seen statistics and calculated meaningful values. The contents\n> should be\n> > > // ignored until then.\n> > >\n> > > +using libcamera::utils::Duration;\n> > > +\n> > > struct AgcStatus {\n> > > - double total_exposure_value; // value for all exposure and\n> gain for this image\n> > > - double target_exposure_value; // (unfiltered) target total\n> exposure AGC is aiming for\n> > > - double shutter_time;\n> > > + Duration total_exposure_value; // value for all exposure and\n> gain for this image\n> > > + Duration target_exposure_value; // (unfiltered) target total\n> exposure AGC is aiming for\n> > > + Duration shutter_time;\n> > > double analogue_gain;\n> > > char exposure_mode[32];\n> > > char constraint_mode[32];\n> > > char metering_mode[32];\n> > > double ev;\n> > > - double flicker_period;\n> > > + Duration flicker_period;\n> > > int floating_region_enable;\n> > > - double fixed_shutter;\n> > > + Duration fixed_shutter;\n> > > double fixed_analogue_gain;\n> > > double digital_gain;\n> > > int locked;\n> > > diff --git a/src/ipa/raspberrypi/controller/device_status.h\n> b/src/ipa/raspberrypi/controller/device_status.h\n> > > index aa08608b5d40..131b4cd344ee 100644\n> > > --- a/src/ipa/raspberrypi/controller/device_status.h\n> > > +++ b/src/ipa/raspberrypi/controller/device_status.h\n> > > @@ -6,6 +6,8 @@\n> > > */\n> > > #pragma once\n> > >\n> > > +#include \"libcamera/internal/utils.h\"\n> > > +\n> > > // Definition of \"device metadata\" which stores things like shutter\n> time and\n> > > // analogue gain that downstream control algorithms will want to know.\n> > >\n> > > @@ -14,8 +16,8 @@ extern \"C\" {\n> > > #endif\n> > >\n> > > struct DeviceStatus {\n> > > - // time shutter is open, in microseconds\n> > > - double shutter_speed;\n> > > + // time shutter is open\n> > > + libcamera::utils::Duration shutter_speed;\n> > > double analogue_gain;\n> > > // 1.0/distance-in-metres, or 0 if unknown\n> > > double lens_position;\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > index 482eb3ef962d..46742d845034 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > @@ -21,6 +21,7 @@\n> > >\n> > > using namespace RPiController;\n> > > using namespace libcamera;\n> > > +using utils::operator<<;\n> >\n> > Still can't say that I like this... just because someone wants to use\n> > Durations, it seems like an awkward thing to make them remember. But\n> > unless anyone has anything better....\n>\n> I got the same feeling while reading the rest of the patches.\n> I can't say toString() is that better though, but probably is less\n> implicit that this using statment, which if read in isolation does not\n> immediately relates to the Duration class ?\n>\n\nI do agree with these points, and I don't like this as well. I can see two\nalternatives without\nusing toString():\n\n1) Move the Duration class into its own namespace under utils, so then the\nabove statement\nwill look a bit more informative:\n\nusing utils::Duration::operator<<;\n\n2) Move the operator() out of the utils namespace. This way we eliminate\nthe need for the\nstatement completely.\n\nRegards,\nNaush\n\n\n>\n> >\n> > >\n> > > LOG_DEFINE_CATEGORY(RPiAgc)\n> > >\n> > > @@ -55,19 +56,27 @@ read_metering_modes(std::map AgcMeteringMode> &metering_modes,\n> > > return first;\n> > > }\n> > >\n> > > -static int read_double_list(std::vector &list,\n> > > - boost::property_tree::ptree const ¶ms)\n> > > +static int read_list(std::vector &list,\n> > > + boost::property_tree::ptree const ¶ms)\n> > > {\n> > > for (auto &p : params)\n> > > list.push_back(p.second.get_value());\n> > > return list.size();\n> > > }\n> > >\n> > > +static int read_list(std::vector &list,\n> > > + boost::property_tree::ptree const ¶ms)\n> > > +{\n> > > + for (auto &p : params)\n> > > + list.push_back(p.second.get_value() * 1us);\n> > > + return list.size();\n> > > +}\n> > > +\n> > > void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms)\n> > > {\n> > > int num_shutters =\n> > > - read_double_list(shutter, params.get_child(\"shutter\"));\n> > > - int num_ags = read_double_list(gain, params.get_child(\"gain\"));\n> > > + read_list(shutter, params.get_child(\"shutter\"));\n> > > + int num_ags = read_list(gain, params.get_child(\"gain\"));\n> > > if (num_shutters < 2 || num_ags < 2)\n> > > throw std::runtime_error(\n> > > \"AgcConfig: must have at least two entries in\n> exposure profile\");\n> > > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree\n> const ¶ms)\n> > > params.get(\"fast_reduce_threshold\", 0.4);\n> > > base_ev = params.get(\"base_ev\", 1.0);\n> > > // Start with quite a low value as ramping up is easier than\n> ramping down.\n> > > - default_exposure_time =\n> params.get(\"default_exposure_time\", 1000);\n> > > + default_exposure_time =\n> params.get(\"default_exposure_time\", 1000) * 1us;\n> > > default_analogue_gain =\n> params.get(\"default_analogue_gain\", 1.0);\n> > > }\n> > >\n> > > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)\n> > > : AgcAlgorithm(controller), metering_mode_(nullptr),\n> > > exposure_mode_(nullptr), constraint_mode_(nullptr),\n> > > frame_count_(0), lock_count_(0),\n> > > - last_target_exposure_(0.0),\n> > > - ev_(1.0), flicker_period_(0.0),\n> > > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> > > + last_target_exposure_(0s),\n> > > + ev_(1.0), flicker_period_(0s),\n> > > + max_shutter_(0s), fixed_shutter_(0s),\n> fixed_analogue_gain_(0.0)\n> > > {\n> > > memset(&awb_, 0, sizeof(awb_));\n> > > // Setting status_.total_exposure_value_ to zero initially\n> tells us\n> > > @@ -203,7 +212,7 @@ void Agc::Pause()\n> > >\n> > > void Agc::Resume()\n> > > {\n> > > - fixed_shutter_ = 0;\n> > > + fixed_shutter_ = 0s;\n> > > fixed_analogue_gain_ = 0;\n> > > }\n> > >\n> > > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)\n> > >\n> > > void Agc::SetFlickerPeriod(const Duration &flicker_period)\n> > > {\n> > > - flicker_period_ = flicker_period.get();\n> > > + flicker_period_ = flicker_period;\n> > > }\n> > >\n> > > void Agc::SetMaxShutter(const Duration &max_shutter)\n> > > {\n> > > - max_shutter_ = max_shutter.get();\n> > > + max_shutter_ = max_shutter;\n> > > }\n> > >\n> > > void Agc::SetFixedShutter(const Duration &fixed_shutter)\n> > > {\n> > > - fixed_shutter_ = fixed_shutter.get();\n> > > + fixed_shutter_ = fixed_shutter;\n> > > // Set this in case someone calls Pause() straight after.\n> > > status_.shutter_time = clipShutter(fixed_shutter_);\n> > > }\n> > > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n> const &camera_mode,\n> > > {\n> > > housekeepConfig();\n> > >\n> > > - double fixed_shutter = clipShutter(fixed_shutter_);\n> > > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> > > + Duration fixed_shutter = clipShutter(fixed_shutter_);\n> > > + if (fixed_shutter && fixed_analogue_gain_) {\n> > > // We're going to reset the algorithm here with these\n> fixed values.\n> > >\n> > > fetchAwbStatus(metadata);\n> > > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > > // Process has run, so we have meaningful values.\n> > > DeviceStatus device_status;\n> > > if (image_metadata->Get(\"device.status\",\n> device_status) == 0) {\n> > > - double actual_exposure =\n> device_status.shutter_speed *\n> > > -\n> device_status.analogue_gain;\n> > > + Duration actual_exposure =\n> device_status.shutter_speed *\n> > > +\n> device_status.analogue_gain;\n> > > if (actual_exposure) {\n> > > status_.digital_gain =\n> > > status_.total_exposure_value /\n> > > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > > std::min(status_.digital_gain,\n> 4.0));\n> > > LOG(RPiAgc, Debug) << \"Actual exposure\n> \" << actual_exposure;\n> > > LOG(RPiAgc, Debug) << \"Use\n> digital_gain \" << status_.digital_gain;\n> > > - LOG(RPiAgc, Debug) << \"Effective\n> exposure \" << actual_exposure * status_.digital_gain;\n> > > + LOG(RPiAgc, Debug) << \"Effective\n> exposure \"\n> > > + << actual_exposure\n> * status_.digital_gain;\n> > > // Decide whether AEC/AGC has\n> converged.\n> > > updateLockStatus(device_status);\n> > > }\n> > > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const\n> &device_status)\n> > > const double RESET_MARGIN = 1.5;\n> > >\n> > > // Add 200us to the exposure time error to allow for line\n> quantisation.\n> > > - double exposure_error = last_device_status_.shutter_speed *\n> ERROR_FACTOR + 200;\n> > > + Duration exposure_error = last_device_status_.shutter_speed *\n> ERROR_FACTOR + 200us;\n> > > double gain_error = last_device_status_.analogue_gain *\n> ERROR_FACTOR;\n> > > - double target_error = last_target_exposure_ * ERROR_FACTOR;\n> > > + Duration target_error = last_target_exposure_ * ERROR_FACTOR;\n> > >\n> > > // Note that we don't know the exposure/gain limits of the\n> sensor, so\n> > > // the values we keep requesting may be unachievable. For this\n> reason\n> > > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata\n> *image_metadata)\n> > > current_.analogue_gain = device_status->analogue_gain;\n> > > AgcStatus *agc_status =\n> > > image_metadata->GetLocked(\"agc.status\");\n> > > - current_.total_exposure = agc_status ?\n> agc_status->total_exposure_value : 0;\n> > > + current_.total_exposure = agc_status ?\n> agc_status->total_exposure_value : 0s;\n> > > current_.total_exposure_no_dg = current_.shutter *\n> current_.analogue_gain;\n> > > }\n> > >\n> > > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats\n> *statistics, Metadata *image_metadata,\n> > >\n> > > void Agc::computeTargetExposure(double gain)\n> > > {\n> > > - if (status_.fixed_shutter != 0.0 &&\n> status_.fixed_analogue_gain != 0.0) {\n> > > + if (status_.fixed_shutter && status_.fixed_analogue_gain) {\n> > > // When ag and shutter are both fixed, we need to\n> drive the\n> > > // total exposure so that we end up with a digital\n> gain of at least\n> > > // 1/min_colour_gain. Otherwise we'd desaturate\n> channels causing\n> > > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)\n> > > target_.total_exposure = current_.total_exposure_no_dg\n> * gain;\n> > > // The final target exposure is also limited to what\n> the exposure\n> > > // mode allows.\n> > > - double max_shutter = status_.fixed_shutter != 0.0\n> > > + Duration max_shutter = status_.fixed_shutter\n> > > ? status_.fixed_shutter\n> > > : exposure_mode_->shutter.back();\n> > > max_shutter = clipShutter(max_shutter);\n> > > - double max_total_exposure =\n> > > + Duration max_total_exposure =\n> > > max_shutter *\n> > > (status_.fixed_analogue_gain != 0.0\n> > > ? status_.fixed_analogue_gain\n> > > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)\n> > > if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||\n> > > frame_count_ <= config_.startup_frames)\n> > > speed = 1.0;\n> > > - if (filtered_.total_exposure == 0.0) {\n> > > + if (!filtered_.total_exposure) {\n> > > filtered_.total_exposure = target_.total_exposure;\n> > > filtered_.total_exposure_no_dg =\n> target_.total_exposure_no_dg;\n> > > } else {\n> > > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()\n> > > // Sending the fixed shutter/gain cases through the same code\n> may seem\n> > > // unnecessary, but it will make more sense when extend this\n> to cover\n> > > // variable aperture.\n> > > - double exposure_value = filtered_.total_exposure_no_dg;\n> > > - double shutter_time, analogue_gain;\n> > > - shutter_time = status_.fixed_shutter != 0.0\n> > > + Duration exposure_value = filtered_.total_exposure_no_dg;\n> > > + Duration shutter_time;\n> > > + double analogue_gain;\n> > > + shutter_time = status_.fixed_shutter\n> > > ? status_.fixed_shutter\n> > > : exposure_mode_->shutter[0];\n> > > shutter_time = clipShutter(shutter_time);\n> > > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()\n> > > if (shutter_time * analogue_gain < exposure_value) {\n> > > for (unsigned int stage = 1;\n> > > stage < exposure_mode_->gain.size(); stage++) {\n> > > - if (status_.fixed_shutter == 0.0) {\n> > > - double stage_shutter =\n> > > + if (!status_.fixed_shutter) {\n> > > + Duration stage_shutter =\n> > >\n> clipShutter(exposure_mode_->shutter[stage]);\n> > > if (stage_shutter * analogue_gain >=\n> > > exposure_value) {\n> > > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()\n> > > << analogue_gain;\n> > > // Finally adjust shutter time for flicker avoidance (require\n> both\n> > > // shutter and gain not to be fixed).\n> > > - if (status_.fixed_shutter == 0.0 &&\n> > > - status_.fixed_analogue_gain == 0.0 &&\n> > > - status_.flicker_period != 0.0) {\n> > > - int flicker_periods = shutter_time /\n> status_.flicker_period;\n> > > - if (flicker_periods > 0) {\n> > > - double new_shutter_time = flicker_periods *\n> status_.flicker_period;\n> > > + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&\n> > > + status_.flicker_period) {\n> > > + double flicker_periods = shutter_time /\n> status_.flicker_period;\n> > > + if (flicker_periods > 0.0) {\n> > > + Duration new_shutter_time = flicker_periods *\n> status_.flicker_period;\n> >\n> > Just wondering if this will behave the same now that flicker_periods\n> > is a double. Won't it now hold fractional values with the result that\n> > new_shutter_time isn't an exact multiple of the period?\n> >\n> > > analogue_gain *= shutter_time /\n> new_shutter_time;\n> > > // We should still not allow the ag to go over\n> the\n> > > // largest value in the exposure mode. Note\n> that this\n> > > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()\n> > > void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> > > {\n> > > status_.total_exposure_value = filtered_.total_exposure;\n> > > - status_.target_exposure_value = desaturate ? 0 :\n> target_.total_exposure_no_dg;\n> > > + status_.target_exposure_value = desaturate ? 0s :\n> target_.total_exposure_no_dg;\n> > > status_.shutter_time = filtered_.shutter;\n> > > status_.analogue_gain = filtered_.analogue_gain;\n> > > // Write to metadata as well, in case anyone wants to update\n> the camera\n> > > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata\n> *image_metadata, bool desaturate)\n> > > << \" analogue gain \" <<\n> filtered_.analogue_gain;\n> > > }\n> > >\n> > > -double Agc::clipShutter(double shutter)\n> > > +Duration Agc::clipShutter(const Duration &shutter)\n> > > {\n> > > + Duration clip_shutter = shutter;\n> >\n> > Given that we copy it anyway I wonder if we might have left this as\n> > call-by-value, resulting in fewer changes. But it's only the teeniest\n> > of nit-picks!\n> >\n> > If you can put my mind at rest over the flicker period thing:\n> >\n> > Reviewed-by: David Plowman \n> >\n> > Thanks!\n> > David\n> >\n> > > if (max_shutter_)\n> > > - shutter = std::min(shutter, max_shutter_);\n> > > - return shutter;\n> > > + clip_shutter = std::min(clip_shutter, max_shutter_);\n> > > + return clip_shutter;\n> > > }\n> > >\n> > > // Register algorithm with the system.\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > index 16a65959d90e..24a6610096c2 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > @@ -9,6 +9,8 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \"libcamera/internal/utils.h\"\n> > > +\n> > > #include \"../agc_algorithm.hpp\"\n> > > #include \"../agc_status.h\"\n> > > #include \"../pwl.hpp\"\n> > > @@ -22,13 +24,15 @@\n> > >\n> > > namespace RPiController {\n> > >\n> > > +using namespace std::literals::chrono_literals;\n> > > +\n> > > struct AgcMeteringMode {\n> > > double weights[AGC_STATS_SIZE];\n> > > void Read(boost::property_tree::ptree const ¶ms);\n> > > };\n> > >\n> > > struct AgcExposureMode {\n> > > - std::vector shutter;\n> > > + std::vector shutter;\n> > > std::vector gain;\n> > > void Read(boost::property_tree::ptree const ¶ms);\n> > > };\n> > > @@ -61,7 +65,7 @@ struct AgcConfig {\n> > > std::string default_exposure_mode;\n> > > std::string default_constraint_mode;\n> > > double base_ev;\n> > > - double default_exposure_time;\n> > > + Duration default_exposure_time;\n> > > double default_analogue_gain;\n> > > };\n> > >\n> > > @@ -101,19 +105,19 @@ private:\n> > > void filterExposure(bool desaturate);\n> > > void divideUpExposure();\n> > > void writeAndFinish(Metadata *image_metadata, bool desaturate);\n> > > - double clipShutter(double shutter);\n> > > + Duration clipShutter(const Duration &shutter);\n> > > AgcMeteringMode *metering_mode_;\n> > > AgcExposureMode *exposure_mode_;\n> > > AgcConstraintMode *constraint_mode_;\n> > > uint64_t frame_count_;\n> > > AwbStatus awb_;\n> > > struct ExposureValues {\n> > > - ExposureValues() : shutter(0), analogue_gain(0),\n> > > - total_exposure(0),\n> total_exposure_no_dg(0) {}\n> > > - double shutter;\n> > > + ExposureValues() : shutter(0s), analogue_gain(0),\n> > > + total_exposure(0s),\n> total_exposure_no_dg(0s) {}\n> > > + Duration shutter;\n> > > double analogue_gain;\n> > > - double total_exposure;\n> > > - double total_exposure_no_dg; // without digital gain\n> > > + Duration total_exposure;\n> > > + Duration total_exposure_no_dg; // without digital gain\n> > > };\n> > > ExposureValues current_; // values for the current frame\n> > > ExposureValues target_; // calculate the values we want here\n> > > @@ -121,15 +125,15 @@ private:\n> > > AgcStatus status_;\n> > > int lock_count_;\n> > > DeviceStatus last_device_status_;\n> > > - double last_target_exposure_;\n> > > + Duration last_target_exposure_;\n> > > // Below here the \"settings\" that applications can change.\n> > > std::string metering_mode_name_;\n> > > std::string exposure_mode_name_;\n> > > std::string constraint_mode_name_;\n> > > double ev_;\n> > > - double flicker_period_;\n> > > - double max_shutter_;\n> > > - double fixed_shutter_;\n> > > + Duration flicker_period_;\n> > > + Duration max_shutter_;\n> > > + Duration fixed_shutter_;\n> > > double fixed_analogue_gain_;\n> > > };\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > > index f74381cab2b4..0fa6457c1376 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > > @@ -16,6 +16,7 @@\n> > >\n> > > using namespace RPiController;\n> > > using namespace libcamera;\n> > > +using namespace std::literals::chrono_literals;\n> > >\n> > > LOG_DEFINE_CATEGORY(RPiLux)\n> > >\n> > > @@ -38,7 +39,7 @@ char const *Lux::Name() const\n> > > void Lux::Read(boost::property_tree::ptree const ¶ms)\n> > > {\n> > > reference_shutter_speed_ =\n> > > - params.get(\"reference_shutter_speed\");\n> > > + params.get(\"reference_shutter_speed\") * 1.0us;\n> > > reference_gain_ = params.get(\"reference_gain\");\n> > > reference_aperture_ = params.get(\"reference_aperture\",\n> 1.0);\n> > > reference_Y_ = params.get(\"reference_Y\");\n> > > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)\n> > > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> > > {\n> > > // set some initial values to shut the compiler up\n> > > - DeviceStatus device_status =\n> > > - { .shutter_speed = 1.0,\n> > > - .analogue_gain = 1.0,\n> > > - .lens_position = 0.0,\n> > > - .aperture = 0.0,\n> > > - .flash_intensity = 0.0 };\n> > > + DeviceStatus device_status = { .shutter_speed = 1.0ms,\n> > > + .analogue_gain = 1.0,\n> > > + .lens_position = 0.0,\n> > > + .aperture = 0.0,\n> > > + .flash_intensity = 0.0 };\n> > > if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> > > double current_gain = device_status.analogue_gain;\n> > > - double current_shutter_speed =\n> device_status.shutter_speed;\n> > > double current_aperture = device_status.aperture;\n> > > if (current_aperture == 0)\n> > > current_aperture = current_aperture_;\n> > > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata\n> *image_metadata)\n> > > double current_Y = sum / (double)num + .5;\n> > > double gain_ratio = reference_gain_ / current_gain;\n> > > double shutter_speed_ratio =\n> > > - reference_shutter_speed_ /\n> current_shutter_speed;\n> > > + reference_shutter_speed_ /\n> device_status.shutter_speed;\n> > > double aperture_ratio = reference_aperture_ /\n> current_aperture;\n> > > double Y_ratio = current_Y * (65536 / num_bins) /\n> reference_Y_;\n> > > double estimated_lux = shutter_speed_ratio *\n> gain_ratio *\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > > index f9090484a136..45c844393e62 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > > @@ -8,6 +8,8 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"libcamera/internal/utils.h\"\n> > > +\n> > > #include \"../lux_status.h\"\n> > > #include \"../algorithm.hpp\"\n> > >\n> > > @@ -28,7 +30,7 @@ public:\n> > > private:\n> > > // These values define the conditions of the reference image,\n> against\n> > > // which we compare the new image.\n> > > - double reference_shutter_speed_; // in micro-seconds\n> > > + libcamera::utils::Duration reference_shutter_speed_;\n> > > double reference_gain_;\n> > > double reference_aperture_; // units of 1/f\n> > > double reference_Y_; // out of 65536\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index f8f36420b076..b77230ded591 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,\n> ipa::RPi::StartConfig *startConf\n> > >\n> > > /* SwitchMode may supply updated exposure/gain values to use.\n> */\n> > > AgcStatus agcStatus;\n> > > - agcStatus.shutter_time = 0.0;\n> > > + agcStatus.shutter_time = 0.0s;\n> > > agcStatus.analogue_gain = 0.0;\n> > >\n> > > metadata.Get(\"agc.status\", agcStatus);\n> > > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain\n> != 0.0) {\n> > > + if (agcStatus.shutter_time && agcStatus.analogue_gain) {\n> > > ControlList ctrls(sensorCtrls_);\n> > > applyAGC(&agcStatus, ctrls);\n> > > startConfig->controls = std::move(ctrls);\n> > > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > > /* Supply initial values for gain and exposure. */\n> > > ControlList ctrls(sensorCtrls_);\n> > > AgcStatus agcStatus;\n> > > - agcStatus.shutter_time =\n> DefaultExposureTime.get();\n> > > + agcStatus.shutter_time = DefaultExposureTime;\n> > > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > applyAGC(&agcStatus, ctrls);\n> > >\n> > > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()\n> > > */\n> > > DeviceStatus *deviceStatus =\n> rpiMetadata_.GetLocked(\"device.status\");\n> > > if (deviceStatus) {\n> > > - libcameraMetadata_.set(controls::ExposureTime,\n> deviceStatus->shutter_speed);\n> > > + libcameraMetadata_.set(controls::ExposureTime,\n> > > +\n> deviceStatus->shutter_speed.get());\n> > > libcameraMetadata_.set(controls::AnalogueGain,\n> deviceStatus->analogue_gain);\n> > > }\n> > >\n> > > @@ -1017,7 +1018,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls)\n> > > int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > >\n> > > - deviceStatus.shutter_speed =\n> helper_->Exposure(exposureLines).get();\n> > > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > >\n> > > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > > @@ -1103,7 +1104,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > >\n> > > /* GetVBlanking might clip exposure time to the fps limits. */\n> > > - Duration exposure = agcStatus->shutter_time * 1.0us;\n> > > + Duration exposure = agcStatus->shutter_time;\n> > > int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> > > int32_t exposureLines = helper_->ExposureLines(exposure);\n> > >\n> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > > index 3131aa2d6666..5fa6f8366f3d 100644\n> > > --- a/src/libcamera/utils.cpp\n> > > +++ b/src/libcamera/utils.cpp\n> > > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()\n> > > * loop, iterates over an indexed view of the \\a iterable\n> > > */\n> > >\n> > > +/**\n> > > + * \\typedef BaseDuration\n> > > + * \\brief Base struct for the \\a Duration helper class.\n> > > + */\n> > > +\n> > > /**\n> > > * \\class Duration\n> > > - * \\brief Helper for std::chrono::duration types. Derived from \\a\n> BaseDuration\n> > > + * \\brief Helper class for std::chrono::duration types.\n> > > */\n> > >\n> > > /**\n> > > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()\n> > > * \\param[in] d The std::chrono::duration object to convert from.\n> > > *\n> > > * Constructs a \\a Duration object from a \\a std::chrono::duration\n> object,\n> > > - * converting the representation to \\a BaseDuration type.\n> > > + * internally converting the representation to \\a BaseDuration type.\n> > > */\n> > >\n> > > /**\n> > > --\n> > > 2.25.1\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5A0ACC3200\n\tfor ;\n\tFri, 21 May 2021 12:49:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C9D016891F;\n\tFri, 21 May 2021 14:49:47 +0200 (CEST)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CE5F568911\n\tfor ;\n\tFri, 21 May 2021 14:49:46 +0200 (CEST)","by mail-lj1-x22c.google.com with SMTP id a4so9000173ljd.5\n\tfor ;\n\tFri, 21 May 2021 05:49:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"i+gK45rn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=/v41P2yXTrgKFZTMAxyZDQNHSanl3T4RRSu+p7jSCMo=;\n\tb=i+gK45rn8ZHQe6Jqoh5wfIma44FbSSs8JI7Y3evdJ3siGe654cIPFiM0DZ5f5aPGQT\n\t9SOR6SZp4liN/T399FSA6KdX6QDISGhJSgJN3a9bq8frmH1QVNXjkuJcsfFZHTrK5kZm\n\tHIL48VkvEam/yCBo3sHEO3bqLPYYYl89kSRcrtaTtW2weZiKCniF6OLJyPVqa1qE0PX+\n\tgr1DZNP1hy7qf/qRXbyej5H3TDatgC9xjEdUVAPIIlYldHybbSbWmXtIoS0GDFBhm2ol\n\tZll2JXZ28l1xWyUMxVRDgavee7ZtCzDo4yeHLs4aLoqiw1f6iM+alRL9aod8cTDRcSUL\n\tSTdQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=/v41P2yXTrgKFZTMAxyZDQNHSanl3T4RRSu+p7jSCMo=;\n\tb=AxucgE3jKGfw7mOexYFsJelYw+ccd+VkAawwNLDuox7KT3YBSMsiRea0USqBV34RMU\n\t6D0N8f2+cvbKkLPn1rBNNQj+aeKsYytTGWqZZeIowkiYp8YQMEtIG01NSpCaEdLSUTa2\n\t47xrzQWEfibbjHuWvpWzvyfqWyrbyVkd7BCwxjBY2XELvxkDMrFmDQrCbD2DvhQ6u1oY\n\tbIiAR9YYneU4j3dq+ff8E7tYfKs3/KWr6iJqaPFn4CJDmnGKISBmTxBkuVd6NbBbXzSW\n\tuOTEUFs0OeFeG0dNh1Rn9Gxa7eyaJWlD7KN4ZRPDvPKUFOJ9RD5NXfSS92f3/vkhbx9g\n\tIazw==","X-Gm-Message-State":"AOAM531OFVapkuKaL9B03Wc5wewdMQxq32T3ryLWZ0ubyistiMB2lpCi\n\tdH7LadNmo4AiCQwbWex1QqQ1RtdKdXyjDSWO/RptyVswr8o=","X-Google-Smtp-Source":"ABdhPJzT7OjPs7Vw0JBrLnyTZ1NikmpI0qe7faXddKfJHT3yEn2yHzEevF/leIqBwBnWoAdr7pQJuhbFhZlruX4h2nY=","X-Received":"by 2002:a2e:9145:: with SMTP id q5mr6655981ljg.400.1621601386166;\n\tFri, 21 May 2021 05:49:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-5-naush@raspberrypi.com>\n\t\n\t<20210521123800.7yktaq6ek4cgfw4i@uno.localdomain>","In-Reply-To":"<20210521123800.7yktaq6ek4cgfw4i@uno.localdomain>","From":"Naushir Patuck ","Date":"Fri, 21 May 2021 13:49:30 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"00000000000097077605c2d67e0a\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17103,"web_url":"https://patchwork.libcamera.org/comment/17103/","msgid":"<20210521130954.tcu3rjltzl4tv2bw@uno.localdomain>","date":"2021-05-21T13:09:54","subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Fri, May 21, 2021 at 01:49:30PM +0100, Naushir Patuck wrote:\n> On Fri, 21 May 2021 at 13:37, Jacopo Mondi wrote:\n>\n> > Hello,\n> >\n> > On Fri, May 21, 2021 at 12:18:55PM +0100, David Plowman wrote:\n> > > Hi Naush\n> > >\n> > > Thanks for all this work!\n> > >\n> > > On Fri, 21 May 2021 at 09:05, Naushir Patuck \n> > wrote:\n> > > >\n> > > > Convert the core AGC and Lux controller code to use\n> > > > utils::Duration for all exposure time related variables and\n> > > > calculations.\n> > > >\n> > > > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus\n> > > > to use utils::Duration.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper.cpp | 2 +-\n> > > > src/ipa/raspberrypi/controller/agc_status.h | 14 +--\n> > > > .../raspberrypi/controller/device_status.h | 6 +-\n> > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 +++++++++++--------\n> > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---\n> > > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--\n> > > > src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 13 +--\n> > > > src/libcamera/utils.cpp | 9 +-\n> > > > 9 files changed, 105 insertions(+), 77 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > index feb02d1806b2..968fae13bd5e 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span > uint8_t> buffer,\n> > > > return;\n> > > > }\n> > > >\n> > > > - deviceStatus.shutter_speed =\n> > Exposure(exposureLines).get();\n> > > > + deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > > deviceStatus.analogue_gain = Gain(gainCode);\n> > > >\n> > > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > > diff --git a/src/ipa/raspberrypi/controller/agc_status.h\n> > b/src/ipa/raspberrypi/controller/agc_status.h\n> > > > index 10381c90a313..c893715af588 100644\n> > > > --- a/src/ipa/raspberrypi/controller/agc_status.h\n> > > > +++ b/src/ipa/raspberrypi/controller/agc_status.h\n> > > > @@ -6,6 +6,8 @@\n> > > > */\n> > > > #pragma once\n> > > >\n> > > > +#include \"libcamera/internal/utils.h\"\n> > > > +\n> > > > // The AGC algorithm should post the following structure into the\n> > image's\n> > > > // \"agc.status\" metadata.\n> > > >\n> > > > @@ -17,18 +19,20 @@ extern \"C\" {\n> > > > // seen statistics and calculated meaningful values. The contents\n> > should be\n> > > > // ignored until then.\n> > > >\n> > > > +using libcamera::utils::Duration;\n> > > > +\n> > > > struct AgcStatus {\n> > > > - double total_exposure_value; // value for all exposure and\n> > gain for this image\n> > > > - double target_exposure_value; // (unfiltered) target total\n> > exposure AGC is aiming for\n> > > > - double shutter_time;\n> > > > + Duration total_exposure_value; // value for all exposure and\n> > gain for this image\n> > > > + Duration target_exposure_value; // (unfiltered) target total\n> > exposure AGC is aiming for\n> > > > + Duration shutter_time;\n> > > > double analogue_gain;\n> > > > char exposure_mode[32];\n> > > > char constraint_mode[32];\n> > > > char metering_mode[32];\n> > > > double ev;\n> > > > - double flicker_period;\n> > > > + Duration flicker_period;\n> > > > int floating_region_enable;\n> > > > - double fixed_shutter;\n> > > > + Duration fixed_shutter;\n> > > > double fixed_analogue_gain;\n> > > > double digital_gain;\n> > > > int locked;\n> > > > diff --git a/src/ipa/raspberrypi/controller/device_status.h\n> > b/src/ipa/raspberrypi/controller/device_status.h\n> > > > index aa08608b5d40..131b4cd344ee 100644\n> > > > --- a/src/ipa/raspberrypi/controller/device_status.h\n> > > > +++ b/src/ipa/raspberrypi/controller/device_status.h\n> > > > @@ -6,6 +6,8 @@\n> > > > */\n> > > > #pragma once\n> > > >\n> > > > +#include \"libcamera/internal/utils.h\"\n> > > > +\n> > > > // Definition of \"device metadata\" which stores things like shutter\n> > time and\n> > > > // analogue gain that downstream control algorithms will want to know.\n> > > >\n> > > > @@ -14,8 +16,8 @@ extern \"C\" {\n> > > > #endif\n> > > >\n> > > > struct DeviceStatus {\n> > > > - // time shutter is open, in microseconds\n> > > > - double shutter_speed;\n> > > > + // time shutter is open\n> > > > + libcamera::utils::Duration shutter_speed;\n> > > > double analogue_gain;\n> > > > // 1.0/distance-in-metres, or 0 if unknown\n> > > > double lens_position;\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > index 482eb3ef962d..46742d845034 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > @@ -21,6 +21,7 @@\n> > > >\n> > > > using namespace RPiController;\n> > > > using namespace libcamera;\n> > > > +using utils::operator<<;\n> > >\n> > > Still can't say that I like this... just because someone wants to use\n> > > Durations, it seems like an awkward thing to make them remember. But\n> > > unless anyone has anything better....\n> >\n> > I got the same feeling while reading the rest of the patches.\n> > I can't say toString() is that better though, but probably is less\n> > implicit that this using statment, which if read in isolation does not\n> > immediately relates to the Duration class ?\n> >\n>\n> I do agree with these points, and I don't like this as well. I can see two\n> alternatives without\n> using toString():\n>\n> 1) Move the Duration class into its own namespace under utils, so then the\n> above statement\n> will look a bit more informative:\n>\n> using utils::Duration::operator<<;\n>\n> 2) Move the operator() out of the utils namespace. This way we eliminate\n> the need for the\n> statement completely.\n\nI vote for 2 if it can be done nicely!\n\n>\n> Regards,\n> Naush\n>\n>\n> >\n> > >\n> > > >\n> > > > LOG_DEFINE_CATEGORY(RPiAgc)\n> > > >\n> > > > @@ -55,19 +56,27 @@ read_metering_modes(std::map > AgcMeteringMode> &metering_modes,\n> > > > return first;\n> > > > }\n> > > >\n> > > > -static int read_double_list(std::vector &list,\n> > > > - boost::property_tree::ptree const ¶ms)\n> > > > +static int read_list(std::vector &list,\n> > > > + boost::property_tree::ptree const ¶ms)\n> > > > {\n> > > > for (auto &p : params)\n> > > > list.push_back(p.second.get_value());\n> > > > return list.size();\n> > > > }\n> > > >\n> > > > +static int read_list(std::vector &list,\n> > > > + boost::property_tree::ptree const ¶ms)\n> > > > +{\n> > > > + for (auto &p : params)\n> > > > + list.push_back(p.second.get_value() * 1us);\n> > > > + return list.size();\n> > > > +}\n> > > > +\n> > > > void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms)\n> > > > {\n> > > > int num_shutters =\n> > > > - read_double_list(shutter, params.get_child(\"shutter\"));\n> > > > - int num_ags = read_double_list(gain, params.get_child(\"gain\"));\n> > > > + read_list(shutter, params.get_child(\"shutter\"));\n> > > > + int num_ags = read_list(gain, params.get_child(\"gain\"));\n> > > > if (num_shutters < 2 || num_ags < 2)\n> > > > throw std::runtime_error(\n> > > > \"AgcConfig: must have at least two entries in\n> > exposure profile\");\n> > > > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree\n> > const ¶ms)\n> > > > params.get(\"fast_reduce_threshold\", 0.4);\n> > > > base_ev = params.get(\"base_ev\", 1.0);\n> > > > // Start with quite a low value as ramping up is easier than\n> > ramping down.\n> > > > - default_exposure_time =\n> > params.get(\"default_exposure_time\", 1000);\n> > > > + default_exposure_time =\n> > params.get(\"default_exposure_time\", 1000) * 1us;\n> > > > default_analogue_gain =\n> > params.get(\"default_analogue_gain\", 1.0);\n> > > > }\n> > > >\n> > > > @@ -155,9 +164,9 @@ Agc::Agc(Controller *controller)\n> > > > : AgcAlgorithm(controller), metering_mode_(nullptr),\n> > > > exposure_mode_(nullptr), constraint_mode_(nullptr),\n> > > > frame_count_(0), lock_count_(0),\n> > > > - last_target_exposure_(0.0),\n> > > > - ev_(1.0), flicker_period_(0.0),\n> > > > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> > > > + last_target_exposure_(0s),\n> > > > + ev_(1.0), flicker_period_(0s),\n> > > > + max_shutter_(0s), fixed_shutter_(0s),\n> > fixed_analogue_gain_(0.0)\n> > > > {\n> > > > memset(&awb_, 0, sizeof(awb_));\n> > > > // Setting status_.total_exposure_value_ to zero initially\n> > tells us\n> > > > @@ -203,7 +212,7 @@ void Agc::Pause()\n> > > >\n> > > > void Agc::Resume()\n> > > > {\n> > > > - fixed_shutter_ = 0;\n> > > > + fixed_shutter_ = 0s;\n> > > > fixed_analogue_gain_ = 0;\n> > > > }\n> > > >\n> > > > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev)\n> > > >\n> > > > void Agc::SetFlickerPeriod(const Duration &flicker_period)\n> > > > {\n> > > > - flicker_period_ = flicker_period.get();\n> > > > + flicker_period_ = flicker_period;\n> > > > }\n> > > >\n> > > > void Agc::SetMaxShutter(const Duration &max_shutter)\n> > > > {\n> > > > - max_shutter_ = max_shutter.get();\n> > > > + max_shutter_ = max_shutter;\n> > > > }\n> > > >\n> > > > void Agc::SetFixedShutter(const Duration &fixed_shutter)\n> > > > {\n> > > > - fixed_shutter_ = fixed_shutter.get();\n> > > > + fixed_shutter_ = fixed_shutter;\n> > > > // Set this in case someone calls Pause() straight after.\n> > > > status_.shutter_time = clipShutter(fixed_shutter_);\n> > > > }\n> > > > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n> > const &camera_mode,\n> > > > {\n> > > > housekeepConfig();\n> > > >\n> > > > - double fixed_shutter = clipShutter(fixed_shutter_);\n> > > > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> > > > + Duration fixed_shutter = clipShutter(fixed_shutter_);\n> > > > + if (fixed_shutter && fixed_analogue_gain_) {\n> > > > // We're going to reset the algorithm here with these\n> > fixed values.\n> > > >\n> > > > fetchAwbStatus(metadata);\n> > > > @@ -312,8 +321,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > > > // Process has run, so we have meaningful values.\n> > > > DeviceStatus device_status;\n> > > > if (image_metadata->Get(\"device.status\",\n> > device_status) == 0) {\n> > > > - double actual_exposure =\n> > device_status.shutter_speed *\n> > > > -\n> > device_status.analogue_gain;\n> > > > + Duration actual_exposure =\n> > device_status.shutter_speed *\n> > > > +\n> > device_status.analogue_gain;\n> > > > if (actual_exposure) {\n> > > > status_.digital_gain =\n> > > > status_.total_exposure_value /\n> > > > @@ -326,7 +335,8 @@ void Agc::Prepare(Metadata *image_metadata)\n> > > > std::min(status_.digital_gain,\n> > 4.0));\n> > > > LOG(RPiAgc, Debug) << \"Actual exposure\n> > \" << actual_exposure;\n> > > > LOG(RPiAgc, Debug) << \"Use\n> > digital_gain \" << status_.digital_gain;\n> > > > - LOG(RPiAgc, Debug) << \"Effective\n> > exposure \" << actual_exposure * status_.digital_gain;\n> > > > + LOG(RPiAgc, Debug) << \"Effective\n> > exposure \"\n> > > > + << actual_exposure\n> > * status_.digital_gain;\n> > > > // Decide whether AEC/AGC has\n> > converged.\n> > > > updateLockStatus(device_status);\n> > > > }\n> > > > @@ -370,9 +380,9 @@ void Agc::updateLockStatus(DeviceStatus const\n> > &device_status)\n> > > > const double RESET_MARGIN = 1.5;\n> > > >\n> > > > // Add 200us to the exposure time error to allow for line\n> > quantisation.\n> > > > - double exposure_error = last_device_status_.shutter_speed *\n> > ERROR_FACTOR + 200;\n> > > > + Duration exposure_error = last_device_status_.shutter_speed *\n> > ERROR_FACTOR + 200us;\n> > > > double gain_error = last_device_status_.analogue_gain *\n> > ERROR_FACTOR;\n> > > > - double target_error = last_target_exposure_ * ERROR_FACTOR;\n> > > > + Duration target_error = last_target_exposure_ * ERROR_FACTOR;\n> > > >\n> > > > // Note that we don't know the exposure/gain limits of the\n> > sensor, so\n> > > > // the values we keep requesting may be unachievable. For this\n> > reason\n> > > > @@ -462,7 +472,7 @@ void Agc::fetchCurrentExposure(Metadata\n> > *image_metadata)\n> > > > current_.analogue_gain = device_status->analogue_gain;\n> > > > AgcStatus *agc_status =\n> > > > image_metadata->GetLocked(\"agc.status\");\n> > > > - current_.total_exposure = agc_status ?\n> > agc_status->total_exposure_value : 0;\n> > > > + current_.total_exposure = agc_status ?\n> > agc_status->total_exposure_value : 0s;\n> > > > current_.total_exposure_no_dg = current_.shutter *\n> > current_.analogue_gain;\n> > > > }\n> > > >\n> > > > @@ -573,7 +583,7 @@ void Agc::computeGain(bcm2835_isp_stats\n> > *statistics, Metadata *image_metadata,\n> > > >\n> > > > void Agc::computeTargetExposure(double gain)\n> > > > {\n> > > > - if (status_.fixed_shutter != 0.0 &&\n> > status_.fixed_analogue_gain != 0.0) {\n> > > > + if (status_.fixed_shutter && status_.fixed_analogue_gain) {\n> > > > // When ag and shutter are both fixed, we need to\n> > drive the\n> > > > // total exposure so that we end up with a digital\n> > gain of at least\n> > > > // 1/min_colour_gain. Otherwise we'd desaturate\n> > channels causing\n> > > > @@ -588,11 +598,11 @@ void Agc::computeTargetExposure(double gain)\n> > > > target_.total_exposure = current_.total_exposure_no_dg\n> > * gain;\n> > > > // The final target exposure is also limited to what\n> > the exposure\n> > > > // mode allows.\n> > > > - double max_shutter = status_.fixed_shutter != 0.0\n> > > > + Duration max_shutter = status_.fixed_shutter\n> > > > ? status_.fixed_shutter\n> > > > : exposure_mode_->shutter.back();\n> > > > max_shutter = clipShutter(max_shutter);\n> > > > - double max_total_exposure =\n> > > > + Duration max_total_exposure =\n> > > > max_shutter *\n> > > > (status_.fixed_analogue_gain != 0.0\n> > > > ? status_.fixed_analogue_gain\n> > > > @@ -637,7 +647,7 @@ void Agc::filterExposure(bool desaturate)\n> > > > if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||\n> > > > frame_count_ <= config_.startup_frames)\n> > > > speed = 1.0;\n> > > > - if (filtered_.total_exposure == 0.0) {\n> > > > + if (!filtered_.total_exposure) {\n> > > > filtered_.total_exposure = target_.total_exposure;\n> > > > filtered_.total_exposure_no_dg =\n> > target_.total_exposure_no_dg;\n> > > > } else {\n> > > > @@ -674,9 +684,10 @@ void Agc::divideUpExposure()\n> > > > // Sending the fixed shutter/gain cases through the same code\n> > may seem\n> > > > // unnecessary, but it will make more sense when extend this\n> > to cover\n> > > > // variable aperture.\n> > > > - double exposure_value = filtered_.total_exposure_no_dg;\n> > > > - double shutter_time, analogue_gain;\n> > > > - shutter_time = status_.fixed_shutter != 0.0\n> > > > + Duration exposure_value = filtered_.total_exposure_no_dg;\n> > > > + Duration shutter_time;\n> > > > + double analogue_gain;\n> > > > + shutter_time = status_.fixed_shutter\n> > > > ? status_.fixed_shutter\n> > > > : exposure_mode_->shutter[0];\n> > > > shutter_time = clipShutter(shutter_time);\n> > > > @@ -686,8 +697,8 @@ void Agc::divideUpExposure()\n> > > > if (shutter_time * analogue_gain < exposure_value) {\n> > > > for (unsigned int stage = 1;\n> > > > stage < exposure_mode_->gain.size(); stage++) {\n> > > > - if (status_.fixed_shutter == 0.0) {\n> > > > - double stage_shutter =\n> > > > + if (!status_.fixed_shutter) {\n> > > > + Duration stage_shutter =\n> > > >\n> > clipShutter(exposure_mode_->shutter[stage]);\n> > > > if (stage_shutter * analogue_gain >=\n> > > > exposure_value) {\n> > > > @@ -713,12 +724,11 @@ void Agc::divideUpExposure()\n> > > > << analogue_gain;\n> > > > // Finally adjust shutter time for flicker avoidance (require\n> > both\n> > > > // shutter and gain not to be fixed).\n> > > > - if (status_.fixed_shutter == 0.0 &&\n> > > > - status_.fixed_analogue_gain == 0.0 &&\n> > > > - status_.flicker_period != 0.0) {\n> > > > - int flicker_periods = shutter_time /\n> > status_.flicker_period;\n> > > > - if (flicker_periods > 0) {\n> > > > - double new_shutter_time = flicker_periods *\n> > status_.flicker_period;\n> > > > + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&\n> > > > + status_.flicker_period) {\n> > > > + double flicker_periods = shutter_time /\n> > status_.flicker_period;\n> > > > + if (flicker_periods > 0.0) {\n> > > > + Duration new_shutter_time = flicker_periods *\n> > status_.flicker_period;\n> > >\n> > > Just wondering if this will behave the same now that flicker_periods\n> > > is a double. Won't it now hold fractional values with the result that\n> > > new_shutter_time isn't an exact multiple of the period?\n> > >\n> > > > analogue_gain *= shutter_time /\n> > new_shutter_time;\n> > > > // We should still not allow the ag to go over\n> > the\n> > > > // largest value in the exposure mode. Note\n> > that this\n> > > > @@ -738,7 +748,7 @@ void Agc::divideUpExposure()\n> > > > void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> > > > {\n> > > > status_.total_exposure_value = filtered_.total_exposure;\n> > > > - status_.target_exposure_value = desaturate ? 0 :\n> > target_.total_exposure_no_dg;\n> > > > + status_.target_exposure_value = desaturate ? 0s :\n> > target_.total_exposure_no_dg;\n> > > > status_.shutter_time = filtered_.shutter;\n> > > > status_.analogue_gain = filtered_.analogue_gain;\n> > > > // Write to metadata as well, in case anyone wants to update\n> > the camera\n> > > > @@ -750,11 +760,12 @@ void Agc::writeAndFinish(Metadata\n> > *image_metadata, bool desaturate)\n> > > > << \" analogue gain \" <<\n> > filtered_.analogue_gain;\n> > > > }\n> > > >\n> > > > -double Agc::clipShutter(double shutter)\n> > > > +Duration Agc::clipShutter(const Duration &shutter)\n> > > > {\n> > > > + Duration clip_shutter = shutter;\n> > >\n> > > Given that we copy it anyway I wonder if we might have left this as\n> > > call-by-value, resulting in fewer changes. But it's only the teeniest\n> > > of nit-picks!\n> > >\n> > > If you can put my mind at rest over the flicker period thing:\n> > >\n> > > Reviewed-by: David Plowman \n> > >\n> > > Thanks!\n> > > David\n> > >\n> > > > if (max_shutter_)\n> > > > - shutter = std::min(shutter, max_shutter_);\n> > > > - return shutter;\n> > > > + clip_shutter = std::min(clip_shutter, max_shutter_);\n> > > > + return clip_shutter;\n> > > > }\n> > > >\n> > > > // Register algorithm with the system.\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > index 16a65959d90e..24a6610096c2 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > @@ -9,6 +9,8 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/utils.h\"\n> > > > +\n> > > > #include \"../agc_algorithm.hpp\"\n> > > > #include \"../agc_status.h\"\n> > > > #include \"../pwl.hpp\"\n> > > > @@ -22,13 +24,15 @@\n> > > >\n> > > > namespace RPiController {\n> > > >\n> > > > +using namespace std::literals::chrono_literals;\n> > > > +\n> > > > struct AgcMeteringMode {\n> > > > double weights[AGC_STATS_SIZE];\n> > > > void Read(boost::property_tree::ptree const ¶ms);\n> > > > };\n> > > >\n> > > > struct AgcExposureMode {\n> > > > - std::vector shutter;\n> > > > + std::vector shutter;\n> > > > std::vector gain;\n> > > > void Read(boost::property_tree::ptree const ¶ms);\n> > > > };\n> > > > @@ -61,7 +65,7 @@ struct AgcConfig {\n> > > > std::string default_exposure_mode;\n> > > > std::string default_constraint_mode;\n> > > > double base_ev;\n> > > > - double default_exposure_time;\n> > > > + Duration default_exposure_time;\n> > > > double default_analogue_gain;\n> > > > };\n> > > >\n> > > > @@ -101,19 +105,19 @@ private:\n> > > > void filterExposure(bool desaturate);\n> > > > void divideUpExposure();\n> > > > void writeAndFinish(Metadata *image_metadata, bool desaturate);\n> > > > - double clipShutter(double shutter);\n> > > > + Duration clipShutter(const Duration &shutter);\n> > > > AgcMeteringMode *metering_mode_;\n> > > > AgcExposureMode *exposure_mode_;\n> > > > AgcConstraintMode *constraint_mode_;\n> > > > uint64_t frame_count_;\n> > > > AwbStatus awb_;\n> > > > struct ExposureValues {\n> > > > - ExposureValues() : shutter(0), analogue_gain(0),\n> > > > - total_exposure(0),\n> > total_exposure_no_dg(0) {}\n> > > > - double shutter;\n> > > > + ExposureValues() : shutter(0s), analogue_gain(0),\n> > > > + total_exposure(0s),\n> > total_exposure_no_dg(0s) {}\n> > > > + Duration shutter;\n> > > > double analogue_gain;\n> > > > - double total_exposure;\n> > > > - double total_exposure_no_dg; // without digital gain\n> > > > + Duration total_exposure;\n> > > > + Duration total_exposure_no_dg; // without digital gain\n> > > > };\n> > > > ExposureValues current_; // values for the current frame\n> > > > ExposureValues target_; // calculate the values we want here\n> > > > @@ -121,15 +125,15 @@ private:\n> > > > AgcStatus status_;\n> > > > int lock_count_;\n> > > > DeviceStatus last_device_status_;\n> > > > - double last_target_exposure_;\n> > > > + Duration last_target_exposure_;\n> > > > // Below here the \"settings\" that applications can change.\n> > > > std::string metering_mode_name_;\n> > > > std::string exposure_mode_name_;\n> > > > std::string constraint_mode_name_;\n> > > > double ev_;\n> > > > - double flicker_period_;\n> > > > - double max_shutter_;\n> > > > - double fixed_shutter_;\n> > > > + Duration flicker_period_;\n> > > > + Duration max_shutter_;\n> > > > + Duration fixed_shutter_;\n> > > > double fixed_analogue_gain_;\n> > > > };\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > > > index f74381cab2b4..0fa6457c1376 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > > > @@ -16,6 +16,7 @@\n> > > >\n> > > > using namespace RPiController;\n> > > > using namespace libcamera;\n> > > > +using namespace std::literals::chrono_literals;\n> > > >\n> > > > LOG_DEFINE_CATEGORY(RPiLux)\n> > > >\n> > > > @@ -38,7 +39,7 @@ char const *Lux::Name() const\n> > > > void Lux::Read(boost::property_tree::ptree const ¶ms)\n> > > > {\n> > > > reference_shutter_speed_ =\n> > > > - params.get(\"reference_shutter_speed\");\n> > > > + params.get(\"reference_shutter_speed\") * 1.0us;\n> > > > reference_gain_ = params.get(\"reference_gain\");\n> > > > reference_aperture_ = params.get(\"reference_aperture\",\n> > 1.0);\n> > > > reference_Y_ = params.get(\"reference_Y\");\n> > > > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)\n> > > > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> > > > {\n> > > > // set some initial values to shut the compiler up\n> > > > - DeviceStatus device_status =\n> > > > - { .shutter_speed = 1.0,\n> > > > - .analogue_gain = 1.0,\n> > > > - .lens_position = 0.0,\n> > > > - .aperture = 0.0,\n> > > > - .flash_intensity = 0.0 };\n> > > > + DeviceStatus device_status = { .shutter_speed = 1.0ms,\n> > > > + .analogue_gain = 1.0,\n> > > > + .lens_position = 0.0,\n> > > > + .aperture = 0.0,\n> > > > + .flash_intensity = 0.0 };\n> > > > if (image_metadata->Get(\"device.status\", device_status) == 0) {\n> > > > double current_gain = device_status.analogue_gain;\n> > > > - double current_shutter_speed =\n> > device_status.shutter_speed;\n> > > > double current_aperture = device_status.aperture;\n> > > > if (current_aperture == 0)\n> > > > current_aperture = current_aperture_;\n> > > > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata\n> > *image_metadata)\n> > > > double current_Y = sum / (double)num + .5;\n> > > > double gain_ratio = reference_gain_ / current_gain;\n> > > > double shutter_speed_ratio =\n> > > > - reference_shutter_speed_ /\n> > current_shutter_speed;\n> > > > + reference_shutter_speed_ /\n> > device_status.shutter_speed;\n> > > > double aperture_ratio = reference_aperture_ /\n> > current_aperture;\n> > > > double Y_ratio = current_Y * (65536 / num_bins) /\n> > reference_Y_;\n> > > > double estimated_lux = shutter_speed_ratio *\n> > gain_ratio *\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > > > index f9090484a136..45c844393e62 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp\n> > > > @@ -8,6 +8,8 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/utils.h\"\n> > > > +\n> > > > #include \"../lux_status.h\"\n> > > > #include \"../algorithm.hpp\"\n> > > >\n> > > > @@ -28,7 +30,7 @@ public:\n> > > > private:\n> > > > // These values define the conditions of the reference image,\n> > against\n> > > > // which we compare the new image.\n> > > > - double reference_shutter_speed_; // in micro-seconds\n> > > > + libcamera::utils::Duration reference_shutter_speed_;\n> > > > double reference_gain_;\n> > > > double reference_aperture_; // units of 1/f\n> > > > double reference_Y_; // out of 65536\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index f8f36420b076..b77230ded591 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -225,11 +225,11 @@ void IPARPi::start(const ControlList &controls,\n> > ipa::RPi::StartConfig *startConf\n> > > >\n> > > > /* SwitchMode may supply updated exposure/gain values to use.\n> > */\n> > > > AgcStatus agcStatus;\n> > > > - agcStatus.shutter_time = 0.0;\n> > > > + agcStatus.shutter_time = 0.0s;\n> > > > agcStatus.analogue_gain = 0.0;\n> > > >\n> > > > metadata.Get(\"agc.status\", agcStatus);\n> > > > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain\n> > != 0.0) {\n> > > > + if (agcStatus.shutter_time && agcStatus.analogue_gain) {\n> > > > ControlList ctrls(sensorCtrls_);\n> > > > applyAGC(&agcStatus, ctrls);\n> > > > startConfig->controls = std::move(ctrls);\n> > > > @@ -392,7 +392,7 @@ int IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > > > /* Supply initial values for gain and exposure. */\n> > > > ControlList ctrls(sensorCtrls_);\n> > > > AgcStatus agcStatus;\n> > > > - agcStatus.shutter_time =\n> > DefaultExposureTime.get();\n> > > > + agcStatus.shutter_time = DefaultExposureTime;\n> > > > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > > applyAGC(&agcStatus, ctrls);\n> > > >\n> > > > @@ -464,7 +464,8 @@ void IPARPi::reportMetadata()\n> > > > */\n> > > > DeviceStatus *deviceStatus =\n> > rpiMetadata_.GetLocked(\"device.status\");\n> > > > if (deviceStatus) {\n> > > > - libcameraMetadata_.set(controls::ExposureTime,\n> > deviceStatus->shutter_speed);\n> > > > + libcameraMetadata_.set(controls::ExposureTime,\n> > > > +\n> > deviceStatus->shutter_speed.get());\n> > > > libcameraMetadata_.set(controls::AnalogueGain,\n> > deviceStatus->analogue_gain);\n> > > > }\n> > > >\n> > > > @@ -1017,7 +1018,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n> > &sensorControls)\n> > > > int32_t exposureLines =\n> > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > int32_t gainCode =\n> > sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > > >\n> > > > - deviceStatus.shutter_speed =\n> > helper_->Exposure(exposureLines).get();\n> > > > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > > > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > > >\n> > > > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > > > @@ -1103,7 +1104,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n> > *agcStatus, ControlList &ctrls)\n> > > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > > >\n> > > > /* GetVBlanking might clip exposure time to the fps limits. */\n> > > > - Duration exposure = agcStatus->shutter_time * 1.0us;\n> > > > + Duration exposure = agcStatus->shutter_time;\n> > > > int32_t vblanking = helper_->GetVBlanking(exposure,\n> > minFrameDuration_, maxFrameDuration_);\n> > > > int32_t exposureLines = helper_->ExposureLines(exposure);\n> > > >\n> > > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > > > index 3131aa2d6666..5fa6f8366f3d 100644\n> > > > --- a/src/libcamera/utils.cpp\n> > > > +++ b/src/libcamera/utils.cpp\n> > > > @@ -506,9 +506,14 @@ std::string libcameraSourcePath()\n> > > > * loop, iterates over an indexed view of the \\a iterable\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\typedef BaseDuration\n> > > > + * \\brief Base struct for the \\a Duration helper class.\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\class Duration\n> > > > - * \\brief Helper for std::chrono::duration types. Derived from \\a\n> > BaseDuration\n> > > > + * \\brief Helper class for std::chrono::duration types.\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -517,7 +522,7 @@ std::string libcameraSourcePath()\n> > > > * \\param[in] d The std::chrono::duration object to convert from.\n> > > > *\n> > > > * Constructs a \\a Duration object from a \\a std::chrono::duration\n> > object,\n> > > > - * converting the representation to \\a BaseDuration type.\n> > > > + * internally converting the representation to \\a BaseDuration type.\n> > > > */\n> > > >\n> > > > /**\n> > > > --\n> > > > 2.25.1\n> > > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3EC5FC3200\n\tfor ;\n\tFri, 21 May 2021 13:09:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AF076891F;\n\tFri, 21 May 2021 15:09:10 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D0B568911\n\tfor ;\n\tFri, 21 May 2021 15:09:09 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 558E9E0008;\n\tFri, 21 May 2021 13:09:08 +0000 (UTC)"],"Date":"Fri, 21 May 2021 15:09:54 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210521130954.tcu3rjltzl4tv2bw@uno.localdomain>","References":"<20210521080530.37591-1-naush@raspberrypi.com>\n\t<20210521080530.37591-5-naush@raspberrypi.com>\n\t\n\t<20210521123800.7yktaq6ek4cgfw4i@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] ipa: raspberrypi: Switch the\n\tAGC/Lux code to use utils::Duration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]