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

[Umang Jain]

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 at 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 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;
>> +
>>   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
>>
>>