Split plugin API to make it thread safe. #49 #65
Conversation
|
I have added some more documentation to some of the methods and traits. This should make it easier for users of the crate to port their code to the new API. |
There was a problem hiding this comment.
This changeset looks pretty good to me. It looks like the biggest change a plugin creator would have to know about is the new PluginParameters trait, and which methods go in which trait -- overall not too big of a change.
(Side-note: maybe we can make some official tutorials like Joe Clay and Doomy have on their blogs, and put them in the repo/wiki somewhere -- make it super-easy for new users to get up-to-speed with the crate as a whole)
The one point I'm not too sure about (as discussed in the Telegram chat) is the inclusion of get_editor within the PluginParameters trait, but it should work either way. Plus, we could change it later as we implement the "Proper editor support" TODO, as we learn what we actually want the Editor API to look like.
[Edit]
I should add the most important part: It also looks safe from an audio-processing perspective, as far as I can tell. Users will have to know to use AtomicFloat instead of some RefCell<f32>, then doing some janky *self.parameter.as_ptr() = val inside an unsafe block -- more of a reason to make some tutorial or something.
Should we include AtomicFloat inside the library instead of just including it in the examples?
|
Thank you for the review @crsaracco. I have moved the As for including And yes, more tutorials and examples will definitely help. It could be good to have an example that communicates via some other mechanism than atomics, though that is tricky to do if you at the same time require the processing thread to be non-blocking and non-allocating. |
|
Can we get a resolution on this change? The number of users of the crate is growing. That's a good thing, but it also means that the longer we wait before performing breaking changes like this, the more people will need to adapt their code. To summarize the status:
More documentation is desirable (in the form of tutorials, for example), but this can be added along the way independently of this PR. Likewise, the change here does not improve the somewhat lacking unit test situation by much, but this is also something that can be improved independently. Some critical threading tests can't even be implemented until this is in place. |
|
I quickly went trough the PR to see if it allows for an abstraction layer (VST, LV2, ...). I basically checked if it is possible to translate a "parameter change" into some kind of event and give that to the plugin. I didn't find anything blocking. I would have to try to be sure, of course, but on a first glance it looks good. I like the comments that explain the states (suspended/resumed) (assuming these are correct). |
|
Prompted by @wrl, I implemented a proof-of-concept data structure for lock-free and allocation-free transfer of parameter change events to the processing thread. This can be seen, along with an example synth with simple parameter smoothing, here. This illustrates the kind of (opinionated) convenience utility code that we could provide to make the (unopinionated but safe) |
|
I tried implementing a simple plugin with message passing instead of atomics, but my attempt failed because -- if I understand it correctly -- the parameters can be read from both threads. I guess the hardest problems for writing an abstraction layer will not be the code from this PR, but the design of VST itself. |
|
Yes, the parameter values need to be available in the shared |
These will be used by the examples in #65.
This is a proposal for a restructuring of the plugin API to fix #49 and make the crate safe.
It is a breaking change. It requires some substantial changes to any plugin that uses parameters, and some slight changes if it uses the editor API or host callback. The processing part of the API and basic plugin setup and querying are unchanged.
In addition to the analysis described in the issue, it has been battle tested by porting the Oidos synth to it. The necessary changes can be seen in this branch. The synth illustrates some non-trivial parameter handling, where the parameters are transformed into an internal form which also depends on the sample rate. The generated sound is cached to save computation time, and whenever a parameter or the sample rate changes, the cache must be flushed.
Oidos does not have strict real-time requirements, so the parameters can simply be held behind a
RwLock. The API as proposed leaves the choice of synchronization open, as described in the issue.Some open items that have not been discussed in the issue:
Hosttrait to take allselfparameters by immutable reference, so the callback can be safely shared between the plugin and the parameter object. This conversely requires the host to employ thread-safe interior mutability for all state manipulated through the callback (such as byautomateorprocess_events). Alternatively, the callback could be split in two, like thePlugintrait, such that all functions only callable by the processing thread (process_events, maybe others) get their own callback handle which is only available to the main plugin.gain_effectexample, but more could be done here. Perhaps a separate porting guide to help transition the existing plugins could also be relevant.I have left the commits on the branch separate for now, but I would suggest they are squashed when merged, as they don't make much sense individually.