From 17a95e703f47bf24d9deeecb3d382774c4451d6c Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 7 Mar 2023 18:02:56 +0100 Subject: [PATCH] Detect incorrect GuiContext method usage In debug builds. --- CHANGELOG.md | 9 ++++ src/wrapper/clap/context.rs | 36 ++++++++++++++++ src/wrapper/clap/wrapper.rs | 16 ++++++- src/wrapper/standalone/context.rs | 38 ++++++++++++++++- src/wrapper/standalone/wrapper.rs | 13 +++++- src/wrapper/util.rs | 2 + src/wrapper/util/context_checks.rs | 67 ++++++++++++++++++++++++++++++ src/wrapper/vst3/context.rs | 36 ++++++++++++++++ src/wrapper/vst3/inner.rs | 16 ++++++- 9 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 src/wrapper/util/context_checks.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 550a9fa8..e34bbc05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,15 @@ Since there is no stable release yet, the changes are organized per day in reverse chronological order. The main purpose of this document in its current state is to list breaking changes. +## [2023-03-07] + +This document is now also used to keep track of non-breaking changes. + +### Added + +- Debug builds now include debug assertions that detect incorrect use of the + `GuiContext`'s parameter setting methods. + ## [2023-02-28] ### Breaking changes diff --git a/src/wrapper/clap/context.rs b/src/wrapper/clap/context.rs index e4d42850..949665b0 100644 --- a/src/wrapper/clap/context.rs +++ b/src/wrapper/clap/context.rs @@ -47,6 +47,9 @@ pub(crate) struct WrapperProcessContext<'a, P: ClapPlugin> { /// with the host for things like setting parameters. pub(crate) struct WrapperGuiContext { pub(super) wrapper: Arc>, + #[cfg(debug_assertions)] + pub(super) param_gesture_checker: + atomic_refcell::AtomicRefCell, } impl Drop for WrapperInitContext<'_, P> { @@ -139,6 +142,17 @@ impl GuiContext for WrapperGuiContext

{ } None => nih_debug_assert_failure!("Unknown parameter: {:?}", param), } + + #[cfg(debug_assertions)] + match self.wrapper.param_id_from_ptr(param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .begin_set_parameter(param_id), + None => nih_debug_assert_failure!( + "raw_begin_set_parameter() called with an unknown ParamPtr" + ), + } } unsafe fn raw_set_parameter_normalized(&self, param: ParamPtr, normalized: f32) { @@ -165,6 +179,17 @@ impl GuiContext for WrapperGuiContext

{ } None => nih_debug_assert_failure!("Unknown parameter: {:?}", param), } + + #[cfg(debug_assertions)] + match self.wrapper.param_id_from_ptr(param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .set_parameter(param_id), + None => { + nih_debug_assert_failure!("raw_set_parameter() called with an unknown ParamPtr") + } + } } unsafe fn raw_end_set_parameter(&self, param: ParamPtr) { @@ -182,6 +207,17 @@ impl GuiContext for WrapperGuiContext

{ } None => nih_debug_assert_failure!("Unknown parameter: {:?}", param), } + + #[cfg(debug_assertions)] + match self.wrapper.param_id_from_ptr(param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .end_set_parameter(param_id), + None => { + nih_debug_assert_failure!("raw_end_set_parameter() called with an unknown ParamPtr") + } + } } fn get_state(&self) -> crate::wrapper::state::PluginState { diff --git a/src/wrapper/clap/wrapper.rs b/src/wrapper/clap/wrapper.rs index dc8c5fbc..a8de6544 100644 --- a/src/wrapper/clap/wrapper.rs +++ b/src/wrapper/clap/wrapper.rs @@ -719,7 +719,11 @@ impl Wrapper

{ } fn make_gui_context(self: Arc) -> Arc> { - Arc::new(WrapperGuiContext { wrapper: self }) + Arc::new(WrapperGuiContext { + wrapper: self, + #[cfg(debug_assertions)] + param_gesture_checker: Default::default(), + }) } /// # Note @@ -742,6 +746,16 @@ impl Wrapper

{ } } + /// Get a parameter's ID based on a `ParamPtr`. Used in the `GuiContext` implementation for the + /// gesture checks. + #[allow(unused)] + pub fn param_id_from_ptr(&self, param: ParamPtr) -> Option<&str> { + self.param_ptr_to_hash + .get(¶m) + .and_then(|hash| self.param_id_by_hash.get(hash)) + .map(|s| s.as_str()) + } + /// Queue a parameter output event to be sent to the host at the end of the audio processing /// cycle, and request a parameter flush from the host if the plugin is not currently processing /// audio. The parameter's actual value will only be updated at that point so the value won't diff --git a/src/wrapper/standalone/context.rs b/src/wrapper/standalone/context.rs index c7dec4e6..4329ac12 100644 --- a/src/wrapper/standalone/context.rs +++ b/src/wrapper/standalone/context.rs @@ -34,6 +34,9 @@ pub(crate) struct WrapperProcessContext<'a, P: Plugin, B: Backend

> { /// with the host for things like setting parameters. pub(crate) struct WrapperGuiContext> { pub(super) wrapper: Arc>, + #[cfg(debug_assertions)] + pub(super) param_gesture_checker: + atomic_refcell::AtomicRefCell, } impl> InitContext

for WrapperInitContext<'_, P, B> { @@ -111,13 +114,46 @@ impl> GuiContext for WrapperGuiContext { unsafe fn raw_begin_set_parameter(&self, _param: ParamPtr) { // Since there's no automation being recorded here, gestures don't mean anything + + #[cfg(debug_assertions)] + match self.wrapper.param_id_from_ptr(_param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .begin_set_parameter(param_id), + None => nih_debug_assert_failure!( + "raw_begin_set_parameter() called with an unknown ParamPtr" + ), + } } unsafe fn raw_set_parameter_normalized(&self, param: ParamPtr, normalized: f32) { self.wrapper.set_parameter(param, normalized); + + #[cfg(debug_assertions)] + match self.wrapper.param_id_from_ptr(param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .set_parameter(param_id), + None => { + nih_debug_assert_failure!("raw_set_parameter() called with an unknown ParamPtr") + } + } } - unsafe fn raw_end_set_parameter(&self, _param: ParamPtr) {} + unsafe fn raw_end_set_parameter(&self, _param: ParamPtr) { + #[cfg(debug_assertions)] + match self.wrapper.param_id_from_ptr(_param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .end_set_parameter(param_id), + None => { + nih_debug_assert_failure!("raw_end_set_parameter() called with an unknown ParamPtr") + } + } + } fn get_state(&self) -> crate::wrapper::state::PluginState { self.wrapper.get_state_object() diff --git a/src/wrapper/standalone/wrapper.rs b/src/wrapper/standalone/wrapper.rs index 711b1390..3d5ad7c7 100644 --- a/src/wrapper/standalone/wrapper.rs +++ b/src/wrapper/standalone/wrapper.rs @@ -378,6 +378,13 @@ impl> Wrapper { Ok(()) } + /// Get a parameter's ID based on a `ParamPtr`. Used in the `GuiContext` implementation for the + /// gesture checks. + #[allow(unused)] + pub fn param_id_from_ptr(&self, param: ParamPtr) -> Option<&str> { + self.param_ptr_to_id.get(¶m).map(|s| s.as_str()) + } + /// Set a parameter based on a `ParamPtr`. The value will be updated at the end of the next /// processing cycle, and this won't do anything if the parameter has not been registered by the /// plugin. @@ -554,7 +561,11 @@ impl> Wrapper { } fn make_gui_context(self: Arc) -> Arc> { - Arc::new(WrapperGuiContext { wrapper: self }) + Arc::new(WrapperGuiContext { + wrapper: self, + #[cfg(debug_assertions)] + param_gesture_checker: Default::default(), + }) } fn make_init_context(&self) -> WrapperInitContext<'_, P, B> { diff --git a/src/wrapper/util.rs b/src/wrapper/util.rs index e8adb7ba..f05656ad 100644 --- a/src/wrapper/util.rs +++ b/src/wrapper/util.rs @@ -5,6 +5,8 @@ use std::os::raw::c_char; use crate::util::permit_alloc; +pub(crate) mod context_checks; + /// The bit that controls flush-to-zero behavior for denormals in 32 and 64-bit floating point /// numbers on AArch64. /// diff --git a/src/wrapper/util/context_checks.rs b/src/wrapper/util/context_checks.rs new file mode 100644 index 00000000..78324bb8 --- /dev/null +++ b/src/wrapper/util/context_checks.rs @@ -0,0 +1,67 @@ +//! Common validation used for the [contexts][crate::context]. + +use std::collections::HashSet; + +/// Ensures that parameter changes send from the GUI are wrapped in parameter gestures, and that the +/// gestures are handled consistently (no duplicate starts and ends, no end before start, etc.). +/// +/// Should only be used in debug builds. +#[derive(Debug, Default)] +pub struct ParamGestureChecker { + /// The parameters with an active gesture. + active_params: HashSet, +} + +impl Drop for ParamGestureChecker { + fn drop(&mut self) { + nih_debug_assert!( + self.active_params.is_empty(), + "GuiContext::end_set_parameter() was never called for {} {} {:?}", + self.active_params.len(), + if self.active_params.len() == 1 { + "parameter" + } else { + "parameters" + }, + self.active_params + ); + } +} + +impl ParamGestureChecker { + /// Called for + /// [`GuiContext::begin_set_parameter()`][crate::prelude::GuiContext::begin_set_parameter()]. + /// Triggers a debug assertion failure if the state is inconsistent. + pub fn begin_set_parameter(&mut self, param_id: &str) { + nih_debug_assert!( + !self.active_params.contains(param_id), + "GuiContext::begin_set_parameter() was called twice for parameter '{}'", + param_id + ); + self.active_params.insert(param_id.to_owned()); + } + + /// Called for [`GuiContext::set_parameter()`][crate::prelude::GuiContext::set_parameter()]. + /// Triggers a debug assertion failure if the state is inconsistent. + pub fn set_parameter(&self, param_id: &str) { + nih_debug_assert!( + self.active_params.contains(param_id), + "GuiContext::set_parameter() was called for parameter '{}' without a preceding \ + begin_set_parameter() call", + param_id + ); + } + + /// Called for + /// [`GuiContext::end_set_parameter()`][crate::prelude::GuiContext::end_set_parameter()]. + /// Triggers a debug assertion failure if the state is inconsistent. + pub fn end_set_parameter(&mut self, param_id: &str) { + nih_debug_assert!( + self.active_params.contains(param_id), + "GuiContext::end_set_parameter() was called for parameter '{}' without a preceding \ + begin_set_parameter() call", + param_id + ); + self.active_params.remove(param_id); + } +} diff --git a/src/wrapper/vst3/context.rs b/src/wrapper/vst3/context.rs index 0194deb2..14ed5847 100644 --- a/src/wrapper/vst3/context.rs +++ b/src/wrapper/vst3/context.rs @@ -52,6 +52,9 @@ pub(crate) struct WrapperProcessContext<'a, P: Vst3Plugin> { /// with the host for things like setting parameters. pub(crate) struct WrapperGuiContext { pub(super) inner: Arc>, + #[cfg(debug_assertions)] + pub(super) param_gesture_checker: + atomic_refcell::AtomicRefCell, } impl Drop for WrapperInitContext<'_, P> { @@ -144,6 +147,17 @@ impl GuiContext for WrapperGuiContext

{ }, None => nih_debug_assert_failure!("Component handler not yet set"), } + + #[cfg(debug_assertions)] + match self.inner.param_id_from_ptr(param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .begin_set_parameter(param_id), + None => nih_debug_assert_failure!( + "raw_begin_set_parameter() called with an unknown ParamPtr" + ), + } } unsafe fn raw_set_parameter_normalized(&self, param: ParamPtr, normalized: f32) { @@ -174,6 +188,17 @@ impl GuiContext for WrapperGuiContext

{ }, None => nih_debug_assert_failure!("Component handler not yet set"), } + + #[cfg(debug_assertions)] + match self.inner.param_id_from_ptr(param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .set_parameter(param_id), + None => { + nih_debug_assert_failure!("raw_set_parameter() called with an unknown ParamPtr") + } + } } unsafe fn raw_end_set_parameter(&self, param: ParamPtr) { @@ -186,6 +211,17 @@ impl GuiContext for WrapperGuiContext

{ }, None => nih_debug_assert_failure!("Component handler not yet set"), } + + #[cfg(debug_assertions)] + match self.inner.param_id_from_ptr(param) { + Some(param_id) => self + .param_gesture_checker + .borrow_mut() + .end_set_parameter(param_id), + None => { + nih_debug_assert_failure!("raw_end_set_parameter() called with an unknown ParamPtr") + } + } } fn get_state(&self) -> PluginState { diff --git a/src/wrapper/vst3/inner.rs b/src/wrapper/vst3/inner.rs index cd2a9953..e8b86bbf 100644 --- a/src/wrapper/vst3/inner.rs +++ b/src/wrapper/vst3/inner.rs @@ -369,7 +369,11 @@ impl WrapperInner

{ } pub fn make_gui_context(self: Arc) -> Arc> { - Arc::new(WrapperGuiContext { inner: self }) + Arc::new(WrapperGuiContext { + inner: self, + #[cfg(debug_assertions)] + param_gesture_checker: Default::default(), + }) } /// # Note @@ -431,6 +435,16 @@ impl WrapperInner

{ } } + /// Get a parameter's ID based on a `ParamPtr`. Used in the `GuiContext` implementation for the + /// gesture checks. + #[allow(unused)] + pub fn param_id_from_ptr(&self, param: ParamPtr) -> Option<&str> { + self.param_ptr_to_hash + .get(¶m) + .and_then(|hash| self.param_id_by_hash.get(hash)) + .map(|s| s.as_str()) + } + /// Convenience function for setting a value for a parameter as triggered by a VST3 parameter /// update. The same rate is for updating parameter smoothing. ///