[PATCH 2/3] ipa: libipa: agc_mean_luminance: Error out when effectiveExposureValue is zero

[Stefan Klug]

Hi Laurent,

Thank you for the review. 

On Wed, Mar 26, 2025 at 02:24:39PM +0200, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Fri, Feb 28, 2025 at 01:55:54PM +0100, Stefan Klug wrote:
> > In a proper system it never happens that the effectiveExposureValue
> > drops to zero. If that still happens due to a bug outside of
> > agc_mean_luminance, the calculated gain goes towards infinity but the
> > newExposureValue is still 0 because it is the result of multiplying the
> > effectiveExposureTime with the gain, leading to wild oscillations.
> > 
> > Catch that condition, print an error message and set the new effective
> > exposure value to an arbitrary 10ms.
> > 
> > Note that in any case the underlying problem must be fixed. The
> > important change is the added error message to be able to detect such a
> > situation.
> 
> I'm tempted to then return all 0's. If this indicates a bug somewhere
> else, the bug should be fixed, is there a value in working around it ?

The idea was that an exposure time of 0 is in all cases a bad thing and
the risk is higher to end up stuck somewhere in the regulation.

> 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/ipa/libipa/agc_mean_luminance.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 02555a44d271..a7343c18f5aa 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -541,6 +541,13 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
> >  	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
> >  		exposureModeHelpers_.at(exposureModeIndex);
> >  
> > +	if (effectiveExposureValue == 0s) {
> > +		LOG(AgcMeanLuminance, Error) << "Effective exposure value is 0."
> > +					     << " Resetting exposure time and"
> > +					     << " gain to arbitrary defaults.";
> 
> You can write
> 
> 		LOG(AgcMeanLuminance, Error)
> 			<< "Effective exposure value is 0. Resetting exposure "
> 			   "time and gain to arbitrary defaults.";
> 
> which will split the string on the lines, but won't call the << operator
> multiple times.
> 
> I'd focus the error message on reporting the underlying problem. As it
> is, I don't think it would be clear for users what to do.
> 
> 		LOG(AgcMeanLuminance, Error)
> 			<< "Effective exposure value is 0. This is a bug in AGC "
> 			   "and must be fixed for proper operation."
> 
> or something like that ?

That makes things clearer. I'll change that.

Thanks,
Stefan

> 
> > +		return exposureModeHelper->splitExposure(10ms);
> > +	}
> > +
> >  	double gain = estimateInitialGain();
> >  	gain = constraintClampGain(constraintModeIndex, yHist, gain);
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart