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

PFFFT Lua wrapper #7930

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

nuoun
Copy link
Contributor

@nuoun nuoun commented Dec 18, 2024

(Draft, do not merge yet)

Draft, do not merge yet
@nuoun
Copy link
Contributor Author

nuoun commented Dec 18, 2024

luafft

So yes, FFTs in Lua would be really neat and I hope this is a sensible approach. I'll line out a few things here while trying to keep it short:

So firstly, adding functions like this in setSurgeFunctionEnvironment() obviously also makes them available in Formula, neat to have FFTs there too but it does means they can also be used in a way that performance matters. For the buffer allocation (which I'll save for last here) these would still be accessed all from the same thread if I'm not mistaken. Alternatively we could of course make it so it's a WTSE thing only.. but I personally prefer to not have the WTSE/Formula Lua environments diverge really.

Then these different functions with arguments for the main wrapper can probably be defined a bit better (maybe using a macro) but I think adding all these different modes (real/complex and ordered/unordered) makes sense so we can get the most out of it. I've yet to add pffft_zconvolve_accumulate and zreorder, probably will end up adding those too but those should also be easily done on the Lua side. (Edit: I think leaving the normalization on the Lua side also makes most sense)

FFT size, as the code comment says a somewhat weird size like 180 should in theory be fine but that crashes, not sure why yet as the allocated buffer size does not seem to be the issue (maybe an MSVC miscompile?) Anyway, enforcing powers of two is the safe workaround for now.

Then lastly, memory allocation, which is a big TODO right now. I noticed we're (almost?) not using malloc in Surge at all and I imagine we have a nice class that does SIMD aligned buffers for floats. As Paul said on Discord a little API would be neat but I hope we can get away with keeping it fairly simple. Being able to pre-allocate from the Lua init function would probably be best but maybe we can also keep it somewhat dynamic by checking the buffer size with the FFT data input array size and create new buffers for different sizes on demand.

A big question is where when do we free/delete? - And another question I have is where would we store the pointers to these buffers?

@mkruselj
Copy link
Collaborator

FFT is absolutely overkill for Formula modulator (it needs to be realtime, remember), so these commands should absolutely NOT be a part of Formula modulator.

@nuoun
Copy link
Contributor Author

nuoun commented Dec 18, 2024

FFT is absolutely overkill for Formula modulator (it needs to be realtime, remember), so these commands should absolutely NOT be a part of Formula modulator.

Not sure if I agree, even if performance would be an issue (I don't really think it is for small FFT sizes as PFFFT is incredibly performant) an FFT table could still be generated in the init just once to setup. Imo, it really is not that different than doing a bunch of table operations in Lua, as in if you do too much it will be, well, too much. Limiting the FFT size might be wise tho.

Edit: quick test, above 4096 it starts to look dicey CPU wise on my several years old by now Ryzen 5

@nuoun
Copy link
Contributor Author

nuoun commented Dec 18, 2024

So arm64ec might be out here as I don't think PFFFT has been patched for that, not sure about MSVC on arm64 tho.

@mkruselj
Copy link
Collaborator

There are people using Surge on machines way older than your Ryzen 5. I still think we shouldn't include FFT in formula.

@baconpaul
Copy link
Collaborator

yeah even if you call it 'just' in init, init is called in every voice on. could be super expensive. i would hesitate. maybe we want a patch-init for formula one day though?

I would add an enum to the setSurgeEnvironment for flavor with values like FORMULA, WTSE, GENERIC or some such and then we can opt in or out. Or maybe add a features enum like

enum EnvironmentFeatures : uint64_t
{
    BASE = 0,
    HAS_BITOPS = 1 << 1
    HAS_FFT = 1 << 2
};

and then we can | it together maybe? And then define static constexpr uint64_t wtsefeatures{BASE | HAS_FFT} and stuff.

You get the idea

@nuoun
Copy link
Contributor Author

nuoun commented Dec 19, 2024

So while trying to not harp on this as I'm not even sure it's important for Formula and am mostly just curious if the concept would even make sense here or what it would make possible, just a few thoughts:

My example of a FFT size of 4,096 in real time was really not very sensible as I don't think there's much point to doing an FFT that large continuously to begin with. Formula can take inputs from other signals but only every sample block so the sampled signal here would have little frequency content to analyze meaning there is no need for such a massive FFT size to begin with. PFFFT can do FFTs as small as 32 samples and I think 128/256 is generally adequate on audio at average sampling rates for most real time FFTs.

As for running just once on init, one or more FFT results could be stored in the shared table just once, and every Formula on every voice would have access to those for free until the shared table gets wiped when a new script gets initialized at which point they need to be recreated but until then they're nothing but some pointers to the functions on the Lua stack

Anyway, like I said more interested what can be done with this as a concept than further arguing over this, I could try and get this working on my Raspberry Pi to see what a slower computer would be dealing with but I'm fine to leave it at this or wonder what the benefits of this are really.

Paul's way of splitting the Lua env seems more elegant than what I had in mind (and also maybe we could have a wavetable LFO someday)

@baconpaul
Copy link
Collaborator

As for running just once on init, one or more FFT results could be stored in the shared table just once, and every Formula on every voice would have access to those for free until the shared table gets wiped when a new script gets initialized at which point they need to be recreated but until then they're nothing but some pointers to the functions on the Lua stack

the formula shared table is per-voice so init runs on every note on.

So to do what you suggest we would need a 'per-patch-init' called to seed the table when we reload the patch, basically, as a separate function than the per-isntance-voice-init. And that would be tricker than in wtse since we can run multiple different formulae interwoven

not impossible, just not what formula init does today.

(since you can't run two wtse at the same time, we can be far more parsimonious with init)

@mkruselj
Copy link
Collaborator

I still feel there's no real need for FFT in formula mod...

@nuoun
Copy link
Contributor Author

nuoun commented Dec 19, 2024

So I'd like to add a new tutorial Patch for this but the shared table is actually the same across all voices and Formula LFO instances, this is how it always was with the Surge function table being set on every new setSurgeFunctionEnvironment(). I just moved that functionality to a dedicated table that gets wiped when a new script gets applied and made it so that the global tables can't be overwritten permanently.

A simple one time lazy initialization like this works and anything set in the table can be accessed across voices and Formula's.

function init(state)
    if not shared.init then
        shared.init = true
        -- stuff that runs just once goes here
    end
end

@baconpaul
Copy link
Collaborator

curious why that approach vs a separate function initshared? shared is a global per script though?

@nuoun
Copy link
Contributor Author

nuoun commented Dec 20, 2024

So the shared table got added in #7727 (see #7727 (comment)) but we could rethink it.

@nuoun
Copy link
Contributor Author

nuoun commented Dec 20, 2024

Alright, how is this looking so far?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants