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

[Kieran Bingham]

Quoting Kieran Bingham (2024-08-06 09:52:25)
> 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 ;-)


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> > 
> > > 
> > > > +       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
> > > >