[libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename IspStatsRegion to Accumulator

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Sep 13, 2021 at 03:28:00PM +0200, Jean-Michel Hautbois wrote:
> The IspStatsRegion structure was introduced as an attempt to prepare for
> a generic AWB algorithm structure. The structure name by itself is not
> explicit and it is too optimistic to try and make a generic one for now.
> 
> Its role is to accumulate the pixels in a given zone. Rename it to
> accumulator, and remove the uncounted field at the same time. It is
> always possible to know how many pixels are not relevant for the
> algorithm by calculating total-counted. The uncounted field was only
> declared and not used. Amend the documentation accordingly.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 80 +++++++++++++++++++++++++--------
>  src/ipa/ipu3/algorithms/awb.h   |  5 +--
>  2 files changed, 64 insertions(+), 21 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index e05647c9..d993e21e 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -21,26 +21,71 @@ static constexpr uint32_t kMinZonesCounted = 16;
>  static constexpr uint32_t kMinGreenLevelInZone = 32;
>  
>  /**
> - * \struct IspStatsRegion
> - * \brief RGB statistics for a given region
> + * \struct Accumulator
> + * \brief RGB statistics for a given zone
>   *
> - * The IspStatsRegion structure is intended to abstract the ISP specific
> - * statistics and use an agnostic algorithm to compute AWB.
> + * - Cells are defined in Pixels
> + * - Zones are defined in Cells
> + * - The total zones may overlap the image width and height to meet hardware
> + *   constraints

Quoting your reply from v6:

"[This] means that the zones will be adapted to the BDS output size
which may be greater than the rendered frame (asking for 1280x720 may
lead to a 1296x720 BDS output and you will have 81 cells of 16 pixels
width and not 80)."

>From a statistics point of view, the zones match the output of the BDS.
The image is then slightly cropped to achieve the required resolution. I
think that's out of scope here. I would thus drop the last point. The
image and text below need to be adapted to replace 1280 with 1296.

>   *
> - * \var IspStatsRegion::counted
> - * \brief Number of pixels used to calculate the sums
> + *                     81 cells
> + *     /───────────── 1280 pixels ───────────\
> + *                      16 zones
> + *      16
> + *    ┌────┬────┬────┬────┬────┬─  ──────┬────┐   \
> + *    │Cell│    │    │    │    │    |    │    │   │
> + * 16 │ px │    │    │    │    │    |    │    │   │
> + *    ├────┼────┼────┼────┼────┼─  ──────┼────┤   │
> + *    │    │    │    │    │    │    |    │    │
> + *    │    │    │    │    │    │    |    │    │   7
> + *    │ ── │ ── │ ── │ ── │ ── │ ──  ── ─┤ ── │ 1 2 4
> + *    │    │    │    │    │    │    |    │    │ 2 0 5
>   *
> - * \var IspStatsRegion::uncounted
> - * \brief Remaining number of pixels in the region
> + *    │    │    │    │    │    │    |    │    │ z p c
> + *    ├────┼────┼────┼────┼────┼─  ──────┼────┤ o i e
> + *    │    │    │    │    │    │    |    │    │ n x l
> + *    │                        │    |    │    │ e e l
> + *    ├───                  ───┼─  ──────┼────┤ s l s
> + *    │                        │    |    │    │   s
> + *    │                        │    |    │    │
> + *    ├───   Zone of Cells  ───┼─  ──────┼────┤   │
> + *    │        (5 x 4)         │    |    │    │   │
> + *    │                        │    |    │    │   │
> + *    ├──                   ───┼─  ──────┼────┤   │
> + *    │                   │    │    |    │    │   │
> + *    │    │    │    │    │    │    |    │    │   │
> + *    └────┴────┴────┴────┴────┴─  ──────┴────┘   /
>   *
> - * \var IspStatsRegion::rSum
> - * \brief Sum of the red values in the region
> + * The algorithm works with a fixed number of zones \a kAwbStatsSizeX x
> + * \a kAwbStatsSizeY. For example, a frame of 1280x720 is divided into 81x45
> + * cells of [16x16] pixels with a BDS output size of 1296x720 to account for the
> + * 16x16 pixel alignment restrictions. In the case of \a kAwbStatsSizeX=16 and
> + * \a kAwbStatsSizeY=12 the zones are made of [5x4] cells. The cells are
> + * left-aligned and calculated by IPAIPU3::calculateBdsGrid().
>   *
> - * \var IspStatsRegion::gSum
> - * \brief Sum of the green values in the region
> + * Each statistics cell represents the average value of the pixels in that cell
> + * split by colour components.
>   *
> - * \var IspStatsRegion::bSum
> - * \brief Sum of the blue values in the region
> + * The Accumulator structure stores the sum of the average of each cell in a
> + * zone of the image, as well as the number of cells which were unsaturated and
> + * therefore included in the average.
> + * \todo move this description and structure into a common header
> + *
> + * Cells which are saturated beyond the threshold defined in
> + * ipu3_uapi_awb_config_s are not included in the average.
> + *
> + * \var Accumulator::counted
> + * \brief Number of unsaturated cells used to calculate the sums
> + *
> + * \var Accumulator::rSum
> + * \brief Sum of the average red values of each unsaturated cell in the zone
> + *
> + * \var Accumulator::gSum
> + * \brief Sum of the average green values of each unsaturated cell in the zone
> + *
> + * \var Accumulator::bSum
> + * \brief Sum of the average blue values of each unsaturated cell in the zone
>   */
>  
>  /**
> @@ -157,7 +202,7 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)
>  	return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
>  }
>  
> -/* Generate an RGB vector with the average values for each region */
> +/* Generate an RGB vector with the average values for each zone */
>  void Awb::generateZones(std::vector<RGB> &zones)
>  {
>  	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> @@ -174,7 +219,7 @@ void Awb::generateZones(std::vector<RGB> &zones)
>  	}
>  }
>  
> -/* Translate the IPU3 statistics into the default statistics region array */
> +/* 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)
>  {
> @@ -215,7 +260,6 @@ void Awb::clearAwbStats()
>  		awbStats_[i].rSum = 0;
>  		awbStats_[i].gSum = 0;
>  		awbStats_[i].counted = 0;
> -		awbStats_[i].uncounted = 0;
>  	}
>  }
>  
> @@ -304,7 +348,7 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
>  
>  	/*
>  	 * Optical center is column start (respectively row start) of the
> -	 * region of interest minus its X center (respectively Y center).
> +	 * cell of interest minus its X center (respectively Y center).
>  	 *
>  	 * For the moment use BDS as a first approximation, but it should
>  	 * be calculated based on Shading (SHD) parameters.
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index cc848060..ac8ccc84 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -33,9 +33,8 @@ struct Ipu3AwbCell {
>  	unsigned char padding[3];
>  } __attribute__((packed));
>  
> -struct IspStatsRegion {
> +struct Accumulator {
>  	unsigned int counted;
> -	unsigned int uncounted;
>  	unsigned long long rSum;
>  	unsigned long long gSum;
>  	unsigned long long bSum;
> @@ -82,7 +81,7 @@ private:
>  	uint32_t estimateCCT(double red, double green, double blue);
>  
>  	std::vector<RGB> zones_;
> -	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
> +	Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>  	AwbStatus asyncResults_;
>  };
>  

-- 
Regards,

Laurent Pinchart