alex/nih-plug
1
0
Fork 0
Code Activity

Defer set_latency_samples() in VST3 plugin init

Browse source 
The host might otherwise restart the plugin while the `Plugin::init()`
call is still active, resulting in a deadlock because the plugin mutex
is still locked. This was causing issues when loading state in Ardour7.
The new approach also removes the need for a previous hack added for
Ardour6.
This commit is contained in:
Robbert van der Helm 2022-12-28 01:28:33 +01:00
parent 46752fc7f0
commit fd28a95231
5 changed files with 53 additions and 46 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/wrapper/clap/context.rs
View file

@ -12,9 +12,7 @@ use crate::midi::NoteEvent;
use crate::params::internals::ParamPtr;
use crate::plugin::ClapPlugin;
/// A [`InitContext`] implementation for the wrapper. This is a separate object so it can hold on
/// to lock guards for event queues. Otherwise reading these events would require constant
/// unnecessary atomic operations to lock the uncontested RwLocks.
/// An [`InitContext`] implementation for the wrapper.
pub(crate) struct WrapperInitContext<'a, P: ClapPlugin> {
pub(super) wrapper: &'a Wrapper<P>,
}

4
src/wrapper/standalone/context.rs
View file

@ -12,9 +12,7 @@ use crate::midi::NoteEvent;
use crate::params::internals::ParamPtr;
use crate::plugin::Plugin;
/// A [`InitContext`] implementation for the standalone wrapper. This is a separate object so it
/// can hold on to lock guards for event queues. Otherwise reading these events would require
/// constant unnecessary atomic operations to lock the uncontested RwLocks.
/// An [`InitContext`] implementation for the standalone wrapper.
pub(crate) struct WrapperInitContext<'a, P: Plugin, B: Backend> {
pub(super) wrapper: &'a Wrapper<P, B>,
}

33
src/wrapper/vst3/context.rs
View file

@ -1,4 +1,5 @@
use atomic_refcell::AtomicRefMut;
use std::cell::Cell;
use std::collections::VecDeque;
use std::sync::atomic::Ordering;
use std::sync::Arc;
@ -14,11 +15,26 @@ use crate::params::internals::ParamPtr;
use crate::plugin::Vst3Plugin;
use crate::wrapper::state::PluginState;
/// A [`InitContext`] implementation for the wrapper. This is a separate object so it can hold on to
/// lock guards for event queues. Otherwise reading these events would require constant unnecessary
/// atomic operations to lock the uncontested locks.
/// An [`InitContext`] implementation for the wrapper.
///
/// # Note
///
/// Requests to change the latency are only sent when this object is dropped. Otherwise there's the
/// risk that the host will immediately deactivate/reactivate the plugin while still in the init
/// call. Reentrannt function calls are difficult to handle in Rust without forcing everything to
/// use interior mutability, so this will have to do for now. This does mean that `Plugin` mutex
/// lock has to be dropped before this object.
pub(crate) struct WrapperInitContext<'a, P: Vst3Plugin> {
pub(super) inner: &'a WrapperInner<P>,
pub(super) pending_requests: PendingInitContextRequests,
}
/// Any requests that should be sent out when the [`WrapperInitContext`] is dropped. See that
/// struct's docstring for mroe information.
#[derive(Debug, Default)]
pub(crate) struct PendingInitContextRequests {
/// The value of the last `.set_latency_samples()` call.
latency_changed: Cell<Option<u32>>,
}
/// A [`ProcessContext`] implementation for the wrapper. This is a separate object so it can hold on
@ -38,6 +54,14 @@ pub(crate) struct WrapperGuiContext<P: Vst3Plugin> {
pub(super) inner: Arc<WrapperInner<P>>,
}
impl<P: Vst3Plugin> Drop for WrapperInitContext<'_, P> {
fn drop(&mut self) {
if let Some(samples) = self.pending_requests.latency_changed.take() {
self.inner.set_latency_samples(samples)
}
}
}
impl<P: Vst3Plugin> InitContext<P> for WrapperInitContext<'_, P> {
fn plugin_api(&self) -> PluginApi {
PluginApi::Vst3
@ -48,7 +72,8 @@ impl<P: Vst3Plugin> InitContext<P> for WrapperInitContext<'_, P> {
}
fn set_latency_samples(&self, samples: u32) {
self.inner.set_latency_samples(samples)
// See this struct's docstring
self.pending_requests.latency_changed.set(Some(samples));
}
fn set_current_voice_capacity(&self, _capacity: u32) {

13
src/wrapper/vst3/inner.rs
View file

@ -362,8 +362,15 @@ impl<P: Vst3Plugin> WrapperInner<P> {
Arc::new(WrapperGuiContext { inner: self })
}
/// # Note
///
/// The lock on the plugin must be dropped before this object is dropped to avoid deadlocks
/// caused by reentrant function calls.
pub fn make_init_context(&self) -> WrapperInitContext<'_, P> {
WrapperInitContext { inner: self }
WrapperInitContext {
inner: self,
pending_requests: Default::default(),
}
}
pub fn make_process_context(&self, transport: Transport) -> WrapperProcessContext<'_, P> {
@ -521,8 +528,10 @@ impl<P: Vst3Plugin> WrapperInner<P> {
self.notify_param_values_changed();
let bus_config = self.current_bus_config.load();
if let Some(buffer_config) = self.current_buffer_config.load() {
// NOTE: This needs to be dropped after the `plugin` lock to avoid deadlocks
let mut init_context = self.make_init_context();
let mut plugin = self.plugin.lock();
plugin.initialize(&bus_config, &buffer_config, &mut self.make_init_context());
plugin.initialize(&bus_config, &buffer_config, &mut init_context);
process_wrapper(|| plugin.reset());
}

45
src/wrapper/vst3/wrapper.rs
View file

@ -388,28 +388,11 @@ impl<P: Vst3Plugin> IComponent for Wrapper<P> {
param.update_smoother(buffer_config.sample_rate, true);
}
// HACK: This is needed because if you change the latency during
// `IComponent::setActive(true)` then Ardour will reentrantly call
// `IComponent::setActive(true)` again during the previous call. This use of `static`
// is also fine here because the host may only call this from the main thread, so
// multiple simultaneous calls of this function are not allowed.
let mut plugin = match self.inner.plugin.try_lock() {
Some(plugin) => plugin,
None => {
nih_debug_assert_failure!(
"The host tried to call IComponent::setActive(true) while it was \
already calling IComponent::setActive(true), returning kResultOk"
);
return kResultOk;
}
};
// NOTE: This needs to be dropped after the `plugin` lock to avoid deadlocks
let mut init_context = self.inner.make_init_context();
let bus_config = self.inner.current_bus_config.load();
if plugin.initialize(
&bus_config,
&buffer_config,
&mut self.inner.make_init_context(),
) {
let mut plugin = self.inner.plugin.lock();
if plugin.initialize(&bus_config, &buffer_config, &mut init_context) {
// NOTE: We don't call `Plugin::reset()` here. The call is done in `set_process()`
// instead. Otherwise we would call the function twice, and `set_process()` needs
// to be called after this function before the plugin may process audio again.
@ -532,14 +515,12 @@ impl<P: Vst3Plugin> IComponent for Wrapper<P> {
// Reinitialize the plugin after loading state so it can respond to the new parameter values
self.inner.notify_param_values_changed();
let bus_config = self.inner.current_bus_config.load();
if let Some(buffer_config) = self.inner.current_buffer_config.load() {
// NOTE: This needs to be dropped after the `plugin` lock to avoid deadlocks
let mut init_context = self.inner.make_init_context();
let bus_config = self.inner.current_bus_config.load();
let mut plugin = self.inner.plugin.lock();
plugin.initialize(
&bus_config,
&buffer_config,
&mut self.inner.make_init_context(),
);
plugin.initialize(&bus_config, &buffer_config, &mut init_context);
// TODO: This also goes for the CLAP version, but should we call reset here? Won't the
// host always restart playback? Check this with a couple of hosts and remove the
// duplicate reset if it's not needed.
@ -1815,6 +1796,8 @@ impl<P: Vst3Plugin> IAudioProcessor for Wrapper<P> {
self.inner.notify_param_values_changed();
// NOTE: This needs to be dropped after the `plugin` lock to avoid deadlocks
let mut init_context = self.inner.make_init_context();
let bus_config = self.inner.current_bus_config.load();
let buffer_config = self.inner.current_buffer_config.load().unwrap();
let mut plugin = self.inner.plugin.lock();
@ -1822,13 +1805,7 @@ impl<P: Vst3Plugin> IAudioProcessor for Wrapper<P> {
// this could lead to inconsistencies. It's the plugin's responsibility to
// not perform any realtime-unsafe work when the initialize function is
// called a second time if it supports runtime preset loading.
permit_alloc(|| {
plugin.initialize(
&bus_config,
&buffer_config,
&mut self.inner.make_init_context(),
)
});
permit_alloc(|| plugin.initialize(&bus_config, &buffer_config, &mut init_context));
plugin.reset();
// We'll pass the state object back to the GUI thread so deallocation can happen