[libcamera-devel,v5,04/14] ipa: ipu3: agc: Limit the number of saturated cells
diff mbox series

Message ID 20211113084947.21892-5-jeanmichel.hautbois@ideasonboard.com
State Accepted
Headers show
Series
  • IPA: IPU3: Introduce per-frame controls
Related show

Commit Message

Jean-Michel Hautbois Nov. 13, 2021, 8:49 a.m. UTC 
When the histogram is calculated, we check if a cell is saturated or not
before cumulating its green value. This is wrong, and it can lead to an
empty histogram in case of a fully saturated frame.

Use a constant to limit the amount of pixels within a cell before
considering it saturated. If at the end of the loop we still have an
empty histogram, then make it a fully saturated one.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=84
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Nov. 15, 2021, 3:39 p.m. UTC | #1 
Hi Jean-Michel,

Thank you for the patch.

On Sat, Nov 13, 2021 at 09:49:37AM +0100, Jean-Michel Hautbois wrote:
> When the histogram is calculated, we check if a cell is saturated or not
> before cumulating its green value. This is wrong, and it can lead to an
> empty histogram in case of a fully saturated frame.
> 
> Use a constant to limit the amount of pixels within a cell before
> considering it saturated. If at the end of the loop we still have an
> empty histogram, then make it a fully saturated one.

Do we need this threshold ? For AWB handling saturation is important,
but for AGC, what will happen if we compute the histogram using all
cells regardless of saturation ?

> Bug: https://bugs.libcamera.org/show_bug.cgi?id=84
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 5723f6f3..83aa3676 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -63,6 +63,12 @@ static constexpr uint32_t knumHistogramBins = 256;
>  /* Target value to reach for the top 2% of the histogram */
>  static constexpr double kEvGainTarget = 0.5;
>  
> +/*
> + * Maximum ratio of saturated pixels in a cell for the cell to be considered
> + * non-saturated and counted by the AGC algorithm.
> + */
> +static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
> +
>  Agc::Agc()
>  	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>  	  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
> @@ -124,7 +130,7 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>  					&stats->awb_raw_buffer.meta_data[cellPosition]
>  				);
>  
> -			if (cell->sat_ratio == 0) {
> +			if (cell->sat_ratio <= kMinCellsPerZoneRatio) {
>  				uint8_t gr = cell->Gr_avg;
>  				uint8_t gb = cell->Gb_avg;
>  				/*
> @@ -137,8 +143,14 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>  		}
>  	}
>  
> +	Histogram cumulativeHist = Histogram(Span<uint32_t>(hist));
>  	/* Estimate the quantile mean of the top 2% of the histogram */
> -	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
> +	if (cumulativeHist.total() == 0) {
> +		/* Force the value as histogram is empty */
> +		iqMean_ = knumHistogramBins - 0.5;
> +	} else {
> +		iqMean_ = cumulativeHist.interQuantileMean(0.98, 1.0);
> +	}
>  }
>  
>  /**
Jean-Michel Hautbois Nov. 16, 2021, 7:54 a.m. UTC | #2 
Hi Laurent,

On 15/11/2021 16:39, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Sat, Nov 13, 2021 at 09:49:37AM +0100, Jean-Michel Hautbois wrote:
>> When the histogram is calculated, we check if a cell is saturated or not
>> before cumulating its green value. This is wrong, and it can lead to an
>> empty histogram in case of a fully saturated frame.
>>
>> Use a constant to limit the amount of pixels within a cell before
>> considering it saturated. If at the end of the loop we still have an
>> empty histogram, then make it a fully saturated one.
> 
> Do we need this threshold ? For AWB handling saturation is important,
> but for AGC, what will happen if we compute the histogram using all
> cells regardless of saturation ?

I think we need this until the "ipa: ipu3: agc: Improve gain 
calculation" is applied. So, on master, I think we can probably remove 
it now.

> 
>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=84
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 5723f6f3..83aa3676 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -63,6 +63,12 @@ static constexpr uint32_t knumHistogramBins = 256;
>>   /* Target value to reach for the top 2% of the histogram */
>>   static constexpr double kEvGainTarget = 0.5;
>>   
>> +/*
>> + * Maximum ratio of saturated pixels in a cell for the cell to be considered
>> + * non-saturated and counted by the AGC algorithm.
>> + */
>> +static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
>> +
>>   Agc::Agc()
>>   	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>>   	  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
>> @@ -124,7 +130,7 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>>   					&stats->awb_raw_buffer.meta_data[cellPosition]
>>   				);
>>   
>> -			if (cell->sat_ratio == 0) {
>> +			if (cell->sat_ratio <= kMinCellsPerZoneRatio) {
>>   				uint8_t gr = cell->Gr_avg;
>>   				uint8_t gb = cell->Gb_avg;
>>   				/*
>> @@ -137,8 +143,14 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>>   		}
>>   	}
>>   
>> +	Histogram cumulativeHist = Histogram(Span<uint32_t>(hist));
>>   	/* Estimate the quantile mean of the top 2% of the histogram */
>> -	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>> +	if (cumulativeHist.total() == 0) {
>> +		/* Force the value as histogram is empty */
>> +		iqMean_ = knumHistogramBins - 0.5;
>> +	} else {
>> +		iqMean_ = cumulativeHist.interQuantileMean(0.98, 1.0);
>> +	}
>>   }
>>   
>>   /**
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 5723f6f3..83aa3676 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -63,6 +63,12 @@  static constexpr uint32_t knumHistogramBins = 256;
 /* Target value to reach for the top 2% of the histogram */
 static constexpr double kEvGainTarget = 0.5;
 
+/*
+ * Maximum ratio of saturated pixels in a cell for the cell to be considered
+ * non-saturated and counted by the AGC algorithm.
+ */
+static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
+
 Agc::Agc()
 	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
 	  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
@@ -124,7 +130,7 @@  void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
 					&stats->awb_raw_buffer.meta_data[cellPosition]
 				);
 
-			if (cell->sat_ratio == 0) {
+			if (cell->sat_ratio <= kMinCellsPerZoneRatio) {
 				uint8_t gr = cell->Gr_avg;
 				uint8_t gb = cell->Gb_avg;
 				/*
@@ -137,8 +143,14 @@  void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
 		}
 	}
 
+	Histogram cumulativeHist = Histogram(Span<uint32_t>(hist));
 	/* Estimate the quantile mean of the top 2% of the histogram */
-	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
+	if (cumulativeHist.total() == 0) {
+		/* Force the value as histogram is empty */
+		iqMean_ = knumHistogramBins - 0.5;
+	} else {
+		iqMean_ = cumulativeHist.interQuantileMean(0.98, 1.0);
+	}
 }
 
 /**