[libcamera-devel,v6,2/4] ipa: raspberrypi: Switch ipa and cam_helper to use utils::Duration

12516 diff mbox series

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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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          | 62 ++++++++++----------
 4 files changed, 53 insertions(+), 45 deletions(-)

Hi Naush,

On Tue, Jun 08, 2021 at 12:03:33PM +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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
>  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          | 62 ++++++++++----------
>  4 files changed, 53 insertions(+), 45 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 09917f3cc079..92a38007f038 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;
>
>  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
>  {
>  	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 +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..f53f5c39b01c 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(libcamera::utils::Duration exposure) const;
> +	libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;
> +	virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,
> +				      libcamera::utils::Duration minFrameDuration,
> +				      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 0c4752ecffb6..33b316b90d16 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -54,19 +54,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)
>
> @@ -110,7 +113,7 @@ private:
>  	void reportMetadata();
>  	void fillDeviceStatus(const ControlList &sensorControls);
>  	void processStats(unsigned int bufferId);
> -	void applyFrameDurations(double minFrameDuration, double maxFrameDuration);
> +	void applyFrameDurations(Duration minFrameDuration, 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);
> @@ -166,9 +169,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)
> @@ -310,10 +313,10 @@ void IPARPi::setMode(const IPACameraSensorInfo &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
> @@ -386,7 +389,7 @@ int IPARPi::configure(const IPACameraSensorInfo &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);
>
> @@ -861,7 +864,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>
>  		case controls::FRAME_DURATION_LIMITS: {
>  			auto frameDurations = ctrl.second.get<Span<const int64_t>>();
> -			applyFrameDurations(frameDurations[0], frameDurations[1]);
> +			applyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us);
>  			break;
>  		}
>
> @@ -936,9 +939,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
> @@ -1011,7 +1014,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 : "
> @@ -1056,10 +1059,10 @@ 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(Duration minFrameDuration, 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.
> @@ -1075,20 +1078,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio
>
>  	/* Return the validated limits via metadata. */
>  	libcameraMetadata_.set(controls::FrameDurationLimits,
> -			       { 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)
> @@ -1096,9 +1099,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
>