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

[Stefan Klug]

On Fri, Aug 30, 2024 at 12:26:51PM +0300, Laurent Pinchart wrote:
> On Fri, Aug 30, 2024 at 09:20:05AM +0200, Stefan Klug wrote:
> > Hi Laurent,
> > 
> > Thank you for the patch. 
> 
> Did I send one ? :-)

Ouch wrong shortcut. Same for the next answers :-/

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