[libcamera-devel,RFC,v2,2/3] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm

11368 diff mbox series

Inherit from the Algorithm class to implement basic AWB functions.

Once AWB is done, a color temperature is estimated and default CCM matrices
are used (yet to be tuned).
Implement a basic "grey-world" AWB algorithm just for demonstration purpose.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp     |  26 +++--
 src/ipa/ipu3/ipu3_awb.cpp | 199 ++++++++++++++++++++++++++++++++++++++
 src/ipa/ipu3/ipu3_awb.h   |  47 +++++++++
 src/ipa/ipu3/meson.build  |   7 +-
 4 files changed, 270 insertions(+), 9 deletions(-)
 create mode 100644 src/ipa/ipu3/ipu3_awb.cpp
 create mode 100644 src/ipa/ipu3/ipu3_awb.h

Hi Jean-Michel,

Thank you for the patch.

On Tue, Feb 23, 2021 at 05:40:40PM +0100, Jean-Michel Hautbois wrote:
> Inherit from the Algorithm class to implement basic AWB functions.
> 
> Once AWB is done, a color temperature is estimated and default CCM matrices
> are used (yet to be tuned).
> Implement a basic "grey-world" AWB algorithm just for demonstration purpose.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     |  26 +++--
>  src/ipa/ipu3/ipu3_awb.cpp | 199 ++++++++++++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3_awb.h   |  47 +++++++++
>  src/ipa/ipu3/meson.build  |   7 +-
>  4 files changed, 270 insertions(+), 9 deletions(-)
>  create mode 100644 src/ipa/ipu3/ipu3_awb.cpp
>  create mode 100644 src/ipa/ipu3/ipu3_awb.h
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b63e58be..6fae5160 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -21,6 +21,8 @@
>  #include "libcamera/internal/buffer.h"
>  #include "libcamera/internal/log.h"
>  
> +#include "ipu3_awb.h"
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -60,6 +62,11 @@ private:
>  	uint32_t gain_;
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
> +
> +	/* Interface to the AWB algorithm */
> +	ipa::IPU3Awb *awbAlgo_;
> +	/* Local parameter storage */
> +	ipu3_uapi_params params_;
>  };
>  
>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls)
> @@ -83,11 +90,16 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>  
>  	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
>  	maxExposure_ = itExp->second.max().get<int32_t>();
> -	exposure_ = maxExposure_;
> +	exposure_ = minExposure_;
>  
>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>  	maxGain_ = itGain->second.max().get<int32_t>();
> -	gain_ = maxGain_;
> +	gain_ = minGain_;
> +
> +	params_ = {};
> +
> +	awbAlgo_ = new ipa::IPU3Awb();
> +	awbAlgo_->initialise(params_);
>  
>  	setControls(0);
>  }
> @@ -161,10 +173,8 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  
>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  {
> -	/* Prepare parameters buffer. */
> -	memset(params, 0, sizeof(*params));
> -
> -	/* \todo Fill in parameters buffer. */
> +	awbAlgo_->updateWbParameters(params_);
> +	*params = params_;
>  
>  	ipa::ipu3::IPU3Action op;
>  	op.op = ipa::ipu3::ActionParamFilled;
> @@ -177,8 +187,8 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  {
>  	ControlList ctrls(controls::controls);
>  
> -	/* \todo React to statistics and update internal state machine. */
> -	/* \todo Add meta-data information to ctrls. */
> +	awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);
> +	setControls(frame);
>  
>  	ipa::ipu3::IPU3Action op;
>  	op.op = ipa::ipu3::ActionMetadataReady;

How about splitting this patch in two, with a first patch that prepares
for the algorithms (all of the above except for the code related to
awbAlgo_), and a patch that adds the AWB algorithm ? The commit message
of the first patch should explain why you switch from the maximum
exposure and gain to the minimum.

> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> new file mode 100644
> index 00000000..3ff239c0
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -0,0 +1,199 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited

Is there actually any code in this file that comes from the RPi IPA ?

> + *
> + * ipu3_awb.cpp - AWB control algorithm
> + */
> +#include <numeric>
> +#include <unordered_map>
> +
> +#include "libcamera/internal/log.h"
> +
> +#include "ipu3_awb.h"

This should be included first, to ensure the header is self-contained
(see Documentation/coding-style.rst).

> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +LOG_DEFINE_CATEGORY(IPU3Awb)
> +
> +static const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {

imguCssBnrDefaults

Same below.

> +	{ 16, 16, 16, 16 }, /* wb_gains */
> +	{ 255, 255, 255, 255 }, /* wb_gains_thr */
> +	{ 0, 0, 8, 6, 0, 14 }, /* thr_coeffs */
> +	{ 0, 0, 0, 0 }, /* thr_ctrl_shd */
> +	{ -648, 0, -366, 0 }, /* opt_center */
> +	{ /* lut */
> +	  { 17, 23, 28, 32, 36, 39, 42, 45,
> +	    48, 51, 53, 55, 58, 60, 62, 64,
> +	    66, 68, 70, 72, 73, 75, 77, 78,
> +	    80, 82, 83, 85, 86, 88, 89, 90 } },
> +	{ 4, 0, 1, 8, 0, 8, 0, 8, 0 }, /* bp_ctrl */
> +	{ 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 }, /* dn_detect_ctrl */
> +	1296,
> +	{ 419904, 133956 },

Could we use designated initializers instead of comments ?

> +};
> +
> +/* settings for Auto White Balance */
> +static const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
> +	8191,
> +	8191,
> +	8191,
> +	8191 | /* rgbs_thr_gr/r/gb/b */
> +		IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,

Same here, designated initializers ?

> +	.grid = {
> +		.width = 160,
> +		.height = 45,
> +		.block_width_log2 = 3,
> +		.block_height_log2 = 4,
> +		.x_start = 0,
> +		.y_start = 0,
> +	},
> +};
> +
> +static const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
> +	7239, -750, -37, 0,
> +	-215, 8196, -200, 0,
> +	-70, -589, 6810, 0
> +};
> +
> +static const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
> +	7379, -526, -296, 0,
> +	-411, 7397, -415, 0,
> +	-224, -564, 7244, 0
> +};

Maybe we should order those by temperature, with 3800k going first ?

> +
> +IPU3Awb::IPU3Awb()
> +	: Algorithm()
> +{
> +}
> +
> +IPU3Awb::~IPU3Awb()
> +{
> +}
> +
> +void IPU3Awb::initialise()
> +{
> +}
> +
> +void IPU3Awb::initialise(ipu3_uapi_params &params)
> +{
> +	params.use.acc_awb = 1;
> +	/*\todo fill the grid calculated based on BDS configuration */
> +	params.acc_param.awb.config = imgu_css_awb_defaults;
> +
> +	params.use.acc_bnr = 1;
> +	params.acc_param.bnr = imgu_css_bnr_defaults;
> +
> +	params.use.acc_ccm = 1;
> +	params.acc_param.ccm = imgu_css_ccm_3800k;
> +
> +	params.use.acc_gamma = 1;
> +	params.acc_param.gamma.gc_ctrl.enable = 1;
> +
> +	uint32_t a = (32 * 245) / (245 - 9);

Should this be a float, to avoid rounding errors ?

> +
> +	for (uint32_t i = 0; i < 10; i++)
> +		params.acc_param.gamma.gc_lut.lut[i] = 0;
> +	for (uint32_t i = 10; i < 245; i++)
> +		params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);

Maybe a * (i - 9) ? Or std::round(a * (i - 9)) if you make a a float ?

> +	for (uint32_t i = 245; i < 255; i++)
> +		params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
> +
> +	wbGains_[0] = 8192 * 0.8;
> +	wbGains_[1] = 8192;
> +	wbGains_[2] = 8192;
> +	wbGains_[3] = 8192 * 0.8;
> +
> +	frame_count_ = 0;
> +}
> +
> +uint32_t IPU3Awb::estimateCCT(uint8_t red, uint8_t green, uint8_t blue)
> +{
> +	double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
> +	double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
> +	double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);

No need for parentheses.

> +
> +	double x = X / (X + Y + Z);
> +	double y = Y / (X + Y + Z);
> +
> +	double n = (x - 0.3320) / (0.1858 - y);
> +	return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33);

I don't think the explicit cast is needed.

> +}
> +
> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi,
> +			       const ipu3_uapi_stats_3a *stats)
> +{
> +	std::vector<uint32_t> redValues, greenRedValues, greenBlueValues, blueValues;
> +	Point topleft = roi.topLeft();
> +	uint32_t startY = (topleft.y / 16) * 160 * 8;
> +	uint32_t startX = (topleft.x / 8) * 8;

	uint32_t startX = utils::alignDown(topleft.x, 8);

> +	uint32_t endX = (startX + (roi.size().width / 8)) * 8;

This seems incorrect, startX is expressed in pixels, it shouldn't be
multiplied by 8.

	uint32_t endX = startX + utils::alignDown(roi.size().width, 8);

> +
> +	for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) + (roi.size().height / 16); j++) {

No need for the inner parentheses.

> +		for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
> +			greenRedValues.push_back(stats->awb_raw_buffer.meta_data[i]);
> +			redValues.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
> +			blueValues.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
> +			greenBlueValues.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
> +		}

Nothing in the inner loop uses j, this seems weird. The code at least
requires comments to explain what's going on. You can assume the reader
has no idea how AWB works.

> +	}
> +
> +	std::sort(redValues.begin(), redValues.end());
> +	std::sort(greenRedValues.begin(), greenRedValues.end());
> +	std::sort(blueValues.begin(), blueValues.end());
> +	std::sort(greenBlueValues.begin(), greenBlueValues.end());
> +
> +	double Grmed = greenRedValues[greenRedValues.size() / 2];
> +	double Rmed = redValues[redValues.size() / 2];
> +	double Bmed = blueValues[blueValues.size() / 2];
> +	double Gbmed = greenBlueValues[greenBlueValues.size() / 2];
> +
> +	double Rgain = Grmed / Rmed;
> +	double Bgain = Gbmed / Bmed;
> +	LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
> +			    << redValues.back() << ","
> +			    << greenRedValues.back() << ","
> +			    << blueValues.back() << ","
> +			    << greenBlueValues.back();

In the 5 blocks above that deal with bayer components, you order them as
RGGB, GRBG, RGBG, GRBG and RGBG. Could we pick one order ?

> +	tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;

No need for the inner parentheses.

tint_ is only used in this function, you can make it a local variable.

> +
> +	/* \todo Those are corrections when light is really low
> +	 * it should be taken into account by AGC somehow */

	/*
	 * \todo Those are corrections when light is really low, it should be
	 * taken into account by AGC somehow
	 */

> +	if ((Rgain >= 2) && (Bgain < 2)) {

There are lots of inner parentheses that are not needed. I'll stop
repeating the comment, you can apply it to other locations below.

> +		wbGains_[0] = 4096 * tint_;
> +		wbGains_[1] = 8192 * Rgain;
> +		wbGains_[2] = 4096 * Bgain;
> +		wbGains_[3] = 4096 * tint_;
> +	} else if ((Bgain >= 2) && (Rgain < 2)) {
> +		wbGains_[0] = 4096 * tint_;
> +		wbGains_[1] = 4096 * Rgain;
> +		wbGains_[2] = 8192 * Bgain;
> +		wbGains_[3] = 4096 * tint_;
> +	} else {
> +		wbGains_[0] = 8192 * tint_;
> +		wbGains_[1] = 8192 * Rgain;
> +		wbGains_[2] = 8192 * Bgain;
> +		wbGains_[3] = 8192 * tint_;
> +	}
> +
> +	frame_count_++;
> +
> +	cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
> +}
> +
> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
> +{
> +	params.acc_param.bnr.wb_gains.gr = wbGains_[0];
> +	params.acc_param.bnr.wb_gains.r = wbGains_[1];
> +	params.acc_param.bnr.wb_gains.b = wbGains_[2];
> +	params.acc_param.bnr.wb_gains.gb = wbGains_[3];
> +	if (cct_ < 5500)
> +		params.acc_param.ccm = imgu_css_ccm_3800k;
> +	else
> +		params.acc_param.ccm = imgu_css_ccm_6000k;
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> \ No newline at end of file
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> new file mode 100644
> index 00000000..ff6906f2
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * ipu3_awb.h - IPU3 AWB control algorithm
> + */
> +#ifndef __LIBCAMERA_IPU3_AWB_H__
> +#define __LIBCAMERA_IPU3_AWB_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libipa/algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class IPU3Awb : public Algorithm
> +{
> +public:
> +	IPU3Awb();
> +	~IPU3Awb();
> +
> +	void initialise() override;

This isn't used, you can drop it.

> +
> +	void initialise(ipu3_uapi_params &params);
> +	void calculateWBGains(Rectangle roi,
> +			      const ipu3_uapi_stats_3a *stats);
> +	void updateWbParameters(ipu3_uapi_params &params);
> +
> +private:
> +	uint32_t estimateCCT(uint8_t red, uint8_t green, uint8_t blue);
> +
> +	/* WB calculated gains */
> +	uint16_t wbGains_[4];
> +	double tint_;
> +	uint32_t cct_;
> +
> +	uint32_t frame_count_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera*/
> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index a241f617..07a864c8 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -2,8 +2,13 @@
>  
>  ipa_name = 'ipa_ipu3'
>  
> +ipu3_ipa_sources = files([
> +  'ipu3.cpp',
> +  'ipu3_awb.cpp',
> +])
> +
>  mod = shared_module(ipa_name,
> -                    ['ipu3.cpp', libcamera_generated_ipa_headers],
> +                    [ipu3_ipa_sources, libcamera_generated_ipa_headers],
>                      name_prefix : '',
>                      include_directories : [ipa_includes, libipa_includes],
>                      dependencies : libcamera_dep,