[PATCH v4 3/6] ipa: rkisp1: awb: Implement ColourTemperature control

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Tue, Aug 13, 2024 at 10:44:20AM +0200, 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. As
> the colour temperature reported in the metadata is always based on the
> measurements, we don't have to touch that.

I thought the metadata would report the colour temperature that was set
manually. This likely calls for a clarification in the control
documentation.

Reading https://lists.libcamera.org/pipermail/libcamera-devel/2023-December/039733.html,
it sounds like the Raspberry Pi implementation would stop evaluating the
colour temperature when AWB is disabled. Do we need to leave this
behaviour undocumented and up to pipeline handlers ? I usually try to
avoid that. Can we standardize on one behaviour that would work for
everybody ?

> Note that in the automatic case, the colour gains are still based on the
> gray world model and the ones from the tuning file get ignored.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index c23f749c192b..d482eda5b541 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms {
>  
>  LOG_DEFINE_CATEGORY(RkISP1Awb)
>  
> +constexpr int32_t kMinColourTemperature = 2500;
> +constexpr int32_t kMaxColourTemperature = 10000;
> +constexpr int32_t kDefaultColourTemperature = 6500;
> +
>  /* Minimum mean value below which AWB can't operate. */
>  constexpr double kMeanMinThreshold = 2.0;
>  
> @@ -42,8 +46,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);
> +
>  	MatrixInterpolator<double, 2, 1> gains;
>  	int ret = gains.readYaml(tuningData["gains"], "ct", "gains");
>  	if (ret < 0)
> @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,
>  			<< ", blue: " << awb.gains.manual.blue;
>  	}
>  
> +	const auto &colourTemperature = controls.get(controls::ColourTemperature);
> +	if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {
> +		Matrix<double, 2, 1> gains = gains_->get(*colourTemperature);
> +		awb.gains.manual.red = gains[0][0];
> +		awb.gains.manual.blue = gains[1][0];
> +
> +		LOG(RkISP1Awb, Debug)
> +			<< "Set colour gains to red: " << awb.gains.manual.red
> +			<< ", blue: " << awb.gains.manual.blue;
> +	}

I wonder if the following would make the logic clearer:

	const auto &colourGains = controls.get(controls::ColourGains);
	const auto &colourTemperature = controls.get(controls::ColourTemperature);

	if (!awb.autoEnabled) {
		bool update = false;

		if (colourGains) {
			awb.gains.manual.red = (*colourGains)[0];
			awb.gains.manual.blue = (*colourGains)[1];
			update = true;
		} else if (colourTemperature && gains_) {
			Matrix<double, 2, 1> gains = gains_->get(*colourTemperature);
			awb.gains.manual.red = gains[0][0];
			awb.gains.manual.blue = gains[1][0];
			update = true;
		}

		if (update)
			LOG(RkISP1Awb, Debug)
				<< "Set colour gains to red: " << awb.gains.manual.red
				<< ", blue: " << awb.gains.manual.blue;
	}

> +
>  	frameContext.awb.autoEnabled = awb.autoEnabled;
>  
>  	if (!awb.autoEnabled) {

-- 
Regards,

Laurent Pinchart