Refactor event receiving to optimize MIDI handlign

This keeps compatibility with some weirdly designed plugins (such as
Kontakt) while avoiding some unnecessary data transformations. Before
this we'd convert from a `DynamicVstEvents` object to a `VstEvents`
object, back to a `DynamicVstEvents` and then finally back into another
`VstEvents` object. With this change we can skip the second half of the
conversions.

README.md

@@ -289,16 +289,19 @@ process works as follows:
    sockets. The actual binary serialization is handled using
    [bitsery](https://github.com/fraillt/bitsery).
 
-   Sending and receiving events happen in the `send_event()` and
-   `passthrough_event()` functions. The `passthrough_event()` function calls the
-   callback functions and handles the marshalling between our data types and the
-   VST API's different pointer types. Reading data and writing the results back
-   for host-to-plugin `dispatcher()` calls and for plugin-to-host
+   Sending and receiving host -> plugin and plugin -> host events happen in the
+   `send_event()` and `receive_event()` functions. Reading data and writing the
+   results back for host-to-plugin `dispatcher()` calls and for plugin-to-host
    `audioMaster()` callbacks happen in the `DispatchDataConverter` and
    `HostCallbackDataConverter` classes respectively, with a bit of extra glue
-   for GUI related operations in `PluginBridge::dispatch_wrapper`. Rewriting all
-   of this tightly coupled logic to be all in one place sadly only makes things
-   even more complicated.
+   for GUI related operations in `PluginBridge::dispatch_wrapper`. On the
+   receiving end, the `passthrough_event()` function calls the callback
+   functions and handles the marshalling between our data types and the VST
+   API's different pointer types. This behaviour is separated from
+   `receive_event()` so we can some special handling for MIDI events, since a
+   select few plugins only store pointers to the received events rather than
+   copies of the objects. This requires the received event data to live at least
+   until the next audio buffer gets processed.
 
 6. The Wine VST host loads the Windows VST plugin and starts forwarding messages
    over the sockets described above.

src/common/events.h

@@ -98,7 +98,7 @@ class DefaultDataConverter {
  * the parameters and return value of this function.
  *
  * @param socket The socket to write over, should be the same socket the other
- *   endpoint is using to call `passthrough_event()`.
+ *   endpoint is using to call `receive_event()`.
  * @param write_mutex A mutex to ensure that only one thread can write to
  *   the socket at once. Needed because VST hosts and plugins can and sometimes
  *   will call the `dispatch()` or `audioMaster()` functions from multiple
@@ -113,6 +113,7 @@ class DefaultDataConverter {
  *   for sending `dispatch()` events or host callbacks. Optional since it
  *   doesn't have to be done on both sides.
  *
+ * @relates receive_event
  * @relates passthrough_event
  */
 template <typename D>
@@ -160,26 +161,28 @@ intptr_t send_event(boost::asio::local::stream_protocol::socket& socket,
 }
 
 /**
- * Receive an event from a socket and pass it through to some callback function.
- * This is used for both the host -> plugin 'dispatch' events and the plugin ->
- * host 'audioMaster' host callbacks. This callback function is either one of
- * those functions.
+ * Receive an event from a socket, call a function to generate a response, and
+ * write the response back over the socket. This is usually used together with
+ * `passthrough_event()` which passes the event data through to an event
+ * dispatcher function. This behaviour is split into two functions to avoid
+ * redundant data conversions when handling MIDI data, as some plugins require
+ * the received data to be temporarily stored until the next event audio buffer
+ * gets processed.
  *
  * @param socket The socket to receive on and to send the response back to.
  * @param logging A pair containing a logger instance and whether or not this is
  *   for sending `dispatch()` events or host callbacks. Optional since it
  *   doesn't have to be done on both sides.
- * @param plugin The `AEffect` instance that should be passed to the callback
- *   function.
- * @param callback The function to call with the arguments received from the
- *   socket.
+ * @param callback The function used to generate a response out of an event.
+ *
+ * @tparam F A function type in the form of `EventResponse(Event)`.
  *
  * @relates send_event
+ * @relates passthrough_event
  */
 template <typename F>
-void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
+void receive_event(boost::asio::local::stream_protocol::socket& socket,
                    std::optional<std::pair<Logger&, bool>> logging,
-                       AEffect* plugin,
                    F callback) {
     auto event = read_object<Event>(socket);
     if (logging.has_value()) {
@@ -188,9 +191,45 @@ void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
                          event.payload, event.option);
     }
 
+    EventResult response = callback(event);
+    if (logging.has_value()) {
+        auto [logger, is_dispatch] = logging.value();
+        logger.log_event_response(is_dispatch, event.opcode,
+                                  response.return_value, response.payload);
+    }
+
+    write_object(socket, response);
+}
+
+/**
+ * Create a callback function that takes an `Event` object, decodes the data
+ * into the expected format for VST2 function calls, calls the given function
+ * (either `AEffect::dispatcher()` for host -> plugin events or `audioMaster()`
+ * for plugin -> host events), and serializes the results back into an
+ * `EventResult` object. I'd rather not get too Haskell-y in my C++, but this is
+ * the cleanest solution for this problem.
+ *
+ * This is the receiving analogue of the `*DataCovnerter` objects.
+ *
+ * @param plugin The `AEffect` instance that should be passed to the callback
+ *   function.
+ * @param callback The function to call with the arguments received from the
+ *   socket.
+ *
+ * @tparam A function with the same signature as `AEffect::dispatcher` or
+ *   `audioMasterCallback`.
+ *
+ * @return A `EventResult(Event)` callback function that can be passed to
+ * `receive_event`.
+ *
+ * @relates receive_event
+ */
+template <typename F>
+auto passthrough_event(AEffect* plugin, F callback) {
+    return [=](Event& event) -> EventResult {
         // This buffer is used to write strings and small objects to. We'll
-    // initialize it with a single null to prevent it from being read as some
-    // arbitrary C-style string.
+        // initialize it with a single null to prevent it from being read as
+        // some arbitrary C-style string.
         std::array<char, max_string_length> string_buffer;
         string_buffer[0] = 0;
 
@@ -204,12 +243,13 @@ void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
                     return const_cast<uint8_t*>(buffer.data());
                 },
                 [&](native_size_t& window_handle) -> void* {
-                // This is the X11 window handle that the editor should reparent
-                // itself to. We have a special wrapper around the dispatch
-                // function that intercepts `effEditOpen` events and creates a
-                // Win32 window and then finally embeds the X11 window Wine
-                // created into this wnidow handle. Make sure to convert the
-                // window ID first to `size_t` in case this is the 32-bit host.
+                    // This is the X11 window handle that the editor should
+                    // reparent itself to. We have a special wrapper around the
+                    // dispatch function that intercepts `effEditOpen` events
+                    // and creates a Win32 window and then finally embeds the
+                    // X11 window Wine created into this wnidow handle. Make
+                    // sure to convert the window ID first to `size_t` in case
+                    // this is the 32-bit host.
                     return reinterpret_cast<void*>(
                         static_cast<size_t>(window_handle));
                 },
@@ -217,7 +257,9 @@ void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
                 [&](DynamicVstEvents& events) -> void* {
                     return &events.as_c_events();
                 },
-            [&](WantsChunkBuffer&) -> void* { return string_buffer.data(); },
+                [&](WantsChunkBuffer&) -> void* {
+                    return string_buffer.data();
+                },
                 [&](VstIOProperties& props) -> void* { return &props; },
                 [&](VstMidiKeyName& key_name) -> void* { return &key_name; },
                 [&](VstParameterProperties& props) -> void* { return &props; },
@@ -226,12 +268,14 @@ void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
                 [&](WantsString&) -> void* { return string_buffer.data(); }},
             event.payload);
 
-    const intptr_t return_value = callback(plugin, event.opcode, event.index,
-                                           event.value, data, event.option);
+        const intptr_t return_value = callback(
+            plugin, event.opcode, event.index, event.value, data, event.option);
 
-    // Only write back data when needed, this depends on the event payload type
+        // Only write back data when needed, this depends on the event payload
+        // type
         const auto response_data = std::visit(
-        overload{[&](auto) -> EventResposnePayload { return nullptr; },
+            overload{
+                [&](auto) -> EventResposnePayload { return nullptr; },
                 [&](const AEffect& updated_plugin) -> EventResposnePayload {
                     // This is a bit of a special case! Instead of writing some
                     // return value, we will update values on the native VST
@@ -300,12 +344,8 @@ void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
                 }},
             event.payload);
 
-    if (logging.has_value()) {
-        auto [logger, is_dispatch] = logging.value();
-        logger.log_event_response(is_dispatch, event.opcode, return_value,
-                                  response_data);
-    }
-
         EventResult response{return_value, response_data};
-    write_object(socket, response);
+
+        return response;
+    };
 }

src/plugin/host-bridge.cpp

@@ -172,9 +172,9 @@ HostBridge::HostBridge(audioMasterCallback host_callback)
     host_callback_handler = std::thread([&]() {
         try {
             while (true) {
-                passthrough_event(vst_host_callback,
-                                  std::pair<Logger&, bool>(logger, false),
-                                  &plugin, host_callback_function);
+                receive_event(
+                    vst_host_callback, std::pair<Logger&, bool>(logger, false),
+                    passthrough_event(&plugin, host_callback_function));
             }
         } catch (const boost::system::system_error&) {
             // This happens when the sockets got closed because the plugin

src/wine-host/plugin-bridge.cpp

@@ -147,9 +147,10 @@ void PluginBridge::handle_dispatch() {
     // lockstep anyway
     try {
         while (true) {
-            passthrough_event(host_vst_dispatch, std::nullopt, plugin,
-                              std::bind(&PluginBridge::dispatch_wrapper, this,
-                                        _1, _2, _3, _4, _5, _6));
+            receive_event(host_vst_dispatch, std::nullopt,
+                          passthrough_event(
+                              plugin, std::bind(&PluginBridge::dispatch_wrapper,
+                                                this, _1, _2, _3, _4, _5, _6)));
 
             // Because of the way the Win32 API works we have to process events
             // on the same thread as the one the window was created on, and that
@@ -179,39 +180,47 @@ void PluginBridge::handle_dispatch() {
 
 [[noreturn]] void PluginBridge::handle_dispatch_midi_events() {
     while (true) {
-        // TODO: Refactor `passthrough_event()` to factor out the data
-        //       conversion specifically for this case so we don't have to do
-        //       this `DynamicVstEvents -> VstEvents -> void* ->
-        //       DynamicVstEvents -> VstEvents -> void*` conversion dance
-        passthrough_event(
-            host_vst_dispatch_midi_events, std::nullopt, plugin,
-            [&](AEffect* plugin, int opcode, int index, intptr_t value,
-                void* data, float option) {
-                if (BOOST_LIKELY(opcode == effProcessEvents)) {
+        receive_event(
+            host_vst_dispatch_midi_events, std::nullopt, [&](Event& event) {
+                if (BOOST_LIKELY(event.opcode == effProcessEvents)) {
                     // For 99% of the plugins we can just call
                     // `effProcessReplacing()` and be done with it, but a select
                     // few plugins (I could only find Kontakt that does this)
                     // don't actually make copies of the events they receive and
                     // only store pointers, meaning that they have to live at
-                    // least until the next audio buffer gets processed. This
-                    // does mean that we have to reconstruct the
-                    // `DynamicVstEvents` object first.
-                    // HACK: Is there a cleaner way to do this, or a way to
-                    //       avoid having to store temporary copies of this?
+                    // least until the next audio buffer gets processed. We're
+                    // not using `passhtourhg_events()` here directly because we
+                    // need to store a copy of the `DynamicVstEvents` struct
+                    // before passing the generated `VstEvents` object to the
+                    // plugin.
                     std::lock_guard lock(next_buffer_midi_events_mutex);
-                    DynamicVstEvents& events =
-                        next_audio_buffer_midi_events.emplace_back(
-                            *static_cast<const VstEvents*>(data));
 
-                    return plugin->dispatcher(plugin, opcode, index, value,
-                                              &events.as_c_events(), option);
+                    next_audio_buffer_midi_events.push_back(
+                        std::get<DynamicVstEvents>(event.payload));
+                    DynamicVstEvents& events =
+                        next_audio_buffer_midi_events.back();
+
+                    // Exact same handling as in `passthrough_event`, apart from
+                    // making a copy of the events first
+                    const intptr_t return_value = plugin->dispatcher(
+                        plugin, event.opcode, event.index, event.value,
+                        &events.as_c_events(), event.option);
+
+                    EventResult response{return_value, nullptr};
+
+                    return response;
                 } else {
+                    using namespace std::placeholders;
+
                     std::cerr << "[Warning] Received non-MIDI "
                                  "event on MIDI processing thread"
                               << std::endl;
 
-                    return dispatch_wrapper(plugin, opcode, index, value, data,
-                                            option);
+                    // Maybe this should just be a hard error instead, since it
+                    // should never happen
+                    return passthrough_event(
+                        plugin, std::bind(&PluginBridge::dispatch_wrapper, this,
+                                          _1, _2, _3, _4, _5, _6))(event);
                 }
             });
     }