[libcamera-devel] [PATCH v2 3/4] ipa: raspberrypi: Pass the maximum allowable shutter speed into the AGC
David Plowman
david.plowman at raspberrypi.com
Wed Jan 27 11:01:22 CET 2021
Hi Naush
On Wed, 27 Jan 2021 at 09:58, Naushir Patuck <naush at raspberrypi.com> wrote:
>
>
> On Wed, 27 Jan 2021 at 09:47, David Plowman <david.plowman at raspberrypi.com> wrote:
>>
>> Hi Naush, Laurent
>>
>> Thanks for all the discussion on this.
>>
>> On Wed, 27 Jan 2021 at 09:34, Naushir Patuck <naush at raspberrypi.com> wrote:
>> >
>> > Hi Laurent,
>> >
>> > Thank you for your review feedback.
>> >
>> > On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:
>> >>
>> >> 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/
>> >
>> >
>> > Ack.
>> >
>> >>
>> >>
>> >> > 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 ?
>> >
>> >
>> > We can indeed store the clipped fixed_shutter_ directly. However, max_shutter_ can change at runtime (based on the application request), and so we must re-clip fixed_shutter_ if that occurs. There is also another simplification here, we always pass max_shutter_ as the second argument to clip_shutter. This can be removed, so clip_shutter takes only one argument.
>> >
>> > David, what are your thoughts?
>>
>> I guess I don't feel very strongly, whatever is tidiest is good with
>> me. Having a new field such as clipped_fixed_shutter_ seems
>> reasonable, I think over-writing fixed_shutter_ itself would be
>> undesirable for the reasons described. Generally I'm a bit nervous
>> about adding extra state variables that you have to remember to
>> update, but I guess if only SetFixedShutter and SetMaxShutter have to
>> do that then it all seems tidy enough, right?
>>
>> Thanks!
>> David
>
>
> My personal preference would be:
>
> - Do not store a clipped_fixed_shutter_, instead do what we currently do, i.e. clip on use. From my scan through, it only happens once per frame and on a switch mode. So it will only add a few cycles overall, but will make the code more readable imo (by avoiding additional state variables, one clipped, one not).
> - clip_shutter becomes a private member function, so it can access max_shutter_ and we do not pass the second argument into the function.
>
> Happy to go with the consensus though.
Thumbs up from me!
David
>
>>
>>
>> >
>> > Naush
>> >
>> >>
>> >>
>> >> 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