| Message ID | 20200610142455.4641-1-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi I was wondering what folks wanted to do about reviewing patches like this one, when they're very much in the domain of the Raspberry Pi IPAs. Do we need a different procedure for them, do you think? Thanks! David On Wed, 10 Jun 2020 at 15:24, David Plowman <david.plowman@raspberrypi.com> wrote: > > When the sensor is switched into a different mode the line > timings may change, yet the V4L2 driver remembers the number of > exposure lines, not the total time. So this will change "under > our feet". > > These patches extend the IPA handling of the camera mode switch > so that correct exposure values are recalculated for the new mode > and written to the sensor before it starts streaming again. They > also allow the IPA to alter how the total exposure is divided > between actual exposure and analogue gain, according to the > selected exposure profile in the camera tuning. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/algorithm.cpp | 3 +- > src/ipa/raspberrypi/controller/algorithm.hpp | 2 +- > src/ipa/raspberrypi/controller/controller.cpp | 4 +-- > src/ipa/raspberrypi/controller/controller.hpp | 2 +- > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++ > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 + > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++- > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +- > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++- > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +- > .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++- > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++--------- > 13 files changed, 46 insertions(+), 24 deletions(-) > > diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp > index 9bd3df8..55cb201 100644 > --- a/src/ipa/raspberrypi/controller/algorithm.cpp > +++ b/src/ipa/raspberrypi/controller/algorithm.cpp > @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms) > > void Algorithm::Initialise() {} > > -void Algorithm::SwitchMode(CameraMode const &camera_mode) > +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > { > (void)camera_mode; > + (void)metadata; > } > > void Algorithm::Prepare(Metadata *image_metadata) > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp > index b82c184..187c50c 100644 > --- a/src/ipa/raspberrypi/controller/algorithm.hpp > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp > @@ -37,7 +37,7 @@ public: > virtual void Resume() { paused_ = false; } > virtual void Read(boost::property_tree::ptree const ¶ms); > virtual void Initialise(); > - virtual void SwitchMode(CameraMode const &camera_mode); > + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); > virtual void Prepare(Metadata *image_metadata); > virtual void Process(StatisticsPtr &stats, Metadata *image_metadata); > Metadata &GetGlobalMetadata() const > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp > index 20dd4c7..7c4b04f 100644 > --- a/src/ipa/raspberrypi/controller/controller.cpp > +++ b/src/ipa/raspberrypi/controller/controller.cpp > @@ -56,11 +56,11 @@ void Controller::Initialise() > RPI_LOG("Controller finished"); > } > > -void Controller::SwitchMode(CameraMode const &camera_mode) > +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > { > RPI_LOG("Controller starting"); > for (auto &algo : algorithms_) > - algo->SwitchMode(camera_mode); > + algo->SwitchMode(camera_mode, metadata); > switch_mode_called_ = true; > RPI_LOG("Controller finished"); > } > diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp > index d685386..6ba9412 100644 > --- a/src/ipa/raspberrypi/controller/controller.hpp > +++ b/src/ipa/raspberrypi/controller/controller.hpp > @@ -39,7 +39,7 @@ public: > Algorithm *CreateAlgorithm(char const *name); > void Read(char const *filename); > void Initialise(); > - void SwitchMode(CameraMode const &camera_mode); > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); > void Prepare(Metadata *image_metadata); > void Process(StatisticsPtr stats, Metadata *image_metadata); > Metadata &GetGlobalMetadata(); > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > index a474287..c02b5ec 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name) > constraint_mode_name_ = constraint_mode_name; > } > > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > +{ > + // On a mode switch, it's possible the exposure profile could change, > + // so we run through the dividing up of exposure/gain again and > + // write the results into the metadata we've been given. > + if (status_.total_exposure_value) { > + housekeepConfig(); > + divvyupExposure(); > + writeAndFinish(metadata, false); > + } > +} > + > void Agc::Prepare(Metadata *image_metadata) > { > AgcStatus status; > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp > index dbcefba..9a7e89c 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp > @@ -75,6 +75,7 @@ public: > void SetMeteringMode(std::string const &metering_mode_name) override; > void SetExposureMode(std::string const &exposure_mode_name) override; > void SetConstraintMode(std::string const &contraint_mode_name) override; > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > void Prepare(Metadata *image_metadata) override; > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > index 821a0ca..76e2f04 100644 > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > @@ -173,8 +173,10 @@ void Alsc::Initialise() > lambda_r_[i] = lambda_b_[i] = 1.0; > } > > -void Alsc::SwitchMode(CameraMode const &camera_mode) > +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > { > + (void)metadata; > + > // There's a bit of a question what we should do if the "crop" of the > // camera mode has changed. Any calculation currently in flight would > // not be useful to the new mode, so arguably we should abort it, and > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > index c8ed3d2..3806257 100644 > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > @@ -50,7 +50,7 @@ public: > ~Alsc(); > char const *Name() const override; > void Initialise() override; > - void SwitchMode(CameraMode const &camera_mode) override; > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > void Read(boost::property_tree::ptree const ¶ms) override; > void Prepare(Metadata *image_metadata) override; > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp > index 2209d79..2cafde3 100644 > --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp > @@ -27,8 +27,10 @@ char const *Noise::Name() const > return NAME; > } > > -void Noise::SwitchMode(CameraMode const &camera_mode) > +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > { > + (void)metadata; > + > // For example, we would expect a 2x2 binned mode to have a "noise > // factor" of sqrt(2x2) = 2. (can't be less than one, right?) > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp > index 51d46a3..25bf188 100644 > --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp > @@ -18,7 +18,7 @@ class Noise : public Algorithm > public: > Noise(Controller *controller); > char const *Name() const override; > - void SwitchMode(CameraMode const &camera_mode) override; > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > void Read(boost::property_tree::ptree const ¶ms) override; > void Prepare(Metadata *image_metadata) override; > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > index 1f07bb6..086952f 100644 > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > @@ -26,8 +26,10 @@ char const *Sharpen::Name() const > return NAME; > } > > -void Sharpen::SwitchMode(CameraMode const &camera_mode) > +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > { > + (void)metadata; > + > // can't be less than one, right? > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > } > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > index 3b0d680..f871aa6 100644 > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > @@ -18,7 +18,7 @@ class Sharpen : public Algorithm > public: > Sharpen(Controller *controller); > char const *Name() const override; > - void SwitchMode(CameraMode const &camera_mode) override; > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > void Read(boost::property_tree::ptree const ¶ms) override; > void Prepare(Metadata *image_metadata) override; > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 9669f21..42c84b1 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo, > mistrust_count_ = helper_->MistrustFramesStartup(); > } > > + struct AgcStatus agcStatus; > + /* These zero values mean not program anything (unless overwritten). */ > + agcStatus.shutter_time = 0.0; > + agcStatus.analogue_gain = 0.0; > + > if (!controllerInit_) { > /* Load the tuning file for this sensor. */ > controller_.Read(tuningFile_.c_str()); > controller_.Initialise(); > controllerInit_ = true; > > - /* Calculate initial values for gain and exposure. */ > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN); > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME); > - > - ControlList ctrls(unicam_ctrls_); > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code); > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines); > - > - IPAOperationData op; > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED; > - op.controls.push_back(ctrls); > - queueFrameAction.emit(0, op); > + /* Supply initial values for gain and exposure. */ > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME; > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN; > } > > - controller_.SwitchMode(mode_); > + RPi::Metadata metadata; > + controller_.SwitchMode(mode_, &metadata); > + > + /* SwitchMode may supply updated exposure/gain values to use. */ > + metadata.Get("agc.status", agcStatus); > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) > + applyAGC(&agcStatus); > > lastMode_ = mode_; > } > -- > 2.20.1 >
Hi David, Thank you for the patch, and sorry for the late reply. On Mon, Jun 15, 2020 at 04:20:58PM +0100, David Plowman wrote: > Hi > > I was wondering what folks wanted to do about reviewing patches like > this one, when they're very much in the domain of the Raspberry Pi > IPAs. Do we need a different procedure for them, do you think? It's indeed difficult for us to review such patches, as they touch code we're not familiar with. There's however value in upholding at least part of the existing process. Sending the patches to the list is necessary for external review, but also serves as a way to notify subscribers of what's going on. There may be people reading the patches to educate themselves on the topic, even if they stay silent (and I certainly want to acquire more knowledge about the Raspberry Pi IPA implementation, so I'm in that category). Posting all patches to the list also creates a single submission mechanism, which I would like to preserve. When it comes to mandatory review, if nobody considers themselves able to review a patch, then review should probably not be mandatory. In this specific case, it would however be nice if someone else from Raspberry Pi could review the code, provided there's at least another developer who has enough expertise on the topic. The reason is simply that it's too easy for bugs to creep in, another pair of eyeballs at least glancing through the code is helpful. If that's not possible, so be it, and I'm fine accepting unreviewed code in that case. There's one part of the process that needs to be updated though. We usually apply patches once we have reviewed them and consider them ready. If nobody reviews a particular patch, then this will never happen. One option would be for you to send a pull request once you consider that the code is ready for integration, and you don't expect any other review. If sending a pull request is too complicated, I'm also fine with just a ping by e-mail (I may reconsider that in the future if the volume of patches gets much larger, but for the foreseable future I don't think it will be an issue). Would this work for you ? This being said, please see my review below :-) > On Wed, 10 Jun 2020 at 15:24, David Plowman wrote: > > > > When the sensor is switched into a different mode the line > > timings may change, yet the V4L2 driver remembers the number of > > exposure lines, not the total time. So this will change "under > > our feet". > > > > These patches extend the IPA handling of the camera mode switch s/These patches extend/This patch extends/ unless I've missed a patch :-) > > so that correct exposure values are recalculated for the new mode > > and written to the sensor before it starts streaming again. They > > also allow the IPA to alter how the total exposure is divided > > between actual exposure and analogue gain, according to the > > selected exposure profile in the camera tuning. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/algorithm.cpp | 3 +- > > src/ipa/raspberrypi/controller/algorithm.hpp | 2 +- > > src/ipa/raspberrypi/controller/controller.cpp | 4 +-- > > src/ipa/raspberrypi/controller/controller.hpp | 2 +- > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++ > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 + > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++- > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +- > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++- > > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +- > > .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++- > > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++--------- > > 13 files changed, 46 insertions(+), 24 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp > > index 9bd3df8..55cb201 100644 > > --- a/src/ipa/raspberrypi/controller/algorithm.cpp > > +++ b/src/ipa/raspberrypi/controller/algorithm.cpp > > @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms) > > > > void Algorithm::Initialise() {} > > > > -void Algorithm::SwitchMode(CameraMode const &camera_mode) > > +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > { > > (void)camera_mode; > > + (void)metadata; We don't enable unused arguments warnings today, so this isn't strictly necessary, but we plan to enable them in the future. We haven't decided yet how to do so. One option would be to remove the parameter name from the function definition: void Algorithm::SwitchMode(CameraMode const &, Metadata *) { } There's however a concern that this would make the code less readable, at least for inline functions, as the only source of information of expected parameter usage would be lost. We have considered void Algorithm::SwitchMode(CameraMode const & /*camera_mode*/, Metadata * /*metadata*/) { } but this was disliked by most people (I don't mind it much personally, even if the result doesn't look extremely pretty). Another option would be to use [[maybe_unused]] once we switch to C++17 (which we very likely will at some point, but we don't know when yet). Conclusion: no need to change your patch, but if you have any feedback on those options, feel free to voice it. > > } > > > > void Algorithm::Prepare(Metadata *image_metadata) > > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp > > index b82c184..187c50c 100644 > > --- a/src/ipa/raspberrypi/controller/algorithm.hpp > > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp > > @@ -37,7 +37,7 @@ public: > > virtual void Resume() { paused_ = false; } > > virtual void Read(boost::property_tree::ptree const ¶ms); > > virtual void Initialise(); > > - virtual void SwitchMode(CameraMode const &camera_mode); > > + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); On a general note, it would be nice to document the Algorithm class API, but that's a separate issue. > > virtual void Prepare(Metadata *image_metadata); > > virtual void Process(StatisticsPtr &stats, Metadata *image_metadata); > > Metadata &GetGlobalMetadata() const > > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp > > index 20dd4c7..7c4b04f 100644 > > --- a/src/ipa/raspberrypi/controller/controller.cpp > > +++ b/src/ipa/raspberrypi/controller/controller.cpp > > @@ -56,11 +56,11 @@ void Controller::Initialise() > > RPI_LOG("Controller finished"); > > } > > > > -void Controller::SwitchMode(CameraMode const &camera_mode) > > +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > { > > RPI_LOG("Controller starting"); > > for (auto &algo : algorithms_) > > - algo->SwitchMode(camera_mode); > > + algo->SwitchMode(camera_mode, metadata); > > switch_mode_called_ = true; > > RPI_LOG("Controller finished"); > > } > > diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp > > index d685386..6ba9412 100644 > > --- a/src/ipa/raspberrypi/controller/controller.hpp > > +++ b/src/ipa/raspberrypi/controller/controller.hpp > > @@ -39,7 +39,7 @@ public: > > Algorithm *CreateAlgorithm(char const *name); > > void Read(char const *filename); > > void Initialise(); > > - void SwitchMode(CameraMode const &camera_mode); > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); > > void Prepare(Metadata *image_metadata); > > void Process(StatisticsPtr stats, Metadata *image_metadata); > > Metadata &GetGlobalMetadata(); > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > index a474287..c02b5ec 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name) > > constraint_mode_name_ = constraint_mode_name; > > } > > > > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > +{ > > + // On a mode switch, it's possible the exposure profile could change, > > + // so we run through the dividing up of exposure/gain again and > > + // write the results into the metadata we've been given. > > + if (status_.total_exposure_value) { > > + housekeepConfig(); > > + divvyupExposure(); I had to use a dictionary to understand divvyup :-) > > + writeAndFinish(metadata, false); > > + } This part I have trouble reviewing, so I'll just trust you. > > +} I think I would have split this patch in two parts, a first patch that adds the metadata parameter, and a second patch that uses it here and in IPARPi::configure(). There's no need to resubmit the patch for this reason, but I think that it would ease review, especially for the IPA module given our lack of familiarity with the code base. > > + > > void Agc::Prepare(Metadata *image_metadata) > > { > > AgcStatus status; > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > index dbcefba..9a7e89c 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > @@ -75,6 +75,7 @@ public: > > void SetMeteringMode(std::string const &metering_mode_name) override; > > void SetExposureMode(std::string const &exposure_mode_name) override; > > void SetConstraintMode(std::string const &contraint_mode_name) override; > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > void Prepare(Metadata *image_metadata) override; > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > index 821a0ca..76e2f04 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > @@ -173,8 +173,10 @@ void Alsc::Initialise() > > lambda_r_[i] = lambda_b_[i] = 1.0; > > } > > > > -void Alsc::SwitchMode(CameraMode const &camera_mode) > > +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > { > > + (void)metadata; > > + > > // There's a bit of a question what we should do if the "crop" of the > > // camera mode has changed. Any calculation currently in flight would > > // not be useful to the new mode, so arguably we should abort it, and > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > index c8ed3d2..3806257 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > @@ -50,7 +50,7 @@ public: > > ~Alsc(); > > char const *Name() const override; > > void Initialise() override; > > - void SwitchMode(CameraMode const &camera_mode) override; > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > void Read(boost::property_tree::ptree const ¶ms) override; > > void Prepare(Metadata *image_metadata) override; > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp > > index 2209d79..2cafde3 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp > > @@ -27,8 +27,10 @@ char const *Noise::Name() const > > return NAME; > > } > > > > -void Noise::SwitchMode(CameraMode const &camera_mode) > > +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > { > > + (void)metadata; > > + > > // For example, we would expect a 2x2 binned mode to have a "noise > > // factor" of sqrt(2x2) = 2. (can't be less than one, right?) > > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp > > index 51d46a3..25bf188 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp > > @@ -18,7 +18,7 @@ class Noise : public Algorithm > > public: > > Noise(Controller *controller); > > char const *Name() const override; > > - void SwitchMode(CameraMode const &camera_mode) override; > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > void Read(boost::property_tree::ptree const ¶ms) override; > > void Prepare(Metadata *image_metadata) override; > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > index 1f07bb6..086952f 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > @@ -26,8 +26,10 @@ char const *Sharpen::Name() const > > return NAME; > > } > > > > -void Sharpen::SwitchMode(CameraMode const &camera_mode) > > +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > { > > + (void)metadata; > > + > > // can't be less than one, right? > > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > > } > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > index 3b0d680..f871aa6 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > @@ -18,7 +18,7 @@ class Sharpen : public Algorithm > > public: > > Sharpen(Controller *controller); > > char const *Name() const override; > > - void SwitchMode(CameraMode const &camera_mode) override; > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > void Read(boost::property_tree::ptree const ¶ms) override; > > void Prepare(Metadata *image_metadata) override; > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 9669f21..42c84b1 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo, > > mistrust_count_ = helper_->MistrustFramesStartup(); > > } > > > > + struct AgcStatus agcStatus; > > + /* These zero values mean not program anything (unless overwritten). */ > > + agcStatus.shutter_time = 0.0; > > + agcStatus.analogue_gain = 0.0; > > + > > if (!controllerInit_) { > > /* Load the tuning file for this sensor. */ > > controller_.Read(tuningFile_.c_str()); > > controller_.Initialise(); > > controllerInit_ = true; > > > > - /* Calculate initial values for gain and exposure. */ > > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN); > > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME); > > - > > - ControlList ctrls(unicam_ctrls_); > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code); > > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines); > > - > > - IPAOperationData op; > > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED; > > - op.controls.push_back(ctrls); > > - queueFrameAction.emit(0, op); > > + /* Supply initial values for gain and exposure. */ > > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME; > > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN; > > } > > > > - controller_.SwitchMode(mode_); > > + RPi::Metadata metadata; > > + controller_.SwitchMode(mode_, &metadata); > > + > > + /* SwitchMode may supply updated exposure/gain values to use. */ > > + metadata.Get("agc.status", agcStatus); > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) > > + applyAGC(&agcStatus); There's something I don't quite get. You mention in the commit message that the issue is caused by the length of the line being changed. Wouldn't it be enough to move the control setting code outside of the !controllerInit_ section ? Or is my understand correct, and the larger rework here is related to the last part of the commit message, the recalculation of the split between exposure time and gain ? With the small fix to the commit message, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Please let me know if you would like to submit a new version with additional changes based on the comments above, or if you would like me to apply this one. > > > > lastMode_ = mode_; > > }
Hi Laurent On Thu, 18 Jun 2020 at 03:10, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch, and sorry for the late reply. > > On Mon, Jun 15, 2020 at 04:20:58PM +0100, David Plowman wrote: > > Hi > > > > I was wondering what folks wanted to do about reviewing patches like > > this one, when they're very much in the domain of the Raspberry Pi > > IPAs. Do we need a different procedure for them, do you think? > > It's indeed difficult for us to review such patches, as they touch code > we're not familiar with. There's however value in upholding at least > part of the existing process. > > Sending the patches to the list is necessary for external review, but > also serves as a way to notify subscribers of what's going on. There may > be people reading the patches to educate themselves on the topic, even > if they stay silent (and I certainly want to acquire more knowledge > about the Raspberry Pi IPA implementation, so I'm in that category). > Posting all patches to the list also creates a single submission > mechanism, which I would like to preserve. > > When it comes to mandatory review, if nobody considers themselves able > to review a patch, then review should probably not be mandatory. In this > specific case, it would however be nice if someone else from Raspberry > Pi could review the code, provided there's at least another developer > who has enough expertise on the topic. The reason is simply that it's > too easy for bugs to creep in, another pair of eyeballs at least > glancing through the code is helpful. If that's not possible, so be it, > and I'm fine accepting unreviewed code in that case. > > There's one part of the process that needs to be updated though. We > usually apply patches once we have reviewed them and consider them > ready. If nobody reviews a particular patch, then this will never > happen. One option would be for you to send a pull request once you > consider that the code is ready for integration, and you don't expect > any other review. If sending a pull request is too complicated, I'm also > fine with just a ping by e-mail (I may reconsider that in the future if > the volume of patches gets much larger, but for the foreseable future I > don't think it will be an issue). > > Would this work for you ? Thanks for your thoughts. I think this works for us. It seems sensible that commits in this area should be reviewed by at least one other Pi person. > > This being said, please see my review below :-) > > > On Wed, 10 Jun 2020 at 15:24, David Plowman wrote: > > > > > > When the sensor is switched into a different mode the line > > > timings may change, yet the V4L2 driver remembers the number of > > > exposure lines, not the total time. So this will change "under > > > our feet". > > > > > > These patches extend the IPA handling of the camera mode switch > > s/These patches extend/This patch extends/ > > unless I've missed a patch :-) > > > > so that correct exposure values are recalculated for the new mode > > > and written to the sensor before it starts streaming again. They > > > also allow the IPA to alter how the total exposure is divided > > > between actual exposure and analogue gain, according to the > > > selected exposure profile in the camera tuning. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/raspberrypi/controller/algorithm.cpp | 3 +- > > > src/ipa/raspberrypi/controller/algorithm.hpp | 2 +- > > > src/ipa/raspberrypi/controller/controller.cpp | 4 +-- > > > src/ipa/raspberrypi/controller/controller.hpp | 2 +- > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++ > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 + > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++- > > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +- > > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++- > > > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +- > > > .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++- > > > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +- > > > src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++--------- > > > 13 files changed, 46 insertions(+), 24 deletions(-) > > > > > > diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp > > > index 9bd3df8..55cb201 100644 > > > --- a/src/ipa/raspberrypi/controller/algorithm.cpp > > > +++ b/src/ipa/raspberrypi/controller/algorithm.cpp > > > @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms) > > > > > > void Algorithm::Initialise() {} > > > > > > -void Algorithm::SwitchMode(CameraMode const &camera_mode) > > > +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > { > > > (void)camera_mode; > > > + (void)metadata; > > We don't enable unused arguments warnings today, so this isn't strictly > necessary, but we plan to enable them in the future. We haven't decided > yet how to do so. One option would be to remove the parameter name from > the function definition: > > void Algorithm::SwitchMode(CameraMode const &, Metadata *) > { > } > > There's however a concern that this would make the code less readable, > at least for inline functions, as the only source of information of > expected parameter usage would be lost. We have considered > > void Algorithm::SwitchMode(CameraMode const & /*camera_mode*/, Metadata * /*metadata*/) > { > } > > but this was disliked by most people (I don't mind it much personally, > even if the result doesn't look extremely pretty). Another option would > be to use [[maybe_unused]] once we switch to C++17 (which we very likely > will at some point, but we don't know when yet). > > Conclusion: no need to change your patch, but if you have any feedback > on those options, feel free to voice it. Tricky one. For now I would agree that "(void)parameter;" is no worse than anything else! > > > > } > > > > > > void Algorithm::Prepare(Metadata *image_metadata) > > > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp > > > index b82c184..187c50c 100644 > > > --- a/src/ipa/raspberrypi/controller/algorithm.hpp > > > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp > > > @@ -37,7 +37,7 @@ public: > > > virtual void Resume() { paused_ = false; } > > > virtual void Read(boost::property_tree::ptree const ¶ms); > > > virtual void Initialise(); > > > - virtual void SwitchMode(CameraMode const &camera_mode); > > > + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); > > On a general note, it would be nice to document the Algorithm class API, > but that's a separate issue. Certainly. There's obviously documentation in the big Pi/libcamera pdf, but you have in mind something more in the code, in the header file itself? If you can point me at what you consider a good example to follow, then I'll definitely have a go! > > > > virtual void Prepare(Metadata *image_metadata); > > > virtual void Process(StatisticsPtr &stats, Metadata *image_metadata); > > > Metadata &GetGlobalMetadata() const > > > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp > > > index 20dd4c7..7c4b04f 100644 > > > --- a/src/ipa/raspberrypi/controller/controller.cpp > > > +++ b/src/ipa/raspberrypi/controller/controller.cpp > > > @@ -56,11 +56,11 @@ void Controller::Initialise() > > > RPI_LOG("Controller finished"); > > > } > > > > > > -void Controller::SwitchMode(CameraMode const &camera_mode) > > > +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > { > > > RPI_LOG("Controller starting"); > > > for (auto &algo : algorithms_) > > > - algo->SwitchMode(camera_mode); > > > + algo->SwitchMode(camera_mode, metadata); > > > switch_mode_called_ = true; > > > RPI_LOG("Controller finished"); > > > } > > > diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp > > > index d685386..6ba9412 100644 > > > --- a/src/ipa/raspberrypi/controller/controller.hpp > > > +++ b/src/ipa/raspberrypi/controller/controller.hpp > > > @@ -39,7 +39,7 @@ public: > > > Algorithm *CreateAlgorithm(char const *name); > > > void Read(char const *filename); > > > void Initialise(); > > > - void SwitchMode(CameraMode const &camera_mode); > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); > > > void Prepare(Metadata *image_metadata); > > > void Process(StatisticsPtr stats, Metadata *image_metadata); > > > Metadata &GetGlobalMetadata(); > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > index a474287..c02b5ec 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name) > > > constraint_mode_name_ = constraint_mode_name; > > > } > > > > > > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > +{ > > > + // On a mode switch, it's possible the exposure profile could change, > > > + // so we run through the dividing up of exposure/gain again and > > > + // write the results into the metadata we've been given. > > > + if (status_.total_exposure_value) { > > > + housekeepConfig(); > > > + divvyupExposure(); > > I had to use a dictionary to understand divvyup :-) Sorry... (and there's a serious point in that using unfamiliar words is unhelpful now that this code is public) > > > > + writeAndFinish(metadata, false); > > > + } > > This part I have trouble reviewing, so I'll just trust you. > > > > +} > > I think I would have split this patch in two parts, a first patch that > adds the metadata parameter, and a second patch that uses it here and in > IPARPi::configure(). There's no need to resubmit the patch for this > reason, but I think that it would ease review, especially for the IPA > module given our lack of familiarity with the code base. Understood, I see that now that you mention it. (Still learning!) > > > > + > > > void Agc::Prepare(Metadata *image_metadata) > > > { > > > AgcStatus status; > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > > index dbcefba..9a7e89c 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > > @@ -75,6 +75,7 @@ public: > > > void SetMeteringMode(std::string const &metering_mode_name) override; > > > void SetExposureMode(std::string const &exposure_mode_name) override; > > > void SetConstraintMode(std::string const &contraint_mode_name) override; > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > void Prepare(Metadata *image_metadata) override; > > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > > > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > > index 821a0ca..76e2f04 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > > @@ -173,8 +173,10 @@ void Alsc::Initialise() > > > lambda_r_[i] = lambda_b_[i] = 1.0; > > > } > > > > > > -void Alsc::SwitchMode(CameraMode const &camera_mode) > > > +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > { > > > + (void)metadata; > > > + > > > // There's a bit of a question what we should do if the "crop" of the > > > // camera mode has changed. Any calculation currently in flight would > > > // not be useful to the new mode, so arguably we should abort it, and > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > > index c8ed3d2..3806257 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > > @@ -50,7 +50,7 @@ public: > > > ~Alsc(); > > > char const *Name() const override; > > > void Initialise() override; > > > - void SwitchMode(CameraMode const &camera_mode) override; > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > void Read(boost::property_tree::ptree const ¶ms) override; > > > void Prepare(Metadata *image_metadata) override; > > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp > > > index 2209d79..2cafde3 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp > > > @@ -27,8 +27,10 @@ char const *Noise::Name() const > > > return NAME; > > > } > > > > > > -void Noise::SwitchMode(CameraMode const &camera_mode) > > > +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > { > > > + (void)metadata; > > > + > > > // For example, we would expect a 2x2 binned mode to have a "noise > > > // factor" of sqrt(2x2) = 2. (can't be less than one, right?) > > > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp > > > index 51d46a3..25bf188 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp > > > @@ -18,7 +18,7 @@ class Noise : public Algorithm > > > public: > > > Noise(Controller *controller); > > > char const *Name() const override; > > > - void SwitchMode(CameraMode const &camera_mode) override; > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > void Read(boost::property_tree::ptree const ¶ms) override; > > > void Prepare(Metadata *image_metadata) override; > > > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > > index 1f07bb6..086952f 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > > @@ -26,8 +26,10 @@ char const *Sharpen::Name() const > > > return NAME; > > > } > > > > > > -void Sharpen::SwitchMode(CameraMode const &camera_mode) > > > +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > { > > > + (void)metadata; > > > + > > > // can't be less than one, right? > > > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > > > } > > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > > index 3b0d680..f871aa6 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > > @@ -18,7 +18,7 @@ class Sharpen : public Algorithm > > > public: > > > Sharpen(Controller *controller); > > > char const *Name() const override; > > > - void SwitchMode(CameraMode const &camera_mode) override; > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > void Read(boost::property_tree::ptree const ¶ms) override; > > > void Prepare(Metadata *image_metadata) override; > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 9669f21..42c84b1 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo, > > > mistrust_count_ = helper_->MistrustFramesStartup(); > > > } > > > > > > + struct AgcStatus agcStatus; > > > + /* These zero values mean not program anything (unless overwritten). */ > > > + agcStatus.shutter_time = 0.0; > > > + agcStatus.analogue_gain = 0.0; > > > + > > > if (!controllerInit_) { > > > /* Load the tuning file for this sensor. */ > > > controller_.Read(tuningFile_.c_str()); > > > controller_.Initialise(); > > > controllerInit_ = true; > > > > > > - /* Calculate initial values for gain and exposure. */ > > > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN); > > > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME); > > > - > > > - ControlList ctrls(unicam_ctrls_); > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code); > > > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines); > > > - > > > - IPAOperationData op; > > > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED; > > > - op.controls.push_back(ctrls); > > > - queueFrameAction.emit(0, op); > > > + /* Supply initial values for gain and exposure. */ > > > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME; > > > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN; > > > } > > > > > > - controller_.SwitchMode(mode_); > > > + RPi::Metadata metadata; > > > + controller_.SwitchMode(mode_, &metadata); > > > + > > > + /* SwitchMode may supply updated exposure/gain values to use. */ > > > + metadata.Get("agc.status", agcStatus); > > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) > > > + applyAGC(&agcStatus); > > There's something I don't quite get. You mention in the commit message > that the issue is caused by the length of the line being changed. > Wouldn't it be enough to move the control setting code outside of the > !controllerInit_ section ? Or is my understand correct, and the larger > rework here is related to the last part of the commit message, the > recalculation of the split between exposure time and gain ? Yes, that's right. > > With the small fix to the commit message, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Please let me know if you would like to submit a new version with > additional changes based on the comments above, or if you would like me > to apply this one. I'd quite like to submit a version 2, split into two separate commits as you suggested. I need the practice!! Best regards David > > > > > > > lastMode_ = mode_; > > > } > > -- > Regards, > > Laurent Pinchart
Hi David, On Thu, Jun 18, 2020 at 10:46:48AM +0100, David Plowman wrote: > On Thu, 18 Jun 2020 at 03:10, Laurent Pinchart wrote: > > On Mon, Jun 15, 2020 at 04:20:58PM +0100, David Plowman wrote: > > > Hi > > > > > > I was wondering what folks wanted to do about reviewing patches like > > > this one, when they're very much in the domain of the Raspberry Pi > > > IPAs. Do we need a different procedure for them, do you think? > > > > It's indeed difficult for us to review such patches, as they touch code > > we're not familiar with. There's however value in upholding at least > > part of the existing process. > > > > Sending the patches to the list is necessary for external review, but > > also serves as a way to notify subscribers of what's going on. There may > > be people reading the patches to educate themselves on the topic, even > > if they stay silent (and I certainly want to acquire more knowledge > > about the Raspberry Pi IPA implementation, so I'm in that category). > > Posting all patches to the list also creates a single submission > > mechanism, which I would like to preserve. > > > > When it comes to mandatory review, if nobody considers themselves able > > to review a patch, then review should probably not be mandatory. In this > > specific case, it would however be nice if someone else from Raspberry > > Pi could review the code, provided there's at least another developer > > who has enough expertise on the topic. The reason is simply that it's > > too easy for bugs to creep in, another pair of eyeballs at least > > glancing through the code is helpful. If that's not possible, so be it, > > and I'm fine accepting unreviewed code in that case. > > > > There's one part of the process that needs to be updated though. We > > usually apply patches once we have reviewed them and consider them > > ready. If nobody reviews a particular patch, then this will never > > happen. One option would be for you to send a pull request once you > > consider that the code is ready for integration, and you don't expect > > any other review. If sending a pull request is too complicated, I'm also > > fine with just a ping by e-mail (I may reconsider that in the future if > > the volume of patches gets much larger, but for the foreseable future I > > don't think it will be an issue). > > > > Would this work for you ? > > Thanks for your thoughts. I think this works for us. It seems > sensible that commits in this area should be reviewed by at > least one other Pi person. > > > This being said, please see my review below :-) > > > > > On Wed, 10 Jun 2020 at 15:24, David Plowman wrote: > > > > > > > > When the sensor is switched into a different mode the line > > > > timings may change, yet the V4L2 driver remembers the number of > > > > exposure lines, not the total time. So this will change "under > > > > our feet". > > > > > > > > These patches extend the IPA handling of the camera mode switch > > > > s/These patches extend/This patch extends/ > > > > unless I've missed a patch :-) > > > > > > so that correct exposure values are recalculated for the new mode > > > > and written to the sensor before it starts streaming again. They > > > > also allow the IPA to alter how the total exposure is divided > > > > between actual exposure and analogue gain, according to the > > > > selected exposure profile in the camera tuning. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/ipa/raspberrypi/controller/algorithm.cpp | 3 +- > > > > src/ipa/raspberrypi/controller/algorithm.hpp | 2 +- > > > > src/ipa/raspberrypi/controller/controller.cpp | 4 +-- > > > > src/ipa/raspberrypi/controller/controller.hpp | 2 +- > > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++ > > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 + > > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++- > > > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +- > > > > src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++- > > > > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +- > > > > .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++- > > > > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++--------- > > > > 13 files changed, 46 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp > > > > index 9bd3df8..55cb201 100644 > > > > --- a/src/ipa/raspberrypi/controller/algorithm.cpp > > > > +++ b/src/ipa/raspberrypi/controller/algorithm.cpp > > > > @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms) > > > > > > > > void Algorithm::Initialise() {} > > > > > > > > -void Algorithm::SwitchMode(CameraMode const &camera_mode) > > > > +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > > { > > > > (void)camera_mode; > > > > + (void)metadata; > > > > We don't enable unused arguments warnings today, so this isn't strictly > > necessary, but we plan to enable them in the future. We haven't decided > > yet how to do so. One option would be to remove the parameter name from > > the function definition: > > > > void Algorithm::SwitchMode(CameraMode const &, Metadata *) > > { > > } > > > > There's however a concern that this would make the code less readable, > > at least for inline functions, as the only source of information of > > expected parameter usage would be lost. We have considered > > > > void Algorithm::SwitchMode(CameraMode const & /*camera_mode*/, Metadata * /*metadata*/) > > { > > } > > > > but this was disliked by most people (I don't mind it much personally, > > even if the result doesn't look extremely pretty). Another option would > > be to use [[maybe_unused]] once we switch to C++17 (which we very likely > > will at some point, but we don't know when yet). > > > > Conclusion: no need to change your patch, but if you have any feedback > > on those options, feel free to voice it. > > Tricky one. For now I would agree that "(void)parameter;" is no > worse than anything else! > > > > > } > > > > > > > > void Algorithm::Prepare(Metadata *image_metadata) > > > > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp > > > > index b82c184..187c50c 100644 > > > > --- a/src/ipa/raspberrypi/controller/algorithm.hpp > > > > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp > > > > @@ -37,7 +37,7 @@ public: > > > > virtual void Resume() { paused_ = false; } > > > > virtual void Read(boost::property_tree::ptree const ¶ms); > > > > virtual void Initialise(); > > > > - virtual void SwitchMode(CameraMode const &camera_mode); > > > > + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); > > > > On a general note, it would be nice to document the Algorithm class API, > > but that's a separate issue. > > Certainly. There's obviously documentation in the big Pi/libcamera pdf, > but you have in mind something more in the code, in the header file > itself? If you can point me at what you consider a good example to > follow, then I'll definitely have a go! I'm thinking about doxygen comments, in the .cpp file, like we do for the libcamera core. > > > > virtual void Prepare(Metadata *image_metadata); > > > > virtual void Process(StatisticsPtr &stats, Metadata *image_metadata); > > > > Metadata &GetGlobalMetadata() const > > > > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp > > > > index 20dd4c7..7c4b04f 100644 > > > > --- a/src/ipa/raspberrypi/controller/controller.cpp > > > > +++ b/src/ipa/raspberrypi/controller/controller.cpp > > > > @@ -56,11 +56,11 @@ void Controller::Initialise() > > > > RPI_LOG("Controller finished"); > > > > } > > > > > > > > -void Controller::SwitchMode(CameraMode const &camera_mode) > > > > +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > > { > > > > RPI_LOG("Controller starting"); > > > > for (auto &algo : algorithms_) > > > > - algo->SwitchMode(camera_mode); > > > > + algo->SwitchMode(camera_mode, metadata); > > > > switch_mode_called_ = true; > > > > RPI_LOG("Controller finished"); > > > > } > > > > diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp > > > > index d685386..6ba9412 100644 > > > > --- a/src/ipa/raspberrypi/controller/controller.hpp > > > > +++ b/src/ipa/raspberrypi/controller/controller.hpp > > > > @@ -39,7 +39,7 @@ public: > > > > Algorithm *CreateAlgorithm(char const *name); > > > > void Read(char const *filename); > > > > void Initialise(); > > > > - void SwitchMode(CameraMode const &camera_mode); > > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); > > > > void Prepare(Metadata *image_metadata); > > > > void Process(StatisticsPtr stats, Metadata *image_metadata); > > > > Metadata &GetGlobalMetadata(); > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > index a474287..c02b5ec 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name) > > > > constraint_mode_name_ = constraint_mode_name; > > > > } > > > > > > > > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > > +{ > > > > + // On a mode switch, it's possible the exposure profile could change, > > > > + // so we run through the dividing up of exposure/gain again and > > > > + // write the results into the metadata we've been given. > > > > + if (status_.total_exposure_value) { > > > > + housekeepConfig(); > > > > + divvyupExposure(); > > > > I had to use a dictionary to understand divvyup :-) > > Sorry... (and there's a serious point in that using unfamiliar words > is unhelpful now that this code is public) > > > > > + writeAndFinish(metadata, false); > > > > + } > > > > This part I have trouble reviewing, so I'll just trust you. > > > > > > +} > > > > I think I would have split this patch in two parts, a first patch that > > adds the metadata parameter, and a second patch that uses it here and in > > IPARPi::configure(). There's no need to resubmit the patch for this > > reason, but I think that it would ease review, especially for the IPA > > module given our lack of familiarity with the code base. > > Understood, I see that now that you mention it. (Still learning!) > > > > > + > > > > void Agc::Prepare(Metadata *image_metadata) > > > > { > > > > AgcStatus status; > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > > > index dbcefba..9a7e89c 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp > > > > @@ -75,6 +75,7 @@ public: > > > > void SetMeteringMode(std::string const &metering_mode_name) override; > > > > void SetExposureMode(std::string const &exposure_mode_name) override; > > > > void SetConstraintMode(std::string const &contraint_mode_name) override; > > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > > void Prepare(Metadata *image_metadata) override; > > > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > > > index 821a0ca..76e2f04 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > > > > @@ -173,8 +173,10 @@ void Alsc::Initialise() > > > > lambda_r_[i] = lambda_b_[i] = 1.0; > > > > } > > > > > > > > -void Alsc::SwitchMode(CameraMode const &camera_mode) > > > > +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > > { > > > > + (void)metadata; > > > > + > > > > // There's a bit of a question what we should do if the "crop" of the > > > > // camera mode has changed. Any calculation currently in flight would > > > > // not be useful to the new mode, so arguably we should abort it, and > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > > > index c8ed3d2..3806257 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp > > > > @@ -50,7 +50,7 @@ public: > > > > ~Alsc(); > > > > char const *Name() const override; > > > > void Initialise() override; > > > > - void SwitchMode(CameraMode const &camera_mode) override; > > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > > void Read(boost::property_tree::ptree const ¶ms) override; > > > > void Prepare(Metadata *image_metadata) override; > > > > void Process(StatisticsPtr &stats, Metadata *image_metadata) override; > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp > > > > index 2209d79..2cafde3 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp > > > > @@ -27,8 +27,10 @@ char const *Noise::Name() const > > > > return NAME; > > > > } > > > > > > > > -void Noise::SwitchMode(CameraMode const &camera_mode) > > > > +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > > { > > > > + (void)metadata; > > > > + > > > > // For example, we would expect a 2x2 binned mode to have a "noise > > > > // factor" of sqrt(2x2) = 2. (can't be less than one, right?) > > > > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp > > > > index 51d46a3..25bf188 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp > > > > @@ -18,7 +18,7 @@ class Noise : public Algorithm > > > > public: > > > > Noise(Controller *controller); > > > > char const *Name() const override; > > > > - void SwitchMode(CameraMode const &camera_mode) override; > > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > > void Read(boost::property_tree::ptree const ¶ms) override; > > > > void Prepare(Metadata *image_metadata) override; > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > > > index 1f07bb6..086952f 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp > > > > @@ -26,8 +26,10 @@ char const *Sharpen::Name() const > > > > return NAME; > > > > } > > > > > > > > -void Sharpen::SwitchMode(CameraMode const &camera_mode) > > > > +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) > > > > { > > > > + (void)metadata; > > > > + > > > > // can't be less than one, right? > > > > mode_factor_ = std::max(1.0, camera_mode.noise_factor); > > > > } > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > > > index 3b0d680..f871aa6 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp > > > > @@ -18,7 +18,7 @@ class Sharpen : public Algorithm > > > > public: > > > > Sharpen(Controller *controller); > > > > char const *Name() const override; > > > > - void SwitchMode(CameraMode const &camera_mode) override; > > > > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; > > > > void Read(boost::property_tree::ptree const ¶ms) override; > > > > void Prepare(Metadata *image_metadata) override; > > > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index 9669f21..42c84b1 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo, > > > > mistrust_count_ = helper_->MistrustFramesStartup(); > > > > } > > > > > > > > + struct AgcStatus agcStatus; > > > > + /* These zero values mean not program anything (unless overwritten). */ > > > > + agcStatus.shutter_time = 0.0; > > > > + agcStatus.analogue_gain = 0.0; > > > > + > > > > if (!controllerInit_) { > > > > /* Load the tuning file for this sensor. */ > > > > controller_.Read(tuningFile_.c_str()); > > > > controller_.Initialise(); > > > > controllerInit_ = true; > > > > > > > > - /* Calculate initial values for gain and exposure. */ > > > > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN); > > > > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME); > > > > - > > > > - ControlList ctrls(unicam_ctrls_); > > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code); > > > > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines); > > > > - > > > > - IPAOperationData op; > > > > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED; > > > > - op.controls.push_back(ctrls); > > > > - queueFrameAction.emit(0, op); > > > > + /* Supply initial values for gain and exposure. */ > > > > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME; > > > > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN; > > > > } > > > > > > > > - controller_.SwitchMode(mode_); > > > > + RPi::Metadata metadata; > > > > + controller_.SwitchMode(mode_, &metadata); > > > > + > > > > + /* SwitchMode may supply updated exposure/gain values to use. */ > > > > + metadata.Get("agc.status", agcStatus); > > > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) > > > > + applyAGC(&agcStatus); > > > > There's something I don't quite get. You mention in the commit message > > that the issue is caused by the length of the line being changed. > > Wouldn't it be enough to move the control setting code outside of the > > !controllerInit_ section ? Or is my understand correct, and the larger > > rework here is related to the last part of the commit message, the > > recalculation of the split between exposure time and gain ? > > Yes, that's right. > > > With the small fix to the commit message, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Please let me know if you would like to submit a new version with > > additional changes based on the comments above, or if you would like me > > to apply this one. > > I'd quite like to submit a version 2, split into two separate commits as > you suggested. I need the practice!! Thanks :-) I'll review it now. > > > > > > > > lastMode_ = mode_; > > > > }
diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp index 9bd3df8..55cb201 100644 --- a/src/ipa/raspberrypi/controller/algorithm.cpp +++ b/src/ipa/raspberrypi/controller/algorithm.cpp @@ -16,9 +16,10 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms) void Algorithm::Initialise() {} -void Algorithm::SwitchMode(CameraMode const &camera_mode) +void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) { (void)camera_mode; + (void)metadata; } void Algorithm::Prepare(Metadata *image_metadata) diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp index b82c184..187c50c 100644 --- a/src/ipa/raspberrypi/controller/algorithm.hpp +++ b/src/ipa/raspberrypi/controller/algorithm.hpp @@ -37,7 +37,7 @@ public: virtual void Resume() { paused_ = false; } virtual void Read(boost::property_tree::ptree const ¶ms); virtual void Initialise(); - virtual void SwitchMode(CameraMode const &camera_mode); + virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); virtual void Prepare(Metadata *image_metadata); virtual void Process(StatisticsPtr &stats, Metadata *image_metadata); Metadata &GetGlobalMetadata() const diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp index 20dd4c7..7c4b04f 100644 --- a/src/ipa/raspberrypi/controller/controller.cpp +++ b/src/ipa/raspberrypi/controller/controller.cpp @@ -56,11 +56,11 @@ void Controller::Initialise() RPI_LOG("Controller finished"); } -void Controller::SwitchMode(CameraMode const &camera_mode) +void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) { RPI_LOG("Controller starting"); for (auto &algo : algorithms_) - algo->SwitchMode(camera_mode); + algo->SwitchMode(camera_mode, metadata); switch_mode_called_ = true; RPI_LOG("Controller finished"); } diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp index d685386..6ba9412 100644 --- a/src/ipa/raspberrypi/controller/controller.hpp +++ b/src/ipa/raspberrypi/controller/controller.hpp @@ -39,7 +39,7 @@ public: Algorithm *CreateAlgorithm(char const *name); void Read(char const *filename); void Initialise(); - void SwitchMode(CameraMode const &camera_mode); + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata); void Prepare(Metadata *image_metadata); void Process(StatisticsPtr stats, Metadata *image_metadata); Metadata &GetGlobalMetadata(); diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp index a474287..c02b5ec 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name) constraint_mode_name_ = constraint_mode_name; } +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) +{ + // On a mode switch, it's possible the exposure profile could change, + // so we run through the dividing up of exposure/gain again and + // write the results into the metadata we've been given. + if (status_.total_exposure_value) { + housekeepConfig(); + divvyupExposure(); + writeAndFinish(metadata, false); + } +} + void Agc::Prepare(Metadata *image_metadata) { AgcStatus status; diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp index dbcefba..9a7e89c 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp @@ -75,6 +75,7 @@ public: void SetMeteringMode(std::string const &metering_mode_name) override; void SetExposureMode(std::string const &exposure_mode_name) override; void SetConstraintMode(std::string const &contraint_mode_name) override; + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; void Prepare(Metadata *image_metadata) override; void Process(StatisticsPtr &stats, Metadata *image_metadata) override; diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp index 821a0ca..76e2f04 100644 --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp @@ -173,8 +173,10 @@ void Alsc::Initialise() lambda_r_[i] = lambda_b_[i] = 1.0; } -void Alsc::SwitchMode(CameraMode const &camera_mode) +void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) { + (void)metadata; + // There's a bit of a question what we should do if the "crop" of the // camera mode has changed. Any calculation currently in flight would // not be useful to the new mode, so arguably we should abort it, and diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp index c8ed3d2..3806257 100644 --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp @@ -50,7 +50,7 @@ public: ~Alsc(); char const *Name() const override; void Initialise() override; - void SwitchMode(CameraMode const &camera_mode) override; + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; void Read(boost::property_tree::ptree const ¶ms) override; void Prepare(Metadata *image_metadata) override; void Process(StatisticsPtr &stats, Metadata *image_metadata) override; diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp index 2209d79..2cafde3 100644 --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp @@ -27,8 +27,10 @@ char const *Noise::Name() const return NAME; } -void Noise::SwitchMode(CameraMode const &camera_mode) +void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) { + (void)metadata; + // For example, we would expect a 2x2 binned mode to have a "noise // factor" of sqrt(2x2) = 2. (can't be less than one, right?) mode_factor_ = std::max(1.0, camera_mode.noise_factor); diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp index 51d46a3..25bf188 100644 --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp @@ -18,7 +18,7 @@ class Noise : public Algorithm public: Noise(Controller *controller); char const *Name() const override; - void SwitchMode(CameraMode const &camera_mode) override; + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; void Read(boost::property_tree::ptree const ¶ms) override; void Prepare(Metadata *image_metadata) override; diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp index 1f07bb6..086952f 100644 --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp @@ -26,8 +26,10 @@ char const *Sharpen::Name() const return NAME; } -void Sharpen::SwitchMode(CameraMode const &camera_mode) +void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata) { + (void)metadata; + // can't be less than one, right? mode_factor_ = std::max(1.0, camera_mode.noise_factor); } diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp index 3b0d680..f871aa6 100644 --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp @@ -18,7 +18,7 @@ class Sharpen : public Algorithm public: Sharpen(Controller *controller); char const *Name() const override; - void SwitchMode(CameraMode const &camera_mode) override; + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override; void Read(boost::property_tree::ptree const ¶ms) override; void Prepare(Metadata *image_metadata) override; diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 9669f21..42c84b1 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -247,27 +247,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo, mistrust_count_ = helper_->MistrustFramesStartup(); } + struct AgcStatus agcStatus; + /* These zero values mean not program anything (unless overwritten). */ + agcStatus.shutter_time = 0.0; + agcStatus.analogue_gain = 0.0; + if (!controllerInit_) { /* Load the tuning file for this sensor. */ controller_.Read(tuningFile_.c_str()); controller_.Initialise(); controllerInit_ = true; - /* Calculate initial values for gain and exposure. */ - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN); - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME); - - ControlList ctrls(unicam_ctrls_); - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code); - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines); - - IPAOperationData op; - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED; - op.controls.push_back(ctrls); - queueFrameAction.emit(0, op); + /* Supply initial values for gain and exposure. */ + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME; + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN; } - controller_.SwitchMode(mode_); + RPi::Metadata metadata; + controller_.SwitchMode(mode_, &metadata); + + /* SwitchMode may supply updated exposure/gain values to use. */ + metadata.Get("agc.status", agcStatus); + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) + applyAGC(&agcStatus); lastMode_ = mode_; }
When the sensor is switched into a different mode the line timings may change, yet the V4L2 driver remembers the number of exposure lines, not the total time. So this will change "under our feet". These patches extend the IPA handling of the camera mode switch so that correct exposure values are recalculated for the new mode and written to the sensor before it starts streaming again. They also allow the IPA to alter how the total exposure is divided between actual exposure and analogue gain, according to the selected exposure profile in the camera tuning. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/raspberrypi/controller/algorithm.cpp | 3 +- src/ipa/raspberrypi/controller/algorithm.hpp | 2 +- src/ipa/raspberrypi/controller/controller.cpp | 4 +-- src/ipa/raspberrypi/controller/controller.hpp | 2 +- src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++ src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 + src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++- src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +- src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++- src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +- .../raspberrypi/controller/rpi/sharpen.cpp | 4 ++- .../raspberrypi/controller/rpi/sharpen.hpp | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 28 ++++++++++--------- 13 files changed, 46 insertions(+), 24 deletions(-)