[libcamera-devel] [PATCH] libcamera: raspberrypi: correct exposure values when camera mode changes
David Plowman
david.plowman at raspberrypi.com
Thu Jun 18 11:46:48 CEST 2020
Hi Laurent
On Thu, 18 Jun 2020 at 03:10, Laurent Pinchart
<laurent.pinchart at 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 at 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 at 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
More information about the libcamera-devel
mailing list