Fix GUI related data races within Serum

src/wine-host/editor.cpp

@@ -32,10 +32,9 @@ ATOM register_window_class(std::string window_class_name);
 
 Editor::Editor(const std::string& window_class_name,
                AEffect* effect,
+               std::mutex& effect_mutex,
                const size_t parent_window_handle)
-    :  // Needed to send update messages on a timer
+    : window_class(register_window_class(window_class_name)),
-      plugin(effect),
-      window_class(register_window_class(window_class_name)),
       // Create a window without any decoratiosn for easy embedding. The
       // combination of `WS_EX_TOOLWINDOW` and `WS_POPUP` causes the window to
       // be drawn without any decorations (making resizes behave as you'd
@@ -56,6 +55,9 @@ Editor::Editor(const std::string& window_class_name,
                    &DestroyWindow),
       parent_window(parent_window_handle),
       child_window(get_x11_handle(win32_handle.get())),
+      // Needed to send update messages on a timer
+      plugin(effect),
+      processing_mutex(effect_mutex),
       x11_connection(xcb_connect(nullptr, nullptr), &xcb_disconnect) {
     // The Win32 API will block the `DispatchMessage` call when opening e.g. a
     // dropdown, but it will still allow timers to be run so the GUI can still
@@ -83,31 +85,24 @@ Editor::Editor(const std::string& window_class_name,
     ShowWindow(win32_handle.get(), SW_SHOWNORMAL);
 }
 
+void Editor::send_idle_event() {
+    plugin->dispatcher(plugin, effEditIdle, 0, 0, nullptr, 0);
+}
+
 void Editor::handle_events() {
-    // Process any remaining events, otherwise we won't be able to interact with
+    win32_event_loop();
-    // the window
-    MSG msg;
 
-    // This timer triggers the `effEditIdle` event, causing the plugin to update
+    {
-    // its internal state. THis has to be done before any `WM_PAINT` events are
+        // Always send the `effEditIdle` event manually instead of relying on
-    // fired as that might cause issues with certain plugins. This is
+        // the timer to match the update frequency with the native VST host.
-    // implemented as a timer event so the GUI can still update while it's being
+        // Because some plugins, such as those using GDI+ like Serum, have data
-    // blocked by a dropdown or a message box.
+        // race issues when drawing at the same time as we're processing sound,
-    SendMessage(win32_handle.get(), WM_TIMER, idle_timer_id, 0);
+        // we'll update the GUI and process the resulting `WM_PAINT` event while
+        // temporarily blocking the processing thread.
+        std::lock_guard lock(processing_mutex);
 
-    // The second argument has to be null since we not only want to handle
+        send_idle_event();
-    // events for this window but also for all child windows (i.e. dropdowns). I
+        win32_event_loop();
-    // spent way longer debugging this than I want to admit.
-    while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
-        // We already fired the idle timer event above. This timer will still be
-        // run implicitely by the child window when the event loop gets blocked.
-        if (msg.message == WM_TIMER && msg.wParam == idle_timer_id &&
-            msg.hwnd == win32_handle.get()) {
-            continue;
-        }
-
-        TranslateMessage(&msg);
-        DispatchMessage(&msg);
     }
 
     // Handle X11 events
@@ -196,8 +191,7 @@ LRESULT CALLBACK window_proc(HWND handle,
             // these either when the host sends `effEditIdle` themself, or
             // periodically when the GUI is being blocked by a dropdown or a
             // message box.
-            editor->plugin->dispatcher(editor->plugin, effEditIdle, 0, 0,
+            editor->send_idle_event();
-                                       nullptr, 0);
             return 0;
         } break;
     }
@@ -205,6 +199,28 @@ LRESULT CALLBACK window_proc(HWND handle,
     return DefWindowProc(handle, message, wParam, lParam);
 }
 
+void Editor::win32_event_loop() {
+    MSG msg;
+
+    // The null value for the second argument is needed to handle interaction
+    // with child GUI components
+    while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
+        // This timer would periodically send `effEditIdle` events so the editor
+        // remains responsive even during blocking GUI operations such as open
+        // dropdowns or message boxes. We filter it out here because we will
+        // send sent the event manually every time the host calls
+        // `effEditIdle()`. It will still be fired implicitely when the GUI
+        // thread gets blocked.
+        if (msg.message == WM_TIMER && msg.wParam == idle_timer_id &&
+            msg.hwnd == win32_handle.get()) {
+            continue;
+        }
+
+        TranslateMessage(&msg);
+        DispatchMessage(&msg);
+    }
+}
+
 xcb_window_t get_x11_handle(HWND win32_handle) {
     return reinterpret_cast<size_t>(
         GetProp(win32_handle, "__wine_x11_whole_window"));

src/wine-host/editor.h

@@ -13,6 +13,7 @@
 #include <windows.h>
 
 #include <memory>
+#include <mutex>
 #include <optional>
 #include <string>
 
@@ -43,6 +44,8 @@ class Editor {
      *   windows.
      * @param effect The plugin this window is being created for. Used to send
      *   `effEditIdle` messages on a timer.
+     * @param processing_mutex The mutex belonging to `effect` audio processing
+     *   routine.
      * @param parent_window_handle The X11 window handle passed by the VST host
      *   for the editor to embed itself into.
      *
@@ -50,16 +53,22 @@ class Editor {
      */
     Editor(const std::string& window_class_name,
            AEffect* effect,
+           std::mutex& processing_mutex,
            const size_t parent_window_handle);
 
+    /**
+     * Send a single `effEditIdle` event to the plugin to allow it to update its
+     * GUI state. This is called periodically from a timer while the GUI is
+     * being blocked.
+     */
+    void send_idle_event();
+
     /**
      * Pump messages from the editor GUI's event loop until all events are
      * process. Must be run from the same thread the GUI was created in because
      * of Win32 limitations. I guess that's what `effEditIdle` is for.
      */
     void handle_events();
-    // Needed to handle idle updates through a timer
-    AEffect* plugin;
 
    private:
     /**
@@ -76,6 +85,14 @@ class Editor {
         win32_handle;
 
    private:
+    /**
+     * Run the win32 message handling loop, filtering out the events generated
+     * by the `effEditIdle` timer since those are sent manually on every
+     * invocation of `handle_events()` to match the `effEditIdle` events sent by
+     * the DAW.
+     */
+    void win32_event_loop();
+
     /**
      * The window handle of the editor window created by the DAW.
      */
@@ -85,10 +102,19 @@ class Editor {
      */
     xcb_window_t child_window;
 
+    /**
+     *Needed to handle idle updates through a timer
+     */
+    AEffect* plugin;
+    /**
+     * The mutex belonging to `plugin`'s audio processing routine, see
+     * `PluginBridge::processing_mutex`'s docstring for more information.
+     */
+    std::mutex& processing_mutex;
+
     /**
      * A pointer to the currently active window. Will be a null pointer if no
      * window is active.
      */
-
     std::unique_ptr<xcb_connection_t, decltype(&xcb_disconnect)> x11_connection;
 };

src/wine-host/plugin-bridge.cpp

@@ -161,6 +161,8 @@ PluginBridge::PluginBridge(std::string plugin_dll_path,
 
             // TODO: Check if the plugin doesn't support `processReplacing` and
             //       call the legacy `process` function instead
+            // TODO: Reuse the output_buffers and resize if necessary to avoid
+            //       unnecessary allocations
             std::vector<std::vector<float>> output_buffers(
                 plugin->numOutputs, std::vector<float>(request.sample_frames));
 
@@ -175,8 +177,13 @@ PluginBridge::PluginBridge(std::string plugin_dll_path,
                 outputs.push_back(buffer.data());
             }
 
-            plugin->processReplacing(plugin, inputs.data(), outputs.data(),
+            {
-                                     request.sample_frames);
+                // Some plugins (e.g. Serum) don't allow audio processing while
+                // the GUI is being updated
+                std::lock_guard lock(processing_mutex);
+                plugin->processReplacing(plugin, inputs.data(), outputs.data(),
+                                         request.sample_frames);
+            }
 
             AudioBuffers response{output_buffers, request.sample_frames};
             write_object(host_vst_process_replacing, response, process_buffer);
@@ -233,7 +240,8 @@ intptr_t PluginBridge::dispatch_wrapper(AEffect* plugin,
             // provided by the host, and let the plugin embed itself into the
             // Wine window
             const auto x11_handle = reinterpret_cast<size_t>(data);
-            editor.emplace("yabridge plugin", plugin, x11_handle);
+            editor.emplace("yabridge plugin", plugin, processing_mutex,
+                           x11_handle);
 
             return plugin->dispatcher(plugin, opcode, index, value,
                                       editor->win32_handle.get(), option);

src/wine-host/plugin-bridge.h

@@ -97,6 +97,17 @@ class PluginBridge {
      * handle.
      */
     AEffect* plugin;
+    /**
+     * A mutex to prevent certain operations from being performed
+     * simultaneously. The VST specification does not specify which operations
+     * have to be thread safe and which do not. Most plugins don't have any
+     * issues as long as all editor related events are sent from the GUI thread.
+     * At the moment this mutex is only used to prevent GUI updates (through the
+     * `effEditIdle()` event and the `WM_PAINT` message that follows) and audio
+     * processing from being performed at the same time. This prevents data
+     * races within Serum.
+     */
+    std::mutex processing_mutex;
 
     boost::asio::io_context io_context;
     boost::asio::local::stream_protocol::endpoint socket_endpoint;