[libcamera-devel] [PATCH v4 4/4] ipa: raspberrypi: Switch the AGC/Lux code to use utils::Duration
Naushir Patuck
naush at raspberrypi.com
Mon May 24 16:03:02 CEST 2021
Hi Jacopo,
Thanks for the review!
On Mon, 24 May 2021 at 14:39, Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Naush,
>
> On Mon, May 24, 2021 at 09:48:22AM +0100, Naushir Patuck wrote:
> > Convert the core AGC and Lux controller code to use
> > utils::Duration for all exposure time related variables and
> > calculations.
> >
> > Convert the exposure/shutter time fields in AgcStatus and DeviceStatus
> > to use utils::Duration.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/cam_helper.cpp | 2 +-
> > src/ipa/raspberrypi/controller/agc_status.h | 14 +--
> > .../raspberrypi/controller/device_status.h | 6 +-
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 86 +++++++++++--------
> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 28 +++---
> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 17 ++--
> > src/ipa/raspberrypi/controller/rpi/lux.hpp | 4 +-
> > src/ipa/raspberrypi/raspberrypi.cpp | 13 +--
> > 8 files changed, 96 insertions(+), 74 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index feb02d1806b2..968fae13bd5e 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -183,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> > return;
> > }
> >
> > - deviceStatus.shutter_speed =
> Exposure(exposureLines).get<std::micro>();
> > + deviceStatus.shutter_speed = Exposure(exposureLines);
> > deviceStatus.analogue_gain = Gain(gainCode);
> >
> > LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > diff --git a/src/ipa/raspberrypi/controller/agc_status.h
> b/src/ipa/raspberrypi/controller/agc_status.h
> > index 10381c90a313..c893715af588 100644
> > --- a/src/ipa/raspberrypi/controller/agc_status.h
> > +++ b/src/ipa/raspberrypi/controller/agc_status.h
> > @@ -6,6 +6,8 @@
> > */
> > #pragma once
> >
> > +#include "libcamera/internal/utils.h"
> > +
> > // The AGC algorithm should post the following structure into the
> image's
> > // "agc.status" metadata.
> >
> > @@ -17,18 +19,20 @@ extern "C" {
> > // seen statistics and calculated meaningful values. The contents
> should be
> > // ignored until then.
> >
> > +using libcamera::utils::Duration;
> > +
>
> Same comment as in other patches
>
Ack.
>
> > struct AgcStatus {
> > - double total_exposure_value; // value for all exposure and gain
> for this image
> > - double target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
> > - double shutter_time;
> > + Duration total_exposure_value; // value for all exposure and gain
> for this image
> > + Duration target_exposure_value; // (unfiltered) target total
> exposure AGC is aiming for
>
> Do we still need the _value postfix ?
>
Yes, that can be remove. However, can I leave that as a separate patch? I
want to do some
non-functional tidy-ups, and iI think it would be better as part of that
change.
>
> > + Duration shutter_time;
> > double analogue_gain;
> > char exposure_mode[32];
> > char constraint_mode[32];
> > char metering_mode[32];
> > double ev;
> > - double flicker_period;
> > + Duration flicker_period;
> > int floating_region_enable;
> > - double fixed_shutter;
> > + Duration fixed_shutter;
> > double fixed_analogue_gain;
> > double digital_gain;
> > int locked;
> > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> > index aa08608b5d40..131b4cd344ee 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.h
> > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > @@ -6,6 +6,8 @@
> > */
> > #pragma once
> >
> > +#include "libcamera/internal/utils.h"
> > +
> > // Definition of "device metadata" which stores things like shutter
> time and
> > // analogue gain that downstream control algorithms will want to know.
> >
> > @@ -14,8 +16,8 @@ extern "C" {
> > #endif
> >
> > struct DeviceStatus {
> > - // time shutter is open, in microseconds
> > - double shutter_speed;
> > + // time shutter is open
> > + libcamera::utils::Duration shutter_speed;
>
> You know, it's getting hard keeping track of what an algorithm assumes
> the time unit to be...
>
> Duration solves this, as you can get the time in any unit you like,
> but when passed around keeping track of the right conversions is hard.
> As we sure we don't want to specify the unit ?
>
I was also hesitant about removing any notion of units, but I have come to
embrace it :-)
The goal is to remove any ambiguity about passing around time based values
through different components, and Duration does that by letting the compiler
worry about conversions. I think mentioning units in the object name
slightly
defeats this purpose.
As proof of this, our AGC algorithm never needs to know about any time units
in all of the calculations it performs.
>
>
> > double analogue_gain;
> > // 1.0/distance-in-metres, or 0 if unknown
> > double lens_position;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 482eb3ef962d..21af561cb900 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -55,19 +55,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)
>
> Alignment
>
> > {
> > 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)
>
> ditto
>
Very curious, I see the alignment problem in the patch, yet my editor seems
to be perfectly aligned....?
check patch does not complain either... need to look at my settings again.
>
> > +{
> > + for (auto &p : params)
> > + list.push_back(p.second.get_value<double>() * 1us);
> > + return list.size();
> > +}
> > +
>
> Seems like we can now populate list with either raw doubles or
> duration in useconds. Is this intentional ?
>
Yes, this is the case. Our config file has doubles for gain and us for
exposure times.
>
> > 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"));
>
> Maybe it fits in one line ?
>
Ack.
>
>
> > + 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 +155,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);
> > }
> >
> > @@ -155,9 +163,9 @@ Agc::Agc(Controller *controller)
> > : AgcAlgorithm(controller), metering_mode_(nullptr),
> > exposure_mode_(nullptr), constraint_mode_(nullptr),
> > frame_count_(0), lock_count_(0),
> > - last_target_exposure_(0.0),
> > - ev_(1.0), flicker_period_(0.0),
> > - max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> > + last_target_exposure_(0s),
> > + ev_(1.0), flicker_period_(0s),
> > + max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)
> > {
> > memset(&awb_, 0, sizeof(awb_));
> > // Setting status_.total_exposure_value_ to zero initially tells us
> > @@ -203,7 +211,7 @@ void Agc::Pause()
> >
> > void Agc::Resume()
> > {
> > - fixed_shutter_ = 0;
> > + fixed_shutter_ = 0s;
> > fixed_analogue_gain_ = 0;
> > }
> >
> > @@ -224,17 +232,17 @@ void Agc::SetEv(double ev)
> >
> > void Agc::SetFlickerPeriod(const Duration &flicker_period)
> > {
> > - flicker_period_ = flicker_period.get<std::micro>();
> > + flicker_period_ = flicker_period;
> > }
> >
> > void Agc::SetMaxShutter(const Duration &max_shutter)
> > {
> > - max_shutter_ = max_shutter.get<std::micro>();
> > + max_shutter_ = max_shutter;
> > }
> >
> > void Agc::SetFixedShutter(const Duration &fixed_shutter)
> > {
> > - fixed_shutter_ = fixed_shutter.get<std::micro>();
> > + fixed_shutter_ = fixed_shutter;
> > // Set this in case someone calls Pause() straight after.
> > status_.shutter_time = clipShutter(fixed_shutter_);
> > }
> > @@ -266,8 +274,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
> const &camera_mode,
> > {
> > housekeepConfig();
> >
> > - double fixed_shutter = clipShutter(fixed_shutter_);
> > - if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> > + Duration fixed_shutter = clipShutter(fixed_shutter_);
> > + if (fixed_shutter && fixed_analogue_gain_) {
> > // We're going to reset the algorithm here with these
> fixed values.
> >
> > fetchAwbStatus(metadata);
> > @@ -312,8 +320,8 @@ void Agc::Prepare(Metadata *image_metadata)
> > // Process has run, so we have meaningful values.
> > DeviceStatus device_status;
> > if (image_metadata->Get("device.status", device_status) ==
> 0) {
> > - double actual_exposure =
> device_status.shutter_speed *
> > -
> device_status.analogue_gain;
> > + Duration actual_exposure =
> device_status.shutter_speed *
> > +
> device_status.analogue_gain;
> > if (actual_exposure) {
> > status_.digital_gain =
> > status_.total_exposure_value /
> > @@ -326,7 +334,8 @@ void Agc::Prepare(Metadata *image_metadata)
> > std::min(status_.digital_gain,
> 4.0));
> > LOG(RPiAgc, Debug) << "Actual exposure "
> << actual_exposure;
> > LOG(RPiAgc, Debug) << "Use digital_gain "
> << status_.digital_gain;
> > - LOG(RPiAgc, Debug) << "Effective exposure
> " << actual_exposure * status_.digital_gain;
> > + LOG(RPiAgc, Debug) << "Effective exposure "
> > + << actual_exposure *
> status_.digital_gain;
> > // Decide whether AEC/AGC has converged.
> > updateLockStatus(device_status);
> > }
> > @@ -370,9 +379,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 +471,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 +582,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics,
> Metadata *image_metadata,
> >
> > void Agc::computeTargetExposure(double gain)
> > {
> > - if (status_.fixed_shutter != 0.0 && status_.fixed_analogue_gain !=
> 0.0) {
> > + if (status_.fixed_shutter && status_.fixed_analogue_gain) {
> > // When ag and shutter are both fixed, we need to drive the
> > // total exposure so that we end up with a digital gain of
> at least
> > // 1/min_colour_gain. Otherwise we'd desaturate channels
> causing
> > @@ -588,11 +597,11 @@ void Agc::computeTargetExposure(double gain)
> > target_.total_exposure = current_.total_exposure_no_dg *
> gain;
> > // The final target exposure is also limited to what the
> exposure
> > // mode allows.
> > - double max_shutter = status_.fixed_shutter != 0.0
> > + Duration max_shutter = status_.fixed_shutter
> > ? status_.fixed_shutter
> > : exposure_mode_->shutter.back();
> > max_shutter = clipShutter(max_shutter);
> > - double max_total_exposure =
> > + Duration max_total_exposure =
> > max_shutter *
> > (status_.fixed_analogue_gain != 0.0
> > ? status_.fixed_analogue_gain
> > @@ -637,7 +646,7 @@ void Agc::filterExposure(bool desaturate)
> > if ((status_.fixed_shutter && status_.fixed_analogue_gain) ||
> > frame_count_ <= config_.startup_frames)
> > speed = 1.0;
> > - if (filtered_.total_exposure == 0.0) {
> > + if (!filtered_.total_exposure) {
> > filtered_.total_exposure = target_.total_exposure;
> > filtered_.total_exposure_no_dg =
> target_.total_exposure_no_dg;
> > } else {
> > @@ -674,9 +683,10 @@ void Agc::divideUpExposure()
> > // Sending the fixed shutter/gain cases through the same code may
> seem
> > // unnecessary, but it will make more sense when extend this to
> cover
> > // variable aperture.
> > - double exposure_value = filtered_.total_exposure_no_dg;
> > - double shutter_time, analogue_gain;
> > - shutter_time = status_.fixed_shutter != 0.0
> > + Duration exposure_value = filtered_.total_exposure_no_dg;
> > + Duration shutter_time;
> > + double analogue_gain;
> > + shutter_time = status_.fixed_shutter
> > ? status_.fixed_shutter
> > : exposure_mode_->shutter[0];
> > shutter_time = clipShutter(shutter_time);
> > @@ -686,8 +696,8 @@ void Agc::divideUpExposure()
> > if (shutter_time * analogue_gain < exposure_value) {
> > for (unsigned int stage = 1;
> > stage < exposure_mode_->gain.size(); stage++) {
> > - if (status_.fixed_shutter == 0.0) {
> > - double stage_shutter =
> > + if (!status_.fixed_shutter) {
> > + Duration stage_shutter =
> >
> clipShutter(exposure_mode_->shutter[stage]);
> > if (stage_shutter * analogue_gain >=
> > exposure_value) {
> > @@ -713,12 +723,11 @@ void Agc::divideUpExposure()
> > << analogue_gain;
> > // Finally adjust shutter time for flicker avoidance (require both
> > // shutter and gain not to be fixed).
> > - if (status_.fixed_shutter == 0.0 &&
> > - status_.fixed_analogue_gain == 0.0 &&
> > - status_.flicker_period != 0.0) {
> > + if (!status_.fixed_shutter && !status_.fixed_analogue_gain &&
> > + status_.flicker_period) {
> > int flicker_periods = shutter_time /
> status_.flicker_period;
> > - if (flicker_periods > 0) {
> > - double new_shutter_time = flicker_periods *
> status_.flicker_period;
> > + if (flicker_periods) {
> > + 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,11 +759,12 @@ void Agc::writeAndFinish(Metadata *image_metadata,
> bool desaturate)
> > << " analogue gain " << filtered_.analogue_gain;
> > }
> >
> > -double Agc::clipShutter(double shutter)
> > +Duration Agc::clipShutter(const Duration &shutter)
> > {
> > + Duration clip_shutter = shutter;
> > if (max_shutter_)
> > - shutter = std::min(shutter, max_shutter_);
> > - return shutter;
> > + clip_shutter = std::min(clip_shutter, max_shutter_);
> > + return clip_shutter;
>
> return max_shutter_ ? std::min(shutter, max_shutter_) : shutter_;
> > }
> >
> > // Register algorithm with the system.
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 16a65959d90e..24a6610096c2 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -9,6 +9,8 @@
> > #include <vector>
> > #include <mutex>
> >
> > +#include "libcamera/internal/utils.h"
> > +
> > #include "../agc_algorithm.hpp"
> > #include "../agc_status.h"
> > #include "../pwl.hpp"
> > @@ -22,13 +24,15 @@
> >
> > namespace RPiController {
> >
> > +using namespace std::literals::chrono_literals;
> > +
> > struct AgcMeteringMode {
> > double weights[AGC_STATS_SIZE];
> > void Read(boost::property_tree::ptree const ¶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 +65,7 @@ struct AgcConfig {
> > std::string default_exposure_mode;
> > std::string default_constraint_mode;
> > double base_ev;
> > - double default_exposure_time;
> > + Duration default_exposure_time;
> > double default_analogue_gain;
> > };
> >
> > @@ -101,19 +105,19 @@ private:
> > void filterExposure(bool desaturate);
> > void divideUpExposure();
> > void writeAndFinish(Metadata *image_metadata, bool desaturate);
> > - double clipShutter(double shutter);
> > + Duration clipShutter(const Duration &shutter);
> > AgcMeteringMode *metering_mode_;
> > AgcExposureMode *exposure_mode_;
> > AgcConstraintMode *constraint_mode_;
> > uint64_t frame_count_;
> > AwbStatus awb_;
> > struct ExposureValues {
> > - ExposureValues() : shutter(0), analogue_gain(0),
> > - total_exposure(0),
> total_exposure_no_dg(0) {}
> > - double shutter;
> > + ExposureValues() : shutter(0s), analogue_gain(0),
> > + total_exposure(0s),
> total_exposure_no_dg(0s) {}
> > + Duration shutter;
> > double analogue_gain;
> > - double total_exposure;
> > - double total_exposure_no_dg; // without digital gain
> > + Duration total_exposure;
> > + Duration total_exposure_no_dg; // without digital gain
> > };
> > ExposureValues current_; // values for the current frame
> > ExposureValues target_; // calculate the values we want here
> > @@ -121,15 +125,15 @@ private:
> > AgcStatus status_;
> > int lock_count_;
> > DeviceStatus last_device_status_;
> > - double last_target_exposure_;
> > + Duration last_target_exposure_;
> > // Below here the "settings" that applications can change.
> > std::string metering_mode_name_;
> > std::string exposure_mode_name_;
> > std::string constraint_mode_name_;
> > double ev_;
> > - double flicker_period_;
> > - double max_shutter_;
> > - double fixed_shutter_;
> > + Duration flicker_period_;
> > + Duration max_shutter_;
> > + Duration fixed_shutter_;
> > double fixed_analogue_gain_;
> > };
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > index f74381cab2b4..0fa6457c1376 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > @@ -16,6 +16,7 @@
> >
> > using namespace RPiController;
> > using namespace libcamera;
> > +using namespace std::literals::chrono_literals;
> >
> > LOG_DEFINE_CATEGORY(RPiLux)
> >
> > @@ -38,7 +39,7 @@ char const *Lux::Name() const
> > void Lux::Read(boost::property_tree::ptree const ¶ms)
> > {
> > reference_shutter_speed_ =
> > - params.get<double>("reference_shutter_speed");
> > + params.get<double>("reference_shutter_speed") * 1.0us;
> > reference_gain_ = params.get<double>("reference_gain");
> > reference_aperture_ = params.get<double>("reference_aperture",
> 1.0);
> > reference_Y_ = params.get<double>("reference_Y");
> > @@ -60,15 +61,13 @@ void Lux::Prepare(Metadata *image_metadata)
> > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> > {
> > // set some initial values to shut the compiler up
> > - DeviceStatus device_status =
> > - { .shutter_speed = 1.0,
> > - .analogue_gain = 1.0,
> > - .lens_position = 0.0,
> > - .aperture = 0.0,
> > - .flash_intensity = 0.0 };
> > + DeviceStatus device_status = { .shutter_speed = 1.0ms,
> > + .analogue_gain = 1.0,
> > + .lens_position = 0.0,
> > + .aperture = 0.0,
> > + .flash_intensity = 0.0 };
>
> That's two weird styles :)
> What about the usual
>
> DeviceStatus device_status = {
> .shutter_speed ....
> ....
> };
>
Ack.
I blindly copied what check_patch suggested for this block.
>
> > if (image_metadata->Get("device.status", device_status) == 0) {
> > double current_gain = device_status.analogue_gain;
> > - double current_shutter_speed = device_status.shutter_speed;
>
> Why is current_shutter_speed different and doesn't deserve a local
> variable now ?
>
The local variable was unneeded as it is only used in the calculation a few
lines below. So I removed it.
>
> > double current_aperture = device_status.aperture;
> > if (current_aperture == 0)
> > current_aperture = current_aperture_;
> > @@ -83,7 +82,7 @@ void Lux::Process(StatisticsPtr &stats, Metadata
> *image_metadata)
> > double current_Y = sum / (double)num + .5;
> > double gain_ratio = reference_gain_ / current_gain;
> > double shutter_speed_ratio =
> > - reference_shutter_speed_ / current_shutter_speed;
> > + reference_shutter_speed_ /
> device_status.shutter_speed;
> > double aperture_ratio = reference_aperture_ /
> current_aperture;
> > double Y_ratio = current_Y * (65536 / num_bins) /
> reference_Y_;
> > double estimated_lux = shutter_speed_ratio * gain_ratio *
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > index f9090484a136..45c844393e62 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.hpp
> > @@ -8,6 +8,8 @@
> >
> > #include <mutex>
> >
> > +#include "libcamera/internal/utils.h"
> > +
> > #include "../lux_status.h"
> > #include "../algorithm.hpp"
> >
> > @@ -28,7 +30,7 @@ public:
> > private:
> > // These values define the conditions of the reference image,
> against
> > // which we compare the new image.
> > - double reference_shutter_speed_; // in micro-seconds
> > + libcamera::utils::Duration reference_shutter_speed_;
> > double reference_gain_;
> > double reference_aperture_; // units of 1/f
> > double reference_Y_; // out of 65536
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index e083f6c254cc..1c77549b618d 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -224,11 +224,11 @@ void IPARPi::start(const ControlList &controls,
> ipa::RPi::StartConfig *startConf
> >
> > /* SwitchMode may supply updated exposure/gain values to use. */
> > AgcStatus agcStatus;
> > - agcStatus.shutter_time = 0.0;
> > + agcStatus.shutter_time = 0.0s;
> > agcStatus.analogue_gain = 0.0;
> >
> > metadata.Get("agc.status", agcStatus);
> > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=
> 0.0) {
> > + if (agcStatus.shutter_time && agcStatus.analogue_gain) {
> > ControlList ctrls(sensorCtrls_);
> > applyAGC(&agcStatus, ctrls);
> > startConfig->controls = std::move(ctrls);
> > @@ -391,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > /* Supply initial values for gain and exposure. */
> > ControlList ctrls(sensorCtrls_);
> > AgcStatus agcStatus;
> > - agcStatus.shutter_time =
> DefaultExposureTime.get<std::micro>();
> > + agcStatus.shutter_time = DefaultExposureTime;
> > agcStatus.analogue_gain = DefaultAnalogueGain;
> > applyAGC(&agcStatus, ctrls);
> >
> > @@ -463,7 +463,8 @@ void IPARPi::reportMetadata()
> > */
> > DeviceStatus *deviceStatus =
> rpiMetadata_.GetLocked<DeviceStatus>("device.status");
> > if (deviceStatus) {
> > - libcameraMetadata_.set(controls::ExposureTime,
> deviceStatus->shutter_speed);
> > + libcameraMetadata_.set(controls::ExposureTime,
> > +
> deviceStatus->shutter_speed.get<std::micro>());
> > libcameraMetadata_.set(controls::AnalogueGain,
> deviceStatus->analogue_gain);
> > }
> >
> > @@ -1016,7 +1017,7 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls)
> > int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >
> > - deviceStatus.shutter_speed =
> helper_->Exposure(exposureLines).get<std::micro>();
> > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >
> > LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > @@ -1102,7 +1103,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);
>
> All minor drive-by comments, so please add
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
> j
>
> >
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210524/a1e2d654/attachment-0001.htm>
More information about the libcamera-devel
mailing list