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

[Laurent Pinchart]

On Fri, Aug 30, 2024 at 09:20:05AM +0200, Stefan Klug wrote:
> Hi Laurent,
> 
> Thank you for the patch. 

Did I send one ? :-)

> On Fri, Aug 30, 2024 at 03:09:25AM +0300, Laurent Pinchart wrote:
> > 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"?

I like that a bit more yes.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > > +	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