| Message ID | 20250228125600.3241397-3-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On 28/02/2025 12:55, 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. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Reviewed-by: Daniel Scally <dan.scally@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."; > + return exposureModeHelper->splitExposure(10ms); > + } > + > double gain = estimateInitialGain(); > gain = constraintClampGain(constraintModeIndex, yHist, gain); >
Hi Stefan, On Fri, 2025-02-28 at 13:55 +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 it never happens that > the effectiveExposureValue drops to zero. If that still happens due > to a bug outside of What conditions lead to this being zero when it shouldn't? If they can be zero, this looks good to me! > + > double gain = estimateInitialGain(); > gain = constraintClampGain(constraintModeIndex, yHist, > gain); >
Hi Isaac, Thank you for the review. On Fri, Feb 28, 2025 at 02:28:19PM +0000, Isaac Scott wrote: > Hi Stefan, > > On Fri, 2025-02-28 at 13:55 +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 it never happens that > > the effectiveExposureValue drops to zero. If that still happens due > > to a bug outside of > > What conditions lead to this being zero when it shouldn't? One condition of this kind gets fixed in the next patch. In that case the sensor was set to an exposure time of 0 (the root cause). Now a bit later statistics for that frame come in and as the frame context contains an exposure time of 0 the effectiveExposureValue is also 0, breaking all calculations. > > If they can be zero, this looks good to me! Great :-). Would you mind giving it a reviewed-by? Best regards, Stefan > > > + > > double gain = estimateInitialGain(); > > gain = constraintClampGain(constraintModeIndex, yHist, > > gain); > > >
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 ? > Signed-off-by: Stefan Klug <stefan.klug@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 ? > + return exposureModeHelper->splitExposure(10ms); > + } > + > double gain = estimateInitialGain(); > gain = constraintClampGain(constraintModeIndex, yHist, gain); >
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@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
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@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); > > >
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."; + return exposureModeHelper->splitExposure(10ms); + } + double gain = estimateInitialGain(); gain = constraintClampGain(constraintModeIndex, yHist, gain);
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. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/libipa/agc_mean_luminance.cpp | 7 +++++++ 1 file changed, 7 insertions(+)