Crash in astrophoto denoise

[paolodepetrillo]

I am getting a random segfault in the astrophoto denoise module - if I drag the exposure slider around it will crash within a few seconds.

The segfault occurs here:

darktable/src/common/nlmeans_core.c

Line 710 in 22078ab

out[col] += (pixel * wt);

It looks like the problem is in this line:
__m128 pixel = _mm_load_ps(in+4*col+offset);

_mm_load_ps requires that its operand is aligned to 16 bytes, but in+4*col+offset is not guaranteed to be aligned that way. As a quick test I changed all instances of _mm_load_ps in the file to _mm_loadu_ps which does not have the alignment requirement, and that fixes the crash.

Platform

• darktable version : 4.1.0+577~gafb0ac5c0
• Linux - Distro : Arch Linux
• Memory : 16GB
• Graphics card : GTX 1080
• Graphics driver :
• OpenCL installed : yes
• OpenCL activated : no
• Xorg : 21.1.4
• Desktop : kde
• GTK+ : 3.24.34
• gcc : 12.2.0
• cflags :
• CMAKE_BUILD_TYPE : RelWithDebInfo

[jenshannoschwalm]

Interesting problem :-) From my understanding the aligned access is bad if one of the pre-calculated offsets is bad. Don't understand the code yet, @ralfbrown likely knows best.

[arturoisilvia]

Being in windows 11
darktable-4.1.0+1171~g95c5c117b-win64.exe
the problem persists.

When moving the sliders in the module "Astrophoto denoise" darktable closes ruining the editing.

I suggest to remove the "Astrophoto denoise" module in 4.2 until it is resolved.

[jenshannoschwalm]

I tried to get into this and on current master i can't reproduce any more. (I did on master before 4.2)

It could very well be that this issue was pipe-cache related, could you check again?

[arturoisilvia]

with darktable-4.3.0+395~g02e48af21-win64.exe
from github nightly builds
on windows 11
continues to crash

darktable -d all:
darktable-log -d all.txt

another (second attempt)
darktable-log -d all 2.txt

[hitausmomentti]

Still happens for me as well with ad064399e9 , Ubuntu 22.10, Intel E5-2695 v2

Not sure if it's just coincidental, but now it seems easier to reproduce by vigorous agitation of the white balance slider instead of the exposure slider. The backtrace looks the same. Happens with the file posted in #13371 and others files from the same camera

[jenshannoschwalm]

@ralfbrown may i ping you here as the nlmeans code seems to be entirely your baby :-)

Although the described crash is very difficult to reproduce here i tested both normal-cpu and sse2 code paths and never could get the non-sse2 code to crash. I started testing performance and couldn't find any significant difference on my 8core/64GB intel thing (4, 8 ,16 threads tested).
From my current testing i suppose the crash is related to some unbound mem access.

Before spending time on understanding & fixing the SSE2 stuff i would propose to

1. get rid of SSE2 code because there seems to be no benefit
2. also get rid of the CACHE_PIXDIFFS code section as you didn't find a benefit (i didn't test that)

That would lead to to maintaining less&complicated code ...

Do you agree? If so - you want to do that yourself or shall i do it?

If your perf results differ from mine, any idea on this?

[ralfbrown]

From what I've seen on this thread, the crash is from using an instruction requiring 16-byte alignment with an address that's not a multiple of 16; the compiler generates the slower variant instruction which does not require alignment for the non-SSE code even when vectorizing. An unaligned access should not be possible unless the input buffer is misaligned....

The first thing I was planning to do when I got around to looking at this issue was compare the SSE and non-SSE speeds, so I'm OK with just removing the SSE codepath.

Before removing the CACHE_PIXDIFFS stuff, we should run benchmarks. It didn't help on my machine, but that could be specific to my hardware. if it doesn't help on yours, we can remove it as well.

[jenshannoschwalm]

I thought the non-aligning might be due to an odd line width here?

Anyway, will do as discussed, remove sse2 code and after that test for cached.

[ralfbrown]

No, because we're offsetting by some number of pixels, which are 16 bytes each.

[jenshannoschwalm]

No, because we're offsetting by some number of pixels, which are 16 bytes each.

Right & sure. And from reading the code your "An unaligned access should not be possible unless the input buffer is misaligned...." is daunting me. I have no idea how that can happen at all. Another explanation would be "out-of-buffer" reading - but tests here & from @paolodepetrillo show the crash can be avoided by _mm_loadu_ps so that hypothesis doesn't hold.

Might be the bad boy is somewhere else. Someone modifies the cacheline adress?

[jenshannoschwalm]

Could you re-check with current master and

1. report if issue persists for you
2. possibly close this issue

[hitausmomentti]

Tested with a new build from master. Seems stable, no problems after trying both intensive slider wiggling and adjusting a few pictures normally, when previously both cases crashed.