Split plugin API to make it thread safe. #49

[askeksa]

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:

• The proposal changes the Host trait to take all self parameters 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 by automate or process_events). Alternatively, the callback could be split in two, like the Plugin trait, 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.
• Some of the documentation could be extended to describe the new overall structure more clearly. I have tried to do that somewhat in the gain_effect example, 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.

[askeksa]

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.

[crsaracco]

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?

[askeksa]

Thank you for the review @crsaracco.

I have moved the get_editor method to the Plugin trait. Since the plugin instantiation code in lib.rs calls get_editor right away after creating the plugin, and then never again, it is safe to have it there, and much more convenient for the plugin.

As for including AtomicFloat: It is my impression that the consensus among the developers of this crate is that it should be a minimal, safe wrapper around the VST2 API. As such, it should not be opinionated about how communication is performed; it should just make sure that as long as the user sticks to safe code (and the host is well-behaved), soundness is guaranteed (which currently is not the case). So I think the example code is the right place for AtomicFloat.

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.

[askeksa]

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:

• There seems to be general agreement that this is a useful way to structure the API, and that it introduces a minimum amount of breakage under the constraints of soundness.
• One thorough review, including judging the soundness of the implementation, has been performed by @crsaracco, and @raphlinus has offered to do a review as well.
• The new API has been battle tested in this plugin, which also includes host callback code.
• The threading assumptions have been verified using the concurrency tester in Ableton Live, Bitwig Studio, Cubase, Reaper and Renoise.
• The documentation for the traits themselves has been updated to explain the new way of structuring a plugin. All of the examples have been updated, and additional documentation has been written in the examples to explain the less obvious parts of the parameter communication.

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.

[PieterPenninckx]

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).

[askeksa]

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) rust-vst crate easier to use.

[PieterPenninckx]

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.

[askeksa]

Yes, the parameter values need to be available in the shared PluginParameters object so they can be read by get_parameter (and the other parameter methods). Both threads have access to them there, so you would usually only need an extra copy in the Plugin object itself if you need to do some further processing on them, such as smoothing.