[PATCH v6 3/9] ipa: rkisp1: awb: Implement ColourTemperature control

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Thu, Dec 19, 2024 at 06:57:20PM +0100, Stefan Klug wrote:
> There are many use-cases (tuning-validation, working in static
> environments) where a manual ColourTemperature control is helpful.
> Implement that by interpolating and applying the white balance gains
> from the tuning file according to the requested colour temperature. If
> colour gains are provided on the same request, they take precedence.
> Store the colour temperature used for a given frame in the frame context
> and report that in metadata.
> 
> Note that in the automatic case, the colour gains are still based on the
> gray world model and the CT curve from the tuning file get ignored.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> 
> Changes in v6:
> - Updated commit message
> - Moved retrieval of controls to the place where needed
> - Output the colour temperature applied to a frame in metadata
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 51 +++++++++++++++++++++++--------
>  src/ipa/rkisp1/ipa_context.h      |  1 +
>  2 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index e23f67a96edf..cffaa06a22c1 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -33,6 +33,10 @@ namespace ipa::rkisp1::algorithms {
>  
>  LOG_DEFINE_CATEGORY(RkISP1Awb)
>  
> +constexpr int32_t kMinColourTemperature = 2500;
> +constexpr int32_t kMaxColourTemperature = 10000;
> +constexpr int32_t kDefaultColourTemperature = 5000;
> +
>  /* Minimum mean value below which AWB can't operate. */
>  constexpr double kMeanMinThreshold = 2.0;
>  
> @@ -44,8 +48,13 @@ Awb::Awb()
>  /**
>   * \copydoc libcamera::ipa::Algorithm::init
>   */
> -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> +int Awb::init(IPAContext &context, const YamlObject &tuningData)
>  {
> +	auto &cmap = context.ctrlMap;
> +	cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> +							 kMaxColourTemperature,
> +							 kDefaultColourTemperature);
> +
>  	Interpolator<Vector<double, 2>> gainCurve;
>  	int ret = gainCurve.readYaml(tuningData["colourGains"], "ct", "gains");
>  	if (ret < 0)
> @@ -68,6 +77,7 @@ int Awb::configure(IPAContext &context,
>  	context.activeState.awb.gains.manual = RGB<double>{ 1.0 };
>  	context.activeState.awb.gains.automatic = RGB<double>{ 1.0 };
>  	context.activeState.awb.autoEnabled = true;
> +	context.activeState.awb.temperatureK = kDefaultColourTemperature;
>  
>  	/*
>  	 * Define the measurement window for AWB as a centered rectangle
> @@ -101,19 +111,37 @@ void Awb::queueRequest(IPAContext &context,
>  			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
>  	}
>  
> +	frameContext.awb.autoEnabled = awb.autoEnabled;
> +
> +	if (awb.autoEnabled)
> +		return;
> +
>  	const auto &colourGains = controls.get(controls::ColourGains);
> -	if (colourGains && !awb.autoEnabled) {
> +	const auto &colourTemperature = controls.get(controls::ColourTemperature);
> +	bool update = false;
> +	if (colourGains) {
>  		awb.gains.manual.r() = (*colourGains)[0];
>  		awb.gains.manual.b() = (*colourGains)[1];
> +		/*
> +		 * \todo: Colour temperature reported in metadata is now
> +		 * incorrect, as we can't deduce the temperature from the gains.
> +		 * This will be fixed with the bayes AWB algorithm.
> +		 */

Will we require all cameras to implement bayesian AWB ? If not, the the
controls documentation need to allow that. I suppose that
ColourTemperature will then not be reported as a settable control by the
camera, so application will know.

I'm fine clarifying the documentation on top of this series. Can you
make sure it gets done ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		update = true;
> +	} else if (colourTemperature && colourGainCurve_) {
> +		const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);
> +		awb.gains.manual.r() = gains[0];
> +		awb.gains.manual.b() = gains[1];
> +		awb.temperatureK = *colourTemperature;
> +		update = true;
> +	}
>  
> +	if (update)
>  		LOG(RkISP1Awb, Debug)
>  			<< "Set colour gains to " << awb.gains.manual;
> -	}
>  
> -	frameContext.awb.autoEnabled = awb.autoEnabled;
> -
> -	if (!awb.autoEnabled)
> -		frameContext.awb.gains = awb.gains.manual;
> +	frameContext.awb.gains = awb.gains.manual;
> +	frameContext.awb.temperatureK = awb.temperatureK;
>  }
>  
>  /**
> @@ -126,8 +154,10 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,
>  	 * This is the latest time we can read the active state. This is the
>  	 * most up-to-date automatic values we can read.
>  	 */
> -	if (frameContext.awb.autoEnabled)
> +	if (frameContext.awb.autoEnabled) {
>  		frameContext.awb.gains = context.activeState.awb.gains.automatic;
> +		frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
> +	}
>  
>  	auto gainConfig = params->block<BlockType::AwbGain>();
>  	gainConfig.setEnabled(true);
> @@ -206,7 +236,7 @@ void Awb::process(IPAContext &context,
>  			static_cast<float>(frameContext.awb.gains.r()),
>  			static_cast<float>(frameContext.awb.gains.b())
>  		});
> -	metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
> +	metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
>  
>  	if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {
>  		LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
> @@ -281,9 +311,6 @@ void Awb::process(IPAContext &context,
>  
>  	activeState.awb.temperatureK = estimateCCT(rgbMeans);
>  
> -	/* 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
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index deb8c196f1b8..4b50015beee8 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -135,6 +135,7 @@ struct IPAFrameContext : public FrameContext {
>  	struct {
>  		RGB<double> gains;
>  		bool autoEnabled;
> +		unsigned int temperatureK;
>  	} awb;
>  
>  	struct {

-- 
Regards,

Laurent Pinchart