From 4ec8b01bcc26916494df0efd8fd955db9ad20590 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 4 Jan 2021 15:50:17 +0100 Subject: [PATCH] Completely run effEditIdle from a timer Although it hasn't shown up, this will get rid of the possibility of off-thread effEditIdle calls causing issues. And since we need some way to run call this function while the event loop is running anyways, doing it entirely from a timer similar to how hosts on Windows would do it seems like the best solution. --- CHANGELOG.md | 4 +--- src/plugin/bridges/vst2.cpp | 14 +++++++++++ src/wine-host/bridges/vst2.cpp | 16 +++++++++---- src/wine-host/editor.cpp | 43 +++++++++++++++++++++++++++++++++- src/wine-host/editor.h | 34 ++++++++++++++++++++++++++- src/wine-host/utils.cpp | 11 +++++---- src/wine-host/utils.h | 2 ++ 7 files changed, 109 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2b39fa1..165bbf6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,9 +44,7 @@ TODO: Add an updated screenshot with some fancy VST3-only plugins to the readme Most hosts already do something similar themselves, so this may not make any difference in responsiveness. - VST2 editor idle events are now handled slightly differently. This should - result in even more responsive GUIs and I have not come across any plugins - where this change introduced issues, but please let me know if it does break - anything for you. + result in even more responsive GUIs for VST2 plugins. - Changed part of the build process considering [this Wine bug](https://bugs.winehq.org/show_bug.cgi?id=49138). Building with Wine 5.7 and 5.8 required a change, but that change now breaks builds using Wine 6.0 diff --git a/src/plugin/bridges/vst2.cpp b/src/plugin/bridges/vst2.cpp index ff1460ae..7b3615dd 100644 --- a/src/plugin/bridges/vst2.cpp +++ b/src/plugin/bridges/vst2.cpp @@ -398,6 +398,20 @@ intptr_t Vst2PluginBridge::dispatch(AEffect* /*plugin*/, return return_value; }; break; + case effEditIdle: { + // This is the only place where we'll deviate from yabridge's + // 'one-to-one passthrough' philosophy. While in practice we can + // just pass through `effEditIdle` and we have been doing so until + // yabridge 3.x, in reality it's much more practical to just run + // this on a Win32 timer. We would either need to run `effEditIdle` + // from a non-GUI thread (which could cause issues), or we would + // need a timer anyways to proc the function when the GUI is being + // blocked by for instance an open dropdown. + logger.log_event(true, opcode, index, value, nullptr, option, + std::nullopt); + logger.log_event_response(true, opcode, 0, nullptr, std::nullopt); + return 0; + }; break; case effCanDo: { const std::string query(static_cast(data)); diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 43fded9b..7d404b76 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -40,12 +40,10 @@ std::mutex current_bridge_instance_mutex; /** * Opcodes that should always be handled on the main thread because they may * involve GUI operations. - * - * XXX: We removed effEditIdle from here and everything still seems to work - * fine. Verify that this didn't break any plugins. */ const std::set unsafe_opcodes{effOpen, effClose, effEditGetRect, - effEditOpen, effEditClose, effEditTop}; + effEditOpen, effEditClose, effEditIdle, + effEditTop}; intptr_t VST_CALL_CONV host_callback_proxy(AEffect*, int, int, intptr_t, void*, float); @@ -313,6 +311,11 @@ void Vst2Bridge::run() { // https://github.com/Ardour/ardour/commit/f7cb1b0b481eeda755bdf8eb9fc5f90a81d2aa01. // We should keep this in until Ardour 6.3 is no // longer in distro repositories. + // + // Note that now that we run `effEditIdle` + // entirely off of a Win32 timer this will never + // get hit, but we'll keep it in for the sake of + // preserving correct behaviour. if (opcode == effEditIdle && !editor) { std::cerr << "WARNING: The host is calling " "`effEditIdle()` while the " @@ -379,7 +382,10 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin, // provided by the host, and let the plugin embed itself into // the Wine window const auto x11_handle = reinterpret_cast(data); - Editor& editor_instance = editor.emplace(config, x11_handle); + Editor& editor_instance = + editor.emplace(config, x11_handle, [plugin = this->plugin]() { + plugin->dispatcher(plugin, effEditIdle, 0, 0, nullptr, 0.0); + }); return plugin->dispatcher(plugin, opcode, index, value, editor_instance.get_win32_handle(), diff --git a/src/wine-host/editor.cpp b/src/wine-host/editor.cpp index 996e1ac7..da0e6746 100644 --- a/src/wine-host/editor.cpp +++ b/src/wine-host/editor.cpp @@ -18,6 +18,15 @@ #include +/** + * The Win32 timer ID we'll use to periodically call the VST2 `effeditidle` + * function with. We have to do this on a timer because the function has to be + * called from the GUI thread, and it should also be called while the Win32 + * event loop is being blocked (for instance when a plugin opens a dropdown + * menu). + */ +constexpr size_t idle_timer_id = 1337; + /** * The most significant bit in an X11 event's response type is used to indicate * the event source. @@ -100,7 +109,9 @@ WindowClass::~WindowClass() { UnregisterClass(reinterpret_cast(atom), GetModuleHandle(nullptr)); } -Editor::Editor(const Configuration& config, const size_t parent_window_handle) +Editor::Editor(const Configuration& config, + const size_t parent_window_handle, + std::optional> timer_proc) : use_xembed(config.editor_xembed), x11_connection(xcb_connect(nullptr, nullptr), xcb_disconnect), client_area(get_maximum_screen_dimensions(*x11_connection)), @@ -128,6 +139,16 @@ Editor::Editor(const Configuration& config, const size_t parent_window_handle) // window in `win32_child_handle`. If we do this before calling // `ShowWindow()` on `win32_handle` we'll run into X11 errors. win32_child_handle(std::nullopt), + idle_timer( + timer_proc + ? Win32Timer( + win32_handle.get(), + idle_timer_id, + std::chrono::duration_cast( + event_loop_interval) + .count()) + : Win32Timer()), + idle_timer_proc(std::move(timer_proc)), parent_window(parent_window_handle), wine_window(get_x11_handle(win32_handle.get())), topmost_window(find_topmost_window(*x11_connection, parent_window)) { @@ -423,6 +444,12 @@ void Editor::set_input_focus(bool grab) const { xcb_flush(x11_connection.get()); } +void Editor::maybe_run_timer_proc() { + if (idle_timer_proc) { + (*idle_timer_proc)(); + } +} + void Editor::destroy_window_async(HWND window_handle) { PostMessage(window_handle, WM_CLOSE, 0, 0); } @@ -566,6 +593,20 @@ LRESULT CALLBACK window_proc(HWND handle, WINDOWPOS* info = reinterpret_cast(lParam); info->flags |= SWP_NOCOPYBITS | SWP_DEFERERASE; } break; + case WM_TIMER: { + auto editor = reinterpret_cast( + GetWindowLongPtr(handle, GWLP_USERDATA)); + if (!editor || wParam != idle_timer_id) { + break; + } + + // We'll send idle messages on a timer for VST2 plugins. This way + // the plugin will get keep periodically updating its editor either + // when the host sends `effEditIdle` themself, or periodically when + // the GUI is being blocked by a dropdown or a message box. + editor->maybe_run_timer_proc(); + return 0; + } break; // In case the WM does not support the EWMH active window property, // we'll fall back to grabbing focus when the user clicks on the window // by listening to the generated `WM_PARENTNOTIFY` messages. Otherwise diff --git a/src/wine-host/editor.h b/src/wine-host/editor.h index faaff095..57593186 100644 --- a/src/wine-host/editor.h +++ b/src/wine-host/editor.h @@ -25,6 +25,7 @@ #define WINE_NOWINSOCK #endif #include +#include // Use the native version of xcb #pragma push_macro("_WIN32") @@ -99,10 +100,16 @@ class Editor { * editor behaviours. * @param parent_window_handle The X11 window handle passed by the VST host * for the editor to embed itself into. + * @param timer_proc A function to run on a timer. This is used for VST2 + * plugins to periodically call `effEditIdle` from the message loop + * thread, even when the GUI is blocked. * * @see win32_handle */ - Editor(const Configuration& config, const size_t parent_window_handle); + Editor( + const Configuration& config, + const size_t parent_window_handle, + std::optional> timer_proc = std::nullopt); ~Editor(); @@ -147,6 +154,14 @@ class Editor { */ void set_input_focus(bool grab) const; + /** + * Run the timer proc function passed to the constructor, if one was passed. + * + * @see idle_timer + * @see idle_timer_proc + */ + void maybe_run_timer_proc(); + /** * Whether to use XEmbed instead of yabridge's normal window embedded. Wine * with XEmbed tends to cause rendering issues, so it's disabled by default. @@ -236,6 +251,23 @@ class Editor { #pragma GCC diagnostic pop + /** + * A timer we'll use to periodically run `idle_timer_proc`, if set. Thisi is + * only needed for VST2 plugins, as they expected the host to periodically + * send an idle event. We used to just pass through the calls from the host + * before yabridge 3.x, but doing it ourselves here makes things m much more + * manageable and we'd still need a timer anyways for when the GUI is + * blocked. + */ + Win32Timer idle_timer; + + /** + * A function to call when the Win32 timer procs. This is used to + * periodically call `effEditIdle()` for VST2 plugins even if the GUI is + * being blocked. + */ + std::optional> idle_timer_proc; + /** * The window handle of the editor window created by the DAW. */ diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index e22e080d..afa1c630 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -50,6 +50,8 @@ Win32Thread::~Win32Thread() { Win32Thread::Win32Thread() : handle(nullptr, nullptr) {} +Win32Timer::Win32Timer() {} + Win32Timer::Win32Timer(HWND window_handle, size_t timer_id, unsigned int interval_ms) @@ -63,13 +65,12 @@ Win32Timer::~Win32Timer() { } } -Win32Timer::Win32Timer(Win32Timer&& o) : timer_id(o.timer_id) { - o.timer_id = std::nullopt; -} +Win32Timer::Win32Timer(Win32Timer&& o) + : window_handle(o.window_handle), timer_id(std::move(o.timer_id)) {} Win32Timer& Win32Timer::operator=(Win32Timer&& o) { - timer_id = o.timer_id; - o.timer_id = std::nullopt; + window_handle = o.window_handle; + timer_id = std::move(o.timer_id); return *this; } diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index 16683432..3477b586 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -226,7 +226,9 @@ class Win32Thread { */ class Win32Timer { public: + Win32Timer(); Win32Timer(HWND window_handle, size_t timer_id, unsigned int interval_ms); + ~Win32Timer(); Win32Timer(const Win32Timer&) = delete;