[libcamera-devel] [PATCH] ipa: ipu3: Use libcamera::utils::Duration helper class for durations

[Jean-Michel Hautbois]

Hi Umang,

Thanks for the patch !

On 23/06/2021 13:22, Umang Jain wrote:
> std::chrono::Duration is provided quite conveniently by
> libcamera::utils::Duration wrapper. Port IPAIPU3 to use that
> for duration-type entities (such as exposure time), such that
> it becomes consistent with rest of the codebase.
> 
> The commit doesn't introduce any functional changes.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3_agc.cpp | 21 ++++++++++++---------
>  src/ipa/ipu3/ipu3_agc.h   | 14 ++++++++------
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> index 8ca95013..101ef288 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -19,6 +19,8 @@
>  
>  namespace libcamera {
>  
> +using namespace std::literals::chrono_literals;
You could also add using libcamera::utils::Duration which would then
allow to simplify the reading ?

> +
>  namespace ipa::ipu3 {
>  
>  LOG_DEFINE_CATEGORY(IPU3Agc)
> @@ -51,9 +53,9 @@ static constexpr uint8_t kCellSize = 8;
>  IPU3Agc::IPU3Agc()
>  	: frameCount_(0), lastFrame_(0), converged_(false),
>  	  updateControls_(false), iqMean_(0.0), gamma_(1.0),
> -	  lineDuration_(0.0), maxExposureTime_(0.0),
> -	  prevExposure_(0.0), prevExposureNoDg_(0.0),
> -	  currentExposure_(0.0), currentExposureNoDg_(0.0)
> +	  lineDuration_(0s), maxExposureTime_(0s),
> +	  prevExposure_(0s), prevExposureNoDg_(0s),
> +	  currentExposure_(0s), currentExposureNoDg_(0s)
>  {
>  }
>  
> @@ -61,8 +63,9 @@ void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraS
>  {
>  	aeGrid_ = bdsGrid;
>  
> -	/* line duration in microseconds */
> -	lineDuration_ = sensorInfo.lineLength * 1000000ULL / static_cast<double>(sensorInfo.pixelRate);
> +	double ld = sensorInfo.lineLength * 1000000ULL / static_cast<double>(sensorInfo.pixelRate);
> +	std::chrono::duration<double, std::micro> lineDuration(ld);
> +	lineDuration_ = lineDuration;
>  	maxExposureTime_ = kMaxExposure * lineDuration_;
>  }
>  
> @@ -113,7 +116,7 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>  void IPU3Agc::filterExposure()
>  {
>  	double speed = 0.2;
> -	if (prevExposure_ == 0.0) {
> +	if (prevExposure_ == 0s) {
>  		/* DG stands for digital gain.*/
>  		prevExposure_ = currentExposure_;
>  		prevExposureNoDg_ = currentExposureNoDg_;
> @@ -162,20 +165,20 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)
>  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
>  		/* extracted from Rpi::Agc::computeTargetExposure */
> -		double currentShutter = exposure * lineDuration_;
> +		libcamera::utils::Duration currentShutter = exposure * lineDuration_;
>  		currentExposureNoDg_ = currentShutter * gain;
>  		LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
>  				    << " Shutter speed " << currentShutter
>  				    << " Gain " << gain;
>  		currentExposure_ = currentExposureNoDg_ * newGain;
> -		double maxTotalExposure = maxExposureTime_ * kMaxGain;
> +		libcamera::utils::Duration maxTotalExposure = maxExposureTime_ * kMaxGain;
>  		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>  		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_;
>  
>  		/* \todo: estimate if we need to desaturate */
>  		filterExposure();
>  
> -		double newExposure = 0.0;
> +		libcamera::utils::Duration newExposure = 0.0s;
>  		if (currentShutter < maxExposureTime_) {
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
>  			newExposure = currentExposure_ / exposure;
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index f3d40557..de3e2dbf 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -14,6 +14,8 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libcamera/internal/utils.h"
> +
>  #include "libipa/algorithm.h"
>  
>  namespace libcamera {
> @@ -51,13 +53,13 @@ private:
>  	double iqMean_;
>  	double gamma_;
>  
> -	double lineDuration_;
> -	double maxExposureTime_;
> +	libcamera::utils::Duration lineDuration_;
> +	libcamera::utils::Duration maxExposureTime_;
>  
> -	double prevExposure_;
> -	double prevExposureNoDg_;
> -	double currentExposure_;
> -	double currentExposureNoDg_;
> +	libcamera::utils::Duration prevExposure_;
> +	libcamera::utils::Duration prevExposureNoDg_;
> +	libcamera::utils::Duration currentExposure_;
> +	libcamera::utils::Duration currentExposureNoDg_;
>  };
>  
>  } /* namespace ipa::ipu3 */
> 

This code will be removed in a short notice, but in the meantime (which
would let me rebase with this Duration usage ;-)):
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>