[libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Ensure shutter speed and gain are clipped in the AGC
Naushir Patuck
naush at raspberrypi.com
Mon Mar 27 15:57:18 CEST 2023
Hi Jacopo,
On Mon, 27 Mar 2023 at 14:48, Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> Hi Naush
>
> On Mon, Mar 27, 2023 at 10:34:38AM +0100, Naushir Patuck via
> libcamera-devel 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
> > + * of this mode for the next SwitchMode.
> > + */
> > + mode_ = cameraMode;
>
> I see that mode_.sensitivity is accessed in the else if() clause
> above, as well as limitShutter() is called before the whole if() {}
> block, but mode_ is only updatd here. I presume this is intentional as
> it was already like this with lastSensitivity_ ?
>
You are right! I really ought to move the mode_ = cameraMode to the top of
this
function. The consequence of doing it this way is minimal, we will use a 0
value for the minimum shutter time when clipping. But it is wrong, so
should be
fix.
I'll post an update to this patch in-line and fix David's reported typo as
well.
Regards,
Naush
>
> Beside David's comments:
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
>
> > }
> >
> > 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 {
> > --
> > 2.34.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230327/912882de/attachment.htm>
More information about the libcamera-devel
mailing list