[libcamera-devel] [PATCH v5 2/4] ipa: ipu3: Rename IspStatsRegion to Accumulator

[Kieran Bingham]

On 09/09/2021 09:25, 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 region.  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>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 36 ++++++++++++++++++++-------------
>  src/ipa/ipu3/algorithms/awb.h   |  5 ++---
>  2 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index e05647c9..97f5839f 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -21,26 +21,35 @@ static constexpr uint32_t kMinZonesCounted = 16;
>  static constexpr uint32_t kMinGreenLevelInZone = 32;
>  
>  /**
> - * \struct IspStatsRegion
> + * \struct Accumulator
>   * \brief RGB statistics for a given region
>   *
> - * The IspStatsRegion structure is intended to abstract the ISP specific
> - * statistics and use an agnostic algorithm to compute AWB.
> + * A frame is divided into zones, each zone is made of multiple regions which
> + * are defined by the grid configuration. The algorithm works with a fixed
> + * number of zones \a kAwbStatsSizeX x \a kAwbStatsSizeY.
> + * For example, a frame of 1280x720 is divided into 81x45 regions of [16x16]
> + * pixels because the BDS output size will be calculated as 1296x720. In the
> + * case of \a kAwbStatsSizeX=16 and \a kAwbStatsSizeY=12 the zones are made of
> + * [5x4] regions. The regions are left-aligned and calculated by
> + * IPAIPU3::calculateBdsGrid().
>   *
> - * \var IspStatsRegion::counted
> - * \brief Number of pixels used to calculate the sums
> + * The Accumulator structure stores the sum of the pixel values in a zone of
> + * the image, as well as the number of relevant regions in this same zone. A
> + * relevant region is an unsaturated region here, depending on the awb
> + * thresholds.
> + * \todo extend the notion of relevant to something else ?

What else could it be?

You've defined relevant as "unsaturated regions" so perhaps we could
just use that ?

"""
* The Accumulator structure stores the sum of the pixel values in a zone of
* the image, as well as the number of unsaturated regions in this same zone.
*
* Zones are determined to be saturated if their value exceeds the
* threshold given by the thresholds in
*     ipu3_uapi_awb_config_s.rgbs_thr_{gr,r,gb,b}.
*
* \todo It is not fully clear from the IPU3 documentation what defines a
* saturated zone, This should be investigated further.
"""

<fix anything above as necessary, I do not assert correctness above>


>   *
> - * \var IspStatsRegion::uncounted
> - * \brief Remaining number of pixels in the region
> + * \var Accumulator::counted
> + * \brief Number of relevant regions used to calculate the sums

s/relevant/unsaturated/

With those:

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

>   *
> - * \var IspStatsRegion::rSum
> - * \brief Sum of the red values in the region
> + * \var Accumulator::rSum
> + * \brief Sum of the red values in the zone
>   *
> - * \var IspStatsRegion::gSum
> - * \brief Sum of the green values in the region
> + * \var Accumulator::gSum
> + * \brief Sum of the green values in the zone
>   *
> - * \var IspStatsRegion::bSum
> - * \brief Sum of the blue values in the region
> + * \var Accumulator::bSum
> + * \brief Sum of the blue values in the zone
>   */
>  
>  /**
> @@ -215,7 +224,6 @@ void Awb::clearAwbStats()
>  		awbStats_[i].rSum = 0;
>  		awbStats_[i].gSum = 0;
>  		awbStats_[i].counted = 0;
> -		awbStats_[i].uncounted = 0;
>  	}
>  }
>  
> 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_;
>  };
>  
>