[v4,2/6] ipa: rkisp1: awb: Load white balance gains from tuning file
diff mbox series

Message ID 20240813084451.44099-3-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • rkisp1: Add manual colour temperature control
Related show

Commit Message

Stefan Klug Aug. 13, 2024, 8:44 a.m. UTC 
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@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++++++++++
 src/ipa/rkisp1/algorithms/awb.h   |  6 ++++++
 2 files changed, 24 insertions(+)

Comments

Paul Elder Aug. 27, 2024, 8:02 a.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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";
> +	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
>
Laurent Pinchart Aug. 30, 2024, 12:09 a.m. UTC | #2 
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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@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_;
>  };
>
Stefan Klug Aug. 30, 2024, 7:20 a.m. UTC | #3 
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@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@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@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
Laurent Pinchart Aug. 30, 2024, 9:26 a.m. UTC | #4 
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@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@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@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_;
> > >  };
> > >
Stefan Klug Aug. 30, 2024, 10:04 a.m. UTC | #5 
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@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@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@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

Patch
diff mbox series 

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";
+	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_;
 };