[PATCH v1 2/6] ipa: rkisp1: awb: Load white balance gains from tuning file

[Kieran Bingham]

Quoting Stefan Klug (2024-08-06 07:26:59)
> On Mon, Aug 05, 2024 at 02:43:39PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2024-08-05 13:05:03)
> > > For the implementation of a manual colour temperature setting, it is
> > > necessary to read predefined colour gains per colour temperature from
> > > the tuning file. Implement this in a backwards compatible way. If no
> > > gains are contained in the tuning file, loading just continues as
> > > before.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++++++++++
> > >  src/ipa/rkisp1/algorithms/awb.h   |  6 ++++++
> > >  2 files changed, 24 insertions(+)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > index 4ccafd48dedd..957d24fe3425 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > @@ -39,6 +39,24 @@ Awb::Awb()
> > >  {
> > >  }
> > >  
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::init
> > > + */
> > > +int Awb::init(IPAContext &context, const YamlObject &tuningData)
> > > +{
> > > +       MatrixInterpolator<double, 2, 1> gains;
> > > +       int ret = gains.readYaml(tuningData["gains"], "ct", "gains");
> > 
> > Do we have a way to check if there are gains set at all? If there are no
> > gains set in the yaml - we wouldn't report a warning at all I'd expect?
> 
> If there are no gains in the yaml, readYaml returns an error. We could
> add a check for that specific case and silence the warning. But do we
> really want to silence that? It basically says "You have an old tuning
> file, some features are limited but I'll continue". So I think it's
> reasonable - up to you.

I think if we can get the schema validation then we can add this as an
expected/required tuning entry then and maybe that also solves the issue
as then the warning really would be a warning, so I'm fine with leaving
this as it is given we're aiming in that direction.

> 
> > 
> > (And presumably we wouldn't expose/report a manual color temperature
> > control as setable either?)
> 
> That's a bit difficult here as the temperature also controls the ccm.
> So we would need to check the existence of ccms also. As the ccms are
> necessary for a properly tuned sensor, I think it is ok to
> unconditionally add the control. 

Ok so we have a dependency on both algo's for the suitable / acceptable
parameters for the control? That makes me think back to how we determine
what the min/max values can be - but that's in a later patch I think ;-)

> 
> > 
> > > +       if (ret < 0)
> > > +               LOG(RkISP1Awb, Warning)
> > > +                       << "Failed to parse 'gains' "
> > > +                       << "parameter from tuning file; "
> > > +                       << "rgb gains will not be set based on colour temperature";
> > > +       else
> > > +               gains_ = gains;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  /**
> > >   * \copydoc libcamera::ipa::Algorithm::configure
> > >   */
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> > > index 06c92896e2dc..a010e6d1cb3c 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.h
> > > +++ b/src/ipa/rkisp1/algorithms/awb.h
> > > @@ -7,6 +7,10 @@
> > >  
> > >  #pragma once
> > >  
> > > +#include <optional>
> > > +
> > > +#include "libipa/matrix_interpolator.h"
> > > +
> > >  #include "algorithm.h"
> > >  
> > >  namespace libcamera {
> > > @@ -19,6 +23,7 @@ public:
> > >         Awb();
> > >         ~Awb() = default;
> > >  
> > > +       int init(IPAContext &context, const YamlObject &tuningData) override;
> > >         int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > >         void queueRequest(IPAContext &context, const uint32_t frame,
> > >                           IPAFrameContext &frameContext,
> > > @@ -34,6 +39,7 @@ public:
> > >  private:
> > >         uint32_t estimateCCT(double red, double green, double blue);
> > >  
> > > +       std::optional<MatrixInterpolator<double, 2, 1>> gains_;
> > >         bool rgbMode_;
> > >  };
> > >  
> > > -- 
> > > 2.43.0
> > >