[libcamera-devel] [PATCH] libcamera: raspberrypi: correct exposure values when camera mode changes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 18 04:10:00 CEST 2020
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 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.
> > }
> >
> > 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 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.
> >
> > lastMode_ = mode_;
> > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list