<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 29 Jan 2021 at 09:51, 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>
  Â I'm not expert of this part, so I trust your and David's review<br>
acks<br>
<br>
On Thu, Jan 28, 2021 at 09:10:49AM +0000, Naushir Patuck wrote:<br>
> In order to provide an optimal split between shutter speed and gain, the<br>
> AGC must know the maximum allowable shutter speed, as limited by the<br>
> maximum frame duration (either application provided or the default).<br>
><br>
> Add a new API function, SetMaxShutter, to the AgcAlgorithm class. The<br>
> IPA provides the the maximum shutter speed for AGC calculations.<br>
> This applies to both the manual and auto AGC modes.<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>
>  .../raspberrypi/controller/agc_algorithm.hpp  |  1 +<br>
>  src/ipa/raspberrypi/controller/rpi/agc.cpp  Â  | 48 +++++++++++++------<br>
>  src/ipa/raspberrypi/controller/rpi/agc.hpp  Â  |  3 ++<br>
>  src/ipa/raspberrypi/raspberrypi.cpp  Â  Â  Â  Â  Â | 12 +++++<br>
>  4 files changed, 49 insertions(+), 15 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp<br>
> index 981f1de2f0e4..f97deb0fca59 100644<br>
> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp<br>
> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp<br>
> @@ -19,6 +19,7 @@ public:<br>
>  Â  Â  Â virtual void SetEv(double ev) = 0;<br>
>  Â  Â  Â virtual void SetFlickerPeriod(double flicker_period) = 0;<br>
>  Â  Â  Â virtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds<br>
> +  Â  Â virtual void SetMaxShutter(double max_shutter) = 0; // microseconds<br>
>  Â  Â  Â virtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;<br>
>  Â  Â  Â virtual void SetMeteringMode(std::string const &metering_mode_name) = 0;<br>
>  Â  Â  Â virtual void SetExposureMode(std::string const &exposure_mode_name) = 0;<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> index eddd1684da12..0023d50029f1 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> @@ -157,7 +157,7 @@ Agc::Agc(Controller *controller)<br>
>  Â  Â  Â  Â frame_count_(0), lock_count_(0),<br>
>  Â  Â  Â  Â last_target_exposure_(0.0),<br>
>  Â  Â  Â  Â ev_(1.0), flicker_period_(0.0),<br>
> -  Â  Â  Â fixed_shutter_(0), fixed_analogue_gain_(0.0)<br>
> +  Â  Â  Â max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)<br>
>  {<br>
>  Â  Â  Â memset(&awb_, 0, sizeof(awb_));<br>
>  Â  Â  Â // Setting status_.total_exposure_value_ to zero initially tells us<br>
> @@ -227,11 +227,16 @@ void Agc::SetFlickerPeriod(double flicker_period)<br>
>  Â  Â  Â flicker_period_ = flicker_period;<br>
>  }<br>
><br>
> +void Agc::SetMaxShutter(double max_shutter)<br>
> +{<br>
> +  Â  Â max_shutter_ = max_shutter;<br>
> +}<br>
> +<br>
>  void Agc::SetFixedShutter(double fixed_shutter)<br>
>  {<br>
>  Â  Â  Â fixed_shutter_ = fixed_shutter;<br>
>  Â  Â  Â // Set this in case someone calls Pause() straight after.<br>
> -  Â  Â status_.shutter_time = fixed_shutter;<br>
> +  Â  Â status_.shutter_time = clipShutter(fixed_shutter_);<br>
>  }<br>
><br>
>  void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)<br>
> @@ -261,7 +266,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,<br>
>  {<br>
>  Â  Â  Â housekeepConfig();<br>
><br>
> -  Â  Â if (fixed_shutter_ != 0.0 && fixed_analogue_gain_ != 0.0) {<br>
> +  Â  Â double fixed_shutter = clipShutter(fixed_shutter_);<br>
> +  Â  Â if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {<br>
>  Â  Â  Â  Â  Â  Â  Â // We're going to reset the algorithm here with these fixed values.<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â fetchAwbStatus(metadata);<br>
> @@ -269,14 +275,14 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,<br>
>  Â  Â  Â  Â  Â  Â  Â ASSERT(min_colour_gain != 0.0);<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â // This is the equivalent of computeTargetExposure and applyDigitalGain.<br>
> -  Â  Â  Â  Â  Â  Â target_.total_exposure_no_dg = fixed_shutter_ * fixed_analogue_gain_;<br>
> +  Â  Â  Â  Â  Â  Â target_.total_exposure_no_dg = fixed_shutter * fixed_analogue_gain_;<br>
>  Â  Â  Â  Â  Â  Â  Â target_.total_exposure = target_.total_exposure_no_dg / min_colour_gain;<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â // Equivalent of filterExposure. This resets any "history".<br>
>  Â  Â  Â  Â  Â  Â  Â filtered_ = target_;<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â // Equivalent of divideUpExposure.<br>
> -  Â  Â  Â  Â  Â  Â filtered_.shutter = fixed_shutter_;<br>
> +  Â  Â  Â  Â  Â  Â filtered_.shutter = fixed_shutter;<br>
>  Â  Â  Â  Â  Â  Â  Â filtered_.analogue_gain = fixed_analogue_gain_;<br>
>  Â  Â  Â } else if (status_.total_exposure_value) {<br>
>  Â  Â  Â  Â  Â  Â  Â // On a mode switch, it's possible the exposure profile could change,<br>
> @@ -290,7 +296,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,<br>
>  Â  Â  Â  Â  Â  Â  Â // for any that weren't set.<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â // Equivalent of divideUpExposure.<br>
> -  Â  Â  Â  Â  Â  Â filtered_.shutter = fixed_shutter_ ? fixed_shutter_ : config_.default_exposure_time;<br>
> +  Â  Â  Â  Â  Â  Â filtered_.shutter = fixed_shutter ? fixed_shutter : config_.default_exposure_time;<br>
>  Â  Â  Â  Â  Â  Â  Â filtered_.analogue_gain = fixed_analogue_gain_ ? fixed_analogue_gain_ : config_.default_analogue_gain;<br>
>  Â  Â  Â }<br>
><br>
> @@ -403,7 +409,8 @@ void Agc::housekeepConfig()<br>
>  {<br>
>  Â  Â  Â // First fetch all the up-to-date settings, so no one else has to do it.<br>
>  Â  Â  Â status_.ev = ev_;<br>
> -  Â  Â status_.fixed_shutter = fixed_shutter_;<br>
> +  Â  Â double fixed_shutter = clipShutter(fixed_shutter_);<br>
> +  Â  Â status_.fixed_shutter = fixed_shutter;<br>
>  Â  Â  Â status_.fixed_analogue_gain = fixed_analogue_gain_;<br>
>  Â  Â  Â status_.flicker_period = flicker_period_;<br>
>  Â  Â  Â LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixed_shutter "<br>
> @@ -582,13 +589,15 @@ void Agc::computeTargetExposure(double gain)<br>
>  Â  Â  Â  Â  Â  Â  Â target_.total_exposure = current_.total_exposure_no_dg * gain;<br>
>  Â  Â  Â  Â  Â  Â  Â // The final target exposure is also limited to what the exposure<br>
>  Â  Â  Â  Â  Â  Â  Â // mode allows.<br>
> +  Â  Â  Â  Â  Â  Â double max_shutter = status_.fixed_shutter != 0.0<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  ? status_.fixed_shutter<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  : exposure_mode_->shutter.back();<br>
<br>
Nit: I would align ? below =, unless checkstyle says otherwise.<br></blockquote><div><br></div><div>Ack.</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>
> +  Â  Â  Â  Â  Â  Â max_shutter = clipShutter(max_shutter);<br>
>  Â  Â  Â  Â  Â  Â  Â double max_total_exposure =<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â (status_.fixed_shutter != 0.0<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  ? status_.fixed_shutter<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  : exposure_mode_->shutter.back()) *<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â max_shutter *<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â (status_.fixed_analogue_gain != 0.0<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  ? status_.fixed_analogue_gain<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  : exposure_mode_->gain.back());<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  ? status_.fixed_analogue_gain<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  : exposure_mode_->gain.back());<br>
>  Â  Â  Â  Â  Â  Â  Â target_.total_exposure = std::min(target_.total_exposure,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â max_total_exposure);<br>
>  Â  Â  Â }<br>
> @@ -671,6 +680,7 @@ void Agc::divideUpExposure()<br>
>  Â  Â  Â shutter_time = status_.fixed_shutter != 0.0<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  ? status_.fixed_shutter<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  : exposure_mode_->shutter[0];<br>
> +  Â  Â shutter_time = clipShutter(shutter_time);<br>
>  Â  Â  Â analogue_gain = status_.fixed_analogue_gain != 0.0<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â ? status_.fixed_analogue_gain<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â : exposure_mode_->gain[0];<br>
> @@ -678,14 +688,15 @@ void Agc::divideUpExposure()<br>
>  Â  Â  Â  Â  Â  Â  Â for (unsigned int stage = 1;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  stage < exposure_mode_->gain.size(); stage++) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (status_.fixed_shutter == 0.0) {<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (exposure_mode_->shutter[stage] *<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â analogue_gain >=<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â double stage_shutter =<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â clipShutter(exposure_mode_->shutter[stage]);<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (stage_shutter * analogue_gain >=<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â exposure_value) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â shutter_time =<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â exposure_value / analogue_gain;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â shutter_time = exposure_mode_->shutter[stage];<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â shutter_time = stage_shutter;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (status_.fixed_analogue_gain == 0.0) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (exposure_mode_->gain[stage] *<br>
> @@ -740,6 +751,13 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  << " analogue gain " << filtered_.analogue_gain;<br>
>  }<br>
><br>
> +double Agc::clipShutter(double shutter)<br>
> +{<br>
> +  Â  Â if (max_shutter_)<br>
> +  Â  Â  Â  Â  Â  Â shutter = std::min(shutter, max_shutter_);<br>
> +  Â  Â return shutter;<br>
> +}<br>
> +<br>
>  // Register algorithm with the system.<br>
>  static Algorithm *Create(Controller *controller)<br>
>  {<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
> index 05c334e4a1de..0427fb59ec1b 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
> @@ -78,6 +78,7 @@ public:<br>
>  Â  Â  Â unsigned int GetConvergenceFrames() const override;<br>
>  Â  Â  Â void SetEv(double ev) override;<br>
>  Â  Â  Â void SetFlickerPeriod(double flicker_period) override;<br>
> +  Â  Â void SetMaxShutter(double max_shutter) override; // microseconds<br>
>  Â  Â  Â void SetFixedShutter(double fixed_shutter) override; // microseconds<br>
>  Â  Â  Â void SetFixedAnalogueGain(double fixed_analogue_gain) override;<br>
>  Â  Â  Â void SetMeteringMode(std::string const &metering_mode_name) override;<br>
> @@ -100,6 +101,7 @@ private:<br>
>  Â  Â  Â void filterExposure(bool desaturate);<br>
>  Â  Â  Â void divideUpExposure();<br>
>  Â  Â  Â void writeAndFinish(Metadata *image_metadata, bool desaturate);<br>
> +  Â  Â double clipShutter(double shutter);<br>
>  Â  Â  Â AgcMeteringMode *metering_mode_;<br>
>  Â  Â  Â AgcExposureMode *exposure_mode_;<br>
>  Â  Â  Â AgcConstraintMode *constraint_mode_;<br>
> @@ -126,6 +128,7 @@ private:<br>
>  Â  Â  Â std::string constraint_mode_name_;<br>
>  Â  Â  Â double ev_;<br>
>  Â  Â  Â double flicker_period_;<br>
> +  Â  Â double max_shutter_;<br>
>  Â  Â  Â double fixed_shutter_;<br>
>  Â  Â  Â double fixed_analogue_gain_;<br>
>  };<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index e4911b734e20..8c0e699184f6 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -1006,6 +1006,18 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio<br>
>  Â  Â  Â libcameraMetadata_.set(controls::FrameDurations,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  { static_cast<int64_t>(minFrameDuration_),<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  static_cast<int64_t>(maxFrameDuration_) });<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>
<br>
Do you need to intialize this, as GetVBlanking will unconditionally<br>
update it, unless assert() fails, but we've bigger troubles in that<br>
case.<br></blockquote><div><br></div><div><font face="arial, sans-serif">I do need to initialize this as GetVBlanking() will clip the passed in exposure value if it needs to.</font></div><div><font face="arial, sans-serif">So, I pass in std::numeric_limits<double>::max() to return out the largest possible exposure achievable.</font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">Regards,</font></div><div><font face="arial, sans-serif">Naush </font></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>
Rest looks good<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
  j<br>
<br>
> +  Â  Â helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);<br>
> +<br>
> +  Â  Â RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
> +  Â  Â  Â  Â  Â  Â controller_.GetAlgorithm("agc"));<br>
> +  Â  Â agc->SetMaxShutter(maxShutter);<br>
>  }<br>
><br>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>