Get rid of message loop skipping and EditorOpening

This special behaviour is no longer needed now that event handling is
fully concurrent and the Win32 message loop no longer blocks
`dispatch()` calls.

CHANGELOG.md

@@ -31,9 +31,11 @@ Versioning](https://semver.org/spec/v2.0.0.html).
     expected.
   - And probably many more improvements.
 
-  I have been testing this extensively to make sure that the change does not
+  Aside from these more noticeable changes, this has also made it possible to
-  introduce any regressions, but please let me know if this does break anything
+  remove a lot of checks and behaviour that existed solely to work around the
-  for you.
+  limitations introduced by the event handling system. I have been testing this
+  extensively to make sure that the change does not introduce any regressions,
+  but please let me know if this does break anything for you.
 
   TODO: Remove known issue about opening Kontakt and certain other plugins causing playback to stall, since this is no longer the case
 

src/wine-host/bridges/group.cpp

@@ -161,22 +161,6 @@ void GroupBridge::handle_incoming_connections() {
     main_context.run();
 }
 
-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;
-}
-
 void GroupBridge::accept_requests() {
     group_socket_acceptor.async_accept(
         [&](const boost::system::error_code& error,
@@ -245,25 +229,21 @@ void GroupBridge::async_handle_events() {
             }
         }
 
-        // Handle Win32 messages unless plugins are in the middle of opening
+        std::lock_guard lock(active_plugins_mutex);
-        // their editor
-        if (!should_skip_message_loop()) {
-            std::lock_guard lock(active_plugins_mutex);
 
-            MSG msg;
+        MSG msg;
 
-            // Keep the loop responsive by not handling too many events at once
+        // Keep the loop responsive by not handling too many events at once
-            //
+        //
-            // For some reason the Melda plugins run into a seemingly infinite
+        // For some reason the Melda plugins run into a seemingly infinite timer
-            // timer loop for a little while after opening a second editor.
+        // loop for a little while after opening a second editor.  Without this
-            // Without this limit everything will get blocked indefinitely. How
+        // limit everything will get blocked indefinitely. How could this be
-            // could this be fixed?
+        // fixed?
-            for (int i = 0; i < max_win32_messages &&
+        for (int i = 0; i < max_win32_messages &&
-                            PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE);
+                        PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE);
-                 i++) {
+             i++) {
-                TranslateMessage(&msg);
+            TranslateMessage(&msg);
-                DispatchMessage(&msg);
+            DispatchMessage(&msg);
-            }
         }
     });
 }

src/wine-host/bridges/group.h

@@ -155,17 +155,6 @@ class GroupBridge {
      */
     void handle_incoming_connections();
 
-    /**
-     * Returns true if the message loop should not be run at this time. This is
-     * necessary because hosts will always call either `effEditOpen()` and then
-     * `effEditGetRect()` or the other way around. If the message loop is
-     * handled in between these two actions, then some plugins will either
-     * freeze or sometimes outright crash. Because every plugin has to be run
-     * from the same thread, this is a simple way to synchronize blocking the
-     * mesage loop between the different plugin instances.
-     */
-    bool should_skip_message_loop();
-
    private:
     /**
      * Listen on the group socket for incoming requests to host a new plugin

src/wine-host/bridges/vst2.cpp

@@ -262,10 +262,6 @@ Vst2Bridge::Vst2Bridge(MainContext& main_context,
     });
 }
 
-bool Vst2Bridge::should_skip_message_loop() const {
-    return std::holds_alternative<EditorOpening>(editor);
-}
-
 void Vst2Bridge::handle_dispatch() {
     sockets.host_vst_dispatch.receive_events(
         std::nullopt, [&](Event& event, bool /*on_main_thread*/) {
@@ -337,20 +333,6 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
     // We have to intercept GUI open calls since we can't use
     // the X11 window handle passed by the host
     switch (opcode) {
-        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)) {
-                editor = EditorOpening{};
-            }
-
-            return plugin->dispatcher(plugin, opcode, index, value, data,
-                                      option);
-        } break;
         case effEditOpen: {
             // Create a Win32 window through Wine, embed it into the window
             // provided by the host, and let the plugin embed itself into
@@ -361,8 +343,8 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
             // should get a unique window class
             const std::string window_class =
                 "yabridge plugin " + sockets.base_dir.string();
-            Editor& editor_instance = editor.emplace<Editor>(
+            Editor& editor_instance =
-                config, window_class, x11_handle, plugin);
+                editor.emplace(config, window_class, x11_handle, plugin);
 
             return plugin->dispatcher(plugin, opcode, index, value,
                                       editor_instance.get_win32_handle(),
@@ -373,7 +355,7 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
                 plugin->dispatcher(plugin, opcode, index, value, data, option);
 
             // Cleanup is handled through RAII
-            editor = std::monostate();
+            editor.reset();
 
             return return_value;
         } break;
@@ -385,29 +367,24 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
 }
 
 void Vst2Bridge::handle_win32_events() {
-    std::visit(overload{[](Editor& editor) { editor.handle_win32_events(); },
+    if (editor) {
-                        [](std::monostate&) {
+        editor->handle_win32_events();
-                            MSG msg;
+    } else {
+        MSG msg;
 
-                            for (int i = 0;
+        for (int i = 0; i < max_win32_messages &&
-                                 i < max_win32_messages &&
+                        PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE);
-                                 PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE);
+             i++) {
-                                 i++) {
+            TranslateMessage(&msg);
-                                TranslateMessage(&msg);
+            DispatchMessage(&msg);
-                                DispatchMessage(&msg);
+        }
-                            }
+    }
-                        },
-                        [](EditorOpening&) {
-                            // Don't handle any events in this particular case
-                            // as explained in `Vst2Bridge::editor`
-                        }},
-               editor);
 }
 
 void Vst2Bridge::handle_x11_events() {
-    std::visit(overload{[](Editor& editor) { editor.handle_x11_events(); },
+    if (editor) {
-                        [](auto&) {}},
+        editor->handle_x11_events();
-               editor);
+    }
 }
 
 class HostCallbackDataConverter : DefaultDataConverter {

src/wine-host/bridges/vst2.h

@@ -35,13 +35,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.
@@ -78,19 +71,6 @@ class Vst2Bridge {
                std::string plugin_dll_path,
                std::string endpoint_base_dir);
 
-    /**
-     * 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. For
-     * individually hosted plugins this check is done implicitely in
-     * `Vst2Bridge::handle_win32_events()`.
-     */
-    bool should_skip_message_loop() const;
-
     /**
      * Handle events until the plugin exits. The actual events are posted to
      * `main_context` to ensure that all operations to could potentially
@@ -105,17 +85,17 @@ class Vst2Bridge {
     void handle_dispatch();
 
     /**
-     * Handle X11 events for the editor window if it is open. This can be run
+     * Handle X11 events for the editor window if it is open. This can safely be
-     * safely from any thread.
+     * run from any thread.
      */
     void handle_x11_events();
 
     /**
      * Run the message loop for this plugin. This is only used for the
-     * individual plugin host. When hosting multiple plugins, a simple central
+     * individual plugin host, so that we can filter out some unnecessary timer
-     * message loop with a check to `should_skip_message_loop()` should be used
+     * events. When hosting multiple plugins, a simple central message loop
-     * instead. This is run on a timer in the same IO context as the one that
+     * should be used instead. This is run on a timer in the same IO context as
-     * handles the events, i.e. `main_context`.
+     * the one that handles the events, i.e. `main_context`.
      *
      * 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
@@ -224,25 +204,7 @@ 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:
-     *
-     * - `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<std::monostate, Editor, EditorOpening> editor;
+    std::optional<Editor> editor;
 };