<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for the review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 24 May 2021 at 13:19, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Mon, May 24, 2021 at 09:48:20AM +0100, Naushir Patuck wrote:<br>
> Switch the ipa and cam_helper code to use libcamera::utils::Duration for<br>
> all time based variables. This improves code readability and avoids<br>
> possible errors when converting between time bases.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/cam_helper.cpp  Â  Â  Â  Â  Â | 19 +++---<br>
>  src/ipa/raspberrypi/cam_helper.hpp  Â  Â  Â  Â  Â | 12 ++--<br>
>  src/ipa/raspberrypi/controller/camera_mode.h |  6 +-<br>
>  src/ipa/raspberrypi/raspberrypi.cpp  Â  Â  Â  Â  | 64 +++++++++++---------<br>
>  4 files changed, 56 insertions(+), 45 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index 09917f3cc079..feb02d1806b2 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -61,20 +61,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,<br>
>  {<br>
>  }<br>
><br>
> -uint32_t CamHelper::ExposureLines(double exposure_us) const<br>
> +uint32_t CamHelper::ExposureLines(const Duration &exposure) const<br>
>  {<br>
>  Â  Â  Â assert(initialized_);<br>
> -  Â  Â return exposure_us * 1000.0 / mode_.line_length;<br>
> +  Â  Â return exposure / mode_.line_length;<br>
>  }<br>
><br>
> -double CamHelper::Exposure(uint32_t exposure_lines) const<br>
> +Duration CamHelper::Exposure(uint32_t exposure_lines) const<br>
>  {<br>
>  Â  Â  Â assert(initialized_);<br>
> -  Â  Â return exposure_lines * mode_.line_length / 1000.0;<br>
> +  Â  Â return exposure_lines * mode_.line_length;<br>
>  }<br>
><br>
> -uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  double maxFrameDuration) const<br>
> +uint32_t CamHelper::GetVBlanking(Duration &exposure,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const Duration &minFrameDuration,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const Duration &maxFrameDuration) const<br>
>  {<br>
>  Â  Â  Â uint32_t frameLengthMin, frameLengthMax, vblank;<br>
>  Â  Â  Â uint32_t exposureLines = ExposureLines(exposure);<br>
> @@ -85,8 +86,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,<br>
>  Â  Â  Â  * minFrameDuration and maxFrameDuration are clamped by the caller<br>
>  Â  Â  Â  * based on the limits for the active sensor mode.<br>
>  Â  Â  Â  */<br>
> -  Â  Â frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;<br>
> -  Â  Â frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;<br>
> +  Â  Â frameLengthMin = minFrameDuration / mode_.line_length;<br>
> +  Â  Â frameLengthMax = maxFrameDuration / mode_.line_length;<br>
><br>
>  Â  Â  Â /*<br>
>  Â  Â  Â  * Limit the exposure to the maximum frame duration requested, and<br>
> @@ -182,7 +183,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,<br>
>  Â  Â  Â  Â  Â  Â  Â return;<br>
>  Â  Â  Â }<br>
><br>
> -  Â  Â deviceStatus.shutter_speed = Exposure(exposureLines);<br>
> +  Â  Â deviceStatus.shutter_speed = Exposure(exposureLines).get<std::micro>();<br>
>  Â  Â  Â deviceStatus.analogue_gain = Gain(gainCode);<br>
><br>
>  Â  Â  Â LOG(IPARPI, Debug) << "Metadata updated - Exposure : "<br>
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp<br>
> index a52f3f0b583c..71ee15463a71 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> @@ -15,10 +15,13 @@<br>
>  #include "controller/metadata.hpp"<br>
>  #include "md_parser.hpp"<br>
><br>
> +#include "libcamera/internal/utils.h"<br>
>  #include "libcamera/internal/v4l2_videodevice.h"<br>
><br>
>  namespace RPiController {<br>
><br>
> +using libcamera::utils::Duration;<br>
<br>
usually, and you do the same below, we spell out the full namespace in<br>
headers, and use 'using ...' in cpp files.<br>
<br>
Up to you.<br></blockquote><div><br></div><div>Agreed, to be consistent, I will remove the using... and expand the namespace</div><div>below.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
>  // The CamHelper class provides a number of facilities that anyone trying<br>
>  // to drive a camera will need to know, but which are not provided by the<br>
>  // standard driver framework. Specifically, it provides:<br>
> @@ -72,10 +75,11 @@ public:<br>
>  Â  Â  Â virtual void Prepare(libcamera::Span<const uint8_t> buffer,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Metadata &metadata);<br>
>  Â  Â  Â virtual void Process(StatisticsPtr &stats, Metadata &metadata);<br>
> -  Â  Â uint32_t ExposureLines(double exposure_us) const;<br>
> -  Â  Â double Exposure(uint32_t exposure_lines) const; // in us<br>
> -  Â  Â virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â double maxFrameDuration) const;<br>
> +  Â  Â uint32_t ExposureLines(const Duration &exposure) const;<br>
> +  Â  Â Duration Exposure(uint32_t exposure_lines) const;<br>
> +  Â  Â virtual uint32_t GetVBlanking(Duration &exposure,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â const Duration &minFrameDuration,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â const Duration &maxFrameDuration) const;<br>
>  Â  Â  Â virtual uint32_t GainCode(double gain) const = 0;<br>
>  Â  Â  Â virtual double Gain(uint32_t gain_code) const = 0;<br>
>  Â  Â  Â virtual void GetDelays(int &exposure_delay, int &gain_delay,<br>
> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> index 256438c931d9..2aa2335dcf90 100644<br>
> --- a/src/ipa/raspberrypi/controller/camera_mode.h<br>
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> @@ -8,6 +8,8 @@<br>
><br>
>  #include <libcamera/transform.h><br>
><br>
> +#include "libcamera/internal/utils.h"<br>
> +<br>
>  // Description of a "camera mode", holding enough information for control<br>
>  // algorithms to adapt their behaviour to the different modes of the camera,<br>
>  // including binning, scaling, cropping etc.<br>
> @@ -33,8 +35,8 @@ struct CameraMode {<br>
>  Â  Â  Â double scale_x, scale_y;<br>
>  Â  Â  Â // scaling of the noise compared to the native sensor mode<br>
>  Â  Â  Â double noise_factor;<br>
> -  Â  Â // line time in nanoseconds<br>
> -  Â  Â double line_length;<br>
> +  Â  Â // line time<br>
> +  Â  Â libcamera::utils::Duration line_length;<br>
>  Â  Â  Â // any camera transform *not* reflected already in the camera tuning<br>
>  Â  Â  Â libcamera::Transform transform;<br>
>  Â  Â  Â // minimum and maximum fame lengths in units of lines<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 52d91db282ea..4e02e94084f7 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -55,19 +55,22 @@<br>
><br>
>  namespace libcamera {<br>
><br>
> +using namespace std::literals::chrono_literals;<br>
> +using utils::Duration;<br>
> +<br>
>  /* Configure the sensor with these values initially. */<br>
>  constexpr double DefaultAnalogueGain = 1.0;<br>
> -constexpr unsigned int DefaultExposureTime = 20000;<br>
> -constexpr double defaultMinFrameDuration = 1e6 / 30.0;<br>
> -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;<br>
> +constexpr Duration DefaultExposureTime = 20.0ms;<br>
> +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;<br>
> +constexpr Duration defaultMaxFrameDuration = 100.0s;<br>
><br>
>  /*<br>
> - * Determine the minimum allowable inter-frame duration (in us) to run the<br>
> - * controller algorithms. If the pipeline handler provider frames at a rate<br>
> - * higher than this, we rate-limit the controller Prepare() and Process() calls<br>
> - * to lower than or equal to this rate.<br>
> + * Determine the minimum allowable inter-frame duration to run the controller<br>
> + * algorithms. If the pipeline handler provider frames at a rate higher than this,<br>
> + * we rate-limit the controller Prepare() and Process() calls to lower than or<br>
> + * equal to this rate.<br>
>  Â */<br>
> -constexpr double controllerMinFrameDuration = 1e6 / 60.0;<br>
> +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;<br>
><br>
>  LOG_DEFINE_CATEGORY(IPARPI)<br>
><br>
> @@ -111,7 +114,8 @@ private:<br>
>  Â  Â  Â void reportMetadata();<br>
>  Â  Â  Â void fillDeviceStatus(const ControlList &sensorControls);<br>
>  Â  Â  Â void processStats(unsigned int bufferId);<br>
> -  Â  Â void applyFrameDurations(double minFrameDuration, double maxFrameDuration);<br>
> +  Â  Â void applyFrameDurations(const Duration &minFrameDuration,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const Duration &maxFrameDuration);<br>
>  Â  Â  Â void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);<br>
>  Â  Â  Â void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);<br>
>  Â  Â  Â void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);<br>
> @@ -167,9 +171,9 @@ private:<br>
>  Â  Â  Â /* Distinguish the first camera start from others. */<br>
>  Â  Â  Â bool firstStart_;<br>
><br>
> -  Â  Â /* Frame duration (1/fps) limits, given in microseconds. */<br>
> -  Â  Â double minFrameDuration_;<br>
> -  Â  Â double maxFrameDuration_;<br>
> +  Â  Â /* Frame duration (1/fps) limits. */<br>
> +  Â  Â Duration minFrameDuration_;<br>
> +  Â  Â Duration maxFrameDuration_;<br>
>  };<br>
><br>
>  int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)<br>
> @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)<br>
>  Â  Â  Â mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);<br>
><br>
>  Â  Â  Â /*<br>
> -  Â  Â  * Calculate the line length in nanoseconds as the ratio between<br>
> -  Â  Â  * the line length in pixels and the pixel rate.<br>
> +  Â  Â  * Calculate the line length as the ratio between the line length in<br>
<br>
I would keep the 'line length in nanoseconds' as two line lengths in<br>
<br>
  Â  Â  Â  mode.line_length = sensorInfo.lineLength<br>
<br>
can be easily confused ? Although the mode.line_length type is a<br>
duration. Would mode.line_duration be a better fit ?<br></blockquote><div><br></div><div>I see your point here.  However, as you pointed out mode.line_length is a Duration type, and</div><div>I did not want to specify a time unit in the comment, as Duration hides all of that for you.</div><div>Perhaps I should update the comment like:</div><div><br></div>s/Calculate the line length as the ratio/Calculate the line length duration as the ratio/</div><div class="gmail_quote"><br></div><div class="gmail_quote">to be more explicit?  I would prefer not to rename mode.line_length if at all possible, as that</div><div class="gmail_quote">is (afaik) standard sensor device terminology.</div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> +  Â  Â  * pixels and the pixel rate.<br>
>  Â  Â  Â  */<br>
> -  Â  Â mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;<br>
> +  Â  Â mode_.line_length = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);<br>
><br>
>  Â  Â  Â /*<br>
>  Â  Â  Â  * Set the frame length limits for the mode to ensure exposure and<br>
> @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>  Â  Â  Â  Â  Â  Â  Â /* Supply initial values for gain and exposure. */<br>
>  Â  Â  Â  Â  Â  Â  Â ControlList ctrls(sensorCtrls_);<br>
>  Â  Â  Â  Â  Â  Â  Â AgcStatus agcStatus;<br>
> -  Â  Â  Â  Â  Â  Â agcStatus.shutter_time = DefaultExposureTime;<br>
> +  Â  Â  Â  Â  Â  Â agcStatus.shutter_time = DefaultExposureTime.get<std::micro>();<br>
>  Â  Â  Â  Â  Â  Â  Â agcStatus.analogue_gain = DefaultAnalogueGain;<br>
>  Â  Â  Â  Â  Â  Â  Â applyAGC(&agcStatus, ctrls);<br>
><br>
> @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â case controls::FRAME_DURATIONS: {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â auto frameDurations = ctrl.second.get<Span<const int64_t>>();<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â applyFrameDurations(frameDurations[0], frameDurations[1]);<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â applyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us);<br>
<br>
It would be lovely to be able to avoid the 1.0us multiplication that<br>
turns the integers frameDurations[] into an std::chrono:duration.<br></blockquote><div><br></div><div>The alternative to this is std::chrono::microseconds(frameDuration[0]).</div><div>I have a slight preference using "1.0us" over that though.  If you feel</div><div>it is better, I am happy to change.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
I wonder if a constructor that takes an integer would make sense. It<br>
would be rather difficult to make sure the integer passed to the<br>
constructor is diligently specified in nanoseconds.<br></blockquote><div><br></div><div>This was my first thought as well, but it becomes a bit ambiguous.  It required</div><div>the user to know that Duration is a nanosecond base, and frameDuration</div><div>is a microsecond base, and we must multiply by 1e3, and that becomes</div><div>error-prone.</div><div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Or, can we make controls like frameDurations being Durations ? We can<br>
use libcamera types in controls definition after all...<br></blockquote><div><br></div><div>I think this should certainly be the end goal of this work!  When all code starts to</div><div>use Duration, none of these integer -> Duration conversion (apart from the start/end</div><div>of the chain of course) will be needed.</div><div><br></div><div>I was a bit afraid of doing the wholesale change across all libcamera without some feedback.</div><div>We can use the Raspberry Pi source code as a test-bed in the short term :-)</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
>  Â  Â  Â  Â  Â  Â  Â }<br>
><br>
> @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
>  Â  Â  Â  Â  Â  Â  Â returnEmbeddedBuffer(data.embeddedBufferId);<br>
><br>
>  Â  Â  Â /* Allow a 10% margin on the comparison below. */<br>
> -  Â  Â constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;<br>
> +  Â  Â Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;<br>
>  Â  Â  Â if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&<br>
> -  Â  Â  Â  Â frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {<br>
> +  Â  Â  Â  Â delta < controllerMinFrameDuration * 0.9) {<br>
>  Â  Â  Â  Â  Â  Â  Â /*<br>
>  Â  Â  Â  Â  Â  Â  Â  * Ensure we merge the previous frame's metadata with the current<br>
>  Â  Â  Â  Â  Â  Â  Â  * frame. This will not overwrite exposure/gain values for the<br>
> @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)<br>
>  Â  Â  Â int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
>  Â  Â  Â int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
><br>
> -  Â  Â deviceStatus.shutter_speed = helper_->Exposure(exposureLines);<br>
> +  Â  Â deviceStatus.shutter_speed = helper_->Exposure(exposureLines).get<std::micro>();<br>
>  Â  Â  Â deviceStatus.analogue_gain = helper_->Gain(gainCode);<br>
><br>
>  Â  Â  Â LOG(IPARPI, Debug) << "Metadata - Exposure : "<br>
> @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â static_cast<int32_t>(awbStatus->gain_b * 1000));<br>
>  }<br>
><br>
> -void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)<br>
> +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const Duration &maxFrameDuration)<br>
>  {<br>
> -  Â  Â const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length;<br>
> -  Â  Â const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length;<br>
> +  Â  Â const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;<br>
> +  Â  Â const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;<br>
><br>
>  Â  Â  Â /*<br>
>  Â  Â  Â  * This will only be applied once AGC recalculations occur.<br>
> @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio<br>
><br>
>  Â  Â  Â /* Return the validated limits via metadata. */<br>
>  Â  Â  Â libcameraMetadata_.set(controls::FrameDurations,<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  { static_cast<int64_t>(minFrameDuration_),<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  static_cast<int64_t>(maxFrameDuration_) });<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });<br>
><br>
>  Â  Â  Â /*<br>
>  Â  Â  Â  * Calculate the maximum exposure time possible for the AGC to use.<br>
>  Â  Â  Â  * GetVBlanking() will update maxShutter with the largest exposure<br>
>  Â  Â  Â  * value possible.<br>
>  Â  Â  Â  */<br>
> -  Â  Â double maxShutter = std::numeric_limits<double>::max();<br>
> +  Â  Â Duration maxShutter = Duration::max();<br>
>  Â  Â  Â helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);<br>
><br>
>  Â  Â  Â RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
>  Â  Â  Â  Â  Â  Â  Â controller_.GetAlgorithm("agc"));<br>
> -  Â  Â agc->SetMaxShutter(maxShutter);<br>
> +  Â  Â agc->SetMaxShutter(maxShutter.get<std::micro>());<br>
>  }<br>
><br>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
>  Â  Â  Â int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);<br>
><br>
>  Â  Â  Â /* GetVBlanking might clip exposure time to the fps limits. */<br>
> -  Â  Â double exposure = agcStatus->shutter_time;<br>
> -  Â  Â int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â maxFrameDuration_);<br>
> +  Â  Â Duration exposure = agcStatus->shutter_time * 1.0us;<br>
> +  Â  Â int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);<br>
<br>
Overall, very nice introduction, Duration will be really helpful.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
One additional concern for sake of discussion. Do we need to specify<br>
the time unit ? As in DurationNs ?<br></blockquote><div><br></div><div>My preference would be not to put a unit to Duration.  The idea is to ensure the user (as much</div><div>as is possible) does not worry about the internal representation, and let the compiler worry</div><div>about it.  I don't think there would be any scenario where having us know that Duration is</div><div>stored as ns would be helpful, since we have a Duration::get<T> to do any conversions needed.</div><div><br></div><div>Regards,</div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks<br>
  j<br>
<br>
>  Â  Â  Â int32_t exposureLines = helper_->ExposureLines(exposure);<br>
><br>
>  Â  Â  Â LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>