[libcamera-devel] [PATCH] ipa: raspberrypi: awb: Better handling of how we disable AWB

[Laurent Pinchart]

Hi David,

On Tue, Feb 08, 2022 at 09:18:42AM +0000, David Plowman wrote:
> On Mon, 7 Feb 2022 at 23:23, Laurent Pinchart wrote:
> > 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!

There may be a need at that point to also keep those algorithms
"running" to produce metadata. I agree that we can consider this later
though.

> > > 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...
> 
> > > +     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 &params) 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