[libcamera-devel] [PATCH v6 5/5] ipa: ipu3: awb: Make the naming consistent

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Thu, Sep 09, 2021 at 03:54:49PM +0200, Jean-Michel Hautbois wrote:
> The variables mix the terms cell, region and zone. It can confuse the
> reader, and make the algorithm more difficult to follow.
> 
> Rename the local variables consistent with their definitions:
> - Cells are defined in Pixels
> - Zones are defined in Cells
> 
> Their is no "region" as such, so replace it with the correct term.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index cf97208f..fd68b359 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -222,30 +222,30 @@ void Awb::generateZones(std::vector<RGB> &zones)
>  void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>  			   const ipu3_uapi_grid_config &grid)
>  {
> -	uint32_t regionWidth = round(grid.width / static_cast<double>(kAwbStatsSizeX));
> -	uint32_t regionHeight = round(grid.height / static_cast<double>(kAwbStatsSizeY));
> +	uint32_t cellWidth = round(grid.width / static_cast<double>(kAwbStatsSizeX));
> +	uint32_t cellHeight = round(grid.height / static_cast<double>(kAwbStatsSizeY));

Here you divide the number of cells in the image by the number of zones
in the image, so you get a number of cells per zone. I'd call this
either zoneWidth and zoneHeight, or cellsPerZoneX and cellsPerZoneY.

>  
>  	/*
>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>  	 * (grid.width x grid.height).
>  	 */
> -	for (unsigned int j = 0; j < kAwbStatsSizeY * regionHeight; j++) {
> -		for (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {
> +	for (unsigned int j = 0; j < kAwbStatsSizeY * cellHeight; j++) {
> +		for (unsigned int i = 0; i < kAwbStatsSizeX * cellWidth; i++) {
>  			uint32_t cellPosition = j * grid.width + i;
> -			uint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;
> -			uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
> +			uint32_t cellX = (cellPosition / cellWidth) % kAwbStatsSizeX;
> +			uint32_t cellY = ((cellPosition / grid.width) / cellHeight) % kAwbStatsSizeY;

Here you compute the zone coordinates, so it should be zoneX
and zoneY.

Wouldn't it also be simpler to compute them as follows ?

			uint32_t zoneX = i / cellsPerZoneX;
			uint32_t zoneY = j / cellsPerZoneY;

That could be done in another patch. I would then also rename i and j to
cellX and cellY.

>  
> -			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> +			uint32_t awbZonePosition = cellY * kAwbStatsSizeX + cellX;
>  
>  			/* Cast the initial IPU3 structure to simplify the reading */
>  			const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[cellPosition];
>  			if (currentCell->sat_ratio == 0) {
>  				/* The cell is not saturated, use the current cell */
> -				awbStats_[awbRegionPosition].counted++;
> +				awbStats_[awbZonePosition].counted++;
>  				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
> -				awbStats_[awbRegionPosition].sum.green += greenValue / 2;
> -				awbStats_[awbRegionPosition].sum.red += currentCell->R_avg;
> -				awbStats_[awbRegionPosition].sum.blue += currentCell->B_avg;
> +				awbStats_[awbZonePosition].sum.green += greenValue / 2;
> +				awbStats_[awbZonePosition].sum.red += currentCell->R_avg;
> +				awbStats_[awbZonePosition].sum.blue += currentCell->B_avg;
>  			}
>  		}
>  	}

-- 
Regards,

Laurent Pinchart