<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>