[libcamera-devel,v3,03/14] ipa: ipu3: awb: Use saturation under 90%
diff mbox series

Message ID 20211021164401.110033-4-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: ipu3: Fix AGC bugs
Related show

Commit Message

Jean-Michel Hautbois Oct. 21, 2021, 4:43 p.m. UTC 
The AWB grey world algorithm tries to find a grey value and it can't do
it on over-exposed images. To exclude those, the saturation ratio is
used for each cell, and the cell is included only if this ratio is 0.

Now that we have changed the threshold, more cells may be considered as
partially saturated and excluded, preventing the algorithm from running
efficiently.

Change that behaviour, and consider 90% as a good enough ratio.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Oct. 21, 2021, 8:57 p.m. UTC | #1 
Quoting Jean-Michel Hautbois (2021-10-21 17:43:50)
> The AWB grey world algorithm tries to find a grey value and it can't do
> it on over-exposed images. To exclude those, the saturation ratio is
> used for each cell, and the cell is included only if this ratio is 0.
> 
> Now that we have changed the threshold, more cells may be considered as
> partially saturated and excluded, preventing the algorithm from running
> efficiently.
> 
> Change that behaviour, and consider 90% as a good enough ratio.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 4364928c..ce01791b 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -19,6 +19,18 @@ LOG_DEFINE_CATEGORY(IPU3Awb)
>  
>  static constexpr uint32_t kMinGreenLevelInZone = 32;
>  
> +/*
> + * Minimum proportion of non-saturated cells in a zone for the zone to be used
> + * by the AWB algorithm.
> + */
> +static constexpr double kMinRelevantCellsRatio = 0.8;
> +
> +/*
> + * Maximum ratio of saturated pixels in a cell for the cell to be considered
> + * non-saturated and counted by the AWB algorithm.
> + */
> +static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;
> +
>  /**
>   * \struct Accumulator
>   * \brief RGB statistics for a given zone
> @@ -160,7 +172,8 @@ int Awb::configure(IPAContext &context,
>          * for it to be relevant for the grey world algorithm.
>          * \todo This proportion could be configured.
>          */
> -       cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;
> +       cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;
> +       LOG(IPU3Awb, Debug) << "Threshold for AWB is set to " << cellsPerZoneThreshold_;
>  
>         return 0;
>  }
> @@ -234,7 +247,18 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>                                 reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>                                         &stats->awb_raw_buffer.meta_data[cellPosition]
>                                 );
> -                       if (currentCell->sat_ratio == 0) {
> +
> +                       /*
> +                        * Use cells which have less than 90%
> +                        * saturation as an initial means to include
> +                        * otherwise bright cells which are not fully
> +                        * saturated.
> +                        *
> +                        * \todo The 90% saturation rate may require
> +                        * further empirical measurements and
> +                        * optimisation during camera tuning phases.
> +                        */

I'm a little wary of the comments stating the value of the constants
which are declared elsewhere, (and the chance of them getting out of
sync), but I think this is fine for now. If we ever change this, we're
likely moving away from a constant, to some tuned/config file value so
we'll be forced to update everything anyway.


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

> +                       if (currentCell->sat_ratio <= kSaturationThreshold) {
>                                 /* The cell is not saturated, use the current cell */
>                                 awbStats_[awbZonePosition].counted++;
>                                 uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
> -- 
> 2.32.0
>
Laurent Pinchart Oct. 22, 2021, 3:59 a.m. UTC | #2 
Hi Jean-Michel,

Thank you for the patch.

On Thu, Oct 21, 2021 at 06:43:50PM +0200, Jean-Michel Hautbois wrote:
> The AWB grey world algorithm tries to find a grey value and it can't do
> it on over-exposed images. To exclude those, the saturation ratio is
> used for each cell, and the cell is included only if this ratio is 0.
> 
> Now that we have changed the threshold, more cells may be considered as
> partially saturated and excluded, preventing the algorithm from running
> efficiently.
> 
> Change that behaviour, and consider 90% as a good enough ratio.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 4364928c..ce01791b 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -19,6 +19,18 @@ LOG_DEFINE_CATEGORY(IPU3Awb)
>  
>  static constexpr uint32_t kMinGreenLevelInZone = 32;
>  
> +/*
> + * Minimum proportion of non-saturated cells in a zone for the zone to be used
> + * by the AWB algorithm.
> + */
> +static constexpr double kMinRelevantCellsRatio = 0.8;
> +
> +/*
> + * Maximum ratio of saturated pixels in a cell for the cell to be considered
> + * non-saturated and counted by the AWB algorithm.
> + */
> +static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;

In addition to the comments, I've also commented on constant names in
the review of v2. You're of course free to disagree with review
comments, but with no answer to the review, I can't know if you have
kept the previous names on purpose or just haven't noticed the proposal.

> +
>  /**
>   * \struct Accumulator
>   * \brief RGB statistics for a given zone
> @@ -160,7 +172,8 @@ int Awb::configure(IPAContext &context,
>  	 * for it to be relevant for the grey world algorithm.
>  	 * \todo This proportion could be configured.
>  	 */
> -	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;
> +	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;
> +	LOG(IPU3Awb, Debug) << "Threshold for AWB is set to " << cellsPerZoneThreshold_;
>  
>  	return 0;
>  }
> @@ -234,7 +247,18 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>  				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>  					&stats->awb_raw_buffer.meta_data[cellPosition]
>  				);
> -			if (currentCell->sat_ratio == 0) {
> +
> +			/*
> +			 * Use cells which have less than 90%
> +			 * saturation as an initial means to include
> +			 * otherwise bright cells which are not fully
> +			 * saturated.
> +			 *
> +			 * \todo The 90% saturation rate may require
> +			 * further empirical measurements and
> +			 * optimisation during camera tuning phases.
> +			 */
> +			if (currentCell->sat_ratio <= kSaturationThreshold) {
>  				/* The cell is not saturated, use the current cell */
>  				awbStats_[awbZonePosition].counted++;
>  				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
Jean-Michel Hautbois Oct. 22, 2021, 5:29 a.m. UTC | #3 
Hi Laurent,

On 22/10/2021 05:59, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Thu, Oct 21, 2021 at 06:43:50PM +0200, Jean-Michel Hautbois wrote:
>> The AWB grey world algorithm tries to find a grey value and it can't do
>> it on over-exposed images. To exclude those, the saturation ratio is
>> used for each cell, and the cell is included only if this ratio is 0.
>>
>> Now that we have changed the threshold, more cells may be considered as
>> partially saturated and excluded, preventing the algorithm from running
>> efficiently.
>>
>> Change that behaviour, and consider 90% as a good enough ratio.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>> index 4364928c..ce01791b 100644
>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>> @@ -19,6 +19,18 @@ LOG_DEFINE_CATEGORY(IPU3Awb)
>>   
>>   static constexpr uint32_t kMinGreenLevelInZone = 32;
>>   
>> +/*
>> + * Minimum proportion of non-saturated cells in a zone for the zone to be used
>> + * by the AWB algorithm.
>> + */
>> +static constexpr double kMinRelevantCellsRatio = 0.8;
>> +
>> +/*
>> + * Maximum ratio of saturated pixels in a cell for the cell to be considered
>> + * non-saturated and counted by the AWB algorithm.
>> + */
>> +static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;
> 
> In addition to the comments, I've also commented on constant names in
> the review of v2. You're of course free to disagree with review
> comments, but with no answer to the review, I can't know if you have
> kept the previous names on purpose or just haven't noticed the proposal.
> 

Oh, yes, you are right, I got the comments, but did not read your 
comment properly. Fixed in v4, thanks and sorry :-/.

>> +
>>   /**
>>    * \struct Accumulator
>>    * \brief RGB statistics for a given zone
>> @@ -160,7 +172,8 @@ int Awb::configure(IPAContext &context,
>>   	 * for it to be relevant for the grey world algorithm.
>>   	 * \todo This proportion could be configured.
>>   	 */
>> -	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;
>> +	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;
>> +	LOG(IPU3Awb, Debug) << "Threshold for AWB is set to " << cellsPerZoneThreshold_;
>>   
>>   	return 0;
>>   }
>> @@ -234,7 +247,18 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>   				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>>   					&stats->awb_raw_buffer.meta_data[cellPosition]
>>   				);
>> -			if (currentCell->sat_ratio == 0) {
>> +
>> +			/*
>> +			 * Use cells which have less than 90%
>> +			 * saturation as an initial means to include
>> +			 * otherwise bright cells which are not fully
>> +			 * saturated.
>> +			 *
>> +			 * \todo The 90% saturation rate may require
>> +			 * further empirical measurements and
>> +			 * optimisation during camera tuning phases.
>> +			 */
>> +			if (currentCell->sat_ratio <= kSaturationThreshold) {
>>   				/* The cell is not saturated, use the current cell */
>>   				awbStats_[awbZonePosition].counted++;
>>   				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
index 4364928c..ce01791b 100644
--- a/src/ipa/ipu3/algorithms/awb.cpp
+++ b/src/ipa/ipu3/algorithms/awb.cpp
@@ -19,6 +19,18 @@  LOG_DEFINE_CATEGORY(IPU3Awb)
 
 static constexpr uint32_t kMinGreenLevelInZone = 32;
 
+/*
+ * Minimum proportion of non-saturated cells in a zone for the zone to be used
+ * by the AWB algorithm.
+ */
+static constexpr double kMinRelevantCellsRatio = 0.8;
+
+/*
+ * Maximum ratio of saturated pixels in a cell for the cell to be considered
+ * non-saturated and counted by the AWB algorithm.
+ */
+static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;
+
 /**
  * \struct Accumulator
  * \brief RGB statistics for a given zone
@@ -160,7 +172,8 @@  int Awb::configure(IPAContext &context,
 	 * for it to be relevant for the grey world algorithm.
 	 * \todo This proportion could be configured.
 	 */
-	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;
+	cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;
+	LOG(IPU3Awb, Debug) << "Threshold for AWB is set to " << cellsPerZoneThreshold_;
 
 	return 0;
 }
@@ -234,7 +247,18 @@  void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
 				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
 					&stats->awb_raw_buffer.meta_data[cellPosition]
 				);
-			if (currentCell->sat_ratio == 0) {
+
+			/*
+			 * Use cells which have less than 90%
+			 * saturation as an initial means to include
+			 * otherwise bright cells which are not fully
+			 * saturated.
+			 *
+			 * \todo The 90% saturation rate may require
+			 * further empirical measurements and
+			 * optimisation during camera tuning phases.
+			 */
+			if (currentCell->sat_ratio <= kSaturationThreshold) {
 				/* The cell is not saturated, use the current cell */
 				awbStats_[awbZonePosition].counted++;
 				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;