From 652e469c1e0b58d99b3bbea50f0ea0faee3483a3 Mon Sep 17 00:00:00 2001 From: Benjamin Klum Date: Fri, 27 Dec 2024 22:33:27 +0100 Subject: [PATCH] #1376 Attempt Improve vst-rs by placing catch_unwind around all calls from C, even shutdown. It shouldn't cause a hard crash at least. --- Cargo.lock | 3 +- Cargo.toml | 8 +- .../infrastructure/plugin/helgobox_plugin.rs | 243 ++++++++---------- .../plugin/helgobox_plugin_editor.rs | 18 +- .../plugin/instance_parameter_container.rs | 112 ++++---- 5 files changed, 172 insertions(+), 212 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d6bf87f0e..89fc113f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8067,8 +8067,7 @@ checksum = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d" [[package]] name = "vst" -version = "0.3.0" -source = "git+https://github.com/helgoboss/vst-rs.git?branch=feature/param-props#679613bc17c39817ba92284c61065d7ae7dc3761" +version = "0.4.0" dependencies = [ "bitflags 1.3.2", "libc", diff --git a/Cargo.toml b/Cargo.toml index 5c2d88711..bb93e287a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -139,7 +139,7 @@ webbrowser = "0.8.12" runas = "1.1.0" qrcode = "0.14.1" uuid = "1.6.1" -vst = "0.3.0" +vst = "0.4.0" c_str_macro = "1.0.2" arboard = "3.3.0" smallvec = "1.7.0" @@ -247,9 +247,9 @@ debug = 1 # time! Turns out the same is actually true for axum, so we use select! there as well. #hyper = { git = "https://github.com/helgoboss/hyper.git", branch = "feature/realearn" } -# TODO-low-wait Wait until https://github.com/RustAudio/vst-rs/issues/184 merged. -vst = { git = "https://github.com/helgoboss/vst-rs.git", branch = "feature/param-props" } -#vst = { path = "../vst-rs" } +# We need to use our on "vst" crate that contains a bunch of improvements +#vst = { git = "https://github.com/helgoboss/vst-rs.git", branch = "feature/param-props" } +vst = { path = "../vst-rs" } # This is for temporary development with local reaper-rs. #[patch.'https://github.com/helgoboss/reaper-rs.git'] diff --git a/main/src/infrastructure/plugin/helgobox_plugin.rs b/main/src/infrastructure/plugin/helgobox_plugin.rs index 721c4bb03..0a8b26b29 100644 --- a/main/src/infrastructure/plugin/helgobox_plugin.rs +++ b/main/src/infrastructure/plugin/helgobox_plugin.rs @@ -19,7 +19,6 @@ use reaper_medium::{Hz, ReaperStr}; use std::ffi::{CStr, CString}; use std::os::raw::{c_char, c_void}; -use std::panic::{catch_unwind, AssertUnwindSafe}; use std::sync::{Arc, OnceLock}; @@ -97,26 +96,23 @@ unsafe impl Send for HelgoboxPlugin {} impl Plugin for HelgoboxPlugin { fn new(host: HostCallback) -> Self { - firewall(|| { - let instance_id = InstanceId::next(); - Self { - instance_id, - host, - _reaper_guard: None, - param_container: Arc::new(InstanceParameterContainer::new()), - was_playing_in_last_cycle: false, - sample_rate: Default::default(), - block_size: 0, - is_plugin_scan: false, - lazy_data: OnceLock::new(), - instance_panel: Rc::new(InstancePanel::new(instance_id)), - } - }) - .unwrap_or_default() + let instance_id = InstanceId::next(); + Self { + instance_id, + host, + _reaper_guard: None, + param_container: Arc::new(InstanceParameterContainer::new()), + was_playing_in_last_cycle: false, + sample_rate: Default::default(), + block_size: 0, + is_plugin_scan: false, + lazy_data: OnceLock::new(), + instance_panel: Rc::new(InstancePanel::new(instance_id)), + } } fn get_info(&self) -> Info { - firewall(|| Info { + Info { name: "Helgobox - ReaLearn & Playtime".to_string(), vendor: "Helgoboss".to_string(), unique_id: HELGOBOX_UNIQUE_VST_PLUGIN_ID, @@ -127,8 +123,7 @@ impl Plugin for HelgoboxPlugin { inputs: 2, outputs: 0, ..Default::default() - }) - .unwrap_or_default() + } } fn get_parameter_info(&self, index: i32) -> Option { @@ -146,69 +141,62 @@ impl Plugin for HelgoboxPlugin { } fn init(&mut self) { - firewall(|| { - // Trick to find out whether we are only being scanned. - self.is_plugin_scan = unsafe { (*self.host.raw_effect()).reserved1 == 0 }; - if self.is_plugin_scan { - tracing::debug!("Helgobox is being scanned by REAPER"); - return; - } - tracing::debug!("Helgobox is being opened by REAPER"); - self._reaper_guard = Some(self.ensure_reaper_setup()); - // At this point, REAPER cannot reliably give us the containing FX. As a - // consequence we also don't have a instance shell yet, because creating an incomplete - // instance shell pushes the problem of not knowing the containing FX into the application - // logic, which we for sure don't want. In the next main loop cycle, it should be - // possible to identify the containing FX. - let host = self.host; - Global::task_support() - .do_later_in_main_thread_from_main_thread_asap(move || { - let plugin = unsafe { (*host.raw_effect()).get_plugin() }; - plugin.vendor_specific(INIT_INSTANCE_SHELL, 0, null_mut(), 0.0); - }) - .unwrap(); - }); + // Trick to find out whether we are only being scanned. + self.is_plugin_scan = unsafe { (*self.host.raw_effect()).reserved1 == 0 }; + if self.is_plugin_scan { + tracing::debug!("Helgobox is being scanned by REAPER"); + return; + } + tracing::debug!("Helgobox is being opened by REAPER"); + self._reaper_guard = Some(self.ensure_reaper_setup()); + // At this point, REAPER cannot reliably give us the containing FX. As a + // consequence we also don't have a instance shell yet, because creating an incomplete + // instance shell pushes the problem of not knowing the containing FX into the application + // logic, which we for sure don't want. In the next main loop cycle, it should be + // possible to identify the containing FX. + let host = self.host; + Global::task_support() + .do_later_in_main_thread_from_main_thread_asap(move || { + let plugin = unsafe { (*host.raw_effect()).get_plugin() }; + plugin.vendor_specific(INIT_INSTANCE_SHELL, 0, null_mut(), 0.0); + }) + .unwrap(); } fn get_editor(&mut self) -> Option> { - firewall(|| { - // Unfortunately, vst-rs calls `get_editor` before the plug-in is initialized by the - // host, e.g. in order to check if it should the hasEditor flag or not. That means - // we don't know yet if this is a plug-in scan or not. We have to create the editor. - let boxed: Box = - Box::new(HelgoboxPluginEditor::new(self.instance_panel.clone())); - Some(boxed) - }) - .unwrap_or(None) + // Unfortunately, vst-rs calls `get_editor` before the plug-in is initialized by the + // host, e.g. in order to check if it should the hasEditor flag or not. That means + // we don't know yet if this is a plug-in scan or not. We have to create the editor. + let boxed: Box = + Box::new(HelgoboxPluginEditor::new(self.instance_panel.clone())); + Some(boxed) } fn can_do(&self, can_do: CanDo) -> Supported { - firewall(|| { - use CanDo::*; - use Supported::*; - #[allow(overflowing_literals)] - match can_do { - SendEvents | SendMidiEvent | ReceiveEvents | ReceiveMidiEvent - | ReceiveSysExEvent => Supported::Yes, - // If we don't do this, REAPER for Linux won't give us a SWELL plug-in window, which - // leads to a horrible crash when doing CreateDialogParam. In our UI we use SWELL - // to put controls into the plug-in window. SWELL assumes that the parent window for - // controls is also a SWELL window. - Other(s) => match s.as_str() { - "hasCockosViewAsConfig" => Custom(0xbeef_0000), - "hasCockosExtensions" => Custom(0xbeef_0000), - // This is necessary for REAPER 6.48 - 6.51 on macOS to not let the background - // turn black. These REAPER versions introduced a change putting third-party - // VSTs into a container window. The following line prevents that. For - // REAPER v6.52+ it's not necessary anymore because it also reacts to - // "hasCockosViewAsConfig". - "hasCockosNoScrollUI" => Custom(0xbeef_0000), - _ => Maybe, - }, - _ => Maybe, + use CanDo::*; + use Supported::*; + #[allow(overflowing_literals)] + match can_do { + SendEvents | SendMidiEvent | ReceiveEvents | ReceiveMidiEvent | ReceiveSysExEvent => { + Supported::Yes } - }) - .unwrap_or(Supported::No) + // If we don't do this, REAPER for Linux won't give us a SWELL plug-in window, which + // leads to a horrible crash when doing CreateDialogParam. In our UI we use SWELL + // to put controls into the plug-in window. SWELL assumes that the parent window for + // controls is also a SWELL window. + Other(s) => match s.as_str() { + "hasCockosViewAsConfig" => Custom(0xbeef_0000), + "hasCockosExtensions" => Custom(0xbeef_0000), + // This is necessary for REAPER 6.48 - 6.51 on macOS to not let the background + // turn black. These REAPER versions introduced a change putting third-party + // VSTs into a container window. The following line prevents that. For + // REAPER v6.52+ it's not necessary anymore because it also reacts to + // "hasCockosViewAsConfig". + "hasCockosNoScrollUI" => Custom(0xbeef_0000), + _ => Maybe, + }, + _ => Maybe, + } } fn get_parameter_object(&mut self) -> Arc { @@ -216,74 +204,65 @@ impl Plugin for HelgoboxPlugin { } fn vendor_specific(&mut self, index: i32, value: isize, ptr: *mut c_void, opt: f32) -> isize { - firewall(|| { - // tracing_debug!("VST vendor specific (index = {})", index); - self.handle_vendor_specific(index, value, ptr, opt) - }) - .unwrap_or(0) + // tracing_debug!("VST vendor specific (index = {})", index); + self.handle_vendor_specific(index, value, ptr, opt) } fn process_events(&mut self, events: &Events) { - firewall(|| { - assert_no_alloc(|| { - let is_transport_start = !self.was_playing_in_last_cycle && self.is_now_playing(); - let block_count = GLOBAL_AUDIO_STATE.load_block_count(); - let sample_count = block_count * self.block_size as u64; - let device_sample_rate = GLOBAL_AUDIO_STATE.load_sample_rate(); - for e in events.events() { - let our_event = match MidiEvent::from_vst(e) { - Err(_) => { - // Just ignore if not a valid MIDI message. Invalid MIDI message was - // observed in the wild: https://github.com/helgoboss/helgobox/issues/82. - continue; - } - Ok(e) => e, - }; - let timestamp = ControlEventTimestamp::from_rt( - sample_count, - device_sample_rate, - our_event.offset().to_seconds(self.sample_rate), - ); - let our_event = ControlEvent::new(our_event, timestamp); - if let Some(lazy_data) = self.lazy_data.get() { - lazy_data.instance_shell.process_incoming_midi_from_plugin( - our_event, - is_transport_start, - self.host, - ); + assert_no_alloc(|| { + let is_transport_start = !self.was_playing_in_last_cycle && self.is_now_playing(); + let block_count = GLOBAL_AUDIO_STATE.load_block_count(); + let sample_count = block_count * self.block_size as u64; + let device_sample_rate = GLOBAL_AUDIO_STATE.load_sample_rate(); + for e in events.events() { + let our_event = match MidiEvent::from_vst(e) { + Err(_) => { + // Just ignore if not a valid MIDI message. Invalid MIDI message was + // observed in the wild: https://github.com/helgoboss/helgobox/issues/82. + continue; } + Ok(e) => e, + }; + let timestamp = ControlEventTimestamp::from_rt( + sample_count, + device_sample_rate, + our_event.offset().to_seconds(self.sample_rate), + ); + let our_event = ControlEvent::new(our_event, timestamp); + if let Some(lazy_data) = self.lazy_data.get() { + lazy_data.instance_shell.process_incoming_midi_from_plugin( + our_event, + is_transport_start, + self.host, + ); } - }); + } }); } fn process_f64(&mut self, buffer: &mut AudioBuffer) { - firewall(|| { - assert_no_alloc(|| { - // Get current time information so we can detect changes in play state reliably - // (TimeInfoFlags::TRANSPORT_CHANGED doesn't work the way we want it). - self.was_playing_in_last_cycle = self.is_now_playing(); - if let Some(lazy_data) = self.lazy_data.get() { - #[cfg(feature = "playtime")] - lazy_data.instance_shell.run_playtime_from_plugin( - buffer, - crate::domain::AudioBlockProps::from_vst(buffer, self.sample_rate), - ); - lazy_data.instance_shell.run_from_plugin(self.host); - } - }); + assert_no_alloc(|| { + // Get current time information so we can detect changes in play state reliably + // (TimeInfoFlags::TRANSPORT_CHANGED doesn't work the way we want it). + self.was_playing_in_last_cycle = self.is_now_playing(); + if let Some(lazy_data) = self.lazy_data.get() { + #[cfg(feature = "playtime")] + lazy_data.instance_shell.run_playtime_from_plugin( + buffer, + crate::domain::AudioBlockProps::from_vst(buffer, self.sample_rate), + ); + lazy_data.instance_shell.run_from_plugin(self.host); + } }); let _ = buffer; } fn set_sample_rate(&mut self, rate: f32) { - firewall(|| { - tracing::debug!("VST set sample rate"); - self.sample_rate = Hz::new_panic(rate as _); - if let Some(lazy_data) = self.lazy_data.get() { - lazy_data.instance_shell.set_sample_rate(rate); - } - }); + tracing::debug!("VST set sample rate"); + self.sample_rate = Hz::new_panic(rate as _); + if let Some(lazy_data) = self.lazy_data.get() { + lazy_data.instance_shell.set_sample_rate(rate); + } } fn suspend(&mut self) { @@ -553,10 +532,6 @@ fn write_to_c_str(dest: *mut c_void, src: String) -> Result<(), &'static str> { Ok(()) } -fn firewall R, R>(f: F) -> Option { - catch_unwind(AssertUnwindSafe(f)).ok() -} - /// This is our own code. We call ourselves in order to safe us an Arc around /// the instance shell. Why use an Arc (and therefore make each internal access to the instance shell have to /// dereference a pointer) if we already have a pointer from outside. diff --git a/main/src/infrastructure/plugin/helgobox_plugin_editor.rs b/main/src/infrastructure/plugin/helgobox_plugin_editor.rs index 0171d321e..f2881f3e1 100644 --- a/main/src/infrastructure/plugin/helgobox_plugin_editor.rs +++ b/main/src/infrastructure/plugin/helgobox_plugin_editor.rs @@ -1,4 +1,3 @@ -use reaper_low::firewall; use reaper_low::raw::HWND; use std::os::raw::c_void; @@ -21,7 +20,7 @@ impl HelgoboxPluginEditor { impl Editor for HelgoboxPluginEditor { fn size(&self) -> (i32, i32) { - firewall(|| self.instance_panel.dimensions().to_vst()).unwrap_or_default() + self.instance_panel.dimensions().to_vst() } fn position(&self) -> (i32, i32) { @@ -29,20 +28,17 @@ impl Editor for HelgoboxPluginEditor { } fn close(&mut self) { - firewall(|| self.instance_panel.close()); + self.instance_panel.close(); } fn open(&mut self, parent: *mut c_void) -> bool { - firewall(|| { - self.instance_panel - .clone() - .open_with_resize(Window::new(parent as HWND).expect("no parent window")); - true - }) - .unwrap_or(false) + self.instance_panel + .clone() + .open_with_resize(Window::new(parent as HWND).expect("no parent window")); + true } fn is_open(&mut self) -> bool { - firewall(|| self.instance_panel.is_open()).unwrap_or(false) + self.instance_panel.is_open() } } diff --git a/main/src/infrastructure/plugin/instance_parameter_container.rs b/main/src/infrastructure/plugin/instance_parameter_container.rs index e5501b4c1..b4642d316 100644 --- a/main/src/infrastructure/plugin/instance_parameter_container.rs +++ b/main/src/infrastructure/plugin/instance_parameter_container.rs @@ -139,88 +139,78 @@ const NOT_READY_YET: &str = "not-ready-yet"; impl PluginParameters for InstanceParameterContainer { fn get_bank_data(&self) -> Vec { - firewall(|| { - let Some(lazy_data) = self.lazy_data.get() else { - return match self.pending_data_to_be_loaded.read().unwrap().as_ref() { - None => NOT_READY_YET.to_string().into_bytes(), - Some(d) => d.clone(), - }; + let Some(lazy_data) = self.lazy_data.get() else { + return match self.pending_data_to_be_loaded.read().unwrap().as_ref() { + None => NOT_READY_YET.to_string().into_bytes(), + Some(d) => d.clone(), }; - lazy_data - .instance_shell - .upgrade() - .expect("instance shell gone") - .save() - }) - .unwrap_or_default() + }; + lazy_data + .instance_shell + .upgrade() + .expect("instance shell gone") + .save() } fn load_bank_data(&self, data: &[u8]) { - firewall(|| { - // TODO-medium-performance We could optimize by getting the config var only once, saving it in a global - // struct and then just dereferencing the var whenever we need it. Justin said that the result of - // get_config_var never changes throughout the lifetime of REAPER. - if let Ok(pref) = Reaper::get().get_preference_ref::("__fx_loadstate_ctx") { - if *pref == b'U' { - // REAPER is loading an updated undo state. We don't want to participate in REAPER's undo because - // it often leads to unpleasant surprises. ReaLearn is its own world. And Playtime even has its - // own undo system. - return; - } - } - if data == NOT_READY_YET.as_bytes() { - if let Some(lazy_data) = self.lazy_data.get() { - // Looks like someone activated the "Reset to factory default" preset. - lazy_data - .instance_shell - .upgrade() - .expect("instance shell gone") - .apply_data(InstanceData::default()) - .expect("couldn't load factory default"); - } + // TODO-medium-performance We could optimize by getting the config var only once, saving it in a global + // struct and then just dereferencing the var whenever we need it. Justin said that the result of + // get_config_var never changes throughout the lifetime of REAPER. + if let Ok(pref) = Reaper::get().get_preference_ref::("__fx_loadstate_ctx") { + if *pref == b'U' { + // REAPER is loading an updated undo state. We don't want to participate in REAPER's undo because + // it often leads to unpleasant surprises. ReaLearn is its own world. And Playtime even has its + // own undo system. return; } - let Some(lazy_data) = self.lazy_data.get() else { - // Unit shell is not available yet. Memorize data so we can apply it - // as soon as the shell is available. - self.pending_data_to_be_loaded - .write() - .unwrap() - .replace(data.to_vec()); - return; - }; - let instance_shell = lazy_data - .instance_shell - .upgrade() - .expect("instance shell gone"); - load_data_or_warn(instance_shell, data); - }); + } + if data == NOT_READY_YET.as_bytes() { + if let Some(lazy_data) = self.lazy_data.get() { + // Looks like someone activated the "Reset to factory default" preset. + lazy_data + .instance_shell + .upgrade() + .expect("instance shell gone") + .apply_data(InstanceData::default()) + .expect("couldn't load factory default"); + } + return; + } + let Some(lazy_data) = self.lazy_data.get() else { + // Unit shell is not available yet. Memorize data so we can apply it + // as soon as the shell is available. + self.pending_data_to_be_loaded + .write() + .unwrap() + .replace(data.to_vec()); + return; + }; + let instance_shell = lazy_data + .instance_shell + .upgrade() + .expect("instance shell gone"); + load_data_or_warn(instance_shell, data); } fn get_parameter_name(&self, index: i32) -> String { - firewall(|| self.get_parameter_name_internal(index).unwrap_or_default()).unwrap_or_default() + self.get_parameter_name_internal(index).unwrap_or_default() } fn get_parameter(&self, index: i32) -> f32 { - firewall(|| self.get_parameter_internal(index).unwrap_or_default()).unwrap_or_default() + self.get_parameter_internal(index).unwrap_or_default() } fn set_parameter(&self, index: i32, value: f32) { - firewall(|| { - let _ = self.set_parameter_internal(index, value); - }); + let _ = self.set_parameter_internal(index, value); } fn get_parameter_text(&self, index: i32) -> String { - firewall(|| self.get_parameter_text_internal(index).unwrap_or_default()).unwrap_or_default() + self.get_parameter_text_internal(index).unwrap_or_default() } fn string_to_parameter(&self, index: i32, text: String) -> bool { - firewall(|| { - self.string_to_parameter_internal(index, text) - .unwrap_or_default() - }) - .unwrap_or(false) + self.string_to_parameter_internal(index, text) + .unwrap_or_default() } }