[libcamera-devel] [PATCH 06/13] ipa: ipu3: agc: Change exposure limits

Jean-Michel Hautbois jeanmichel.hautbois at ideasonboard.com
Fri Oct 15 07:49:45 CEST 2021


Hi Laurent,

On 15/10/2021 02:34, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> (CC'ing David for a question below)
> 
> Thank you for the patch.
> 
> On Wed, Oct 13, 2021 at 05:41:18PM +0200, Jean-Michel Hautbois wrote:
>> The exposure limits are set for one sensor until now, in a number of
>> lines. We should not use those, but exposure limits in a time unit.
>>
>> Introduce default limits which with a default minimum of 20ms. This
> 
> That's a default maximum, right ?
> 
> Isn't 20ms a bit low ?
> 

It is not meant to stay like that, the value should come from IPAIPU3
based on what the sensor can do, maybe with a local maximum of the
maximum if we want (60ms sounds better). It also means we need to change
exposure and VBLANK in the setControls() call. For that, we need to pass
a full exposure time and not only the exposure in a number of lines.

>> value should be given by the IPAIPU3. Use cached values expressed as a
>> number of lines in the configure() call.
>>
>> Adapt the process() call accordingly.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> ---
>>  src/ipa/ipu3/algorithms/agc.cpp | 30 +++++++++++++++++-------------
>>  src/ipa/ipu3/algorithms/agc.h   |  3 ++-
>>  2 files changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 7c2d4201..2dd5c5c1 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -40,8 +40,8 @@ static constexpr uint32_t kMinGain = kMinISO / 100;
>>  static constexpr uint32_t kMaxGain = kMaxISO / 100;
>>  
>>  /* \todo use calculated value based on sensor */
>> -static constexpr uint32_t kMinExposure = 1;
>> -static constexpr uint32_t kMaxExposure = 1976;
>> +static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us;
>> +static constexpr libcamera::utils::Duration kMaxShutterSpeed = 20ms;
> 
> I'd like to rename shutter speed to integration time (in a separate
> patch). What do you think ? "exposure time" may also be a candidate, but
> it is often shortened to "exposure" which is then easily confused with
> the photographic exposure.
> 

I hesitated, between the three. I think "integration time" is digital
photography term, while shutter speed is more general, but maybe David
and others have a different feeling ?

>>  /* Histogram constants */
>>  static constexpr uint32_t knumHistogramBins = 256;
>> @@ -49,8 +49,8 @@ static constexpr double kEvGainTarget = 0.5;
>>  
>>  Agc::Agc()
>>  	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>> -	  maxExposureTime_(0s), filteredExposure_(0s), filteredExposureNoDg_(0s),
>> -	  currentExposure_(0s), currentExposureNoDg_(0s)
>> +	  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
>> +	  filteredExposureNoDg_(0s), currentExposure_(0s), currentExposureNoDg_(0s)
>>  {
>>  }
>>  
>> @@ -60,7 +60,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>>  
>>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>>  		      / configInfo.sensorInfo.pixelRate;
>> -	maxExposureTime_ = kMaxExposure * lineDuration_;
> 
> Removing the maxExposureTime_ member variable is fine as it's currently
> a constant, but that won't always be the case. Shouldn't we keep it to
> prepare for calculation of the maximum exposure time from the sensor's
> limits ?
> 
>> +
>> +	minExposureLines_ = kMinShutterSpeed / lineDuration_;
>> +	maxExposureLines_ = kMaxShutterSpeed / lineDuration_;
> 
> I wonder if this goes in the right direction. Wouldn't it be better for
> the algorithm to operate on a time, with conversion to/from lines being
> handled outside (in ipu3.cpp, or possibly even in the pipeline handler)
> ? David, what's your opinion on this ?

Yes, we should do that. I could keep the previous maxExposureTime, and
use local variables in lockExposureGain() for the moment...

> 
>>  
>>  	return 0;
>>  }
>> @@ -140,28 +142,30 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>>  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>>  
>>  		/* extracted from Rpi::Agc::computeTargetExposure */
>> -		libcamera::utils::Duration currentShutter = exposure * lineDuration_;
>> +		Duration currentShutter = exposure * lineDuration_;
> 
> This compiles because there's a "using utils::Duration" in agc.h. This
> is harmful, I'll send a patch to drop it, so you will need
> utils::duration here (the libcamera:: prefix can be dropped as we're in
> the libcamera namespace).

OK, thanks.

> 
>>  		currentExposureNoDg_ = currentShutter * gain;
>>  		LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
>>  				    << " Shutter speed " << currentShutter
>>  				    << " Gain " << gain;
>> +
>>  		currentExposure_ = currentExposureNoDg_ * newGain;
>> -		libcamera::utils::Duration maxTotalExposure = maxExposureTime_ * kMaxGain;
>> +		Duration maxTotalExposure = kMaxShutterSpeed * kMaxGain;
>>  		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>> -		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_;
>> +		LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
>> +				    << " maximum is " << maxTotalExposure;
> 
> 				    << ", maximum is " << maxTotalExposure;
> 
>>  
>>  		/* \todo: estimate if we need to desaturate */
>>  		filterExposure();
>>  
>> -		libcamera::utils::Duration newExposure = 0.0s;
>> -		if (currentShutter < maxExposureTime_) {
>> -			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
>> +		Duration newExposure = 0.0s;
>> +		if (currentShutter < kMaxShutterSpeed) {
>> +			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_);
> 
> This is a very good occasion to wrap lines:
> 
> 			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_),
> 					      minExposureLines_, maxExposureLines_);
> 
>>  			newExposure = currentExposure_ / exposure;
>>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
>> -		} else if (currentShutter >= maxExposureTime_) {
>> +		} else if (currentShutter >= kMaxShutterSpeed) {
>>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
>>  			newExposure = currentExposure_ / gain;
>> -			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
>> +			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_);
> 
> Same here.
> 
>>  		}
>>  		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
>>  	}
>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
>> index cd4d4855..7605ab39 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -44,7 +44,8 @@ private:
>>  	double iqMean_;
>>  
>>  	Duration lineDuration_;
>> -	Duration maxExposureTime_;
>> +	uint32_t minExposureLines_;
>> +	uint32_t maxExposureLines_;
>>  
>>  	Duration filteredExposure_;
>>  	Duration filteredExposureNoDg_;
> 


More information about the libcamera-devel mailing list