diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index ef1c16d5..607201fe 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -125,8 +125,8 @@ void GroupBridge::handle_plugin_dispatch(const GroupRequest request) { // TODO: Maybe add a function to each vstbridge that returns whether the // message loop should be postponed, and pass a function here that // checks if this is the case for any of the plugins? - bridge->handle_dispatch_multi( - plugin_context, [&]() { return should_postpone_message_loop(); }); + bridge->handle_dispatch_multi(plugin_context, + [&]() { return should_skip_message_loop(); }); logger.log("'" + request.plugin_path + "' has exited"); // After the plugin has exited, we'll remove this thread's plugin from the @@ -162,8 +162,19 @@ void GroupBridge::handle_incoming_connections() { plugin_context.run(); } -bool GroupBridge::should_postpone_message_loop() { - // TODO: Iterate over the plugins +bool GroupBridge::should_skip_message_loop() { + // We do not need additional locking since the call to `AEffect::dispatcher` + // and the actual event handling and message loop handling are performed + // within the IO context and these values thus can't change while another + // the message loop is being running + std::lock_guard lock(active_plugins_mutex); + for (auto& [parameters, value] : active_plugins) { + auto& [thread, bridge] = value; + if (bridge->should_skip_message_loop()) { + return true; + } + } + return false; } diff --git a/src/wine-host/bridges/group.h b/src/wine-host/bridges/group.h index 074d0ee5..87fd9d07 100644 --- a/src/wine-host/bridges/group.h +++ b/src/wine-host/bridges/group.h @@ -162,7 +162,7 @@ class GroupBridge { * from the same thread, this is a simple way to synchronize blocking the * mesage loop between the different plugin instances. */ - bool should_postpone_message_loop(); + bool should_skip_message_loop(); private: /** diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 0331356a..7ebd852b 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -139,6 +139,10 @@ Vst2Bridge::Vst2Bridge(std::string plugin_dll_path, Win32Thread(handle_process_replacing_proxy, this); } +bool Vst2Bridge::should_skip_message_loop() { + return editor_is_opening; +} + void Vst2Bridge::handle_dispatch_single() { using namespace std::placeholders; @@ -152,7 +156,12 @@ void Vst2Bridge::handle_dispatch_single() { plugin, std::bind(&Vst2Bridge::dispatch_wrapper, this, _1, _2, _3, _4, _5, _6))); - handle_win32_events(); + // Don't run them message loop during the two step process of + // opening the plugin editor since some plugins don't expect this + if (!should_skip_message_loop()) { + handle_win32_events(); + } + handle_x11_events(); } } catch (const boost::system::system_error&) { @@ -320,18 +329,27 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin, case effEditGetRect: { // Some plugins will have a race condition if the message loops gets // handled between the call to `effEditGetRect()` and - // `effEditOpen()`, although this behavior never appears on Windows - // as hosts will always either call these functions in sequence or - // in reverse. We need to temporarily stop handling messages when - // this happens. - if (!std::holds_alternative(editor)) { - editor = EditorOpening{}; - } + // `effEditOpen()`, since this won't ever happen on Windows and + // plugins thus assume that this can't happen at all. If + // `effEditOpen()` has not yet been called, then we'll mark the + // editor as currently opening to prevent the message loop from + // running. + editor_is_opening = !editor.has_value(); return plugin->dispatcher(plugin, opcode, index, value, data, option); } break; case effEditOpen: { + // As explained above, if `effEditGetRect()` was called first then + // after this the editor has finally been opened. Otherwise if this + // function was first then we'll say that the editor is in the + // process of being opened as the host will call `effEditGetRect()` + // next. + // TODO: Without plugin groups only the `effEditGetRect()` -> + // `effEditOpen()`, is skipping the message loop in between + // `effEditOpen()` and `effEditGetRect()` really needed? + editor_is_opening = !editor_is_opening; + // Create a Win32 window through Wine, embed it into the window // provided by the host, and let the plugin embed itself into // the Wine window @@ -341,19 +359,17 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin, // should get a unique window class const std::string window_class = "yabridge plugin " + socket_endpoint.path(); - Editor& editor_instance = - editor.emplace(window_class, plugin, x11_handle); + editor.emplace(window_class, plugin, x11_handle); return plugin->dispatcher(plugin, opcode, index, value, - editor_instance.win32_handle.get(), - option); + editor->win32_handle.get(), option); } break; case effEditClose: { const intptr_t return_value = plugin->dispatcher(plugin, opcode, index, value, data, option); // Cleanup is handled through RAII - editor = std::monostate(); + editor.reset(); return return_value; } break; @@ -364,23 +380,23 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin, } } -void Vst2Bridge::pump_message_loop() { - std::visit( - overload{[](Editor& editor) { editor.handle_events(); }, - [](std::monostate&) { - MSG msg; +void Vst2Bridge::handle_win32_events() { + if (editor.has_value()) { + editor->handle_win32_events(); + } else { + MSG msg; - while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { - TranslateMessage(&msg); - DispatchMessage(&msg); - } - }, - [](EditorOpening&) { - // Don't handle any events in this - // particular case as explained in - // `Vst2Bridge::editor` - }}, - editor); + while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { + TranslateMessage(&msg); + DispatchMessage(&msg); + } + } +} + +void Vst2Bridge::handle_x11_events() { + if (editor.has_value()) { + editor->handle_x11_events(); + } } class HostCallbackDataConverter : DefaultDataConverter { diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 6f6404a5..4a35db09 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -37,13 +37,6 @@ #include "../editor.h" #include "../utils.h" -/** - * A marker struct to indicate that the editor is about to be opened. - * - * @see Vst2Bridge::editor - */ -struct EditorOpening {}; - /** * This hosts a Windows VST2 plugin, forwards messages sent by the Linux VST * plugin and provides host callback function for the plugin to talk back. @@ -79,6 +72,17 @@ class Vst2Bridge { */ Vst2Bridge(std::string plugin_dll_path, std::string socket_endpoint_path); + /** + * Returns true if the message loop should be skipped. This happens when the + * editor is in the process of being opened. In VST hosts on Windows + * `effEditOpen()` and `effEditGetRect()` will always be called in sequence, + * but in our approach there will be an opportunity to handle events in + * between these two calls. Most plugins will handle this just fine, but + * some plugins end up blocking indefinitely while waiting for the other + * function to be called, hence why this function is needed. + */ + bool should_skip_message_loop(); + /** * Handle events on the main thread until the plugin quits. This can't be * done on another thread since some plugins (e.g. Melda) expect certain @@ -178,7 +182,9 @@ class Vst2Bridge { /** * Run the message loop for this plugin and potentially also for other - * plugins. This is called by both versions of `handle_dispatch()`. + * plugins. This is only used in `handle_dispatch_single()`, as this will be + * run on a timer when using plugin groups. The caller should first check + * whether the event loop can be run through `should_skip_message_loop()`. * * Because of the way the Win32 API works we have to process events on the * same thread as the one the window was created on, and that thread is the @@ -189,7 +195,13 @@ class Vst2Bridge { * because of incorrect assumptions made by the plugin. See the dostring for * `Vst2Bridge::editor` for more information. */ - void pump_message_loop(); + void handle_win32_events(); + + /** + * Handle X11 events for the editor window if it is open. This can be run + * safely from any thread. + */ + void handle_x11_events(); /** * The shared library handle of the VST plugin. I sadly could not get @@ -280,23 +292,17 @@ class Vst2Bridge { * Wine window, and embedding that Wine window into a window provided by the * host. Should be empty when the editor is not open. * - * This field can have three possible states: + * There is some special behavior with regards to message handling when the + * editor is in the process of being opened, see + * `should_postpone_message_loop()`. * - * - `std::nullopt` when the editor is closed. - * - An `Editor` object when the editor is open. - * - `EditorOpening` when the editor is not yet open, but the host has - * already called `effEditGetRect()` and is about to call `effEditOpen()`. - * This is needed because there is a race condition in some bugs that - * cause them to crash or enter an infinite Win32 message loop when - * `effEditGetRect()` gets dispatched and we then enter the message loop - * loop before `effEditOpen()` gets called. Most plugins will handle this - * just fine, but a select few plugins make the assumption that the editor - * is already open once `effEditGetRect()` has been called, even if - * `effEditOpen` has not yet been dispatched. VST hsots on Windows will - * call these two events in sequence, so the bug would never occur there. - * To work around this we'll use this third state to temporarily stop - * processing Windows events in the one or two ticks between these two - * events. + * @see should_postpone_message_loop */ - std::variant editor; + std::optional editor; + + /** + * Keeps track of when the editor is being opened during the two-phase + * process of the host calling `effEditOpen()` and `effEditGetRect()`. + */ + bool editor_is_opening = false; };