<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 27 Mar 2023 at 14:48, Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.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<br>
<br>
On Mon, Mar 27, 2023 at 10:34:38AM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> Make a copy of the CameraMode structure on a switch mode call. This<br>
> replaces the existing lastSensitivity_ field.<br>
><br>
> Limit the AGC gain calculations to the minimum value given by the<br>
> CameraMode structure. The maximum value remains unclipped as any gain<br>
> over the sensor maximum will be made up by digital gain.<br>
><br>
> Rename clipShutter to limitShutter for consistency, and have the latter<br>
> limit the shutter speed to both upper and lower bounds.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 55 +++++++++++++++++-----<br>
>  src/ipa/raspberrypi/controller/rpi/agc.h  Â |  4 +-<br>
>  2 files changed, 45 insertions(+), 14 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> index 4ea0dd41e66c..ed7ecbe05a8e 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> @@ -309,14 +309,14 @@ void Agc::setFixedShutter(Duration fixedShutter)<br>
>  {<br>
>  Â  Â  Â fixedShutter_ = fixedShutter;<br>
>  Â  Â  Â /* Set this in case someone calls disableAuto() straight after. */<br>
> -  Â  Â status_.shutterTime = clipShutter(fixedShutter_);<br>
> +  Â  Â status_.shutterTime = limitShutter(fixedShutter_);<br>
>  }<br>
><br>
>  void Agc::setFixedAnalogueGain(double fixedAnalogueGain)<br>
>  {<br>
>  Â  Â  Â fixedAnalogueGain_ = fixedAnalogueGain;<br>
>  Â  Â  Â /* Set this in case someone calls disableAuto() straight after. */<br>
> -  Â  Â status_.analogueGain = fixedAnalogueGain;<br>
> +  Â  Â status_.analogueGain = limitGain(fixedAnalogueGain);<br>
>  }<br>
><br>
>  void Agc::setMeteringMode(std::string const &meteringModeName)<br>
> @@ -342,7 +342,7 @@ void Agc::switchMode(CameraMode const &cameraMode,<br>
><br>
>  Â  Â  Â housekeepConfig();<br>
><br>
> -  Â  Â Duration fixedShutter = clipShutter(fixedShutter_);<br>
> +  Â  Â Duration fixedShutter = limitShutter(fixedShutter_);<br>
>  Â  Â  Â if (fixedShutter && fixedAnalogueGain_) {<br>
>  Â  Â  Â  Â  Â  Â  Â /* We're going to reset the algorithm here with these fixed values. */<br>
><br>
> @@ -371,7 +371,7 @@ void Agc::switchMode(CameraMode const &cameraMode,<br>
>  Â  Â  Â  Â  Â  Â  Â  * current exposure profile, which takes care of everything else.<br>
>  Â  Â  Â  Â  Â  Â  Â  */<br>
><br>
> -  Â  Â  Â  Â  Â  Â double ratio = lastSensitivity_ / cameraMode.sensitivity;<br>
> +  Â  Â  Â  Â  Â  Â double ratio = mode_.sensitivity / cameraMode.sensitivity;<br>
>  Â  Â  Â  Â  Â  Â  Â target_.totalExposureNoDG *= ratio;<br>
>  Â  Â  Â  Â  Â  Â  Â target_.totalExposure *= ratio;<br>
>  Â  Â  Â  Â  Â  Â  Â filtered_.totalExposureNoDG *= ratio;<br>
> @@ -393,8 +393,11 @@ void Agc::switchMode(CameraMode const &cameraMode,<br>
><br>
>  Â  Â  Â writeAndFinish(metadata, false);<br>
><br>
> -  Â  Â /* We must remember the sensitivity of this mode for the next SwitchMode. */<br>
> -  Â  Â lastSensitivity_ = cameraMode.sensitivity;<br>
> +  Â  Â /*<br>
> +  Â  Â  * Store the more in the local state. We must remember the sensitivity<br>
> +  Â  Â  * of this mode for the next SwitchMode.<br>
> +  Â  Â  */<br>
> +  Â  Â mode_ = cameraMode;<br>
<br>
I see that mode_.sensitivity is accessed in the else if() clause<br>
above, as well as limitShutter() is called before the whole if() {}<br>
block, but mode_ is only updatd here. I presume this is intentional as<br>
it was already like this with lastSensitivity_ ?<br></blockquote><div><br></div><div>You are right!  I really ought to move the mode_ = cameraMode to the top of this<br>function.  The consequence of doing it this way is minimal, we will use a 0<br>value for the minimum shutter time when clipping.  But it is wrong, so should be<br>fix.<br><br>I'll post an update to this patch in-line and fix David's reported typo as well.<br><br>Regards,</div><div>Naush<br></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>
Beside David's comments:<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
<br>
<br>
>  }<br>
><br>
>  void Agc::prepare(Metadata *imageMetadata)<br>
> @@ -516,7 +519,7 @@ 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_.fixedShutter = clipShutter(fixedShutter_);<br>
> +  Â  Â status_.fixedShutter = limitShutter(fixedShutter_);<br>
>  Â  Â  Â status_.fixedAnalogueGain = fixedAnalogueGain_;<br>
>  Â  Â  Â status_.flickerPeriod = flickerPeriod_;<br>
>  Â  Â  Â LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixedShutter "<br>
> @@ -703,7 +706,7 @@ void Agc::computeTargetExposure(double gain)<br>
>  Â  Â  Â  Â  Â  Â  Â Duration maxShutter = status_.fixedShutter<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â ? status_.fixedShutter<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â : exposureMode_->shutter.back();<br>
> -  Â  Â  Â  Â  Â  Â maxShutter = clipShutter(maxShutter);<br>
> +  Â  Â  Â  Â  Â  Â maxShutter = limitShutter(maxShutter);<br>
>  Â  Â  Â  Â  Â  Â  Â Duration maxTotalExposure =<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â maxShutter *<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â (status_.fixedAnalogueGain != 0.0<br>
> @@ -803,15 +806,16 @@ void Agc::divideUpExposure()<br>
>  Â  Â  Â double analogueGain;<br>
>  Â  Â  Â shutterTime = status_.fixedShutter ? status_.fixedShutter<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  : exposureMode_->shutter[0];<br>
> -  Â  Â shutterTime = clipShutter(shutterTime);<br>
> +  Â  Â shutterTime = limitShutter(shutterTime);<br>
>  Â  Â  Â analogueGain = status_.fixedAnalogueGain != 0.0 ? status_.fixedAnalogueGain<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â : exposureMode_->gain[0];<br>
> +  Â  Â analogueGain = limitGain(analogueGain);<br>
>  Â  Â  Â if (shutterTime * analogueGain < exposureValue) {<br>
>  Â  Â  Â  Â  Â  Â  Â for (unsigned int stage = 1;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  stage < exposureMode_->gain.size(); stage++) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (!status_.fixedShutter) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â Duration stageShutter =<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â clipShutter(exposureMode_->shutter[stage]);<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â limitShutter(exposureMode_->shutter[stage]);<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (stageShutter * analogueGain >= exposureValue) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â shutterTime = exposureValue / analogueGain;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
> @@ -824,6 +828,7 @@ void Agc::divideUpExposure()<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â analogueGain = exposureMode_->gain[stage];<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â analogueGain = limitGain(analogueGain);<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
>  Â  Â  Â  Â  Â  Â  Â }<br>
>  Â  Â  Â }<br>
> @@ -846,6 +851,7 @@ void Agc::divideUpExposure()<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  * gain as a side-effect.<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  */<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â analogueGain = std::min(analogueGain, exposureMode_->gain.back());<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â analogueGain = limitGain(analogueGain);<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â shutterTime = newShutterTime;<br>
>  Â  Â  Â  Â  Â  Â  Â }<br>
>  Â  Â  Â  Â  Â  Â  Â LOG(RPiAgc, Debug) << "After flicker avoidance, shutter "<br>
> @@ -872,13 +878,36 @@ void Agc::writeAndFinish(Metadata *imageMetadata, bool desaturate)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  << " analogue gain " << filtered_.analogueGain;<br>
>  }<br>
><br>
> -Duration Agc::clipShutter(Duration shutter)<br>
> +Duration Agc::limitShutter(Duration shutter)<br>
>  {<br>
> -  Â  Â if (maxShutter_)<br>
> -  Â  Â  Â  Â  Â  Â shutter = std::min(shutter, maxShutter_);<br>
> +  Â  Â /*<br>
> +  Â  Â  * shutter == 0 is a special case for fixed shutter values, and must pass<br>
> +  Â  Â  * through unchanged<br>
> +  Â  Â  */<br>
> +  Â  Â if (!shutter)<br>
> +  Â  Â  Â  Â  Â  Â return shutter;<br>
> +<br>
> +  Â  Â shutter = std::clamp(shutter, mode_.minShutter, maxShutter_);<br>
>  Â  Â  Â return shutter;<br>
>  }<br>
><br>
> +double Agc::limitGain(double gain) const<br>
> +{<br>
> +  Â  Â /*<br>
> +  Â  Â  * Only limit the lower bounds of the gain value to what the sensor limits.<br>
> +  Â  Â  * The upper bound on analogue gain will be made up with additional digital<br>
> +  Â  Â  * gain applied by the ISP.<br>
> +  Â  Â  *<br>
> +  Â  Â  * gain == 0.0 is a special case for fixed shutter values, and must pass<br>
> +  Â  Â  * through unchanged<br>
> +  Â  Â  */<br>
> +  Â  Â if (!gain)<br>
> +  Â  Â  Â  Â  Â  Â return gain;<br>
> +<br>
> +  Â  Â gain = std::max(gain, mode_.minAnalogueGain);<br>
> +  Â  Â return gain;<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.h b/src/ipa/raspberrypi/controller/rpi/agc.h<br>
> index f04896ca25ad..661d8b08cea2 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.h<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h<br>
> @@ -103,10 +103,12 @@ private:<br>
>  Â  Â  Â void filterExposure(bool desaturate);<br>
>  Â  Â  Â void divideUpExposure();<br>
>  Â  Â  Â void writeAndFinish(Metadata *imageMetadata, bool desaturate);<br>
> -  Â  Â libcamera::utils::Duration clipShutter(libcamera::utils::Duration shutter);<br>
> +  Â  Â libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);<br>
> +  Â  Â double limitGain(double gain) const;<br>
>  Â  Â  Â AgcMeteringMode *meteringMode_;<br>
>  Â  Â  Â AgcExposureMode *exposureMode_;<br>
>  Â  Â  Â AgcConstraintMode *constraintMode_;<br>
> +  Â  Â CameraMode mode_;<br>
>  Â  Â  Â uint64_t frameCount_;<br>
>  Â  Â  Â AwbStatus awb_;<br>
>  Â  Â  Â struct ExposureValues {<br>
> --<br>
> 2.34.1<br>
><br>
</blockquote></div></div>