[libcamera-devel] [PATCH 09/12] ipa: ipu3: awb: Use the line stride for the stats

[Laurent Pinchart]

On Wed, Sep 29, 2021 at 02:47:15PM +0200, Jean-Michel Hautbois wrote:
> On 29/09/2021 14:44, Laurent Pinchart wrote:
> > On Tue, Sep 28, 2021 at 05:02:42PM +0100, Kieran Bingham wrote:
> >> On Thu, Sep 23, 2021 at 10:16:22AM +0200, Jean-Michel Hautbois wrote:
> >>> The statistics buffer 'ipu3_uapi_awb_raw_buffer' stores the ImgU
> >>> calculation results in a buffer aligned to a multiple of 4. The AWB loop
> > 
> > s/aligned to a multiple of 4/aligned horizontally to a multiple of 4 cells/
> > 
> >>> should take care of it to add the proper offset between lines and avoid
> >>> any staircase effect.
> >>>
> >>> It is now no more required to pass the grid configuration context to the
> >>
> >> "It is no longer required to ..."
> >>
> >> That's a nice perk indeed.
> >>
> >>> private functions called from process() which simplifies the code flow.
> >>>
> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >>> ---
> >>>  src/ipa/ipu3/algorithms/awb.cpp | 16 ++++++++--------
> >>>  src/ipa/ipu3/algorithms/awb.h   |  7 +++----
> >>>  2 files changed, 11 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> >>> index 8a926691..a5391653 100644
> >>> --- a/src/ipa/ipu3/algorithms/awb.cpp
> >>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> >>> @@ -172,8 +172,10 @@ int Awb::configure(IPAContext &context,
> >>>  		   [[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 */
> > 
> > s/grid/grid width/
> > 
> >>> +	stride_ = (grid.width + 3) / 4 * 4;
> >>
> >> Feels like something that could have been handled by an alignedUp macro.
> >> We have that in geometry.h but for Size ...
> >>
> >> So perhaps we can just open code it (unless anyone knows of a
> >> std:: macro that does this?)
> > 
> > utils::alignUp()
> > 
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >>> -	cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX));
> >>> +	cellsPerZoneX_ = round(stride_ / static_cast<double>(kAwbStatsSizeX));
> > 
> > This isn't right, you will end up using the padding cells in your
> > calculations.
> 
> The padding cell is all black, so it will not be taken into account in
> Awb::generateZones() as the green value will be lower than
> kMinGreenLevelInZone.

That's not a documented guarantee, I don't think we should rely on this.

> >>>  	cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY));
> >>>  
> >>>  	/*
> >>> @@ -234,8 +236,7 @@ void Awb::generateZones(std::vector<RGB> &zones)
> >>>  }
> >>>  
> >>>  /* Translate the IPU3 statistics into the default statistics zone array */
> >>> -void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
> >>> -			   const ipu3_uapi_grid_config &grid)
> >>> +void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
> >>>  {
> >>>  	/*
> >>>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
> >>> @@ -243,7 +244,7 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
> >>>  	 */
> >>>  	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)
> >>> +			uint32_t cellPosition = (cellY * stride_ + cellX)
> >>>  					      * sizeof(Ipu3AwbCell);
> >>>  			uint32_t zoneX = cellX / cellsPerZoneX_;
> >>>  			uint32_t zoneY = cellY / cellsPerZoneY_;
> >>> @@ -318,13 +319,12 @@ void Awb::awbGreyWorld()
> >>>  	asyncResults_.blueGain = blueGain;
> >>>  }
> >>>  
> >>> -void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,
> >>> -			   const ipu3_uapi_grid_config &grid)
> >>> +void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> >>>  {
> >>>  	ASSERT(stats->stats_3a_status.awb_en);
> >>>  	zones_.clear();
> >>>  	clearAwbStats();
> >>> -	generateAwbStats(stats, grid);
> >>> +	generateAwbStats(stats);
> >>>  	generateZones(zones_);
> >>>  	LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
> >>>  	if (zones_.size() > 10) {
> >>> @@ -336,7 +336,7 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,
> >>>  
> >>>  void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >>>  {
> >>> -	calculateWBGains(stats, context.configuration.grid.bdsGrid);
> >>> +	calculateWBGains(stats);
> >>>  
> >>>  	/*
> >>>  	 * Gains are only recalculated if enough zones were detected.
> >>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> >>> index 681d8c2b..b3e0ad82 100644
> >>> --- a/src/ipa/ipu3/algorithms/awb.h
> >>> +++ b/src/ipa/ipu3/algorithms/awb.h
> >>> @@ -74,11 +74,9 @@ public:
> >>>  	};
> >>>  
> >>>  private:
> >>> -	void calculateWBGains(const ipu3_uapi_stats_3a *stats,
> >>> -			      const ipu3_uapi_grid_config &grid);
> >>> +	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> >>>  	void generateZones(std::vector<RGB> &zones);
> >>> -	void generateAwbStats(const ipu3_uapi_stats_3a *stats,
> >>> -			      const ipu3_uapi_grid_config &grid);
> >>> +	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
> >>>  	void clearAwbStats();
> >>>  	void awbGreyWorld();
> >>>  	uint32_t estimateCCT(double red, double green, double blue);
> >>> @@ -87,6 +85,7 @@ private:
> >>>  	Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
> >>>  	AwbStatus asyncResults_;
> >>>  
> >>> +	uint32_t stride_;
> >>>  	uint32_t cellsPerZoneX_;
> >>>  	uint32_t cellsPerZoneY_;
> >>>  	uint32_t cellsPerZoneThreshold_;

-- 
Regards,

Laurent Pinchart