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

[Kieran Bingham]

On 02/09/2021 09:32, Jean-Michel Hautbois wrote:
> The structure name is not very explicit, rename it to accumulator, as it
> is accumulating the pixels in a given region, and amend the documentation.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 34 ++++++++++++++++++++-------------
>  src/ipa/ipu3/algorithms/awb.h   |  4 ++--
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index e05647c9..8cfbb23d 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -21,26 +21,34 @@ 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.
> + * 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.


I'm a bit confused with terminology here.


>   *
> - * \var IspStatsRegion::counted
> - * \brief Number of pixels used to calculate the sums
> + * A frame is divided into zones, each zone is made of multiple regions which

Maybe this should be at the beginning so the terms are defined before
they are used? That might have solved my confusion above on my first read.


> + * are defined by the grid configuration. The algorithm works with 16x12 zones.

It might not be wise to hardcode those zone sizes which are parameters
of the algorithm in the documentation of the structure.

What happens if they change, then this documentation needs to be updated
too.


> + * For example, a frame of 1296x720 is divided into 81x45 regions of [16x16]
> + * pixels. And the zones are made of [5x4] regions.

Though examples are helpful.

But, 5*16*16=1280, what happens to the other 16 horizontal pixels?
Are the regions / zones left aligned, centered? algorithm specific?

Would it help perhaps to have a dedicated documentation of the 'grid'
which is what I assume these zones, regions, pixels are all defined by,
... then the documentation for the Accumulator can focus on what it is
defining.


> + * \todo extend the notion of relevant to something else ?
>   *
> - * \var IspStatsRegion::uncounted
> - * \brief Remaining number of pixels in the region
> + * \var Accumulator::counted
> + * \brief Number of relevant regions used to calculate the sums
>   *
> - * \var IspStatsRegion::rSum
> - * \brief Sum of the red values in the region
> + * \var Accumulator::uncounted
> + * \brief Remaining number of regions in the zone
>   *
> - * \var IspStatsRegion::gSum
> - * \brief Sum of the green values in the region
> + * \var Accumulator::rSum
> + * \brief Sum of the red values in the zone
>   *
> - * \var IspStatsRegion::bSum
> - * \brief Sum of the blue values in the region
> + * \var Accumulator::gSum
> + * \brief Sum of the green values in the zone
> + *
> + * \var Accumulator::bSum
> + * \brief Sum of the blue values in the zone
>   */
>  
>  /**
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index cc848060..6ae934fc 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -33,7 +33,7 @@ struct Ipu3AwbCell {
>  	unsigned char padding[3];
>  } __attribute__((packed));
>  
> -struct IspStatsRegion {
> +struct Accumulator {
>  	unsigned int counted;
>  	unsigned int uncounted;
>  	unsigned long long rSum;
> @@ -82,7 +82,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_;
>  };
>  
>