Pretty sure it's a blocksize vs padding vs order issue, specifically that object bombs if order > blocksize - padding

I'll investigate. For info, this crashes too:

{FluidTransientSlice.ar(SinOsc.ar(100,mul: LFPulse.ar(0.5)), order: 196)}.play

ok it is the resize of the mFFT in the autocorrelation (ARMmodel line 308) that throws an assert. Line 42 the FFT mMaxSize is allocated twice the blocksize - but I never see when we call it!

also, I don't know if that is related, but TransientSlices do not give the same results between platforms... and crashes with the UT in Linux with simple settings that do not crash in Windows and MacOS... this code crashes:

b = Buffer.read(s, FluidFilesPath("Nicol-LoopE-M.wav"))
c = Buffer(s)
FluidBufTransientSlice.process(s,b,indices: c)

ok we get the same assert with:

{FluidTransients.ar(SinOsc.ar(100,mul: LFPulse.ar(0.5)), order: 196)}.play

Please be specific about the assert that fires, permalinking to the relevant line of code.

I would suggest as a first step that ARModel should include an appropriate assert to check that the estimate methods are not called with an input that exceeds the largest expected size (which is currently not stored directly, so would either need to be stored or derived). This would help to isolate issues in code calling the ARModel from those in the model itself. If it turns out that ARModel is being called with incorrectly-sized input the task will then be to figure out why that is happening.

or you could just run the test code posted in debug 😄 in all cases, the assert thrown is when resizing the FFT as described above:

ok it is the resize of the mFFT in the autocorrelation (ARMmodel line 308) that throws an assert. Line 42 the FFT mMaxSize is allocated twice the blocksize - but I never see when we call it!

which calls resize on the mFFT with in.size (which is larger than what it has been allocated - the assert itself is line 86 of FFT.hpp but that is not very helpful!) I assume the algo was right (and took a long time to find the actual value calculation) and corrected it so we have enough ram.

The assert() on the estimate should be added as it guards against other cases of misuse (there are a lot of asserts for other uses of ARModel), and then we trace back from there (or if it doesn't fire for this crash then we know that the ARModel is the likely the place to look). The assert in the FFT is far from "not very helpful" as it is the point of failure. That's where we would trace from. I'm not set up to easily test so my starting point for a review will always be reading the code. At the moment my feeling is that the issue is not yet fully understood and that should be the aim before merging any fix (regardless of whether the fix stops a crash).

Merging a change that stops a crash (assert or otherwise), but isn't fully understood risks making it harder to implement the correct fix in future, and my current reading of the code is that the assumptions of the transient extractor code in terms of sizing appear to be consistent.

This confusion is down to me, plus ça change. When I retrofitted the runtime allocation to all the algorithms, I don't think I paid the complexities of the ARModel and Transient<X> classes quite enough heed, and I certainly didn't do due diligence on the clients.

So, I think what's biting us is that the slight shift in idiom towards all algorithms being instantiated with inviolable maximum storage requirements isn't yet observed by the respective clients, but also I should have instrumented ARModel and TransientExtraction with more consistency asserts where it still calls resize on sub-objects.

The concrete problem in this issue, then, is actually that the clients neither actually pass in proper maximal values nor consider the invariants of TransientExtraction when the objects are created (namely that the blockSize and padSize must be >= model order). For this particular case: the actual padding size being used ends up being 196 (the model order), because that's bigger than the default of 128. Hence kaboom.

So, no change to ARModel is needed: if kosher numbers are passed in the first place, then all is well. I'll do a PR to fix up the clients, but also enforce the invariants in TransientExtraction.

Thanks @weefuzzy - I think the assert I suggest above would be useful for anyone using ARmodel directly, and similar asserts should maybe be used for the issues you state. From what you've put here I am guessing that the fix PA proposes happens to fix it in certain scenarios, but isn't actually the correct thing to do? In that case the merge shouldn't be made in my opinion.

If I can help by testing further I will as I have just set up to compile the flucoma-sc repo if needed, but if all is clear I'm happy to await better fixes.

Yeah, an assert there will be useful too. And, yes, we don't need the change in #288.

I'll put something together once I've un-broken my own build system, which seems to have gone bonkers.

Lol. I did just realise that I had dropped the ball in ARModel in quite a spectacular way though because the code largely assumes that the size of the mParameters buffer is always the same as the current model order, which is no longer the case 🤦

Because I'd also been lazy with updating the client, this probably wouldn't yet have been experienced in the wild unless someone was adjusting the model order a lot on a single instance.