[libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa and cam_helper to use utils::Duration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 8 03:16:58 CEST 2021
Hi Naush,
Thank you for the patch.
On Tue, May 25, 2021 at 12:42:39PM +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 | 20 +++---
> src/ipa/raspberrypi/cam_helper.hpp | 10 +--
> src/ipa/raspberrypi/controller/camera_mode.h | 6 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 64 +++++++++++---------
> 4 files changed, 55 insertions(+), 45 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 09917f3cc079..7b4331a87a42 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -18,6 +18,7 @@
>
> using namespace RPiController;
> using namespace libcamera;
> +using libcamera::utils::Duration;
I would have used utils::Duration below, but I don't mind.
>
> namespace libcamera {
> LOG_DECLARE_CATEGORY(IPARPI)
> @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
> {
> }
>
> -uint32_t CamHelper::ExposureLines(double exposure_us) const
> +uint32_t CamHelper::ExposureLines(const Duration &exposure) const
Given that the duration only stores a double value, you could pass it by
value instead of by reference. The comment applies to the whole series.
That would be my preference, but it's up to you.
Overall the code is really much nicer !
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> {
> 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 +87,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 +184,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..07481e4174a7 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -15,6 +15,7 @@
> #include "controller/metadata.hpp"
> #include "md_parser.hpp"
>
> +#include "libcamera/internal/utils.h"
> #include "libcamera/internal/v4l2_videodevice.h"
>
> namespace RPiController {
> @@ -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(const libcamera::utils::Duration &exposure) const;
> + libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;
> + virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,
> + const libcamera::utils::Duration &minFrameDuration,
> + const libcamera::utils::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
> + * 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);
> 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_);
> int32_t exposureLines = helper_->ExposureLines(exposure);
>
> LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list