[0/5] Fix stats related problems in software ISP
mbox series

Message ID 20250821134141.83236-1-mzamazal@redhat.com
Headers show
Series
  • Fix stats related problems in software ISP
Related show

Message

Milan Zamazal Aug. 21, 2025, 1:41 p.m. UTC
Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
problems discussed there.

Milan Zamazal (5):
  libcamera: software_isp: Clarify SwStatsCpu::setWindow use
  libcamera: software_isp: Pass correct y-coordinate to stats
  libcamera: software_isp: Check processed window size alignment
  libcamera: simple: Avoid incorrect arithmetic in AWB
  libcamera: simple: Prevent division by zero in BLC

 src/ipa/simple/algorithms/awb.cpp          |  8 ++++---
 src/ipa/simple/algorithms/blc.cpp          |  5 +++++
 src/libcamera/software_isp/debayer_cpu.cpp | 26 +++++++++++++++++-----
 src/libcamera/software_isp/swstats_cpu.cpp | 16 +++++++++++++
 4 files changed, 46 insertions(+), 9 deletions(-)

Comments

Maciej S. Szmigiero Aug. 21, 2025, 9:35 p.m. UTC | #1
Hi Milan,

On 21.08.2025 15:41, Milan Zamazal wrote:
> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
> problems discussed there.
> 
> Milan Zamazal (5):
>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>    libcamera: software_isp: Pass correct y-coordinate to stats
>    libcamera: software_isp: Check processed window size alignment
>    libcamera: simple: Avoid incorrect arithmetic in AWB
>    libcamera: simple: Prevent division by zero in BLC

Thanks for this patch series.

I tested it on an IPU6 (Meteor Lake) laptop with an ov02e10 sensor.

The "division by zero" crashes with small capture sizes
are definitely gone.

However a few issues remain on this platform:
* If I request smaller capture resolution I don't get full
image just with smaller capture dimensions, like on most
self-contained cameras.

Instead, I get a crop of the requested size of the full image.
For example with 160 x 120 capture size the captured picture
just shows my eye.

I presume it is because DebayerCpu::sizes() returns a very
wide size range and SimpleCameraData::tryPipeline()
returns such wide range in config.outputSizes without
further constraining it by capture hardware capabilities?

Such wide config.outputSizes then gets used in
SimpleCameraConfiguration::validate() to (not) constrain the
requested picture size.

* The AGC algorithm seems to go into unstable oscillation
mode when the amount of light in the picture exceeds certain
threshold, with exposure bins alternating between 0 and tens
of thousands rapidly.

This makes the output picture look like it has been shot
thorough some very slow mechanical shutter, alternating
rapidly between almost normal picture and almost completely
black frame.

This happens especially with small capture sizes so I
presume is related to the previous point.

Thanks,
Maciej
Robert Mader Aug. 21, 2025, 11:08 p.m. UTC | #2
Hi, thanks for this series!

I just tested it on a Pixel 3a and a OnePlus6 and am happy to report 
that it fixes the "purple tint" that has previously been visible at 
certain resolutions for one of the sensors - so this is a great 
improvement for linux mobile users! I also quickly tested on a Librem5 
and didn't see any regressions, as expected.

Further more I checked whether it fixes 
https://bugs.libcamera.org/show_bug.cgi?id=236, but unfortunately it 
doesn't - https://patchwork.libcamera.org/patch/21602/ is still needed 
to make Snapshot  work on the Pixel 3a (we probably really need a patch 
like the mentioned one - hope to get around to send a v2 for 0.6).

Thus this series is:

Tested-by: Robert Mader <robert.mader@collabora.com>

On 21.08.25 15:41, Milan Zamazal wrote:
> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
> problems discussed there.
>
> Milan Zamazal (5):
>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>    libcamera: software_isp: Pass correct y-coordinate to stats
>    libcamera: software_isp: Check processed window size alignment
>    libcamera: simple: Avoid incorrect arithmetic in AWB
>    libcamera: simple: Prevent division by zero in BLC
>
>   src/ipa/simple/algorithms/awb.cpp          |  8 ++++---
>   src/ipa/simple/algorithms/blc.cpp          |  5 +++++
>   src/libcamera/software_isp/debayer_cpu.cpp | 26 +++++++++++++++++-----
>   src/libcamera/software_isp/swstats_cpu.cpp | 16 +++++++++++++
>   4 files changed, 46 insertions(+), 9 deletions(-)
>
Milan Zamazal Aug. 25, 2025, 8:31 p.m. UTC | #3
"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> Hi Milan,
>
> On 21.08.2025 15:41, Milan Zamazal wrote:
>> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
>> problems discussed there.
>> Milan Zamazal (5):
>>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>>    libcamera: software_isp: Pass correct y-coordinate to stats
>>    libcamera: software_isp: Check processed window size alignment
>>    libcamera: simple: Avoid incorrect arithmetic in AWB
>>    libcamera: simple: Prevent division by zero in BLC
>
> Thanks for this patch series.
>
> I tested it on an IPU6 (Meteor Lake) laptop with an ov02e10 sensor.
>
> The "division by zero" crashes with small capture sizes
> are definitely gone.

Good, thank you for testing.

> However a few issues remain on this platform:
> * If I request smaller capture resolution I don't get full
> image just with smaller capture dimensions, like on most
> self-contained cameras.
>
> Instead, I get a crop of the requested size of the full image.
> For example with 160 x 120 capture size the captured picture
> just shows my eye.
> 
> I presume it is because DebayerCpu::sizes() returns a very
> wide size range and SimpleCameraData::tryPipeline()
> returns such wide range in config.outputSizes without
> further constraining it by capture hardware capabilities?

Do you have specific hardware capabilities other than the resolution on
mind?

> Such wide config.outputSizes then gets used in
> SimpleCameraConfiguration::validate() to (not) constrain the
> requested picture size.

SimpleCameraConfiguration::validate() should pick the smallest sensor
resolution that can accommodate the requested output resolution, is it
otherwise by you?  Or are there other parameters that should be
considered?

> * The AGC algorithm seems to go into unstable oscillation
> mode when the amount of light in the picture exceeds certain
> threshold, with exposure bins alternating between 0 and tens
> of thousands rapidly.
>
> This makes the output picture look like it has been shot
> thorough some very slow mechanical shutter, alternating
> rapidly between almost normal picture and almost completely
> black frame.

I can reproduce the effect, at least with LED lighting.  Does it happen
under daylight too?

> This happens especially with small capture sizes so I
> presume is related to the previous point.

How specifically?  Maybe smaller resolutions are more prone to
variations?

> Thanks,
> Maciej
Milan Zamazal Aug. 25, 2025, 8:50 p.m. UTC | #4
Hi Robert,

Robert Mader <robert.mader@collabora.com> writes:

> Hi, thanks for this series!
>
> I just tested it on a Pixel 3a and a OnePlus6 and am happy to report that it fixes the "purple tint" that
> has previously been visible at certain resolutions for one of the
> sensors

Interesting.

> - so this is a great improvement for linux mobile users! I also
> quickly tested on a Librem5 and didn't see any regressions, as
> expected.

Good to know, thank you for testing.

> Further more I checked whether it fixes https://bugs.libcamera.org/show_bug.cgi?id=236, but unfortunately
> it doesn't - https://patchwork.libcamera.org/patch/21602/ is still needed to make Snapshot  work on the
> Pixel 3a (we probably really need a patch like the mentioned one - hope to get around to send a v2 for
> 0.6).

Hmm, what exactly breaks there?  With my imx219, for instance, when the
output resolution is 1920x1080, which is also one of the resolutions the
sensor supports, 3280x2464 sensor resolution is selected due to the
extra width needed for debayering.  Which is expected and works.  But it
results in cropping due to the resolution difference, which is maybe
what Maciej has on mind.  The question is what can be done about it
(this is not the first time I wonder what's worse: possible colour
trouble at the borders or the trouble with resolutions).

> Thus this series is:
>
> Tested-by: Robert Mader <robert.mader@collabora.com>
>
> On 21.08.25 15:41, Milan Zamazal wrote:
>> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
>> problems discussed there.
>>
>> Milan Zamazal (5):
>>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>>    libcamera: software_isp: Pass correct y-coordinate to stats
>>    libcamera: software_isp: Check processed window size alignment
>>    libcamera: simple: Avoid incorrect arithmetic in AWB
>>    libcamera: simple: Prevent division by zero in BLC
>>
>>   src/ipa/simple/algorithms/awb.cpp          |  8 ++++---
>>   src/ipa/simple/algorithms/blc.cpp          |  5 +++++
>>   src/libcamera/software_isp/debayer_cpu.cpp | 26 +++++++++++++++++-----
>>   src/libcamera/software_isp/swstats_cpu.cpp | 16 +++++++++++++
>>   4 files changed, 46 insertions(+), 9 deletions(-)
>>
Maciej S. Szmigiero Aug. 27, 2025, 9:10 p.m. UTC | #5
On 25.08.2025 22:31, Milan Zamazal wrote:
> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
> 
>> Hi Milan,
>>
(..)
>> However a few issues remain on this platform:
>> * If I request smaller capture resolution I don't get full
>> image just with smaller capture dimensions, like on most
>> self-contained cameras.
>>
>> Instead, I get a crop of the requested size of the full image.
>> For example with 160 x 120 capture size the captured picture
>> just shows my eye.
>>
>> I presume it is because DebayerCpu::sizes() returns a very
>> wide size range and SimpleCameraData::tryPipeline()
>> returns such wide range in config.outputSizes without
>> further constraining it by capture hardware capabilities?
> 
> Do you have specific hardware capabilities other than the resolution on
> mind?

In my case I meant hardware resolution capabilities.

>> Such wide config.outputSizes then gets used in
>> SimpleCameraConfiguration::validate() to (not) constrain the
>> requested picture size.
> 
> SimpleCameraConfiguration::validate() should pick the smallest sensor
> resolution that can accommodate the requested output resolution, is it
> otherwise by you?  Or are there other parameters that should be
> considered?

This sensor supports just one resolution of 1928x1088:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/ov02e10.c#n208

So I presume libcamera should allow just this single resolution
and reject smaller ones like 160x120?
Or, to be specific, always adjust the resolution in the requested
configuration to 1928x1088.

Since the current behavior of just returning a window crop of the
middle of the picture seems not very useful to libcamera consumers.

AFAIK, there is not even API to move this crop window to return
different parts of the source image than the center of it.

Traditional V4L2 drivers adjust their analog capture parameters to
something close to the requested resolution and/or have some kind
of a rescaler on-board.
Even venerable Bt878 had a built-in video scaler.

>> * The AGC algorithm seems to go into unstable oscillation
>> mode when the amount of light in the picture exceeds certain
>> threshold, with exposure bins alternating between 0 and tens
>> of thousands rapidly.
>>
>> This makes the output picture look like it has been shot
>> thorough some very slow mechanical shutter, alternating
>> rapidly between almost normal picture and almost completely
>> black frame.
> 
> I can reproduce the effect, at least with LED lighting.  Does it happen
> under daylight too?

Yes, but only under strong daylight, like summer sun illuminating
the scene.

It's also very borderline with sunlight since even slightly closing
the camera privacy shutter lowers the light input below the
threshold where this effect happens.

>> This happens especially with small capture sizes so I
>> presume is related to the previous point.
> 
> How specifically?  Maybe smaller resolutions are more prone to
> variations?
> 

Just a wild theory that smaller total pixel count somehow provides
less neighborhood to average any extreme values.
But at the other hand, it could be just a coincidence.

Thanks,
Maciej
Barnabás Pőcze Aug. 28, 2025, 7:11 a.m. UTC | #6
Hi

2025. 08. 27. 23:10 keltezéssel, Maciej S. Szmigiero írta:
> On 25.08.2025 22:31, Milan Zamazal wrote:
>> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
>>
>>> Hi Milan,
>>>
> (..)
>>> However a few issues remain on this platform:
>>> * If I request smaller capture resolution I don't get full
>>> image just with smaller capture dimensions, like on most
>>> self-contained cameras.
>>>
>>> Instead, I get a crop of the requested size of the full image.
>>> For example with 160 x 120 capture size the captured picture
>>> just shows my eye.
>>>
>>> I presume it is because DebayerCpu::sizes() returns a very
>>> wide size range and SimpleCameraData::tryPipeline()
>>> returns such wide range in config.outputSizes without
>>> further constraining it by capture hardware capabilities?
>>
>> Do you have specific hardware capabilities other than the resolution on
>> mind?
> 
> In my case I meant hardware resolution capabilities.
> 
>>> Such wide config.outputSizes then gets used in
>>> SimpleCameraConfiguration::validate() to (not) constrain the
>>> requested picture size.
>>
>> SimpleCameraConfiguration::validate() should pick the smallest sensor
>> resolution that can accommodate the requested output resolution, is it
>> otherwise by you?  Or are there other parameters that should be
>> considered?
> 
> This sensor supports just one resolution of 1928x1088:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/ov02e10.c#n208
> 
> So I presume libcamera should allow just this single resolution
> and reject smaller ones like 160x120?
> Or, to be specific, always adjust the resolution in the requested
> configuration to 1928x1088.
> 
> Since the current behavior of just returning a window crop of the
> middle of the picture seems not very useful to libcamera consumers.

I agree that it is probably not what users expect. But I imagine it's
still better than only offering fewer choices.


> 
> AFAIK, there is not even API to move this crop window to return
> different parts of the source image than the center of it.

I think you're looking for the `ScalerCrop` control. Of course the
"simple" pipeline handler does not support it yet.


Regards,
Barnabás Pőcze


> 
> Traditional V4L2 drivers adjust their analog capture parameters to
> something close to the requested resolution and/or have some kind
> of a rescaler on-board.
> Even venerable Bt878 had a built-in video scaler.
> 
>>> * The AGC algorithm seems to go into unstable oscillation
>>> mode when the amount of light in the picture exceeds certain
>>> threshold, with exposure bins alternating between 0 and tens
>>> of thousands rapidly.
>>>
>>> This makes the output picture look like it has been shot
>>> thorough some very slow mechanical shutter, alternating
>>> rapidly between almost normal picture and almost completely
>>> black frame.
>>
>> I can reproduce the effect, at least with LED lighting.  Does it happen
>> under daylight too?
> 
> Yes, but only under strong daylight, like summer sun illuminating
> the scene.
> 
> It's also very borderline with sunlight since even slightly closing
> the camera privacy shutter lowers the light input below the
> threshold where this effect happens.
> 
>>> This happens especially with small capture sizes so I
>>> presume is related to the previous point.
>>
>> How specifically?  Maybe smaller resolutions are more prone to
>> variations?
>>
> 
> Just a wild theory that smaller total pixel count somehow provides
> less neighborhood to average any extreme values.
> But at the other hand, it could be just a coincidence.
> 
> Thanks,
> Maciej
>
Milan Zamazal Aug. 28, 2025, 4:30 p.m. UTC | #7
"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

>>> * The AGC algorithm seems to go into unstable oscillation
>>> mode when the amount of light in the picture exceeds certain
>>> threshold, with exposure bins alternating between 0 and tens
>>> of thousands rapidly.
>>>
>>> This makes the output picture look like it has been shot
>>> thorough some very slow mechanical shutter, alternating
>>> rapidly between almost normal picture and almost completely
>>> black frame.
>> I can reproduce the effect, at least with LED lighting.  Does it happen
>> under daylight too?
>
> Yes, but only under strong daylight, like summer sun illuminating
> the scene.
>
> It's also very borderline with sunlight since even slightly closing
> the camera privacy shutter lowers the light input below the
> threshold where this effect happens.

Thanks for info.  These exposure instabilities should really be fixed,
I'll look at what can be possibly done.

As for cropping, I don't have a clear opinion what's the best option of
changing the resolution, scaling, cropping.  As Barnabás pointed out,
implementing ScalerCrop would perhaps help substantially.
Maciej S. Szmigiero Aug. 28, 2025, 9:07 p.m. UTC | #8
On 28.08.2025 09:11, Barnabás Pőcze wrote:
> Hi
> 
> 2025. 08. 27. 23:10 keltezéssel, Maciej S. Szmigiero írta:
>> On 25.08.2025 22:31, Milan Zamazal wrote:
>>> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
>>>
>>>> Hi Milan,
>>>>
>> (..)
>>>> However a few issues remain on this platform:
>>>> * If I request smaller capture resolution I don't get full
>>>> image just with smaller capture dimensions, like on most
>>>> self-contained cameras.
>>>>
>>>> Instead, I get a crop of the requested size of the full image.
>>>> For example with 160 x 120 capture size the captured picture
>>>> just shows my eye.
>>>>
>>>> I presume it is because DebayerCpu::sizes() returns a very
>>>> wide size range and SimpleCameraData::tryPipeline()
>>>> returns such wide range in config.outputSizes without
>>>> further constraining it by capture hardware capabilities?
>>>
>>> Do you have specific hardware capabilities other than the resolution on
>>> mind?
>>
>> In my case I meant hardware resolution capabilities.
>>
>>>> Such wide config.outputSizes then gets used in
>>>> SimpleCameraConfiguration::validate() to (not) constrain the
>>>> requested picture size.
>>>
>>> SimpleCameraConfiguration::validate() should pick the smallest sensor
>>> resolution that can accommodate the requested output resolution, is it
>>> otherwise by you?  Or are there other parameters that should be
>>> considered?
>>
>> This sensor supports just one resolution of 1928x1088:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/ov02e10.c#n208
>>
>> So I presume libcamera should allow just this single resolution
>> and reject smaller ones like 160x120?
>> Or, to be specific, always adjust the resolution in the requested
>> configuration to 1928x1088.
>>
>> Since the current behavior of just returning a window crop of the
>> middle of the picture seems not very useful to libcamera consumers.
> 
> I agree that it is probably not what users expect. But I imagine it's
> still better than only offering fewer choices.
> 
> 
>>
>> AFAIK, there is not even API to move this crop window to return
>> different parts of the source image than the center of it.
> 
> I think you're looking for the `ScalerCrop` control. Of course the
> "simple" pipeline handler does not support it yet.

Yeah, that may be the case here.

Still, libcamera consumers like PipeWire generally expect scaling
of the full picture rather than cropping when they when they ask for
lower output resolution.
Maybe adding a libcamera API flag selecting the behavior would help
here.

I created a Bugzilla entry for this issue so it won't get lost:
https://bugs.libcamera.org/show_bug.cgi?id=284

For now, I simply reduce the DebayerCpu::sizes() output to
small range around the input size with the patch that I attached
in the aforementioned Bugzilla entry.
This makes PipeWire camera applications get what they expect.

> Regards,
> Barnabás Pőcze
> 

Thanks,
Maciej
Maciej S. Szmigiero Aug. 28, 2025, 9:09 p.m. UTC | #9
On 28.08.2025 18:30, Milan Zamazal wrote:
> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
> 
>>>> * The AGC algorithm seems to go into unstable oscillation
>>>> mode when the amount of light in the picture exceeds certain
>>>> threshold, with exposure bins alternating between 0 and tens
>>>> of thousands rapidly.
>>>>
>>>> This makes the output picture look like it has been shot
>>>> thorough some very slow mechanical shutter, alternating
>>>> rapidly between almost normal picture and almost completely
>>>> black frame.
>>> I can reproduce the effect, at least with LED lighting.  Does it happen
>>> under daylight too?
>>
>> Yes, but only under strong daylight, like summer sun illuminating
>> the scene.
>>
>> It's also very borderline with sunlight since even slightly closing
>> the camera privacy shutter lowers the light input below the
>> threshold where this effect happens.
> 
> Thanks for info.  These exposure instabilities should really be fixed,
> I'll look at what can be possibly done.

Thanks - I created a Bugzilla entry for this too so it won't get lost:
https://bugs.libcamera.org/show_bug.cgi?id=283

By the way, with the full output picture the amount the camera shutter
needs to be closed in order to prevent these osculations in sunlight is
pretty significant, so the "even slightly closing shutter stops it" above
was effectively a side effect of the small crop size.

Thanks,
Maciej
Laurent Pinchart Aug. 29, 2025, 1:21 p.m. UTC | #10
On Thu, Aug 28, 2025 at 06:30:06PM +0200, Milan Zamazal wrote:
> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
> 
> >>> * The AGC algorithm seems to go into unstable oscillation
> >>> mode when the amount of light in the picture exceeds certain
> >>> threshold, with exposure bins alternating between 0 and tens
> >>> of thousands rapidly.
> >>>
> >>> This makes the output picture look like it has been shot
> >>> thorough some very slow mechanical shutter, alternating
> >>> rapidly between almost normal picture and almost completely
> >>> black frame.
> >> I can reproduce the effect, at least with LED lighting.  Does it happen
> >> under daylight too?
> >
> > Yes, but only under strong daylight, like summer sun illuminating
> > the scene.
> >
> > It's also very borderline with sunlight since even slightly closing
> > the camera privacy shutter lowers the light input below the
> > threshold where this effect happens.
> 
> Thanks for info.  These exposure instabilities should really be fixed,
> I'll look at what can be possibly done.
> 
> As for cropping, I don't have a clear opinion what's the best option of
> changing the resolution, scaling, cropping.  As Barnabás pointed out,
> implementing ScalerCrop would perhaps help substantially.

We will likely not support scaling with the CPU-based soft ISP, so
ScalerCrop won't help. The GPU implementation would be a different
story, it should hopefully be easy to support scaling there.
Maciej S. Szmigiero Sept. 10, 2025, 10:57 a.m. UTC | #11
On 21.08.2025 23:35, Maciej S. Szmigiero wrote:
> Hi Milan,
> 
> On 21.08.2025 15:41, Milan Zamazal wrote:
>> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
>> problems discussed there.
>>
>> Milan Zamazal (5):
>>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>>    libcamera: software_isp: Pass correct y-coordinate to stats
>>    libcamera: software_isp: Check processed window size alignment
>>    libcamera: simple: Avoid incorrect arithmetic in AWB
>>    libcamera: simple: Prevent division by zero in BLC
> 
> Thanks for this patch series.
> 
> I tested it on an IPU6 (Meteor Lake) laptop with an ov02e10 sensor.
> 
> The "division by zero" crashes with small capture sizes
> are definitely gone.
> 

Is this patch series going to be merged?
I don't see it in the libcamera git, even though it definitely
improves things.

In case this helps:
Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>

Thanks,
Maciej
Kieran Bingham Sept. 10, 2025, 11:14 a.m. UTC | #12
Quoting Maciej S. Szmigiero (2025-09-10 11:57:30)
> On 21.08.2025 23:35, Maciej S. Szmigiero wrote:
> > Hi Milan,
> > 
> > On 21.08.2025 15:41, Milan Zamazal wrote:
> >> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
> >> problems discussed there.
> >>
> >> Milan Zamazal (5):
> >>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
> >>    libcamera: software_isp: Pass correct y-coordinate to stats
> >>    libcamera: software_isp: Check processed window size alignment
> >>    libcamera: simple: Avoid incorrect arithmetic in AWB
> >>    libcamera: simple: Prevent division by zero in BLC
> > 
> > Thanks for this patch series.
> > 
> > I tested it on an IPU6 (Meteor Lake) laptop with an ov02e10 sensor.
> > 
> > The "division by zero" crashes with small capture sizes
> > are definitely gone.
> > 
> 
> Is this patch series going to be merged?
> I don't see it in the libcamera git, even though it definitely
> improves things.
> 
> In case this helps:
> Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>

Thank you - that indeed helps know that it improves the issue.

The actual blockage at the moment is "Reviewed-by:" tags. CI won't merge
anything without at least one RB tag (or an Acked-by: from a
'maintainer').

I think I see in this series that you may have authored one of the
patches. That sounds like you're a great candidate to do a review of the
others?

Could you read through them and make sure they're ok please?

If you notice anything, please comment inline - and if you think it's ok
- just add "Reviewed-by: <....>" in the mail.

--
Thanks

Kieran


> 
> Thanks,
> Maciej
>
Barnabás Pőcze Sept. 10, 2025, 12:04 p.m. UTC | #13
Hi

2025. 08. 21. 15:41 keltezéssel, Milan Zamazal írta:
> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
> problems discussed there.

Not sure if a fix was planned here, so I just wanted to note that "Agc"
also doesn't handle an empty histogram:

cam0: Capture 32 frames
../src/ipa/simple/algorithms/agc.cpp:128:30: runtime error: division by zero

Thread 3 "cam" received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7bffe99ff6c0 (LWP 2447)]
0x00007bffe9d7f5e8 in libcamera::ipa::soft::algorithms::Agc::process (this=0x7c1fedc16fb0, context=..., frame=0, frameContext=..., stats=0x7fffefe20000, metadata=...) at ../src/ipa/simple/algorithms/agc.cpp:128
128			unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
(gdb) p yHistValsPerBin
$1 = 0


Regards,
Barnabás Pőcze


> 
> Milan Zamazal (5):
>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>    libcamera: software_isp: Pass correct y-coordinate to stats
>    libcamera: software_isp: Check processed window size alignment
>    libcamera: simple: Avoid incorrect arithmetic in AWB
>    libcamera: simple: Prevent division by zero in BLC
> 
>   src/ipa/simple/algorithms/awb.cpp          |  8 ++++---
>   src/ipa/simple/algorithms/blc.cpp          |  5 +++++
>   src/libcamera/software_isp/debayer_cpu.cpp | 26 +++++++++++++++++-----
>   src/libcamera/software_isp/swstats_cpu.cpp | 16 +++++++++++++
>   4 files changed, 46 insertions(+), 9 deletions(-)
>
Maciej S. Szmigiero Sept. 10, 2025, 9:15 p.m. UTC | #14
On 10.09.2025 13:14, Kieran Bingham wrote:
> Quoting Maciej S. Szmigiero (2025-09-10 11:57:30)
>> On 21.08.2025 23:35, Maciej S. Szmigiero wrote:
>>> Hi Milan,
>>>
>>> On 21.08.2025 15:41, Milan Zamazal wrote:
>>>> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
>>>> problems discussed there.
>>>>
>>>> Milan Zamazal (5):
>>>>     libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>>>>     libcamera: software_isp: Pass correct y-coordinate to stats
>>>>     libcamera: software_isp: Check processed window size alignment
>>>>     libcamera: simple: Avoid incorrect arithmetic in AWB
>>>>     libcamera: simple: Prevent division by zero in BLC
>>>
>>> Thanks for this patch series.
>>>
>>> I tested it on an IPU6 (Meteor Lake) laptop with an ov02e10 sensor.
>>>
>>> The "division by zero" crashes with small capture sizes
>>> are definitely gone.
>>>
>>
>> Is this patch series going to be merged?
>> I don't see it in the libcamera git, even though it definitely
>> improves things.
>>
>> In case this helps:
>> Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> 
> Thank you - that indeed helps know that it improves the issue.
> 
> The actual blockage at the moment is "Reviewed-by:" tags. CI won't merge
> anything without at least one RB tag (or an Acked-by: from a
> 'maintainer').
> 
> I think I see in this series that you may have authored one of the
> patches. That sounds like you're a great candidate to do a review of the
> others?
> 
> Could you read through them and make sure they're ok please?
>
> If you notice anything, please comment inline - and if you think it's ok
> - just add "Reviewed-by: <....>" in the mail.

I am actually a newcomer to libcamera, however I will try to do my
best to review this series in the coming days or so. 
> --
> Thanks
> 
> Kieran
Thanks,
Maciej
Milan Zamazal Sept. 11, 2025, 12:50 p.m. UTC | #15
Hi Barnabás,

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2025. 08. 21. 15:41 keltezéssel, Milan Zamazal írta:
>> Fixes of https://bugs.libcamera.org/show_bug.cgi?id=280 and the related
>> problems discussed there.
>
> Not sure if a fix was planned here, so I just wanted to note that "Agc"
> also doesn't handle an empty histogram:
>
> cam0: Capture 32 frames
> ../src/ipa/simple/algorithms/agc.cpp:128:30: runtime error: division by zero
>
> Thread 3 "cam" received signal SIGFPE, Arithmetic exception.
> [Switching to Thread 0x7bffe99ff6c0 (LWP 2447)]
> 0x00007bffe9d7f5e8 in libcamera::ipa::soft::algorithms::Agc::process (this=0x7c1fedc16fb0, context=..., frame=0,
> frameContext=..., stats=0x7fffefe20000, metadata=...) at ../src/ipa/simple/algorithms/agc.cpp:128
> 128			unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
> (gdb) p yHistValsPerBin
> $1 = 0

I missed this one, thanks for pointing it out, I'll add a fix for this.

> Regards,
> Barnabás Pőcze
>
>
>> Milan Zamazal (5):
>>    libcamera: software_isp: Clarify SwStatsCpu::setWindow use
>>    libcamera: software_isp: Pass correct y-coordinate to stats
>>    libcamera: software_isp: Check processed window size alignment
>>    libcamera: simple: Avoid incorrect arithmetic in AWB
>>    libcamera: simple: Prevent division by zero in BLC
>>   src/ipa/simple/algorithms/awb.cpp          |  8 ++++---
>>   src/ipa/simple/algorithms/blc.cpp          |  5 +++++
>>   src/libcamera/software_isp/debayer_cpu.cpp | 26 +++++++++++++++++-----
>>   src/libcamera/software_isp/swstats_cpu.cpp | 16 +++++++++++++
>>   4 files changed, 46 insertions(+), 9 deletions(-)
>>
Milan Zamazal Sept. 11, 2025, 12:59 p.m. UTC | #16
"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> On 28.08.2025 18:30, Milan Zamazal wrote:
>> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
>> 
>
>>>>> * The AGC algorithm seems to go into unstable oscillation
>>>>> mode when the amount of light in the picture exceeds certain
>>>>> threshold, with exposure bins alternating between 0 and tens
>>>>> of thousands rapidly.
>>>>>
>>>>> This makes the output picture look like it has been shot
>>>>> thorough some very slow mechanical shutter, alternating
>>>>> rapidly between almost normal picture and almost completely
>>>>> black frame.
>>>> I can reproduce the effect, at least with LED lighting.  Does it happen
>>>> under daylight too?
>>>
>>> Yes, but only under strong daylight, like summer sun illuminating
>>> the scene.
>>>
>>> It's also very borderline with sunlight since even slightly closing
>>> the camera privacy shutter lowers the light input below the
>>> threshold where this effect happens.
>> Thanks for info.  These exposure instabilities should really be fixed,
>> I'll look at what can be possibly done.
>
> Thanks - I created a Bugzilla entry for this too so it won't get lost:
> https://bugs.libcamera.org/show_bug.cgi?id=283

For the track, Hans plans to post a fix.  IIRC the idea was to adjust
the exposure less frequently, to ensure the actual exposure/gain values
for the given frame are the expected ones.

> By the way, with the full output picture the amount the camera shutter
> needs to be closed in order to prevent these osculations in sunlight is
> pretty significant, so the "even slightly closing shutter stops it" above
> was effectively a side effect of the small crop size.
>
> Thanks,
> Maciej