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

Message ID 20210923081625.60276-5-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • Improve ImgU statistics usage
Related show

Commit Message

Jean-Michel Hautbois Sept. 23, 2021, 8:16 a.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
There 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 | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Kieran Bingham Sept. 28, 2021, 2:10 p.m. UTC | #1 
On Thu, Sep 23, 2021 at 10:16:17AM +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:

s/variables consistent/variables to be consistent/

> - Cells are defined in Pixels
> - Zones are defined in Cells
> There is no "region" as such, so replace it with the correct term.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

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

> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index d78ae4f4..51a38d05 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -220,31 +220,34 @@ 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 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 j = 0; j < kAwbStatsSizeY * regionHeight; j++) {
> -		for (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {
> -			uint32_t cellPosition = j * grid.width + i;
> -			uint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;
> -			uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
> +	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 awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> -			cellPosition *= 8;
> +			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
>  
>  			/* 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]));
> +			const Ipu3AwbCell *currentCell =
> +				reinterpret_cast<const Ipu3AwbCell *>(
> +					&stats->awb_raw_buffer.meta_data[cellPosition]
> +				);
>  			if (currentCell->satRatio == 0) {
>  				/* The cell is not saturated, use the current cell */
> -				awbStats_[awbRegionPosition].counted++;
> +				awbStats_[awbZonePosition].counted++;
>  				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
> -				awbStats_[awbRegionPosition].sum.green += greenValue / 2;
> -				awbStats_[awbRegionPosition].sum.red += currentCell->redAvg;
> -				awbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;
> +				awbStats_[awbZonePosition].sum.green += greenValue / 2;
> +				awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
> +				awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
>  			}
>  		}
>  	}
> -- 
> 2.30.2
> 

--
Kieran
Laurent Pinchart Sept. 29, 2021, 11:40 a.m. UTC | #2 
On Tue, Sep 28, 2021 at 03:10:20PM +0100, Kieran Bingham wrote:
> On Thu, Sep 23, 2021 at 10:16:17AM +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:
> 
> s/variables consistent/variables to be consistent/
> 
> > - Cells are defined in Pixels
> > - Zones are defined in Cells
> > There is no "region" as such, so replace it with the correct term.
> > 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > ---
> >  src/ipa/ipu3/algorithms/awb.cpp | 31 +++++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > index d78ae4f4..51a38d05 100644
> > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > @@ -220,31 +220,34 @@ 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 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 j = 0; j < kAwbStatsSizeY * regionHeight; j++) {
> > -		for (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {
> > -			uint32_t cellPosition = j * grid.width + i;
> > -			uint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;
> > -			uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
> > +	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 awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> > -			cellPosition *= 8;
> > +			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
> >  
> >  			/* 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]));
> > +			const Ipu3AwbCell *currentCell =
> > +				reinterpret_cast<const Ipu3AwbCell *>(
> > +					&stats->awb_raw_buffer.meta_data[cellPosition]
> > +				);
> >  			if (currentCell->satRatio == 0) {
> >  				/* The cell is not saturated, use the current cell */
> > -				awbStats_[awbRegionPosition].counted++;
> > +				awbStats_[awbZonePosition].counted++;
> >  				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
> > -				awbStats_[awbRegionPosition].sum.green += greenValue / 2;
> > -				awbStats_[awbRegionPosition].sum.red += currentCell->redAvg;
> > -				awbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;
> > +				awbStats_[awbZonePosition].sum.green += greenValue / 2;
> > +				awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
> > +				awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
> >  			}
> >  		}
> >  	}

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
index d78ae4f4..51a38d05 100644
--- a/src/ipa/ipu3/algorithms/awb.cpp
+++ b/src/ipa/ipu3/algorithms/awb.cpp
@@ -220,31 +220,34 @@  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 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 j = 0; j < kAwbStatsSizeY * regionHeight; j++) {
-		for (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {
-			uint32_t cellPosition = j * grid.width + i;
-			uint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;
-			uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
+	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 awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
-			cellPosition *= 8;
+			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
 
 			/* 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]));
+			const Ipu3AwbCell *currentCell =
+				reinterpret_cast<const Ipu3AwbCell *>(
+					&stats->awb_raw_buffer.meta_data[cellPosition]
+				);
 			if (currentCell->satRatio == 0) {
 				/* The cell is not saturated, use the current cell */
-				awbStats_[awbRegionPosition].counted++;
+				awbStats_[awbZonePosition].counted++;
 				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
-				awbStats_[awbRegionPosition].sum.green += greenValue / 2;
-				awbStats_[awbRegionPosition].sum.red += currentCell->redAvg;
-				awbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;
+				awbStats_[awbZonePosition].sum.green += greenValue / 2;
+				awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
+				awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
 			}
 		}
 	}