diff --git a/src/wrapper/clap/context.rs b/src/wrapper/clap/context.rs index 67e182f4..e6b0a27d 100644 --- a/src/wrapper/clap/context.rs +++ b/src/wrapper/clap/context.rs @@ -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

, } diff --git a/src/wrapper/standalone/context.rs b/src/wrapper/standalone/context.rs index 72ed7a46..bcb45965 100644 --- a/src/wrapper/standalone/context.rs +++ b/src/wrapper/standalone/context.rs @@ -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, } diff --git a/src/wrapper/vst3/context.rs b/src/wrapper/vst3/context.rs index e42cab99..e9efc50f 100644 --- a/src/wrapper/vst3/context.rs +++ b/src/wrapper/vst3/context.rs @@ -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

, + 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>, } /// A [`ProcessContext`] implementation for the wrapper. This is a separate object so it can hold on @@ -38,6 +54,14 @@ pub(crate) struct WrapperGuiContext { pub(super) inner: Arc>, } +impl 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 InitContext

for WrapperInitContext<'_, P> { fn plugin_api(&self) -> PluginApi { PluginApi::Vst3 @@ -48,7 +72,8 @@ impl InitContext

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) { diff --git a/src/wrapper/vst3/inner.rs b/src/wrapper/vst3/inner.rs index e0df47db..e8998fda 100644 --- a/src/wrapper/vst3/inner.rs +++ b/src/wrapper/vst3/inner.rs @@ -362,8 +362,15 @@ impl WrapperInner

{ 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 WrapperInner

{ 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()); } diff --git a/src/wrapper/vst3/wrapper.rs b/src/wrapper/vst3/wrapper.rs index 0a7095a5..d0166ee4 100644 --- a/src/wrapper/vst3/wrapper.rs +++ b/src/wrapper/vst3/wrapper.rs @@ -388,28 +388,11 @@ impl IComponent for Wrapper

{ 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 IComponent for Wrapper

{ // 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 IAudioProcessor for Wrapper

{ 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 IAudioProcessor for Wrapper

{ // 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