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

[Umang Jain]

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 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).

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