[v4,6/7] ipa: simple: blc: Prevent division by zero in BLC
diff mbox series

Message ID 20250925192856.77881-7-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Fix stats related problems in software ISP
Related show

Commit Message

Milan Zamazal Sept. 25, 2025, 7:28 p.m. UTC 
When there are no values in the histogram, BLC simple IPA can crash on
division by zero.  We cannot get anything meaningful in such a case
anyway, let's simply return from `process()' then.

Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Hans de Goede Sept. 29, 2025, 11:35 a.m. UTC | #1 
Hi,

On 25-Sep-25 21:28, Milan Zamazal wrote:
> When there are no values in the histogram, BLC simple IPA can crash on
> division by zero.  We cannot get anything meaningful in such a case
> anyway, let's simply return from `process()' then.
> 
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 8c1e9ed08..ea5356443 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -77,6 +77,11 @@ void BlackLevel::process(IPAContext &context,
>  	constexpr float ignoredPercentage = 0.02;
>  	const unsigned int total =
>  		std::accumulate(begin(histogram), end(histogram), 0);
> +	if (total == 0) {
> +		LOG(IPASoftBL, Debug) << "Not guessing black level, histogram is empty";
> +		return;
> +	}
> +
t 
I don't think this can ever happen ?

What I've seen happening which triggers a divide by 0 in the AGC code is
all samples being in the highest bin of the histogram, because the first
frame of the sensor sometimes seems to have all 0x[3]ff as output, at least
for the top 3/4th of the frame or something like that.

Which causes a blacklevel of >= 244 which in turn causes a divide by 0
in the AGC code.

Regards,

Hans



>  	const unsigned int pixelThreshold = ignoredPercentage * total;
>  	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>  	const unsigned int currentBlackIdx =
Hans de Goede Sept. 29, 2025, 11:36 a.m. UTC | #2 
Hi,

On 25-Sep-25 21:28, Milan Zamazal wrote:
> When there are no values in the histogram, BLC simple IPA can crash on
> division by zero.  We cannot get anything meaningful in such a case
> anyway, let's simply return from `process()' then.
> 
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>

Forgot to mention that despite that this should never happen it still
is a good fix to have:

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans


> ---
>  src/ipa/simple/algorithms/blc.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 8c1e9ed08..ea5356443 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -77,6 +77,11 @@ void BlackLevel::process(IPAContext &context,
>  	constexpr float ignoredPercentage = 0.02;
>  	const unsigned int total =
>  		std::accumulate(begin(histogram), end(histogram), 0);
> +	if (total == 0) {
> +		LOG(IPASoftBL, Debug) << "Not guessing black level, histogram is empty";
> +		return;
> +	}
> +
>  	const unsigned int pixelThreshold = ignoredPercentage * total;
>  	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>  	const unsigned int currentBlackIdx =
Barnabás Pőcze Sept. 29, 2025, 11:40 a.m. UTC | #3 
Hi

2025. 09. 29. 13:35 keltezéssel, Hans de Goede írta:
> Hi,
> 
> On 25-Sep-25 21:28, Milan Zamazal wrote:
>> When there are no values in the histogram, BLC simple IPA can crash on
>> division by zero.  We cannot get anything meaningful in such a case
>> anyway, let's simply return from `process()' then.
>>
>> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   src/ipa/simple/algorithms/blc.cpp | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 8c1e9ed08..ea5356443 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -77,6 +77,11 @@ void BlackLevel::process(IPAContext &context,
>>   	constexpr float ignoredPercentage = 0.02;
>>   	const unsigned int total =
>>   		std::accumulate(begin(histogram), end(histogram), 0);
>> +	if (total == 0) {
>> +		LOG(IPASoftBL, Debug) << "Not guessing black level, histogram is empty";
>> +		return;
>> +	}
>> +
> t
> I don't think this can ever happen ?

It can, if something else is buggy. At the moment if the output size is too small,
then the histogram will stay empty because the `y` coordinate passed to processLine*()
is out of range. This should be easily reproducible with just `cam` and selecting the
smallest resolution.

$ cam -c1 -s width=160,height=120 -C32
[3:56:45.273196182] [8608]  INFO IPAManager ipa_manager.cpp:147 libcamera is not installed. Adding '/libcamera/build/src/ipa' to the IPA search path
[3:56:45.276523870] [8608]  INFO Camera camera_manager.cpp:340 libcamera v0.5.2+102-24905025
[...]
Using camera \_SB_.PC00.LNK1 as cam0
[3:56:45.335555691] [8608]  INFO Camera camera.cpp:1215 configuring streams: (0) 160x120-ABGR8888/Unset
[3:56:45.336489404] [8609]  INFO IPASoft soft_simple.cpp:264 IPASoft: Exposure 4-1102, gain 1-15.4922 (0.144922)
cam0: Capture 32 frames
../src/ipa/simple/algorithms/blc.cpp:96:44: runtime error: division by zero
AddressSanitizer:DEADLYSIGNAL
=================================================================
==8608==ERROR: AddressSanitizer: FPE on unknown address 0x7b3560b92e35 (pc 0x7b3560b92e35 bp 0x7b35607fd570 sp 0x7b35607fd240 T2)
     #0 0x7b3560b92e35 in libcamera::ipa::soft::algorithms::BlackLevel::process(libcamera::ipa::soft::IPAContext&, unsigned int, libcamera::ipa::soft::IPAFrameContext&, libcamera::SwIspStats const*, libcamera::ControlList&) ../src/ipa/simple/algorithms/blc.cpp:96
     [...]


Regards,
Barnabás Pőcze


> 
> What I've seen happening which triggers a divide by 0 in the AGC code is
> all samples being in the highest bin of the histogram, because the first
> frame of the sensor sometimes seems to have all 0x[3]ff as output, at least
> for the top 3/4th of the frame or something like that.
> 
> Which causes a blacklevel of >= 244 which in turn causes a divide by 0
> in the AGC code.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>   	const unsigned int pixelThreshold = ignoredPercentage * total;
>>   	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>>   	const unsigned int currentBlackIdx =
>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
index 8c1e9ed08..ea5356443 100644
--- a/src/ipa/simple/algorithms/blc.cpp
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -77,6 +77,11 @@  void BlackLevel::process(IPAContext &context,
 	constexpr float ignoredPercentage = 0.02;
 	const unsigned int total =
 		std::accumulate(begin(histogram), end(histogram), 0);
+	if (total == 0) {
+		LOG(IPASoftBL, Debug) << "Not guessing black level, histogram is empty";
+		return;
+	}
+
 	const unsigned int pixelThreshold = ignoredPercentage * total;
 	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
 	const unsigned int currentBlackIdx =