Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fluid.bufnmf~ crashes with fft size larger than 65k #258

Closed
rconstanzo opened this issue Sep 22, 2023 · 14 comments · Fixed by #281
Closed

fluid.bufnmf~ crashes with fft size larger than 65k #258

rconstanzo opened this issue Sep 22, 2023 · 14 comments · Fixed by #281
Labels
bug Something isn't working

Comments

@rconstanzo
Copy link

Please tell us what you were doing! You can include code and files by drag and dropping them into the text area.

So was trying to do something with fluid.bufnmf~ where I analyze using really large fft windows hoping to catch changes in "material" in there, and found a reproducible instacrash.

I tried to do a 3 component NMF decomposition using @fftsettings 131072 2048 131072 on jongly.aif (which is only 124438 samples long) and it instantly crashes.

What was the expected result?

It should analyze the buffer with these settings, perhaps zero padding the end of the window, or at least return an error saying the file has to be < the FFT size.

What was the actual result?

Instacrash.

What operating system were you using?

Mac

Operating system version

macOS 13.4.1

FluCoMa Version

1.0.6

@rconstanzo rconstanzo added the bug Something isn't working label Sep 22, 2023
@rconstanzo
Copy link
Author

This is on version 1.0.6+sha.e748875.core.sha.f694a366 but I have some of the new bits that @tremblap has posted on discourse.

Here's a crash report with the spicy bit being here methinks:

Thread 0 Crashed:: CrBrowserMain Dispatch queue: com.apple.main-thread
0   libvDSP.dylib                 	       0x1a2195b40 0x1a20e5000 + 723776
1   libvDSP.dylib                 	       0x1a212dc9c vDSP_fft_zropD + 132
2   fluid.bufnmf~                 	       0x12a1f83e8 fluid::algorithm::STFT::process(fluid::FluidTensorView<double, 1ul>, fluid::FluidTensorView<std::__1::complex<double>, 2ul>) + 848
3   fluid.bufnmf~                 	       0x12a1f5dfc fluid::client::Result fluid::client::bufnmf::NMFClient::process<float>(fluid::client::FluidContext&) + 3556
4   fluid.bufnmf~                 	       0x12a1f4d08 fluid::client::NRTThreadingAdaptor<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>::ThreadedTask::process(std::__1::promise<void>) + 68
5   fluid.bufnmf~                 	       0x12a1f49a4 fluid::client::NRTThreadingAdaptor<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>::ThreadedTask::ThreadedTask(std::__1::shared_ptr<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>, fluid::client::NRTThreadingAdaptor<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>::NRTJob&, bool) + 440
6   fluid.bufnmf~                 	       0x12a213c14 fluid::client::NRTThreadingAdaptor<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>::process() + 344
7   fluid.bufnmf~                 	       0x12a2139a0 fluid::client::impl::NonRealTime<fluid::client::FluidMaxWrapper<fluid::client::NRTThreadingAdaptor<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>>>::process() + 136
8   Max                           	       0x100cf2df0 defer + 68
9   fluid.bufnmf~                 	       0x12a212958 fluid::client::impl::NonRealTime<fluid::client::FluidMaxWrapper<fluid::client::NRTThreadingAdaptor<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>>>::deferProcess(fluid::client::FluidMaxWrapper<fluid::client::NRTThreadingAdaptor<fluid::client::ClientWrapper<fluid::client::bufnmf::NMFClient>>>*) + 272

And a simple patch to cause the crash.
Screenshot 2023-09-22 at 5 03 30 PM

<pre><code>
----------begin_max5_patcher----------
548.3ocqTssiaBCD8Y3qvx8UJBaL25S7eTUsxPLIDA1Hay1jtZ2u8NXfjzlr
a2TkGhS7wi8LmyYx7huGtRcPXvnug9Nxy6EeOOGzDf2xdObO+PcG23BCWq56
ERKNX9Lq3f0gWwkaQVEpVyM6VOsqUJpUiRWHzEP4XeqrSXcuG4LnZzthFsfN
vs06ZkaeRKpsyUYdQXVZFsHNNpHuHkDwBPIrvj.TZVXT.JFVQ+X49UiUUchK
yS6FW0pp1+UFdB5Ue+okfOI8qFsVkD+4oB48oBK+ZpjRlHAkstdhJyOo83fX
9xNAGe53K3UL9TJ07dgUneRH4K5Pz+AmkheBu60N9XSiP+FZuRtsSXl+9XHu
s4NjG56KOjbmJPRSCyRhKHE44TZbbVLA.iclMk9QJTSmhCMpefTQvOd43KQU
bC7epGhFbqVjaKHQ4OBAgwdfJRS2X6lPPWj8MugJMpQcs3T6RoSlVkKToVXN
Js65UaDHBpDlyLnjvjFCJFU1zXM.U.ww.dOIJihnQr70eWBkyTHs+RbBBz35
cbID+cXEr6zJnQgQwrzrIjhDVBAlCkSK92VwjKb5yMchzKcBWDtwo+0vZWgN
g+m1yrVujqkgBnykyFgAzRtsEFkcNFHktftJu7ggmEZyRztb.M.6U5os4Ats
sx4sItsZwysqwm5P3ZPPsfZNpc0E9PJCOeUvw0xwVmq3O01Aoz0bIgwWlA9L
Qb8f9u5+azAtehG
-----------end_max5_patcher-----------
</code></pre>

large fft crash.zip

@tremblap
Copy link
Member

tremblap commented Oct 11, 2024

ok I can reproduce in SC:

b = Buffer.read(s, FluidFilesPath("Nicol-LoopE-M.wav"),numFrames: 22050)
b.play
c = Buffer(s)
FluidBufNMF.process(s,b,bases: c,components: 3,windowSize: 131072, hopSize: 2048, fftSize: 131072, action:{\done.postln})

@tremblap
Copy link
Member

actually, I'll change the bug title, as I can reproduce with anything above 65536 of fft size.

b = Buffer.read(s, FluidFilesPath("Nicol-LoopE-M.wav"))
b.play
c = Buffer(s)
FluidBufNMF.process(s,b,bases: c,components: 3,windowSize: 22050, hopSize: 2048, fftSize: 131072, action:{\done.postln})

@rconstanzo are you able to run any FFTSize above 65k by any chance. @weefuzzy did we hard-code somewhere that as maximum?

@rconstanzo
Copy link
Author

Ideally this would zero pad and still finish the frame of analysis (just to be able to run big analyses on whatever files and not always need to check sizes vs windows first).

Hmm, I don't know if I tried that methodically. I'll try that now.

@rconstanzo
Copy link
Author

Indeed, it does crash with 131072 even on longer/larger files. I guess it just so happened I found this with jongly.aif, which is very close to that duration in the first place.

@tremblap
Copy link
Member

indeed, this crashes (1024 -1 131072)

this is now a @weefuzzy question :)


----------begin_max5_patcher----------
427.3ocqTsraBCCD7bxWgkOGPw4Az1Sbo2Z6OPUExIwgZTvNx1AnBAe60dc.
5qnVT4Pr0tdWO6L6FuKL.WH2xzXzcnmQAA6BCB.WNGA81A3UzskMTMDFtnyX
jBbj+HYmogYLu0x72AtfJVfQuzeLuBxQVrbT9wTZoJ5JlgolyDzhFHw39yDc
q3B6EBPQNkfo7UtXwbEqz3ggLIabbDJgDCaYGWOAr8h7kl+lbN2GF5Vh9i7T
v1XK6iEsgsEfFW2zwqFWzUKVUe.MSK6TkLTIZVAUyznJzr5Zi1JI1BViHwIY
nQDDIkDOMYPQCG8gueR7RwWp.kNEzjootszbuNkLnBkc8THq1TyTGPUCR25F
I0335fCKIWLeIvL.IOwskO4Wnax0mtknm3kxlQOHks2O5wwanq+GB.4REfza
cT12tISRu.9Cgfa3hu9N.fhy+mEE+HeOS5+yFcFnJl1N6SMb6iDmiI0Gy2fk
11tloz8ACPXU8kRky7lHvjK7l4fohsleLduGpxJFFqRzofxBucRF1mprhoDc
792.r8ZKjPGUXeBR2R87.Z7g6CeGUy2MP.
-----------end_max5_patcher-----------

@tremblap tremblap changed the title fluid.bufnmf~ crashes with fft size larger than file length fluid.bufnmf~ crashes with fft size larger than 65k Oct 11, 2024
@rconstanzo
Copy link
Author

Is @weefuzzy the reason we've been limited to such crude and small FFT sizes all these years?!

@weefuzzy
Copy link
Member

There is a default maximum, iirc, can't remember what it is. But whatever it is, things shouldn't crash if you ask for more (it should clip).

I'll need to remind myself how it all works. My vague memory is that you can set whatever maximum you think you'll need with a 4th arg to the fftsettings.

@tremblap
Copy link
Member

4th argument, or maxfftsize, still crash, but I will investigate further and make a better report, thanks

@weefuzzy
Copy link
Member

You know the drill 😸 Although I see now that the crash is in a system call. So either we're talking to it wrong, or there's a limit to that API we weren't aware of. Us being wrong is much more likely.

@tremblap
Copy link
Member

it'll be a fun one to track I think, good for me, I'll report back

@weefuzzy
Copy link
Member

Don't worry, I've found it. Currently a hard ceiling of 65356 since a change some time ago to stop making (expensive) FFT setup objects here there and everywhere. Now there is a global.

So two issues to fix:

  • not crashing whenever this maximum is broken (which it oughtn't)
  • arguing about what a maximum sensible FFT size might be

2nd of these should be a separate issue

@weefuzzy
Copy link
Member

Fix is non-obvious, see PR #281 above. Idea is to try and have both Nice Things: efficiency of having a default fft setup for routine use, but also to allow this sort of wanton profligacy.

@tremblap
Copy link
Member

ok I'll review and learn, once again humbled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants