[libcamera-devel,2/2] ipa: ipu3: Use metadata and improve the doc

12911 diff mbox series

Using the metadata to exchange the status of the awb algorithm helps to
simplify the interface. The structures used by the AWB statistics are in
ipu3_awb.h and documented.
A bit of the doc has been improved in this same patch.

There is one metadata variable which will be used and set in the AGC
algorithm in a near future, which is the agcGamma. Use it as if it exists, the
Metadata class won't find it and awb will default to a 1.0 value.
Doing it now is convenient to have nice process() and updateParamaters() calls.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp     |   8 ++-
 src/ipa/ipu3/ipu3_awb.cpp | 116 ++++++++++++++++++++++++++++----------
 src/ipa/ipu3/ipu3_awb.h   |  34 ++++++-----
 3 files changed, 111 insertions(+), 47 deletions(-)

Hi JM,

On 12/07/2021 14:16, Jean-Michel Hautbois wrote:
> Using the metadata to exchange the status of the awb algorithm helps to
> simplify the interface. The structures used by the AWB statistics are in
> ipu3_awb.h and documented.
> A bit of the doc has been improved in this same patch.

I would split out the documentation additions as a separate patch.

It feels like there are a few other potentially unrelated changes in
here too ...


> There is one metadata variable which will be used and set in the AGC
> algorithm in a near future, which is the agcGamma. Use it as if it exists, the
> Metadata class won't find it and awb will default to a 1.0 value.

Eeep, ok - I wonder if we should add that part when it is there instead
- but maybe that highlights one of the design benefits of the Metadata
class so I'll see below...


> Doing it now is convenient to have nice process() and updateParamaters() calls.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     |   8 ++-
>  src/ipa/ipu3/ipu3_awb.cpp | 116 ++++++++++++++++++++++++++++----------
>  src/ipa/ipu3/ipu3_awb.h   |  34 ++++++-----
>  3 files changed, 111 insertions(+), 47 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 091856f5..1a3d98e9 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -80,6 +80,8 @@ private:
>  	std::unique_ptr<IPU3Agc> agcAlgo_;
>  	/* Interface to the Camera Helper */
>  	std::unique_ptr<CameraSensorHelper> camHelper_;
> +	/* Metadata storage */

"Metadata metadata_" might already express that. What can we add on top
to explain what it stores?

/* Key/value pairs for algorithms to share results and context */ ?



> +	Metadata metadata_;
>  
>  	/* Local parameter storage */
>  	struct ipu3_uapi_params params_;
> @@ -277,7 +279,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  {
>  	if (agcAlgo_->updateControls())
> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> +		awbAlgo_->updateWbParameters(params_, &metadata_);

Given that metadata_ is a big global shared data, I wonder if we should
pass it to the algorithms at init time, so they have a reference to it -
and can reference it internally.

Is that worthwhile? or is it anticipated that there might be a new
metadata instance for each sequential run of the algorithms?



>  	*params = params_;
>  
> @@ -297,8 +299,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  	agcAlgo_->process(stats, exposure_, gain);
>  	gain_ = camHelper_->gainCode(gain);
>  
> -	awbAlgo_->calculateWBGains(stats);
> +	/* Calculate the AWB gains */
> +	awbAlgo_->process(stats, &metadata_);
>  
> +	/* Update the exposure and gains on sensor side */
>  	if (agcAlgo_->updateControls())
>  		setControls(frame);
>  
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> index 9b409c8f..d441e835 100644
> --- a/src/ipa/ipu3/ipu3_awb.cpp
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -18,29 +18,54 @@ namespace ipa::ipu3 {
>  
>  LOG_DEFINE_CATEGORY(IPU3Awb)
>  
> -static constexpr uint32_t kMinZonesCounted = 16;
> -static constexpr uint32_t kMinGreenLevelInZone = 32;
> +/**

Should this be the documentation for this AWB class? It doesn't seem to
have an identifier here...


> + * The Grey World algorithm assumes that the scene, in average, is neutral grey.
> + * Reference: Lam, Edmund & Fung, George. (2008). Automatic White Balancing in
> + * Digital Photography. 10.1201/9781420054538.ch10.

Is there a public link to that? or is it a paper that would be likely
paywalled? (Still valid to reference it even if it's not public I think).

I was going to ask what the 10.1201... code is, but it looks like
googling that brought me to researchgate.net and gave that text as the
correct 'citation', so I guess this is a defined reference style.


> + *
> + * The IPU3 is generating statistics from the Bayer Demosaic Scaler output

s/is generating/generates/

Is it 'Demosaic' or 'Domain' ?

I thought BDS would be Bayer domain scaler ... but I could be wrong.


> + * into a grid defined in the ipu3_uapi_awb_config_s structure.
> + *
> + * For example, when the BDS output is 2592x1944 then the grid is calculated to be:
> + * 81*30 with a cell beeing of size 32*64.

s/beeing/


"""
For example, when the BDS outputs a frame of 2592x1944, the grid may be
configured to 81x30 cells each with a size of 32x64 pixels.
"""

> + * We then have an average of 2048 R, G and B pixels per cell.
> + *
> + * The AWB algorithm could use those variable grid sizes as an input, but it would
> + * make it a bit more complex. In order to have something consistent with what is
> + * done on RPi, fix a default grid size to kAwbStatsSizeX x kAwbStatsSizeY.

The IPU3 doesn't need to reference being consistent with other IPAs -
this should document what it is doing.


> + *
> + * Before calculating the gains, we will convert the statistics to go from the BDS
> + * grid configuration to this intern grid size in generateAwbStats.

"we will convert the statistics from the BDS grid to an internal grid
configuration in generateAwbStats."


> + * When the stats are converted, the saturation flag in the initial grid is used to

"As part of converting the statistics to an internal grid, the
saturation flag from the originating grid cell is used to"

(I've only tried to improve the wording there, I don't know if that's
accurate).



> + * decide if the zone is saturated or not, making the zone relevant or not.

What happens to saturated cells?

What is a zone, is that a grid cell?

> + *
> + * The Grey World algorithm will then estimate the red and blue gains to apply, and
> + * send those back through metadata.

/send those back through/store the results in the/

> + */
>  
>  /**
> - * \struct IspStatsRegion
> + * \struct StatsRegion

Renaming a structure should be an independent patch.


>   * \brief RGB statistics for a given region

Is a region a cell? or a zone? or just a defined area that comprises of
... some pixels?

It's not clear which terms define which concepts.


>   *
> - * The IspStatsRegion structure is intended to abstract the ISP specific
> - * statistics and use an agnostic algorithm to compute AWB.
> + * The StatsRegion structure is intended to abstract the ISP specific

Is this just a good intention? I suspect it either does it, or it doesn't.

"The StatsRegions structure abstracts the ISP specific..."


> + * statistics to compute AWB. The Grey World algorithm uses an average

an average of what?



> + * for a specific counted pixels. 

oh, perhaps that was "an average of the pixels in a given region."


>     When a specific zone in the scene is
> + * saturated, we want to exclude it from the calculation, and consider
> + * it as an outlier.

aha, ok - that answers an earlier question.


>   *
> - * \var IspStatsRegion::counted
> - * \brief Number of pixels used to calculate the sums
> + * \var StatsRegion::counted
> + * \brief Number of unsatured pixels used to calculate the sums

s/unsatured/unsaturated/

Oh - So it distinguishes between a pixel being saturated, not a zone
containing saturat


>   *
> - * \var IspStatsRegion::uncounted
> - * \brief Remaining number of pixels in the region
> + * \var StatsRegion::uncounted
> + * \brief Remaining number of pixels in the region (ie saturated)
>   *
> - * \var IspStatsRegion::rSum
> + * \var StatsRegion::rSum
>   * \brief Sum of the red values in the region
>   *
> - * \var IspStatsRegion::gSum
> + * \var StatsRegion::gSum
>   * \brief Sum of the green values in the region
>   *
> - * \var IspStatsRegion::bSum
> + * \var StatsRegion::bSum
>   * \brief Sum of the blue values in the region
>   */
>  
> @@ -48,26 +73,35 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>   * \struct AwbStatus
>   * \brief AWB parameters calculated
>   *
> - * The AwbStatus structure is intended to store the AWB
> - * parameters calculated by the algorithm
> + * The AwbStatus structure is intended to store the AWB parameters

I think we should declare what it does, not an intention.

> + * calculated by the algorithm, and shared through the metadata
> + * object, for other algorithms

s/algorithms/algorithms./


>   *
>   * \var AwbStatus::temperatureK
> - * \brief Color temperature calculated
> + * \brief Color temperature calculated, in Kelvin
>   *
>   * \var AwbStatus::redGain
> - * \brief Gain calculated for the red channel
> + * \brief Gain calculated for the red channel. This is a floating-point
> + * value used as a multiplier on the ISP side
>   *
>   * \var AwbStatus::greenGain
> - * \brief Gain calculated for the green channel
> + * \brief Gain calculated for the green channel. This is a floating-point
> + * value used as a multiplier on the ISP side
>   *
>   * \var AwbStatus::blueGain
> - * \brief Gain calculated for the blue channel
> + * \brief Gain calculated for the blue channel. This is a floating-point
> + * value used as a multiplier on the ISP side
>   */
>  
>  /**
>   * \struct Ipu3AwbCell
>   * \brief Memory layout for each cell in AWB metadata
>   *
> + * This is the internal layout on IPU3 for one cell of AWB statistics.
> + * There is ipu3_uapi_awb_config_s->grid.width * 2^block_width_log2 per

/There is/There are/

But this isn't very readable or clear :S

> + * ipu3_uapi_awb_config_s->grid.height * 2^block_height_log2 cells for
> + * one frame statistics.

I'm not sure I can correctly interpret it to rewrite it at the moment...

In particular 'per' is throwing me off.

I presume this is describing that the grid has a width and a height? and
that the number of cells is ... width * height, but width and height are
stored as the log2 ?


> + *
>   * The Ipu3AwbCell structure is used to get individual values
>   * such as red average or saturation ratio in a particular cell.
>   *
> @@ -84,7 +118,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>   * \brief Green average for blue lines
>   *
>   * \var Ipu3AwbCell::satRatio
> - * \brief Saturation ratio in the cell
> + * \brief Saturation ratio in the cell. It depends on the rgbs_thr_* values, and
> + * will be set if rgbs_thr_b has IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit set.

"Saturation ratio of the cell" sounds like the brief, and the expanded
sentences sound like something that would be a new line to make it
render as more context beyond the brief.

But I'd express this then as:

"""
The saturation ratio is determined based upon the threshold values set
in rgbs_thr_*, and will be set if the threshold value has the
corresponding enable bit set in the (Where is it set?)..

For example, to obtain the Blue saturation value, the
IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit must be set in rgbs_thr_b.
"""

(But it might be good to reference what rgbs_thr_b is and where it can
be set perhaps ...)




>   *
>   * \var Ipu3AwbCell::padding
>   * \brief array of unused bytes for padding
> @@ -159,6 +194,9 @@ const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
>  	7807, 7871, 7935, 7999, 8063, 8127, 8191
>  } };
>  
> +/* Minimum level of green (on a 8 bits base) in a given zone */

What is an 8 bits base?

Do you mean it must be a multiple of 8?
What happens if it isn't? Is it rounded up? down?


> +static constexpr uint32_t kMinGreenLevelInZone = 16;
> +
>  IPU3Awb::IPU3Awb()
>  	: Algorithm()
>  {
> @@ -166,6 +204,7 @@ IPU3Awb::IPU3Awb()
>  	asyncResults_.greenGain = 1.0;
>  	asyncResults_.redGain = 1.0;
>  	asyncResults_.temperatureK = 4500;
> +	minZonesCounted_ = 0;
>  }
>  
>  IPU3Awb::~IPU3Awb()
> @@ -202,7 +241,7 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
>  	params.acc_param.gamma.gc_lut = imguCssGammaLut;
>  	params.acc_param.gamma.gc_ctrl.enable = 1;
>  
> -	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> +	zones_.reserve(kAwbStatsSize);
>  }
>  
>  /**
> @@ -238,10 +277,10 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)
>  /* Generate an RGB vector with the average values for each region */
>  void IPU3Awb::generateZones(std::vector<RGB> &zones)
>  {
> -	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> +	for (unsigned int i = 0; i < kAwbStatsSize; i++) {
>  		RGB zone;
>  		double counted = awbStats_[i].counted;
> -		if (counted >= kMinZonesCounted) {
> +		if (counted >= minZonesCounted_) {
>  			zone.G = awbStats_[i].gSum / counted;
>  			if (zone.G >= kMinGreenLevelInZone) {
>  				zone.R = awbStats_[i].rSum / counted;
> @@ -258,6 +297,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>  	uint32_t regionWidth = round(awbGrid_.width / static_cast<double>(kAwbStatsSizeX));
>  	uint32_t regionHeight = round(awbGrid_.height / static_cast<double>(kAwbStatsSizeY));
>  
> +	minZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;

Where does 4/5 come from here?


>  	/*
>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>  	 * (awbGrid_.width x awbGrid_.height).
> @@ -269,7 +309,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>  			uint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;
>  
>  			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> -			cellPosition *= 8;
> +			cellPosition *= sizeof(Ipu3AwbCell);

Is this a bug fix? or because of the move to metadata? Looks like it
almost deserves it's own patch ? I can't quite tell.



>  			/* Cast the initial IPU3 structure to simplify the reading */
>  			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
> @@ -287,7 +327,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>  
>  void IPU3Awb::clearAwbStats()
>  {
> -	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> +	for (unsigned int i = 0; i < kAwbStatsSize; i++) {
>  		awbStats_[i].bSum = 0;
>  		awbStats_[i].rSum = 0;
>  		awbStats_[i].gSum = 0;
> @@ -344,24 +384,38 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>  	generateAwbStats(stats);
>  	generateZones(zones_);
>  	LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
> -	if (zones_.size() > 10) {
> +
> +	/* We need at least 5% of valid zones to estimate the gain correction */

5 sounds quite low? Is that determined from empirical results? or just a
rough estimate?

What effect does increasing this ratio have?

> +	if (zones_.size() > kAwbStatsSize / 20) {
>  		awbGreyWorld();
>  		LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.redGain
>  				    << " and for blue: " << asyncResults_.blueGain;
>  	}
>  }
>  
> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
> +void IPU3Awb::process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata)
> +{
> +	calculateWBGains(stats);
> +	/* We need to update the AWB status, to give back the gains */
> +	imageMetadata->set("awb.status", asyncResults_);

I still think these look like results not status ;)

Do we need to define a naming scheme for the metadata keys?


> +}
> +
> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, Metadata *imageMetadata)
>  {
>  	/*
>  	 * Green gains should not be touched and considered 1.
>  	 * Default is 16, so do not change it at all.
> -	 * 4096 is the value for a gain of 1.0
> +	 * 8192 is the value for a gain of 1.0
>  	 */
> -	params.acc_param.bnr.wb_gains.gr = 16;
> -	params.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;
> -	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
> -	params.acc_param.bnr.wb_gains.gb = 16;
> +	params.acc_param.bnr.wb_gains.gr = 8192;
> +	params.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;
> +	params.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;
> +	params.acc_param.bnr.wb_gains.gb = 8192;

Is this change part of using the metadata?

We're going from 4096 to 8192. That sounds like a specific change worthy
of it's own commit message to explain why this change occurs ?



> +
> +	/* When the AGC algorithm has run, it may have set a new gamma */
> +	double agcGamma = 1.0;
> +	if (imageMetadata->get("agc.gamma", agcGamma) != 0)
> +		LOG(IPU3Awb, Debug) << "Awb: no gamma found, defaulted to 1.0";
>  
>  	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
>  			    << " and gamma calculated: " << agcGamma;
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> index 122cf68c..cf12032d 100644
> --- a/src/ipa/ipu3/ipu3_awb.h
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -14,14 +14,18 @@
>  #include <libcamera/geometry.h>
>  
>  #include "libipa/algorithm.h"
> +#include "libipa/metadata.h"
>  
>  namespace libcamera {
>  
>  namespace ipa::ipu3 {
>  
> -/* Region size for the statistics generation algorithm */
> +/* Width of the AWB regions used for Grey World calculation */
>  static constexpr uint32_t kAwbStatsSizeX = 16;
> +/* Height of the AWB regions used for Grey World calculation */
>  static constexpr uint32_t kAwbStatsSizeY = 12;
> +/* Total size of the AWB regions used for Grey World calculation */
> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY;
>  
>  class IPU3Awb : public Algorithm

Should this be named IPU3AwbGreyworld?


Presumably we might potentially have another Awb implementation which
would run a different algorithm?

(I wonder then what the implications are for switching between the
algorithms in that case).

But perhaps we should leave that name as is for now until we have a more
modular construction for the independent algorithms.



>  {
> @@ -30,17 +34,8 @@ public:
>  	~IPU3Awb();
>  
>  	void initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
> -	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
> -
> -	struct Ipu3AwbCell {
> -		unsigned char greenRedAvg;
> -		unsigned char redAvg;
> -		unsigned char blueAvg;
> -		unsigned char greenBlueAvg;
> -		unsigned char satRatio;
> -		unsigned char padding[3];
> -	} __attribute__((packed));
> +	void process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata);
> +	void updateWbParameters(ipu3_uapi_params &params, Metadata *imageMetadata);
>  
>  	/* \todo Make these three structs available to all the ISPs ? */
>  	struct RGB {
> @@ -56,7 +51,7 @@ public:
>  		}
>  	};
>  
> -	struct IspStatsRegion {
> +	struct StatsRegion {
>  		unsigned int counted;
>  		unsigned int uncounted;
>  		unsigned long long rSum;
> @@ -71,18 +66,29 @@ public:
>  		double blueGain;
>  	};
>  
> +	struct Ipu3AwbCell {
> +		unsigned char greenRedAvg;
> +		unsigned char redAvg;
> +		unsigned char blueAvg;
> +		unsigned char greenBlueAvg;
> +		unsigned char satRatio;
> +		unsigned char padding[3];
> +	};
> +
>  private:
>  	void generateZones(std::vector<RGB> &zones);
>  	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
>  	void clearAwbStats();
>  	void awbGreyWorld();
>  	uint32_t estimateCCT(double red, double green, double blue);
> +	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>  
>  	struct ipu3_uapi_grid_config awbGrid_;
>  
>  	std::vector<RGB> zones_;
> -	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
> +	StatsRegion awbStats_[kAwbStatsSize];
>  	AwbStatus asyncResults_;
> +	uint32_t minZonesCounted_;
>  };
>  
>  } /* namespace ipa::ipu3 */
>

Hi Kieran,

On 13/07/2021 18:06, Kieran Bingham wrote:
> Hi JM,
> 
> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:
>> Using the metadata to exchange the status of the awb algorithm helps to
>> simplify the interface. The structures used by the AWB statistics are in
>> ipu3_awb.h and documented.
>> A bit of the doc has been improved in this same patch.
> 
> I would split out the documentation additions as a separate patch.
> 
> It feels like there are a few other potentially unrelated changes in
> here too ...

Yes, I will split it :-)

> 
>> There is one metadata variable which will be used and set in the AGC
>> algorithm in a near future, which is the agcGamma. Use it as if it exists, the
>> Metadata class won't find it and awb will default to a 1.0 value.
> 
> Eeep, ok - I wonder if we should add that part when it is there instead
> - but maybe that highlights one of the design benefits of the Metadata
> class so I'll see below...
> 
> 
>> Doing it now is convenient to have nice process() and updateParamaters() calls.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>  src/ipa/ipu3/ipu3.cpp     |   8 ++-
>>  src/ipa/ipu3/ipu3_awb.cpp | 116 ++++++++++++++++++++++++++++----------
>>  src/ipa/ipu3/ipu3_awb.h   |  34 ++++++-----
>>  3 files changed, 111 insertions(+), 47 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 091856f5..1a3d98e9 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -80,6 +80,8 @@ private:
>>  	std::unique_ptr<IPU3Agc> agcAlgo_;
>>  	/* Interface to the Camera Helper */
>>  	std::unique_ptr<CameraSensorHelper> camHelper_;
>> +	/* Metadata storage */
> 
> "Metadata metadata_" might already express that. What can we add on top
> to explain what it stores?
> 
> /* Key/value pairs for algorithms to share results and context */ ?
> 
> 
> 
>> +	Metadata metadata_;
>>  
>>  	/* Local parameter storage */
>>  	struct ipu3_uapi_params params_;
>> @@ -277,7 +279,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>  {
>>  	if (agcAlgo_->updateControls())
>> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
>> +		awbAlgo_->updateWbParameters(params_, &metadata_);
> 
> Given that metadata_ is a big global shared data, I wonder if we should
> pass it to the algorithms at init time, so they have a reference to it -
> and can reference it internally.
> 
> Is that worthwhile? or is it anticipated that there might be a new
> metadata instance for each sequential run of the algorithms?
> 

We might, in the future, want to pass a different reference, even for
now, it could make sense to remove the metadata reference from the
process() calls...
It makes its design easier to understand from a new comer, I think, as
you can see the algorithm takes some data as input... ?
But it is very subjective.

> 
>>  	*params = params_;
>>  
>> @@ -297,8 +299,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>  	agcAlgo_->process(stats, exposure_, gain);
>>  	gain_ = camHelper_->gainCode(gain);
>>  
>> -	awbAlgo_->calculateWBGains(stats);
>> +	/* Calculate the AWB gains */
>> +	awbAlgo_->process(stats, &metadata_);
>>  
>> +	/* Update the exposure and gains on sensor side */
>>  	if (agcAlgo_->updateControls())
>>  		setControls(frame);
>>  
>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>> index 9b409c8f..d441e835 100644
>> --- a/src/ipa/ipu3/ipu3_awb.cpp
>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>> @@ -18,29 +18,54 @@ namespace ipa::ipu3 {
>>  
>>  LOG_DEFINE_CATEGORY(IPU3Awb)
>>  
>> -static constexpr uint32_t kMinZonesCounted = 16;
>> -static constexpr uint32_t kMinGreenLevelInZone = 32;
>> +/**
> 
> Should this be the documentation for this AWB class? It doesn't seem to
> have an identifier here...
> 
> 
>> + * The Grey World algorithm assumes that the scene, in average, is neutral grey.
>> + * Reference: Lam, Edmund & Fung, George. (2008). Automatic White Balancing in
>> + * Digital Photography. 10.1201/9781420054538.ch10.
> 
> Is there a public link to that? or is it a paper that would be likely
> paywalled? (Still valid to reference it even if it's not public I think).
> 
> I was going to ask what the 10.1201... code is, but it looks like
> googling that brought me to researchgate.net and gave that text as the
> correct 'citation', so I guess this is a defined reference style.
> 

This is the DOI: Digital Object Identifier

>> + *
>> + * The IPU3 is generating statistics from the Bayer Demosaic Scaler output
> 
> s/is generating/generates/
> 
> Is it 'Demosaic' or 'Domain' ?
> 
> I thought BDS would be Bayer domain scaler ... but I could be wrong.
> 

Thanks for noticing the type, it is Bayer Down Scaler btw ;-)

>> + * into a grid defined in the ipu3_uapi_awb_config_s structure.
>> + *
>> + * For example, when the BDS output is 2592x1944 then the grid is calculated to be:
>> + * 81*30 with a cell beeing of size 32*64.
> 
> s/beeing/
> 
> 
> """
> For example, when the BDS outputs a frame of 2592x1944, the grid may be
> configured to 81x30 cells each with a size of 32x64 pixels.
> """
> 
>> + * We then have an average of 2048 R, G and B pixels per cell.
>> + *
>> + * The AWB algorithm could use those variable grid sizes as an input, but it would
>> + * make it a bit more complex. In order to have something consistent with what is
>> + * done on RPi, fix a default grid size to kAwbStatsSizeX x kAwbStatsSizeY.
> 
> The IPU3 doesn't need to reference being consistent with other IPAs -
> this should document what it is doing.
> 
> 
>> + *
>> + * Before calculating the gains, we will convert the statistics to go from the BDS
>> + * grid configuration to this intern grid size in generateAwbStats.
> 
> "we will convert the statistics from the BDS grid to an internal grid
> configuration in generateAwbStats."
> 
> 
>> + * When the stats are converted, the saturation flag in the initial grid is used to
> 
> "As part of converting the statistics to an internal grid, the
> saturation flag from the originating grid cell is used to"
> 
> (I've only tried to improve the wording there, I don't know if that's
> accurate).
> 
> 
> 
>> + * decide if the zone is saturated or not, making the zone relevant or not.
> 
> What happens to saturated cells?
> 
> What is a zone, is that a grid cell?
> 
>> + *
>> + * The Grey World algorithm will then estimate the red and blue gains to apply, and
>> + * send those back through metadata.
> 
> /send those back through/store the results in the/
> 
>> + */
>>  
>>  /**
>> - * \struct IspStatsRegion
>> + * \struct StatsRegion
> 
> Renaming a structure should be an independent patch.
> 
> 
>>   * \brief RGB statistics for a given region
> 
> Is a region a cell? or a zone? or just a defined area that comprises of
> ... some pixels?
> 
> It's not clear which terms define which concepts.
> 
> 
>>   *
>> - * The IspStatsRegion structure is intended to abstract the ISP specific
>> - * statistics and use an agnostic algorithm to compute AWB.
>> + * The StatsRegion structure is intended to abstract the ISP specific
> 
> Is this just a good intention? I suspect it either does it, or it doesn't.
> 
> "The StatsRegions structure abstracts the ISP specific..."
> 
> 
>> + * statistics to compute AWB. The Grey World algorithm uses an average
> 
> an average of what?
> 
> 
> 
>> + * for a specific counted pixels. 
> 
> oh, perhaps that was "an average of the pixels in a given region."
> 
> 
>>     When a specific zone in the scene is
>> + * saturated, we want to exclude it from the calculation, and consider
>> + * it as an outlier.
> 
> aha, ok - that answers an earlier question.
> 
> 
>>   *
>> - * \var IspStatsRegion::counted
>> - * \brief Number of pixels used to calculate the sums
>> + * \var StatsRegion::counted
>> + * \brief Number of unsatured pixels used to calculate the sums
> 
> s/unsatured/unsaturated/
> 
> Oh - So it distinguishes between a pixel being saturated, not a zone
> containing saturat
> 
> 
>>   *
>> - * \var IspStatsRegion::uncounted
>> - * \brief Remaining number of pixels in the region
>> + * \var StatsRegion::uncounted
>> + * \brief Remaining number of pixels in the region (ie saturated)
>>   *
>> - * \var IspStatsRegion::rSum
>> + * \var StatsRegion::rSum
>>   * \brief Sum of the red values in the region
>>   *
>> - * \var IspStatsRegion::gSum
>> + * \var StatsRegion::gSum
>>   * \brief Sum of the green values in the region
>>   *
>> - * \var IspStatsRegion::bSum
>> + * \var StatsRegion::bSum
>>   * \brief Sum of the blue values in the region
>>   */
>>  
>> @@ -48,26 +73,35 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>>   * \struct AwbStatus
>>   * \brief AWB parameters calculated
>>   *
>> - * The AwbStatus structure is intended to store the AWB
>> - * parameters calculated by the algorithm
>> + * The AwbStatus structure is intended to store the AWB parameters
> 
> I think we should declare what it does, not an intention.
> 
>> + * calculated by the algorithm, and shared through the metadata
>> + * object, for other algorithms
> 
> s/algorithms/algorithms./
> 
> 
>>   *
>>   * \var AwbStatus::temperatureK
>> - * \brief Color temperature calculated
>> + * \brief Color temperature calculated, in Kelvin
>>   *
>>   * \var AwbStatus::redGain
>> - * \brief Gain calculated for the red channel
>> + * \brief Gain calculated for the red channel. This is a floating-point
>> + * value used as a multiplier on the ISP side
>>   *
>>   * \var AwbStatus::greenGain
>> - * \brief Gain calculated for the green channel
>> + * \brief Gain calculated for the green channel. This is a floating-point
>> + * value used as a multiplier on the ISP side
>>   *
>>   * \var AwbStatus::blueGain
>> - * \brief Gain calculated for the blue channel
>> + * \brief Gain calculated for the blue channel. This is a floating-point
>> + * value used as a multiplier on the ISP side
>>   */
>>  
>>  /**
>>   * \struct Ipu3AwbCell
>>   * \brief Memory layout for each cell in AWB metadata
>>   *
>> + * This is the internal layout on IPU3 for one cell of AWB statistics.
>> + * There is ipu3_uapi_awb_config_s->grid.width * 2^block_width_log2 per
> 
> /There is/There are/
> 
> But this isn't very readable or clear :S
> 
>> + * ipu3_uapi_awb_config_s->grid.height * 2^block_height_log2 cells for
>> + * one frame statistics.
> 
> I'm not sure I can correctly interpret it to rewrite it at the moment...
> 
> In particular 'per' is throwing me off.
> 
> I presume this is describing that the grid has a width and a height? and
> that the number of cells is ... width * height, but width and height are
> stored as the log2 ?
> 

Yes. How should it be reworded :-) ?

>> + *
>>   * The Ipu3AwbCell structure is used to get individual values
>>   * such as red average or saturation ratio in a particular cell.
>>   *
>> @@ -84,7 +118,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>>   * \brief Green average for blue lines
>>   *
>>   * \var Ipu3AwbCell::satRatio
>> - * \brief Saturation ratio in the cell
>> + * \brief Saturation ratio in the cell. It depends on the rgbs_thr_* values, and
>> + * will be set if rgbs_thr_b has IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit set.
> 
> "Saturation ratio of the cell" sounds like the brief, and the expanded
> sentences sound like something that would be a new line to make it
> render as more context beyond the brief.
> 
> But I'd express this then as:
> 
> """
> The saturation ratio is determined based upon the threshold values set
> in rgbs_thr_*, and will be set if the threshold value has the
> corresponding enable bit set in the (Where is it set?)..
> 
> For example, to obtain the Blue saturation value, the
> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit must be set in rgbs_thr_b.
> """
> 
> (But it might be good to reference what rgbs_thr_b is and where it can
> be set perhaps ...)
> 
> 
> 
> 
>>   *
>>   * \var Ipu3AwbCell::padding
>>   * \brief array of unused bytes for padding
>> @@ -159,6 +194,9 @@ const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
>>  	7807, 7871, 7935, 7999, 8063, 8127, 8191
>>  } };
>>  
>> +/* Minimum level of green (on a 8 bits base) in a given zone */
> 
> What is an 8 bits base?
> 
> Do you mean it must be a multiple of 8?
> What happens if it isn't? Is it rounded up? down?

It means that the value has a maximum of 256, so it is not really useful
to say anything about it.

+/* Minimum level of green in a given zone */

> 
>> +static constexpr uint32_t kMinGreenLevelInZone = 16;
>> +
>>  IPU3Awb::IPU3Awb()
>>  	: Algorithm()
>>  {
>> @@ -166,6 +204,7 @@ IPU3Awb::IPU3Awb()
>>  	asyncResults_.greenGain = 1.0;
>>  	asyncResults_.redGain = 1.0;
>>  	asyncResults_.temperatureK = 4500;
>> +	minZonesCounted_ = 0;
>>  }
>>  
>>  IPU3Awb::~IPU3Awb()
>> @@ -202,7 +241,7 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
>>  	params.acc_param.gamma.gc_lut = imguCssGammaLut;
>>  	params.acc_param.gamma.gc_ctrl.enable = 1;
>>  
>> -	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
>> +	zones_.reserve(kAwbStatsSize);
>>  }
>>  
>>  /**
>> @@ -238,10 +277,10 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)
>>  /* Generate an RGB vector with the average values for each region */
>>  void IPU3Awb::generateZones(std::vector<RGB> &zones)
>>  {
>> -	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>> +	for (unsigned int i = 0; i < kAwbStatsSize; i++) {
>>  		RGB zone;
>>  		double counted = awbStats_[i].counted;
>> -		if (counted >= kMinZonesCounted) {
>> +		if (counted >= minZonesCounted_) {
>>  			zone.G = awbStats_[i].gSum / counted;
>>  			if (zone.G >= kMinGreenLevelInZone) {
>>  				zone.R = awbStats_[i].rSum / counted;
>> @@ -258,6 +297,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>  	uint32_t regionWidth = round(awbGrid_.width / static_cast<double>(kAwbStatsSizeX));
>>  	uint32_t regionHeight = round(awbGrid_.height / static_cast<double>(kAwbStatsSizeY));
>>  
>> +	minZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;
> 
> Where does 4/5 come from here?

This is a way to say 80% :-p.
It is the minimum proportion of pixels counted within AWB region for it
to be relevant. But this value is empirical.

> 
>>  	/*
>>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>>  	 * (awbGrid_.width x awbGrid_.height).
>> @@ -269,7 +309,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>  			uint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;
>>  
>>  			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
>> -			cellPosition *= 8;
>> +			cellPosition *= sizeof(Ipu3AwbCell);
> 
> Is this a bug fix? or because of the move to metadata? Looks like it
> almost deserves it's own patch ? I can't quite tell.
> 
> 
> 
>>  			/* Cast the initial IPU3 structure to simplify the reading */
>>  			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
>> @@ -287,7 +327,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>  
>>  void IPU3Awb::clearAwbStats()
>>  {
>> -	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>> +	for (unsigned int i = 0; i < kAwbStatsSize; i++) {
>>  		awbStats_[i].bSum = 0;
>>  		awbStats_[i].rSum = 0;
>>  		awbStats_[i].gSum = 0;
>> @@ -344,24 +384,38 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>>  	generateAwbStats(stats);
>>  	generateZones(zones_);
>>  	LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
>> -	if (zones_.size() > 10) {
>> +
>> +	/* We need at least 5% of valid zones to estimate the gain correction */
> 
> 5 sounds quite low? Is that determined from empirical results? or just a
> rough estimate?
> 
> What effect does increasing this ratio have?

If you increase it too much you will have to wait longer for the AWB to
start correcting the colors. And if the AGC is "slow" then you may
experience weird colors before it is locked.

> 
>> +	if (zones_.size() > kAwbStatsSize / 20) {
>>  		awbGreyWorld();
>>  		LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.redGain
>>  				    << " and for blue: " << asyncResults_.blueGain;
>>  	}
>>  }
>>  
>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>> +void IPU3Awb::process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata)
>> +{
>> +	calculateWBGains(stats);
>> +	/* We need to update the AWB status, to give back the gains */
>> +	imageMetadata->set("awb.status", asyncResults_);
> 
> I still think these look like results not status ;)
> 
> Do we need to define a naming scheme for the metadata keys?

I will add the name used in the s/AwbStatus/AwbResults structure
documentation. I don't really care using awb.status or awb.results.

> 
> 
>> +}
>> +
>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, Metadata *imageMetadata)
>>  {
>>  	/*
>>  	 * Green gains should not be touched and considered 1.
>>  	 * Default is 16, so do not change it at all.
>> -	 * 4096 is the value for a gain of 1.0
>> +	 * 8192 is the value for a gain of 1.0
>>  	 */
>> -	params.acc_param.bnr.wb_gains.gr = 16;
>> -	params.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;
>> -	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
>> -	params.acc_param.bnr.wb_gains.gb = 16;
>> +	params.acc_param.bnr.wb_gains.gr = 8192;
>> +	params.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;
>> +	params.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;
>> +	params.acc_param.bnr.wb_gains.gb = 8192;
> 
> Is this change part of using the metadata?
> 
> We're going from 4096 to 8192. That sounds like a specific change worthy
> of it's own commit message to explain why this change occurs ?

Yes.

> 
> 
>> +
>> +	/* When the AGC algorithm has run, it may have set a new gamma */
>> +	double agcGamma = 1.0;
>> +	if (imageMetadata->get("agc.gamma", agcGamma) != 0)
>> +		LOG(IPU3Awb, Debug) << "Awb: no gamma found, defaulted to 1.0";
>>  
>>  	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
>>  			    << " and gamma calculated: " << agcGamma;
>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>> index 122cf68c..cf12032d 100644
>> --- a/src/ipa/ipu3/ipu3_awb.h
>> +++ b/src/ipa/ipu3/ipu3_awb.h
>> @@ -14,14 +14,18 @@
>>  #include <libcamera/geometry.h>
>>  
>>  #include "libipa/algorithm.h"
>> +#include "libipa/metadata.h"
>>  
>>  namespace libcamera {
>>  
>>  namespace ipa::ipu3 {
>>  
>> -/* Region size for the statistics generation algorithm */
>> +/* Width of the AWB regions used for Grey World calculation */
>>  static constexpr uint32_t kAwbStatsSizeX = 16;
>> +/* Height of the AWB regions used for Grey World calculation */
>>  static constexpr uint32_t kAwbStatsSizeY = 12;
>> +/* Total size of the AWB regions used for Grey World calculation */
>> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY;
>>  
>>  class IPU3Awb : public Algorithm
> 
> Should this be named IPU3AwbGreyworld?
> 
> 
> Presumably we might potentially have another Awb implementation which
> would run a different algorithm?
> 
> (I wonder then what the implications are for switching between the
> algorithms in that case).
> 
> But perhaps we should leave that name as is for now until we have a more
> modular construction for the independent algorithms.
> 

I think we should not name it differently for now, as we don't have
another algorithm yet :-).

> 
>>  {
>> @@ -30,17 +34,8 @@ public:
>>  	~IPU3Awb();
>>  
>>  	void initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>> -	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
>> -
>> -	struct Ipu3AwbCell {
>> -		unsigned char greenRedAvg;
>> -		unsigned char redAvg;
>> -		unsigned char blueAvg;
>> -		unsigned char greenBlueAvg;
>> -		unsigned char satRatio;
>> -		unsigned char padding[3];
>> -	} __attribute__((packed));
>> +	void process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata);
>> +	void updateWbParameters(ipu3_uapi_params &params, Metadata *imageMetadata);
>>  
>>  	/* \todo Make these three structs available to all the ISPs ? */
>>  	struct RGB {
>> @@ -56,7 +51,7 @@ public:
>>  		}
>>  	};
>>  
>> -	struct IspStatsRegion {
>> +	struct StatsRegion {
>>  		unsigned int counted;
>>  		unsigned int uncounted;
>>  		unsigned long long rSum;
>> @@ -71,18 +66,29 @@ public:
>>  		double blueGain;
>>  	};
>>  
>> +	struct Ipu3AwbCell {
>> +		unsigned char greenRedAvg;
>> +		unsigned char redAvg;
>> +		unsigned char blueAvg;
>> +		unsigned char greenBlueAvg;
>> +		unsigned char satRatio;
>> +		unsigned char padding[3];
>> +	};
>> +
>>  private:
>>  	void generateZones(std::vector<RGB> &zones);
>>  	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
>>  	void clearAwbStats();
>>  	void awbGreyWorld();
>>  	uint32_t estimateCCT(double red, double green, double blue);
>> +	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>>  
>>  	struct ipu3_uapi_grid_config awbGrid_;
>>  
>>  	std::vector<RGB> zones_;
>> -	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>> +	StatsRegion awbStats_[kAwbStatsSize];
>>  	AwbStatus asyncResults_;
>> +	uint32_t minZonesCounted_;
>>  };
>>  
>>  } /* namespace ipa::ipu3 */
>>

Hi JM,

On 14/07/2021 09:00, Jean-Michel Hautbois wrote:
> Hi Kieran,
> 
> On 13/07/2021 18:06, Kieran Bingham wrote:
>> Hi JM,
>>
>> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:
>>> Using the metadata to exchange the status of the awb algorithm helps to
>>> simplify the interface. The structures used by the AWB statistics are in
>>> ipu3_awb.h and documented.
>>> A bit of the doc has been improved in this same patch.
>>
>> I would split out the documentation additions as a separate patch.
>>
>> It feels like there are a few other potentially unrelated changes in
>> here too ...
> 
> Yes, I will split it :-)
> 
>>
>>> There is one metadata variable which will be used and set in the AGC
>>> algorithm in a near future, which is the agcGamma. Use it as if it exists, the
>>> Metadata class won't find it and awb will default to a 1.0 value.
>>
>> Eeep, ok - I wonder if we should add that part when it is there instead
>> - but maybe that highlights one of the design benefits of the Metadata
>> class so I'll see below...
>>
>>
>>> Doing it now is convenient to have nice process() and updateParamaters() calls.
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>> ---
>>>  src/ipa/ipu3/ipu3.cpp     |   8 ++-
>>>  src/ipa/ipu3/ipu3_awb.cpp | 116 ++++++++++++++++++++++++++++----------
>>>  src/ipa/ipu3/ipu3_awb.h   |  34 ++++++-----
>>>  3 files changed, 111 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>> index 091856f5..1a3d98e9 100644
>>> --- a/src/ipa/ipu3/ipu3.cpp
>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>> @@ -80,6 +80,8 @@ private:
>>>  	std::unique_ptr<IPU3Agc> agcAlgo_;
>>>  	/* Interface to the Camera Helper */
>>>  	std::unique_ptr<CameraSensorHelper> camHelper_;
>>> +	/* Metadata storage */
>>
>> "Metadata metadata_" might already express that. What can we add on top
>> to explain what it stores?
>>
>> /* Key/value pairs for algorithms to share results and context */ ?
>>
>>
>>
>>> +	Metadata metadata_;
>>>  
>>>  	/* Local parameter storage */
>>>  	struct ipu3_uapi_params params_;
>>> @@ -277,7 +279,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>>>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>>  {
>>>  	if (agcAlgo_->updateControls())
>>> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
>>> +		awbAlgo_->updateWbParameters(params_, &metadata_);
>>
>> Given that metadata_ is a big global shared data, I wonder if we should
>> pass it to the algorithms at init time, so they have a reference to it -
>> and can reference it internally.
>>
>> Is that worthwhile? or is it anticipated that there might be a new
>> metadata instance for each sequential run of the algorithms?
>>
> 
> We might, in the future, want to pass a different reference, even for
> now, it could make sense to remove the metadata reference from the
> process() calls...
> It makes its design easier to understand from a new comer, I think, as
> you can see the algorithm takes some data as input... ?
> But it is very subjective.
> 
>>
>>>  	*params = params_;
>>>  
>>> @@ -297,8 +299,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>>  	agcAlgo_->process(stats, exposure_, gain);
>>>  	gain_ = camHelper_->gainCode(gain);
>>>  
>>> -	awbAlgo_->calculateWBGains(stats);
>>> +	/* Calculate the AWB gains */
>>> +	awbAlgo_->process(stats, &metadata_);
>>>  
>>> +	/* Update the exposure and gains on sensor side */
>>>  	if (agcAlgo_->updateControls())
>>>  		setControls(frame);
>>>  
>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>>> index 9b409c8f..d441e835 100644
>>> --- a/src/ipa/ipu3/ipu3_awb.cpp
>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>>> @@ -18,29 +18,54 @@ namespace ipa::ipu3 {
>>>  
>>>  LOG_DEFINE_CATEGORY(IPU3Awb)
>>>  
>>> -static constexpr uint32_t kMinZonesCounted = 16;
>>> -static constexpr uint32_t kMinGreenLevelInZone = 32;
>>> +/**
>>
>> Should this be the documentation for this AWB class? It doesn't seem to
>> have an identifier here...
>>
>>
>>> + * The Grey World algorithm assumes that the scene, in average, is neutral grey.
>>> + * Reference: Lam, Edmund & Fung, George. (2008). Automatic White Balancing in
>>> + * Digital Photography. 10.1201/9781420054538.ch10.
>>
>> Is there a public link to that? or is it a paper that would be likely
>> paywalled? (Still valid to reference it even if it's not public I think).
>>
>> I was going to ask what the 10.1201... code is, but it looks like
>> googling that brought me to researchgate.net and gave that text as the
>> correct 'citation', so I guess this is a defined reference style.
>>
> 
> This is the DOI: Digital Object Identifier
> 
>>> + *
>>> + * The IPU3 is generating statistics from the Bayer Demosaic Scaler output
>>
>> s/is generating/generates/
>>
>> Is it 'Demosaic' or 'Domain' ?
>>
>> I thought BDS would be Bayer domain scaler ... but I could be wrong.
>>
> 
> Thanks for noticing the type, it is Bayer Down Scaler btw ;-)
> 
>>> + * into a grid defined in the ipu3_uapi_awb_config_s structure.
>>> + *
>>> + * For example, when the BDS output is 2592x1944 then the grid is calculated to be:
>>> + * 81*30 with a cell beeing of size 32*64.
>>
>> s/beeing/
>>
>>
>> """
>> For example, when the BDS outputs a frame of 2592x1944, the grid may be
>> configured to 81x30 cells each with a size of 32x64 pixels.
>> """
>>
>>> + * We then have an average of 2048 R, G and B pixels per cell.
>>> + *
>>> + * The AWB algorithm could use those variable grid sizes as an input, but it would
>>> + * make it a bit more complex. In order to have something consistent with what is
>>> + * done on RPi, fix a default grid size to kAwbStatsSizeX x kAwbStatsSizeY.
>>
>> The IPU3 doesn't need to reference being consistent with other IPAs -
>> this should document what it is doing.
>>
>>
>>> + *
>>> + * Before calculating the gains, we will convert the statistics to go from the BDS
>>> + * grid configuration to this intern grid size in generateAwbStats.
>>
>> "we will convert the statistics from the BDS grid to an internal grid
>> configuration in generateAwbStats."
>>
>>
>>> + * When the stats are converted, the saturation flag in the initial grid is used to
>>
>> "As part of converting the statistics to an internal grid, the
>> saturation flag from the originating grid cell is used to"
>>
>> (I've only tried to improve the wording there, I don't know if that's
>> accurate).
>>
>>
>>
>>> + * decide if the zone is saturated or not, making the zone relevant or not.
>>
>> What happens to saturated cells?
>>
>> What is a zone, is that a grid cell?
>>
>>> + *
>>> + * The Grey World algorithm will then estimate the red and blue gains to apply, and
>>> + * send those back through metadata.
>>
>> /send those back through/store the results in the/
>>
>>> + */
>>>  
>>>  /**
>>> - * \struct IspStatsRegion
>>> + * \struct StatsRegion
>>
>> Renaming a structure should be an independent patch.
>>
>>
>>>   * \brief RGB statistics for a given region
>>
>> Is a region a cell? or a zone? or just a defined area that comprises of
>> ... some pixels?
>>
>> It's not clear which terms define which concepts.
>>
>>
>>>   *
>>> - * The IspStatsRegion structure is intended to abstract the ISP specific
>>> - * statistics and use an agnostic algorithm to compute AWB.
>>> + * The StatsRegion structure is intended to abstract the ISP specific
>>
>> Is this just a good intention? I suspect it either does it, or it doesn't.
>>
>> "The StatsRegions structure abstracts the ISP specific..."
>>
>>
>>> + * statistics to compute AWB. The Grey World algorithm uses an average
>>
>> an average of what?
>>
>>
>>
>>> + * for a specific counted pixels. 
>>
>> oh, perhaps that was "an average of the pixels in a given region."
>>
>>
>>>     When a specific zone in the scene is
>>> + * saturated, we want to exclude it from the calculation, and consider
>>> + * it as an outlier.
>>
>> aha, ok - that answers an earlier question.
>>
>>
>>>   *
>>> - * \var IspStatsRegion::counted
>>> - * \brief Number of pixels used to calculate the sums
>>> + * \var StatsRegion::counted
>>> + * \brief Number of unsatured pixels used to calculate the sums
>>
>> s/unsatured/unsaturated/
>>
>> Oh - So it distinguishes between a pixel being saturated, not a zone
>> containing saturat
>>
>>
>>>   *
>>> - * \var IspStatsRegion::uncounted
>>> - * \brief Remaining number of pixels in the region
>>> + * \var StatsRegion::uncounted
>>> + * \brief Remaining number of pixels in the region (ie saturated)
>>>   *
>>> - * \var IspStatsRegion::rSum
>>> + * \var StatsRegion::rSum
>>>   * \brief Sum of the red values in the region
>>>   *
>>> - * \var IspStatsRegion::gSum
>>> + * \var StatsRegion::gSum
>>>   * \brief Sum of the green values in the region
>>>   *
>>> - * \var IspStatsRegion::bSum
>>> + * \var StatsRegion::bSum
>>>   * \brief Sum of the blue values in the region
>>>   */
>>>  
>>> @@ -48,26 +73,35 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>>>   * \struct AwbStatus
>>>   * \brief AWB parameters calculated
>>>   *
>>> - * The AwbStatus structure is intended to store the AWB
>>> - * parameters calculated by the algorithm
>>> + * The AwbStatus structure is intended to store the AWB parameters
>>
>> I think we should declare what it does, not an intention.
>>
>>> + * calculated by the algorithm, and shared through the metadata
>>> + * object, for other algorithms
>>
>> s/algorithms/algorithms./
>>
>>
>>>   *
>>>   * \var AwbStatus::temperatureK
>>> - * \brief Color temperature calculated
>>> + * \brief Color temperature calculated, in Kelvin
>>>   *
>>>   * \var AwbStatus::redGain
>>> - * \brief Gain calculated for the red channel
>>> + * \brief Gain calculated for the red channel. This is a floating-point
>>> + * value used as a multiplier on the ISP side
>>>   *
>>>   * \var AwbStatus::greenGain
>>> - * \brief Gain calculated for the green channel
>>> + * \brief Gain calculated for the green channel. This is a floating-point
>>> + * value used as a multiplier on the ISP side
>>>   *
>>>   * \var AwbStatus::blueGain
>>> - * \brief Gain calculated for the blue channel
>>> + * \brief Gain calculated for the blue channel. This is a floating-point
>>> + * value used as a multiplier on the ISP side
>>>   */
>>>  
>>>  /**
>>>   * \struct Ipu3AwbCell
>>>   * \brief Memory layout for each cell in AWB metadata
>>>   *
>>> + * This is the internal layout on IPU3 for one cell of AWB statistics.
>>> + * There is ipu3_uapi_awb_config_s->grid.width * 2^block_width_log2 per
>>
>> /There is/There are/
>>
>> But this isn't very readable or clear :S
>>
>>> + * ipu3_uapi_awb_config_s->grid.height * 2^block_height_log2 cells for
>>> + * one frame statistics.
>>
>> I'm not sure I can correctly interpret it to rewrite it at the moment...
>>
>> In particular 'per' is throwing me off.
>>
>> I presume this is describing that the grid has a width and a height? and
>> that the number of cells is ... width * height, but width and height are
>> stored as the log2 ?
>>
> 
> Yes. How should it be reworded :-) ?

That's not easy for me to do, as I haven't understood the existing
description, and I don't know what a cell is to document it myself.

I think the documentation summary for the struct needs to define or
describe some or all of the following:

  - [1] What the structure is
  - [2] What the structure is used for
  - [3] What the structure needs
  - [4] How the structure is used by other parts.

I do not know if any of the following is true or accurate, so please do
not directly copy and paste this. It's just an effort to show how this
structure could be documented.


"""
An Ipu3AwbCell represents one sector of the statistics data of the image
when divided into a grid by the ISP.

The cell reports the statistics from a rectangular region of the image
giving an average for each colour component within that region, along
with a saturation ratio, which details how many of the pixels were
saturated beyond the saturation threshold (if enabled).

The cell size is defined as part of the grid configuration, where the
the grid size is defined through the ipu3_uapi_awb_config_s structure.

On the IPU3, the cell width and height must be a multiple of 2, and
greater than 16.
"""


It sort of feels like that's documentation and a structure that should
have been provided as part of the IPU3 kernel driver though ;-) - but if
we declare the structure here, then we should document what we know
about it.


>>> + *
>>>   * The Ipu3AwbCell structure is used to get individual values
>>>   * such as red average or saturation ratio in a particular cell.
>>>   *
>>> @@ -84,7 +118,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>>>   * \brief Green average for blue lines
>>>   *
>>>   * \var Ipu3AwbCell::satRatio
>>> - * \brief Saturation ratio in the cell
>>> + * \brief Saturation ratio in the cell. It depends on the rgbs_thr_* values, and
>>> + * will be set if rgbs_thr_b has IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit set.
>>
>> "Saturation ratio of the cell" sounds like the brief, and the expanded
>> sentences sound like something that would be a new line to make it
>> render as more context beyond the brief.
>>
>> But I'd express this then as:
>>
>> """
>> The saturation ratio is determined based upon the threshold values set
>> in rgbs_thr_*, and will be set if the threshold value has the
>> corresponding enable bit set in the (Where is it set?)..
>>
>> For example, to obtain the Blue saturation value, the
>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit must be set in rgbs_thr_b.
>> """
>>
>> (But it might be good to reference what rgbs_thr_b is and where it can
>> be set perhaps ...)
>>
>>
>>
>>
>>>   *
>>>   * \var Ipu3AwbCell::padding
>>>   * \brief array of unused bytes for padding
>>> @@ -159,6 +194,9 @@ const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
>>>  	7807, 7871, 7935, 7999, 8063, 8127, 8191
>>>  } };
>>>  
>>> +/* Minimum level of green (on a 8 bits base) in a given zone */
>>
>> What is an 8 bits base?
>>
>> Do you mean it must be a multiple of 8?
>> What happens if it isn't? Is it rounded up? down?
> 
> It means that the value has a maximum of 256, so it is not really useful
> to say anything about it.
> 
> +/* Minimum level of green in a given zone */
> 
>>
>>> +static constexpr uint32_t kMinGreenLevelInZone = 16;
>>> +
>>>  IPU3Awb::IPU3Awb()
>>>  	: Algorithm()
>>>  {
>>> @@ -166,6 +204,7 @@ IPU3Awb::IPU3Awb()
>>>  	asyncResults_.greenGain = 1.0;
>>>  	asyncResults_.redGain = 1.0;
>>>  	asyncResults_.temperatureK = 4500;
>>> +	minZonesCounted_ = 0;
>>>  }
>>>  
>>>  IPU3Awb::~IPU3Awb()
>>> @@ -202,7 +241,7 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
>>>  	params.acc_param.gamma.gc_lut = imguCssGammaLut;
>>>  	params.acc_param.gamma.gc_ctrl.enable = 1;
>>>  
>>> -	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
>>> +	zones_.reserve(kAwbStatsSize);
>>>  }
>>>  
>>>  /**
>>> @@ -238,10 +277,10 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)
>>>  /* Generate an RGB vector with the average values for each region */
>>>  void IPU3Awb::generateZones(std::vector<RGB> &zones)
>>>  {
>>> -	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>>> +	for (unsigned int i = 0; i < kAwbStatsSize; i++) {
>>>  		RGB zone;
>>>  		double counted = awbStats_[i].counted;
>>> -		if (counted >= kMinZonesCounted) {
>>> +		if (counted >= minZonesCounted_) {
>>>  			zone.G = awbStats_[i].gSum / counted;
>>>  			if (zone.G >= kMinGreenLevelInZone) {
>>>  				zone.R = awbStats_[i].rSum / counted;
>>> @@ -258,6 +297,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>>  	uint32_t regionWidth = round(awbGrid_.width / static_cast<double>(kAwbStatsSizeX));
>>>  	uint32_t regionHeight = round(awbGrid_.height / static_cast<double>(kAwbStatsSizeY));
>>>  
>>> +	minZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;
>>
>> Where does 4/5 come from here?
> 
> This is a way to say 80% :-p.
> It is the minimum proportion of pixels counted within AWB region for it
> to be relevant. But this value is empirical.


It's fine to be an empirical value, but it should be documented as such,
and perhaps even guidance on how to tweak it if desired (i.e., why was
this particular value selected, what happens if you increase it,
decrease it).


> 
>>
>>>  	/*
>>>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>>>  	 * (awbGrid_.width x awbGrid_.height).
>>> @@ -269,7 +309,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>>  			uint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;
>>>  
>>>  			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
>>> -			cellPosition *= 8;
>>> +			cellPosition *= sizeof(Ipu3AwbCell);
>>
>> Is this a bug fix? or because of the move to metadata? Looks like it
>> almost deserves it's own patch ? I can't quite tell.
>>
>>
>>
>>>  			/* Cast the initial IPU3 structure to simplify the reading */
>>>  			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
>>> @@ -287,7 +327,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>>  
>>>  void IPU3Awb::clearAwbStats()
>>>  {
>>> -	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>>> +	for (unsigned int i = 0; i < kAwbStatsSize; i++) {
>>>  		awbStats_[i].bSum = 0;
>>>  		awbStats_[i].rSum = 0;
>>>  		awbStats_[i].gSum = 0;
>>> @@ -344,24 +384,38 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>>>  	generateAwbStats(stats);
>>>  	generateZones(zones_);
>>>  	LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
>>> -	if (zones_.size() > 10) {
>>> +
>>> +	/* We need at least 5% of valid zones to estimate the gain correction */
>>
>> 5 sounds quite low? Is that determined from empirical results? or just a
>> rough estimate?
>>
>> What effect does increasing this ratio have?
> 
> If you increase it too much you will have to wait longer for the AWB to
> start correcting the colors. And if the AGC is "slow" then you may
> experience weird colors before it is locked.

Ok - more descriptions that deserve a local comment then, like the previous.



>>> +	if (zones_.size() > kAwbStatsSize / 20) {
>>>  		awbGreyWorld();
>>>  		LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.redGain
>>>  				    << " and for blue: " << asyncResults_.blueGain;
>>>  	}
>>>  }
>>>  
>>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>>> +void IPU3Awb::process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata)
>>> +{
>>> +	calculateWBGains(stats);
>>> +	/* We need to update the AWB status, to give back the gains */
>>> +	imageMetadata->set("awb.status", asyncResults_);
>>
>> I still think these look like results not status ;)
>>
>> Do we need to define a naming scheme for the metadata keys?
> 
> I will add the name used in the s/AwbStatus/AwbResults structure
> documentation. I don't really care using awb.status or awb.results.

I hear from Laurent he may also prefer an integer key. I like the sound
of that as it means we can define the storage elements as an enum at
least by name.

I would expect that in turn would save on either the string compares or
the hash function of the map (though, even integers I presume would get
hashed for a map).

But at least it defines what entries 'can' go into the metadata somewhere.

That would then need a centralised header that all users of the metadata
include to define those keys though. I'm not sure if I see that as a
benefit or a compromise yet ;-)


>>
>>
>>> +}
>>> +
>>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, Metadata *imageMetadata)
>>>  {
>>>  	/*
>>>  	 * Green gains should not be touched and considered 1.
>>>  	 * Default is 16, so do not change it at all.
>>> -	 * 4096 is the value for a gain of 1.0
>>> +	 * 8192 is the value for a gain of 1.0
>>>  	 */
>>> -	params.acc_param.bnr.wb_gains.gr = 16;
>>> -	params.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;
>>> -	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
>>> -	params.acc_param.bnr.wb_gains.gb = 16;
>>> +	params.acc_param.bnr.wb_gains.gr = 8192;
>>> +	params.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;
>>> +	params.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;
>>> +	params.acc_param.bnr.wb_gains.gb = 8192;
>>
>> Is this change part of using the metadata?
>>
>> We're going from 4096 to 8192. That sounds like a specific change worthy
>> of it's own commit message to explain why this change occurs ?
> 
> Yes.
> 
>>
>>
>>> +
>>> +	/* When the AGC algorithm has run, it may have set a new gamma */
>>> +	double agcGamma = 1.0;
>>> +	if (imageMetadata->get("agc.gamma", agcGamma) != 0)
>>> +		LOG(IPU3Awb, Debug) << "Awb: no gamma found, defaulted to 1.0";
>>>  
>>>  	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
>>>  			    << " and gamma calculated: " << agcGamma;
>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>>> index 122cf68c..cf12032d 100644
>>> --- a/src/ipa/ipu3/ipu3_awb.h
>>> +++ b/src/ipa/ipu3/ipu3_awb.h
>>> @@ -14,14 +14,18 @@
>>>  #include <libcamera/geometry.h>
>>>  
>>>  #include "libipa/algorithm.h"
>>> +#include "libipa/metadata.h"
>>>  
>>>  namespace libcamera {
>>>  
>>>  namespace ipa::ipu3 {
>>>  
>>> -/* Region size for the statistics generation algorithm */
>>> +/* Width of the AWB regions used for Grey World calculation */
>>>  static constexpr uint32_t kAwbStatsSizeX = 16;
>>> +/* Height of the AWB regions used for Grey World calculation */
>>>  static constexpr uint32_t kAwbStatsSizeY = 12;
>>> +/* Total size of the AWB regions used for Grey World calculation */
>>> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY;
>>>  
>>>  class IPU3Awb : public Algorithm
>>
>> Should this be named IPU3AwbGreyworld?
>>
>>
>> Presumably we might potentially have another Awb implementation which
>> would run a different algorithm?
>>
>> (I wonder then what the implications are for switching between the
>> algorithms in that case).
>>
>> But perhaps we should leave that name as is for now until we have a more
>> modular construction for the independent algorithms.
>>
> 
> I think we should not name it differently for now, as we don't have
> another algorithm yet :-).
> 
>>
>>>  {
>>> @@ -30,17 +34,8 @@ public:
>>>  	~IPU3Awb();
>>>  
>>>  	void initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>>> -	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>>> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
>>> -
>>> -	struct Ipu3AwbCell {
>>> -		unsigned char greenRedAvg;
>>> -		unsigned char redAvg;
>>> -		unsigned char blueAvg;
>>> -		unsigned char greenBlueAvg;
>>> -		unsigned char satRatio;
>>> -		unsigned char padding[3];
>>> -	} __attribute__((packed));
>>> +	void process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata);
>>> +	void updateWbParameters(ipu3_uapi_params &params, Metadata *imageMetadata);
>>>  
>>>  	/* \todo Make these three structs available to all the ISPs ? */
>>>  	struct RGB {
>>> @@ -56,7 +51,7 @@ public:
>>>  		}
>>>  	};
>>>  
>>> -	struct IspStatsRegion {
>>> +	struct StatsRegion {
>>>  		unsigned int counted;
>>>  		unsigned int uncounted;
>>>  		unsigned long long rSum;
>>> @@ -71,18 +66,29 @@ public:
>>>  		double blueGain;
>>>  	};
>>>  
>>> +	struct Ipu3AwbCell {
>>> +		unsigned char greenRedAvg;
>>> +		unsigned char redAvg;
>>> +		unsigned char blueAvg;
>>> +		unsigned char greenBlueAvg;
>>> +		unsigned char satRatio;
>>> +		unsigned char padding[3];
>>> +	};
>>> +
>>>  private:
>>>  	void generateZones(std::vector<RGB> &zones);
>>>  	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
>>>  	void clearAwbStats();
>>>  	void awbGreyWorld();
>>>  	uint32_t estimateCCT(double red, double green, double blue);
>>> +	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>>>  
>>>  	struct ipu3_uapi_grid_config awbGrid_;
>>>  
>>>  	std::vector<RGB> zones_;
>>> -	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>>> +	StatsRegion awbStats_[kAwbStatsSize];
>>>  	AwbStatus asyncResults_;
>>> +	uint32_t minZonesCounted_;
>>>  };
>>>  
>>>  } /* namespace ipa::ipu3 */
>>>