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

[Kieran Bingham]

On 23/06/2021 12:31, Jean-Michel Hautbois wrote:
> 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_;

No need to prefix with libcamera:: when you're in the libcamera
namespace already.


>>  
>> -	double prevExposure_;
>> -	double prevExposureNoDg_;
>> -	double currentExposure_;
>> -	double currentExposureNoDg_;
>> +	libcamera::utils::Duration prevExposure_;
>> +	libcamera::utils::Duration prevExposureNoDg_;
>> +	libcamera::utils::Duration currentExposure_;
>> +	libcamera::utils::Duration currentExposureNoDg_;

And same of course.

But as JM suggested, perhaps this is a case where
'using libcamera::utils::Duration'

would let us express these lines concisely as:

	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>


Sounds like this is better in sooner than later to ease the races.

I bet I'm also going to be racing here with the library splits.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>