[libcamera-devel,0/4] Move and improve AWB structures
mbox series

Message ID 20210902083207.33976-1-jeanmichel.hautbois@ideasonboard.com
Headers show
Series
  • Move and improve AWB structures
Related show

Message

Jean-Michel Hautbois Sept. 2, 2021, 8:32 a.m. UTC 
This is a preliminary series to introduce improvements in the AGC
algorithm.

Patch 1/4 moves the AWB structures to be able to use those from the
ipa::ipu3::algorithms namespace (by AGC at least).

Patch 2/4 renames the stats region structure to make it clear it is an
accumulator structure.

Patch 3/4 and 4/4 are improving the Accumulator structure to have the
same layout as the IPAFrameContext::awb structure. 

Jean-Michel Hautbois (4):
  ipa: ipu3: Move the AWB stats structures
  ipa: ipu3: Rename IspStatsRegion to Accumulator
  ipa: ipu3: Replace Accumulator::uncounted to Accumulator::total
  ipa: ipu3: Change Accumulator structure layout

 src/ipa/ipu3/algorithms/awb.cpp | 58 ++++++++++++++++++++-------------
 src/ipa/ipu3/algorithms/awb.h   | 41 ++++++++++++-----------
 2 files changed, 57 insertions(+), 42 deletions(-)

Comments

Kieran Bingham Sept. 2, 2021, 12:27 p.m. UTC | #1 
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@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_;
>  };
>  
>
Kieran Bingham Sept. 2, 2021, 12:33 p.m. UTC | #2 
On 02/09/2021 09:32, Jean-Michel Hautbois wrote:
> Replace uncounted by total as it is clearer on what it contains. It is
> always possible to know how many pixels are not relevant for the
> algorithm by calculating total-counted.

Was uncounted not set before? I expected to see code removing references
to setting the uncounted value.

If it was previously unused (other than initialisation) add that
information to the commit message.


> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 10 +++++++---
>  src/ipa/ipu3/algorithms/awb.h   |  2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 8cfbb23d..9b2cbba7 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -38,8 +38,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>   * \var Accumulator::counted
>   * \brief Number of relevant regions used to calculate the sums
>   *
> - * \var Accumulator::uncounted
> - * \brief Remaining number of regions in the zone
> + * \var Accumulator::total
> + * \brief Total number of regions in the zone
>   *
>   * \var Accumulator::rSum
>   * \brief Sum of the red values in the zone
> @@ -189,6 +189,10 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>  	uint32_t regionWidth = round(grid.width / static_cast<double>(kAwbStatsSizeX));
>  	uint32_t regionHeight = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>  
> +	for (unsigned int i = 0; i < kAwbStatsSizeX; i++)
> +		for (unsigned int j = 0; j < kAwbStatsSizeY; j++)
> +			awbStats_[j * kAwbStatsSizeX + i].total = regionWidth * regionHeight;

If all (zones?) have the same total number of regions, do we even need
to set it in the structure?

Perhaps if uncounted/total are unused, it could simply be dropped?

> +
>  	/*
>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>  	 * (grid.width x grid.height).
> @@ -223,7 +227,7 @@ void Awb::clearAwbStats()
>  		awbStats_[i].rSum = 0;
>  		awbStats_[i].gSum = 0;
>  		awbStats_[i].counted = 0;
> -		awbStats_[i].uncounted = 0;
> +		awbStats_[i].total = 0;
>  	}
>  }
>  
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index 6ae934fc..13490b07 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -35,7 +35,7 @@ struct Ipu3AwbCell {
>  
>  struct Accumulator {
>  	unsigned int counted;
> -	unsigned int uncounted;
> +	unsigned int total;
>  	unsigned long long rSum;
>  	unsigned long long gSum;
>  	unsigned long long bSum;
>
Jean-Michel Hautbois Sept. 2, 2021, 12:52 p.m. UTC | #3 
On 02/09/2021 14:33, Kieran Bingham wrote:
> On 02/09/2021 09:32, Jean-Michel Hautbois wrote:
>> Replace uncounted by total as it is clearer on what it contains. It is
>> always possible to know how many pixels are not relevant for the
>> algorithm by calculating total-counted.
> 
> Was uncounted not set before? I expected to see code removing references
> to setting the uncounted value.
> 
> If it was previously unused (other than initialisation) add that
> information to the commit message.
> 

It was not used and won't be used in a near future either :-).

> 
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>  src/ipa/ipu3/algorithms/awb.cpp | 10 +++++++---
>>  src/ipa/ipu3/algorithms/awb.h   |  2 +-
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>> index 8cfbb23d..9b2cbba7 100644
>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>> @@ -38,8 +38,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>>   * \var Accumulator::counted
>>   * \brief Number of relevant regions used to calculate the sums
>>   *
>> - * \var Accumulator::uncounted
>> - * \brief Remaining number of regions in the zone
>> + * \var Accumulator::total
>> + * \brief Total number of regions in the zone
>>   *
>>   * \var Accumulator::rSum
>>   * \brief Sum of the red values in the zone
>> @@ -189,6 +189,10 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>>  	uint32_t regionWidth = round(grid.width / static_cast<double>(kAwbStatsSizeX));
>>  	uint32_t regionHeight = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>>  
>> +	for (unsigned int i = 0; i < kAwbStatsSizeX; i++)
>> +		for (unsigned int j = 0; j < kAwbStatsSizeY; j++)
>> +			awbStats_[j * kAwbStatsSizeX + i].total = regionWidth * regionHeight;
> 
> If all (zones?) have the same total number of regions, do we even need
> to set it in the structure?
> 
> Perhaps if uncounted/total are unused, it could simply be dropped?
> 
>> +
>>  	/*
>>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>>  	 * (grid.width x grid.height).
>> @@ -223,7 +227,7 @@ void Awb::clearAwbStats()
>>  		awbStats_[i].rSum = 0;
>>  		awbStats_[i].gSum = 0;
>>  		awbStats_[i].counted = 0;
>> -		awbStats_[i].uncounted = 0;
>> +		awbStats_[i].total = 0;
>>  	}
>>  }
>>  
>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
>> index 6ae934fc..13490b07 100644
>> --- a/src/ipa/ipu3/algorithms/awb.h
>> +++ b/src/ipa/ipu3/algorithms/awb.h
>> @@ -35,7 +35,7 @@ struct Ipu3AwbCell {
>>  
>>  struct Accumulator {
>>  	unsigned int counted;
>> -	unsigned int uncounted;
>> +	unsigned int total;
>>  	unsigned long long rSum;
>>  	unsigned long long gSum;
>>  	unsigned long long bSum;
>>
Kieran Bingham Sept. 2, 2021, 12:59 p.m. UTC | #4 
On 02/09/2021 13:52, Jean-Michel Hautbois wrote:
> On 02/09/2021 14:33, Kieran Bingham wrote:
>> On 02/09/2021 09:32, Jean-Michel Hautbois wrote:
>>> Replace uncounted by total as it is clearer on what it contains. It is
>>> always possible to know how many pixels are not relevant for the
>>> algorithm by calculating total-counted.
>>
>> Was uncounted not set before? I expected to see code removing references
>> to setting the uncounted value.
>>
>> If it was previously unused (other than initialisation) add that
>> information to the commit message.
>>
> 
> It was not used and won't be used in a near future either :-).

Ok - maybe lets remove it instead then.


>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>> ---
>>>  src/ipa/ipu3/algorithms/awb.cpp | 10 +++++++---
>>>  src/ipa/ipu3/algorithms/awb.h   |  2 +-
>>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>>> index 8cfbb23d..9b2cbba7 100644
>>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>>> @@ -38,8 +38,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>>>   * \var Accumulator::counted
>>>   * \brief Number of relevant regions used to calculate the sums
>>>   *
>>> - * \var Accumulator::uncounted
>>> - * \brief Remaining number of regions in the zone
>>> + * \var Accumulator::total
>>> + * \brief Total number of regions in the zone
>>>   *
>>>   * \var Accumulator::rSum
>>>   * \brief Sum of the red values in the zone
>>> @@ -189,6 +189,10 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>>>  	uint32_t regionWidth = round(grid.width / static_cast<double>(kAwbStatsSizeX));
>>>  	uint32_t regionHeight = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>>>  
>>> +	for (unsigned int i = 0; i < kAwbStatsSizeX; i++)
>>> +		for (unsigned int j = 0; j < kAwbStatsSizeY; j++)
>>> +			awbStats_[j * kAwbStatsSizeX + i].total = regionWidth * regionHeight;
>>
>> If all (zones?) have the same total number of regions, do we even need
>> to set it in the structure?
>>
>> Perhaps if uncounted/total are unused, it could simply be dropped?
>>
>>> +
>>>  	/*
>>>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>>>  	 * (grid.width x grid.height).
>>> @@ -223,7 +227,7 @@ void Awb::clearAwbStats()
>>>  		awbStats_[i].rSum = 0;
>>>  		awbStats_[i].gSum = 0;
>>>  		awbStats_[i].counted = 0;
>>> -		awbStats_[i].uncounted = 0;
>>> +		awbStats_[i].total = 0;
>>>  	}
>>>  }
>>>  
>>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
>>> index 6ae934fc..13490b07 100644
>>> --- a/src/ipa/ipu3/algorithms/awb.h
>>> +++ b/src/ipa/ipu3/algorithms/awb.h
>>> @@ -35,7 +35,7 @@ struct Ipu3AwbCell {
>>>  
>>>  struct Accumulator {
>>>  	unsigned int counted;
>>> -	unsigned int uncounted;
>>> +	unsigned int total;
>>>  	unsigned long long rSum;
>>>  	unsigned long long gSum;
>>>  	unsigned long long bSum;
>>>