| Message ID | 20210124140506.786503-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@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)
Hi Laurent, Thank you for your review feedback. On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart < laurent.pinchart@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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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? 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 >
Hi Naush, Laurent Thanks for all the discussion on this. On Wed, 27 Jan 2021 at 09:34, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi Laurent, > > Thank you for your review feedback. > > On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart <laurent.pinchart@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@raspberrypi.com> >> > Reviewed-by: David Plowman <david.plowman@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 > > 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
On Wed, 27 Jan 2021 at 09:47, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush, Laurent > > Thanks for all the discussion on this. > > On Wed, 27 Jan 2021 at 09:34, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > Hi Laurent, > > > > Thank you for your review feedback. > > > > On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart < > laurent.pinchart@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@raspberrypi.com> > >> > Reviewed-by: David Plowman <david.plowman@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. > > > > > 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 >
Hi Naush On Wed, 27 Jan 2021 at 09:58, Naushir Patuck <naush@raspberrypi.com> wrote: > > > On Wed, 27 Jan 2021 at 09:47, David Plowman <david.plowman@raspberrypi.com> wrote: >> >> Hi Naush, Laurent >> >> Thanks for all the discussion on this. >> >> On Wed, 27 Jan 2021 at 09:34, Naushir Patuck <naush@raspberrypi.com> wrote: >> > >> > Hi Laurent, >> > >> > Thank you for your review feedback. >> > >> > On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart <laurent.pinchart@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@raspberrypi.com> >> >> > Reviewed-by: David Plowman <david.plowman@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
Hi Laurent, On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart < laurent.pinchart@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/ > > > 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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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). > Sorry, forgot to comment about this. I do think it's a great idea to use std::chrono::duration throughout for time based variables. It reduces so much of the ambiguity over time bases. When I get a chance, perhaps I'll have a go within our IPA and controller. Regards, Naush > > > 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 >
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; } 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)