[libcamera-devel] [PATCH 07/12] ipa: ipu3: awb: Correct the relevant zones proportion

[Jean-Michel Hautbois]

Hi Laurent,

On 29/09/2021 14:36, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> On Wed, Sep 29, 2021 at 02:32:00PM +0200, Jean-Michel Hautbois wrote:
>> On 29/09/2021 14:15, Laurent Pinchart wrote:
>>> On Thu, Sep 23, 2021 at 10:16:20AM +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
>>>> there are a limited number of outliers.
>>>> 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.
>>>>
>>>> While at it, implement it in a configure() call as it will not change
>>>> during execution, and cache the cellsPerZone values.
>>>>
>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>>> ---
>>>>  src/ipa/ipu3/algorithms/awb.cpp | 32 +++++++++++++++++++++++---------
>>>>  src/ipa/ipu3/algorithms/awb.h   |  5 +++++
>>>>  2 files changed, 28 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>>>> index 51a38d05..800d023c 100644
>>>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>>>> @@ -17,7 +17,6 @@ namespace ipa::ipu3::algorithms {
>>>>  
>>>>  LOG_DEFINE_CATEGORY(IPU3Awb)
>>>>  
>>>> -static constexpr uint32_t kMinZonesCounted = 16;
>>>>  static constexpr uint32_t kMinGreenLevelInZone = 32;
>>>>  
>>>>  /**
>>>> @@ -169,6 +168,24 @@ Awb::Awb()
>>>>  
>>>>  Awb::~Awb() = default;
>>>>  
>>>> +int Awb::configure(IPAContext &context,
>>>> +		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>>>> +{
>>>> +	const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;
>>>> +
>>>> +	cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX));
>>>
>>> s/round/std::round/
>>>
>>> as you're using cmath (with a small corresponding comment in the commit
>>> message).
>>
>> OK :-).
>>
>>>> +	cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>>>> +
>>>> +	/*
>>>> +	 * Configure the minimum proportion of cells counted within a zone
>>>> +	 * for it to be relevant for the grey world algorithm.
>>>> +	 * \todo This proportion could be configured.
>>>> +	 */
>>>> +	cellsPerZoneThreshold_ = (cellsPerZoneX_ * cellsPerZoneY_) * 80 / 100;
>>>
>>> No need for parentheses.
>>>
>>> The patch looks fine to me, so
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>
>>> This however led me to notice that if the grid width isn't a multiple of
>>> 16, we'll ignore some of the cells. If the grid width is 79, for
>>> instance, 15 cells will be ignored. Would it be worth fixing that ?
>>
>> We could still do :
>> cellsPerZoneX_ = std::round(stride_ / static_cast<double>(kAwbStatsSizeX));
>>
>> But we haven't included stride yet :-)
> 
> I think the best solution for this issue may be a variable number of
> cells per zone. The question still holds though, is this an issue that
> needs to be addressed, or is it harmless in practice ?
> 

I think it is not needed, as it would not improve the behavior
significantly imo (I may be wrong though !).

>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  /**
>>>>   * The function estimates the correlated color temperature using
>>>>   * from RGB color space input.
>>>> @@ -205,7 +222,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 >= cellsPerZoneThreshold_) {
>>>>  			zone.G = awbStats_[i].sum.green / counted;
>>>>  			if (zone.G >= kMinGreenLevelInZone) {
>>>>  				zone.R = awbStats_[i].sum.red / counted;
>>>> @@ -220,19 +237,16 @@ void Awb::generateZones(std::vector<RGB> &zones)
>>>>  void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>>>>  			   const ipu3_uapi_grid_config &grid)
>>>>  {
>>>> -	uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX));
>>>> -	uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>>>> -
>>>>  	/*
>>>>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>>>>  	 * (grid.width x grid.height).
>>>>  	 */
>>>> -	for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) {
>>>> -		for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) {
>>>> +	for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) {
>>>> +		for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) {
>>>>  			uint32_t cellPosition = (cellY * grid.width + cellX)
>>>>  					      * sizeof(Ipu3AwbCell);
>>>> -			uint32_t zoneX = cellX / cellsPerZoneX;
>>>> -			uint32_t zoneY = cellY / cellsPerZoneY;
>>>> +			uint32_t zoneX = cellX / cellsPerZoneX_;
>>>> +			uint32_t zoneY = cellY / cellsPerZoneY_;
>>>>  
>>>>  			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
>>>>  
>>>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
>>>> index 3385ebe7..681d8c2b 100644
>>>> --- a/src/ipa/ipu3/algorithms/awb.h
>>>> +++ b/src/ipa/ipu3/algorithms/awb.h
>>>> @@ -48,6 +48,7 @@ public:
>>>>  	Awb();
>>>>  	~Awb();
>>>>  
>>>> +	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>>>>  	void prepare(IPAContext &context, ipu3_uapi_params *params) override;
>>>>  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>>>  
>>>> @@ -85,6 +86,10 @@ private:
>>>>  	std::vector<RGB> zones_;
>>>>  	Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>>>>  	AwbStatus asyncResults_;
>>>> +
>>>> +	uint32_t cellsPerZoneX_;
>>>> +	uint32_t cellsPerZoneY_;
>>>> +	uint32_t cellsPerZoneThreshold_;
>>>>  };
>>>>  
>>>>  } /* namespace ipa::ipu3::algorithms */
>