Faster yin fft

[AlexHarker]

Address #168.

Done quickly so I need to check it, but I think this is worth doing and I believe I have done it correctly. @weefuzzy should be able to confirm fairly easily.

[weefuzzy]

Thanks for this. So, from a quick glance, you've made the FFT instance for YinFFT a member variable to save on the significant cost of running FFT setup, and as a consequence YinFFT now takes a maxFFT size argument?

Looks good. TestNoveltySegmentation needs to be amended for the tests to pass (because it instantiates YinFFT). I'll prod more thoroughly later, but this is a general pattern that needs to be repeated across most of the algorithms, both for efficiency and for moving towards sorting out the Allocator of Shame problem.

[AlexHarker]

Yes - I only fixed things from ALL_BUILD in flucoma-max.

There are other patterns to consider also. I'm currently looking at the init() in DCT which also comes up in stack traces. In this case the trig functions will run even if the size is the same as last time it was run, which is inefficient - so costly init() methods might also be worth a look.

Overall, I'm still finding the pitch detection adds loads of cost to my analysis (multiple times the cost of anything else) so unless I'm missing something about the fundamental cost (totally possible) I think there might be more to discover about why this particular analysis is so cost heavy.

[weefuzzy]

Well, it's not meant to be crazy expensive. I'll make a note to run it through a profiler though and see where the cost might be. I'm certainly aware of instances where either Eigen or STL containers are causing excessive allocation.

[weefuzzy]

Hmm. I'm not seeing anything alarming with the RT fluid.pitch~. Could you outline how you're doing pitch detection so I can try and recreate your problem? Could be something pathological in the buf version, for instance.

[AlexHarker]

I am looking at the DCT init issue, which is also related to a remaking of the DCT in CepstrumF0 called by reset(). That's a bit vague but that's my heaviest trace. I am about to test a quick fix.

[AlexHarker]

For context my overall processing was previously taking 75 secs and now it's about 525 secs down from 600 odd (that reduction is from the YinFFT storage change).

I can't seem to block mDCT.init() from taking place, so either my code is wrong, or something is getting remade each time the code runs. I'm now going to test with that call removed (as I don't need it for yin) and see how that affects the speed.

[AlexHarker]

OK - in general CepstrumF0.init() is really expensive and gets called in circumstances where it won't even be used. I'll start a new thread.

[AlexHarker]

The lastest issue is now covered in #171 and any discussion can happen there rather than on this PR, which hopefully is fairly straightforward to integrate.

[tremblap]

is this PR still needed or is it included in #171 ?

[AlexHarker]

#171 is an issue (so doesn't include any changes) - there is however a related PR #172. The reason for separating from this PR was that the two issues are distinct with separate causes and impacts (although they both affect speed in a particular situation that I can't now exactly remember). Anyway, the PRs don't overlap - this one deals with the speed of rebuilding the FFT for pitch detection per frame, whereas the other deals with the speed of the cepstrum init.

I will have a branch somewhere that merges both, as I needed both speed-ups, but I would advise that if these are merged then they are dealt with separately.

[weefuzzy]

YInFFT (and related) do still need updating not to make a new FFT all the time (although the penalty for doing so has been considerably reduced). In any case, things would look slightly different now, so I'll absorb these in spirit rather than trying to merge.

[AlexHarker]

I can redo this work against the latest main and resubmit if that is helpful - once done it should just be faster in certain cases (mostly with short buffers).

[weefuzzy]

I can't remember why I didn't do this when doing other allocation stuff. Perhaps I was just trying to stay focused, or forgot, or it was gnarly for some reason. Anyway, yes, please do update because remaking an FFT every frame is saddening.

[AlexHarker]

Closing as it is superseded by a newer PR. This thread will archive the conversation.