[libcamera-devel] ipa: ipu3: Use libcamera::utils::Duration helper class for durations
diff mbox series

Message ID 20210623112203.33561-1-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel] ipa: ipu3: Use libcamera::utils::Duration helper class for durations
Related show

Commit Message

Umang Jain June 23, 2021, 11:22 a.m. UTC 
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@ideasonboard.com>
---
 src/ipa/ipu3/ipu3_agc.cpp | 21 ++++++++++++---------
 src/ipa/ipu3/ipu3_agc.h   | 14 ++++++++------
 2 files changed, 20 insertions(+), 15 deletions(-)

Comments

Jean-Michel Hautbois June 23, 2021, 11:31 a.m. UTC | #1 
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@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@ideasonboard.com>
Kieran Bingham June 23, 2021, 11:49 a.m. UTC | #2 
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@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@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@ideasonboard.com>
Naushir Patuck June 23, 2021, 11:57 a.m. UTC | #3 
Hi Umang,


On Wed, 23 Jun 2021 at 12:22, Umang Jain <umang.jain@ideasonboard.com>
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@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;
> +
>  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;
>

You could write this as:
lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate

and let the compiler worry about any and all conversions.

Naush



>         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 */
> --
> 2.31.1
>
>
Umang Jain June 23, 2021, 12:28 p.m. UTC | #4 
Hi Naushir,

On 6/23/21 5:27 PM, Naushir Patuck wrote:
> Hi Umang,
>
>
> On Wed, 23 Jun 2021 at 12:22, Umang Jain <umang.jain@ideasonboard.com>
> 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@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;
>> +
>>   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;
>>
> You could write this as:
> lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate
>
> and let the compiler worry about any and all conversions.


this is neat - I think I should have read more rather than assuming I 
need to convert it manually at first!

Thanks!

>
> Naush
>
>
>
>>          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 */
>> --
>> 2.31.1
>>
>>
Laurent Pinchart June 23, 2021, 12:36 p.m. UTC | #5 
Hi Umang,

On Wed, Jun 23, 2021 at 05:58:40PM +0530, Umang Jain wrote:
> On 6/23/21 5:27 PM, Naushir Patuck wrote:
> > On Wed, 23 Jun 2021 at 12: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@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;
> >> +
> >>   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;
> >>
> >
> > You could write this as:
> > lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate
> >
> > and let the compiler worry about any and all conversions.
> 
> this is neat - I think I should have read more rather than assuming I 
> need to convert it manually at first!

It's kind of the whole point of std::chrono :-) If you have to do the
conversions explicitly, there's a high chance the code isn't using
std::chrono correctly (there are of course exceptions).

> >>          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 */
Umang Jain June 23, 2021, 12:40 p.m. UTC | #6 
Hi Laurent,

On 6/23/21 6:06 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Wed, Jun 23, 2021 at 05:58:40PM +0530, Umang Jain wrote:
>> On 6/23/21 5:27 PM, Naushir Patuck wrote:
>>> On Wed, 23 Jun 2021 at 12: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@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;
>>>> +
>>>>    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;
>>>>
>>> You could write this as:
>>> lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate
>>>
>>> and let the compiler worry about any and all conversions.
>> this is neat - I think I should have read more rather than assuming I
>> need to convert it manually at first!
> It's kind of the whole point of std::chrono :-) If you have to do the
> conversions explicitly, there's a high chance the code isn't using
> std::chrono correctly (there are of course exceptions).

Yes, I got that. In my mind, I was initially thinking, once we have 
atleast one std::chrono , we can derive/operate others with that 
std::chrono entity. In this case, lineDuration_ is the base variable 
from which further calculations or entities follow.  And I just assumed, 
that the first one, we need to go from a scaler-value -> std::chrono, 
doing manual conversion.

Naushir's introduction of ( * 1.0s) is just converts everything nicely!

>>>>           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 */

Patch
diff mbox series 

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;
+
 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 */