| Message ID | 20210518100706.578526-5-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for all this work! On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > Convert the core AGC and Lux controller code to use > RPiController::Duration for all exposure time related variables and > calculations. I wonder if I might have tried to keep the AGC and Lux patches separate? Having said that, they both *have* to change as soon as you touch the DeviceStatus, so maybe that would be difficult. Hmm, I think I'll leave that up to you! > > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus > to use RPiController::Duration. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/cam_helper.cpp | 2 +- > src/ipa/raspberrypi/controller/agc_status.h | 12 +-- > .../raspberrypi/controller/device_status.h | 6 +- > src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 ++++++++++--------- > src/ipa/raspberrypi/controller/rpi/agc.hpp | 26 +++--- > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++-- > src/ipa/raspberrypi/controller/rpi/lux.hpp | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 13 +-- > 8 files changed, 91 insertions(+), 76 deletions(-) > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp > index e2b6c8eb8e03..c399987e47bf 100644 > --- a/src/ipa/raspberrypi/cam_helper.cpp > +++ b/src/ipa/raspberrypi/cam_helper.cpp > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer, > return; > } > > - deviceStatus.shutter_speed = DurationValue<std::micro>(Exposure(exposureLines)); > + deviceStatus.shutter_speed = Exposure(exposureLines); Much nicer again! > deviceStatus.analogue_gain = Gain(gainCode); > > LOG(IPARPI, Debug) << "Metadata updated - Exposure : " > diff --git a/src/ipa/raspberrypi/controller/agc_status.h b/src/ipa/raspberrypi/controller/agc_status.h > index 10381c90a313..b2a64ce562fa 100644 > --- a/src/ipa/raspberrypi/controller/agc_status.h > +++ b/src/ipa/raspberrypi/controller/agc_status.h > @@ -6,6 +6,8 @@ > */ > #pragma once > > +#include "duration.hpp" > + > // The AGC algorithm should post the following structure into the image's > // "agc.status" metadata. > > @@ -18,17 +20,17 @@ extern "C" { > // ignored until then. > > struct AgcStatus { > - double total_exposure_value; // value for all exposure and gain for this image > - double target_exposure_value; // (unfiltered) target total exposure AGC is aiming for > - double shutter_time; > + RPiController::Duration total_exposure_value; // value for all exposure and gain for this image > + RPiController::Duration target_exposure_value; // (unfiltered) target total exposure AGC is aiming for > + RPiController::Duration shutter_time; > double analogue_gain; > char exposure_mode[32]; > char constraint_mode[32]; > char metering_mode[32]; > double ev; > - double flicker_period; > + RPiController::Duration flicker_period; > int floating_region_enable; > - double fixed_shutter; > + RPiController::Duration fixed_shutter; > double fixed_analogue_gain; > double digital_gain; > int locked; > diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h > index aa08608b5d40..a8496176eb92 100644 > --- a/src/ipa/raspberrypi/controller/device_status.h > +++ b/src/ipa/raspberrypi/controller/device_status.h > @@ -6,6 +6,8 @@ > */ > #pragma once > > +#include "duration.hpp" > + > // Definition of "device metadata" which stores things like shutter time and > // analogue gain that downstream control algorithms will want to know. > > @@ -14,8 +16,8 @@ extern "C" { > #endif > > struct DeviceStatus { > - // time shutter is open, in microseconds > - double shutter_speed; > + // time shutter is open > + RPiController::Duration shutter_speed; > double analogue_gain; > // 1.0/distance-in-metres, or 0 if unknown > double lens_position; > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > index 1cb4472b2691..3af2ef3cf6ed 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > @@ -21,6 +21,7 @@ > > using namespace RPiController; > using namespace libcamera; > +using namespace std::literals::chrono_literals; > > LOG_DEFINE_CATEGORY(RPiAgc) > > @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string, AgcMeteringMode> &metering_modes, > return first; > } > > -static int read_double_list(std::vector<double> &list, > - boost::property_tree::ptree const ¶ms) > +static int read_list(std::vector<double> &list, > + boost::property_tree::ptree const ¶ms) > { > for (auto &p : params) > list.push_back(p.second.get_value<double>()); > return list.size(); > } > > +static int read_list(std::vector<Duration> &list, > + boost::property_tree::ptree const ¶ms) > +{ > + for (auto &p : params) > + list.push_back(p.second.get_value<double>() * 1us); > + return list.size(); > +} > + I wonder if there's a template-y way to do these in one go? (sorry!) > void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms) > { > int num_shutters = > - read_double_list(shutter, params.get_child("shutter")); > - int num_ags = read_double_list(gain, params.get_child("gain")); > + read_list(shutter, params.get_child("shutter")); > + int num_ags = read_list(gain, params.get_child("gain")); > if (num_shutters < 2 || num_ags < 2) > throw std::runtime_error( > "AgcConfig: must have at least two entries in exposure profile"); > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const ¶ms) > params.get<double>("fast_reduce_threshold", 0.4); > base_ev = params.get<double>("base_ev", 1.0); > // Start with quite a low value as ramping up is easier than ramping down. > - default_exposure_time = params.get<double>("default_exposure_time", 1000); > + default_exposure_time = params.get<double>("default_exposure_time", 1000) * 1us; Mostly I notice that you write, for example, "1.0us" rather than "1us" as you have here. I take it that it makes no difference, right? > default_analogue_gain = params.get<double>("default_analogue_gain", 1.0); > } > > @@ -157,7 +166,7 @@ Agc::Agc(Controller *controller) > frame_count_(0), lock_count_(0), > last_target_exposure_(0.0), > ev_(1.0), flicker_period_(0.0), > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0) > + max_shutter_(0.0s), fixed_shutter_(0.0s), fixed_analogue_gain_(0.0) > { > memset(&awb_, 0, sizeof(awb_)); > // Setting status_.total_exposure_value_ to zero initially tells us > @@ -203,7 +212,7 @@ void Agc::Pause() > > void Agc::Resume() > { > - fixed_shutter_ = 0; > + fixed_shutter_ = 0.0s; > fixed_analogue_gain_ = 0; > } > > @@ -211,7 +220,7 @@ unsigned int Agc::GetConvergenceFrames() const > { > // If shutter and gain have been explicitly set, there is no > // convergence to happen, so no need to drop any frames - return zero. > - if (fixed_shutter_ && fixed_analogue_gain_) > + if (fixed_shutter_ > 0.0s && fixed_analogue_gain_) No implicit bool conversion for Durations, I take it. I notice that throughout this file we sometimes use "> 0.0s", sometimes "!= 0.0s". I assume that they're effectively interchangeable for us (and that the units don't matter when we compare for zero!), so should we be consistent? Apart from that nothing much to add, so: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > return 0; > else > return config_.convergence_frames; > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev) > > void Agc::SetFlickerPeriod(Duration flicker_period) > { > - flicker_period_ = DurationValue<std::micro>(flicker_period); > + flicker_period_ = flicker_period; > } > > void Agc::SetMaxShutter(Duration max_shutter) > { > - max_shutter_ = DurationValue<std::micro>(max_shutter); > + max_shutter_ = max_shutter; > } > > void Agc::SetFixedShutter(Duration fixed_shutter) > { > - fixed_shutter_ = DurationValue<std::micro>(fixed_shutter); > + fixed_shutter_ = fixed_shutter; > // Set this in case someone calls Pause() straight after. > status_.shutter_time = clipShutter(fixed_shutter_); > } > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, > { > housekeepConfig(); > > - double fixed_shutter = clipShutter(fixed_shutter_); > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) { > + Duration fixed_shutter = clipShutter(fixed_shutter_); > + if (fixed_shutter != 0.0s && fixed_analogue_gain_ != 0.0) { > // We're going to reset the algorithm here with these fixed values. > > fetchAwbStatus(metadata); > @@ -284,7 +293,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, > // Equivalent of divideUpExposure. > filtered_.shutter = fixed_shutter; > filtered_.analogue_gain = fixed_analogue_gain_; > - } else if (status_.total_exposure_value) { > + } else if (status_.total_exposure_value > 0.0s) { > // On a mode switch, it's possible the exposure profile could change, > // or a fixed exposure/gain might be set so we divide up the exposure/ > // gain again, but we don't change any target values. > @@ -296,7 +305,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, > // for any that weren't set. > > // Equivalent of divideUpExposure. > - filtered_.shutter = fixed_shutter ? fixed_shutter : config_.default_exposure_time; > + filtered_.shutter = fixed_shutter > 0.0s ? fixed_shutter : config_.default_exposure_time; > filtered_.analogue_gain = fixed_analogue_gain_ ? fixed_analogue_gain_ : config_.default_analogue_gain; > } > > @@ -308,13 +317,12 @@ void Agc::Prepare(Metadata *image_metadata) > status_.digital_gain = 1.0; > fetchAwbStatus(image_metadata); // always fetch it so that Process knows it's been done > > - if (status_.total_exposure_value) { > + if (status_.total_exposure_value > 0.0s) { > // Process has run, so we have meaningful values. > DeviceStatus device_status; > if (image_metadata->Get("device.status", device_status) == 0) { > - double actual_exposure = device_status.shutter_speed * > - device_status.analogue_gain; > - if (actual_exposure) { > + Duration actual_exposure = device_status.shutter_speed * device_status.analogue_gain; > + if (actual_exposure > 0.0s) { > status_.digital_gain = > status_.total_exposure_value / > actual_exposure; > @@ -370,9 +378,9 @@ void Agc::updateLockStatus(DeviceStatus const &device_status) > const double RESET_MARGIN = 1.5; > > // Add 200us to the exposure time error to allow for line quantisation. > - double exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200; > + Duration exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200us; > double gain_error = last_device_status_.analogue_gain * ERROR_FACTOR; > - double target_error = last_target_exposure_ * ERROR_FACTOR; > + Duration target_error = last_target_exposure_ * ERROR_FACTOR; > > // Note that we don't know the exposure/gain limits of the sensor, so > // the values we keep requesting may be unachievable. For this reason > @@ -462,7 +470,7 @@ void Agc::fetchCurrentExposure(Metadata *image_metadata) > current_.analogue_gain = device_status->analogue_gain; > AgcStatus *agc_status = > image_metadata->GetLocked<AgcStatus>("agc.status"); > - current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0; > + current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0s; > current_.total_exposure_no_dg = current_.shutter * current_.analogue_gain; > } > > @@ -573,7 +581,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *image_metadata, > > void Agc::computeTargetExposure(double gain) > { > - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain != 0.0) { > + if (status_.fixed_shutter != 0.0s && status_.fixed_analogue_gain != 0.0) { > // When ag and shutter are both fixed, we need to drive the > // total exposure so that we end up with a digital gain of at least > // 1/min_colour_gain. Otherwise we'd desaturate channels causing > @@ -588,11 +596,11 @@ void Agc::computeTargetExposure(double gain) > target_.total_exposure = current_.total_exposure_no_dg * gain; > // The final target exposure is also limited to what the exposure > // mode allows. > - double max_shutter = status_.fixed_shutter != 0.0 > + Duration max_shutter = status_.fixed_shutter != 0.0s > ? status_.fixed_shutter > : exposure_mode_->shutter.back(); > max_shutter = clipShutter(max_shutter); > - double max_total_exposure = > + Duration max_total_exposure = > max_shutter * > (status_.fixed_analogue_gain != 0.0 > ? status_.fixed_analogue_gain > @@ -634,10 +642,10 @@ void Agc::filterExposure(bool desaturate) > double speed = config_.speed; > // AGC adapts instantly if both shutter and gain are directly specified > // or we're in the startup phase. > - if ((status_.fixed_shutter && status_.fixed_analogue_gain) || > + if ((status_.fixed_shutter > 0.0s && status_.fixed_analogue_gain) || > frame_count_ <= config_.startup_frames) > speed = 1.0; > - if (filtered_.total_exposure == 0.0) { > + if (filtered_.total_exposure == 0.0s) { > filtered_.total_exposure = target_.total_exposure; > filtered_.total_exposure_no_dg = target_.total_exposure_no_dg; > } else { > @@ -674,9 +682,10 @@ void Agc::divideUpExposure() > // Sending the fixed shutter/gain cases through the same code may seem > // unnecessary, but it will make more sense when extend this to cover > // variable aperture. > - double exposure_value = filtered_.total_exposure_no_dg; > - double shutter_time, analogue_gain; > - shutter_time = status_.fixed_shutter != 0.0 > + Duration exposure_value = filtered_.total_exposure_no_dg; > + Duration shutter_time; > + double analogue_gain; > + shutter_time = status_.fixed_shutter != 0.0s > ? status_.fixed_shutter > : exposure_mode_->shutter[0]; > shutter_time = clipShutter(shutter_time); > @@ -686,8 +695,8 @@ void Agc::divideUpExposure() > if (shutter_time * analogue_gain < exposure_value) { > for (unsigned int stage = 1; > stage < exposure_mode_->gain.size(); stage++) { > - if (status_.fixed_shutter == 0.0) { > - double stage_shutter = > + if (status_.fixed_shutter == 0.0s) { > + Duration stage_shutter = > clipShutter(exposure_mode_->shutter[stage]); > if (stage_shutter * analogue_gain >= > exposure_value) { > @@ -713,12 +722,12 @@ void Agc::divideUpExposure() > << analogue_gain; > // Finally adjust shutter time for flicker avoidance (require both > // shutter and gain not to be fixed). > - if (status_.fixed_shutter == 0.0 && > + if (status_.fixed_shutter == 0.0s && > status_.fixed_analogue_gain == 0.0 && > - status_.flicker_period != 0.0) { > - int flicker_periods = shutter_time / status_.flicker_period; > - if (flicker_periods > 0) { > - double new_shutter_time = flicker_periods * status_.flicker_period; > + status_.flicker_period != 0.0s) { > + double flicker_periods = shutter_time / status_.flicker_period; > + if (flicker_periods > 0.0) { > + Duration new_shutter_time = flicker_periods * status_.flicker_period; > analogue_gain *= shutter_time / new_shutter_time; > // We should still not allow the ag to go over the > // largest value in the exposure mode. Note that this > @@ -738,7 +747,7 @@ void Agc::divideUpExposure() > void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate) > { > status_.total_exposure_value = filtered_.total_exposure; > - status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg; > + status_.target_exposure_value = desaturate ? 0s : target_.total_exposure_no_dg; > status_.shutter_time = filtered_.shutter; > status_.analogue_gain = filtered_.analogue_gain; > // Write to metadata as well, in case anyone wants to update the camera > @@ -750,9 +759,9 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate) > << " analogue gain " << filtered_.analogue_gain; > } > > -double Agc::clipShutter(double shutter) > +Duration Agc::clipShutter(Duration shutter) > { > - if (max_shutter_) > + if (max_shutter_ > 0.0s) > shutter = std::min(shutter, max_shutter_); > return shutter; > } > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp > index cb79bf61ba42..68b97ce91c99 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp > @@ -22,13 +22,15 @@ > > namespace RPiController { > > +using namespace std::literals::chrono_literals; > + > struct AgcMeteringMode { > double weights[AGC_STATS_SIZE]; > void Read(boost::property_tree::ptree const ¶ms); > }; > > struct AgcExposureMode { > - std::vector<double> shutter; > + std::vector<Duration> shutter; > std::vector<double> gain; > void Read(boost::property_tree::ptree const ¶ms); > }; > @@ -61,7 +63,7 @@ struct AgcConfig { > std::string default_exposure_mode; > std::string default_constraint_mode; > double base_ev; > - double default_exposure_time; > + Duration default_exposure_time; > double default_analogue_gain; > }; > > @@ -101,19 +103,19 @@ private: > void filterExposure(bool desaturate); > void divideUpExposure(); > void writeAndFinish(Metadata *image_metadata, bool desaturate); > - double clipShutter(double shutter); > + Duration clipShutter(Duration shutter); > AgcMeteringMode *metering_mode_; > AgcExposureMode *exposure_mode_; > AgcConstraintMode *constraint_mode_; > uint64_t frame_count_; > AwbStatus awb_; > struct ExposureValues { > - ExposureValues() : shutter(0), analogue_gain(0), > - total_exposure(0), total_exposure_no_dg(0) {} > - double shutter; > + ExposureValues() : shutter(0.0s), analogue_gain(0), > + total_exposure(0.0s), total_exposure_no_dg(0.0s) {} > + Duration shutter; > double analogue_gain; > - double total_exposure; > - double total_exposure_no_dg; // without digital gain > + Duration total_exposure; > + Duration total_exposure_no_dg; // without digital gain > }; > ExposureValues current_; // values for the current frame > ExposureValues target_; // calculate the values we want here > @@ -121,15 +123,15 @@ private: > AgcStatus status_; > int lock_count_; > DeviceStatus last_device_status_; > - double last_target_exposure_; > + Duration last_target_exposure_; > // Below here the "settings" that applications can change. > std::string metering_mode_name_; > std::string exposure_mode_name_; > std::string constraint_mode_name_; > double ev_; > - double flicker_period_; > - double max_shutter_; > - double fixed_shutter_; > + Duration flicker_period_; > + Duration max_shutter_; > + Duration fixed_shutter_; > double fixed_analogue_gain_; > }; > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp > index f74381cab2b4..46d3f3fab2c6 100644 > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp > @@ -16,6 +16,7 @@ > > using namespace RPiController; > using namespace libcamera; > +using namespace std::literals::chrono_literals; > > LOG_DEFINE_CATEGORY(RPiLux) > > @@ -38,7 +39,7 @@ char const *Lux::Name() const > void Lux::Read(boost::property_tree::ptree const ¶ms) > { > reference_shutter_speed_ = > - params.get<double>("reference_shutter_speed"); > + params.get<double>("reference_shutter_speed") * 1us; > reference_gain_ = params.get<double>("reference_gain"); > reference_aperture_ = params.get<double>("reference_aperture", 1.0); > reference_Y_ = params.get<double>("reference_Y"); > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata) > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) > { > // set some initial values to shut the compiler up > - DeviceStatus device_status = > - { .shutter_speed = 1.0, > - .analogue_gain = 1.0, > - .lens_position = 0.0, > - .aperture = 0.0, > - .flash_intensity = 0.0 }; > + DeviceStatus device_status = { .shutter_speed = 1.0ms, > + .analogue_gain = 1.0, > + .lens_position = 0.0, > + .aperture = 0.0, > + .flash_intensity = 0.0 }; > if (image_metadata->Get("device.status", device_status) == 0) { > double current_gain = device_status.analogue_gain; > - double current_shutter_speed = device_status.shutter_speed; > double current_aperture = device_status.aperture; > if (current_aperture == 0) > current_aperture = current_aperture_; > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) > double current_Y = sum / (double)num + .5; > double gain_ratio = reference_gain_ / current_gain; > double shutter_speed_ratio = > - reference_shutter_speed_ / current_shutter_speed; > + reference_shutter_speed_ / device_status.shutter_speed; > double aperture_ratio = reference_aperture_ / current_aperture; > double Y_ratio = current_Y * (65536 / num_bins) / reference_Y_; > double estimated_lux = shutter_speed_ratio * gain_ratio * > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp b/src/ipa/raspberrypi/controller/rpi/lux.hpp > index f9090484a136..726a7f7ca627 100644 > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp > @@ -28,7 +28,7 @@ public: > private: > // These values define the conditions of the reference image, against > // which we compare the new image. > - double reference_shutter_speed_; // in micro-seconds > + Duration reference_shutter_speed_; > double reference_gain_; > double reference_aperture_; // units of 1/f > double reference_Y_; // out of 65536 > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index f080f2e53bac..15f51162afec 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -227,11 +227,11 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > > /* SwitchMode may supply updated exposure/gain values to use. */ > AgcStatus agcStatus; > - agcStatus.shutter_time = 0.0; > + agcStatus.shutter_time = 0.0s; > agcStatus.analogue_gain = 0.0; > > metadata.Get("agc.status", agcStatus); > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) { > + if (agcStatus.shutter_time != 0.0s && agcStatus.analogue_gain != 0.0) { > ControlList ctrls(sensorCtrls_); > applyAGC(&agcStatus, ctrls); > startConfig->controls = std::move(ctrls); > @@ -394,7 +394,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo, > /* Supply initial values for gain and exposure. */ > ControlList ctrls(sensorCtrls_); > AgcStatus agcStatus; > - agcStatus.shutter_time = DurationValue<std::micro>(DefaultExposureTime); > + agcStatus.shutter_time = DefaultExposureTime; > agcStatus.analogue_gain = DefaultAnalogueGain; > applyAGC(&agcStatus, ctrls); > > @@ -466,7 +466,8 @@ void IPARPi::reportMetadata() > */ > DeviceStatus *deviceStatus = rpiMetadata_.GetLocked<DeviceStatus>("device.status"); > if (deviceStatus) { > - libcameraMetadata_.set(controls::ExposureTime, deviceStatus->shutter_speed); > + libcameraMetadata_.set(controls::ExposureTime, > + DurationValue<std::micro>(deviceStatus->shutter_speed)); > libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain); > } > > @@ -1019,7 +1020,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls) > int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > > - deviceStatus.shutter_speed = DurationValue<std::micro>(helper_->Exposure(exposureLines)); > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines); > deviceStatus.analogue_gain = helper_->Gain(gainCode); > > LOG(IPARPI, Debug) << "Metadata - Exposure : " > @@ -1105,7 +1106,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); > > /* GetVBlanking might clip exposure time to the fps limits. */ > - Duration exposure = agcStatus->shutter_time * 1.0us; > + Duration exposure = agcStatus->shutter_time; > int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_); > int32_t exposureLines = helper_->ExposureLines(exposure); > > -- > 2.25.1 >
Hi David, Thank you for your feedback. On Wed, 19 May 2021 at 16:28, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for all this work! > > On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > Convert the core AGC and Lux controller code to use > > RPiController::Duration for all exposure time related variables and > > calculations. > > I wonder if I might have tried to keep the AGC and Lux patches > separate? Having said that, they both *have* to change as soon as you > touch the DeviceStatus, so maybe that would be difficult. Hmm, I think > I'll leave that up to you! > > > > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus > > to use RPiController::Duration. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/cam_helper.cpp | 2 +- > > src/ipa/raspberrypi/controller/agc_status.h | 12 +-- > > .../raspberrypi/controller/device_status.h | 6 +- > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 ++++++++++--------- > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 26 +++--- > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++-- > > src/ipa/raspberrypi/controller/rpi/lux.hpp | 2 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 13 +-- > > 8 files changed, 91 insertions(+), 76 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp > b/src/ipa/raspberrypi/cam_helper.cpp > > index e2b6c8eb8e03..c399987e47bf 100644 > > --- a/src/ipa/raspberrypi/cam_helper.cpp > > +++ b/src/ipa/raspberrypi/cam_helper.cpp > > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const > uint8_t> buffer, > > return; > > } > > > > - deviceStatus.shutter_speed = > DurationValue<std::micro>(Exposure(exposureLines)); > > + deviceStatus.shutter_speed = Exposure(exposureLines); > > Much nicer again! > > > deviceStatus.analogue_gain = Gain(gainCode); > > > > LOG(IPARPI, Debug) << "Metadata updated - Exposure : " > > diff --git a/src/ipa/raspberrypi/controller/agc_status.h > b/src/ipa/raspberrypi/controller/agc_status.h > > index 10381c90a313..b2a64ce562fa 100644 > > --- a/src/ipa/raspberrypi/controller/agc_status.h > > +++ b/src/ipa/raspberrypi/controller/agc_status.h > > @@ -6,6 +6,8 @@ > > */ > > #pragma once > > > > +#include "duration.hpp" > > + > > // The AGC algorithm should post the following structure into the > image's > > // "agc.status" metadata. > > > > @@ -18,17 +20,17 @@ extern "C" { > > // ignored until then. > > > > struct AgcStatus { > > - double total_exposure_value; // value for all exposure and gain > for this image > > - double target_exposure_value; // (unfiltered) target total > exposure AGC is aiming for > > - double shutter_time; > > + RPiController::Duration total_exposure_value; // value for all > exposure and gain for this image > > + RPiController::Duration target_exposure_value; // (unfiltered) > target total exposure AGC is aiming for > > + RPiController::Duration shutter_time; > > double analogue_gain; > > char exposure_mode[32]; > > char constraint_mode[32]; > > char metering_mode[32]; > > double ev; > > - double flicker_period; > > + RPiController::Duration flicker_period; > > int floating_region_enable; > > - double fixed_shutter; > > + RPiController::Duration fixed_shutter; > > double fixed_analogue_gain; > > double digital_gain; > > int locked; > > diff --git a/src/ipa/raspberrypi/controller/device_status.h > b/src/ipa/raspberrypi/controller/device_status.h > > index aa08608b5d40..a8496176eb92 100644 > > --- a/src/ipa/raspberrypi/controller/device_status.h > > +++ b/src/ipa/raspberrypi/controller/device_status.h > > @@ -6,6 +6,8 @@ > > */ > > #pragma once > > > > +#include "duration.hpp" > > + > > // Definition of "device metadata" which stores things like shutter > time and > > // analogue gain that downstream control algorithms will want to know. > > > > @@ -14,8 +16,8 @@ extern "C" { > > #endif > > > > struct DeviceStatus { > > - // time shutter is open, in microseconds > > - double shutter_speed; > > + // time shutter is open > > + RPiController::Duration shutter_speed; > > double analogue_gain; > > // 1.0/distance-in-metres, or 0 if unknown > > double lens_position; > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp > b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > index 1cb4472b2691..3af2ef3cf6ed 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > @@ -21,6 +21,7 @@ > > > > using namespace RPiController; > > using namespace libcamera; > > +using namespace std::literals::chrono_literals; > > > > LOG_DEFINE_CATEGORY(RPiAgc) > > > > @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string, > AgcMeteringMode> &metering_modes, > > return first; > > } > > > > -static int read_double_list(std::vector<double> &list, > > - boost::property_tree::ptree const ¶ms) > > +static int read_list(std::vector<double> &list, > > + boost::property_tree::ptree const ¶ms) > > { > > for (auto &p : params) > > list.push_back(p.second.get_value<double>()); > > return list.size(); > > } > > > > +static int read_list(std::vector<Duration> &list, > > + boost::property_tree::ptree const ¶ms) > > +{ > > + for (auto &p : params) > > + list.push_back(p.second.get_value<double>() * 1us); > > + return list.size(); > > +} > > + > > I wonder if there's a template-y way to do these in one go? (sorry!) > I did consider that, but I do not know how to translate the "* 1us" with a simple template. Any ideas? > > > void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms) > > { > > int num_shutters = > > - read_double_list(shutter, params.get_child("shutter")); > > - int num_ags = read_double_list(gain, params.get_child("gain")); > > + read_list(shutter, params.get_child("shutter")); > > + int num_ags = read_list(gain, params.get_child("gain")); > > if (num_shutters < 2 || num_ags < 2) > > throw std::runtime_error( > > "AgcConfig: must have at least two entries in > exposure profile"); > > @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree > const ¶ms) > > params.get<double>("fast_reduce_threshold", 0.4); > > base_ev = params.get<double>("base_ev", 1.0); > > // Start with quite a low value as ramping up is easier than > ramping down. > > - default_exposure_time = > params.get<double>("default_exposure_time", 1000); > > + default_exposure_time = > params.get<double>("default_exposure_time", 1000) * 1us; > > Mostly I notice that you write, for example, "1.0us" rather than "1us" > as you have here. I take it that it makes no difference, right? > No difference. But I will try to be consistent :-) > > > default_analogue_gain = > params.get<double>("default_analogue_gain", 1.0); > > } > > > > @@ -157,7 +166,7 @@ Agc::Agc(Controller *controller) > > frame_count_(0), lock_count_(0), > > last_target_exposure_(0.0), > > ev_(1.0), flicker_period_(0.0), > > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0) > > + max_shutter_(0.0s), fixed_shutter_(0.0s), > fixed_analogue_gain_(0.0) > > { > > memset(&awb_, 0, sizeof(awb_)); > > // Setting status_.total_exposure_value_ to zero initially tells > us > > @@ -203,7 +212,7 @@ void Agc::Pause() > > > > void Agc::Resume() > > { > > - fixed_shutter_ = 0; > > + fixed_shutter_ = 0.0s; > > fixed_analogue_gain_ = 0; > > } > > > > @@ -211,7 +220,7 @@ unsigned int Agc::GetConvergenceFrames() const > > { > > // If shutter and gain have been explicitly set, there is no > > // convergence to happen, so no need to drop any frames - return > zero. > > - if (fixed_shutter_ && fixed_analogue_gain_) > > + if (fixed_shutter_ > 0.0s && fixed_analogue_gain_) > > No implicit bool conversion for Durations, I take it. I notice that > throughout this file we sometimes use "> 0.0s", sometimes "!= 0.0s". I > assume that they're effectively interchangeable for us (and that the > units don't matter when we compare for zero!), so should we be > consistent? > My next revision with the formal class definition for Duration will have an operator bool() so this (and other) statements can now be simplified and be consistent! Regards, Naush > > Apart from that nothing much to add, so: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David > > > return 0; > > else > > return config_.convergence_frames; > > @@ -224,17 +233,17 @@ void Agc::SetEv(double ev) > > > > void Agc::SetFlickerPeriod(Duration flicker_period) > > { > > - flicker_period_ = DurationValue<std::micro>(flicker_period); > > + flicker_period_ = flicker_period; > > } > > > > void Agc::SetMaxShutter(Duration max_shutter) > > { > > - max_shutter_ = DurationValue<std::micro>(max_shutter); > > + max_shutter_ = max_shutter; > > } > > > > void Agc::SetFixedShutter(Duration fixed_shutter) > > { > > - fixed_shutter_ = DurationValue<std::micro>(fixed_shutter); > > + fixed_shutter_ = fixed_shutter; > > // Set this in case someone calls Pause() straight after. > > status_.shutter_time = clipShutter(fixed_shutter_); > > } > > @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode > const &camera_mode, > > { > > housekeepConfig(); > > > > - double fixed_shutter = clipShutter(fixed_shutter_); > > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) { > > + Duration fixed_shutter = clipShutter(fixed_shutter_); > > + if (fixed_shutter != 0.0s && fixed_analogue_gain_ != 0.0) { > > // We're going to reset the algorithm here with these > fixed values. > > > > fetchAwbStatus(metadata); > > @@ -284,7 +293,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode > const &camera_mode, > > // Equivalent of divideUpExposure. > > filtered_.shutter = fixed_shutter; > > filtered_.analogue_gain = fixed_analogue_gain_; > > - } else if (status_.total_exposure_value) { > > + } else if (status_.total_exposure_value > 0.0s) { > > // On a mode switch, it's possible the exposure profile > could change, > > // or a fixed exposure/gain might be set so we divide up > the exposure/ > > // gain again, but we don't change any target values. > > @@ -296,7 +305,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode > const &camera_mode, > > // for any that weren't set. > > > > // Equivalent of divideUpExposure. > > - filtered_.shutter = fixed_shutter ? fixed_shutter : > config_.default_exposure_time; > > + filtered_.shutter = fixed_shutter > 0.0s ? fixed_shutter > : config_.default_exposure_time; > > filtered_.analogue_gain = fixed_analogue_gain_ ? > fixed_analogue_gain_ : config_.default_analogue_gain; > > } > > > > @@ -308,13 +317,12 @@ void Agc::Prepare(Metadata *image_metadata) > > status_.digital_gain = 1.0; > > fetchAwbStatus(image_metadata); // always fetch it so that > Process knows it's been done > > > > - if (status_.total_exposure_value) { > > + if (status_.total_exposure_value > 0.0s) { > > // Process has run, so we have meaningful values. > > DeviceStatus device_status; > > if (image_metadata->Get("device.status", device_status) > == 0) { > > - double actual_exposure = > device_status.shutter_speed * > > - > device_status.analogue_gain; > > - if (actual_exposure) { > > + Duration actual_exposure = > device_status.shutter_speed * device_status.analogue_gain; > > + if (actual_exposure > 0.0s) { > > status_.digital_gain = > > status_.total_exposure_value / > > actual_exposure; > > @@ -370,9 +378,9 @@ void Agc::updateLockStatus(DeviceStatus const > &device_status) > > const double RESET_MARGIN = 1.5; > > > > // Add 200us to the exposure time error to allow for line > quantisation. > > - double exposure_error = last_device_status_.shutter_speed * > ERROR_FACTOR + 200; > > + Duration exposure_error = last_device_status_.shutter_speed * > ERROR_FACTOR + 200us; > > double gain_error = last_device_status_.analogue_gain * > ERROR_FACTOR; > > - double target_error = last_target_exposure_ * ERROR_FACTOR; > > + Duration target_error = last_target_exposure_ * ERROR_FACTOR; > > > > // Note that we don't know the exposure/gain limits of the > sensor, so > > // the values we keep requesting may be unachievable. For this > reason > > @@ -462,7 +470,7 @@ void Agc::fetchCurrentExposure(Metadata > *image_metadata) > > current_.analogue_gain = device_status->analogue_gain; > > AgcStatus *agc_status = > > image_metadata->GetLocked<AgcStatus>("agc.status"); > > - current_.total_exposure = agc_status ? > agc_status->total_exposure_value : 0; > > + current_.total_exposure = agc_status ? > agc_status->total_exposure_value : 0s; > > current_.total_exposure_no_dg = current_.shutter * > current_.analogue_gain; > > } > > > > @@ -573,7 +581,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, > Metadata *image_metadata, > > > > void Agc::computeTargetExposure(double gain) > > { > > - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain > != 0.0) { > > + if (status_.fixed_shutter != 0.0s && status_.fixed_analogue_gain > != 0.0) { > > // When ag and shutter are both fixed, we need to drive > the > > // total exposure so that we end up with a digital gain > of at least > > // 1/min_colour_gain. Otherwise we'd desaturate channels > causing > > @@ -588,11 +596,11 @@ void Agc::computeTargetExposure(double gain) > > target_.total_exposure = current_.total_exposure_no_dg * > gain; > > // The final target exposure is also limited to what the > exposure > > // mode allows. > > - double max_shutter = status_.fixed_shutter != 0.0 > > + Duration max_shutter = status_.fixed_shutter != 0.0s > > ? status_.fixed_shutter > > : exposure_mode_->shutter.back(); > > max_shutter = clipShutter(max_shutter); > > - double max_total_exposure = > > + Duration max_total_exposure = > > max_shutter * > > (status_.fixed_analogue_gain != 0.0 > > ? status_.fixed_analogue_gain > > @@ -634,10 +642,10 @@ void Agc::filterExposure(bool desaturate) > > double speed = config_.speed; > > // AGC adapts instantly if both shutter and gain are directly > specified > > // or we're in the startup phase. > > - if ((status_.fixed_shutter && status_.fixed_analogue_gain) || > > + if ((status_.fixed_shutter > 0.0s && > status_.fixed_analogue_gain) || > > frame_count_ <= config_.startup_frames) > > speed = 1.0; > > - if (filtered_.total_exposure == 0.0) { > > + if (filtered_.total_exposure == 0.0s) { > > filtered_.total_exposure = target_.total_exposure; > > filtered_.total_exposure_no_dg = > target_.total_exposure_no_dg; > > } else { > > @@ -674,9 +682,10 @@ void Agc::divideUpExposure() > > // Sending the fixed shutter/gain cases through the same code > may seem > > // unnecessary, but it will make more sense when extend this to > cover > > // variable aperture. > > - double exposure_value = filtered_.total_exposure_no_dg; > > - double shutter_time, analogue_gain; > > - shutter_time = status_.fixed_shutter != 0.0 > > + Duration exposure_value = filtered_.total_exposure_no_dg; > > + Duration shutter_time; > > + double analogue_gain; > > + shutter_time = status_.fixed_shutter != 0.0s > > ? status_.fixed_shutter > > : exposure_mode_->shutter[0]; > > shutter_time = clipShutter(shutter_time); > > @@ -686,8 +695,8 @@ void Agc::divideUpExposure() > > if (shutter_time * analogue_gain < exposure_value) { > > for (unsigned int stage = 1; > > stage < exposure_mode_->gain.size(); stage++) { > > - if (status_.fixed_shutter == 0.0) { > > - double stage_shutter = > > + if (status_.fixed_shutter == 0.0s) { > > + Duration stage_shutter = > > > clipShutter(exposure_mode_->shutter[stage]); > > if (stage_shutter * analogue_gain >= > > exposure_value) { > > @@ -713,12 +722,12 @@ void Agc::divideUpExposure() > > << analogue_gain; > > // Finally adjust shutter time for flicker avoidance (require > both > > // shutter and gain not to be fixed). > > - if (status_.fixed_shutter == 0.0 && > > + if (status_.fixed_shutter == 0.0s && > > status_.fixed_analogue_gain == 0.0 && > > - status_.flicker_period != 0.0) { > > - int flicker_periods = shutter_time / > status_.flicker_period; > > - if (flicker_periods > 0) { > > - double new_shutter_time = flicker_periods * > status_.flicker_period; > > + status_.flicker_period != 0.0s) { > > + double flicker_periods = shutter_time / > status_.flicker_period; > > + if (flicker_periods > 0.0) { > > + Duration new_shutter_time = flicker_periods * > status_.flicker_period; > > analogue_gain *= shutter_time / new_shutter_time; > > // We should still not allow the ag to go over > the > > // largest value in the exposure mode. Note that > this > > @@ -738,7 +747,7 @@ void Agc::divideUpExposure() > > void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate) > > { > > status_.total_exposure_value = filtered_.total_exposure; > > - status_.target_exposure_value = desaturate ? 0 : > target_.total_exposure_no_dg; > > + status_.target_exposure_value = desaturate ? 0s : > target_.total_exposure_no_dg; > > status_.shutter_time = filtered_.shutter; > > status_.analogue_gain = filtered_.analogue_gain; > > // Write to metadata as well, in case anyone wants to update the > camera > > @@ -750,9 +759,9 @@ void Agc::writeAndFinish(Metadata *image_metadata, > bool desaturate) > > << " analogue gain " << > filtered_.analogue_gain; > > } > > > > -double Agc::clipShutter(double shutter) > > +Duration Agc::clipShutter(Duration shutter) > > { > > - if (max_shutter_) > > + if (max_shutter_ > 0.0s) > > shutter = std::min(shutter, max_shutter_); > > return shutter; > > } > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp > b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > index cb79bf61ba42..68b97ce91c99 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > @@ -22,13 +22,15 @@ > > > > namespace RPiController { > > > > +using namespace std::literals::chrono_literals; > > + > > struct AgcMeteringMode { > > double weights[AGC_STATS_SIZE]; > > void Read(boost::property_tree::ptree const ¶ms); > > }; > > > > struct AgcExposureMode { > > - std::vector<double> shutter; > > + std::vector<Duration> shutter; > > std::vector<double> gain; > > void Read(boost::property_tree::ptree const ¶ms); > > }; > > @@ -61,7 +63,7 @@ struct AgcConfig { > > std::string default_exposure_mode; > > std::string default_constraint_mode; > > double base_ev; > > - double default_exposure_time; > > + Duration default_exposure_time; > > double default_analogue_gain; > > }; > > > > @@ -101,19 +103,19 @@ private: > > void filterExposure(bool desaturate); > > void divideUpExposure(); > > void writeAndFinish(Metadata *image_metadata, bool desaturate); > > - double clipShutter(double shutter); > > + Duration clipShutter(Duration shutter); > > AgcMeteringMode *metering_mode_; > > AgcExposureMode *exposure_mode_; > > AgcConstraintMode *constraint_mode_; > > uint64_t frame_count_; > > AwbStatus awb_; > > struct ExposureValues { > > - ExposureValues() : shutter(0), analogue_gain(0), > > - total_exposure(0), > total_exposure_no_dg(0) {} > > - double shutter; > > + ExposureValues() : shutter(0.0s), analogue_gain(0), > > + total_exposure(0.0s), > total_exposure_no_dg(0.0s) {} > > + Duration shutter; > > double analogue_gain; > > - double total_exposure; > > - double total_exposure_no_dg; // without digital gain > > + Duration total_exposure; > > + Duration total_exposure_no_dg; // without digital gain > > }; > > ExposureValues current_; // values for the current frame > > ExposureValues target_; // calculate the values we want here > > @@ -121,15 +123,15 @@ private: > > AgcStatus status_; > > int lock_count_; > > DeviceStatus last_device_status_; > > - double last_target_exposure_; > > + Duration last_target_exposure_; > > // Below here the "settings" that applications can change. > > std::string metering_mode_name_; > > std::string exposure_mode_name_; > > std::string constraint_mode_name_; > > double ev_; > > - double flicker_period_; > > - double max_shutter_; > > - double fixed_shutter_; > > + Duration flicker_period_; > > + Duration max_shutter_; > > + Duration fixed_shutter_; > > double fixed_analogue_gain_; > > }; > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp > b/src/ipa/raspberrypi/controller/rpi/lux.cpp > > index f74381cab2b4..46d3f3fab2c6 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp > > @@ -16,6 +16,7 @@ > > > > using namespace RPiController; > > using namespace libcamera; > > +using namespace std::literals::chrono_literals; > > > > LOG_DEFINE_CATEGORY(RPiLux) > > > > @@ -38,7 +39,7 @@ char const *Lux::Name() const > > void Lux::Read(boost::property_tree::ptree const ¶ms) > > { > > reference_shutter_speed_ = > > - params.get<double>("reference_shutter_speed"); > > + params.get<double>("reference_shutter_speed") * 1us; > > reference_gain_ = params.get<double>("reference_gain"); > > reference_aperture_ = params.get<double>("reference_aperture", > 1.0); > > reference_Y_ = params.get<double>("reference_Y"); > > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata) > > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) > > { > > // set some initial values to shut the compiler up > > - DeviceStatus device_status = > > - { .shutter_speed = 1.0, > > - .analogue_gain = 1.0, > > - .lens_position = 0.0, > > - .aperture = 0.0, > > - .flash_intensity = 0.0 }; > > + DeviceStatus device_status = { .shutter_speed = 1.0ms, > > + .analogue_gain = 1.0, > > + .lens_position = 0.0, > > + .aperture = 0.0, > > + .flash_intensity = 0.0 }; > > if (image_metadata->Get("device.status", device_status) == 0) { > > double current_gain = device_status.analogue_gain; > > - double current_shutter_speed = > device_status.shutter_speed; > > double current_aperture = device_status.aperture; > > if (current_aperture == 0) > > current_aperture = current_aperture_; > > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata > *image_metadata) > > double current_Y = sum / (double)num + .5; > > double gain_ratio = reference_gain_ / current_gain; > > double shutter_speed_ratio = > > - reference_shutter_speed_ / current_shutter_speed; > > + reference_shutter_speed_ / > device_status.shutter_speed; > > double aperture_ratio = reference_aperture_ / > current_aperture; > > double Y_ratio = current_Y * (65536 / num_bins) / > reference_Y_; > > double estimated_lux = shutter_speed_ratio * gain_ratio * > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp > b/src/ipa/raspberrypi/controller/rpi/lux.hpp > > index f9090484a136..726a7f7ca627 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp > > @@ -28,7 +28,7 @@ public: > > private: > > // These values define the conditions of the reference image, > against > > // which we compare the new image. > > - double reference_shutter_speed_; // in micro-seconds > > + Duration reference_shutter_speed_; > > double reference_gain_; > > double reference_aperture_; // units of 1/f > > double reference_Y_; // out of 65536 > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index f080f2e53bac..15f51162afec 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -227,11 +227,11 @@ void IPARPi::start(const ControlList &controls, > ipa::RPi::StartConfig *startConf > > > > /* SwitchMode may supply updated exposure/gain values to use. */ > > AgcStatus agcStatus; > > - agcStatus.shutter_time = 0.0; > > + agcStatus.shutter_time = 0.0s; > > agcStatus.analogue_gain = 0.0; > > > > metadata.Get("agc.status", agcStatus); > > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != > 0.0) { > > + if (agcStatus.shutter_time != 0.0s && agcStatus.analogue_gain != > 0.0) { > > ControlList ctrls(sensorCtrls_); > > applyAGC(&agcStatus, ctrls); > > startConfig->controls = std::move(ctrls); > > @@ -394,7 +394,7 @@ int IPARPi::configure(const CameraSensorInfo > &sensorInfo, > > /* Supply initial values for gain and exposure. */ > > ControlList ctrls(sensorCtrls_); > > AgcStatus agcStatus; > > - agcStatus.shutter_time = > DurationValue<std::micro>(DefaultExposureTime); > > + agcStatus.shutter_time = DefaultExposureTime; > > agcStatus.analogue_gain = DefaultAnalogueGain; > > applyAGC(&agcStatus, ctrls); > > > > @@ -466,7 +466,8 @@ void IPARPi::reportMetadata() > > */ > > DeviceStatus *deviceStatus = > rpiMetadata_.GetLocked<DeviceStatus>("device.status"); > > if (deviceStatus) { > > - libcameraMetadata_.set(controls::ExposureTime, > deviceStatus->shutter_speed); > > + libcameraMetadata_.set(controls::ExposureTime, > > + > DurationValue<std::micro>(deviceStatus->shutter_speed)); > > libcameraMetadata_.set(controls::AnalogueGain, > deviceStatus->analogue_gain); > > } > > > > @@ -1019,7 +1020,7 @@ void IPARPi::fillDeviceStatus(const ControlList > &sensorControls) > > int32_t exposureLines = > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > int32_t gainCode = > sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > > > > - deviceStatus.shutter_speed = > DurationValue<std::micro>(helper_->Exposure(exposureLines)); > > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines); > > deviceStatus.analogue_gain = helper_->Gain(gainCode); > > > > LOG(IPARPI, Debug) << "Metadata - Exposure : " > > @@ -1105,7 +1106,7 @@ void IPARPi::applyAGC(const struct AgcStatus > *agcStatus, ControlList &ctrls) > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); > > > > /* GetVBlanking might clip exposure time to the fps limits. */ > > - Duration exposure = agcStatus->shutter_time * 1.0us; > > + Duration exposure = agcStatus->shutter_time; > > int32_t vblanking = helper_->GetVBlanking(exposure, > minFrameDuration_, maxFrameDuration_); > > int32_t exposureLines = helper_->ExposureLines(exposure); > > > > -- > > 2.25.1 > > >
diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp index e2b6c8eb8e03..c399987e47bf 100644 --- a/src/ipa/raspberrypi/cam_helper.cpp +++ b/src/ipa/raspberrypi/cam_helper.cpp @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer, return; } - deviceStatus.shutter_speed = DurationValue<std::micro>(Exposure(exposureLines)); + deviceStatus.shutter_speed = Exposure(exposureLines); deviceStatus.analogue_gain = Gain(gainCode); LOG(IPARPI, Debug) << "Metadata updated - Exposure : " diff --git a/src/ipa/raspberrypi/controller/agc_status.h b/src/ipa/raspberrypi/controller/agc_status.h index 10381c90a313..b2a64ce562fa 100644 --- a/src/ipa/raspberrypi/controller/agc_status.h +++ b/src/ipa/raspberrypi/controller/agc_status.h @@ -6,6 +6,8 @@ */ #pragma once +#include "duration.hpp" + // The AGC algorithm should post the following structure into the image's // "agc.status" metadata. @@ -18,17 +20,17 @@ extern "C" { // ignored until then. struct AgcStatus { - double total_exposure_value; // value for all exposure and gain for this image - double target_exposure_value; // (unfiltered) target total exposure AGC is aiming for - double shutter_time; + RPiController::Duration total_exposure_value; // value for all exposure and gain for this image + RPiController::Duration target_exposure_value; // (unfiltered) target total exposure AGC is aiming for + RPiController::Duration shutter_time; double analogue_gain; char exposure_mode[32]; char constraint_mode[32]; char metering_mode[32]; double ev; - double flicker_period; + RPiController::Duration flicker_period; int floating_region_enable; - double fixed_shutter; + RPiController::Duration fixed_shutter; double fixed_analogue_gain; double digital_gain; int locked; diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h index aa08608b5d40..a8496176eb92 100644 --- a/src/ipa/raspberrypi/controller/device_status.h +++ b/src/ipa/raspberrypi/controller/device_status.h @@ -6,6 +6,8 @@ */ #pragma once +#include "duration.hpp" + // Definition of "device metadata" which stores things like shutter time and // analogue gain that downstream control algorithms will want to know. @@ -14,8 +16,8 @@ extern "C" { #endif struct DeviceStatus { - // time shutter is open, in microseconds - double shutter_speed; + // time shutter is open + RPiController::Duration shutter_speed; double analogue_gain; // 1.0/distance-in-metres, or 0 if unknown double lens_position; diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp index 1cb4472b2691..3af2ef3cf6ed 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp @@ -21,6 +21,7 @@ using namespace RPiController; using namespace libcamera; +using namespace std::literals::chrono_literals; LOG_DEFINE_CATEGORY(RPiAgc) @@ -55,19 +56,27 @@ read_metering_modes(std::map<std::string, AgcMeteringMode> &metering_modes, return first; } -static int read_double_list(std::vector<double> &list, - boost::property_tree::ptree const ¶ms) +static int read_list(std::vector<double> &list, + boost::property_tree::ptree const ¶ms) { for (auto &p : params) list.push_back(p.second.get_value<double>()); return list.size(); } +static int read_list(std::vector<Duration> &list, + boost::property_tree::ptree const ¶ms) +{ + for (auto &p : params) + list.push_back(p.second.get_value<double>() * 1us); + return list.size(); +} + void AgcExposureMode::Read(boost::property_tree::ptree const ¶ms) { int num_shutters = - read_double_list(shutter, params.get_child("shutter")); - int num_ags = read_double_list(gain, params.get_child("gain")); + read_list(shutter, params.get_child("shutter")); + int num_ags = read_list(gain, params.get_child("gain")); if (num_shutters < 2 || num_ags < 2) throw std::runtime_error( "AgcConfig: must have at least two entries in exposure profile"); @@ -147,7 +156,7 @@ void AgcConfig::Read(boost::property_tree::ptree const ¶ms) params.get<double>("fast_reduce_threshold", 0.4); base_ev = params.get<double>("base_ev", 1.0); // Start with quite a low value as ramping up is easier than ramping down. - default_exposure_time = params.get<double>("default_exposure_time", 1000); + default_exposure_time = params.get<double>("default_exposure_time", 1000) * 1us; default_analogue_gain = params.get<double>("default_analogue_gain", 1.0); } @@ -157,7 +166,7 @@ Agc::Agc(Controller *controller) frame_count_(0), lock_count_(0), last_target_exposure_(0.0), ev_(1.0), flicker_period_(0.0), - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0) + max_shutter_(0.0s), fixed_shutter_(0.0s), fixed_analogue_gain_(0.0) { memset(&awb_, 0, sizeof(awb_)); // Setting status_.total_exposure_value_ to zero initially tells us @@ -203,7 +212,7 @@ void Agc::Pause() void Agc::Resume() { - fixed_shutter_ = 0; + fixed_shutter_ = 0.0s; fixed_analogue_gain_ = 0; } @@ -211,7 +220,7 @@ unsigned int Agc::GetConvergenceFrames() const { // If shutter and gain have been explicitly set, there is no // convergence to happen, so no need to drop any frames - return zero. - if (fixed_shutter_ && fixed_analogue_gain_) + if (fixed_shutter_ > 0.0s && fixed_analogue_gain_) return 0; else return config_.convergence_frames; @@ -224,17 +233,17 @@ void Agc::SetEv(double ev) void Agc::SetFlickerPeriod(Duration flicker_period) { - flicker_period_ = DurationValue<std::micro>(flicker_period); + flicker_period_ = flicker_period; } void Agc::SetMaxShutter(Duration max_shutter) { - max_shutter_ = DurationValue<std::micro>(max_shutter); + max_shutter_ = max_shutter; } void Agc::SetFixedShutter(Duration fixed_shutter) { - fixed_shutter_ = DurationValue<std::micro>(fixed_shutter); + fixed_shutter_ = fixed_shutter; // Set this in case someone calls Pause() straight after. status_.shutter_time = clipShutter(fixed_shutter_); } @@ -266,8 +275,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, { housekeepConfig(); - double fixed_shutter = clipShutter(fixed_shutter_); - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) { + Duration fixed_shutter = clipShutter(fixed_shutter_); + if (fixed_shutter != 0.0s && fixed_analogue_gain_ != 0.0) { // We're going to reset the algorithm here with these fixed values. fetchAwbStatus(metadata); @@ -284,7 +293,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, // Equivalent of divideUpExposure. filtered_.shutter = fixed_shutter; filtered_.analogue_gain = fixed_analogue_gain_; - } else if (status_.total_exposure_value) { + } else if (status_.total_exposure_value > 0.0s) { // On a mode switch, it's possible the exposure profile could change, // or a fixed exposure/gain might be set so we divide up the exposure/ // gain again, but we don't change any target values. @@ -296,7 +305,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, // for any that weren't set. // Equivalent of divideUpExposure. - filtered_.shutter = fixed_shutter ? fixed_shutter : config_.default_exposure_time; + filtered_.shutter = fixed_shutter > 0.0s ? fixed_shutter : config_.default_exposure_time; filtered_.analogue_gain = fixed_analogue_gain_ ? fixed_analogue_gain_ : config_.default_analogue_gain; } @@ -308,13 +317,12 @@ void Agc::Prepare(Metadata *image_metadata) status_.digital_gain = 1.0; fetchAwbStatus(image_metadata); // always fetch it so that Process knows it's been done - if (status_.total_exposure_value) { + if (status_.total_exposure_value > 0.0s) { // Process has run, so we have meaningful values. DeviceStatus device_status; if (image_metadata->Get("device.status", device_status) == 0) { - double actual_exposure = device_status.shutter_speed * - device_status.analogue_gain; - if (actual_exposure) { + Duration actual_exposure = device_status.shutter_speed * device_status.analogue_gain; + if (actual_exposure > 0.0s) { status_.digital_gain = status_.total_exposure_value / actual_exposure; @@ -370,9 +378,9 @@ void Agc::updateLockStatus(DeviceStatus const &device_status) const double RESET_MARGIN = 1.5; // Add 200us to the exposure time error to allow for line quantisation. - double exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200; + Duration exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200us; double gain_error = last_device_status_.analogue_gain * ERROR_FACTOR; - double target_error = last_target_exposure_ * ERROR_FACTOR; + Duration target_error = last_target_exposure_ * ERROR_FACTOR; // Note that we don't know the exposure/gain limits of the sensor, so // the values we keep requesting may be unachievable. For this reason @@ -462,7 +470,7 @@ void Agc::fetchCurrentExposure(Metadata *image_metadata) current_.analogue_gain = device_status->analogue_gain; AgcStatus *agc_status = image_metadata->GetLocked<AgcStatus>("agc.status"); - current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0; + current_.total_exposure = agc_status ? agc_status->total_exposure_value : 0s; current_.total_exposure_no_dg = current_.shutter * current_.analogue_gain; } @@ -573,7 +581,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *image_metadata, void Agc::computeTargetExposure(double gain) { - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain != 0.0) { + if (status_.fixed_shutter != 0.0s && status_.fixed_analogue_gain != 0.0) { // When ag and shutter are both fixed, we need to drive the // total exposure so that we end up with a digital gain of at least // 1/min_colour_gain. Otherwise we'd desaturate channels causing @@ -588,11 +596,11 @@ void Agc::computeTargetExposure(double gain) target_.total_exposure = current_.total_exposure_no_dg * gain; // The final target exposure is also limited to what the exposure // mode allows. - double max_shutter = status_.fixed_shutter != 0.0 + Duration max_shutter = status_.fixed_shutter != 0.0s ? status_.fixed_shutter : exposure_mode_->shutter.back(); max_shutter = clipShutter(max_shutter); - double max_total_exposure = + Duration max_total_exposure = max_shutter * (status_.fixed_analogue_gain != 0.0 ? status_.fixed_analogue_gain @@ -634,10 +642,10 @@ void Agc::filterExposure(bool desaturate) double speed = config_.speed; // AGC adapts instantly if both shutter and gain are directly specified // or we're in the startup phase. - if ((status_.fixed_shutter && status_.fixed_analogue_gain) || + if ((status_.fixed_shutter > 0.0s && status_.fixed_analogue_gain) || frame_count_ <= config_.startup_frames) speed = 1.0; - if (filtered_.total_exposure == 0.0) { + if (filtered_.total_exposure == 0.0s) { filtered_.total_exposure = target_.total_exposure; filtered_.total_exposure_no_dg = target_.total_exposure_no_dg; } else { @@ -674,9 +682,10 @@ void Agc::divideUpExposure() // Sending the fixed shutter/gain cases through the same code may seem // unnecessary, but it will make more sense when extend this to cover // variable aperture. - double exposure_value = filtered_.total_exposure_no_dg; - double shutter_time, analogue_gain; - shutter_time = status_.fixed_shutter != 0.0 + Duration exposure_value = filtered_.total_exposure_no_dg; + Duration shutter_time; + double analogue_gain; + shutter_time = status_.fixed_shutter != 0.0s ? status_.fixed_shutter : exposure_mode_->shutter[0]; shutter_time = clipShutter(shutter_time); @@ -686,8 +695,8 @@ void Agc::divideUpExposure() if (shutter_time * analogue_gain < exposure_value) { for (unsigned int stage = 1; stage < exposure_mode_->gain.size(); stage++) { - if (status_.fixed_shutter == 0.0) { - double stage_shutter = + if (status_.fixed_shutter == 0.0s) { + Duration stage_shutter = clipShutter(exposure_mode_->shutter[stage]); if (stage_shutter * analogue_gain >= exposure_value) { @@ -713,12 +722,12 @@ void Agc::divideUpExposure() << analogue_gain; // Finally adjust shutter time for flicker avoidance (require both // shutter and gain not to be fixed). - if (status_.fixed_shutter == 0.0 && + if (status_.fixed_shutter == 0.0s && status_.fixed_analogue_gain == 0.0 && - status_.flicker_period != 0.0) { - int flicker_periods = shutter_time / status_.flicker_period; - if (flicker_periods > 0) { - double new_shutter_time = flicker_periods * status_.flicker_period; + status_.flicker_period != 0.0s) { + double flicker_periods = shutter_time / status_.flicker_period; + if (flicker_periods > 0.0) { + Duration new_shutter_time = flicker_periods * status_.flicker_period; analogue_gain *= shutter_time / new_shutter_time; // We should still not allow the ag to go over the // largest value in the exposure mode. Note that this @@ -738,7 +747,7 @@ void Agc::divideUpExposure() void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate) { status_.total_exposure_value = filtered_.total_exposure; - status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg; + status_.target_exposure_value = desaturate ? 0s : target_.total_exposure_no_dg; status_.shutter_time = filtered_.shutter; status_.analogue_gain = filtered_.analogue_gain; // Write to metadata as well, in case anyone wants to update the camera @@ -750,9 +759,9 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate) << " analogue gain " << filtered_.analogue_gain; } -double Agc::clipShutter(double shutter) +Duration Agc::clipShutter(Duration shutter) { - if (max_shutter_) + if (max_shutter_ > 0.0s) shutter = std::min(shutter, max_shutter_); return shutter; } diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp index cb79bf61ba42..68b97ce91c99 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp @@ -22,13 +22,15 @@ namespace RPiController { +using namespace std::literals::chrono_literals; + struct AgcMeteringMode { double weights[AGC_STATS_SIZE]; void Read(boost::property_tree::ptree const ¶ms); }; struct AgcExposureMode { - std::vector<double> shutter; + std::vector<Duration> shutter; std::vector<double> gain; void Read(boost::property_tree::ptree const ¶ms); }; @@ -61,7 +63,7 @@ struct AgcConfig { std::string default_exposure_mode; std::string default_constraint_mode; double base_ev; - double default_exposure_time; + Duration default_exposure_time; double default_analogue_gain; }; @@ -101,19 +103,19 @@ private: void filterExposure(bool desaturate); void divideUpExposure(); void writeAndFinish(Metadata *image_metadata, bool desaturate); - double clipShutter(double shutter); + Duration clipShutter(Duration shutter); AgcMeteringMode *metering_mode_; AgcExposureMode *exposure_mode_; AgcConstraintMode *constraint_mode_; uint64_t frame_count_; AwbStatus awb_; struct ExposureValues { - ExposureValues() : shutter(0), analogue_gain(0), - total_exposure(0), total_exposure_no_dg(0) {} - double shutter; + ExposureValues() : shutter(0.0s), analogue_gain(0), + total_exposure(0.0s), total_exposure_no_dg(0.0s) {} + Duration shutter; double analogue_gain; - double total_exposure; - double total_exposure_no_dg; // without digital gain + Duration total_exposure; + Duration total_exposure_no_dg; // without digital gain }; ExposureValues current_; // values for the current frame ExposureValues target_; // calculate the values we want here @@ -121,15 +123,15 @@ private: AgcStatus status_; int lock_count_; DeviceStatus last_device_status_; - double last_target_exposure_; + Duration last_target_exposure_; // Below here the "settings" that applications can change. std::string metering_mode_name_; std::string exposure_mode_name_; std::string constraint_mode_name_; double ev_; - double flicker_period_; - double max_shutter_; - double fixed_shutter_; + Duration flicker_period_; + Duration max_shutter_; + Duration fixed_shutter_; double fixed_analogue_gain_; }; diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp index f74381cab2b4..46d3f3fab2c6 100644 --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp @@ -16,6 +16,7 @@ using namespace RPiController; using namespace libcamera; +using namespace std::literals::chrono_literals; LOG_DEFINE_CATEGORY(RPiLux) @@ -38,7 +39,7 @@ char const *Lux::Name() const void Lux::Read(boost::property_tree::ptree const ¶ms) { reference_shutter_speed_ = - params.get<double>("reference_shutter_speed"); + params.get<double>("reference_shutter_speed") * 1us; reference_gain_ = params.get<double>("reference_gain"); reference_aperture_ = params.get<double>("reference_aperture", 1.0); reference_Y_ = params.get<double>("reference_Y"); @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata) void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) { // set some initial values to shut the compiler up - DeviceStatus device_status = - { .shutter_speed = 1.0, - .analogue_gain = 1.0, - .lens_position = 0.0, - .aperture = 0.0, - .flash_intensity = 0.0 }; + DeviceStatus device_status = { .shutter_speed = 1.0ms, + .analogue_gain = 1.0, + .lens_position = 0.0, + .aperture = 0.0, + .flash_intensity = 0.0 }; if (image_metadata->Get("device.status", device_status) == 0) { double current_gain = device_status.analogue_gain; - double current_shutter_speed = device_status.shutter_speed; double current_aperture = device_status.aperture; if (current_aperture == 0) current_aperture = current_aperture_; @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) double current_Y = sum / (double)num + .5; double gain_ratio = reference_gain_ / current_gain; double shutter_speed_ratio = - reference_shutter_speed_ / current_shutter_speed; + reference_shutter_speed_ / device_status.shutter_speed; double aperture_ratio = reference_aperture_ / current_aperture; double Y_ratio = current_Y * (65536 / num_bins) / reference_Y_; double estimated_lux = shutter_speed_ratio * gain_ratio * diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp b/src/ipa/raspberrypi/controller/rpi/lux.hpp index f9090484a136..726a7f7ca627 100644 --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp @@ -28,7 +28,7 @@ public: private: // These values define the conditions of the reference image, against // which we compare the new image. - double reference_shutter_speed_; // in micro-seconds + Duration reference_shutter_speed_; double reference_gain_; double reference_aperture_; // units of 1/f double reference_Y_; // out of 65536 diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index f080f2e53bac..15f51162afec 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -227,11 +227,11 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf /* SwitchMode may supply updated exposure/gain values to use. */ AgcStatus agcStatus; - agcStatus.shutter_time = 0.0; + agcStatus.shutter_time = 0.0s; agcStatus.analogue_gain = 0.0; metadata.Get("agc.status", agcStatus); - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) { + if (agcStatus.shutter_time != 0.0s && agcStatus.analogue_gain != 0.0) { ControlList ctrls(sensorCtrls_); applyAGC(&agcStatus, ctrls); startConfig->controls = std::move(ctrls); @@ -394,7 +394,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo, /* Supply initial values for gain and exposure. */ ControlList ctrls(sensorCtrls_); AgcStatus agcStatus; - agcStatus.shutter_time = DurationValue<std::micro>(DefaultExposureTime); + agcStatus.shutter_time = DefaultExposureTime; agcStatus.analogue_gain = DefaultAnalogueGain; applyAGC(&agcStatus, ctrls); @@ -466,7 +466,8 @@ void IPARPi::reportMetadata() */ DeviceStatus *deviceStatus = rpiMetadata_.GetLocked<DeviceStatus>("device.status"); if (deviceStatus) { - libcameraMetadata_.set(controls::ExposureTime, deviceStatus->shutter_speed); + libcameraMetadata_.set(controls::ExposureTime, + DurationValue<std::micro>(deviceStatus->shutter_speed)); libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain); } @@ -1019,7 +1020,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls) int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); - deviceStatus.shutter_speed = DurationValue<std::micro>(helper_->Exposure(exposureLines)); + deviceStatus.shutter_speed = helper_->Exposure(exposureLines); deviceStatus.analogue_gain = helper_->Gain(gainCode); LOG(IPARPI, Debug) << "Metadata - Exposure : " @@ -1105,7 +1106,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); /* GetVBlanking might clip exposure time to the fps limits. */ - Duration exposure = agcStatus->shutter_time * 1.0us; + Duration exposure = agcStatus->shutter_time; int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_); int32_t exposureLines = helper_->ExposureLines(exposure);
Convert the core AGC and Lux controller code to use RPiController::Duration for all exposure time related variables and calculations. Convert the exposure/shutter time fields in AgcStatus and DeviceStatus to use RPiController::Duration. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/cam_helper.cpp | 2 +- src/ipa/raspberrypi/controller/agc_status.h | 12 +-- .../raspberrypi/controller/device_status.h | 6 +- src/ipa/raspberrypi/controller/rpi/agc.cpp | 89 ++++++++++--------- src/ipa/raspberrypi/controller/rpi/agc.hpp | 26 +++--- src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++-- src/ipa/raspberrypi/controller/rpi/lux.hpp | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 13 +-- 8 files changed, 91 insertions(+), 76 deletions(-)