[libcamera-devel] [PATCH 2/3] ipa: rpi: agc: Filter exposures before dealing with digital gain

[Jacopo Mondi]

Hi David

On Fri, Jul 28, 2023 at 02:36:59PM +0100, David Plowman via libcamera-devel wrote:
> We now time-filter the exposure before sorting out how much digital
> gain is required. This is actually a little more natural and
> simplifies the code.  It also prepares us for some future work where
> this arrangement will be helpful.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++-------------------------
>  src/ipa/rpi/controller/rpi/agc.h   |  2 +-
>  2 files changed, 6 insertions(+), 26 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index e8526355..6087fc60 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -469,14 +469,14 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
>  	computeGain(stats, imageMetadata, gain, targetY);
>  	/* Now compute the target (final) exposure which we think we want. */
>  	computeTargetExposure(gain);
> +	/* The results have to be filtered so as not to change too rapidly. */
> +	filterExposure();
>  	/*
>  	 * Some of the exposure has to be applied as digital gain, so work out
>  	 * what that is. This function also tells us whether it's decided to
>  	 * "desaturate" the image more quickly.
>  	 */
>  	bool desaturate = applyDigitalGain(gain, targetY);
> -	/* The results have to be filtered so as not to change too rapidly. */
> -	filterExposure(desaturate);
>  	/*
>  	 * The last thing is to divide up the exposure value into a shutter time
>  	 * and analogue gain, according to the current exposure mode.
> @@ -757,12 +757,12 @@ bool Agc::applyDigitalGain(double gain, double targetY)
>  	if (desaturate)
>  		dg /= config_.fastReduceThreshold;
>  	LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate;
> -	target_.totalExposureNoDG = target_.totalExposure / dg;
> -	LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << target_.totalExposureNoDG;
> +	filtered_.totalExposureNoDG = filtered_.totalExposure / dg;
> +	LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << filtered_.totalExposureNoDG;
>  	return desaturate;
>  }
>
> -void Agc::filterExposure(bool desaturate)
> +void Agc::filterExposure()
>  {
>  	double speed = config_.speed;
>  	/*
> @@ -774,7 +774,6 @@ void Agc::filterExposure(bool desaturate)
>  		speed = 1.0;
>  	if (!filtered_.totalExposure) {
>  		filtered_.totalExposure = target_.totalExposure;
> -		filtered_.totalExposureNoDG = target_.totalExposureNoDG;
>  	} else {
>  		/*
>  		 * If close to the result go faster, to save making so many
> @@ -785,26 +784,7 @@ void Agc::filterExposure(bool desaturate)
>  			speed = sqrt(speed);
>  		filtered_.totalExposure = speed * target_.totalExposure +
>  					  filtered_.totalExposure * (1.0 - speed);
> -		/*
> -		 * When desaturing, take a big jump down in totalExposureNoDG,
> -		 * which we'll hide with digital gain.
> -		 */
> -		if (desaturate)
> -			filtered_.totalExposureNoDG =
> -				target_.totalExposureNoDG;
> -		else
> -			filtered_.totalExposureNoDG =
> -				speed * target_.totalExposureNoDG +
> -				filtered_.totalExposureNoDG * (1.0 - speed);
>  	}
> -	/*
> -	 * We can't let the totalExposureNoDG exposure deviate too far below the
> -	 * total exposure, as there might not be enough digital gain available
> -	 * in the ISP to hide it (which will cause nasty oscillation).
> -	 */
> -	if (filtered_.totalExposureNoDG <
> -	    filtered_.totalExposure * config_.fastReduceThreshold)
> -		filtered_.totalExposureNoDG = filtered_.totalExposure * config_.fastReduceThreshold;

I won't pretend I've fully understood why this check can now be
dropped, but I the flow seems indeed more natural

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

>  	LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure
>  			   << " no dg " << filtered_.totalExposureNoDG;
>  }
> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> index 939f9729..b7122de3 100644
> --- a/src/ipa/rpi/controller/rpi/agc.h
> +++ b/src/ipa/rpi/controller/rpi/agc.h
> @@ -93,8 +93,8 @@ private:
>  	void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,
>  			 double &gain, double &targetY);
>  	void computeTargetExposure(double gain);
> +	void filterExposure();
>  	bool applyDigitalGain(double gain, double targetY);
> -	void filterExposure(bool desaturate);
>  	void divideUpExposure();
>  	void writeAndFinish(Metadata *imageMetadata, bool desaturate);
>  	libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);
> --
> 2.30.2
>