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

[Laurent Pinchart]

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 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!

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

-- 
Regards,

Laurent Pinchart