From f235bdf9a12783f1bdadb04eca6fd93330cb282a Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 19 Apr 2020 15:16:51 +0200 Subject: [PATCH] Fix GUI related data races within Serum --- src/wine-host/editor.cpp | 70 ++++++++++++++++++++------------- src/wine-host/editor.h | 32 +++++++++++++-- src/wine-host/plugin-bridge.cpp | 14 +++++-- src/wine-host/plugin-bridge.h | 11 ++++++ 4 files changed, 94 insertions(+), 33 deletions(-) diff --git a/src/wine-host/editor.cpp b/src/wine-host/editor.cpp index d9da34c3..26d2494d 100644 --- a/src/wine-host/editor.cpp +++ b/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 - plugin(effect), - window_class(register_window_class(window_class_name)), + : 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 - // the window - MSG msg; + win32_event_loop(); - // 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 - // fired as that might cause issues with certain plugins. This is - // implemented as a timer event so the GUI can still update while it's being - // blocked by a dropdown or a message box. - SendMessage(win32_handle.get(), WM_TIMER, idle_timer_id, 0); + { + // Always send the `effEditIdle` event manually instead of relying on + // the timer to match the update frequency with the native VST host. + // Because some plugins, such as those using GDI+ like Serum, have data + // race issues when drawing at the same time as we're processing sound, + // 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 - // events for this window but also for all child windows (i.e. dropdowns). I - // 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); + send_idle_event(); + win32_event_loop(); } // 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, - nullptr, 0); + editor->send_idle_event(); 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( GetProp(win32_handle, "__wine_x11_whole_window")); diff --git a/src/wine-host/editor.h b/src/wine-host/editor.h index 44818365..9a2400d1 100644 --- a/src/wine-host/editor.h +++ b/src/wine-host/editor.h @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -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 x11_connection; }; diff --git a/src/wine-host/plugin-bridge.cpp b/src/wine-host/plugin-bridge.cpp index 19b7daf9..6cd1585b 100644 --- a/src/wine-host/plugin-bridge.cpp +++ b/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> output_buffers( plugin->numOutputs, std::vector(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(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); diff --git a/src/wine-host/plugin-bridge.h b/src/wine-host/plugin-bridge.h index 45355f1e..c98f0fb9 100644 --- a/src/wine-host/plugin-bridge.h +++ b/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;