[libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and simplify the brightness loop

[Jean-Michel Hautbois]

On 28/09/2021 18:26, Kieran Bingham wrote:
> On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:
>> Now that we know how exactly 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 at ideasonboard.com>
>> ---
>>  src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------
>>  src/ipa/ipu3/algorithms/agc.h   |  2 ++
>>  2 files changed, 23 insertions(+), 33 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 5ff50f4a..ebbc5676 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),
>> @@ -60,6 +58,10 @@ Agc::Agc()
>>  int Agc::configure([[maybe_unused]] IPAContext &context,
> 
> looks like context is no longer [[maybe_unused]].
> 
> 
>>  		   const IPAConfigInfo &configInfo)
>>  {
>> +	const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;
>> +	/* The grid is aligned to the next multiple of 4 */
>> +	stride_ = (grid.width + 3) / 4 * 4;
> 
> This is familiar...
> 
> How about putting a stride into context.configuration.grid.stride and
> calculating it once during calculateBdsGrid()?
> 

Then I need to pass the grid to the processBrightness(), again (and in
AWB too) while we could get rid of it. Or cache the value in configure() ?

>> +
>>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>>  		      / configInfo.sensorInfo.pixelRate;
>>  	maxExposureTime_ = kMaxExposure * lineDuration_;
>> @@ -70,37 +72,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 < stride_; 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) {
> 
> Is the satRatio usage clear yet? Is it always 0? or do you see other
> values here?
> 
> Should it be less than a saturation threshold? (perhaps not unless we
> have a way to configure that anyway)
> 
> 
>> +				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 */
>> -- 
>> 2.30.2
>>
> 
> --
> Kieran
>