[libcamera-devel] [PATCH] ipa: ipu3: agc: Fix -nan exception

[Jean-Michel Hautbois]

Hi Kate,

Thanks for the patch !

On 11/11/2021 09:10, Kate Hsuan wrote:
> The user may equip a cam cover to make sure security.If they want to
> disable the cam, the cam cover could be physically closed to ensure
> the image will not be delivered. In this situation, the image will be
> all black and it triggers the -nan exception for some values.
> Eventually, the exposure will be locked. As a result, we only can get
> a black image.
> 
> evGain is calculated by kEvGainTarget * knumHistogramBins / iqMean_.
> For a black image, a -nan of iqMean_ will lead evGain miss calculated.
> Consequently, the exposure value can not be correctly estimated and be
> locked at -nan as well. In this situation, evGain is set to 1.0 and try
> to estimate again through the next frame. Also, an additional check
> makes sure the exposure value is between minTotalExposure and
> maxTotalExposure.
> 
> Signed-off-by: Kate Hsuan <hpa at redhat.com>
> ---
>   src/ipa/ipu3/algorithms/agc.cpp | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index b5d736c1..d965febe 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -191,6 +191,8 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>   
>   	/* Estimate the gain needed to have the proportion wanted */
>   	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> +	if(std::isnan(evGain))
> +		evGain = 1.0;

This has been found on our side too :-) but we solved it in a different 
way, improving the AGC behaviour. Maybe would you like to have a look to 
the "[PATCH v2 00/14] IPA: IPU3: Introduce per-frame controls" series ?

To be honest, I hesitated between testing nan, patching the Histogram 
class and the third option you can find in "ipa: ipu3: agc: Limit the 
number of saturated cells".

>   
>   	/* extracted from Rpi::Agc::computeTargetExposure */
>   	/* Calculate the shutter time in seconds */
> @@ -210,7 +212,9 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>   
>   	/* Clamp the exposure value to the min and max authorized */
>   	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
> -	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> +	utils::Duration minTotalExposure = minShutterSpeed * minAnalogueGain_;
> +
> +	currentExposure_ = std::clamp<utils::Duration>(currentExposure_, minTotalExposure, maxTotalExposure);
>   	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
>   			    << ", maximum is " << maxTotalExposure;
>   
> @@ -232,7 +236,6 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>   	LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
>   			    << shutterTime << " and "
>   			    << stepGain;
> -
>   	exposure = shutterTime / lineDuration_;
>   	analogueGain = stepGain;
>   
>