[libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Switch ipa/cam_helper to use RPiController::Duration

Naushir Patuck naush at raspberrypi.com
Wed May 19 16:25:11 CEST 2021


Hi David,

Thank you for your review feedback.

On Wed, 19 May 2021 at 15:09, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for this patch.
>
> On Tue, 18 May 2021 at 11:09, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > Switch the ipa and cam_helper code to use RPiController::Duration for
> > all time based variables. This improves code readability and avoids
> > possible errors when converting between time bases.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp           | 19 +++---
> >  src/ipa/raspberrypi/cam_helper.hpp           | 10 +--
> >  src/ipa/raspberrypi/controller/camera_mode.h |  5 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp          | 67 +++++++++++---------
> >  4 files changed, 56 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index 09917f3cc079..e2b6c8eb8e03 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -61,20 +61,21 @@ void CamHelper::Process([[maybe_unused]]
> StatisticsPtr &stats,
> >  {
> >  }
> >
> > -uint32_t CamHelper::ExposureLines(double exposure_us) const
> > +uint32_t CamHelper::ExposureLines(Duration exposure) const
>
> I take it we're happy to pass these by value. I assume they're not
> big, just a double plus some kind of time unit?
>

There is minimal overhead, but perhaps I should change to a const
reference to be consistent.


>
> >  {
> >         assert(initialized_);
> > -       return exposure_us * 1000.0 / mode_.line_length;
> > +       return exposure / mode_.line_length;
> >  }
> >
> > -double CamHelper::Exposure(uint32_t exposure_lines) const
> > +Duration CamHelper::Exposure(uint32_t exposure_lines) const
> >  {
> >         assert(initialized_);
> > -       return exposure_lines * mode_.line_length / 1000.0;
> > +       return exposure_lines * mode_.line_length;
>
> Yes, I do prefer this!
>
> >  }
> >
> > -uint32_t CamHelper::GetVBlanking(double &exposure, double
> minFrameDuration,
> > -                                double maxFrameDuration) const
> > +uint32_t CamHelper::GetVBlanking(Duration &exposure,
> > +                                Duration minFrameDuration,
> > +                                Duration maxFrameDuration) const
> >  {
> >         uint32_t frameLengthMin, frameLengthMax, vblank;
> >         uint32_t exposureLines = ExposureLines(exposure);
> > @@ -85,8 +86,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure,
> double minFrameDuration,
> >          * minFrameDuration and maxFrameDuration are clamped by the
> caller
> >          * based on the limits for the active sensor mode.
> >          */
> > -       frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;
> > -       frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;
> > +       frameLengthMin = minFrameDuration / mode_.line_length;
> > +       frameLengthMax = maxFrameDuration / mode_.line_length;
> >
> >         /*
> >          * Limit the exposure to the maximum frame duration requested,
> and
> > @@ -182,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> >                 return;
> >         }
> >
> > -       deviceStatus.shutter_speed = Exposure(exposureLines);
> > +       deviceStatus.shutter_speed =
> DurationValue<std::micro>(Exposure(exposureLines));
>
> Ah yes, so this is one of those "in between" changes which will get
> better again in a later patch...
>

Yes, this will be reverted in the later patch.


>
> >         deviceStatus.analogue_gain = Gain(gainCode);
> >
> >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> b/src/ipa/raspberrypi/cam_helper.hpp
> > index a52f3f0b583c..a91afaa59909 100644
> > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > @@ -13,6 +13,7 @@
> >  #include "camera_mode.h"
> >  #include "controller/controller.hpp"
> >  #include "controller/metadata.hpp"
> > +#include "duration.hpp"
> >  #include "md_parser.hpp"
> >
> >  #include "libcamera/internal/v4l2_videodevice.h"
> > @@ -72,10 +73,11 @@ public:
> >         virtual void Prepare(libcamera::Span<const uint8_t> buffer,
> >                              Metadata &metadata);
> >         virtual void Process(StatisticsPtr &stats, Metadata &metadata);
> > -       uint32_t ExposureLines(double exposure_us) const;
> > -       double Exposure(uint32_t exposure_lines) const; // in us
> > -       virtual uint32_t GetVBlanking(double &exposure_us, double
> minFrameDuration,
> > -                                     double maxFrameDuration) const;
> > +       uint32_t ExposureLines(Duration exposure) const;
> > +       Duration Exposure(uint32_t exposure_lines) const;
> > +       virtual uint32_t GetVBlanking(Duration &exposure,
> > +                                     Duration minFrameDuration,
> > +                                     Duration maxFrameDuration) const;
> >         virtual uint32_t GainCode(double gain) const = 0;
> >         virtual double Gain(uint32_t gain_code) const = 0;
> >         virtual void GetDelays(int &exposure_delay, int &gain_delay,
> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h
> b/src/ipa/raspberrypi/controller/camera_mode.h
> > index 256438c931d9..ab7cf2912a06 100644
> > --- a/src/ipa/raspberrypi/controller/camera_mode.h
> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> > @@ -6,6 +6,7 @@
> >   */
> >  #pragma once
> >
> > +#include "duration.hpp"
> >  #include <libcamera/transform.h>
> >
> >  // Description of a "camera mode", holding enough information for
> control
> > @@ -33,8 +34,8 @@ struct CameraMode {
> >         double scale_x, scale_y;
> >         // scaling of the noise compared to the native sensor mode
> >         double noise_factor;
> > -       // line time in nanoseconds
> > -       double line_length;
> > +       // line time
> > +       RPiController::Duration line_length;
> >         // any camera transform *not* reflected already in the camera
> tuning
> >         libcamera::Transform transform;
> >         // minimum and maximum fame lengths in units of lines
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 52d91db282ea..994fb7e057a9 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -45,6 +45,7 @@
> >  #include "denoise_algorithm.hpp"
> >  #include "denoise_status.h"
> >  #include "dpc_status.h"
> > +#include "duration.hpp"
> >  #include "focus_status.h"
> >  #include "geq_status.h"
> >  #include "lux_status.h"
> > @@ -55,19 +56,24 @@
> >
> >  namespace libcamera {
> >
> > +using namespace std::literals::chrono_literals;
> > +using RPiController::Duration;
> > +using RPiController::DurationValue;
> > +using RPiController::operator<<;
> > +
> >  /* Configure the sensor with these values initially. */
> >  constexpr double DefaultAnalogueGain = 1.0;
> > -constexpr unsigned int DefaultExposureTime = 20000;
> > -constexpr double defaultMinFrameDuration = 1e6 / 30.0;
> > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;
> > +constexpr Duration DefaultExposureTime = 20.0ms;
> > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;
> > +constexpr Duration defaultMaxFrameDuration = 100.0s;
> >
> >  /*
> > - * Determine the minimum allowable inter-frame duration (in us) to run
> the
> > - * controller algorithms. If the pipeline handler provider frames at a
> rate
> > - * higher than this, we rate-limit the controller Prepare() and
> Process() calls
> > - * to lower than or equal to this rate.
> > + * Determine the minimum allowable inter-frame duration to run the
> controller
> > + * algorithms. If the pipeline handler provider frames at a rate higher
> than this,
> > + * we rate-limit the controller Prepare() and Process() calls to lower
> than or
> > + * equal to this rate.
> >   */
> > -constexpr double controllerMinFrameDuration = 1e6 / 60.0;
> > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;
> >
> >  LOG_DEFINE_CATEGORY(IPARPI)
> >
> > @@ -111,7 +117,8 @@ private:
> >         void reportMetadata();
> >         void fillDeviceStatus(const ControlList &sensorControls);
> >         void processStats(unsigned int bufferId);
> > -       void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
> > +       void applyFrameDurations(const Duration &minFrameDuration,
> > +                                const Duration &maxFrameDuration);
>
> Some pass-by-reference, I notice. Do we want to be consistent? Are we
> bothered (I'm not...)?
>

I will change to using const references in the other places to be
consistent.


>
> >         void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> >         void applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls);
> >         void applyDG(const struct AgcStatus *dgStatus, ControlList
> &ctrls);
> > @@ -167,9 +174,9 @@ private:
> >         /* Distinguish the first camera start from others. */
> >         bool firstStart_;
> >
> > -       /* Frame duration (1/fps) limits, given in microseconds. */
> > -       double minFrameDuration_;
> > -       double maxFrameDuration_;
> > +       /* Frame duration (1/fps) limits. */
> > +       Duration minFrameDuration_;
> > +       Duration maxFrameDuration_;
> >  };
> >
> >  int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig
> *sensorConfig)
> > @@ -314,7 +321,7 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
> >          * Calculate the line length in nanoseconds as the ratio between
>
> Elsewhere you've tended to remove the mention of units, so does it
> want the same here?
>

Oops, that one I missed.


>
> >          * the line length in pixels and the pixel rate.
> >          */
> > -       mode_.line_length = 1e9 * sensorInfo.lineLength /
> sensorInfo.pixelRate;
> > +       mode_.line_length = sensorInfo.lineLength * (1.0s /
> sensorInfo.pixelRate);
> >
> >         /*
> >          * Set the frame length limits for the mode to ensure exposure
> and
> > @@ -387,7 +394,7 @@ int IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >                 /* Supply initial values for gain and exposure. */
> >                 ControlList ctrls(sensorCtrls_);
> >                 AgcStatus agcStatus;
> > -               agcStatus.shutter_time = DefaultExposureTime;
> > +               agcStatus.shutter_time =
> DurationValue<std::micro>(DefaultExposureTime);
> >                 agcStatus.analogue_gain = DefaultAnalogueGain;
> >                 applyAGC(&agcStatus, ctrls);
> >
> > @@ -862,7 +869,7 @@ void IPARPi::queueRequest(const ControlList
> &controls)
> >
> >                 case controls::FRAME_DURATIONS: {
> >                         auto frameDurations = ctrl.second.get<Span<const
> int64_t>>();
> > -                       applyFrameDurations(frameDurations[0],
> frameDurations[1]);
> > +                       applyFrameDurations(frameDurations[0] * 1.0us,
> frameDurations[1] * 1.0us);
> >                         break;
> >                 }
> >
> > @@ -937,9 +944,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig
> &data)
> >                 returnEmbeddedBuffer(data.embeddedBufferId);
> >
> >         /* Allow a 10% margin on the comparison below. */
> > -       constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;
> > +       Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> >         if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > -           frameTimestamp - lastRunTimestamp_ + eps <
> controllerMinFrameDuration * 1e3) {
> > +           delta < controllerMinFrameDuration * 0.9) {
> >                 /*
> >                  * Ensure we merge the previous frame's metadata with
> the current
> >                  * frame. This will not overwrite exposure/gain values
> for the
> > @@ -1012,7 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls)
> >         int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >         int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >
> > -       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > +       deviceStatus.shutter_speed =
> DurationValue<std::micro>(helper_->Exposure(exposureLines));
> >         deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >
> >         LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > @@ -1057,17 +1064,18 @@ void IPARPi::applyAWB(const struct AwbStatus
> *awbStatus, ControlList &ctrls)
> >                   static_cast<int32_t>(awbStatus->gain_b * 1000));
> >  }
> >
> > -void IPARPi::applyFrameDurations(double minFrameDuration, double
> maxFrameDuration)
> > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,
> > +                                const Duration &maxFrameDuration)
> >  {
> > -       const double minSensorFrameDuration = 1e-3 *
> mode_.min_frame_length * mode_.line_length;
> > -       const double maxSensorFrameDuration = 1e-3 *
> mode_.max_frame_length * mode_.line_length;
> > +       const Duration minSensorFrameDuration = mode_.min_frame_length *
> mode_.line_length;
> > +       const Duration maxSensorFrameDuration = mode_.max_frame_length *
> mode_.line_length;
> >
> >         /*
> >          * This will only be applied once AGC recalculations occur.
> >          * The values may be clamped based on the sensor mode
> capabilities as well.
> >          */
> > -       minFrameDuration_ = minFrameDuration ? minFrameDuration :
> defaultMaxFrameDuration;
> > -       maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :
> defaultMinFrameDuration;
> > +       minFrameDuration_ = (minFrameDuration > 0.0s) ? minFrameDuration
> : defaultMaxFrameDuration;
> > +       maxFrameDuration_ = (maxFrameDuration > 0.0s) ? maxFrameDuration
> : defaultMinFrameDuration;
> >         minFrameDuration_ = std::clamp(minFrameDuration_,
> >                                        minSensorFrameDuration,
> maxSensorFrameDuration);
> >         maxFrameDuration_ = std::clamp(maxFrameDuration_,
> > @@ -1076,20 +1084,20 @@ void IPARPi::applyFrameDurations(double
> minFrameDuration, double maxFrameDuratio
> >
> >         /* Return the validated limits via metadata. */
> >         libcameraMetadata_.set(controls::FrameDurations,
> > -                              { static_cast<int64_t>(minFrameDuration_),
> > -                                static_cast<int64_t>(maxFrameDuration_)
> });
> > +                              {
> static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)),
> > +
> static_cast<int64_t>(DurationValue<std::micro>(minFrameDuration_)) });
>
> And this will look nicer again once the whole of libcamera adopts
> std::chrono! :)
>

Yes, but that comes later :-)


>
> So no significant comments really from me, thus:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks
> David
>
> >
> >         /*
> >          * Calculate the maximum exposure time possible for the AGC to
> use.
> >          * GetVBlanking() will update maxShutter with the largest
> exposure
> >          * value possible.
> >          */
> > -       double maxShutter = std::numeric_limits<double>::max();
> > +       Duration maxShutter = Duration::max();
> >         helper_->GetVBlanking(maxShutter, minFrameDuration_,
> maxFrameDuration_);
> >
> >         RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> >                 controller_.GetAlgorithm("agc"));
> > -       agc->SetMaxShutter(maxShutter);
> > +       agc->SetMaxShutter(DurationValue<std::micro>(maxShutter));
> >  }
> >
> >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls)
> > @@ -1097,9 +1105,8 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> >         int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> >
> >         /* GetVBlanking might clip exposure time to the fps limits. */
> > -       double exposure = agcStatus->shutter_time;
> > -       int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_,
> > -                                                 maxFrameDuration_);
> > +       Duration exposure = agcStatus->shutter_time * 1.0us;
> > +       int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
> >         int32_t exposureLines = helper_->ExposureLines(exposure);
> >
> >         LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210519/3c1500a4/attachment-0001.htm>


More information about the libcamera-devel mailing list