Swap out std::thread for CreateThread

Not entirely sure why, but this gets rid of the impossible to debug data
races when Serum's GUI is being repainted while another thread is
calling `processReplacing`. This is possibly because std::thread does
not respect Windows calling conventions and CreateThread does.

src/wine-host/plugin-bridge.cpp

@@ -38,6 +38,13 @@ PluginBridge* current_bridge_isntance = nullptr;
 intptr_t VST_CALL_CONV
 host_callback_proxy(AEffect*, int, int, intptr_t, void*, float);
 
+// We need to use the `CreateThread` WinAPI functions instead of `std::thread`
+// to use the correct calling conventions within threads. Otherwise we'll get
+// some rare and impossible to debug data races in some particular plugins.
+uint32_t WINAPI handle_dispatch_midi_events_proxy(void*);
+uint32_t WINAPI handle_parameters_proxy(void*);
+uint32_t WINAPI handle_process_replacing_proxy(void*);
+
 /**
  * Fetch the Pluginbridge instance stored in one of the two pointers reserved
  * for the host of the hosted VST plugin. This is sadly needed as a workaround
@@ -122,76 +129,14 @@ PluginBridge::PluginBridge(std::string plugin_dll_path,
     // This works functionally identically to the `handle_dispatch()` function
     // below, but this socket will only handle midi events. This is needed
     // because of Win32 API limitations.
-    dispatch_midi_events_handler = std::thread([&]() {
+    dispatch_midi_events_handler = CreateThread(
-        while (true) {
+        nullptr, 0, handle_dispatch_midi_events_proxy, this, 0, nullptr);
-            passthrough_event(host_vst_dispatch_midi_events, std::nullopt,
-                              plugin, plugin->dispatcher);
-        }
-    });
 
-    parameters_handler = std::thread([&]() {
+    parameters_handler =
-        while (true) {
+        CreateThread(nullptr, 0, handle_parameters_proxy, this, 0, nullptr);
-            // Both `getParameter` and `setParameter` functions are passed
-            // through on this socket since they have a lot of overlap. The
-            // presence of the `value` field tells us which one we're dealing
-            // with.
-            auto request = read_object<Parameter>(host_vst_parameters);
-            if (request.value.has_value()) {
-                // `setParameter`
-                plugin->setParameter(plugin, request.index,
-                                     request.value.value());
 
-                ParameterResult response{std::nullopt};
+    process_replacing_handler = CreateThread(
-                write_object(host_vst_parameters, response);
+        nullptr, 0, handle_process_replacing_proxy, this, 0, nullptr);
-            } else {
-                // `getParameter`
-                float value = plugin->getParameter(plugin, request.index);
-
-                ParameterResult response{value};
-                write_object(host_vst_parameters, response);
-            }
-        }
-    });
-
-    process_replacing_handler = std::thread([&]() {
-        std::vector<std::vector<float>> output_buffers(plugin->numOutputs);
-
-        while (true) {
-            AudioBuffers request;
-            request = read_object(host_vst_process_replacing, request,
-                                  process_buffer);
-
-            // TODO: Check if the plugin doesn't support `processReplacing` and
-            //       call the legacy `process` function instead
-            // The process functions expect a `float**` for their inputs and
-            // their outputs
-            std::vector<float*> inputs;
-            for (auto& buffer : request.buffers) {
-                inputs.push_back(buffer.data());
-            }
-
-            // We reuse the buffers to avoid some unnecessary heap allocations,
-            // so we need to make sure the buffers are large enough since
-            // plugins can change their output configuration
-            std::vector<float*> outputs;
-            output_buffers.resize(plugin->numOutputs);
-            for (auto& buffer : output_buffers) {
-                buffer.resize(request.sample_frames);
-                outputs.push_back(buffer.data());
-            }
-
-            // 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);
-        }
-    });
 
     std::cout << "Finished initializing '" << plugin_dll_path << "'"
               << std::endl;
@@ -210,11 +155,81 @@ void PluginBridge::handle_dispatch() {
                                         _1, _2, _3, _4, _5, _6));
         }
     } catch (const boost::system::system_error&) {
-        // This happens when the sockets got closed because the plugin is being
+        // TODO: Implement this with a simple RAII wrapper just like we did for
-        // shut down. In that case we can just let the whole host terminate.
+        //       Win32 window classes
-        dispatch_midi_events_handler.detach();
+        CloseHandle(dispatch_midi_events_handler);
-        parameters_handler.detach();
+        CloseHandle(parameters_handler);
-        process_replacing_handler.detach();
+        CloseHandle(process_replacing_handler);
+    }
+}
+
+[[noreturn]] void PluginBridge::handle_dispatch_midi_events() {
+    while (true) {
+        passthrough_event(host_vst_dispatch_midi_events, std::nullopt, plugin,
+                          plugin->dispatcher);
+    }
+}
+
+[[noreturn]] void PluginBridge::handle_parameters() {
+    while (true) {
+        // Both `getParameter` and `setParameter` functions are passed
+        // through on this socket since they have a lot of overlap. The
+        // presence of the `value` field tells us which one we're dealing
+        // with.
+        auto request = read_object<Parameter>(host_vst_parameters);
+        if (request.value.has_value()) {
+            // `setParameter`
+            plugin->setParameter(plugin, request.index, request.value.value());
+
+            ParameterResult response{std::nullopt};
+            write_object(host_vst_parameters, response);
+        } else {
+            // `getParameter`
+            float value = plugin->getParameter(plugin, request.index);
+
+            ParameterResult response{value};
+            write_object(host_vst_parameters, response);
+        }
+    }
+}
+
+[[noreturn]] void PluginBridge::handle_process_replacing() {
+    std::vector<std::vector<float>> output_buffers(plugin->numOutputs);
+
+    while (true) {
+        AudioBuffers request;
+        request =
+            read_object(host_vst_process_replacing, request, process_buffer);
+
+        // TODO: Check if the plugin doesn't support `processReplacing` and
+        //       call the legacy `process` function instead
+        // The process functions expect a `float**` for their inputs and
+        // their outputs
+        std::vector<float*> inputs;
+        for (auto& buffer : request.buffers) {
+            inputs.push_back(buffer.data());
+        }
+
+        // We reuse the buffers to avoid some unnecessary heap allocations,
+        // so we need to make sure the buffers are large enough since
+        // plugins can change their output configuration
+        std::vector<float*> outputs;
+        output_buffers.resize(plugin->numOutputs);
+        for (auto& buffer : output_buffers) {
+            buffer.resize(request.sample_frames);
+            outputs.push_back(buffer.data());
+        }
+
+        // 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);
     }
 }
 
@@ -357,3 +372,15 @@ intptr_t VST_CALL_CONV host_callback_proxy(AEffect* effect,
     return get_bridge_instance(effect).host_callback(effect, opcode, index,
                                                      value, data, option);
 }
+
+uint32_t WINAPI handle_dispatch_midi_events_proxy(void* instance) {
+    static_cast<PluginBridge*>(instance)->handle_dispatch_midi_events();
+}
+
+uint32_t WINAPI handle_parameters_proxy(void* instance) {
+    static_cast<PluginBridge*>(instance)->handle_parameters();
+}
+
+uint32_t WINAPI handle_process_replacing_proxy(void* instance) {
+    static_cast<PluginBridge*>(instance)->handle_process_replacing();
+}

src/wine-host/plugin-bridge.h

@@ -28,7 +28,6 @@
 
 #include <boost/asio/local/stream_protocol.hpp>
 #include <mutex>
-#include <thread>
 
 #include "../common/logging.h"
 #include "editor.h"
@@ -63,6 +62,18 @@ class PluginBridge {
      */
     void handle_dispatch();
 
+    // These functions are the entry points for the `*_handler` threads defined
+    // below. They're defined here because we can't use lambdas with WinAPI's
+    // `CreateThread` which is needed to support the proper call conventions the
+    // VST plugins expect.
+    [[noreturn]] void handle_dispatch_midi_events();
+    [[noreturn]] void handle_parameters();
+    [[noreturn]] void handle_process_replacing();
+
+    /**
+     * Forward the host callback made by the plugin to the host and return the
+     * results.
+     */
     intptr_t host_callback(AEffect*, int, int, intptr_t, void*, float);
 
     /**
@@ -139,20 +150,25 @@ class PluginBridge {
      */
     boost::asio::local::stream_protocol::socket vst_host_aeffect;
 
+    // These threads are implemented using `CreateThread` rather than
+    // `std::thread` because in some cases `std::thread` in winelib causes very
+    // hard to debug data races within plugins such as Serum. This might be
+    // caused by calling conventions being handled differently.
+
     /**
      * The thread that specifically handles `effProcessEvents` opcodes so the
      * plugin can still receive midi during GUI interaction to work around Win32
      * API limitations.
      */
-    std::thread dispatch_midi_events_handler;
+    HANDLE dispatch_midi_events_handler;
     /**
      * The thread that responds to `getParameter` and `setParameter` requests.
      */
-    std::thread parameters_handler;
+    HANDLE parameters_handler;
     /**
      * The thread that handles calls to `processReplacing` (and `process`).
      */
-    std::thread process_replacing_handler;
+    HANDLE process_replacing_handler;
 
     /**
      * A binary semaphore to prevent race conditions from the host callback