<div dir="ltr"><div dir="ltr"></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 27 Jan 2021 at 09:47, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</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, Laurent<br>
<br>
Thanks for all the discussion on this.<br>
<br>
On Wed, 27 Jan 2021 at 09:34, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi Laurent,<br>
><br>
> Thank you for your review feedback.<br>
><br>
> On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br>
>><br>
>> Hi Naush,<br>
>><br>
>> Thank you for the patch.<br>
>><br>
>> On Sun, Jan 24, 2021 at 02:05:05PM +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 AgcAlgorihtm class. The<br>
>><br>
>> s/AgcAlgorihtm/AgcAlgorithm/<br>
><br>
><br>
> Ack.<br>
><br>
>><br>
>><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 | 49 +++++++++++++------<br>
>> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 +<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>
>><br>
>> Not something to be addressed now, but would it make sense to express<br>
>> durations using std::chrono::duration ? This would avoid the risk of<br>
>> passing a value expressed in the wrong unit. Another option would be to<br>
>> use double across the code base, using the same unit everywhere (which<br>
>> could be seconds, double should give us enough precision).<br>
>><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..937bb70ece67 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,23 @@ void Agc::SetFlickerPeriod(double flicker_period)<br>
>> > flicker_period_ = flicker_period;<br>
>> > }<br>
>> ><br>
>> > +static double clip_shutter(double shutter, double max_shutter)<br>
>> > +{<br>
>> > + if (max_shutter)<br>
>> > + shutter = std::min(shutter, max_shutter);<br>
>> > + return shutter;<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 = clip_shutter(fixed_shutter_, max_shutter_);<br>
>><br>
>> Instead of clipping every time the fixed shutter value is accessed,<br>
>> wouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?<br>
>> This would possibly cause the fixed shutter to be clipped based on the<br>
>> current mode and not get increased on mode change, but is that a problem<br>
>> ? An application setting the ExposureTime control should expect it to be<br>
>> clipped, but should it expect the original value to be remembered ?<br>
><br>
><br>
> We can indeed store the clipped fixed_shutter_ directly. However, max_shutter_ can change at runtime (based on the application request), and so we must re-clip fixed_shutter_ if that occurs. There is also another simplification here, we always pass max_shutter_ as the second argument to clip_shutter. This can be removed, so clip_shutter takes only one argument.<br>
><br>
> David, what are your thoughts?<br>
<br>
I guess I don't feel very strongly, whatever is tidiest is good with<br>
me. Having a new field such as clipped_fixed_shutter_ seems<br>
reasonable, I think over-writing fixed_shutter_ itself would be<br>
undesirable for the reasons described. Generally I'm a bit nervous<br>
about adding extra state variables that you have to remember to<br>
update, but I guess if only SetFixedShutter and SetMaxShutter have to<br>
do that then it all seems tidy enough, right?<br>
<br>
Thanks!<br>
David<br></blockquote><div><br></div><div>My personal preference would be:</div><div><br></div><div>- Do not store a clipped_fixed_shutter_, instead do what we currently do, i.e. clip on use. From my scan through, it only happens once per frame and on a switch mode. So it will only add a few cycles overall, but will make the code more readable imo (by avoiding additional state variables, one clipped, one not).</div><div>- clip_shutter becomes a private member function, so it can access max_shutter_ and we do not pass the second argument into the function.</div><div><br></div><div>Happy to go with the consensus though.</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>
> Naush<br>
><br>
>><br>
>><br>
>> An alternative would be to turn clip_shutter() into a GetFixedShutter()<br>
>> member function that would use fixed_shutter_ and max_shutter_<br>
>> internally and not take any argument. There's one caller that uses the<br>
>> function with a different set of arguments, so maybe we would need to<br>
>> keep clip_shutter() and add a GetFixedShutter() wrapper.<br>
>><br>
>> > }<br>
>> ><br>
>> > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)<br>
>> > @@ -261,7 +273,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 = clip_shutter(fixed_shutter_, max_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 +282,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 +303,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 +416,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 = clip_shutter(fixed_shutter_, max_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 +596,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>
>> > + max_shutter = clip_shutter(max_shutter, 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 +687,7 @@ void Agc::divideUpExposure()<br>
>> > shutter_time = status_.fixed_shutter != 0.0<br>
>> > ? status_.fixed_shutter<br>
>> > : exposure_mode_->shutter[0];<br>
>> > + shutter_time = clip_shutter(shutter_time, max_shutter_);<br>
>> > analogue_gain = status_.fixed_analogue_gain != 0.0<br>
>> > ? status_.fixed_analogue_gain<br>
>> > : exposure_mode_->gain[0];<br>
>> > @@ -678,14 +695,16 @@ 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>
>> > + clip_shutter(exposure_mode_->shutter[stage],<br>
>> > + max_shutter_);<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>
>> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
>> > index 05c334e4a1de..2ce3b1a1700a 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>
>> > @@ -126,6 +127,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 30ba9aef9d97..75c9e404dcc1 100644<br>
>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
>> > @@ -1008,6 +1008,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>
>> > + 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>
>> --<br>
>> Regards,<br>
>><br>
>> Laurent Pinchart<br>
</blockquote></div></div>