From 6e5ea3a4d892abb3da231fc8c6aeb004612f7a60 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 27 Jan 2021 19:03:28 +0100 Subject: [PATCH] Skip event loop with partially initialized plugins This should prevent T-RackS 5 from potentially stalling indefinitely when using plugin groups. --- CHANGELOG.md | 5 +++ src/wine-host/bridges/group.cpp | 61 +++++++++++++++++++------------ src/wine-host/bridges/group.h | 8 ++++ src/wine-host/individual-host.cpp | 10 +++-- src/wine-host/utils.h | 55 +++++++++++++--------------- 5 files changed, 83 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20b7026f..24e50e10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -107,6 +107,11 @@ TODO: Add an updated screenshot with some fancy VST3-only plugins to the readme always executed from the GUI thread. This fixes **EZdrummer** not producing any sound because the plugin makes the incorrect assumption that `effMainsChanged()` is always called from the GUI thread. +- Event handling is now temporarily disabled while plugins are in a partially + initialized state. The VST2 versions of **T-RackS 5** would have a chance to + hang indefinitely if the event loop was being run before those plugins were + fully initialized because of a race condition within those plugins. This was + issue only noticeable when using plugin groups. - Fixed a potential issue where an interaction between _Bitwig Studio_ and yabridge's input focus grabbing method could cause delayed mouse events when clicking on a plugin's GUI in Bitwig. This issue has not been reported for diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index cdd1ca4b..2ced9723 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -117,6 +117,19 @@ GroupBridge::~GroupBridge() { stdio_context.stop(); } +bool GroupBridge::is_event_loop_inhibited() { + std::lock_guard lock(active_plugins_mutex); + + for (auto& [parameters, value] : active_plugins) { + auto& [thread, bridge] = value; + if (bridge->inhibits_event_loop()) { + return true; + } + } + + return false; +} + void GroupBridge::handle_plugin_run(size_t plugin_id, HostBridge* bridge) { // Blocks this thread until the plugin shuts down bridge->run(); @@ -256,33 +269,35 @@ void GroupBridge::accept_requests() { } void GroupBridge::async_handle_events() { - main_context.async_handle_events([&]() { - { - // Always handle X11 events - std::lock_guard lock(active_plugins_mutex); - for (auto& [parameters, value] : active_plugins) { - auto& [thread, bridge] = value; - bridge->handle_x11_events(); + main_context.async_handle_events( + [&]() { + { + // Always handle X11 events + std::lock_guard lock(active_plugins_mutex); + for (auto& [parameters, value] : active_plugins) { + auto& [thread, bridge] = value; + bridge->handle_x11_events(); + } } - } - std::lock_guard lock(active_plugins_mutex); + std::lock_guard lock(active_plugins_mutex); - MSG msg; + MSG msg; - // Keep the loop responsive by not handling too many events at once - // - // For some reason the Melda plugins run into a seemingly infinite timer - // loop for a little while after opening a second editor. Without this - // limit everything will get blocked indefinitely. How could this be - // fixed? - for (int i = 0; i < max_win32_messages && - PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE); - i++) { - TranslateMessage(&msg); - DispatchMessage(&msg); - } - }); + // Keep the loop responsive by not handling too many events at once + // + // For some reason the Melda plugins run into a seemingly infinite + // timer loop for a little while after opening a second editor. + // Without this limit everything will get blocked indefinitely. How + // could this be fixed? + for (int i = 0; i < max_win32_messages && + PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE); + i++) { + TranslateMessage(&msg); + DispatchMessage(&msg); + } + }, + [&]() { return !is_event_loop_inhibited(); }); } void GroupBridge::async_log_pipe_lines( diff --git a/src/wine-host/bridges/group.h b/src/wine-host/bridges/group.h index 86e208cb..02421c65 100644 --- a/src/wine-host/bridges/group.h +++ b/src/wine-host/bridges/group.h @@ -130,6 +130,14 @@ class GroupBridge { GroupBridge(const GroupBridge&) = delete; GroupBridge& operator=(const GroupBridge&) = delete; + /** + * If this returns `true`, then the group host's event loop should + * temporarily be disabled. This simply calls + * `HostBridge::inhibits_event_loop()` for all plugins hosted in this group + * process. + */ + bool is_event_loop_inhibited(); + /** * Run a plugin's dispatcher and message loop, processing all events on the * main IO context. The plugin will have already been created in diff --git a/src/wine-host/individual-host.cpp b/src/wine-host/individual-host.cpp index 569af64c..aadf0d01 100644 --- a/src/wine-host/individual-host.cpp +++ b/src/wine-host/individual-host.cpp @@ -132,9 +132,11 @@ __cdecl // Handle Win32 messages and X11 events on a timer, just like in // `GroupBridge::async_handle_events()`` - main_context.async_handle_events([&]() { - bridge->handle_x11_events(); - bridge->handle_win32_events(); - }); + main_context.async_handle_events( + [&]() { + bridge->handle_x11_events(); + bridge->handle_win32_events(); + }, + [&]() { return !bridge->inhibits_event_loop(); }); main_context.run(); } diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index d5f99162..d0043aec 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -41,20 +41,6 @@ * which can be required because in the Win32 model all GUI related operations * have to be handled from the same thread. This will be run from the * application's main thread. - * - * TODO: Add some point we might have to add some way to inhibit the event loop - * from running when a plugin is in a partially initialized state. So far - * only the VST2 versions of the T-RackS 5 plugins have this issue (and - * only when running them in a plugin group because with individually - * hosted plugins the event loop and the plugin's initialization will - * align). This will require a fairly significant architectural change to - * do this in a clean way (`HostBridge` would need some function that - * returns a boolean to indicate whether the event loop should be - * inhibited, and then we would need to pass a function object or lambda - * to `MainContext` that checks this for all currently hosted plugins so - * we can integrate that into `async_handle_events()`). Now that I've - * written this out, it's not actually as bad as I thought it would be, so - * we'll probably add this sooner rather than later. */ class MainContext { public: @@ -112,33 +98,44 @@ class MainContext { * @param handler The function that should be executed in the IO context * when the timer ticks. This should be a function that handles both the * X11 events and the Win32 message loop. + * @param predicate A function returning a boolean to indicate whether + * `handler` should be run. If this returns `false`, then the current + * event loop cycle will be skipped. This is used to prevent the Win32 + * message loop from being run when there are partially initialized + * plugins. So far the VST2 versions of T-RackS 5 are the only plugins + * where this has been an issue as those plugins have a race condition + * that will cause them to stall indefinitely in this situation, but who + * knows which other plugins exert similar behaviour. */ - template - void async_handle_events(F handler) { + template + void async_handle_events(F handler, P predicate) { // Try to keep a steady framerate, but add in delays to let other events // get handled if the GUI message handling somehow takes very long. events_timer.expires_at( std::max(events_timer.expiry() + timer_interval, std::chrono::steady_clock::now() + timer_interval / 4)); events_timer.async_wait( - [&, handler](const boost::system::error_code& error) { + [&, handler, predicate](const boost::system::error_code& error) { if (error.failed()) { return; } - // NOTE: These periodic callbacks should not be able to - // interrupt other threads that are actively processing - // audio. For me personally having the GUI open makes - // absolutely zero difference on DSP usage (as it should), - // but for some others it does have an impact. - // TODO: Benchmark this further on a properly configured system, - // see if it does not increase average load because of the - // rapid scheduling switching. - set_realtime_priority(false); - handler(); - set_realtime_priority(true); + if (predicate()) { + // NOTE: These periodic callbacks should not be able to + // interrupt other threads that are actively + // processing audio. For me personally having the GUI + // open makes absolutely zero difference on DSP usage + // (as it should), but for some others it does have an + // impact. + // TODO: Benchmark this further on a properly configured + // system, see if it does not increase average load + // because of the rapid scheduling switching. + set_realtime_priority(false); + handler(); + set_realtime_priority(true); + } - async_handle_events(handler); + async_handle_events(handler, predicate); }); }