[libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Switch ipa/cam_helper to use RPiController::Duration
David Plowman
david.plowman at raspberrypi.com
Wed May 19 16:09:17 CEST 2021
Hi Naush
Thanks for this patch.
On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush at 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 at 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 at 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
>
More information about the libcamera-devel
mailing list