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

[Dan Scally]

Morning Stefan

On 21/01/2025 06:08, Stefan Klug wrote:
> Hi Dan,
>
> Thank you for the review.
>
> On Mon, Jan 20, 2025 at 11:59:56AM +0000, Dan Scally wrote:
>> 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?
> Yes that would be possible. This was merely to get the patch series into
> logical blocks. It is removed in patch 10/11. I believe the "not
> implemented" shouldn't be a default case, so that the IPA implementation
> needs to take active action if something should not be implemented.
Alright, works for me
>
>>> +
>>> +	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?
> The reason was that the top level (lets' call it RkISPAwb) algorithm
> should support both cases grey world and bayes and be able to switch
> based on the tuning file.
Yes I encountered that when I reached the later patches - maybe I should read the whole series 
before commenting on anything :D. It's different but better than how I was expecting it to work, so 
it's good.
>   I like the clear separation of these two
> algorithms as it keeps the door open to easily improve by adding another
> one without breaking the old ones.
>
> We could add a "master-AWB" algorithm that contains the
> switching/instantiation logic and add that to libipa. This could then be
> subclassed by RkISPAwb. I don't know exactly if it is worth it. It
> depends a bit on how we see libipa: a) the place to put some often used
> building blocks for IPAs or b) the place where we put as much of the IPA
> logic as we can so that all IPAs behave the same eventually.  This
> implements a) but b) could later be added on top. What do you think?

Personally I think b is desirable, but I don't think it needs to be done right now - particularly 
since no other IPA to my knowledge has multiple implementations of the same algorithm yet.


So, all good by me:


Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>

>
> Cheers,
> Stefan
>
>>>    	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_;
>>>    };