Skip the message loop when an editor is opening

This is a bit more restrictive than the old approach that only skipped
when `effEditOpen()` got called after `effEditGetRect()`. Not sure why
the Melda plugins block indefinitely on the message loop without this
now.

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(
+    bridge->handle_dispatch_multi(plugin_context,
-        plugin_context, [&]() { return should_postpone_message_loop(); });
+                                  [&]() { 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() {
+bool GroupBridge::should_skip_message_loop() {
-    // TODO: Iterate over the plugins
+    // 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;
 }
 

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:
     /**

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
+            // `effEditOpen()`, since this won't ever happen on Windows and
-            // as hosts will always either call these functions in sequence or
+            // plugins thus assume that this can't happen at all. If
-            // in reverse. We need to temporarily stop handling messages when
+            // `effEditOpen()` has not yet been called, then we'll mark the
-            // this happens.
+            // editor as currently opening to prevent the message loop from
-            if (!std::holds_alternative<Editor>(editor)) {
+            // running.
-                editor = EditorOpening{};
+            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<Editor>(window_class, plugin, x11_handle);
 
+            editor.emplace(window_class, plugin, x11_handle);
             return plugin->dispatcher(plugin, opcode, index, value,
-                                      editor_instance.win32_handle.get(),
+                                      editor->win32_handle.get(), option);
-                                      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() {
+void Vst2Bridge::handle_win32_events() {
-    std::visit(
+    if (editor.has_value()) {
-        overload{[](Editor& editor) { editor.handle_events(); },
+        editor->handle_win32_events();
-                 [](std::monostate&) {
+    } else {
-                     MSG msg;
+        MSG msg;
 
-                     while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
+        while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
-                         TranslateMessage(&msg);
+            TranslateMessage(&msg);
-                         DispatchMessage(&msg);
+            DispatchMessage(&msg);
-                     }
+        }
-                 },
+    }
-                 [](EditorOpening&) {
+}
-                     // Don't handle any events in this
+
-                     // particular case as explained in
+void Vst2Bridge::handle_x11_events() {
-                     // `Vst2Bridge::editor`
+    if (editor.has_value()) {
-                 }},
+        editor->handle_x11_events();
-        editor);
+    }
 }
 
 class HostCallbackDataConverter : DefaultDataConverter {

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.
+     * @see should_postpone_message_loop
-     * - 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.
      */
-    std::variant<std::monostate, Editor, EditorOpening> editor;
+    std::optional<Editor> 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;
 };