| Message ID | 20210518100706.578526-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch. On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > Switch the ipa and cam_helper code to use RPiController::Duration for > all time based variables. This improves code readability and avoids > possible errors when converting between time bases. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/cam_helper.cpp | 19 +++--- > src/ipa/raspberrypi/cam_helper.hpp | 10 +-- > src/ipa/raspberrypi/controller/camera_mode.h | 5 +- > src/ipa/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------- > 4 files changed, 56 insertions(+), 45 deletions(-) > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp > index 09917f3cc079..e2b6c8eb8e03 100644 > --- a/src/ipa/raspberrypi/cam_helper.cpp > +++ b/src/ipa/raspberrypi/cam_helper.cpp > @@ -61,20 +61,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats, > { > } > > -uint32_t CamHelper::ExposureLines(double exposure_us) const > +uint32_t CamHelper::ExposureLines(Duration exposure) const I take it we're happy to pass these by value. I assume they're not big, just a double plus some kind of time unit? > { > assert(initialized_); > - return exposure_us * 1000.0 / mode_.line_length; > + return exposure / mode_.line_length; > } > > -double CamHelper::Exposure(uint32_t exposure_lines) const > +Duration CamHelper::Exposure(uint32_t exposure_lines) const > { > assert(initialized_); > - return exposure_lines * mode_.line_length / 1000.0; > + return exposure_lines * mode_.line_length; Yes, I do prefer this! > } > > -uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration, > - double maxFrameDuration) const > +uint32_t CamHelper::GetVBlanking(Duration &exposure, > + Duration minFrameDuration, > + Duration maxFrameDuration) const > { > uint32_t frameLengthMin, frameLengthMax, vblank; > uint32_t exposureLines = ExposureLines(exposure); > @@ -85,8 +86,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration, > * minFrameDuration and maxFrameDuration are clamped by the caller > * based on the limits for the active sensor mode. > */ > - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length; > - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length; > + frameLengthMin = minFrameDuration / mode_.line_length; > + frameLengthMax = maxFrameDuration / mode_.line_length; > > /* > * Limit the exposure to the maximum frame duration requested, and > @@ -182,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer, > return; > } > > - deviceStatus.shutter_speed = Exposure(exposureLines); > + deviceStatus.shutter_speed = DurationValue<std::micro>(Exposure(exposureLines)); Ah yes, so this is one of those "in between" changes which will get better again in a later patch... > deviceStatus.analogue_gain = Gain(gainCode); > > LOG(IPARPI, Debug) << "Metadata updated - Exposure : " > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp > index a52f3f0b583c..a91afaa59909 100644 > --- a/src/ipa/raspberrypi/cam_helper.hpp > +++ b/src/ipa/raspberrypi/cam_helper.hpp > @@ -13,6 +13,7 @@ > #include "camera_mode.h" > #include "controller/controller.hpp" > #include "controller/metadata.hpp" > +#include "duration.hpp" > #include "md_parser.hpp" > > #include "libcamera/internal/v4l2_videodevice.h" > @@ -72,10 +73,11 @@ public: > virtual void Prepare(libcamera::Span<const uint8_t> buffer, > Metadata &metadata); > virtual void Process(StatisticsPtr &stats, Metadata &metadata); > - uint32_t ExposureLines(double exposure_us) const; > - double Exposure(uint32_t exposure_lines) const; // in us > - virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration, > - double maxFrameDuration) const; > + uint32_t ExposureLines(Duration exposure) const; > + Duration Exposure(uint32_t exposure_lines) const; > + virtual uint32_t GetVBlanking(Duration &exposure, > + Duration minFrameDuration, > + Duration maxFrameDuration) const; > virtual uint32_t GainCode(double gain) const = 0; > virtual double Gain(uint32_t gain_code) const = 0; > virtual void GetDelays(int &exposure_delay, int &gain_delay, > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h > index 256438c931d9..ab7cf2912a06 100644 > --- a/src/ipa/raspberrypi/controller/camera_mode.h > +++ b/src/ipa/raspberrypi/controller/camera_mode.h > @@ -6,6 +6,7 @@ > */ > #pragma once > > +#include "duration.hpp" > #include <libcamera/transform.h> > > // Description of a "camera mode", holding enough information for control > @@ -33,8 +34,8 @@ struct CameraMode { > double scale_x, scale_y; > // scaling of the noise compared to the native sensor mode > double noise_factor; > - // line time in nanoseconds > - double line_length; > + // line time > + RPiController::Duration line_length; > // any camera transform *not* reflected already in the camera tuning > libcamera::Transform transform; > // minimum and maximum fame lengths in units of lines > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 52d91db282ea..994fb7e057a9 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -45,6 +45,7 @@ > #include "denoise_algorithm.hpp" > #include "denoise_status.h" > #include "dpc_status.h" > +#include "duration.hpp" > #include "focus_status.h" > #include "geq_status.h" > #include "lux_status.h" > @@ -55,19 +56,24 @@ > > namespace libcamera { > > +using namespace std::literals::chrono_literals; > +using RPiController::Duration; > +using RPiController::DurationValue; > +using RPiController::operator<<; > + > /* Configure the sensor with these values initially. */ > constexpr double DefaultAnalogueGain = 1.0; > -constexpr unsigned int DefaultExposureTime = 20000; > -constexpr double defaultMinFrameDuration = 1e6 / 30.0; > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01; > +constexpr Duration DefaultExposureTime = 20.0ms; > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0; > +constexpr Duration defaultMaxFrameDuration = 100.0s; > > /* > - * Determine the minimum allowable inter-frame duration (in us) to run the > - * controller algorithms. If the pipeline handler provider frames at a rate > - * higher than this, we rate-limit the controller Prepare() and Process() calls > - * to lower than or equal to this rate. > + * Determine the minimum allowable inter-frame duration to run the controller > + * algorithms. If the pipeline handler provider frames at a rate higher than this, > + * we rate-limit the controller Prepare() and Process() calls to lower than or > + * equal to this rate. > */ > -constexpr double controllerMinFrameDuration = 1e6 / 60.0; > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0; > > LOG_DEFINE_CATEGORY(IPARPI) > > @@ -111,7 +117,8 @@ private: > void reportMetadata(); > void fillDeviceStatus(const ControlList &sensorControls); > void processStats(unsigned int bufferId); > - void applyFrameDurations(double minFrameDuration, double maxFrameDuration); > + void applyFrameDurations(const Duration &minFrameDuration, > + const Duration &maxFrameDuration); Some pass-by-reference, I notice. Do we want to be consistent? Are we bothered (I'm not...)? > void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls); > @@ -167,9 +174,9 @@ private: > /* Distinguish the first camera start from others. */ > bool firstStart_; > > - /* Frame duration (1/fps) limits, given in microseconds. */ > - double minFrameDuration_; > - double maxFrameDuration_; > + /* Frame duration (1/fps) limits. */ > + Duration minFrameDuration_; > + Duration maxFrameDuration_; > }; > > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) > @@ -314,7 +321,7 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo) > * Calculate the line length in nanoseconds as the ratio between Elsewhere you've tended to remove the mention of units, so does it want the same here? > * the line length in pixels and the pixel rate. > */ > - mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate; > + mode_.line_length = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate); > > /* > * Set the frame length limits for the mode to ensure exposure and > @@ -387,7 +394,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo, > /* Supply initial values for gain and exposure. */ > ControlList ctrls(sensorCtrls_); > AgcStatus agcStatus; > - agcStatus.shutter_time = DefaultExposureTime; > + agcStatus.shutter_time = DurationValue<std::micro>(DefaultExposureTime); > agcStatus.analogue_gain = DefaultAnalogueGain; > applyAGC(&agcStatus, ctrls); > > @@ -862,7 +869,7 @@ void IPARPi::queueRequest(const ControlList &controls) > > case controls::FRAME_DURATIONS: { > auto frameDurations = ctrl.second.get<Span<const int64_t>>(); > - applyFrameDurations(frameDurations[0], frameDurations[1]); > + applyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us); > break; > } > > @@ -937,9 +944,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > returnEmbeddedBuffer(data.embeddedBufferId); > > /* Allow a 10% margin on the comparison below. */ > - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1; > + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > - frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) { > + delta < controllerMinFrameDuration * 0.9) { > /* > * Ensure we merge the previous frame's metadata with the current > * frame. This will not overwrite exposure/gain values for the > @@ -1012,7 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls) > int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > > - deviceStatus.shutter_speed = helper_->Exposure(exposureLines); > + deviceStatus.shutter_speed = DurationValue<std::micro>(helper_->Exposure(exposureLines)); > deviceStatus.analogue_gain = helper_->Gain(gainCode); > > LOG(IPARPI, Debug) << "Metadata - Exposure : " > @@ -1057,17 +1064,18 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) > static_cast<int32_t>(awbStatus->gain_b * 1000)); > } > > -void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration) > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration, > + const Duration &maxFrameDuration) > { > - const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length; > - const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length; > + const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length; > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; > > /* > * This will only be applied once AGC recalculations occur. > * The values may be clamped based on the sensor mode capabilities as well. > */ > - minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration; > - maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration; > + minFrameDuration_ = (minFrameDuration > 0.0s) ? minFrameDuration : defaultMaxFrameDuration; > + maxFrameDuration_ = (maxFrameDuration > 0.0s) ? maxFrameDuration : defaultMinFrameDuration; > minFrameDuration_ = std::clamp(minFrameDuration_, > minSensorFrameDuration, maxSensorFrameDuration); > maxFrameDuration_ = std::clamp(maxFrameDuration_, > @@ -1076,20 +1084,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio > > /* Return the validated limits via metadata. */ > libcameraMetadata_.set(controls::FrameDurations, > - { static_cast<int64_t>(minFrameDuration_), > - static_cast<int64_t>(maxFrameDuration_) }); > + { static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)), > + static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)) }); And this will look nicer again once the whole of libcamera adopts std::chrono! :) So no significant comments really from me, thus: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > > /* > * Calculate the maximum exposure time possible for the AGC to use. > * GetVBlanking() will update maxShutter with the largest exposure > * value possible. > */ > - double maxShutter = std::numeric_limits<double>::max(); > + Duration maxShutter = Duration::max(); > helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_); > > RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( > controller_.GetAlgorithm("agc")); > - agc->SetMaxShutter(maxShutter); > + agc->SetMaxShutter(DurationValue<std::micro>(maxShutter)); > } > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > @@ -1097,9 +1105,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); > > /* GetVBlanking might clip exposure time to the fps limits. */ > - double exposure = agcStatus->shutter_time; > - int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, > - maxFrameDuration_); > + Duration exposure = agcStatus->shutter_time * 1.0us; > + int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_); > int32_t exposureLines = helper_->ExposureLines(exposure); > > LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure > -- > 2.25.1 >
Hi David, Thank you for your review feedback. On Wed, 19 May 2021 at 15:09, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this patch. > > On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > Switch the ipa and cam_helper code to use RPiController::Duration for > > all time based variables. This improves code readability and avoids > > possible errors when converting between time bases. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/cam_helper.cpp | 19 +++--- > > src/ipa/raspberrypi/cam_helper.hpp | 10 +-- > > src/ipa/raspberrypi/controller/camera_mode.h | 5 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------- > > 4 files changed, 56 insertions(+), 45 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp > b/src/ipa/raspberrypi/cam_helper.cpp > > index 09917f3cc079..e2b6c8eb8e03 100644 > > --- a/src/ipa/raspberrypi/cam_helper.cpp > > +++ b/src/ipa/raspberrypi/cam_helper.cpp > > @@ -61,20 +61,21 @@ void CamHelper::Process([[maybe_unused]] > StatisticsPtr &stats, > > { > > } > > > > -uint32_t CamHelper::ExposureLines(double exposure_us) const > > +uint32_t CamHelper::ExposureLines(Duration exposure) const > > I take it we're happy to pass these by value. I assume they're not > big, just a double plus some kind of time unit? > There is minimal overhead, but perhaps I should change to a const reference to be consistent. > > > { > > assert(initialized_); > > - return exposure_us * 1000.0 / mode_.line_length; > > + return exposure / mode_.line_length; > > } > > > > -double CamHelper::Exposure(uint32_t exposure_lines) const > > +Duration CamHelper::Exposure(uint32_t exposure_lines) const > > { > > assert(initialized_); > > - return exposure_lines * mode_.line_length / 1000.0; > > + return exposure_lines * mode_.line_length; > > Yes, I do prefer this! > > > } > > > > -uint32_t CamHelper::GetVBlanking(double &exposure, double > minFrameDuration, > > - double maxFrameDuration) const > > +uint32_t CamHelper::GetVBlanking(Duration &exposure, > > + Duration minFrameDuration, > > + Duration maxFrameDuration) const > > { > > uint32_t frameLengthMin, frameLengthMax, vblank; > > uint32_t exposureLines = ExposureLines(exposure); > > @@ -85,8 +86,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure, > double minFrameDuration, > > * minFrameDuration and maxFrameDuration are clamped by the > caller > > * based on the limits for the active sensor mode. > > */ > > - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length; > > - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length; > > + frameLengthMin = minFrameDuration / mode_.line_length; > > + frameLengthMax = maxFrameDuration / mode_.line_length; > > > > /* > > * Limit the exposure to the maximum frame duration requested, > and > > @@ -182,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const > uint8_t> buffer, > > return; > > } > > > > - deviceStatus.shutter_speed = Exposure(exposureLines); > > + deviceStatus.shutter_speed = > DurationValue<std::micro>(Exposure(exposureLines)); > > Ah yes, so this is one of those "in between" changes which will get > better again in a later patch... > Yes, this will be reverted in the later patch. > > > deviceStatus.analogue_gain = Gain(gainCode); > > > > LOG(IPARPI, Debug) << "Metadata updated - Exposure : " > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp > b/src/ipa/raspberrypi/cam_helper.hpp > > index a52f3f0b583c..a91afaa59909 100644 > > --- a/src/ipa/raspberrypi/cam_helper.hpp > > +++ b/src/ipa/raspberrypi/cam_helper.hpp > > @@ -13,6 +13,7 @@ > > #include "camera_mode.h" > > #include "controller/controller.hpp" > > #include "controller/metadata.hpp" > > +#include "duration.hpp" > > #include "md_parser.hpp" > > > > #include "libcamera/internal/v4l2_videodevice.h" > > @@ -72,10 +73,11 @@ public: > > virtual void Prepare(libcamera::Span<const uint8_t> buffer, > > Metadata &metadata); > > virtual void Process(StatisticsPtr &stats, Metadata &metadata); > > - uint32_t ExposureLines(double exposure_us) const; > > - double Exposure(uint32_t exposure_lines) const; // in us > > - virtual uint32_t GetVBlanking(double &exposure_us, double > minFrameDuration, > > - double maxFrameDuration) const; > > + uint32_t ExposureLines(Duration exposure) const; > > + Duration Exposure(uint32_t exposure_lines) const; > > + virtual uint32_t GetVBlanking(Duration &exposure, > > + Duration minFrameDuration, > > + Duration maxFrameDuration) const; > > virtual uint32_t GainCode(double gain) const = 0; > > virtual double Gain(uint32_t gain_code) const = 0; > > virtual void GetDelays(int &exposure_delay, int &gain_delay, > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h > b/src/ipa/raspberrypi/controller/camera_mode.h > > index 256438c931d9..ab7cf2912a06 100644 > > --- a/src/ipa/raspberrypi/controller/camera_mode.h > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h > > @@ -6,6 +6,7 @@ > > */ > > #pragma once > > > > +#include "duration.hpp" > > #include <libcamera/transform.h> > > > > // Description of a "camera mode", holding enough information for > control > > @@ -33,8 +34,8 @@ struct CameraMode { > > double scale_x, scale_y; > > // scaling of the noise compared to the native sensor mode > > double noise_factor; > > - // line time in nanoseconds > > - double line_length; > > + // line time > > + RPiController::Duration line_length; > > // any camera transform *not* reflected already in the camera > tuning > > libcamera::Transform transform; > > // minimum and maximum fame lengths in units of lines > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index 52d91db282ea..994fb7e057a9 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -45,6 +45,7 @@ > > #include "denoise_algorithm.hpp" > > #include "denoise_status.h" > > #include "dpc_status.h" > > +#include "duration.hpp" > > #include "focus_status.h" > > #include "geq_status.h" > > #include "lux_status.h" > > @@ -55,19 +56,24 @@ > > > > namespace libcamera { > > > > +using namespace std::literals::chrono_literals; > > +using RPiController::Duration; > > +using RPiController::DurationValue; > > +using RPiController::operator<<; > > + > > /* Configure the sensor with these values initially. */ > > constexpr double DefaultAnalogueGain = 1.0; > > -constexpr unsigned int DefaultExposureTime = 20000; > > -constexpr double defaultMinFrameDuration = 1e6 / 30.0; > > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01; > > +constexpr Duration DefaultExposureTime = 20.0ms; > > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0; > > +constexpr Duration defaultMaxFrameDuration = 100.0s; > > > > /* > > - * Determine the minimum allowable inter-frame duration (in us) to run > the > > - * controller algorithms. If the pipeline handler provider frames at a > rate > > - * higher than this, we rate-limit the controller Prepare() and > Process() calls > > - * to lower than or equal to this rate. > > + * Determine the minimum allowable inter-frame duration to run the > controller > > + * algorithms. If the pipeline handler provider frames at a rate higher > than this, > > + * we rate-limit the controller Prepare() and Process() calls to lower > than or > > + * equal to this rate. > > */ > > -constexpr double controllerMinFrameDuration = 1e6 / 60.0; > > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0; > > > > LOG_DEFINE_CATEGORY(IPARPI) > > > > @@ -111,7 +117,8 @@ private: > > void reportMetadata(); > > void fillDeviceStatus(const ControlList &sensorControls); > > void processStats(unsigned int bufferId); > > - void applyFrameDurations(double minFrameDuration, double > maxFrameDuration); > > + void applyFrameDurations(const Duration &minFrameDuration, > > + const Duration &maxFrameDuration); > > Some pass-by-reference, I notice. Do we want to be consistent? Are we > bothered (I'm not...)? > I will change to using const references in the other places to be consistent. > > > void applyAGC(const struct AgcStatus *agcStatus, ControlList > &ctrls); > > void applyAWB(const struct AwbStatus *awbStatus, ControlList > &ctrls); > > void applyDG(const struct AgcStatus *dgStatus, ControlList > &ctrls); > > @@ -167,9 +174,9 @@ private: > > /* Distinguish the first camera start from others. */ > > bool firstStart_; > > > > - /* Frame duration (1/fps) limits, given in microseconds. */ > > - double minFrameDuration_; > > - double maxFrameDuration_; > > + /* Frame duration (1/fps) limits. */ > > + Duration minFrameDuration_; > > + Duration maxFrameDuration_; > > }; > > > > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig > *sensorConfig) > > @@ -314,7 +321,7 @@ void IPARPi::setMode(const CameraSensorInfo > &sensorInfo) > > * Calculate the line length in nanoseconds as the ratio between > > Elsewhere you've tended to remove the mention of units, so does it > want the same here? > Oops, that one I missed. > > > * the line length in pixels and the pixel rate. > > */ > > - mode_.line_length = 1e9 * sensorInfo.lineLength / > sensorInfo.pixelRate; > > + mode_.line_length = sensorInfo.lineLength * (1.0s / > sensorInfo.pixelRate); > > > > /* > > * Set the frame length limits for the mode to ensure exposure > and > > @@ -387,7 +394,7 @@ int IPARPi::configure(const CameraSensorInfo > &sensorInfo, > > /* Supply initial values for gain and exposure. */ > > ControlList ctrls(sensorCtrls_); > > AgcStatus agcStatus; > > - agcStatus.shutter_time = DefaultExposureTime; > > + agcStatus.shutter_time = > DurationValue<std::micro>(DefaultExposureTime); > > agcStatus.analogue_gain = DefaultAnalogueGain; > > applyAGC(&agcStatus, ctrls); > > > > @@ -862,7 +869,7 @@ void IPARPi::queueRequest(const ControlList > &controls) > > > > case controls::FRAME_DURATIONS: { > > auto frameDurations = ctrl.second.get<Span<const > int64_t>>(); > > - applyFrameDurations(frameDurations[0], > frameDurations[1]); > > + applyFrameDurations(frameDurations[0] * 1.0us, > frameDurations[1] * 1.0us); > > break; > > } > > > > @@ -937,9 +944,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig > &data) > > returnEmbeddedBuffer(data.embeddedBufferId); > > > > /* Allow a 10% margin on the comparison below. */ > > - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1; > > + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > > - frameTimestamp - lastRunTimestamp_ + eps < > controllerMinFrameDuration * 1e3) { > > + delta < controllerMinFrameDuration * 0.9) { > > /* > > * Ensure we merge the previous frame's metadata with > the current > > * frame. This will not overwrite exposure/gain values > for the > > @@ -1012,7 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList > &sensorControls) > > int32_t exposureLines = > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > int32_t gainCode = > sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > > > > - deviceStatus.shutter_speed = helper_->Exposure(exposureLines); > > + deviceStatus.shutter_speed = > DurationValue<std::micro>(helper_->Exposure(exposureLines)); > > deviceStatus.analogue_gain = helper_->Gain(gainCode); > > > > LOG(IPARPI, Debug) << "Metadata - Exposure : " > > @@ -1057,17 +1064,18 @@ void IPARPi::applyAWB(const struct AwbStatus > *awbStatus, ControlList &ctrls) > > static_cast<int32_t>(awbStatus->gain_b * 1000)); > > } > > > > -void IPARPi::applyFrameDurations(double minFrameDuration, double > maxFrameDuration) > > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration, > > + const Duration &maxFrameDuration) > > { > > - const double minSensorFrameDuration = 1e-3 * > mode_.min_frame_length * mode_.line_length; > > - const double maxSensorFrameDuration = 1e-3 * > mode_.max_frame_length * mode_.line_length; > > + const Duration minSensorFrameDuration = mode_.min_frame_length * > mode_.line_length; > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * > mode_.line_length; > > > > /* > > * This will only be applied once AGC recalculations occur. > > * The values may be clamped based on the sensor mode > capabilities as well. > > */ > > - minFrameDuration_ = minFrameDuration ? minFrameDuration : > defaultMaxFrameDuration; > > - maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : > defaultMinFrameDuration; > > + minFrameDuration_ = (minFrameDuration > 0.0s) ? minFrameDuration > : defaultMaxFrameDuration; > > + maxFrameDuration_ = (maxFrameDuration > 0.0s) ? maxFrameDuration > : defaultMinFrameDuration; > > minFrameDuration_ = std::clamp(minFrameDuration_, > > minSensorFrameDuration, > maxSensorFrameDuration); > > maxFrameDuration_ = std::clamp(maxFrameDuration_, > > @@ -1076,20 +1084,20 @@ void IPARPi::applyFrameDurations(double > minFrameDuration, double maxFrameDuratio > > > > /* Return the validated limits via metadata. */ > > libcameraMetadata_.set(controls::FrameDurations, > > - { static_cast<int64_t>(minFrameDuration_), > > - static_cast<int64_t>(maxFrameDuration_) > }); > > + { > static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)), > > + > static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)) }); > > And this will look nicer again once the whole of libcamera adopts > std::chrono! :) > Yes, but that comes later :-) > > So no significant comments really from me, thus: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks > David > > > > > /* > > * Calculate the maximum exposure time possible for the AGC to > use. > > * GetVBlanking() will update maxShutter with the largest > exposure > > * value possible. > > */ > > - double maxShutter = std::numeric_limits<double>::max(); > > + Duration maxShutter = Duration::max(); > > helper_->GetVBlanking(maxShutter, minFrameDuration_, > maxFrameDuration_); > > > > RPiController::AgcAlgorithm *agc = > dynamic_cast<RPiController::AgcAlgorithm *>( > > controller_.GetAlgorithm("agc")); > > - agc->SetMaxShutter(maxShutter); > > + agc->SetMaxShutter(DurationValue<std::micro>(maxShutter)); > > } > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList > &ctrls) > > @@ -1097,9 +1105,8 @@ void IPARPi::applyAGC(const struct AgcStatus > *agcStatus, ControlList &ctrls) > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); > > > > /* GetVBlanking might clip exposure time to the fps limits. */ > > - double exposure = agcStatus->shutter_time; > > - int32_t vblanking = helper_->GetVBlanking(exposure, > minFrameDuration_, > > - maxFrameDuration_); > > + Duration exposure = agcStatus->shutter_time * 1.0us; > > + int32_t vblanking = helper_->GetVBlanking(exposure, > minFrameDuration_, maxFrameDuration_); > > int32_t exposureLines = helper_->ExposureLines(exposure); > > > > LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure > > -- > > 2.25.1 > > >
diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp index 09917f3cc079..e2b6c8eb8e03 100644 --- a/src/ipa/raspberrypi/cam_helper.cpp +++ b/src/ipa/raspberrypi/cam_helper.cpp @@ -61,20 +61,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats, { } -uint32_t CamHelper::ExposureLines(double exposure_us) const +uint32_t CamHelper::ExposureLines(Duration exposure) const { assert(initialized_); - return exposure_us * 1000.0 / mode_.line_length; + return exposure / mode_.line_length; } -double CamHelper::Exposure(uint32_t exposure_lines) const +Duration CamHelper::Exposure(uint32_t exposure_lines) const { assert(initialized_); - return exposure_lines * mode_.line_length / 1000.0; + return exposure_lines * mode_.line_length; } -uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration, - double maxFrameDuration) const +uint32_t CamHelper::GetVBlanking(Duration &exposure, + Duration minFrameDuration, + Duration maxFrameDuration) const { uint32_t frameLengthMin, frameLengthMax, vblank; uint32_t exposureLines = ExposureLines(exposure); @@ -85,8 +86,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration, * minFrameDuration and maxFrameDuration are clamped by the caller * based on the limits for the active sensor mode. */ - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length; - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length; + frameLengthMin = minFrameDuration / mode_.line_length; + frameLengthMax = maxFrameDuration / mode_.line_length; /* * Limit the exposure to the maximum frame duration requested, and @@ -182,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer, return; } - deviceStatus.shutter_speed = Exposure(exposureLines); + deviceStatus.shutter_speed = DurationValue<std::micro>(Exposure(exposureLines)); deviceStatus.analogue_gain = Gain(gainCode); LOG(IPARPI, Debug) << "Metadata updated - Exposure : " diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp index a52f3f0b583c..a91afaa59909 100644 --- a/src/ipa/raspberrypi/cam_helper.hpp +++ b/src/ipa/raspberrypi/cam_helper.hpp @@ -13,6 +13,7 @@ #include "camera_mode.h" #include "controller/controller.hpp" #include "controller/metadata.hpp" +#include "duration.hpp" #include "md_parser.hpp" #include "libcamera/internal/v4l2_videodevice.h" @@ -72,10 +73,11 @@ public: virtual void Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata); virtual void Process(StatisticsPtr &stats, Metadata &metadata); - uint32_t ExposureLines(double exposure_us) const; - double Exposure(uint32_t exposure_lines) const; // in us - virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration, - double maxFrameDuration) const; + uint32_t ExposureLines(Duration exposure) const; + Duration Exposure(uint32_t exposure_lines) const; + virtual uint32_t GetVBlanking(Duration &exposure, + Duration minFrameDuration, + Duration maxFrameDuration) const; virtual uint32_t GainCode(double gain) const = 0; virtual double Gain(uint32_t gain_code) const = 0; virtual void GetDelays(int &exposure_delay, int &gain_delay, diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h index 256438c931d9..ab7cf2912a06 100644 --- a/src/ipa/raspberrypi/controller/camera_mode.h +++ b/src/ipa/raspberrypi/controller/camera_mode.h @@ -6,6 +6,7 @@ */ #pragma once +#include "duration.hpp" #include <libcamera/transform.h> // Description of a "camera mode", holding enough information for control @@ -33,8 +34,8 @@ struct CameraMode { double scale_x, scale_y; // scaling of the noise compared to the native sensor mode double noise_factor; - // line time in nanoseconds - double line_length; + // line time + RPiController::Duration line_length; // any camera transform *not* reflected already in the camera tuning libcamera::Transform transform; // minimum and maximum fame lengths in units of lines diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 52d91db282ea..994fb7e057a9 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -45,6 +45,7 @@ #include "denoise_algorithm.hpp" #include "denoise_status.h" #include "dpc_status.h" +#include "duration.hpp" #include "focus_status.h" #include "geq_status.h" #include "lux_status.h" @@ -55,19 +56,24 @@ namespace libcamera { +using namespace std::literals::chrono_literals; +using RPiController::Duration; +using RPiController::DurationValue; +using RPiController::operator<<; + /* Configure the sensor with these values initially. */ constexpr double DefaultAnalogueGain = 1.0; -constexpr unsigned int DefaultExposureTime = 20000; -constexpr double defaultMinFrameDuration = 1e6 / 30.0; -constexpr double defaultMaxFrameDuration = 1e6 / 0.01; +constexpr Duration DefaultExposureTime = 20.0ms; +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0; +constexpr Duration defaultMaxFrameDuration = 100.0s; /* - * Determine the minimum allowable inter-frame duration (in us) to run the - * controller algorithms. If the pipeline handler provider frames at a rate - * higher than this, we rate-limit the controller Prepare() and Process() calls - * to lower than or equal to this rate. + * Determine the minimum allowable inter-frame duration to run the controller + * algorithms. If the pipeline handler provider frames at a rate higher than this, + * we rate-limit the controller Prepare() and Process() calls to lower than or + * equal to this rate. */ -constexpr double controllerMinFrameDuration = 1e6 / 60.0; +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0; LOG_DEFINE_CATEGORY(IPARPI) @@ -111,7 +117,8 @@ private: void reportMetadata(); void fillDeviceStatus(const ControlList &sensorControls); void processStats(unsigned int bufferId); - void applyFrameDurations(double minFrameDuration, double maxFrameDuration); + void applyFrameDurations(const Duration &minFrameDuration, + const Duration &maxFrameDuration); void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls); @@ -167,9 +174,9 @@ private: /* Distinguish the first camera start from others. */ bool firstStart_; - /* Frame duration (1/fps) limits, given in microseconds. */ - double minFrameDuration_; - double maxFrameDuration_; + /* Frame duration (1/fps) limits. */ + Duration minFrameDuration_; + Duration maxFrameDuration_; }; int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) @@ -314,7 +321,7 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo) * Calculate the line length in nanoseconds as the ratio between * the line length in pixels and the pixel rate. */ - mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate; + mode_.line_length = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate); /* * Set the frame length limits for the mode to ensure exposure and @@ -387,7 +394,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo, /* Supply initial values for gain and exposure. */ ControlList ctrls(sensorCtrls_); AgcStatus agcStatus; - agcStatus.shutter_time = DefaultExposureTime; + agcStatus.shutter_time = DurationValue<std::micro>(DefaultExposureTime); agcStatus.analogue_gain = DefaultAnalogueGain; applyAGC(&agcStatus, ctrls); @@ -862,7 +869,7 @@ void IPARPi::queueRequest(const ControlList &controls) case controls::FRAME_DURATIONS: { auto frameDurations = ctrl.second.get<Span<const int64_t>>(); - applyFrameDurations(frameDurations[0], frameDurations[1]); + applyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us); break; } @@ -937,9 +944,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) returnEmbeddedBuffer(data.embeddedBufferId); /* Allow a 10% margin on the comparison below. */ - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1; + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && - frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) { + delta < controllerMinFrameDuration * 0.9) { /* * Ensure we merge the previous frame's metadata with the current * frame. This will not overwrite exposure/gain values for the @@ -1012,7 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls) int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); - deviceStatus.shutter_speed = helper_->Exposure(exposureLines); + deviceStatus.shutter_speed = DurationValue<std::micro>(helper_->Exposure(exposureLines)); deviceStatus.analogue_gain = helper_->Gain(gainCode); LOG(IPARPI, Debug) << "Metadata - Exposure : " @@ -1057,17 +1064,18 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) static_cast<int32_t>(awbStatus->gain_b * 1000)); } -void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration) +void IPARPi::applyFrameDurations(const Duration &minFrameDuration, + const Duration &maxFrameDuration) { - const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length; - const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length; + const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length; + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; /* * This will only be applied once AGC recalculations occur. * The values may be clamped based on the sensor mode capabilities as well. */ - minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration; - maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration; + minFrameDuration_ = (minFrameDuration > 0.0s) ? minFrameDuration : defaultMaxFrameDuration; + maxFrameDuration_ = (maxFrameDuration > 0.0s) ? maxFrameDuration : defaultMinFrameDuration; minFrameDuration_ = std::clamp(minFrameDuration_, minSensorFrameDuration, maxSensorFrameDuration); maxFrameDuration_ = std::clamp(maxFrameDuration_, @@ -1076,20 +1084,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio /* Return the validated limits via metadata. */ libcameraMetadata_.set(controls::FrameDurations, - { static_cast<int64_t>(minFrameDuration_), - static_cast<int64_t>(maxFrameDuration_) }); + { static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)), + static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)) }); /* * Calculate the maximum exposure time possible for the AGC to use. * GetVBlanking() will update maxShutter with the largest exposure * value possible. */ - double maxShutter = std::numeric_limits<double>::max(); + Duration maxShutter = Duration::max(); helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_); RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>( controller_.GetAlgorithm("agc")); - agc->SetMaxShutter(maxShutter); + agc->SetMaxShutter(DurationValue<std::micro>(maxShutter)); } void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) @@ -1097,9 +1105,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); /* GetVBlanking might clip exposure time to the fps limits. */ - double exposure = agcStatus->shutter_time; - int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, - maxFrameDuration_); + Duration exposure = agcStatus->shutter_time * 1.0us; + int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_); int32_t exposureLines = helper_->ExposureLines(exposure); LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
Switch the ipa and cam_helper code to use RPiController::Duration for all time based variables. This improves code readability and avoids possible errors when converting between time bases. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/cam_helper.cpp | 19 +++--- src/ipa/raspberrypi/cam_helper.hpp | 10 +-- src/ipa/raspberrypi/controller/camera_mode.h | 5 +- src/ipa/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------- 4 files changed, 56 insertions(+), 45 deletions(-)