[libcamera-devel,v6,5/5] ipa: ipu3: awb: Make the naming consistent
diff mbox series

Message ID 20210909135449.68017-6-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • Move and improve AWB structures
Related show

Commit Message

Jean-Michel Hautbois Sept. 9, 2021, 1:54 p.m. UTC 
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@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/awb.cpp | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Kieran Bingham Sept. 9, 2021, 2:19 p.m. UTC | #1 
On 09/09/2021 14:54, 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.

s/Their/There/

Can be fixed before merging. No need for a resend only on that.



> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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)

Locally I have a comment here:

/* Translate the IPU3 statistics into the default statistics region array */

Does that need updating? (only because I searched for "region".

It still seems to make sense in that context, so I don't think it needs
to change unless you think it's now inaccurate.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>  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));
>  
>  	/*
>  	 * 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;
>  
> -			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;
>  			}
>  		}
>  	}
>
Kieran Bingham Sept. 9, 2021, 2:33 p.m. UTC | #2 
On 09/09/2021 15:19, Kieran Bingham wrote:
> On 09/09/2021 14:54, 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.
> 
> s/Their/There/
> 
> Can be fixed before merging. No need for a resend only on that.
> 
> 
> 
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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)
> 
> Locally I have a comment here:
> 
> /* Translate the IPU3 statistics into the default statistics region array */
> 
> Does that need updating? (only because I searched for "region".
> 
> It still seems to make sense in that context, so I don't think it needs
> to change unless you think it's now inaccurate.

Never mind, I now see that's done in this series already.

--
Kieran


> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
>>  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));
>>  
>>  	/*
>>  	 * 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;
>>  
>> -			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;
>>  			}
>>  		}
>>  	}
>>
Laurent Pinchart Sept. 10, 2021, 4:21 p.m. UTC | #3 
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@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;
>  			}
>  		}
>  	}

Patch
diff mbox series 

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));
 
 	/*
 	 * 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;
 
-			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;
 			}
 		}
 	}