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

[Stefan Klug]

Hi Laurent,

Thank you for the patch. 

On Fri, Aug 30, 2024 at 03:09:25AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Tue, Aug 13, 2024 at 10:44:19AM +0200, Stefan Klug wrote:
> > 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>
> > Reviewed-by: Kieran Bingham <kieran.bingham 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..c23f749c192b 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([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > +{
> > +	MatrixInterpolator<double, 2, 1> gains;
> > +	int ret = gains.readYaml(tuningData["gains"], "ct", "gains");
> > +	if (ret < 0)
> > +		LOG(RkISP1Awb, Warning)
> > +			<< "Failed to parse 'gains' "
> > +			<< "parameter from tuning file; "
> > +			<< "rgb gains will not be set based on colour temperature";
> 
> Do you expect we will do more with the tuning parameters later ? If so I
> would make the message less specific already and just mention that AWB
> will not work optimally.

I really don't know atm. And the current gray world model works well
without the gains, so I'd prefer to be specific if that's ok for you.
Maybe "manual color temperature will not work properly"?

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 

Thanks.
Stefan

> > +	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_;
> >  };
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart