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

[Laurent Pinchart]

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 ?

> 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.

>  /* 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 ?

>  
>  	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).

>  		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_;

-- 
Regards,

Laurent Pinchart