[libcamera-devel,v2,11/12] ipa: ipu3: agc: Rewrite and simplify the brightness loop
diff mbox series

Message ID 20210930093715.73293-12-jeanmichel.hautbois@ideasonboard.com
State Accepted
Headers show
Series
  • Improve ImgU statistics usage
Related show

Commit Message

Jean-Michel Hautbois Sept. 30, 2021, 9:37 a.m. UTC 
Now that we know how the AWB statistics are formatted, use a simplified
loop in processBrightness() to parse the green values and get the
histogram.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 55 ++++++++++++---------------------
 src/ipa/ipu3/algorithms/agc.h   |  2 ++
 2 files changed, 22 insertions(+), 35 deletions(-)

Comments

Kieran Bingham Sept. 30, 2021, 3:17 p.m. UTC | #1 
On 30/09/2021 10:37, Jean-Michel Hautbois wrote:
> Now that we know how the AWB statistics are formatted, use a simplified
> loop in processBrightness() to parse the green values and get the
> histogram.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 55 ++++++++++++---------------------
>  src/ipa/ipu3/algorithms/agc.h   |  2 ++
>  2 files changed, 22 insertions(+), 35 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 5ff50f4a..46e01fc4 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "agc.h"
> +#include "awb.h"
>  
>  #include <algorithm>
>  #include <chrono>
> @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;
>  static constexpr uint32_t knumHistogramBins = 256;
>  static constexpr double kEvGainTarget = 0.5;
>  
> -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
> -static constexpr uint8_t kCellSize = 8;
> -
>  Agc::Agc()
>  	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>  	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
> @@ -57,9 +55,10 @@ Agc::Agc()
>  {
>  }
>  
> -int Agc::configure([[maybe_unused]] IPAContext &context,
> -		   const IPAConfigInfo &configInfo)
> +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>  {
> +	stride_ = context.configuration.grid.stride;
> +
>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>  		      / configInfo.sensorInfo.pixelRate;
>  	maxExposureTime_ = kMaxExposure * lineDuration_;
> @@ -70,37 +69,23 @@ int Agc::configure([[maybe_unused]] IPAContext &context,
>  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>  			    const ipu3_uapi_grid_config &grid)
>  {
> -	const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
> -	Rectangle aeRegion = { statsAeGrid.x_start,
> -			       statsAeGrid.y_start,
> -			       static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,
> -			       static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };
> -	Point topleft = aeRegion.topLeft();
> -	int topleftX = topleft.x >> grid.block_width_log2;
> -	int topleftY = topleft.y >> grid.block_height_log2;
> -
> -	/* Align to the grid cell width and height */
> -	uint32_t startX = topleftX << grid.block_width_log2;
> -	uint32_t startY = topleftY * grid.width << grid.block_width_log2;
> -	uint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;
> -	uint32_t i, j;
> -	uint32_t count = 0;
> -
>  	uint32_t hist[knumHistogramBins] = { 0 };
> -	for (j = topleftY;
> -	     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);
> -	     j++) {
> -		for (i = startX + startY; i < endX + startY; i += kCellSize) {
> -			/*
> -			 * The grid width (and maybe height) is not reliable.
> -			 * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
> -			 * Use the one passed at init time.
> -			 */
> -			if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
> -				uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
> -				uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
> -				hist[(Gr + Gb) / 2]++;
> -				count++;
> +
> +	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> +		for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
> +			uint32_t cellPosition = cellY * stride_ + cellX
> +					      * sizeof(Ipu3AwbCell);
> +
> +			/* Cast the initial IPU3 structure to simplify the reading */

I'm not sure that comment adds much, but it doesn't hurt anything.

Stride issues are all fixed up, and I think the Ipu3AwbCell gets changed
to the new structure in the next patch...

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


> +			const Ipu3AwbCell *cell =
> +				reinterpret_cast<const Ipu3AwbCell *>(
> +					&stats->awb_raw_buffer.meta_data[cellPosition]
> +				);
> +
> +			if (cell->satRatio == 0) {
> +				uint8_t gr = cell->greenRedAvg;
> +				uint8_t gb = cell->greenBlueAvg;
> +				hist[(gr + gb) / 2]++;
>  			}
>  		}
>  	}
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index e36797d3..64b71c65 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -50,6 +50,8 @@ private:
>  	Duration prevExposureNoDg_;
>  	Duration currentExposure_;
>  	Duration currentExposureNoDg_;
> +
> +	uint32_t stride_;
>  };
>  
>  } /* namespace ipa::ipu3::algorithms */
>
Laurent Pinchart Oct. 5, 2021, 1:38 a.m. UTC | #2 
Hi Jean-Michel,

Thank you for the patch.

On Thu, Sep 30, 2021 at 04:17:27PM +0100, Kieran Bingham wrote:
> On 30/09/2021 10:37, Jean-Michel Hautbois wrote:
> > Now that we know how the AWB statistics are formatted, use a simplified
> > loop in processBrightness() to parse the green values and get the
> > histogram.
> > 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp | 55 ++++++++++++---------------------
> >  src/ipa/ipu3/algorithms/agc.h   |  2 ++
> >  2 files changed, 22 insertions(+), 35 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index 5ff50f4a..46e01fc4 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include "agc.h"
> > +#include "awb.h"
> >  
> >  #include <algorithm>
> >  #include <chrono>
> > @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;
> >  static constexpr uint32_t knumHistogramBins = 256;
> >  static constexpr double kEvGainTarget = 0.5;
> >  
> > -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
> > -static constexpr uint8_t kCellSize = 8;
> > -
> >  Agc::Agc()
> >  	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> >  	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
> > @@ -57,9 +55,10 @@ Agc::Agc()
> >  {
> >  }
> >  
> > -int Agc::configure([[maybe_unused]] IPAContext &context,
> > -		   const IPAConfigInfo &configInfo)
> > +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >  {
> > +	stride_ = context.configuration.grid.stride;
> > +
> >  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
> >  		      / configInfo.sensorInfo.pixelRate;
> >  	maxExposureTime_ = kMaxExposure * lineDuration_;
> > @@ -70,37 +69,23 @@ int Agc::configure([[maybe_unused]] IPAContext &context,
> >  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> >  			    const ipu3_uapi_grid_config &grid)
> >  {
> > -	const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
> > -	Rectangle aeRegion = { statsAeGrid.x_start,
> > -			       statsAeGrid.y_start,
> > -			       static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,
> > -			       static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };
> > -	Point topleft = aeRegion.topLeft();
> > -	int topleftX = topleft.x >> grid.block_width_log2;
> > -	int topleftY = topleft.y >> grid.block_height_log2;
> > -
> > -	/* Align to the grid cell width and height */
> > -	uint32_t startX = topleftX << grid.block_width_log2;
> > -	uint32_t startY = topleftY * grid.width << grid.block_width_log2;
> > -	uint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;
> > -	uint32_t i, j;
> > -	uint32_t count = 0;
> > -
> >  	uint32_t hist[knumHistogramBins] = { 0 };
> > -	for (j = topleftY;
> > -	     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);
> > -	     j++) {
> > -		for (i = startX + startY; i < endX + startY; i += kCellSize) {
> > -			/*
> > -			 * The grid width (and maybe height) is not reliable.
> > -			 * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
> > -			 * Use the one passed at init time.
> > -			 */
> > -			if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
> > -				uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
> > -				uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
> > -				hist[(Gr + Gb) / 2]++;
> > -				count++;
> > +
> > +	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> > +		for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
> > +			uint32_t cellPosition = cellY * stride_ + cellX
> > +					      * sizeof(Ipu3AwbCell);

			uint32_t cellPosition = (cellY * stride_ + cellX)
					      * sizeof(Ipu3AwbCell);

Likely went unnoticed because the next patch modifies this.

> > +
> > +			/* Cast the initial IPU3 structure to simplify the reading */
> 
> I'm not sure that comment adds much, but it doesn't hurt anything.

Indeed, I would drop it too.

> Stride issues are all fixed up, and I think the Ipu3AwbCell gets changed
> to the new structure in the next patch...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

> > +			const Ipu3AwbCell *cell =
> > +				reinterpret_cast<const Ipu3AwbCell *>(
> > +					&stats->awb_raw_buffer.meta_data[cellPosition]
> > +				);
> > +
> > +			if (cell->satRatio == 0) {
> > +				uint8_t gr = cell->greenRedAvg;
> > +				uint8_t gb = cell->greenBlueAvg;
> > +				hist[(gr + gb) / 2]++;
> >  			}
> >  		}
> >  	}
> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > index e36797d3..64b71c65 100644
> > --- a/src/ipa/ipu3/algorithms/agc.h
> > +++ b/src/ipa/ipu3/algorithms/agc.h
> > @@ -50,6 +50,8 @@ private:
> >  	Duration prevExposureNoDg_;
> >  	Duration currentExposure_;
> >  	Duration currentExposureNoDg_;
> > +
> > +	uint32_t stride_;
> >  };
> >  
> >  } /* namespace ipa::ipu3::algorithms */

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 5ff50f4a..46e01fc4 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -6,6 +6,7 @@ 
  */
 
 #include "agc.h"
+#include "awb.h"
 
 #include <algorithm>
 #include <chrono>
@@ -47,9 +48,6 @@  static constexpr uint32_t kMaxExposure = 1976;
 static constexpr uint32_t knumHistogramBins = 256;
 static constexpr double kEvGainTarget = 0.5;
 
-/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
-static constexpr uint8_t kCellSize = 8;
-
 Agc::Agc()
 	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
 	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
@@ -57,9 +55,10 @@  Agc::Agc()
 {
 }
 
-int Agc::configure([[maybe_unused]] IPAContext &context,
-		   const IPAConfigInfo &configInfo)
+int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
 {
+	stride_ = context.configuration.grid.stride;
+
 	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
 		      / configInfo.sensorInfo.pixelRate;
 	maxExposureTime_ = kMaxExposure * lineDuration_;
@@ -70,37 +69,23 @@  int Agc::configure([[maybe_unused]] IPAContext &context,
 void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
 			    const ipu3_uapi_grid_config &grid)
 {
-	const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
-	Rectangle aeRegion = { statsAeGrid.x_start,
-			       statsAeGrid.y_start,
-			       static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,
-			       static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };
-	Point topleft = aeRegion.topLeft();
-	int topleftX = topleft.x >> grid.block_width_log2;
-	int topleftY = topleft.y >> grid.block_height_log2;
-
-	/* Align to the grid cell width and height */
-	uint32_t startX = topleftX << grid.block_width_log2;
-	uint32_t startY = topleftY * grid.width << grid.block_width_log2;
-	uint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;
-	uint32_t i, j;
-	uint32_t count = 0;
-
 	uint32_t hist[knumHistogramBins] = { 0 };
-	for (j = topleftY;
-	     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);
-	     j++) {
-		for (i = startX + startY; i < endX + startY; i += kCellSize) {
-			/*
-			 * The grid width (and maybe height) is not reliable.
-			 * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
-			 * Use the one passed at init time.
-			 */
-			if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
-				uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
-				uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
-				hist[(Gr + Gb) / 2]++;
-				count++;
+
+	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
+		for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
+			uint32_t cellPosition = cellY * stride_ + cellX
+					      * sizeof(Ipu3AwbCell);
+
+			/* Cast the initial IPU3 structure to simplify the reading */
+			const Ipu3AwbCell *cell =
+				reinterpret_cast<const Ipu3AwbCell *>(
+					&stats->awb_raw_buffer.meta_data[cellPosition]
+				);
+
+			if (cell->satRatio == 0) {
+				uint8_t gr = cell->greenRedAvg;
+				uint8_t gb = cell->greenBlueAvg;
+				hist[(gr + gb) / 2]++;
 			}
 		}
 	}
diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
index e36797d3..64b71c65 100644
--- a/src/ipa/ipu3/algorithms/agc.h
+++ b/src/ipa/ipu3/algorithms/agc.h
@@ -50,6 +50,8 @@  private:
 	Duration prevExposureNoDg_;
 	Duration currentExposure_;
 	Duration currentExposureNoDg_;
+
+	uint32_t stride_;
 };
 
 } /* namespace ipa::ipu3::algorithms */