[libcamera-devel] [PATCH v4 2/4] ipa: raspberrypi: Switch ipa and cam_helper to use utils::Duration
Jacopo Mondi
jacopo at jmondi.org
Mon May 24 14:19:45 CEST 2021
Hi Naush,
On Mon, May 24, 2021 at 09:48:20AM +0100, Naushir Patuck wrote:
> Switch the ipa and cam_helper code to use libcamera::utils::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>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
> src/ipa/raspberrypi/cam_helper.cpp | 19 +++---
> src/ipa/raspberrypi/cam_helper.hpp | 12 ++--
> src/ipa/raspberrypi/controller/camera_mode.h | 6 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 64 +++++++++++---------
> 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..feb02d1806b2 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(const 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,
> + const Duration &minFrameDuration,
> + const 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 = Exposure(exposureLines).get<std::micro>();
> 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..71ee15463a71 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -15,10 +15,13 @@
> #include "controller/metadata.hpp"
> #include "md_parser.hpp"
>
> +#include "libcamera/internal/utils.h"
> #include "libcamera/internal/v4l2_videodevice.h"
>
> namespace RPiController {
>
> +using libcamera::utils::Duration;
usually, and you do the same below, we spell out the full namespace in
headers, and use 'using ...' in cpp files.
Up to you.
> +
> // The CamHelper class provides a number of facilities that anyone trying
> // to drive a camera will need to know, but which are not provided by the
> // standard driver framework. Specifically, it provides:
> @@ -72,10 +75,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(const Duration &exposure) const;
> + Duration Exposure(uint32_t exposure_lines) const;
> + virtual uint32_t GetVBlanking(Duration &exposure,
> + const Duration &minFrameDuration,
> + const 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..2aa2335dcf90 100644
> --- a/src/ipa/raspberrypi/controller/camera_mode.h
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> @@ -8,6 +8,8 @@
>
> #include <libcamera/transform.h>
>
> +#include "libcamera/internal/utils.h"
> +
> // Description of a "camera mode", holding enough information for control
> // algorithms to adapt their behaviour to the different modes of the camera,
> // including binning, scaling, cropping etc.
> @@ -33,8 +35,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
> + libcamera::utils::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..4e02e94084f7 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -55,19 +55,22 @@
>
> namespace libcamera {
>
> +using namespace std::literals::chrono_literals;
> +using utils::Duration;
> +
> /* 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 +114,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 +171,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)
> @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);
>
> /*
> - * Calculate the line length in nanoseconds as the ratio between
> - * the line length in pixels and the pixel rate.
> + * Calculate the line length as the ratio between the line length in
I would keep the 'line length in nanoseconds' as two line lengths in
mode.line_length = sensorInfo.lineLength
can be easily confused ? Although the mode.line_length type is a
duration. Would mode.line_duration be a better fit ?
> + * 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 +391,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 = DefaultExposureTime.get<std::micro>();
> agcStatus.analogue_gain = DefaultAnalogueGain;
> applyAGC(&agcStatus, ctrls);
>
> @@ -862,7 +866,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);
It would be lovely to be able to avoid the 1.0us multiplication that
turns the integers frameDurations[] into an std::chrono:duration.
I wonder if a constructor that takes an integer would make sense. It
would be rather difficult to make sure the integer passed to the
constructor is diligently specified in nanoseconds.
Or, can we make controls like frameDurations being Durations ? We can
use libcamera types in controls definition after all...
> break;
> }
>
> @@ -937,9 +941,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 +1016,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 = helper_->Exposure(exposureLines).get<std::micro>();
> deviceStatus.analogue_gain = helper_->Gain(gainCode);
>
> LOG(IPARPI, Debug) << "Metadata - Exposure : "
> @@ -1057,10 +1061,11 @@ 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.
> @@ -1076,20 +1081,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>(minFrameDuration_.get<std::micro>()),
> + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
>
> /*
> * 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(maxShutter.get<std::micro>());
> }
>
> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> @@ -1097,9 +1102,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_);
Overall, very nice introduction, Duration will be really helpful.
One additional concern for sake of discussion. Do we need to specify
the time unit ? As in DurationNs ?
Thanks
j
> int32_t exposureLines = helper_->ExposureLines(exposure);
>
> LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> --
> 2.25.1
>
More information about the libcamera-devel
mailing list