[PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from libipa

[Dan Scally]

Hi Stefan

On 09/01/2025 11:53, Stefan Klug wrote:
> Now that libipa contains a grey world algorithm, use that.
>
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>   src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++-----------
>   src/ipa/rkisp1/algorithms/awb.h   |  4 +-
>   2 files changed, 49 insertions(+), 27 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index f6449565834b..42a4784998bc 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -16,6 +16,7 @@
>   
>   #include <libcamera/ipa/core_ipa_interface.h>
>   
> +#include "libipa/awb_grey.h"
>   #include "libipa/colours.h"
>   
>   /**
> @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000;
>   /* Minimum mean value below which AWB can't operate. */
>   constexpr double kMeanMinThreshold = 2.0;
>   
> +class RkISP1AwbStats : public AwbStats
> +{
> +public:
> +	RkISP1AwbStats(const RGB<double> &rgbMeans)
> +		: rgbMeans_(rgbMeans) {}
> +
> +	double computeColourError([[maybe_unused]] const RGB<double> &gains) const override
> +	{
> +		LOG(RkISP1Awb, Error)
> +			<< "RkISP1AwbStats::computeColourError is not implemented";
> +		return 0.0;
> +	}
If it's optional, perhaps rather than a pure virtual function it should have this "not implemented" 
variant in the base class?
> +
> +	RGB<double> getRGBMeans() const override
> +	{
> +		return rgbMeans_;
> +	};
> +
> +private:
> +	RGB<double> rgbMeans_;
> +};
> +
>   Awb::Awb()
>   	: rgbMode_(false)
>   {
> @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
>   							 kMaxColourTemperature,
>   							 kDefaultColourTemperature);
>   
> -	Interpolator<Vector<double, 2>> gainCurve;
> -	int ret = gainCurve.readYaml(tuningData["colourGains"], "ct", "gains");
> -	if (ret < 0)
> -		LOG(RkISP1Awb, Warning)
> -			<< "Failed to parse 'colourGains' "
> -			<< "parameter from tuning file; "
> -			<< "manual colour temperature will not work properly";
> -	else
> -		colourGainCurve_ = gainCurve;
> +	awbAlgo_ = std::make_unique<AwbGrey>();
> +	int ret = awbAlgo_->init(tuningData);
> +	if (ret) {
> +		LOG(RkISP1Awb, Error) << "Failed to init awb algorithm";
> +		return ret;
> +	}
I'm a bit surprised to see a pointer to the base class, I was expecting this class to derive the 
other one (and just call the base class init() here). Why'd you choose to do it this way?
>   
>   	return 0;
>   }
> @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context,
>   		 * This will be fixed with the bayes AWB algorithm.
>   		 */
>   		update = true;
> -	} else if (colourTemperature && colourGainCurve_) {
> -		const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);
> -		awb.gains.manual.r() = gains[0];
> -		awb.gains.manual.b() = gains[1];
> +	} else if (colourTemperature) {
> +		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> +		awb.gains.manual.r() = gains.r();
> +		awb.gains.manual.b() = gains.b();
>   		awb.temperatureK = *colourTemperature;
>   		update = true;
>   	}
> @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context,
>   	    rgbMeans.b() < kMeanMinThreshold)
>   		return;
>   
> -	activeState.awb.temperatureK = estimateCCT(rgbMeans);
> +	/*
> +	 * \Todo: Hardcode lux to a fixed value, until an estimation is
> +	 * implemented.
> +	 */
> +	int lux = 1000;
> +
> +	RkISP1AwbStats awbStats{ rgbMeans };
> +	AwbResult r = awbAlgo_->calculateAwb(awbStats, lux);
A more descriptive variable name would be slightly better I think; "r" in this context makes me 
think "red" not "results".
> +
> +	activeState.awb.temperatureK = r.colourTemperature;
>   
>   	/* Metadata shall contain the up to date measurement */
>   	metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
>   
> -	/*
> -	 * Estimate the red and blue gains to apply in a grey world. The green
> -	 * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the
> -	 * divisor to a minimum value of 1.0.
> -	 */
> -	RGB<double> gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0),
> -			    1.0,
> -			    rgbMeans.g() / std::max(rgbMeans.b(), 1.0) });
> -
>   	/*
>   	 * Clamp the gain values to the hardware, which expresses gains as Q2.8
>   	 * unsigned integer values. Set the minimum just above zero to avoid
>   	 * divisions by zero when computing the raw means in subsequent
>   	 * iterations.
>   	 */
> -	gains = gains.max(1.0 / 256).min(1023.0 / 256);
> +	r.gains = r.gains.max(1.0 / 256).min(1023.0 / 256);
>   
>   	/* Filter the values to avoid oscillations. */
>   	double speed = 0.2;
> -	gains = gains * speed + activeState.awb.gains.automatic * (1 - speed);
> +	r.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed);
>   
> -	activeState.awb.gains.automatic = gains;
> +	activeState.awb.gains.automatic = r.gains;
>   
>   	LOG(RkISP1Awb, Debug)
>   		<< std::showpoint
> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> index 2a64cb60d5f4..3382b2178567 100644
> --- a/src/ipa/rkisp1/algorithms/awb.h
> +++ b/src/ipa/rkisp1/algorithms/awb.h
> @@ -9,6 +9,7 @@
>   
>   #include <optional>
>   
> +#include "libipa/awb.h"
>   #include "libipa/interpolator.h"
>   #include "libipa/vector.h"
>   
> @@ -41,7 +42,8 @@ private:
>   	RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext,
>   				      const rkisp1_cif_isp_awb_stat *awb) const;
>   
> -	std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
> +	std::unique_ptr<AwbAlgorithm> awbAlgo_;
> +
>   	bool rgbMode_;
>   };
>