[libcamera-devel] [PATCH 3/4] ipa: raspberrypi: Pass the maximum allowable shutter speed into the AGC
David Plowman
david.plowman at raspberrypi.com
Thu Jan 21 16:29:01 CET 2021
Hi Naush
Thanks for submitting this... most of it looks rather familiar! As
such I'm not likely to have too much to say, just one or two minor
observations.
On Thu, 21 Jan 2021 at 11:59, Naushir Patuck <naush at raspberrypi.com> 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 AgcAlgorihtm class. The
> IPA provides the the maximum shutter speed for AGC calculations.
> This applies to both the manual and auto AGC modes.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> .../raspberrypi/controller/agc_algorithm.hpp | 1 +
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 49 +++++++++++++------
> src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 +
> 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..937bb70ece67 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,23 @@ void Agc::SetFlickerPeriod(double flicker_period)
> flicker_period_ = flicker_period;
> }
>
> +static double clip_shutter(double shutter, double max_shutter)
> +{
> + if (max_shutter)
> + shutter = std::min(shutter, max_shutter);
> + return shutter;
> +}
> +
> +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 = clip_shutter(fixed_shutter_, max_shutter_);
> }
>
> void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
> @@ -261,7 +273,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
> {
> housekeepConfig();
>
> - if (fixed_shutter_ != 0.0 && fixed_analogue_gain_ != 0.0) {
> + double fixed_shutter = clip_shutter(fixed_shutter_, max_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 +282,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 +303,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 +416,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 = clip_shutter(fixed_shutter_, max_shutter_);
> + status_.fixed_shutter = fixed_shutter;
> status_.fixed_analogue_gain = fixed_analogue_gain_;
> status_.flicker_period = flicker_period_;
> LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixed_shutter "
> @@ -582,13 +596,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();
> + max_shutter = clip_shutter(max_shutter, 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 +687,7 @@ void Agc::divideUpExposure()
> shutter_time = status_.fixed_shutter != 0.0
> ? status_.fixed_shutter
> : exposure_mode_->shutter[0];
> + shutter_time = clip_shutter(shutter_time, max_shutter_);
> analogue_gain = status_.fixed_analogue_gain != 0.0
> ? status_.fixed_analogue_gain
> : exposure_mode_->gain[0];
> @@ -678,14 +695,16 @@ 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 =
> + clip_shutter(exposure_mode_->shutter[stage],
> + max_shutter_);
> + if (stage_shutter * analogue_gain >=
> exposure_value) {
> shutter_time =
> exposure_value / analogue_gain;
> break;
> }
> - shutter_time = exposure_mode_->shutter[stage];
> + shutter_time = stage_shutter;
It's not a subject for this patch set, but I do rather feel here that
the (my) attempts to placate the line-length-gods have rendered
everything a bit unreadable. Maybe worth revisiting at some point.
> }
> if (status_.fixed_analogue_gain == 0.0) {
> if (exposure_mode_->gain[stage] *
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 05c334e4a1de..2ce3b1a1700a 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;
> @@ -126,6 +127,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 9e6e030c4997..2b71efc63f10 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1008,6 +1008,18 @@ void IPARPi::applyFrameDurations(double min, double max)
> 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"));
I notice we don't check for NULL here. It's true that without AGC,
realistically, what could you actually do? Again, it may be a topic
for another time, but we should probably revisit exactly which
algorithms we mandate and which could be left out (for example,
someone should probably be able to omit the "sdn" algorithm without
everything blowing up).
But as my comments are really just notes to look at other stuff:
Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
Thanks!
David
> + agc->SetMaxShutter(maxShutter);
> }
>
> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list