[libcamera-devel] [PATCH] ipa: rpi: awb: Make is possible to set the colour temperature directly

[Kieran Bingham]

Hi Naush,

Are you happy with this patch?


Quoting Kieran Bingham (2023-11-20 14:57:49)
> Quoting David Plowman via libcamera-devel (2023-11-10 16:14:33)
> > ColourTemperature is now exported as a writable control so that
> > algorithms can set it directly. The AWB algorithm class now requires a
> > method to be provided to perform this operation. The method should
> > clamp the passed value to the calibrated range known to the algorithm.
> > 
> > The default range is set very wide to cover all conceivable future AWB
> > calibrations. It will always be clamped before use.
> > 
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp        | 20 ++++++++++++++++++++
> >  src/ipa/rpi/controller/awb_algorithm.h |  1 +
> >  src/ipa/rpi/controller/rpi/awb.cpp     | 18 ++++++++++++++++++
> >  src/ipa/rpi/controller/rpi/awb.h       |  1 +
> >  4 files changed, 40 insertions(+)
> > 
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index 97a32522..cd1d6e3d 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -80,6 +80,7 @@ const ControlInfoMap::Map ipaColourControls{
> >         { &controls::AwbEnable, ControlInfo(false, true) },
> >         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> >         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > +       { &controls::ColourTemperature, ControlInfo(100, 100000) },
> >         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> >  };
> >  
> > @@ -972,6 +973,25 @@ void IpaBase::applyControls(const ControlList &controls)
> >                         break;
> >                 }
> >  
> > +               case controls::COLOUR_TEMPERATURE: {
> > +                       /* Silently ignore this control for a mono sensor. */
> > +                       if (monoSensor_)
> > +                               break;
> 
> I wonder if this is where the construction of the control info map
> should be handled / supported by the algorithms so they can populate it
> correctly / dynamically based on what they can handle. But that's not
> for this patch - and I suspect this is ok.
> 
> 
> > +
> > +                       auto temperatureK = ctrl.second.get<int32_t>();
> > +                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > +                               controller_.getAlgorithm("awb"));
> > +                       if (!awb) {
> > +                               LOG(IPARPI, Warning)
> > +                                       << "Could not set COLOUR_TEMPERATURE - no AWB algorithm";
> > +                               break;
> 
> I guess it would also prevent exposing a control when there's no
> algorithm to implement/support it though. Still - for now this seems
> like all that could be done here then.
> 
> 
> But otherwise
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Naush ... are you happy with this patch ?

> 
> > +                       }
> > +
> > +                       awb->setColourTemperature(temperatureK);
> > +                       /* This metadata will get reported back automatically. */
> > +                       break;
> > +               }
> > +
> >                 case controls::BRIGHTNESS: {
> >                         RPiController::ContrastAlgorithm *contrast = dynamic_cast<RPiController::ContrastAlgorithm *>(
> >                                 controller_.getAlgorithm("contrast"));
> > diff --git a/src/ipa/rpi/controller/awb_algorithm.h b/src/ipa/rpi/controller/awb_algorithm.h
> > index 8462c4db..d966dfa8 100644
> > --- a/src/ipa/rpi/controller/awb_algorithm.h
> > +++ b/src/ipa/rpi/controller/awb_algorithm.h
> > @@ -18,6 +18,7 @@ public:
> >         virtual unsigned int getConvergenceFrames() const = 0;
> >         virtual void setMode(std::string const &modeName) = 0;
> >         virtual void setManualGains(double manualR, double manualB) = 0;
> > +       virtual void setColourTemperature(double temperatureK) = 0;
> >         virtual void enableAuto() = 0;
> >         virtual void disableAuto() = 0;
> >  };
> > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> > index 5ae0c2fa..0918e20d 100644
> > --- a/src/ipa/rpi/controller/rpi/awb.cpp
> > +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> > @@ -275,6 +275,24 @@ void Awb::setManualGains(double manualR, double manualB)
> >         }
> >  }
> >  
> > +void Awb::setColourTemperature(double temperatureK)
> > +{
> > +       if (!config_.bayes) {
> > +               LOG(RPiAwb, Warning) << "AWB uncalibrated - cannot set colour temperature";
> > +               return;
> > +       }
> > +
> > +       temperatureK = config_.ctR.domain().clip(temperatureK);
> > +       manualR_ = 1 / config_.ctR.eval(temperatureK);
> > +       manualB_ = 1 / config_.ctB.eval(temperatureK);
> > +
> > +       syncResults_.temperatureK = temperatureK;
> > +       syncResults_.gainR = manualR_;
> > +       syncResults_.gainG = 1.0;
> > +       syncResults_.gainB = manualB_;
> > +       prevSyncResults_ = syncResults_;
> > +}
> > +
> >  void Awb::switchMode([[maybe_unused]] CameraMode const &cameraMode,
> >                      Metadata *metadata)
> >  {
> > diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h
> > index e7d49cd8..dbd79eda 100644
> > --- a/src/ipa/rpi/controller/rpi/awb.h
> > +++ b/src/ipa/rpi/controller/rpi/awb.h
> > @@ -97,6 +97,7 @@ public:
> >         unsigned int getConvergenceFrames() const override;
> >         void setMode(std::string const &name) override;
> >         void setManualGains(double manualR, double manualB) override;
> > +       void setColourTemperature(double temperatureK) override;
> >         void enableAuto() override;
> >         void disableAuto() override;
> >         void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
> > -- 
> > 2.39.2
> >