[libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Pass the maximum allowable shutter speed into the AGC
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 27 01:17:23 CET 2021
Hi Naush,
Thank you for the patch.
On Sun, Jan 24, 2021 at 02:05:05PM +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 AgcAlgorihtm class. The
s/AgcAlgorihtm/AgcAlgorithm/
> 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>
> Reviewed-by: David Plowman <david.plowman 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
Not something to be addressed now, but would it make sense to express
durations using std::chrono::duration ? This would avoid the risk of
passing a value expressed in the wrong unit. Another option would be to
use double across the code base, using the same unit everywhere (which
could be seconds, double should give us enough precision).
> 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_);
Instead of clipping every time the fixed shutter value is accessed,
wouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?
This would possibly cause the fixed shutter to be clipped based on the
current mode and not get increased on mode change, but is that a problem
? An application setting the ExposureTime control should expect it to be
clipped, but should it expect the original value to be remembered ?
An alternative would be to turn clip_shutter() into a GetFixedShutter()
member function that would use fixed_shutter_ and max_shutter_
internally and not take any argument. There's one caller that uses the
function with a different set of arguments, so maybe we would need to
keep clip_shutter() and add a GetFixedShutter() wrapper.
> }
>
> 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;
> }
> 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 30ba9aef9d97..75c9e404dcc1 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1008,6 +1008,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
More information about the libcamera-devel
mailing list