From 5a0c3c4627d4af4429a8a7f3f3e861ef1f1287af Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 27 May 2020 14:05:57 +0200 Subject: [PATCH] Simplify the opening editor behaviour again This partially reverts 16fce5577d30ddb09769aa163f58c66c0025b7ea. --- src/wine-host/bridges/vst2.cpp | 96 ++++++++++++++++------------------ src/wine-host/bridges/vst2.h | 39 ++++++++++---- 2 files changed, 73 insertions(+), 62 deletions(-) diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index a52683de..726b1a9c 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -146,7 +146,7 @@ Vst2Bridge::Vst2Bridge(std::string plugin_dll_path, } bool Vst2Bridge::should_skip_message_loop() { - return editor_is_opening; + return std::holds_alternative(editor); } void Vst2Bridge::handle_dispatch_single() { @@ -362,27 +362,18 @@ 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()`, since this won't ever happen on Windows and - // plugins thus assume that this can't happen at all. If - // `effEditOpen()` has not yet been called, then we'll mark the - // editor as currently opening to prevent the message loop from - // running. - editor_is_opening = !editor.has_value(); + // `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 = EditorOpening{}; + } 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 @@ -392,17 +383,19 @@ 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(window_class, plugin, x11_handle); - editor.emplace(window_class, plugin, x11_handle); return plugin->dispatcher(plugin, opcode, index, value, - editor->win32_handle.get(), option); + editor_instance.win32_handle.get(), + option); } break; case effEditClose: { const intptr_t return_value = plugin->dispatcher(plugin, opcode, index, value, data, option); // Cleanup is handled through RAII - editor.reset(); + editor = std::monostate(); return return_value; } break; @@ -414,28 +407,28 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin, } void Vst2Bridge::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()) { - return; - } + std::visit( + overload{[](Editor& editor) { editor.handle_win32_events(); }, + [](std::monostate&) { + MSG msg; - if (editor.has_value()) { - editor->handle_win32_events(); - } else { - MSG msg; - - while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { - TranslateMessage(&msg); - DispatchMessage(&msg); - } - } + while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { + TranslateMessage(&msg); + DispatchMessage(&msg); + } + }, + [](EditorOpening&) { + // Don't handle any events in this + // particular case as explained in + // `Vst2Bridge::editor` + }}, + editor); } void Vst2Bridge::handle_x11_events() { - if (editor.has_value()) { - editor->handle_x11_events(); - } + std::visit(overload{[](Editor& editor) { editor.handle_x11_events(); }, + [](auto&) {}}, + editor); } class HostCallbackDataConverter : DefaultDataConverter { @@ -453,17 +446,18 @@ class HostCallbackDataConverter : DefaultDataConverter { return WantsVstTimeInfo{}; break; case audioMasterIOChanged: - // This is a helpful event that indicates that the VST plugin's - // `AEffect` struct has changed. Writing these results back is - // done inside of `passthrough_event`. + // This is a helpful event that indicates that the VST + // plugin's `AEffect` struct has changed. Writing these + // results back is done inside of `passthrough_event`. return AEffect(*plugin); break; case audioMasterProcessEvents: return DynamicVstEvents(*static_cast(data)); break; - // We detect whether an opcode should return a string by checking - // whether there's a zeroed out buffer behind the void pointer. This - // works for any host, but not all plugins zero out their buffers. + // We detect whether an opcode should return a string by + // checking whether there's a zeroed out buffer behind the void + // pointer. This works for any host, but not all plugins zero + // out their buffers. case audioMasterGetVendorString: case audioMasterGetProductString: return WantsString{}; @@ -482,11 +476,11 @@ class HostCallbackDataConverter : DefaultDataConverter { void write(const int opcode, void* data, const EventResult& response) { switch (opcode) { case audioMasterGetTime: - // Write the returned `VstTimeInfo` struct into a field and make - // the function return a poitner to it in the function below. - // Depending on whether the host supported the requested time - // information this operations returns either a null pointer or - // a pointer to a `VstTimeInfo` object. + // Write the returned `VstTimeInfo` struct into a field and + // make the function return a poitner to it in the function + // below. Depending on whether the host supported the + // requested time information this operations returns either + // a null pointer or a pointer to a `VstTimeInfo` object. if (std::holds_alternative(response.payload)) { time_info = std::nullopt; } else { @@ -502,8 +496,8 @@ class HostCallbackDataConverter : DefaultDataConverter { intptr_t return_value(const int opcode, const intptr_t original) { switch (opcode) { case audioMasterGetTime: { - // Return a pointer to the `VstTimeInfo` object written in the - // function above + // Return a pointer to the `VstTimeInfo` object written in + // the function above VstTimeInfo* time_info_pointer = nullptr; if (time_info.has_value()) { time_info_pointer = &time_info.value(); diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 0622c891..5bd1fc93 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -33,6 +33,13 @@ #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. @@ -75,7 +82,9 @@ class Vst2Bridge { * 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. + * 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(); @@ -261,17 +270,25 @@ 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. * - * 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()`. + * 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::optional 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; + std::variant editor; };