Merge branch 'feature/xembed-workaround'

README.md

@@ -6,17 +6,11 @@ Yet Another way to use Windows VST2 plugins in Linux VST hosts.
 
 There are a few things that should be done before releasing this, including:
 
-- Implement missing features:
-  - Small quality of life related GUI fixes. Wine's XEmbed implementation
-    doesn't always update the reparented window's client area when the window
-    gets resized. The current workaround works much better than not doing
-    anything at all, but it isn't fully reliably yet.
 - Fix implementation bugs:
-  - Serum's GUI has redrawing issues when using XEmbed, even though it doesn't
+  - Valhalla DSP plugins have a GUI where either the dropdowns appear in the
-    have this issue when running in a standalone window or when simply
+    wrong place or all mouse based events are calculated with incorrect
-    reparenting the window without XEmbed. None of the other plugin's I've tried
+    coordinates. This definitely has to do with the way embedding is handled but
-    have this issue, so it might have something to do with d2d1 or the way Serum
+    none of the other plugins I've tested have this issue.
-    uses it.
 - Add missing details if any to the architecture section.
 - Document what this has been tested on and what does or does not work.
 - Document wine32 support.

src/wine-host/editor.cpp

@@ -3,40 +3,24 @@
 // The Win32 API requires you to hardcode identifiers for tiemrs
 constexpr size_t idle_timer_id = 1337;
 
-constexpr char xembed_proeprty[] = "_XEMBED";
+/**
-constexpr char xembed_info_proeprty[] = "_XEMBED_INFO";
+ * The most significant bit in an event's response type is used to indicate
+ * whether the event source.
+ */
+constexpr uint16_t event_type_mask = ((1 << 7) - 1);
 
-// Constants from the XEmbed spec
+/**
-constexpr uint32_t xembed_protocol_version = 0;
+ * Return the X11 window handle for the window if it's currently open.
-
+ */
-constexpr uint32_t xembed_embedded_notify_msg = 0;
+xcb_window_t get_x11_handle(HWND win32_handle);
-constexpr uint32_t xembed_window_activate_msg = 1;
-constexpr uint32_t xembed_focus_in_msg = 4;
-
-constexpr uint32_t xembed_focus_first = 1;
 
 ATOM register_window_class(std::string window_class_name);
 
 Editor::Editor(std::string window_class_name)
     : window_class(register_window_class(window_class_name)),
-      x11_connection(xcb_connect(nullptr, nullptr), &xcb_disconnect) {
+      x11_connection(xcb_connect(nullptr, nullptr), &xcb_disconnect) {}
-    // We need a bunch of property atoms for the XEmbed protocol
-    xcb_generic_error_t* err;
 
-    const auto xembed_cookie = xcb_intern_atom(
+HWND Editor::open(AEffect* effect) {
-        x11_connection.get(), 0, strlen(xembed_proeprty), xembed_proeprty);
-    const auto xembed_info_cookie =
-        xcb_intern_atom(x11_connection.get(), 0, strlen(xembed_info_proeprty),
-                        xembed_info_proeprty);
-
-    xcb_xembed =
-        xcb_intern_atom_reply(x11_connection.get(), xembed_cookie, &err)->atom;
-    xcb_xembed_info =
-        xcb_intern_atom_reply(x11_connection.get(), xembed_info_cookie, &err)
-            ->atom;
-}
-
-HWND Editor::open(AEffect* effect, xcb_window_t parent_window_handle) {
     // 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 expect) and
@@ -46,13 +30,12 @@ HWND Editor::open(AEffect* effect, xcb_window_t parent_window_handle) {
             CreateWindowEx(WS_EX_TOOLWINDOW | WS_EX_ACCEPTFILES,
                            reinterpret_cast<LPCSTR>(window_class),
                            "yabridge plugin", WS_POPUP, CW_USEDEFAULT,
-                           CW_USEDEFAULT, 2048, 2048, nullptr, nullptr,
+                           CW_USEDEFAULT, 2560, 1440, nullptr, nullptr,
                            GetModuleHandle(nullptr), this),
             &DestroyWindow);
 
     // Needed to send update messages on a timer
     plugin = effect;
-    parent_window = parent_window_handle;
 
     // 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
@@ -62,14 +45,19 @@ HWND Editor::open(AEffect* effect, xcb_window_t parent_window_handle) {
     // the plugin is not busy.
     SetTimer(win32_handle->get(), idle_timer_id, 100, nullptr);
 
-    const uint32_t event_mask = XCB_EVENT_MASK_VISIBILITY_CHANGE;
-    xcb_change_window_attributes(x11_connection.get(), parent_window,
-                                 XCB_CW_EVENT_MASK, &event_mask);
-    xcb_flush(x11_connection.get());
-
     return win32_handle->get();
 }
 
+bool Editor::resize(const int width, const int height) {
+    if (!win32_handle.has_value()) {
+        return false;
+    }
+
+    SetWindowPos(win32_handle->get(), HWND_TOP, 0, 0, width, height, 0);
+
+    return true;
+}
+
 void Editor::close() {
     // RAII will destroy the window and tiemrs for us
     win32_handle = std::nullopt;
@@ -78,30 +66,29 @@ void Editor::close() {
     //       everything for us?
 }
 
-bool Editor::xembed() {
+bool Editor::embed_into(const size_t parent_window_handle) {
     if (!win32_handle.has_value()) {
         return false;
     }
 
-    // This follows the embedding procedure specified in the XEmbed sped:
+    child_window = get_x11_handle(win32_handle->get());
-    // https://specifications.freedesktop.org/xembed-spec/xembed-spec-latest.html
+    parent_window = parent_window_handle;
-    // under 'Embedding life cycle
-    // Sadly there's doesn't seem to be any implementation of this available as
-    // a library
-    const size_t child_window = get_x11_handle().value();
 
+    // See the X11 events part of `Editor::handle_events`.
+    // const uint32_t child_event_mask = XCB_EVENT_MASK_STRUCTURE_NOTIFY;
+    // xcb_change_window_attributes(x11_connection.get(), child_window,
+    //                              XCB_CW_EVENT_MASK, &child_event_mask);
+    const uint32_t parent_event_mask = XCB_EVENT_MASK_STRUCTURE_NOTIFY;
+    xcb_change_window_attributes(x11_connection.get(), parent_window,
+                                 XCB_CW_EVENT_MASK, &parent_event_mask);
+    xcb_flush(x11_connection.get());
+
+    // Embed the Win32 window into the window provided by the host. Instead of
+    // using the XEmbed protocol, we'll register a few events and manage the
+    // child window ourselves. This is a hack to work around the issue's
+    // described in `Editor`'s docstring'.
     xcb_reparent_window(x11_connection.get(), child_window, parent_window, 0,
                         0);
-
-    // Tell the window from Wine it's embedded into the window provided by the
-    // host
-    send_xembed_event(child_window, xembed_embedded_notify_msg, 0,
-                      parent_window, xembed_protocol_version);
-
-    send_xembed_event(child_window, xembed_focus_in_msg, xembed_focus_first, 0,
-                      0);
-    send_xembed_event(child_window, xembed_window_activate_msg, 0, 0, 0);
-
     xcb_map_window(x11_connection.get(), child_window);
     xcb_flush(x11_connection.get());
 
@@ -137,69 +124,55 @@ void Editor::handle_events() {
         }
 
         // Handle X11 events
-        // TODO: We don't listen for XEmbed property changes for mapping and
+        // TODO: Check if we should forward other events mostly to prevent
-        //       unmapping the window as described in the spec. Is this
+        //       unnecessary GUI processing in the background
-        //       something we should do?
+        // TODO: Check whether drag and drop works out of the box
-        // TODO: Also, increasing window size does not work reliably, any way we
+        xcb_generic_event_t* generic_event;
-        //       could make this work better?
+        while ((generic_event = xcb_poll_for_event(x11_connection.get())) !=
-        xcb_generic_event_t* event;
+               nullptr) {
-        while ((event = xcb_poll_for_event(x11_connection.get())) != nullptr) {
+            switch (generic_event->response_type & event_type_mask) {
-            // The most significant bit in an event's response type is used to
+                case XCB_CONFIGURE_NOTIFY: {
-            // indicate whether the event source
+                    xcb_configure_notify_event_t event =
-            switch (event->response_type & ((1 << 7) - 1)) {
+                        *reinterpret_cast<xcb_configure_notify_event_t*>(
-                case XCB_VISIBILITY_NOTIFY: {
+                            generic_event);
-                    xcb_visibility_notify_event_t visibility_event =
+                    if (event.window != parent_window) {
-                        *reinterpret_cast<xcb_visibility_notify_event_t*>(
+                        break;
-                            event);
-
-                    // Wine's XEmbed implementation is a bit iffy when it comes
-                    // to window size changes, including editor windows being
-                    // increased in size by the DAW when first opening the
-                    // editor. Restarting the XEmbed protocol on visibility and
-                    // size changes is an easy workaround, but it's not 100%
-                    // reliable.
-                    if (visibility_event.window == parent_window &&
-                        visibility_event.state !=
-                            XCB_VISIBILITY_FULLY_OBSCURED) {
-                        xembed();
                     }
+
+
+                    // We're purposely not using XEmbed. This has the
+                    // consequence that wine still thinks that any X and Y
+                    // coordinates are relative to the x11 window root instead
+                    // of the parent window provided by the DAW, causing all
+                    // sorts of GUI interactions to break. To alleviate this
+                    // we'll just lie to Wine and tell it that it's located at
+                    // the parent window's location. We'll only send the event
+                    // instead of actually configuring the window.
+                    xcb_configure_notify_event_t translated_event{};
+                    translated_event.response_type = XCB_CONFIGURE_NOTIFY;
+                    translated_event.event = child_window;
+                    translated_event.window = child_window;
+                    translated_event.width = event.width;
+                    translated_event.height = event.height;
+                    translated_event.x = event.x;
+                    translated_event.y = event.y;
+
+                    xcb_send_event(x11_connection.get(), false, child_window,
+                                   XCB_EVENT_MASK_STRUCTURE_NOTIFY |
+                                       XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY,
+                                   reinterpret_cast<char*>(&translated_event));
+                    xcb_flush(x11_connection.get());
+
+                    // The client area of the Win32 window doesn't expand
+                    // automatically
+                    resize(event.width, event.height);
                 } break;
             }
-
+            free(generic_event);
-            free(event);
         }
     }
 }
 
-std::optional<size_t> Editor::get_x11_handle() {
-    if (!win32_handle.has_value()) {
-        return std::nullopt;
-    }
-
-    return reinterpret_cast<size_t>(
-        GetProp(win32_handle.value().get(), "__wine_x11_whole_window"));
-}
-
-void Editor::send_xembed_event(const xcb_window_t& window,
-                               const uint32_t message,
-                               const uint32_t detail,
-                               const uint32_t data1,
-                               const uint32_t data2) {
-    xcb_client_message_event_t event;
-    event.response_type = XCB_CLIENT_MESSAGE;
-    event.type = xcb_xembed;
-    event.window = window;
-    event.format = 32;
-    event.data.data32[0] = XCB_CURRENT_TIME;
-    event.data.data32[1] = message;
-    event.data.data32[2] = detail;
-    event.data.data32[3] = data1;
-    event.data.data32[4] = data2;
-
-    xcb_send_event(x11_connection.get(), false, window, XCB_EVENT_MASK_NO_EVENT,
-                   reinterpret_cast<char*>(&event));
-}
-
 LRESULT CALLBACK window_proc(HWND handle,
                              UINT message,
                              WPARAM wParam,
@@ -241,6 +214,11 @@ LRESULT CALLBACK window_proc(HWND handle,
     return DefWindowProc(handle, message, wParam, lParam);
 }
 
+xcb_window_t get_x11_handle(HWND win32_handle) {
+    return reinterpret_cast<size_t>(
+        GetProp(win32_handle, "__wine_x11_whole_window"));
+}
+
 ATOM register_window_class(std::string window_class_name) {
     WNDCLASSEX window_class{};
 

src/wine-host/editor.h

@@ -20,6 +20,21 @@
  * A wrapper around the win32 windowing API to create and destroy editor
  * windows. A VST plugin can embed itself in that window, and we can then later
  * embed the window in a VST host provided X11 window.
+ *
+ * This was originally implemented using XEmbed. While that sounded like the
+ * right thing to do, there were a few small issues with Wine's XEmbed
+ * implementation. The most important of which is that resizing GUIs sometimes
+ * works fine, but often fails to expand the embedded window's client area
+ * leaving part of the window inaccessible. There are also some a small number
+ * of plugins (such as Serum) that have rendering issues when using XEmbed but
+ * otherwise draw fine when running standalone or when just reparenting the
+ * window without using XEmbed. If anyone knows how to work around these two
+ * issues, please let me know and I'll switch to using XEmbed again.
+ *
+ * This workaround was inspired by LinVST.
+ *
+ * TODO: Refactor this so we don't need to stage initialization and just add an
+ *       `std::optional<Editor>` to `PluginBridge` instead
  */
 class Editor {
    public:
@@ -30,16 +45,36 @@ class Editor {
     Editor(std::string window_class_name);
 
     /**
-     * Open a window and return a handle to the new Win32 window that can be
+     * Open a window, embed it into the DAW's parent window and return a handle
-     * used by the hosted VST plugin.
+     * to the new Win32 window that can be used by the hosted VST plugin.
      *
      * @param effect The plugin this window is being created for. Used to send
      *   `effEditIdle` messages on a timer.
+     *
+     * @return The Win32 window handle of the newly created window.
+     */
+    HWND open(AEffect* effect);
+
+    void close();
+
+    /**
+     * Resize the window to match the given size, if open.
+     *
+     * @return Whether the resizing was succesful. Will return false if the
+     *   editor isn't open.
+     */
+    bool resize(const int width, const int height);
+
+    /**
+     * Embed the (open) window into a parent window.
+     *
      * @param parent_window_handle The X11 window handle passed by the VST host
      *   for the editor to embed itself into.
+     *
+     * @return Whether the embedding was succesful. Will return false if the
+     *   window is not open.
      */
-    HWND open(AEffect* effect, xcb_window_t parent_window_handle);
+    bool embed_into(const size_t parent_window_handle);
-    void close();
 
     /**
      * Pump messages from the editor GUI's event loop until all events are
@@ -48,41 +83,21 @@ class Editor {
      */
     void handle_events();
 
-    /**
-     * Embed the (open) window into the parent window.
-     *
-     * @return Whether the embedding was succesful. Will return false if the
-     *   window is not open.
-     */
-    bool xembed();
-
     // Needed to handle idle updates through a timer
     AEffect* plugin;
 
+   private:
     /**
      * The window handle of the editor window created by the DAW.
      */
     xcb_window_t parent_window;
-
-   private:
     /**
-     * Return the X11 window handle for the window if it's currently open.
+     * The X11 window handle of the window belonging to  `win32_handle`.
      */
-    std::optional<size_t> get_x11_handle();
+    xcb_window_t child_window;
 
     /**
-     * Send an XEmbed message to a window. See the spec for more information.
+     * The Win32 window class registered for the windows window.
-     *
-     * https://specifications.freedesktop.org/xembed-spec/xembed-spec-latest.html#lifecycle
-     */
-    void send_xembed_event(const xcb_window_t& window,
-                           const uint32_t message,
-                           const uint32_t detail,
-                           const uint32_t data1,
-                           const uint32_t data2);
-
-    /**
-     * The Win32 window class registered for the window window.
      */
     ATOM window_class;
 
@@ -95,6 +110,4 @@ class Editor {
         win32_handle;
 
     std::unique_ptr<xcb_connection_t, decltype(&xcb_disconnect)> x11_connection;
-    xcb_atom_t xcb_xembed;
-    xcb_atom_t xcb_xembed_info;
 };

src/wine-host/plugin-bridge.cpp

@@ -238,13 +238,20 @@ intptr_t PluginBridge::dispatch_wrapper(AEffect* plugin,
                                       option);
         } break;
         case effEditOpen: {
-            const auto x11_handle = reinterpret_cast<size_t>(data);
+            const auto win32_handle = editor.open(plugin);
-            const auto win32_handle = editor.open(plugin, x11_handle);
 
-            // The actual XEmbed handling is done after the host's window has
+            const auto return_value = plugin->dispatcher(
-            // been set to the correct size, in `Editor::handle_events()`
+                plugin, opcode, index, value, win32_handle, option);
-            return plugin->dispatcher(plugin, opcode, index, value,
+            if (return_value == 0) {
-                                      win32_handle, option);
+                return 0;
+            }
+
+            // If opening the editor was succesful, reparent it to the window
+            // provided by the DAW
+            const auto x11_handle = reinterpret_cast<size_t>(data);
+            editor.embed_into(x11_handle);
+
+            return return_value;
         } break;
         case effEditClose: {
             const intptr_t return_value =
@@ -291,6 +298,17 @@ class HostCallbackDataConverter : DefaultDataConverter {
             case audioMasterGetTime:
                 return WantsVstTimeInfo{};
                 break;
+            case audioMasterSizeWindow:
+                // Plugins use this opcode to indicate that their editor should
+                // be resized. This is handled implicitly when handling the
+                // ConfigureNotify X11 events but handling this here as well
+                // makes the resizing look much smoother.
+                // TODO: Check if this actually makes drag resizing feel better,
+                //       otherwise just remove this
+                editor.resize(value, index);
+
+                return DefaultDataConverter::read(opcode, index, value, data);
+                break;
             case audioMasterIOChanged:
                 // This is a helpful event that indicates that the VST plugin's
                 // `AEffect` struct has changed. Writing these results back is