[libcamera-devel] [PATCH v1 2/7] ipa: ipu3: awb: Correct the relevant zones proportion

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Aug 23, 2021 at 02:49:32PM +0200, Jean-Michel Hautbois wrote:
> The algorithm uses the statistics of a cell only if there is not too
> much saturated pixels in it. The grey world algorithm works fine when

s/much/many/

> there are a limited number of outliers.

Blank line or no line break, your decision.

> Consider a valid zone to be at least 80% of unsaturated cells in it.
> This value could very well be configurable, and make the algorithm more
> or less tolerant.

You use "zone", "cell" and "pixel" in a way that confuses me (but it may
be me). In the first paragraph a cell is made of pixels, and in the
second paragraph a zone is made of cells. Furthermore, we have a
IspStatsRegion structure that mentions "region".

It would be easier to review this if there were a patch earlier in the
series that cleaned up the vocabulary.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++++++++---
>  src/ipa/ipu3/algorithms/awb.h   |  1 +
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index e05647c9..9497a21b 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -17,8 +17,6 @@ namespace ipa::ipu3::algorithms {
>  
>  LOG_DEFINE_CATEGORY(IPU3Awb)
>  
> -static constexpr uint32_t kMinZonesCounted = 16;
> -static constexpr uint32_t kMinGreenLevelInZone = 32;
>  
>  /**
>   * \struct IspStatsRegion
> @@ -114,6 +112,9 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
>  	0, 0, 8191, 0
>  };
>  
> +/* Minimum level of green in a given zone */
> +static constexpr uint32_t kMinGreenLevelInZone = 16;
> +
>  Awb::Awb()
>  	: Algorithm()
>  {
> @@ -123,6 +124,7 @@ Awb::Awb()
>  	asyncResults_.temperatureK = 4500;
>  
>  	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> +	minZonesCounted_ = 0;
>  }
>  
>  Awb::~Awb() = default;
> @@ -163,7 +165,7 @@ void Awb::generateZones(std::vector<RGB> &zones)
>  	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; 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;
> @@ -181,6 +183,13 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>  	uint32_t regionWidth = round(grid.width / static_cast<double>(kAwbStatsSizeX));
>  	uint32_t regionHeight = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>  
> +	/*
> +	 * It is the minimum proportion of pixels counted within AWB region
> +	 * for it to be relevant.
> +	 * \todo This proportion could be configured.
> +	 */
> +	minZonesCounted_ = (regionWidth * regionHeight) * 80 / 100;

The grid is a configuration parameter that doesn't change during the
capture session, right ? This should then be moved to configure().

> +
>  	/*
>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>  	 * (grid.width x grid.height).
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index 332652d0..95238b6a 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -84,6 +84,7 @@ private:
>  	std::vector<RGB> zones_;
>  	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>  	AwbStatus asyncResults_;
> +	uint32_t minZonesCounted_;
>  };
>  
>  } /* namespace ipa::ipu3::algorithms */

-- 
Regards,

Laurent Pinchart