[libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure shutter speed and gain are clipped in the AGC
David Plowman
david.plowman at raspberrypi.com
Mon Mar 27 15:31:06 CEST 2023
Hi Naush
Thanks for the patch.
On Mon, 27 Mar 2023 at 10:34, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Make a copy of the CameraMode structure on a switch mode call. This
> replaces the existing lastSensitivity_ field.
>
> Limit the AGC gain calculations to the minimum value given by the
> CameraMode structure. The maximum value remains unclipped as any gain
> over the sensor maximum will be made up by digital gain.
>
> Rename clipShutter to limitShutter for consistency, and have the latter
> limit the shutter speed to both upper and lower bounds.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 55 +++++++++++++++++-----
> src/ipa/raspberrypi/controller/rpi/agc.h | 4 +-
> 2 files changed, 45 insertions(+), 14 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 4ea0dd41e66c..ed7ecbe05a8e 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -309,14 +309,14 @@ void Agc::setFixedShutter(Duration fixedShutter)
> {
> fixedShutter_ = fixedShutter;
> /* Set this in case someone calls disableAuto() straight after. */
> - status_.shutterTime = clipShutter(fixedShutter_);
> + status_.shutterTime = limitShutter(fixedShutter_);
> }
>
> void Agc::setFixedAnalogueGain(double fixedAnalogueGain)
> {
> fixedAnalogueGain_ = fixedAnalogueGain;
> /* Set this in case someone calls disableAuto() straight after. */
> - status_.analogueGain = fixedAnalogueGain;
> + status_.analogueGain = limitGain(fixedAnalogueGain);
> }
>
> void Agc::setMeteringMode(std::string const &meteringModeName)
> @@ -342,7 +342,7 @@ void Agc::switchMode(CameraMode const &cameraMode,
>
> housekeepConfig();
>
> - Duration fixedShutter = clipShutter(fixedShutter_);
> + Duration fixedShutter = limitShutter(fixedShutter_);
> if (fixedShutter && fixedAnalogueGain_) {
> /* We're going to reset the algorithm here with these fixed values. */
>
> @@ -371,7 +371,7 @@ void Agc::switchMode(CameraMode const &cameraMode,
> * current exposure profile, which takes care of everything else.
> */
>
> - double ratio = lastSensitivity_ / cameraMode.sensitivity;
> + double ratio = mode_.sensitivity / cameraMode.sensitivity;
> target_.totalExposureNoDG *= ratio;
> target_.totalExposure *= ratio;
> filtered_.totalExposureNoDG *= ratio;
> @@ -393,8 +393,11 @@ void Agc::switchMode(CameraMode const &cameraMode,
>
> writeAndFinish(metadata, false);
>
> - /* We must remember the sensitivity of this mode for the next SwitchMode. */
> - lastSensitivity_ = cameraMode.sensitivity;
> + /*
> + * Store the more in the local state. We must remember the sensitivity
s/more/mode/ I guess.
> + * of this mode for the next SwitchMode.
> + */
> + mode_ = cameraMode;
> }
>
> void Agc::prepare(Metadata *imageMetadata)
> @@ -516,7 +519,7 @@ void Agc::housekeepConfig()
> {
> /* First fetch all the up-to-date settings, so no one else has to do it. */
> status_.ev = ev_;
> - status_.fixedShutter = clipShutter(fixedShutter_);
> + status_.fixedShutter = limitShutter(fixedShutter_);
> status_.fixedAnalogueGain = fixedAnalogueGain_;
> status_.flickerPeriod = flickerPeriod_;
> LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixedShutter "
> @@ -703,7 +706,7 @@ void Agc::computeTargetExposure(double gain)
> Duration maxShutter = status_.fixedShutter
> ? status_.fixedShutter
> : exposureMode_->shutter.back();
> - maxShutter = clipShutter(maxShutter);
> + maxShutter = limitShutter(maxShutter);
> Duration maxTotalExposure =
> maxShutter *
> (status_.fixedAnalogueGain != 0.0
> @@ -803,15 +806,16 @@ void Agc::divideUpExposure()
> double analogueGain;
> shutterTime = status_.fixedShutter ? status_.fixedShutter
> : exposureMode_->shutter[0];
> - shutterTime = clipShutter(shutterTime);
> + shutterTime = limitShutter(shutterTime);
> analogueGain = status_.fixedAnalogueGain != 0.0 ? status_.fixedAnalogueGain
> : exposureMode_->gain[0];
> + analogueGain = limitGain(analogueGain);
> if (shutterTime * analogueGain < exposureValue) {
> for (unsigned int stage = 1;
> stage < exposureMode_->gain.size(); stage++) {
> if (!status_.fixedShutter) {
> Duration stageShutter =
> - clipShutter(exposureMode_->shutter[stage]);
> + limitShutter(exposureMode_->shutter[stage]);
> if (stageShutter * analogueGain >= exposureValue) {
> shutterTime = exposureValue / analogueGain;
> break;
> @@ -824,6 +828,7 @@ void Agc::divideUpExposure()
> break;
> }
> analogueGain = exposureMode_->gain[stage];
> + analogueGain = limitGain(analogueGain);
> }
> }
> }
> @@ -846,6 +851,7 @@ void Agc::divideUpExposure()
> * gain as a side-effect.
> */
> analogueGain = std::min(analogueGain, exposureMode_->gain.back());
> + analogueGain = limitGain(analogueGain);
> shutterTime = newShutterTime;
> }
> LOG(RPiAgc, Debug) << "After flicker avoidance, shutter "
> @@ -872,13 +878,36 @@ void Agc::writeAndFinish(Metadata *imageMetadata, bool desaturate)
> << " analogue gain " << filtered_.analogueGain;
> }
>
> -Duration Agc::clipShutter(Duration shutter)
> +Duration Agc::limitShutter(Duration shutter)
> {
> - if (maxShutter_)
> - shutter = std::min(shutter, maxShutter_);
> + /*
> + * shutter == 0 is a special case for fixed shutter values, and must pass
> + * through unchanged
> + */
> + if (!shutter)
> + return shutter;
> +
> + shutter = std::clamp(shutter, mode_.minShutter, maxShutter_);
> return shutter;
> }
>
> +double Agc::limitGain(double gain) const
> +{
> + /*
> + * Only limit the lower bounds of the gain value to what the sensor limits.
> + * The upper bound on analogue gain will be made up with additional digital
> + * gain applied by the ISP.
> + *
> + * gain == 0.0 is a special case for fixed shutter values, and must pass
> + * through unchanged
> + */
> + if (!gain)
> + return gain;
> +
> + gain = std::max(gain, mode_.minAnalogueGain);
> + return gain;
> +}
> +
> /* Register algorithm with the system. */
> static Algorithm *create(Controller *controller)
> {
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h
> index f04896ca25ad..661d8b08cea2 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.h
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h
> @@ -103,10 +103,12 @@ private:
> void filterExposure(bool desaturate);
> void divideUpExposure();
> void writeAndFinish(Metadata *imageMetadata, bool desaturate);
> - libcamera::utils::Duration clipShutter(libcamera::utils::Duration shutter);
> + libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);
> + double limitGain(double gain) const;
> AgcMeteringMode *meteringMode_;
> AgcExposureMode *exposureMode_;
> AgcConstraintMode *constraintMode_;
> + CameraMode mode_;
> uint64_t frameCount_;
> AwbStatus awb_;
> struct ExposureValues {
> --
Just wondering if there's a lastSensitivity_ field in here that wants
removing now?
Other than these trivial things:
Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
Thanks!
David
> 2.34.1
>
More information about the libcamera-devel
mailing list