[libcamera-devel] [PATCH] ipa: raspberrypi: awb: Better handling of how we disable AWB
David Plowman
david.plowman at raspberrypi.com
Tue Feb 8 10:18:42 CET 2022
Hi Laurent, Naush
Thanks for your reviews.
On Mon, 7 Feb 2022 at 23:23, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Fri, Feb 04, 2022 at 09:24:35AM +0000, David Plowman wrote:
> > We now handle disabling ("pausing") AWB in the same way as
> > AEC/AGC. Instead of letting the pause flag be set so that the code
> > never runs at all, we instead fix the manual settings to the current
> > values (but continue to be called).
> >
> > The algorithm does not restart any calculations in this state, but
> > continues to add AWB metadata to every frame. Therefore certain other
> > algorithms that want to know it (CCM and ALSC, for example) can still
> > find it.
>
> That seems to be a good step forward.
>
> Given that the only algorithms to ever be paused are AGC and AWB, it
> seems that we could remove the IsPaused() function as it will always
> return false (hardcoded for AGC and AWB, and because Pause() is never
> called for all the other algorithms). This can be done on top.
That's an interesting thought, though I'm not quite sure about that
yet. For example, there are lots of other control algorithms that you
could legitimately pause, perhaps ALSC being the most obvious. If you
wanted absolutely to *guarantee* identical processing of all frames
you'd have to pause that too. Same goes for the "contrast" algorithm,
which is liable to make adaptive changes to the gamma curve. Both
these algos would work fine with the "default" pause implementation.
Now we don't have libcamera controls for doing any of that, so maybe
that's another one to think about!
>
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/controller/rpi/awb.cpp | 21 +++++++++++++++++++++
> > src/ipa/raspberrypi/controller/rpi/awb.hpp | 4 ++++
> > 2 files changed, 25 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > index 5cfd33a3..1ad912c7 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > @@ -172,6 +172,27 @@ void Awb::Initialise()
> > async_results_ = sync_results_;
> > }
> >
> > +bool Awb::IsPaused() const
> > +{
> > + return false;
> > +}
> > +
> > +void Awb::Pause()
> > +{
> > + // "Pause" by fixing everything to the most recent values.
> > + manual_r_ = sync_results_.gain_r = prev_sync_results_.gain_r;
> > + manual_b_ = sync_results_.gain_b = prev_sync_results_.gain_b;
> > + sync_results_.gain_g = prev_sync_results_.gain_g;
> > + sync_results_.gain_g = prev_sync_results_.gain_g;
>
> Once should be enough :-)
Years of programming has made me paranoid! I'll submit a v2...
Thanks
David
>
> > + sync_results_.temperature_K = prev_sync_results_.temperature_K;
> > +}
> > +
> > +void Awb::Resume()
> > +{
> > + manual_r_ = 0.0;
> > + manual_b_ = 0.0;
> > +}
> > +
> > unsigned int Awb::GetConvergenceFrames() const
> > {
> > // If not in auto mode, there is no convergence
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > index 8af1f27c..ac3dca6f 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > @@ -83,6 +83,10 @@ public:
> > char const *Name() const override;
> > void Initialise() override;
> > void Read(boost::property_tree::ptree const ¶ms) override;
> > + // AWB handles "pausing" for itself.
> > + bool IsPaused() const override;
> > + void Pause() override;
> > + void Resume() override;
> > unsigned int GetConvergenceFrames() const override;
> > void SetMode(std::string const &name) override;
> > void SetManualGains(double manual_r, double manual_b) override;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list