[libcamera-devel,v2,13/13] ipa: ipu3: Use sensor limits for analogue gain
diff mbox series

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

Commit Message

Jean-Michel Hautbois Oct. 20, 2021, 3:46 p.m. UTC 
Instead of using constants for the analogue gains limits, use the
minimum and maximum from the configured sensor.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++------------
 src/ipa/ipu3/algorithms/agc.h   |  3 +++
 2 files changed, 16 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart Oct. 21, 2021, 2:39 a.m. UTC | #1 
Hi Jean-Michel,

Thank you for the patch.

On Wed, Oct 20, 2021 at 05:46:07PM +0200, Jean-Michel Hautbois wrote:
> Instead of using constants for the analogue gains limits, use the
> minimum and maximum from the configured sensor.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++------------
>  src/ipa/ipu3/algorithms/agc.h   |  3 +++
>  2 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index fc37eca4..690ad7e6 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -30,10 +30,9 @@ static constexpr uint32_t kInitialFrameMinAECount = 4;
>  /* Number of frames to wait between new gain/exposure estimations */
>  static constexpr uint32_t kFrameSkipCount = 6;
>  
> -/* Maximum analogue gain value
> - * \todo grab it from a camera helper */
> -static constexpr double kMinGain = 1.0;
> -static constexpr double kMaxGain = 8.0;
> +/* Limits for analogue gain values */
> +static constexpr double kMinAnalogueGain = 1.0;
> +static constexpr double kMaxAnalogueGain = 16.0;

Should we keep 8.0, or is there a reason to allow gains up to 16.0 ? It
seems like it will add lots of noise.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  /* Histogram constants */
>  static constexpr uint32_t knumHistogramBins = 256;
> @@ -53,15 +52,17 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>  		      / configInfo.sensorInfo.pixelRate;
>  
> -	/* Configure the default exposure and gain */
> -	context.frameContext.agc.gain =
> -		context.configuration.agc.minAnalogueGain;
> -	context.frameContext.agc.exposure = minExposureLines_;
> -
>  	/* \todo replace the exposure in lines storage with time based ones */
>  	minExposureLines_ = context.configuration.agc.minShutterSpeed / lineDuration_;
>  	maxExposureLines_ = context.configuration.agc.maxShutterSpeed / lineDuration_;
>  
> +	minAnalogueGain_ = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> +	maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
> +
> +	/* Configure the default exposure and gain */
> +	context.frameContext.agc.gain = minAnalogueGain_;
> +	context.frameContext.agc.exposure = minExposureLines_;
> +
>  	prevExposureValue_ = context.frameContext.agc.gain
>  			   * context.frameContext.agc.exposure
>  			   * lineDuration_;
> @@ -139,7 +140,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  	utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>  	utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>  
> -	utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
> +	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
>  	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>  	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
>  			    << ", maximum is " << maxTotalExposure;
> @@ -154,10 +155,10 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  	* Push the shutter time up to the maximum first, and only then
>  	* increase the gain.
>  	*/
> -	shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
> +	shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
>  						  minShutterSpeed, maxShutterSpeed);
>  	double stepGain = std::clamp(exposureValue / shutterTime,
> -				     kMinGain, kMaxGain);
> +				     minAnalogueGain_, maxAnalogueGain_);
>  	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
>  			    << shutterTime << " and "
>  			    << stepGain;
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index ad133b98..1840205b 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -45,6 +45,9 @@ private:
>  	uint32_t minExposureLines_;
>  	uint32_t maxExposureLines_;
>  
> +	double minAnalogueGain_;
> +	double maxAnalogueGain_;
> +
>  	utils::Duration filteredExposure_;
>  	utils::Duration currentExposure_;
>  	utils::Duration prevExposureValue_;
Jean-Michel Hautbois Oct. 21, 2021, 5:45 a.m. UTC | #2 
Hi Laurent,

On 21/10/2021 04:39, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Wed, Oct 20, 2021 at 05:46:07PM +0200, Jean-Michel Hautbois wrote:
>> Instead of using constants for the analogue gains limits, use the
>> minimum and maximum from the configured sensor.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++------------
>>   src/ipa/ipu3/algorithms/agc.h   |  3 +++
>>   2 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index fc37eca4..690ad7e6 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -30,10 +30,9 @@ static constexpr uint32_t kInitialFrameMinAECount = 4;
>>   /* Number of frames to wait between new gain/exposure estimations */
>>   static constexpr uint32_t kFrameSkipCount = 6;
>>   
>> -/* Maximum analogue gain value
>> - * \todo grab it from a camera helper */
>> -static constexpr double kMinGain = 1.0;
>> -static constexpr double kMaxGain = 8.0;
>> +/* Limits for analogue gain values */
>> +static constexpr double kMinAnalogueGain = 1.0;
>> +static constexpr double kMaxAnalogueGain = 16.0;
> 
> Should we keep 8.0, or is there a reason to allow gains up to 16.0 ? It
> seems like it will add lots of noise.

It is a clamp value, there is only one sensor I tested which can do more 
than 8.0 which is the ov5670 (I believe because of a broken driver). We 
could keep 8.0 indeed.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>   
>>   /* Histogram constants */
>>   static constexpr uint32_t knumHistogramBins = 256;
>> @@ -53,15 +52,17 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>>   	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>>   		      / configInfo.sensorInfo.pixelRate;
>>   
>> -	/* Configure the default exposure and gain */
>> -	context.frameContext.agc.gain =
>> -		context.configuration.agc.minAnalogueGain;
>> -	context.frameContext.agc.exposure = minExposureLines_;
>> -
>>   	/* \todo replace the exposure in lines storage with time based ones */
>>   	minExposureLines_ = context.configuration.agc.minShutterSpeed / lineDuration_;
>>   	maxExposureLines_ = context.configuration.agc.maxShutterSpeed / lineDuration_;
>>   
>> +	minAnalogueGain_ = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
>> +	maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
>> +
>> +	/* Configure the default exposure and gain */
>> +	context.frameContext.agc.gain = minAnalogueGain_;
>> +	context.frameContext.agc.exposure = minExposureLines_;
>> +
>>   	prevExposureValue_ = context.frameContext.agc.gain
>>   			   * context.frameContext.agc.exposure
>>   			   * lineDuration_;
>> @@ -139,7 +140,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>   	utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>>   	utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>>   
>> -	utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
>> +	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
>>   	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>>   	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
>>   			    << ", maximum is " << maxTotalExposure;
>> @@ -154,10 +155,10 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>   	* Push the shutter time up to the maximum first, and only then
>>   	* increase the gain.
>>   	*/
>> -	shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
>> +	shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
>>   						  minShutterSpeed, maxShutterSpeed);
>>   	double stepGain = std::clamp(exposureValue / shutterTime,
>> -				     kMinGain, kMaxGain);
>> +				     minAnalogueGain_, maxAnalogueGain_);
>>   	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
>>   			    << shutterTime << " and "
>>   			    << stepGain;
>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
>> index ad133b98..1840205b 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -45,6 +45,9 @@ private:
>>   	uint32_t minExposureLines_;
>>   	uint32_t maxExposureLines_;
>>   
>> +	double minAnalogueGain_;
>> +	double maxAnalogueGain_;
>> +
>>   	utils::Duration filteredExposure_;
>>   	utils::Duration currentExposure_;
>>   	utils::Duration prevExposureValue_;
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index fc37eca4..690ad7e6 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -30,10 +30,9 @@  static constexpr uint32_t kInitialFrameMinAECount = 4;
 /* Number of frames to wait between new gain/exposure estimations */
 static constexpr uint32_t kFrameSkipCount = 6;
 
-/* Maximum analogue gain value
- * \todo grab it from a camera helper */
-static constexpr double kMinGain = 1.0;
-static constexpr double kMaxGain = 8.0;
+/* Limits for analogue gain values */
+static constexpr double kMinAnalogueGain = 1.0;
+static constexpr double kMaxAnalogueGain = 16.0;
 
 /* Histogram constants */
 static constexpr uint32_t knumHistogramBins = 256;
@@ -53,15 +52,17 @@  int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
 	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
 		      / configInfo.sensorInfo.pixelRate;
 
-	/* Configure the default exposure and gain */
-	context.frameContext.agc.gain =
-		context.configuration.agc.minAnalogueGain;
-	context.frameContext.agc.exposure = minExposureLines_;
-
 	/* \todo replace the exposure in lines storage with time based ones */
 	minExposureLines_ = context.configuration.agc.minShutterSpeed / lineDuration_;
 	maxExposureLines_ = context.configuration.agc.maxShutterSpeed / lineDuration_;
 
+	minAnalogueGain_ = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
+	maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
+
+	/* Configure the default exposure and gain */
+	context.frameContext.agc.gain = minAnalogueGain_;
+	context.frameContext.agc.exposure = minExposureLines_;
+
 	prevExposureValue_ = context.frameContext.agc.gain
 			   * context.frameContext.agc.exposure
 			   * lineDuration_;
@@ -139,7 +140,7 @@  void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
 	utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
 	utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
 
-	utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
+	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
 	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
 	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
 			    << ", maximum is " << maxTotalExposure;
@@ -154,10 +155,10 @@  void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
 	* Push the shutter time up to the maximum first, and only then
 	* increase the gain.
 	*/
-	shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
+	shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
 						  minShutterSpeed, maxShutterSpeed);
 	double stepGain = std::clamp(exposureValue / shutterTime,
-				     kMinGain, kMaxGain);
+				     minAnalogueGain_, maxAnalogueGain_);
 	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
 			    << shutterTime << " and "
 			    << stepGain;
diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
index ad133b98..1840205b 100644
--- a/src/ipa/ipu3/algorithms/agc.h
+++ b/src/ipa/ipu3/algorithms/agc.h
@@ -45,6 +45,9 @@  private:
 	uint32_t minExposureLines_;
 	uint32_t maxExposureLines_;
 
+	double minAnalogueGain_;
+	double maxAnalogueGain_;
+
 	utils::Duration filteredExposure_;
 	utils::Duration currentExposure_;
 	utils::Duration prevExposureValue_;