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

[Laurent Pinchart]

On Wed, Mar 26, 2025 at 01:50:18PM +0100, Stefan Klug wrote:
> On Wed, Mar 26, 2025 at 02:24:39PM +0200, Laurent Pinchart wrote:
> > 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.

Hmmm... so your point is that this patch will help recovering from an
effectiveExposureValue occasionally being 0 ? I was thinking of a case
where it would be stuck at 0 constantly. I agree recovering from
transient bugs in the caller is a good idea. A comment to explain this
in the code would be useful.

> > > 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.
> 
> > > +		return exposureModeHelper->splitExposure(10ms);
> > > +	}
> > > +
> > >  	double gain = estimateInitialGain();
> > >  	gain = constraintClampGain(constraintModeIndex, yHist, gain);
> > >  

-- 
Regards,

Laurent Pinchart