[2/3] ipa: libipa: agc_mean_luminance: Error out when effectiveExposureValue is zero
diff mbox series

Message ID 20250228125600.3241397-3-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • rkisp1: Reduce oscillations on startup
Related show

Commit Message

Stefan Klug Feb. 28, 2025, 12:55 p.m. UTC
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(+)

Comments

Dan Scally Feb. 28, 2025, 1 p.m. UTC | #1
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);
>
Isaac Scott Feb. 28, 2025, 2:28 p.m. UTC | #2
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);
>
Stefan Klug March 5, 2025, 9:57 a.m. UTC | #3
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);
> >  
>

Patch
diff mbox series

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