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

Reworked AudioBuffer, removed memory allocations in the audio thread,… #32

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Boscop
Copy link
Contributor

@Boscop Boscop commented Jun 13, 2017

… also in process_events(), eliminated Box transmutes where possible etc.

This addresses #28 and other things.
The diff might look confusing (especially for buffer.rs) so I'll explain the changes here:

  • Before, AudioBuffer allocated in every frame due to the collect()s in from_raw(). Now, AudioBuffer doesn't allocate anymore. I modified the tests to use the new AudioBuffer.
    When you split an AudioBuffer you get zero overhead iterable types Inputs and Outputs that behave like slices, you can also zip the buffer directly. As you can see in the changes to the examples, both ways work with minimal changes to existing plugins.
  • I also removed the allocations in process_events() mentioned in that issue, by implementing a function api::Events::events() for iterating over midi events when receiving and implementing a SendEventBuffer for sending which reuses the memory and only allocates during new() for a given maximum of midi events. Only plugins that send midi to the host need to use this (and hosts sending midi to plugins). Unfortunately impl Trait is still not in stable, so api::Events::events() which returns impl Iterator only works on nightly (and the type couldn't be expressed by writing it out because it's a Map with an anonymous closure type). But this function could be in a nightly feature (#[cfg(feature = "nightly")]), then we could make events_raw() public for people on stable so the user code would use that and do the .map(..) part manually, like:
use event::Event;
use api::Events;
fn process_events(&mut self, events: &Events) {
    for &e in events.events_raw() {
        let event: Event = Event::from(unsafe { *e });
        match event {
            // ...
        }
    }
}

But on nightly it's:

fn process_events(&mut self, events: &Events) {
    for event in events.events() {
        match event {
            // ...
        }
    }
}

Btw, I tested this in some example plugins, also in the sine_synth example.

  • Changed the examples to use the current crate instead of pulling it from github, by using relative paths. Also changed crate-type to cdylib, because that's what it should be. Also makes the resulting dlls smaller and more optimized.
  • Changed midi_note_to_hz() in the sine_synth example to be more intuitive (and with one less division).
  • Changed ProcessProc and ProcessProcF64 to take the input buffer pointers as immutable. I always make the rust pointers for ffi functions immutable if that's the intended usage, even when the C lib didn't do this, because it's an oversight in the C lib and it makes the Rust code stricter/better because in this case AudioBuffer doesn't need to have mutable references to the inputs.
  • Changed Box transmutes to Box::into_raw() and from_raw() in AEffect::get_plugin() and lib::main() because it's more idiomatic (transmutes should be avoided).
  • Added a PluginCache. There was a huge inefficiency in interfaces::process_replacing() and process_replacing_f64() because it called plugin.get_info() on EVERY frame to get the number of inputs/outputs and whether it supports f64 precision. First of all, that check for f64 precision isn't necessary. I asked more experienced VST plugin devs, they said that the host won't even call this function if the plugin doesn't support f64 precision processing (which they know from calling get_info() when loading a plugin and caching that), and even if, that should be a NOP. Secondly and more importantly, Info contains multiple Strings and each of them allocates every time get_info() is called. This is very wasteful. Now the Info gets called only during plugin initialization and cached in the PluginCache which gets stored in the AEffect::object user data pointer.
    The goal is to have NO allocations in the audio thread at all.

Overall the API didn't change much at all:

  • Plugin::process() now takes a &mut AudioBuffer<f32> instead of immutable. Same for the f64 variant.
  • Plugin::process_events() now takes &api::Events instead of Vec<event::Event> and uses the zero allocation iterator mentioned above.
  • Sending midi events is now done with the SendEventBuffer, I added a usage example to the doc.

I made sure the tests succeed and changed the examples in the documentation to match the new API and also tested the audio and midi processing with actual VSTs.

… also in process_events(), eliminated Box transmutes where possible etc
@Boscop
Copy link
Contributor Author

Boscop commented Jun 13, 2017

Ah, the CI builds are failing because they are on stable.
Should I do the suggestion above?
(Make api::Events::events_raw() public and make api::Events::events() conditionally available with a nightly feature?)

@zyvitski
Copy link
Contributor

I would vote to add the feature gate. Aside from that Evrything looks good to me.

@Boscop
Copy link
Contributor Author

Boscop commented Jun 14, 2017

Right now the builds fail on stable because of the pub(crate) syntax, which isn't on stable yet..
Should I make those symbols completely public?

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.

None yet

2 participants