[11/13] ipa: libipa: awb: Move configure to common
diff mbox series

Message ID 20260407-kbingham-awb-split-v1-11-a39af3f4dc20@ideasonboard.com
State New
Headers show
Series
  • ipa: simple: Convert to libipa AWB implementation
Related show

Commit Message

Kieran Bingham April 7, 2026, 10:01 p.m. UTC 
Refactor the AWB configuration to use the common implementation supplied
by libipa awb module.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/libipa/awb.cpp            | 29 +++++++++++++++++++++++++++++
 src/ipa/libipa/awb.h              |  3 +++
 src/ipa/rkisp1/algorithms/awb.cpp | 13 +------------
 src/ipa/simple/algorithms/awb.cpp | 16 ++--------------
 4 files changed, 35 insertions(+), 26 deletions(-)

Comments

Jacopo Mondi April 8, 2026, 8:47 a.m. UTC | #1 
Hi Kieran

On Tue, Apr 07, 2026 at 11:01:14PM +0100, Kieran Bingham wrote:
> Refactor the AWB configuration to use the common implementation supplied
> by libipa awb module.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/libipa/awb.cpp            | 29 +++++++++++++++++++++++++++++
>  src/ipa/libipa/awb.h              |  3 +++
>  src/ipa/rkisp1/algorithms/awb.cpp | 13 +------------
>  src/ipa/simple/algorithms/awb.cpp | 16 ++--------------
>  4 files changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> index f215bea0b59483eadf95572f073928a4eb1275f4..d0c577958caebc0b7e24fe58506da964df2200fe 100644
> --- a/src/ipa/libipa/awb.cpp
> +++ b/src/ipa/libipa/awb.cpp
> @@ -11,6 +11,8 @@
>
>  #include <libcamera/control_ids.h>
>
> +constexpr int32_t kDefaultColourTemperature = 5000;
> +
>  /**
>   * \file awb.h
>   * \brief Base classes for AWB algorithms
> @@ -139,6 +141,33 @@ namespace ipa {
>   * \return 0 on success, a negative error code otherwise
>   */
>
> +/**
> + * \brief Configure the Awb algorithm given an IPAConfigInfo

no IPAConfigInfo :)

I would simply:
        \brief Configure the Awb algorithm initial state

> + * \param[in] state The AWB specific active state shared across frames
> + * \param[in] session The AWB specific session configuration

[inout] maybe, as this function initializes these two arguments ?

> + *
> + * Configure and initialise the AWB algorithm module.
> + *
> + * \return 0 if successful, an error code otherwise

Doesn't seem to return errors at all. What is a failure condition for
configure() ?

> + */
> +int AwbAlgorithm::configure(awb::ActiveState &state, awb::Session &session)
> +{
> +	state.manual.gains = RGB<double>{ 1.0 };
> +	auto gains = gainsFromColourTemperature(kDefaultColourTemperature);
> +	if (gains)

I think the AwbGrey implemenation could be updated

 * \return The colour gains if a colour temperature curve is available,
 * [1, 1, 1] otherwise.
 */
std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature)
{
	if (!colourGainCurve_) {
		LOG(Awb, Error) << "No gains defined";
		return std::nullopt;
	}

to return RGB<double>{ 1.0 };

so that it matches its documentation and you could avoid handling the
failure case here

> +		state.automatic.gains = *gains;
> +	else
> +		state.automatic.gains = RGB<double>{ 1.0 };
> +
> +	state.autoEnabled = true;
> +	state.manual.temperatureK = kDefaultColourTemperature;
> +	state.automatic.temperatureK = kDefaultColourTemperature;
> +
> +	session.enabled = true;
> +
> +	return 0;
> +}
> +
>  /**
>   * \fn AwbAlgorithm::calculateAwb()
>   * \brief Calculate AWB data from the given statistics
> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> index 764be849270bcd42ecdf67aea3d13afa170c7499..4ceae537686f8f4c93686fab4b9efbc06e112b1d 100644
> --- a/src/ipa/libipa/awb.h
> +++ b/src/ipa/libipa/awb.h
> @@ -65,6 +65,9 @@ public:
>  	virtual ~AwbAlgorithm() = default;
>
>  	virtual int init(const YamlObject &tuningData) = 0;
> +
> +	int configure(awb::ActiveState &state, awb::Session &session);
> +
>  	virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
>  	virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index f83da545be856c08cd758dc20a5ace91344101cf..86d5dfed3e1c2bb587705f05362229db3cdadafd 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -128,16 +128,7 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
>  int Awb::configure(IPAContext &context,
>  		   const IPACameraSensorInfo &configInfo)
>  {
> -	context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> -	auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> -	if (gains)
> -		context.activeState.awb.automatic.gains = *gains;
> -	else
> -		context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> -
> -	context.activeState.awb.autoEnabled = true;
> -	context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> -	context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> +	awbAlgo_->configure(context.activeState.awb, context.configuration.awb);
>
>  	/*
>  	 * Define the measurement window for AWB as a centered rectangle
> @@ -148,8 +139,6 @@ int Awb::configure(IPAContext &context,
>  	context.configuration.awb.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
>  	context.configuration.awb.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
>
> -	context.configuration.awb.enabled = true;
> -
>  	return 0;
>  }
>
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index 90c05e86bae6eefe4874feeb1263af07acd5fcfc..230b7807075941f086911549066e39c0a04dff5c 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -106,20 +106,8 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
>  int Awb::configure(IPAContext &context,
>  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
> -	context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> -	auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> -	if (gains)
> -		context.activeState.awb.automatic.gains = *gains;
> -	else
> -		context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> -
> -	context.activeState.awb.autoEnabled = true;
> -	context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> -	context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> -
> -	context.configuration.awb.enabled = true;
> -
> -	return 0;
> +	return awbAlgo_->configure(context.activeState.awb,
> +				   context.configuration.awb);
>  }
>
>  /**
>
> --
> 2.53.0
>
Kieran Bingham April 8, 2026, 5:19 p.m. UTC | #2 
Quoting Jacopo Mondi (2026-04-08 09:47:29)
> Hi Kieran
> 
> On Tue, Apr 07, 2026 at 11:01:14PM +0100, Kieran Bingham wrote:
> > Refactor the AWB configuration to use the common implementation supplied
> > by libipa awb module.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/libipa/awb.cpp            | 29 +++++++++++++++++++++++++++++
> >  src/ipa/libipa/awb.h              |  3 +++
> >  src/ipa/rkisp1/algorithms/awb.cpp | 13 +------------
> >  src/ipa/simple/algorithms/awb.cpp | 16 ++--------------
> >  4 files changed, 35 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > index f215bea0b59483eadf95572f073928a4eb1275f4..d0c577958caebc0b7e24fe58506da964df2200fe 100644
> > --- a/src/ipa/libipa/awb.cpp
> > +++ b/src/ipa/libipa/awb.cpp
> > @@ -11,6 +11,8 @@
> >
> >  #include <libcamera/control_ids.h>
> >
> > +constexpr int32_t kDefaultColourTemperature = 5000;
> > +
> >  /**
> >   * \file awb.h
> >   * \brief Base classes for AWB algorithms
> > @@ -139,6 +141,33 @@ namespace ipa {
> >   * \return 0 on success, a negative error code otherwise
> >   */
> >
> > +/**
> > + * \brief Configure the Awb algorithm given an IPAConfigInfo
> 
> no IPAConfigInfo :)

Argh, Sorry, too much copya-pasta.

> 
> I would simply:
>         \brief Configure the Awb algorithm initial state

Ack,

> 
> > + * \param[in] state The AWB specific active state shared across frames
> > + * \param[in] session The AWB specific session configuration
> 
> [inout] maybe, as this function initializes these two arguments ?

Yes.

> 
> > + *
> > + * Configure and initialise the AWB algorithm module.
> > + *
> > + * \return 0 if successful, an error code otherwise
> 
> Doesn't seem to return errors at all. What is a failure condition for
> configure() ?

When I did this I was more or less trying to take and mirror the
existing IPA interface documentation. Ultimately these are supposed to
be the module specific implementations for those, but as you see there
isn't actually a direct mapping as these functions then use the local or
module specific context structures.

I didn't want to change too much as I hope to try to keep things as
close as possible to the main interface or as much as possible the same
for each algo - but maybe that won't be reasonable.

This is the start of the path to find out ;-)

After lux/awb, I want to see AEGC and other algorithms describe /their/
expected storage types and work out how much can be factored out to
generic code.

I know there may always be differences, but that's the point of this
series - to find out what I've always wanted to know. How much can we
keep common to make it easy to add a new platform with libipa.


> 
> > + */
> > +int AwbAlgorithm::configure(awb::ActiveState &state, awb::Session &session)
> > +{
> > +     state.manual.gains = RGB<double>{ 1.0 };
> > +     auto gains = gainsFromColourTemperature(kDefaultColourTemperature);
> > +     if (gains)
> 
> I think the AwbGrey implemenation could be updated
> 
>  * \return The colour gains if a colour temperature curve is available,
>  * [1, 1, 1] otherwise.
>  */
> std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> {
>         if (!colourGainCurve_) {
>                 LOG(Awb, Error) << "No gains defined";
>                 return std::nullopt;
>         }
> 
> to return RGB<double>{ 1.0 };
> 
> so that it matches its documentation and you could avoid handling the
> failure case here
> 
> > +             state.automatic.gains = *gains;
> > +     else
> > +             state.automatic.gains = RGB<double>{ 1.0 };

Ohh I like that.

--
Thanks

Kieran

> > +
> > +     state.autoEnabled = true;
> > +     state.manual.temperatureK = kDefaultColourTemperature;
> > +     state.automatic.temperatureK = kDefaultColourTemperature;
> > +
> > +     session.enabled = true;
> > +
> > +     return 0;
> > +}
> > +
> >  /**
> >   * \fn AwbAlgorithm::calculateAwb()
> >   * \brief Calculate AWB data from the given statistics
> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > index 764be849270bcd42ecdf67aea3d13afa170c7499..4ceae537686f8f4c93686fab4b9efbc06e112b1d 100644
> > --- a/src/ipa/libipa/awb.h
> > +++ b/src/ipa/libipa/awb.h
> > @@ -65,6 +65,9 @@ public:
> >       virtual ~AwbAlgorithm() = default;
> >
> >       virtual int init(const YamlObject &tuningData) = 0;
> > +
> > +     int configure(awb::ActiveState &state, awb::Session &session);
> > +
> >       virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
> >       virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
> >
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index f83da545be856c08cd758dc20a5ace91344101cf..86d5dfed3e1c2bb587705f05362229db3cdadafd 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -128,16 +128,7 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
> >  int Awb::configure(IPAContext &context,
> >                  const IPACameraSensorInfo &configInfo)
> >  {
> > -     context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> > -     auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> > -     if (gains)
> > -             context.activeState.awb.automatic.gains = *gains;
> > -     else
> > -             context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> > -
> > -     context.activeState.awb.autoEnabled = true;
> > -     context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> > -     context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> > +     awbAlgo_->configure(context.activeState.awb, context.configuration.awb);
> >
> >       /*
> >        * Define the measurement window for AWB as a centered rectangle
> > @@ -148,8 +139,6 @@ int Awb::configure(IPAContext &context,
> >       context.configuration.awb.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
> >       context.configuration.awb.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
> >
> > -     context.configuration.awb.enabled = true;
> > -
> >       return 0;
> >  }
> >
> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> > index 90c05e86bae6eefe4874feeb1263af07acd5fcfc..230b7807075941f086911549066e39c0a04dff5c 100644
> > --- a/src/ipa/simple/algorithms/awb.cpp
> > +++ b/src/ipa/simple/algorithms/awb.cpp
> > @@ -106,20 +106,8 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
> >  int Awb::configure(IPAContext &context,
> >                  [[maybe_unused]] const IPAConfigInfo &configInfo)
> >  {
> > -     context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> > -     auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> > -     if (gains)
> > -             context.activeState.awb.automatic.gains = *gains;
> > -     else
> > -             context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> > -
> > -     context.activeState.awb.autoEnabled = true;
> > -     context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> > -     context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> > -
> > -     context.configuration.awb.enabled = true;
> > -
> > -     return 0;
> > +     return awbAlgo_->configure(context.activeState.awb,
> > +                                context.configuration.awb);
> >  }
> >
> >  /**
> >
> > --
> > 2.53.0
> >

Patch
diff mbox series 

diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
index f215bea0b59483eadf95572f073928a4eb1275f4..d0c577958caebc0b7e24fe58506da964df2200fe 100644
--- a/src/ipa/libipa/awb.cpp
+++ b/src/ipa/libipa/awb.cpp
@@ -11,6 +11,8 @@ 
 
 #include <libcamera/control_ids.h>
 
+constexpr int32_t kDefaultColourTemperature = 5000;
+
 /**
  * \file awb.h
  * \brief Base classes for AWB algorithms
@@ -139,6 +141,33 @@  namespace ipa {
  * \return 0 on success, a negative error code otherwise
  */
 
+/**
+ * \brief Configure the Awb algorithm given an IPAConfigInfo
+ * \param[in] state The AWB specific active state shared across frames
+ * \param[in] session The AWB specific session configuration
+ *
+ * Configure and initialise the AWB algorithm module.
+ *
+ * \return 0 if successful, an error code otherwise
+ */
+int AwbAlgorithm::configure(awb::ActiveState &state, awb::Session &session)
+{
+	state.manual.gains = RGB<double>{ 1.0 };
+	auto gains = gainsFromColourTemperature(kDefaultColourTemperature);
+	if (gains)
+		state.automatic.gains = *gains;
+	else
+		state.automatic.gains = RGB<double>{ 1.0 };
+
+	state.autoEnabled = true;
+	state.manual.temperatureK = kDefaultColourTemperature;
+	state.automatic.temperatureK = kDefaultColourTemperature;
+
+	session.enabled = true;
+
+	return 0;
+}
+
 /**
  * \fn AwbAlgorithm::calculateAwb()
  * \brief Calculate AWB data from the given statistics
diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
index 764be849270bcd42ecdf67aea3d13afa170c7499..4ceae537686f8f4c93686fab4b9efbc06e112b1d 100644
--- a/src/ipa/libipa/awb.h
+++ b/src/ipa/libipa/awb.h
@@ -65,6 +65,9 @@  public:
 	virtual ~AwbAlgorithm() = default;
 
 	virtual int init(const YamlObject &tuningData) = 0;
+
+	int configure(awb::ActiveState &state, awb::Session &session);
+
 	virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
 	virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
 
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
index f83da545be856c08cd758dc20a5ace91344101cf..86d5dfed3e1c2bb587705f05362229db3cdadafd 100644
--- a/src/ipa/rkisp1/algorithms/awb.cpp
+++ b/src/ipa/rkisp1/algorithms/awb.cpp
@@ -128,16 +128,7 @@  int Awb::init(IPAContext &context, const YamlObject &tuningData)
 int Awb::configure(IPAContext &context,
 		   const IPACameraSensorInfo &configInfo)
 {
-	context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
-	auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
-	if (gains)
-		context.activeState.awb.automatic.gains = *gains;
-	else
-		context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
-
-	context.activeState.awb.autoEnabled = true;
-	context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
-	context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
+	awbAlgo_->configure(context.activeState.awb, context.configuration.awb);
 
 	/*
 	 * Define the measurement window for AWB as a centered rectangle
@@ -148,8 +139,6 @@  int Awb::configure(IPAContext &context,
 	context.configuration.awb.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
 	context.configuration.awb.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
 
-	context.configuration.awb.enabled = true;
-
 	return 0;
 }
 
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
index 90c05e86bae6eefe4874feeb1263af07acd5fcfc..230b7807075941f086911549066e39c0a04dff5c 100644
--- a/src/ipa/simple/algorithms/awb.cpp
+++ b/src/ipa/simple/algorithms/awb.cpp
@@ -106,20 +106,8 @@  int Awb::init(IPAContext &context, const YamlObject &tuningData)
 int Awb::configure(IPAContext &context,
 		   [[maybe_unused]] const IPAConfigInfo &configInfo)
 {
-	context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
-	auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
-	if (gains)
-		context.activeState.awb.automatic.gains = *gains;
-	else
-		context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
-
-	context.activeState.awb.autoEnabled = true;
-	context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
-	context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
-
-	context.configuration.awb.enabled = true;
-
-	return 0;
+	return awbAlgo_->configure(context.activeState.awb,
+				   context.configuration.awb);
 }
 
 /**