[v5,03/36] ipa: simple: awb: Port to use libipa AwbAlgorithm

27311 diff mbox series

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Port the SoftISP Awb algorithm to use the new libipa implementation
of AwbAlgorithm.

The awbAlgo_ class member is initialized with the Q<4, 8> type even if
there is no physical register representation for SoftISP.

The usage of libipa::awb::Context, which defines the gains vector as
RGB<double>, in the SimpleIPA ActiveState and FrameContext requires
changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB<float>
and RGB<double>.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Robert Mader <robert.mader@collabora.com>
---
 .../internal/software_isp/debayer_params.h         |   4 +-
 src/ipa/simple/algorithms/awb.cpp                  | 141 ++++++++++++++++-----
 src/ipa/simple/algorithms/awb.h                    |  29 +++++
 src/ipa/simple/algorithms/blc.cpp                  |   2 +-
 src/ipa/simple/algorithms/ccm.cpp                  |   2 +-
 src/ipa/simple/ipa_context.h                       |  12 +-
 src/libcamera/software_isp/debayer_cpu.cpp         |  12 +-
 7 files changed, 151 insertions(+), 51 deletions(-)

Hi Jacopo,

Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:

> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Port the SoftISP Awb algorithm to use the new libipa implementation
> of AwbAlgorithm.
>
> The awbAlgo_ class member is initialized with the Q<4, 8> type even if
> there is no physical register representation for SoftISP.
>
> The usage of libipa::awb::Context, which defines the gains vector as
> RGB<double>, in the SimpleIPA ActiveState and FrameContext requires
> changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB<float>
> and RGB<double>.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Tested-by: Robert Mader <robert.mader@collabora.com>
> ---
>  .../internal/software_isp/debayer_params.h         |   4 +-
>  src/ipa/simple/algorithms/awb.cpp                  | 141 ++++++++++++++++-----
>  src/ipa/simple/algorithms/awb.h                    |  29 +++++
>  src/ipa/simple/algorithms/blc.cpp                  |   2 +-
>  src/ipa/simple/algorithms/ccm.cpp                  |   2 +-
>  src/ipa/simple/ipa_context.h                       |  12 +-
>  src/libcamera/software_isp/debayer_cpu.cpp         |  12 +-
>  7 files changed, 151 insertions(+), 51 deletions(-)
>
> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> index 6772b43bced4..1074720d73c2 100644
> --- a/include/libcamera/internal/software_isp/debayer_params.h
> +++ b/include/libcamera/internal/software_isp/debayer_params.h
> @@ -21,10 +21,10 @@ struct DebayerParams {
>  	Matrix<float, 3, 3> combinedMatrix = { { 1.0, 0.0, 0.0,
>  						 0.0, 1.0, 0.0,
>  						 0.0, 0.0, 1.0 } };
> -	RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 });
> +	RGB<double> blackLevel = RGB<double>({ 0.0, 0.0, 0.0 });
>  	float gamma = 1.0;
>  	float contrastExp = 1.0;
> -	RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 });
> +	RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 });
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index 05155c83d172..64ce4f91b21c 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -15,7 +15,6 @@
>  #include <libcamera/control_ids.h>
>  
>  #include "libipa/colours.h"

This include can be removed too.

> -#include "simple/ipa_context.h"
>  
>  namespace libcamera {
>  
> @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)
>  
>  namespace ipa::soft::algorithms {
>  
> +/*
> + * \todo Replace it with a proper Lux algorithm
> + */
> +static constexpr unsigned int kDefaultLux = 500;
> +
> +class SimpleAwbStats final : public AwbStats

This class is the same across three pipelines -- should it be a part of
libipa?

> +{
> +public:
> +	SimpleAwbStats() = default;
> +
> +	SimpleAwbStats(const RGB<double> &rgbMeans)
> +	{
> +		rgbMeans_ = rgbMeans;
> +
> +		rg_ = rgbMeans_.r() / rgbMeans_.g();
> +		bg_ = rgbMeans_.b() / rgbMeans_.g();
> +	}
> +
> +	double computeColourError(const RGB<double> &gains) const override
> +	{
> +		/*
> +		 * Compute the sum of the squared colour error (non-greyness) as
> +		 * it appears in the log likelihood equation.
> +		 */
> +		double deltaR = gains.r() * rg_ - 1.0;
> +		double deltaB = gains.b() * bg_ - 1.0;
> +		double delta2 = deltaR * deltaR + deltaB * deltaB;
> +
> +		return delta2;
> +	}
> +
> +	RGB<double> rgbMeans() const override
> +	{
> +		return rgbMeans_;
> +	}
> +
> +	bool valid() const override
> +	{
> +		/* Minimum mean value below which AWB can't operate. */
> +		constexpr double minValue = 0.2;
> +
> +		return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
> +		       rgbMeans_.b() > minValue;
> +	}
> +
> +private:
> +	RGB<double> rgbMeans_;
> +	double rg_;
> +	double bg_;
> +};
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
> +int Awb::init(IPAContext &context, const ValueNode &tuningData)
> +{
> +	return awbAlgo_.init(tuningData, context.ctrlMap);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::configure
> + */
>  int Awb::configure(IPAContext &context,
>  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
> -	auto &gains = context.activeState.awb.gains;
> -	gains = { { 1.0, 1.0, 1.0 } };
> +	return awbAlgo_.configure(context.activeState.awb);
> +}
>  
> -	return 0;
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void Awb::queueRequest(IPAContext &context,
> +		       const uint32_t frame,
> +		       IPAFrameContext &frameContext,
> +		       const ControlList &controls)
> +{
> +	awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
> +			      controls);
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
>  void Awb::prepare(IPAContext &context,
>  		  [[maybe_unused]] const uint32_t frame,
>  		  IPAFrameContext &frameContext,
>  		  DebayerParams *params)
>  {
> -	auto &gains = context.activeState.awb.gains;
> +	awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
>  
> -	frameContext.gains = gains;
> -	params->gains = gains;
> +	params->gains = frameContext.awb.gains;
>  }
>  
> -void Awb::process(IPAContext &context,
> -		  [[maybe_unused]] const uint32_t frame,
> -		  IPAFrameContext &frameContext,
> -		  const SwIspStats *stats,
> -		  ControlList &metadata)
> +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,
> +				      const SwIspStats *stats) const
>  {
> +	if (!stats->valid)
> +		return {};
> +
>  	const SwIspStats::Histogram &histogram = stats->yHistogram;
>  	const uint8_t blackLevel = context.activeState.blc.level;
>  
> -	metadata.set(controls::ColourGains, { frameContext.gains.r(),
> -					      frameContext.gains.b() });
> -
> -	if (!stats->valid)
> -		return;
> -
>  	/*
>  	 * Black level must be subtracted to get the correct AWB ratios, they
>  	 * would be off if they were computed from the whole brightness range
> @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,
>  		histogram.begin(), histogram.end(), uint64_t(0));
>  	const uint64_t offset = blackLevel * nPixels;
>  	const uint64_t minValid = 1;
> +
>  	/*
>  	 * Make sure the sums are at least minValid, while preventing unsigned
>  	 * integer underflow.
>  	 */
>  	const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset;
>  
> +	RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels),
> +				   static_cast<double>(sum.g() / nPixels),
> +				   static_cast<double>(sum.b() / nPixels) } };


It would be a little bit more correct to cast the sums, before applying
the division, rather than the integer division result:

  static_cast<double>(sum.r()) / nPixels

> +
>  	/*
> -	 * Calculate red and blue gains for AWB.
> -	 * Clamp max gain at 4.0, this also avoids 0 division.
> +	 * \todo Determine the minimum allowed thresholds from the mean
> +	 * but we currently have the sum - not the mean value!

I don't understand the TODO -- what is it talking about and what mean
and sum are referred here?

> +	 *
> +	 * Currently set to SimpleAwbStats::minColourValue() = 0.2.
>  	 */
> -	auto &gains = context.activeState.awb.gains;
> -	gains = { {
> -		sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(),
> -		1.0,
> -		sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(),
> -	} };
> -
> -	RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
> -	context.activeState.awb.temperatureK = estimateCCT(rgbGains);
> -	metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);
> -
> -	LOG(IPASoftAwb, Debug)
> -		<< "gain R/B: " << gains << "; temperature: "
> -		<< context.activeState.awb.temperatureK;
> +	return SimpleAwbStats(rgbMeans);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> +		  IPAFrameContext &frameContext, const SwIspStats *stats,
> +		  ControlList &metadata)
> +{
> +	SimpleAwbStats awbStats = calculateRgbMeans(context, stats);
> +
> +	awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,
> +			 kDefaultLux, metadata);
>  }
>  
>  REGISTER_IPA_ALGORITHM(Awb, "Awb")
> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
> index ad993f39c180..fec48c6eea1c 100644
> --- a/src/ipa/simple/algorithms/awb.h
> +++ b/src/ipa/simple/algorithms/awb.h
> @@ -7,19 +7,37 @@
>  
>  #pragma once
>  
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/value_node.h"
> +
> +#include "libipa/awb.h"
> +#include "libipa/fixedpoint.h"
> +#include "simple/ipa_context.h"
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
>  
>  namespace ipa::soft::algorithms {
>  
> +class SimpleAwbStats;
> +
>  class Awb : public Algorithm
>  {
>  public:
>  	Awb() = default;
>  	~Awb() = default;
>  
> +	int init(IPAContext &context,
> +		 const ValueNode &tuningData) override;
>  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +
> +	void queueRequest(IPAContext &context,
> +			  [[maybe_unused]] const uint32_t frame,
> +			  IPAFrameContext &frameContext,
> +			  const ControlList &controls) override;
>  	void prepare(IPAContext &context,
>  		     const uint32_t frame,
>  		     IPAFrameContext &frameContext,
> @@ -29,6 +47,17 @@ public:
>  		     IPAFrameContext &frameContext,
>  		     const SwIspStats *stats,
>  		     ControlList &metadata) override;
> +
> +private:
> +	SimpleAwbStats calculateRgbMeans(IPAContext &context,
> +					 const SwIspStats *stats) const;
> +
> +	/*
> +	 * There actually is no Q register format for SoftISP, but allow the
> +	 * colour gains to range in the [0.0f, 3.999f] interval, which seems
> +	 * reasonable.
> +	 */
> +	AwbAlgorithm<UQ<2, 8>> awbAlgo_;
>  };
>  
>  } /* namespace ipa::soft::algorithms */
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 677be56ed669..e45a913cd970 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,
>  			 DebayerParams *params)
>  {
>  	/* Latch the blacklevel gain so GPUISP can apply. */
> -	params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
> +	params->blackLevel = RGB<double>(context.activeState.blc.level / 255.0f);
>  }
>  
>  void BlackLevel::process(IPAContext &context,
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index ace9c35dc462..1174784edc7e 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)
>  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  		  IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>  {
> -	const unsigned int ct = context.activeState.awb.temperatureK;
> +	const unsigned int ct = frameContext.awb.colourTemperature;
>  
>  	/* Change CCM only on bigger temperature changes. */
>  	if (!currentCcm_ ||
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 8ccfacb46a59..29643a655ce1 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -16,6 +16,7 @@
>  #include "libcamera/internal/matrix.h"
>  #include "libcamera/internal/vector.h"
>  
> +#include <libipa/awb.h>
>  #include <libipa/fc_queue.h>
>  
>  #include "core_ipa_interface.h"
> @@ -36,6 +37,8 @@ struct IPASessionConfiguration {
>  };
>  
>  struct IPAActiveState {
> +	ipa::awb::ActiveState awb;
> +
>  	struct {
>  		int32_t exposure;
>  		double again;
> @@ -48,11 +51,6 @@ struct IPAActiveState {
>  		double lastGain;
>  	} blc;
>  
> -	struct {
> -		RGB<float> gains;
> -		unsigned int temperatureK;
> -	} awb;
> -
>  	Matrix<float, 3, 3> combinedMatrix;
>  
>  	struct {
> @@ -64,6 +62,8 @@ struct IPAActiveState {
>  };
>  
>  struct IPAFrameContext : public FrameContext {
> +	ipa::awb::FrameContext awb;
> +
>  	Matrix<float, 3, 3> ccm;
>  
>  	struct {
> @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {
>  		double gain;
>  	} sensor;
>  
> -	RGB<float> gains;
> -
>  	float gamma;
>  	std::optional<float> contrast;
>  	std::optional<float> saturation;
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 49382b4c2719..ab22635fdfaf 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  	};
>  	const unsigned int gammaTableSize = gammaTable_.size();
>  
> -	const RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
> -	const RGB<float> gains = params.gains;
> -	const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0);
> +	const RGB<double> blackIndex = params.blackLevel * kRGBLookupSize;
> +	const RGB<double> gains = params.gains;
> +	const RGB<double> div = (RGB<double>(kRGBLookupSize) - blackIndex).max(1.0);
>  
>  	if (ccmEnabled_) {
>  		if (gammaUpdateNeeded ||
> @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			const unsigned int greenIndex = 1;
>  			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				const RGB<float> rgb = (gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div)
> +				const RGB<double> rgb = (gains * (RGB<double>(i) - blackIndex) * kRGBLookupSize / div)
>  							       .clamp(0.0, kRGBLookupSize - 1);
>  				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
>  				red[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);
> @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>  			auto &green = green_;
>  			auto &blue = swapRedBlueGains_ ? red_ : blue_;
>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> -				const RGB<float> lutGains =
> -					(gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div)
> +				const RGB<double> lutGains =
> +					(gains * (RGB<double>(i) - blackIndex) * gammaTableSize / div)
>  						.clamp(0.0, gammaTableSize - 1);
>  				red[i] = gammaTable_[lutGains.r()];
>  				green[i] = gammaTable_[lutGains.g()];

Sorry, I've forgotten about awb.h...

Milan Zamazal <mzamazal@redhat.com> writes:

> Hi Jacopo,
>
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Port the SoftISP Awb algorithm to use the new libipa implementation
>> of AwbAlgorithm.
>>
>> The awbAlgo_ class member is initialized with the Q<4, 8> type even if

It's actually UQ<2, 8>.

>> there is no physical register representation for SoftISP.
>>
>> The usage of libipa::awb::Context, which defines the gains vector as
>> RGB<double>, in the SimpleIPA ActiveState and FrameContext requires
>> changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB<float>
>> and RGB<double>.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Tested-by: Robert Mader <robert.mader@collabora.com>
>> ---
>>  .../internal/software_isp/debayer_params.h         |   4 +-
>>  src/ipa/simple/algorithms/awb.cpp                  | 141 ++++++++++++++++-----
>>  src/ipa/simple/algorithms/awb.h                    |  29 +++++
>>  src/ipa/simple/algorithms/blc.cpp                  |   2 +-
>>  src/ipa/simple/algorithms/ccm.cpp                  |   2 +-
>>  src/ipa/simple/ipa_context.h                       |  12 +-
>>  src/libcamera/software_isp/debayer_cpu.cpp         |  12 +-
>>  7 files changed, 151 insertions(+), 51 deletions(-)
>>
>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
>> index 6772b43bced4..1074720d73c2 100644
>> --- a/include/libcamera/internal/software_isp/debayer_params.h
>> +++ b/include/libcamera/internal/software_isp/debayer_params.h
>> @@ -21,10 +21,10 @@ struct DebayerParams {
>>  	Matrix<float, 3, 3> combinedMatrix = { { 1.0, 0.0, 0.0,
>>  						 0.0, 1.0, 0.0,
>>  						 0.0, 0.0, 1.0 } };
>> -	RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 });
>> +	RGB<double> blackLevel = RGB<double>({ 0.0, 0.0, 0.0 });
>>  	float gamma = 1.0;
>>  	float contrastExp = 1.0;
>> -	RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 });
>> +	RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 });
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
>> index 05155c83d172..64ce4f91b21c 100644
>> --- a/src/ipa/simple/algorithms/awb.cpp
>> +++ b/src/ipa/simple/algorithms/awb.cpp
>> @@ -15,7 +15,6 @@
>>  #include <libcamera/control_ids.h>
>>  
>>  #include "libipa/colours.h"
>
> This include can be removed too.
>
>> -#include "simple/ipa_context.h"
>>  
>>  namespace libcamera {
>>  
>> @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)
>>  
>>  namespace ipa::soft::algorithms {
>>  
>> +/*
>> + * \todo Replace it with a proper Lux algorithm
>> + */
>> +static constexpr unsigned int kDefaultLux = 500;
>> +
>> +class SimpleAwbStats final : public AwbStats
>
> This class is the same across three pipelines -- should it be a part of
> libipa?
>
>> +{
>> +public:
>> +	SimpleAwbStats() = default;
>> +
>> +	SimpleAwbStats(const RGB<double> &rgbMeans)
>> +	{
>> +		rgbMeans_ = rgbMeans;
>> +
>> +		rg_ = rgbMeans_.r() / rgbMeans_.g();
>> +		bg_ = rgbMeans_.b() / rgbMeans_.g();
>> +	}
>> +
>> +	double computeColourError(const RGB<double> &gains) const override
>> +	{
>> +		/*
>> +		 * Compute the sum of the squared colour error (non-greyness) as
>> +		 * it appears in the log likelihood equation.
>> +		 */
>> +		double deltaR = gains.r() * rg_ - 1.0;
>> +		double deltaB = gains.b() * bg_ - 1.0;
>> +		double delta2 = deltaR * deltaR + deltaB * deltaB;
>> +
>> +		return delta2;
>> +	}
>> +
>> +	RGB<double> rgbMeans() const override
>> +	{
>> +		return rgbMeans_;
>> +	}
>> +
>> +	bool valid() const override
>> +	{
>> +		/* Minimum mean value below which AWB can't operate. */
>> +		constexpr double minValue = 0.2;
>> +
>> +		return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
>> +		       rgbMeans_.b() > minValue;
>> +	}
>> +
>> +private:
>> +	RGB<double> rgbMeans_;
>> +	double rg_;
>> +	double bg_;
>> +};
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::init
>> + */
>> +int Awb::init(IPAContext &context, const ValueNode &tuningData)
>> +{
>> +	return awbAlgo_.init(tuningData, context.ctrlMap);
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::configure
>> + */
>>  int Awb::configure(IPAContext &context,
>>  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>>  {
>> -	auto &gains = context.activeState.awb.gains;
>> -	gains = { { 1.0, 1.0, 1.0 } };
>> +	return awbAlgo_.configure(context.activeState.awb);
>> +}
>>  
>> -	return 0;
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::queueRequest
>> + */
>> +void Awb::queueRequest(IPAContext &context,
>> +		       const uint32_t frame,
>> +		       IPAFrameContext &frameContext,
>> +		       const ControlList &controls)
>> +{
>> +	awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
>> +			      controls);
>>  }
>>  
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>>  void Awb::prepare(IPAContext &context,
>>  		  [[maybe_unused]] const uint32_t frame,
>>  		  IPAFrameContext &frameContext,
>>  		  DebayerParams *params)
>>  {
>> -	auto &gains = context.activeState.awb.gains;
>> +	awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
>>  
>> -	frameContext.gains = gains;
>> -	params->gains = gains;
>> +	params->gains = frameContext.awb.gains;
>>  }
>>  
>> -void Awb::process(IPAContext &context,
>> -		  [[maybe_unused]] const uint32_t frame,
>> -		  IPAFrameContext &frameContext,
>> -		  const SwIspStats *stats,
>> -		  ControlList &metadata)
>> +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,
>> +				      const SwIspStats *stats) const
>>  {
>> +	if (!stats->valid)
>> +		return {};
>> +
>>  	const SwIspStats::Histogram &histogram = stats->yHistogram;
>>  	const uint8_t blackLevel = context.activeState.blc.level;
>>  
>> -	metadata.set(controls::ColourGains, { frameContext.gains.r(),
>> -					      frameContext.gains.b() });
>> -
>> -	if (!stats->valid)
>> -		return;
>> -
>>  	/*
>>  	 * Black level must be subtracted to get the correct AWB ratios, they
>>  	 * would be off if they were computed from the whole brightness range
>> @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,
>>  		histogram.begin(), histogram.end(), uint64_t(0));
>>  	const uint64_t offset = blackLevel * nPixels;
>>  	const uint64_t minValid = 1;
>> +
>>  	/*
>>  	 * Make sure the sums are at least minValid, while preventing unsigned
>>  	 * integer underflow.
>>  	 */
>>  	const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset;
>>  
>> +	RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels),
>> +				   static_cast<double>(sum.g() / nPixels),
>> +				   static_cast<double>(sum.b() / nPixels) } };
>
>
> It would be a little bit more correct to cast the sums, before applying
> the division, rather than the integer division result:
>
>   static_cast<double>(sum.r()) / nPixels
>
>> +
>>  	/*
>> -	 * Calculate red and blue gains for AWB.
>> -	 * Clamp max gain at 4.0, this also avoids 0 division.
>> +	 * \todo Determine the minimum allowed thresholds from the mean
>> +	 * but we currently have the sum - not the mean value!
>
> I don't understand the TODO -- what is it talking about and what mean
> and sum are referred here?
>
>> +	 *
>> +	 * Currently set to SimpleAwbStats::minColourValue() = 0.2.
>>  	 */
>> -	auto &gains = context.activeState.awb.gains;
>> -	gains = { {
>> -		sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(),
>> -		1.0,
>> -		sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(),
>> -	} };
>> -
>> -	RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
>> -	context.activeState.awb.temperatureK = estimateCCT(rgbGains);
>> -	metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);
>> -
>> -	LOG(IPASoftAwb, Debug)
>> -		<< "gain R/B: " << gains << "; temperature: "
>> -		<< context.activeState.awb.temperatureK;
>> +	return SimpleAwbStats(rgbMeans);
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::process
>> + */
>> +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>> +		  IPAFrameContext &frameContext, const SwIspStats *stats,
>> +		  ControlList &metadata)
>> +{
>> +	SimpleAwbStats awbStats = calculateRgbMeans(context, stats);
>> +
>> +	awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,
>> +			 kDefaultLux, metadata);
>>  }
>>  
>>  REGISTER_IPA_ALGORITHM(Awb, "Awb")
>> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
>> index ad993f39c180..fec48c6eea1c 100644
>> --- a/src/ipa/simple/algorithms/awb.h
>> +++ b/src/ipa/simple/algorithms/awb.h
>> @@ -7,19 +7,37 @@
>>  
>>  #pragma once
>>  
>> +#include <libcamera/controls.h>
>> +
>> +#include "libcamera/internal/software_isp/debayer_params.h"
>> +#include "libcamera/internal/value_node.h"
>> +
>> +#include "libipa/awb.h"
>> +#include "libipa/fixedpoint.h"
>> +#include "simple/ipa_context.h"
>> +
>>  #include "algorithm.h"
>>  
>>  namespace libcamera {
>>  
>>  namespace ipa::soft::algorithms {
>>  
>> +class SimpleAwbStats;
>> +
>>  class Awb : public Algorithm
>>  {
>>  public:
>>  	Awb() = default;
>>  	~Awb() = default;
>>  
>> +	int init(IPAContext &context,
>> +		 const ValueNode &tuningData) override;
>>  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>> +
>> +	void queueRequest(IPAContext &context,
>> +			  [[maybe_unused]] const uint32_t frame,

No need to use [[maybe_unused]] in a header file.

>> +			  IPAFrameContext &frameContext,
>> +			  const ControlList &controls) override;
>>  	void prepare(IPAContext &context,
>>  		     const uint32_t frame,
>>  		     IPAFrameContext &frameContext,
>> @@ -29,6 +47,17 @@ public:
>>  		     IPAFrameContext &frameContext,
>>  		     const SwIspStats *stats,
>>  		     ControlList &metadata) override;
>> +
>> +private:
>> +	SimpleAwbStats calculateRgbMeans(IPAContext &context,
>> +					 const SwIspStats *stats) const;
>> +
>> +	/*
>> +	 * There actually is no Q register format for SoftISP, but allow the
>> +	 * colour gains to range in the [0.0f, 3.999f] interval, which seems
>> +	 * reasonable.
>> +	 */
>> +	AwbAlgorithm<UQ<2, 8>> awbAlgo_;
>>  };
>>  
>>  } /* namespace ipa::soft::algorithms */
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 677be56ed669..e45a913cd970 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,
>>  			 DebayerParams *params)
>>  {
>>  	/* Latch the blacklevel gain so GPUISP can apply. */
>> -	params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
>> +	params->blackLevel = RGB<double>(context.activeState.blc.level / 255.0f);
>>  }
>>  
>>  void BlackLevel::process(IPAContext &context,
>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> index ace9c35dc462..1174784edc7e 100644
>> --- a/src/ipa/simple/algorithms/ccm.cpp
>> +++ b/src/ipa/simple/algorithms/ccm.cpp
>> @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)
>>  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>  		  IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>>  {
>> -	const unsigned int ct = context.activeState.awb.temperatureK;
>> +	const unsigned int ct = frameContext.awb.colourTemperature;
>>  
>>  	/* Change CCM only on bigger temperature changes. */
>>  	if (!currentCcm_ ||
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 8ccfacb46a59..29643a655ce1 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -16,6 +16,7 @@
>>  #include "libcamera/internal/matrix.h"
>>  #include "libcamera/internal/vector.h"
>>  
>> +#include <libipa/awb.h>
>>  #include <libipa/fc_queue.h>
>>  
>>  #include "core_ipa_interface.h"
>> @@ -36,6 +37,8 @@ struct IPASessionConfiguration {
>>  };
>>  
>>  struct IPAActiveState {
>> +	ipa::awb::ActiveState awb;
>> +
>>  	struct {
>>  		int32_t exposure;
>>  		double again;
>> @@ -48,11 +51,6 @@ struct IPAActiveState {
>>  		double lastGain;
>>  	} blc;
>>  
>> -	struct {
>> -		RGB<float> gains;
>> -		unsigned int temperatureK;
>> -	} awb;
>> -
>>  	Matrix<float, 3, 3> combinedMatrix;
>>  
>>  	struct {
>> @@ -64,6 +62,8 @@ struct IPAActiveState {
>>  };
>>  
>>  struct IPAFrameContext : public FrameContext {
>> +	ipa::awb::FrameContext awb;
>> +
>>  	Matrix<float, 3, 3> ccm;
>>  
>>  	struct {
>> @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {
>>  		double gain;
>>  	} sensor;
>>  
>> -	RGB<float> gains;
>> -
>>  	float gamma;
>>  	std::optional<float> contrast;
>>  	std::optional<float> saturation;
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 49382b4c2719..ab22635fdfaf 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>>  	};
>>  	const unsigned int gammaTableSize = gammaTable_.size();
>>  
>> -	const RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
>> -	const RGB<float> gains = params.gains;
>> -	const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0);
>> +	const RGB<double> blackIndex = params.blackLevel * kRGBLookupSize;
>> +	const RGB<double> gains = params.gains;
>> +	const RGB<double> div = (RGB<double>(kRGBLookupSize) - blackIndex).max(1.0);
>>  
>>  	if (ccmEnabled_) {
>>  		if (gammaUpdateNeeded ||
>> @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>>  			const unsigned int greenIndex = 1;
>>  			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
>>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
>> -				const RGB<float> rgb = (gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div)
>> +				const RGB<double> rgb = (gains * (RGB<double>(i) - blackIndex) * kRGBLookupSize / div)
>>  							       .clamp(0.0, kRGBLookupSize - 1);
>>  				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
>>  				red[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);
>> @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>>  			auto &green = green_;
>>  			auto &blue = swapRedBlueGains_ ? red_ : blue_;
>>  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
>> -				const RGB<float> lutGains =
>> -					(gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div)
>> +				const RGB<double> lutGains =
>> +					(gains * (RGB<double>(i) - blackIndex) * gammaTableSize / div)
>>  						.clamp(0.0, gammaTableSize - 1);
>>  				red[i] = gammaTable_[lutGains.r()];
>>  				green[i] = gammaTable_[lutGains.g()];

Hi Milan

On Fri, Jul 10, 2026 at 11:25:30AM +0200, Milan Zamazal wrote:
> Hi Jacopo,
>
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Port the SoftISP Awb algorithm to use the new libipa implementation
> > of AwbAlgorithm.
> >
> > The awbAlgo_ class member is initialized with the Q<4, 8> type even if
> > there is no physical register representation for SoftISP.
> >
> > The usage of libipa::awb::Context, which defines the gains vector as
> > RGB<double>, in the SimpleIPA ActiveState and FrameContext requires
> > changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB<float>
> > and RGB<double>.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-by: Robert Mader <robert.mader@collabora.com>
> > ---
> >  .../internal/software_isp/debayer_params.h         |   4 +-
> >  src/ipa/simple/algorithms/awb.cpp                  | 141 ++++++++++++++++-----
> >  src/ipa/simple/algorithms/awb.h                    |  29 +++++
> >  src/ipa/simple/algorithms/blc.cpp                  |   2 +-
> >  src/ipa/simple/algorithms/ccm.cpp                  |   2 +-
> >  src/ipa/simple/ipa_context.h                       |  12 +-
> >  src/libcamera/software_isp/debayer_cpu.cpp         |  12 +-
> >  7 files changed, 151 insertions(+), 51 deletions(-)
> >
> > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> > index 6772b43bced4..1074720d73c2 100644
> > --- a/include/libcamera/internal/software_isp/debayer_params.h
> > +++ b/include/libcamera/internal/software_isp/debayer_params.h
> > @@ -21,10 +21,10 @@ struct DebayerParams {
> >  	Matrix<float, 3, 3> combinedMatrix = { { 1.0, 0.0, 0.0,
> >  						 0.0, 1.0, 0.0,
> >  						 0.0, 0.0, 1.0 } };
> > -	RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 });
> > +	RGB<double> blackLevel = RGB<double>({ 0.0, 0.0, 0.0 });
> >  	float gamma = 1.0;
> >  	float contrastExp = 1.0;
> > -	RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 });
> > +	RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 });
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> > index 05155c83d172..64ce4f91b21c 100644
> > --- a/src/ipa/simple/algorithms/awb.cpp
> > +++ b/src/ipa/simple/algorithms/awb.cpp
> > @@ -15,7 +15,6 @@
> >  #include <libcamera/control_ids.h>
> >
> >  #include "libipa/colours.h"
>
> This include can be removed too.
>

ack!

> > -#include "simple/ipa_context.h"
> >
> >  namespace libcamera {
> >
> > @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)
> >
> >  namespace ipa::soft::algorithms {
> >
> > +/*
> > + * \todo Replace it with a proper Lux algorithm
> > + */
> > +static constexpr unsigned int kDefaultLux = 500;
> > +
> > +class SimpleAwbStats final : public AwbStats
>
> This class is the same across three pipelines -- should it be a part of
> libipa?
>

Eh, that's what I had initially, but I've been asked to move the
implementation to each single IPA. I understand why, right now we just
copied around the RkISP1 implementation instead of having each
platform calculate statistics in their own way and moving the RkISP1
implementation to the base class would have communicated that this is
what we expect all platforms to do, while in reality, we just copied
code around.

> > +{
> > +public:
> > +	SimpleAwbStats() = default;
> > +
> > +	SimpleAwbStats(const RGB<double> &rgbMeans)
> > +	{
> > +		rgbMeans_ = rgbMeans;
> > +
> > +		rg_ = rgbMeans_.r() / rgbMeans_.g();
> > +		bg_ = rgbMeans_.b() / rgbMeans_.g();
> > +	}
> > +
> > +	double computeColourError(const RGB<double> &gains) const override
> > +	{
> > +		/*
> > +		 * Compute the sum of the squared colour error (non-greyness) as
> > +		 * it appears in the log likelihood equation.
> > +		 */
> > +		double deltaR = gains.r() * rg_ - 1.0;
> > +		double deltaB = gains.b() * bg_ - 1.0;
> > +		double delta2 = deltaR * deltaR + deltaB * deltaB;
> > +
> > +		return delta2;
> > +	}
> > +
> > +	RGB<double> rgbMeans() const override
> > +	{
> > +		return rgbMeans_;
> > +	}
> > +
> > +	bool valid() const override
> > +	{
> > +		/* Minimum mean value below which AWB can't operate. */
> > +		constexpr double minValue = 0.2;
> > +
> > +		return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
> > +		       rgbMeans_.b() > minValue;
> > +	}
> > +
> > +private:
> > +	RGB<double> rgbMeans_;
> > +	double rg_;
> > +	double bg_;
> > +};
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int Awb::init(IPAContext &context, const ValueNode &tuningData)
> > +{
> > +	return awbAlgo_.init(tuningData, context.ctrlMap);
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::configure
> > + */
> >  int Awb::configure(IPAContext &context,
> >  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
> >  {
> > -	auto &gains = context.activeState.awb.gains;
> > -	gains = { { 1.0, 1.0, 1.0 } };
> > +	return awbAlgo_.configure(context.activeState.awb);
> > +}
> >
> > -	return 0;
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> > + */
> > +void Awb::queueRequest(IPAContext &context,
> > +		       const uint32_t frame,
> > +		       IPAFrameContext &frameContext,
> > +		       const ControlList &controls)
> > +{
> > +	awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
> > +			      controls);
> >  }
> >
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::prepare
> > + */
> >  void Awb::prepare(IPAContext &context,
> >  		  [[maybe_unused]] const uint32_t frame,
> >  		  IPAFrameContext &frameContext,
> >  		  DebayerParams *params)
> >  {
> > -	auto &gains = context.activeState.awb.gains;
> > +	awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
> >
> > -	frameContext.gains = gains;
> > -	params->gains = gains;
> > +	params->gains = frameContext.awb.gains;
> >  }
> >
> > -void Awb::process(IPAContext &context,
> > -		  [[maybe_unused]] const uint32_t frame,
> > -		  IPAFrameContext &frameContext,
> > -		  const SwIspStats *stats,
> > -		  ControlList &metadata)
> > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,
> > +				      const SwIspStats *stats) const
> >  {
> > +	if (!stats->valid)
> > +		return {};
> > +
> >  	const SwIspStats::Histogram &histogram = stats->yHistogram;
> >  	const uint8_t blackLevel = context.activeState.blc.level;
> >
> > -	metadata.set(controls::ColourGains, { frameContext.gains.r(),
> > -					      frameContext.gains.b() });
> > -
> > -	if (!stats->valid)
> > -		return;
> > -
> >  	/*
> >  	 * Black level must be subtracted to get the correct AWB ratios, they
> >  	 * would be off if they were computed from the whole brightness range
> > @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,
> >  		histogram.begin(), histogram.end(), uint64_t(0));
> >  	const uint64_t offset = blackLevel * nPixels;
> >  	const uint64_t minValid = 1;
> > +
> >  	/*
> >  	 * Make sure the sums are at least minValid, while preventing unsigned
> >  	 * integer underflow.
> >  	 */
> >  	const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset;
> >
> > +	RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels),
> > +				   static_cast<double>(sum.g() / nPixels),
> > +				   static_cast<double>(sum.b() / nPixels) } };
>
>
> It would be a little bit more correct to cast the sums, before applying
> the division, rather than the integer division result:
>
>   static_cast<double>(sum.r()) / nPixels
>
> > +
> >  	/*
> > -	 * Calculate red and blue gains for AWB.
> > -	 * Clamp max gain at 4.0, this also avoids 0 division.
> > +	 * \todo Determine the minimum allowed thresholds from the mean
> > +	 * but we currently have the sum - not the mean value!
>
> I don't understand the TODO -- what is it talking about and what mean
> and sum are referred here?

To be honest I don't know how the soft ISP calculate statistics and I
didn't write the \todo, but if I just look at struct SwIspStats
definition:

	/**
	 * \brief Sums of colour channels of all the sampled pixels
	 */
	RGB<uint64_t> sum_;

I presume to generate stats the image is divided into grids, the mean
value of each colour channel is calculated per zone, the averaged and
reported in sums_.

So we don't have a per-window mean value which we can compared with
SimpleAwbStats::minColourValue() to discard zones with not enough
colour information, but we only have the sum when we get here.

I guess the comment means we can't really discard zones by filtering
on the mean colour value.

>
> > +	 *
> > +	 * Currently set to SimpleAwbStats::minColourValue() = 0.2.
> >  	 */
> > -	auto &gains = context.activeState.awb.gains;
> > -	gains = { {
> > -		sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(),
> > -		1.0,
> > -		sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(),
> > -	} };
> > -
> > -	RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
> > -	context.activeState.awb.temperatureK = estimateCCT(rgbGains);
> > -	metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);
> > -
> > -	LOG(IPASoftAwb, Debug)
> > -		<< "gain R/B: " << gains << "; temperature: "
> > -		<< context.activeState.awb.temperatureK;
> > +	return SimpleAwbStats(rgbMeans);
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::process
> > + */
> > +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > +		  IPAFrameContext &frameContext, const SwIspStats *stats,
> > +		  ControlList &metadata)
> > +{
> > +	SimpleAwbStats awbStats = calculateRgbMeans(context, stats);
> > +
> > +	awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,
> > +			 kDefaultLux, metadata);
> >  }
> >
> >  REGISTER_IPA_ALGORITHM(Awb, "Awb")
> > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
> > index ad993f39c180..fec48c6eea1c 100644
> > --- a/src/ipa/simple/algorithms/awb.h
> > +++ b/src/ipa/simple/algorithms/awb.h
> > @@ -7,19 +7,37 @@
> >
> >  #pragma once
> >
> > +#include <libcamera/controls.h>
> > +
> > +#include "libcamera/internal/software_isp/debayer_params.h"
> > +#include "libcamera/internal/value_node.h"
> > +
> > +#include "libipa/awb.h"
> > +#include "libipa/fixedpoint.h"
> > +#include "simple/ipa_context.h"
> > +
> >  #include "algorithm.h"
> >
> >  namespace libcamera {
> >
> >  namespace ipa::soft::algorithms {
> >
> > +class SimpleAwbStats;
> > +
> >  class Awb : public Algorithm
> >  {
> >  public:
> >  	Awb() = default;
> >  	~Awb() = default;
> >
> > +	int init(IPAContext &context,
> > +		 const ValueNode &tuningData) override;
> >  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > +
> > +	void queueRequest(IPAContext &context,
> > +			  [[maybe_unused]] const uint32_t frame,
> > +			  IPAFrameContext &frameContext,
> > +			  const ControlList &controls) override;
> >  	void prepare(IPAContext &context,
> >  		     const uint32_t frame,
> >  		     IPAFrameContext &frameContext,
> > @@ -29,6 +47,17 @@ public:
> >  		     IPAFrameContext &frameContext,
> >  		     const SwIspStats *stats,
> >  		     ControlList &metadata) override;
> > +
> > +private:
> > +	SimpleAwbStats calculateRgbMeans(IPAContext &context,
> > +					 const SwIspStats *stats) const;
> > +
> > +	/*
> > +	 * There actually is no Q register format for SoftISP, but allow the
> > +	 * colour gains to range in the [0.0f, 3.999f] interval, which seems
> > +	 * reasonable.
> > +	 */
> > +	AwbAlgorithm<UQ<2, 8>> awbAlgo_;
> >  };
> >
> >  } /* namespace ipa::soft::algorithms */
> > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> > index 677be56ed669..e45a913cd970 100644
> > --- a/src/ipa/simple/algorithms/blc.cpp
> > +++ b/src/ipa/simple/algorithms/blc.cpp
> > @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,
> >  			 DebayerParams *params)
> >  {
> >  	/* Latch the blacklevel gain so GPUISP can apply. */
> > -	params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
> > +	params->blackLevel = RGB<double>(context.activeState.blc.level / 255.0f);
> >  }
> >
> >  void BlackLevel::process(IPAContext &context,
> > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> > index ace9c35dc462..1174784edc7e 100644
> > --- a/src/ipa/simple/algorithms/ccm.cpp
> > +++ b/src/ipa/simple/algorithms/ccm.cpp
> > @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)
> >  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  		  IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
> >  {
> > -	const unsigned int ct = context.activeState.awb.temperatureK;
> > +	const unsigned int ct = frameContext.awb.colourTemperature;
> >
> >  	/* Change CCM only on bigger temperature changes. */
> >  	if (!currentCcm_ ||
> > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> > index 8ccfacb46a59..29643a655ce1 100644
> > --- a/src/ipa/simple/ipa_context.h
> > +++ b/src/ipa/simple/ipa_context.h
> > @@ -16,6 +16,7 @@
> >  #include "libcamera/internal/matrix.h"
> >  #include "libcamera/internal/vector.h"
> >
> > +#include <libipa/awb.h>
> >  #include <libipa/fc_queue.h>
> >
> >  #include "core_ipa_interface.h"
> > @@ -36,6 +37,8 @@ struct IPASessionConfiguration {
> >  };
> >
> >  struct IPAActiveState {
> > +	ipa::awb::ActiveState awb;
> > +
> >  	struct {
> >  		int32_t exposure;
> >  		double again;
> > @@ -48,11 +51,6 @@ struct IPAActiveState {
> >  		double lastGain;
> >  	} blc;
> >
> > -	struct {
> > -		RGB<float> gains;
> > -		unsigned int temperatureK;
> > -	} awb;
> > -
> >  	Matrix<float, 3, 3> combinedMatrix;
> >
> >  	struct {
> > @@ -64,6 +62,8 @@ struct IPAActiveState {
> >  };
> >
> >  struct IPAFrameContext : public FrameContext {
> > +	ipa::awb::FrameContext awb;
> > +
> >  	Matrix<float, 3, 3> ccm;
> >
> >  	struct {
> > @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {
> >  		double gain;
> >  	} sensor;
> >
> > -	RGB<float> gains;
> > -
> >  	float gamma;
> >  	std::optional<float> contrast;
> >  	std::optional<float> saturation;
> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> > index 49382b4c2719..ab22635fdfaf 100644
> > --- a/src/libcamera/software_isp/debayer_cpu.cpp
> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> > @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
> >  	};
> >  	const unsigned int gammaTableSize = gammaTable_.size();
> >
> > -	const RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
> > -	const RGB<float> gains = params.gains;
> > -	const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0);
> > +	const RGB<double> blackIndex = params.blackLevel * kRGBLookupSize;
> > +	const RGB<double> gains = params.gains;
> > +	const RGB<double> div = (RGB<double>(kRGBLookupSize) - blackIndex).max(1.0);
> >
> >  	if (ccmEnabled_) {
> >  		if (gammaUpdateNeeded ||
> > @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
> >  			const unsigned int greenIndex = 1;
> >  			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
> >  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> > -				const RGB<float> rgb = (gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div)
> > +				const RGB<double> rgb = (gains * (RGB<double>(i) - blackIndex) * kRGBLookupSize / div)
> >  							       .clamp(0.0, kRGBLookupSize - 1);
> >  				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
> >  				red[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);
> > @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
> >  			auto &green = green_;
> >  			auto &blue = swapRedBlueGains_ ? red_ : blue_;
> >  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> > -				const RGB<float> lutGains =
> > -					(gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div)
> > +				const RGB<double> lutGains =
> > +					(gains * (RGB<double>(i) - blackIndex) * gammaTableSize / div)
> >  						.clamp(0.0, gammaTableSize - 1);
> >  				red[i] = gammaTable_[lutGains.r()];
> >  				green[i] = gammaTable_[lutGains.g()];
>

Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:

> Hi Milan
>
> On Fri, Jul 10, 2026 at 11:25:30AM +0200, Milan Zamazal wrote:
>> Hi Jacopo,
>>
>> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>>
>> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> >
>> > Port the SoftISP Awb algorithm to use the new libipa implementation
>> > of AwbAlgorithm.
>> >
>> > The awbAlgo_ class member is initialized with the Q<4, 8> type even if
>> > there is no physical register representation for SoftISP.
>> >
>> > The usage of libipa::awb::Context, which defines the gains vector as
>> > RGB<double>, in the SimpleIPA ActiveState and FrameContext requires
>> > changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB<float>
>> > and RGB<double>.
>> >
>> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> > Tested-by: Robert Mader <robert.mader@collabora.com>
>> > ---
>> >  .../internal/software_isp/debayer_params.h         |   4 +-
>> >  src/ipa/simple/algorithms/awb.cpp                  | 141 ++++++++++++++++-----
>> >  src/ipa/simple/algorithms/awb.h                    |  29 +++++
>> >  src/ipa/simple/algorithms/blc.cpp                  |   2 +-
>> >  src/ipa/simple/algorithms/ccm.cpp                  |   2 +-
>> >  src/ipa/simple/ipa_context.h                       |  12 +-
>> >  src/libcamera/software_isp/debayer_cpu.cpp         |  12 +-
>> >  7 files changed, 151 insertions(+), 51 deletions(-)
>> >
>> > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
>> > index 6772b43bced4..1074720d73c2 100644
>> > --- a/include/libcamera/internal/software_isp/debayer_params.h
>> > +++ b/include/libcamera/internal/software_isp/debayer_params.h
>> > @@ -21,10 +21,10 @@ struct DebayerParams {
>> >  	Matrix<float, 3, 3> combinedMatrix = { { 1.0, 0.0, 0.0,
>> >  						 0.0, 1.0, 0.0,
>> >  						 0.0, 0.0, 1.0 } };
>> > -	RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 });
>> > +	RGB<double> blackLevel = RGB<double>({ 0.0, 0.0, 0.0 });
>> >  	float gamma = 1.0;
>> >  	float contrastExp = 1.0;
>> > -	RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 });
>> > +	RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 });
>> >  };
>> >
>> >  } /* namespace libcamera */
>> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
>> > index 05155c83d172..64ce4f91b21c 100644
>> > --- a/src/ipa/simple/algorithms/awb.cpp
>> > +++ b/src/ipa/simple/algorithms/awb.cpp
>> > @@ -15,7 +15,6 @@
>> >  #include <libcamera/control_ids.h>
>> >
>> >  #include "libipa/colours.h"
>>
>> This include can be removed too.
>>
>
> ack!
>
>> > -#include "simple/ipa_context.h"
>> >
>> >  namespace libcamera {
>> >
>> > @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)
>> >
>> >  namespace ipa::soft::algorithms {
>> >
>> > +/*
>> > + * \todo Replace it with a proper Lux algorithm
>> > + */
>> > +static constexpr unsigned int kDefaultLux = 500;
>> > +
>> > +class SimpleAwbStats final : public AwbStats
>>
>> This class is the same across three pipelines -- should it be a part of
>> libipa?
>>
>
> Eh, that's what I had initially, but I've been asked to move the
> implementation to each single IPA. I understand why, right now we just
> copied around the RkISP1 implementation instead of having each
> platform calculate statistics in their own way and moving the RkISP1
> implementation to the base class would have communicated that this is
> what we expect all platforms to do, while in reality, we just copied
> code around.

OK, let's see...

>> > +{
>> > +public:
>> > +	SimpleAwbStats() = default;
>> > +
>> > +	SimpleAwbStats(const RGB<double> &rgbMeans)
>> > +	{
>> > +		rgbMeans_ = rgbMeans;
>> > +
>> > +		rg_ = rgbMeans_.r() / rgbMeans_.g();
>> > +		bg_ = rgbMeans_.b() / rgbMeans_.g();
>> > +	}
>> > +
>> > +	double computeColourError(const RGB<double> &gains) const override
>> > +	{
>> > +		/*
>> > +		 * Compute the sum of the squared colour error (non-greyness) as
>> > +		 * it appears in the log likelihood equation.
>> > +		 */
>> > +		double deltaR = gains.r() * rg_ - 1.0;
>> > +		double deltaB = gains.b() * bg_ - 1.0;
>> > +		double delta2 = deltaR * deltaR + deltaB * deltaB;
>> > +
>> > +		return delta2;
>> > +	}
>> > +
>> > +	RGB<double> rgbMeans() const override
>> > +	{
>> > +		return rgbMeans_;
>> > +	}
>> > +
>> > +	bool valid() const override
>> > +	{
>> > +		/* Minimum mean value below which AWB can't operate. */
>> > +		constexpr double minValue = 0.2;
>> > +
>> > +		return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
>> > +		       rgbMeans_.b() > minValue;
>> > +	}
>> > +
>> > +private:
>> > +	RGB<double> rgbMeans_;
>> > +	double rg_;
>> > +	double bg_;
>> > +};
>> > +
>> > +/**
>> > + * \copydoc libcamera::ipa::Algorithm::init
>> > + */
>> > +int Awb::init(IPAContext &context, const ValueNode &tuningData)
>> > +{
>> > +	return awbAlgo_.init(tuningData, context.ctrlMap);
>> > +}
>> > +
>> > +/**
>> > + * \copydoc libcamera::ipa::Algorithm::configure
>> > + */
>> >  int Awb::configure(IPAContext &context,
>> >  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>> >  {
>> > -	auto &gains = context.activeState.awb.gains;
>> > -	gains = { { 1.0, 1.0, 1.0 } };
>> > +	return awbAlgo_.configure(context.activeState.awb);
>> > +}
>> >
>> > -	return 0;
>> > +/**
>> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
>> > + */
>> > +void Awb::queueRequest(IPAContext &context,
>> > +		       const uint32_t frame,
>> > +		       IPAFrameContext &frameContext,
>> > +		       const ControlList &controls)
>> > +{
>> > +	awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
>> > +			      controls);
>> >  }
>> >
>> > +/**
>> > + * \copydoc libcamera::ipa::Algorithm::prepare
>> > + */
>> >  void Awb::prepare(IPAContext &context,
>> >  		  [[maybe_unused]] const uint32_t frame,
>> >  		  IPAFrameContext &frameContext,
>> >  		  DebayerParams *params)
>> >  {
>> > -	auto &gains = context.activeState.awb.gains;
>> > +	awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
>> >
>> > -	frameContext.gains = gains;
>> > -	params->gains = gains;
>> > +	params->gains = frameContext.awb.gains;
>> >  }
>> >
>> > -void Awb::process(IPAContext &context,
>> > -		  [[maybe_unused]] const uint32_t frame,
>> > -		  IPAFrameContext &frameContext,
>> > -		  const SwIspStats *stats,
>> > -		  ControlList &metadata)
>> > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,
>> > +				      const SwIspStats *stats) const
>> >  {
>> > +	if (!stats->valid)
>> > +		return {};
>> > +
>> >  	const SwIspStats::Histogram &histogram = stats->yHistogram;
>> >  	const uint8_t blackLevel = context.activeState.blc.level;
>> >
>> > -	metadata.set(controls::ColourGains, { frameContext.gains.r(),
>> > -					      frameContext.gains.b() });
>> > -
>> > -	if (!stats->valid)
>> > -		return;
>> > -
>> >  	/*
>> >  	 * Black level must be subtracted to get the correct AWB ratios, they
>> >  	 * would be off if they were computed from the whole brightness range
>> > @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,
>> >  		histogram.begin(), histogram.end(), uint64_t(0));
>> >  	const uint64_t offset = blackLevel * nPixels;
>> >  	const uint64_t minValid = 1;
>> > +
>> >  	/*
>> >  	 * Make sure the sums are at least minValid, while preventing unsigned
>> >  	 * integer underflow.
>> >  	 */
>> >  	const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset;
>> >
>> > +	RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels),
>> > +				   static_cast<double>(sum.g() / nPixels),
>> > +				   static_cast<double>(sum.b() / nPixels) } };
>>
>>
>> It would be a little bit more correct to cast the sums, before applying
>> the division, rather than the integer division result:
>>
>>   static_cast<double>(sum.r()) / nPixels
>>
>> > +
>> >  	/*
>> > -	 * Calculate red and blue gains for AWB.
>> > -	 * Clamp max gain at 4.0, this also avoids 0 division.
>> > +	 * \todo Determine the minimum allowed thresholds from the mean
>> > +	 * but we currently have the sum - not the mean value!
>>
>> I don't understand the TODO -- what is it talking about and what mean
>> and sum are referred here?
>
> To be honest I don't know how the soft ISP calculate statistics and I
> didn't write the \todo, but if I just look at struct SwIspStats
> definition:
>
> 	/**
> 	 * \brief Sums of colour channels of all the sampled pixels
> 	 */
> 	RGB<uint64_t> sum_;
>
> I presume to generate stats the image is divided into grids, the mean
> value of each colour channel is calculated per zone, the averaged and
> reported in sums_.

There are no real zones, the image may be split by lines to areas
processed separately by different threads (if enabled), purely for
speedup.  But there is no inherent reason why zones couldn't be used in
future, especially in GPU ISP.

> So we don't have a per-window mean value which we can compared with
> SimpleAwbStats::minColourValue() to discard zones with not enough
> colour information, but we only have the sum when we get here.
>
> I guess the comment means we can't really discard zones by filtering
> on the mean colour value.

I see, thank you for clarification.  Since there are currently no zones,
SimpleAwbStats::minColourValue() mentioned below doesn't exist, and the
\todo is especially confusing in the context of the (whole image) RGB
means computed just above it, I'd suggest dropping the whole comment.
If anybody thinks it's important to retain it, a clearer wording should
be suggested.

>>
>> > +	 *
>> > +	 * Currently set to SimpleAwbStats::minColourValue() = 0.2.
>> >  	 */
>> > -	auto &gains = context.activeState.awb.gains;
>> > -	gains = { {
>> > -		sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(),
>> > -		1.0,
>> > -		sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(),
>> > -	} };
>> > -
>> > -	RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
>> > -	context.activeState.awb.temperatureK = estimateCCT(rgbGains);
>> > -	metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK);
>> > -
>> > -	LOG(IPASoftAwb, Debug)
>> > -		<< "gain R/B: " << gains << "; temperature: "
>> > -		<< context.activeState.awb.temperatureK;
>> > +	return SimpleAwbStats(rgbMeans);
>> > +}
>> > +
>> > +/**
>> > + * \copydoc libcamera::ipa::Algorithm::process
>> > + */
>> > +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>> > +		  IPAFrameContext &frameContext, const SwIspStats *stats,
>> > +		  ControlList &metadata)
>> > +{
>> > +	SimpleAwbStats awbStats = calculateRgbMeans(context, stats);
>> > +
>> > +	awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats,
>> > +			 kDefaultLux, metadata);
>> >  }
>> >
>> >  REGISTER_IPA_ALGORITHM(Awb, "Awb")
>> > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
>> > index ad993f39c180..fec48c6eea1c 100644
>> > --- a/src/ipa/simple/algorithms/awb.h
>> > +++ b/src/ipa/simple/algorithms/awb.h
>> > @@ -7,19 +7,37 @@
>> >
>> >  #pragma once
>> >
>> > +#include <libcamera/controls.h>
>> > +
>> > +#include "libcamera/internal/software_isp/debayer_params.h"
>> > +#include "libcamera/internal/value_node.h"
>> > +
>> > +#include "libipa/awb.h"
>> > +#include "libipa/fixedpoint.h"
>> > +#include "simple/ipa_context.h"
>> > +
>> >  #include "algorithm.h"
>> >
>> >  namespace libcamera {
>> >
>> >  namespace ipa::soft::algorithms {
>> >
>> > +class SimpleAwbStats;
>> > +
>> >  class Awb : public Algorithm
>> >  {
>> >  public:
>> >  	Awb() = default;
>> >  	~Awb() = default;
>> >
>> > +	int init(IPAContext &context,
>> > +		 const ValueNode &tuningData) override;
>> >  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>> > +
>> > +	void queueRequest(IPAContext &context,
>> > +			  [[maybe_unused]] const uint32_t frame,
>> > +			  IPAFrameContext &frameContext,
>> > +			  const ControlList &controls) override;
>> >  	void prepare(IPAContext &context,
>> >  		     const uint32_t frame,
>> >  		     IPAFrameContext &frameContext,
>> > @@ -29,6 +47,17 @@ public:
>> >  		     IPAFrameContext &frameContext,
>> >  		     const SwIspStats *stats,
>> >  		     ControlList &metadata) override;
>> > +
>> > +private:
>> > +	SimpleAwbStats calculateRgbMeans(IPAContext &context,
>> > +					 const SwIspStats *stats) const;
>> > +
>> > +	/*
>> > +	 * There actually is no Q register format for SoftISP, but allow the
>> > +	 * colour gains to range in the [0.0f, 3.999f] interval, which seems
>> > +	 * reasonable.
>> > +	 */
>> > +	AwbAlgorithm<UQ<2, 8>> awbAlgo_;
>> >  };
>> >
>> >  } /* namespace ipa::soft::algorithms */
>> > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> > index 677be56ed669..e45a913cd970 100644
>> > --- a/src/ipa/simple/algorithms/blc.cpp
>> > +++ b/src/ipa/simple/algorithms/blc.cpp
>> > @@ -53,7 +53,7 @@ void BlackLevel::prepare(IPAContext &context,
>> >  			 DebayerParams *params)
>> >  {
>> >  	/* Latch the blacklevel gain so GPUISP can apply. */
>> > -	params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
>> > +	params->blackLevel = RGB<double>(context.activeState.blc.level / 255.0f);
>> >  }
>> >
>> >  void BlackLevel::process(IPAContext &context,
>> > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> > index ace9c35dc462..1174784edc7e 100644
>> > --- a/src/ipa/simple/algorithms/ccm.cpp
>> > +++ b/src/ipa/simple/algorithms/ccm.cpp
>> > @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData)
>> >  void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>> >  		  IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>> >  {
>> > -	const unsigned int ct = context.activeState.awb.temperatureK;
>> > +	const unsigned int ct = frameContext.awb.colourTemperature;
>> >
>> >  	/* Change CCM only on bigger temperature changes. */
>> >  	if (!currentCcm_ ||
>> > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> > index 8ccfacb46a59..29643a655ce1 100644
>> > --- a/src/ipa/simple/ipa_context.h
>> > +++ b/src/ipa/simple/ipa_context.h
>> > @@ -16,6 +16,7 @@
>> >  #include "libcamera/internal/matrix.h"
>> >  #include "libcamera/internal/vector.h"
>> >
>> > +#include <libipa/awb.h>
>> >  #include <libipa/fc_queue.h>
>> >
>> >  #include "core_ipa_interface.h"
>> > @@ -36,6 +37,8 @@ struct IPASessionConfiguration {
>> >  };
>> >
>> >  struct IPAActiveState {
>> > +	ipa::awb::ActiveState awb;
>> > +
>> >  	struct {
>> >  		int32_t exposure;
>> >  		double again;
>> > @@ -48,11 +51,6 @@ struct IPAActiveState {
>> >  		double lastGain;
>> >  	} blc;
>> >
>> > -	struct {
>> > -		RGB<float> gains;
>> > -		unsigned int temperatureK;
>> > -	} awb;
>> > -
>> >  	Matrix<float, 3, 3> combinedMatrix;
>> >
>> >  	struct {
>> > @@ -64,6 +62,8 @@ struct IPAActiveState {
>> >  };
>> >
>> >  struct IPAFrameContext : public FrameContext {
>> > +	ipa::awb::FrameContext awb;
>> > +
>> >  	Matrix<float, 3, 3> ccm;
>> >
>> >  	struct {
>> > @@ -71,8 +71,6 @@ struct IPAFrameContext : public FrameContext {
>> >  		double gain;
>> >  	} sensor;
>> >
>> > -	RGB<float> gains;
>> > -
>> >  	float gamma;
>> >  	std::optional<float> contrast;
>> >  	std::optional<float> saturation;
>> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> > index 49382b4c2719..ab22635fdfaf 100644
>> > --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> > @@ -1010,9 +1010,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>> >  	};
>> >  	const unsigned int gammaTableSize = gammaTable_.size();
>> >
>> > -	const RGB<float> blackIndex = params.blackLevel * kRGBLookupSize;
>> > -	const RGB<float> gains = params.gains;
>> > -	const RGB<float> div = (RGB<float>(kRGBLookupSize) - blackIndex).max(1.0);
>> > +	const RGB<double> blackIndex = params.blackLevel * kRGBLookupSize;
>> > +	const RGB<double> gains = params.gains;
>> > +	const RGB<double> div = (RGB<double>(kRGBLookupSize) - blackIndex).max(1.0);
>> >
>> >  	if (ccmEnabled_) {
>> >  		if (gammaUpdateNeeded ||
>> > @@ -1025,7 +1025,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>> >  			const unsigned int greenIndex = 1;
>> >  			const unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;
>> >  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
>> > -				const RGB<float> rgb = (gains * (RGB<float>(i) - blackIndex) * kRGBLookupSize / div)
>> > +				const RGB<double> rgb = (gains * (RGB<double>(i) - blackIndex) * kRGBLookupSize / div)
>> >  							       .clamp(0.0, kRGBLookupSize - 1);
>> >  				red[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);
>> >  				red[i].g = std::round(rgb.r() * params.combinedMatrix[greenIndex][0]);
>> > @@ -1045,8 +1045,8 @@ void DebayerCpu::updateLookupTables(const DebayerParams &params)
>> >  			auto &green = green_;
>> >  			auto &blue = swapRedBlueGains_ ? red_ : blue_;
>> >  			for (unsigned int i = 0; i < kRGBLookupSize; i++) {
>> > -				const RGB<float> lutGains =
>> > -					(gains * (RGB<float>(i) - blackIndex) * gammaTableSize / div)
>> > +				const RGB<double> lutGains =
>> > +					(gains * (RGB<double>(i) - blackIndex) * gammaTableSize / div)
>> >  						.clamp(0.0, gammaTableSize - 1);
>> >  				red[i] = gammaTable_[lutGains.r()];
>> >  				green[i] = gammaTable_[lutGains.g()];
>>

Quoting Milan Zamazal (2026-07-10 16:21:22)
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
> 
> > Hi Milan
> >
> > On Fri, Jul 10, 2026 at 11:25:30AM +0200, Milan Zamazal wrote:
> >> Hi Jacopo,
> >>
> >> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
> >>
> >> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> >
> >> > Port the SoftISP Awb algorithm to use the new libipa implementation
> >> > of AwbAlgorithm.
> >> >
> >> > The awbAlgo_ class member is initialized with the Q<4, 8> type even if
> >> > there is no physical register representation for SoftISP.
> >> >
> >> > The usage of libipa::awb::Context, which defines the gains vector as
> >> > RGB<double>, in the SimpleIPA ActiveState and FrameContext requires
> >> > changes to debayer_cpu.cpp and blc.cpp in order not to mix RGB<float>
> >> > and RGB<double>.
> >> >
> >> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> > Tested-by: Robert Mader <robert.mader@collabora.com>
> >> > ---
> >> >  .../internal/software_isp/debayer_params.h         |   4 +-

<snip>

> >> > -#include "simple/ipa_context.h"
> >> >
> >> >  namespace libcamera {
> >> >
> >> > @@ -23,41 +22,108 @@ LOG_DEFINE_CATEGORY(IPASoftAwb)
> >> >
> >> >  namespace ipa::soft::algorithms {
> >> >
> >> > +/*
> >> > + * \todo Replace it with a proper Lux algorithm
> >> > + */
> >> > +static constexpr unsigned int kDefaultLux = 500;
> >> > +
> >> > +class SimpleAwbStats final : public AwbStats
> >>
> >> This class is the same across three pipelines -- should it be a part of
> >> libipa?
> >>
> >
> > Eh, that's what I had initially, but I've been asked to move the
> > implementation to each single IPA. I understand why, right now we just
> > copied around the RkISP1 implementation instead of having each
> > platform calculate statistics in their own way and moving the RkISP1
> > implementation to the base class would have communicated that this is
> > what we expect all platforms to do, while in reality, we just copied
> > code around.
> 
> OK, let's see...
> 
> >> > +{
> >> > +public:
> >> > +  SimpleAwbStats() = default;
> >> > +
> >> > +  SimpleAwbStats(const RGB<double> &rgbMeans)
> >> > +  {
> >> > +          rgbMeans_ = rgbMeans;
> >> > +
> >> > +          rg_ = rgbMeans_.r() / rgbMeans_.g();
> >> > +          bg_ = rgbMeans_.b() / rgbMeans_.g();
> >> > +  }
> >> > +
> >> > +  double computeColourError(const RGB<double> &gains) const override
> >> > +  {
> >> > +          /*
> >> > +           * Compute the sum of the squared colour error (non-greyness) as
> >> > +           * it appears in the log likelihood equation.
> >> > +           */
> >> > +          double deltaR = gains.r() * rg_ - 1.0;
> >> > +          double deltaB = gains.b() * bg_ - 1.0;
> >> > +          double delta2 = deltaR * deltaR + deltaB * deltaB;
> >> > +
> >> > +          return delta2;
> >> > +  }
> >> > +
> >> > +  RGB<double> rgbMeans() const override
> >> > +  {
> >> > +          return rgbMeans_;
> >> > +  }
> >> > +
> >> > +  bool valid() const override
> >> > +  {
> >> > +          /* Minimum mean value below which AWB can't operate. */
> >> > +          constexpr double minValue = 0.2;
> >> > +
> >> > +          return rgbMeans_.r() > minValue || rgbMeans_.g() > minValue ||
> >> > +                 rgbMeans_.b() > minValue;
> >> > +  }
> >> > +
> >> > +private:
> >> > +  RGB<double> rgbMeans_;
> >> > +  double rg_;
> >> > +  double bg_;
> >> > +};
> >> > +
> >> > +/**
> >> > + * \copydoc libcamera::ipa::Algorithm::init
> >> > + */
> >> > +int Awb::init(IPAContext &context, const ValueNode &tuningData)
> >> > +{
> >> > +  return awbAlgo_.init(tuningData, context.ctrlMap);
> >> > +}
> >> > +
> >> > +/**
> >> > + * \copydoc libcamera::ipa::Algorithm::configure
> >> > + */
> >> >  int Awb::configure(IPAContext &context,
> >> >               [[maybe_unused]] const IPAConfigInfo &configInfo)
> >> >  {
> >> > -  auto &gains = context.activeState.awb.gains;
> >> > -  gains = { { 1.0, 1.0, 1.0 } };
> >> > +  return awbAlgo_.configure(context.activeState.awb);
> >> > +}
> >> >
> >> > -  return 0;
> >> > +/**
> >> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> >> > + */
> >> > +void Awb::queueRequest(IPAContext &context,
> >> > +                 const uint32_t frame,
> >> > +                 IPAFrameContext &frameContext,
> >> > +                 const ControlList &controls)
> >> > +{
> >> > +  awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb,
> >> > +                        controls);
> >> >  }
> >> >
> >> > +/**
> >> > + * \copydoc libcamera::ipa::Algorithm::prepare
> >> > + */
> >> >  void Awb::prepare(IPAContext &context,
> >> >              [[maybe_unused]] const uint32_t frame,
> >> >              IPAFrameContext &frameContext,
> >> >              DebayerParams *params)
> >> >  {
> >> > -  auto &gains = context.activeState.awb.gains;
> >> > +  awbAlgo_.prepare(context.activeState.awb, frameContext.awb);
> >> >
> >> > -  frameContext.gains = gains;
> >> > -  params->gains = gains;
> >> > +  params->gains = frameContext.awb.gains;
> >> >  }
> >> >
> >> > -void Awb::process(IPAContext &context,
> >> > -            [[maybe_unused]] const uint32_t frame,
> >> > -            IPAFrameContext &frameContext,
> >> > -            const SwIspStats *stats,
> >> > -            ControlList &metadata)
> >> > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context,
> >> > +                                const SwIspStats *stats) const
> >> >  {
> >> > +  if (!stats->valid)
> >> > +          return {};
> >> > +
> >> >    const SwIspStats::Histogram &histogram = stats->yHistogram;
> >> >    const uint8_t blackLevel = context.activeState.blc.level;
> >> >
> >> > -  metadata.set(controls::ColourGains, { frameContext.gains.r(),
> >> > -                                        frameContext.gains.b() });
> >> > -
> >> > -  if (!stats->valid)
> >> > -          return;
> >> > -
> >> >    /*
> >> >     * Black level must be subtracted to get the correct AWB ratios, they
> >> >     * would be off if they were computed from the whole brightness range
> >> > @@ -67,30 +133,37 @@ void Awb::process(IPAContext &context,
> >> >            histogram.begin(), histogram.end(), uint64_t(0));
> >> >    const uint64_t offset = blackLevel * nPixels;
> >> >    const uint64_t minValid = 1;
> >> > +
> >> >    /*
> >> >     * Make sure the sums are at least minValid, while preventing unsigned
> >> >     * integer underflow.
> >> >     */
> >> >    const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset;
> >> >
> >> > +  RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels),
> >> > +                             static_cast<double>(sum.g() / nPixels),
> >> > +                             static_cast<double>(sum.b() / nPixels) } };
> >>
> >>
> >> It would be a little bit more correct to cast the sums, before applying
> >> the division, rather than the integer division result:
> >>
> >>   static_cast<double>(sum.r()) / nPixels

Can't we do anything helpful with
	constexpr Vector operator/(T scalar) const?

constexpr Vector operator/(T scalar) const
	{
		return apply(*this, std::divides<>{}, scalar);
	}


Or does the casting in from uint64_t still cause a fuss there anyway.
It would be nice if we could do:

	RGB<double> rgbMeans = static_cast<double>(sum) / nPixels;

But I think we'd have to add some sort of casting or template
constructor for that.

I think that's close to something I tried before but maybe it needs a
different thought.

Anyway, yes, we should do the cast first then the divide!

> >>
> >> > +
> >> >    /*
> >> > -   * Calculate red and blue gains for AWB.
> >> > -   * Clamp max gain at 4.0, this also avoids 0 division.
> >> > +   * \todo Determine the minimum allowed thresholds from the mean
> >> > +   * but we currently have the sum - not the mean value!
> >>
> >> I don't understand the TODO -- what is it talking about and what mean
> >> and sum are referred here?
> >

So it looks likely that I might have written that about 6 or 7 months
ago, but I don't really remember it.


https://git.ideasonboard.com/kbingham/libcamera/pulls/26/commits/54a7a8c7f674f281fd5bf59b7e0192e3e0cb1abd
has:

 		/*
		 * Todo: Determine the minimum allowed thresholds from the mean
		 * but we currently have the sum - not the mean value!
		 */
		SimpleAwbStats awbStats{ rgbMeans };

I'm not really sure what I meant - because ... we have the mean ? don't
we ?

Maybe we drop the comment/todo/



> > To be honest I don't know how the soft ISP calculate statistics and I
> > didn't write the \todo, but if I just look at struct SwIspStats
> > definition:
> >
> >       /**
> >        * \brief Sums of colour channels of all the sampled pixels
> >        */
> >       RGB<uint64_t> sum_;
> >
> > I presume to generate stats the image is divided into grids, the mean
> > value of each colour channel is calculated per zone, the averaged and
> > reported in sums_.
> 
> There are no real zones, the image may be split by lines to areas
> processed separately by different threads (if enabled), purely for
> speedup.  But there is no inherent reason why zones couldn't be used in
> future, especially in GPU ISP.
> 
> > So we don't have a per-window mean value which we can compared with
> > SimpleAwbStats::minColourValue() to discard zones with not enough
> > colour information, but we only have the sum when we get here.
> >
> > I guess the comment means we can't really discard zones by filtering
> > on the mean colour value.
> 
> I see, thank you for clarification.  Since there are currently no zones,
> SimpleAwbStats::minColourValue() mentioned below doesn't exist, and the
> \todo is especially confusing in the context of the (whole image) RGB
> means computed just above it, I'd suggest dropping the whole comment.
> If anybody thinks it's important to retain it, a clearer wording should
> be suggested.

Maybe drop it if it's not adding value. I won't be offended :-) I
presume I wrote it while reading something from a different IPA that I
was interpreting while trying to shoe-horn this in here ?

--
Kieran