[libcamera-devel] [PATCH v4 4/5] ipa: raspberrypi: Pass the maximum allowable shutter speed into the AGC
Naushir Patuck
naush at raspberrypi.com
Thu Feb 4 23:20:31 CET 2021
Hi Laurent,
Thank you for your review feedback.
On Thu, 4 Feb 2021 at 20:59, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Jan 29, 2021 at 11:16:15AM +0000, Naushir Patuck wrote:
> > In order to provide an optimal split between shutter speed and gain, the
> > AGC must know the maximum allowable shutter speed, as limited by the
> > maximum frame duration (either application provided or the default).
> >
> > Add a new API function, SetMaxShutter, to the AgcAlgorithm class. The
> > IPA provides the the maximum shutter speed for AGC calculations.
>
> s/the the/the/
>
> > This applies to both the manual and auto AGC modes.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > .../raspberrypi/controller/agc_algorithm.hpp | 1 +
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 48 +++++++++++++------
> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 ++
> > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++
> > 4 files changed, 49 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > index 981f1de2f0e4..f97deb0fca59 100644
> > --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp
> > @@ -19,6 +19,7 @@ public:
> > virtual void SetEv(double ev) = 0;
> > virtual void SetFlickerPeriod(double flicker_period) = 0;
> > virtual void SetFixedShutter(double fixed_shutter) = 0; //
> microseconds
> > + virtual void SetMaxShutter(double max_shutter) = 0; // microseconds
> > virtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;
> > virtual void SetMeteringMode(std::string const
> &metering_mode_name) = 0;
> > virtual void SetExposureMode(std::string const
> &exposure_mode_name) = 0;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index eddd1684da12..0023d50029f1 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -157,7 +157,7 @@ Agc::Agc(Controller *controller)
> > frame_count_(0), lock_count_(0),
> > last_target_exposure_(0.0),
> > ev_(1.0), flicker_period_(0.0),
> > - fixed_shutter_(0), fixed_analogue_gain_(0.0)
> > + max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)
> > {
> > memset(&awb_, 0, sizeof(awb_));
> > // Setting status_.total_exposure_value_ to zero initially tells us
> > @@ -227,11 +227,16 @@ void Agc::SetFlickerPeriod(double flicker_period)
> > flicker_period_ = flicker_period;
> > }
> >
> > +void Agc::SetMaxShutter(double max_shutter)
> > +{
> > + max_shutter_ = max_shutter;
> > +}
> > +
> > void Agc::SetFixedShutter(double fixed_shutter)
> > {
> > fixed_shutter_ = fixed_shutter;
> > // Set this in case someone calls Pause() straight after.
> > - status_.shutter_time = fixed_shutter;
> > + status_.shutter_time = clipShutter(fixed_shutter_);
> > }
> >
> > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
> > @@ -261,7 +266,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
> const &camera_mode,
> > {
> > housekeepConfig();
> >
> > - if (fixed_shutter_ != 0.0 && fixed_analogue_gain_ != 0.0) {
> > + double fixed_shutter = clipShutter(fixed_shutter_);
> > + if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {
> > // We're going to reset the algorithm here with these
> fixed values.
> >
> > fetchAwbStatus(metadata);
> > @@ -269,14 +275,14 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
> const &camera_mode,
> > ASSERT(min_colour_gain != 0.0);
> >
> > // This is the equivalent of computeTargetExposure and
> applyDigitalGain.
> > - target_.total_exposure_no_dg = fixed_shutter_ *
> fixed_analogue_gain_;
> > + target_.total_exposure_no_dg = fixed_shutter *
> fixed_analogue_gain_;
> > target_.total_exposure = target_.total_exposure_no_dg /
> min_colour_gain;
> >
> > // Equivalent of filterExposure. This resets any "history".
> > filtered_ = target_;
> >
> > // Equivalent of divideUpExposure.
> > - filtered_.shutter = fixed_shutter_;
> > + filtered_.shutter = fixed_shutter;
> > filtered_.analogue_gain = fixed_analogue_gain_;
> > } else if (status_.total_exposure_value) {
> > // On a mode switch, it's possible the exposure profile
> could change,
> > @@ -290,7 +296,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode
> const &camera_mode,
> > // for any that weren't set.
> >
> > // Equivalent of divideUpExposure.
> > - filtered_.shutter = fixed_shutter_ ? fixed_shutter_ :
> config_.default_exposure_time;
> > + filtered_.shutter = fixed_shutter ? fixed_shutter :
> config_.default_exposure_time;
> > filtered_.analogue_gain = fixed_analogue_gain_ ?
> fixed_analogue_gain_ : config_.default_analogue_gain;
> > }
> >
> > @@ -403,7 +409,8 @@ void Agc::housekeepConfig()
> > {
> > // First fetch all the up-to-date settings, so no one else has to
> do it.
> > status_.ev = ev_;
> > - status_.fixed_shutter = fixed_shutter_;
> > + double fixed_shutter = clipShutter(fixed_shutter_);
> > + status_.fixed_shutter = fixed_shutter;
>
> You could write
>
> status_.fixed_shutter = clipShutter(fixed_shutter_);
>
> > status_.fixed_analogue_gain = fixed_analogue_gain_;
> > status_.flicker_period = flicker_period_;
> > LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixed_shutter "
> > @@ -582,13 +589,15 @@ void Agc::computeTargetExposure(double gain)
> > target_.total_exposure = current_.total_exposure_no_dg *
> gain;
> > // The final target exposure is also limited to what the
> exposure
> > // mode allows.
> > + double max_shutter = status_.fixed_shutter != 0.0
> > + ? status_.fixed_shutter
> > + :
> exposure_mode_->shutter.back();
>
> The indentation is a bit weird, how about this ?
>
> double max_shutter = status_.fixed_shutter != 0.0
> ? status_.fixed_shutter
> : exposure_mode_->shutter.back();
>
> Those are minor issues,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> If you're fine with these changes, there's no need to resent, I'll fix
> when applying.
>
Ack all the minors. Please do fix when applying if it is not too much
trouble.
Regards,
Naush
>
> > + max_shutter = clipShutter(max_shutter);
> > double max_total_exposure =
> > - (status_.fixed_shutter != 0.0
> > - ? status_.fixed_shutter
> > - : exposure_mode_->shutter.back()) *
> > + max_shutter *
> > (status_.fixed_analogue_gain != 0.0
> > - ? status_.fixed_analogue_gain
> > - : exposure_mode_->gain.back());
> > + ? status_.fixed_analogue_gain
> > + : exposure_mode_->gain.back());
> > target_.total_exposure = std::min(target_.total_exposure,
> > max_total_exposure);
> > }
> > @@ -671,6 +680,7 @@ void Agc::divideUpExposure()
> > shutter_time = status_.fixed_shutter != 0.0
> > ? status_.fixed_shutter
> > : exposure_mode_->shutter[0];
> > + shutter_time = clipShutter(shutter_time);
> > analogue_gain = status_.fixed_analogue_gain != 0.0
> > ? status_.fixed_analogue_gain
> > : exposure_mode_->gain[0];
> > @@ -678,14 +688,15 @@ void Agc::divideUpExposure()
> > for (unsigned int stage = 1;
> > stage < exposure_mode_->gain.size(); stage++) {
> > if (status_.fixed_shutter == 0.0) {
> > - if (exposure_mode_->shutter[stage] *
> > - analogue_gain >=
> > + double stage_shutter =
> > +
> clipShutter(exposure_mode_->shutter[stage]);
> > + if (stage_shutter * analogue_gain >=
> > exposure_value) {
> > shutter_time =
> > exposure_value /
> analogue_gain;
> > break;
> > }
> > - shutter_time =
> exposure_mode_->shutter[stage];
> > + shutter_time = stage_shutter;
> > }
> > if (status_.fixed_analogue_gain == 0.0) {
> > if (exposure_mode_->gain[stage] *
> > @@ -740,6 +751,13 @@ void Agc::writeAndFinish(Metadata *image_metadata,
> bool desaturate)
> > << " analogue gain " << filtered_.analogue_gain;
> > }
> >
> > +double Agc::clipShutter(double shutter)
> > +{
> > + if (max_shutter_)
> > + shutter = std::min(shutter, max_shutter_);
> > + return shutter;
> > +}
> > +
> > // Register algorithm with the system.
> > static Algorithm *Create(Controller *controller)
> > {
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 05c334e4a1de..0427fb59ec1b 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -78,6 +78,7 @@ public:
> > unsigned int GetConvergenceFrames() const override;
> > void SetEv(double ev) override;
> > void SetFlickerPeriod(double flicker_period) override;
> > + void SetMaxShutter(double max_shutter) override; // microseconds
> > void SetFixedShutter(double fixed_shutter) override; //
> microseconds
> > void SetFixedAnalogueGain(double fixed_analogue_gain) override;
> > void SetMeteringMode(std::string const &metering_mode_name)
> override;
> > @@ -100,6 +101,7 @@ private:
> > void filterExposure(bool desaturate);
> > void divideUpExposure();
> > void writeAndFinish(Metadata *image_metadata, bool desaturate);
> > + double clipShutter(double shutter);
> > AgcMeteringMode *metering_mode_;
> > AgcExposureMode *exposure_mode_;
> > AgcConstraintMode *constraint_mode_;
> > @@ -126,6 +128,7 @@ private:
> > std::string constraint_mode_name_;
> > double ev_;
> > double flicker_period_;
> > + double max_shutter_;
> > double fixed_shutter_;
> > double fixed_analogue_gain_;
> > };
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index e4911b734e20..8c0e699184f6 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1006,6 +1006,18 @@ void IPARPi::applyFrameDurations(double
> minFrameDuration, double maxFrameDuratio
> > libcameraMetadata_.set(controls::FrameDurations,
> > { static_cast<int64_t>(minFrameDuration_),
> > static_cast<int64_t>(maxFrameDuration_)
> });
> > +
> > + /*
> > + * Calculate the maximum exposure time possible for the AGC to use.
> > + * GetVBlanking() will update maxShutter with the largest exposure
> > + * value possible.
> > + */
> > + double maxShutter = std::numeric_limits<double>::max();
> > + helper_->GetVBlanking(maxShutter, minFrameDuration_,
> maxFrameDuration_);
> > +
> > + RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> > + controller_.GetAlgorithm("agc"));
> > + agc->SetMaxShutter(maxShutter);
> > }
> >
> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210204/4f7a22fe/attachment-0001.htm>
More information about the libcamera-devel
mailing list