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

14544 diff mbox series

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@redhat.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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

Hi Jean-Michel,

On Thu, Nov 11, 2021 at 4:25 PM Jean-Michel Hautbois
<jeanmichel.hautbois@ideasonboard.com> wrote:
>
> 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@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".

Thank you for noticing me this :)

We are working on the same issue.

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