Use different types for strings and binary data

README.md

@@ -11,15 +11,13 @@ There are a few things that should be done before releasing this, including:
 
 - Fix implementation bugs:
   - Phase Plant doesn't play any sound until the editor has been opened?
-  - Phase Plant sometimes crashes during `effEditIdle()`, not sure what
-    situation triggers this.
-  - Phase plant can't load large chunks. Seems to be a problem with Phase Plant
-    since other plugins don't have this issue.
+  - Large chunk buffers can't be sent over the socket in one go, this causes
+    issues with presets that are multiple megabytes in size.
+  - Serum crashed and audio engine froze while browsing through Serum presets in
+    the browser?
   - KiloHearts plugins create a ridiculous amount of file descriptor leaks in
     wineserver when esync is enabled. I haven't come across any other plugins
     that do this. Not sure if this is fixable in yabridge.
-  - Serum crashed and audio engine froze while browsing through Serum presets in
-    the browser?
   - Polish GUIs even further. There are some todos left in
     `src/wine-host/editor.{h,cpp}`.
 - Add missing details if any to the architecture section.

src/common/events.h

@@ -190,30 +190,39 @@ void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
     // arbitrary C-style string.
     std::array<char, max_string_length> string_buffer;
     string_buffer[0] = 0;
+    // This buffer is only used for retrieving chunk data and will be allocated
+    // as needed
+    std::vector<uint8_t> binary_buffer;
+
     void* data = std::visit(
-        overload{
-            [&](const std::nullptr_t&) -> void* { return nullptr; },
-            [&](const std::string& s) -> void* {
-                return const_cast<char*>(s.c_str());
-            },
-            [&](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.
-                return reinterpret_cast<void*>(window_handle);
-            },
-            [&](const AEffect&) -> void* { return nullptr; },
-            [&](DynamicVstEvents& events) -> void* {
-                return &events.as_c_events();
-            },
-            [&](WantsChunkBuffer&) -> void* { return string_buffer.data(); },
-            [&](VstIOProperties& props) -> void* { return &props; },
-            [&](VstParameterProperties& props) -> void* { return &props; },
-            [&](WantsVstRect&) -> void* { return string_buffer.data(); },
-            [&](const WantsVstTimeInfo&) -> void* { return nullptr; },
-            [&](WantsString&) -> void* { return string_buffer.data(); }},
+        overload{[&](const std::nullptr_t&) -> void* { return nullptr; },
+                 [&](const std::string& s) -> void* {
+                     return const_cast<char*>(s.c_str());
+                 },
+                 [&](const std::vector<uint8_t>& buffer) -> void* {
+                     return const_cast<uint8_t*>(buffer.data());
+                 },
+                 [&](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.
+                     return reinterpret_cast<void*>(window_handle);
+                 },
+                 [&](const AEffect&) -> void* { return nullptr; },
+                 [&](DynamicVstEvents& events) -> void* {
+                     return &events.as_c_events();
+                 },
+                 [&](WantsChunkBuffer&) -> void* {
+                     binary_buffer.resize(binary_buffer_size);
+                     return binary_buffer.data();
+                 },
+                 [&](VstIOProperties& props) -> void* { return &props; },
+                 [&](VstParameterProperties& props) -> void* { return &props; },
+                 [&](WantsVstRect&) -> void* { return string_buffer.data(); },
+                 [&](const WantsVstTimeInfo&) -> void* { return nullptr; },
+                 [&](WantsString&) -> void* { return string_buffer.data(); }},
         event.payload);
 
     const intptr_t return_value = callback(plugin, event.opcode, event.index,
@@ -256,8 +265,9 @@ void passthrough_event(boost::asio::local::stream_protocol::socket& socket,
                      // stored in an array to which a pointer is stored in
                      // `data`, with the return value from the event determines
                      // how much data the plugin has written
-                     return std::string(*static_cast<char**>(data),
-                                        return_value);
+                     const uint8_t* chunk_data = *static_cast<uint8_t**>(data);
+                     return std::vector<uint8_t>(chunk_data,
+                                                 chunk_data + return_value);
                  },
                  [&](VstIOProperties& props) -> EventResposnePayload {
                      return props;

src/common/logging.cpp

@@ -173,6 +173,7 @@ void Logger::log_event(bool is_dispatch,
                         message << "<" << s.size() << " bytes>";
                     }
                 },
+                [&](const std::vector<uint8_t>&) { message << "<chunk>"; },
                 [&](const intptr_t&) { message << "<nullptr>"; },
                 [&](const AEffect&) { message << "<nullptr>"; },
                 [&](const DynamicVstEvents& events) {
@@ -226,6 +227,9 @@ void Logger::log_event_response(bool is_dispatch,
                         message << ", <" << s.size() << " bytes>";
                     }
                 },
+                [&](const std::vector<uint8_t>& buffer) {
+                    message << "<" << buffer.size() << "byte chunk>";
+                },
                 [&](const AEffect&) { message << ", <AEffect_object>"; },
                 [&](const VstIOProperties&) { message << ", <io_properties>"; },
                 [&](const VstParameterProperties& props) {

src/common/serialization.h

@@ -51,11 +51,11 @@ constexpr size_t max_midi_events = max_buffer_size / sizeof(size_t);
 [[maybe_unused]] constexpr size_t max_string_length = 64;
 
 /**
- * The size for a buffer in which we're receiving chunks. Allow for up to 1 GB
+ * The size for a buffer in which we're receiving chunks. Allow for up to 50 MB
  * chunks. Hopefully no plugin will come anywhere near this limit, but it will
  * add up when plugins start to audio samples in their presets.
  */
-constexpr size_t binary_buffer_size = 1 << 30;
+constexpr size_t binary_buffer_size = 50 << 20;
 
 // The cannonical overloading template for `std::visitor`, not sure why this
 // isn't part of the standard library
@@ -218,6 +218,10 @@ struct WantsString {};
  *         size. We can replace `std::string` with `char*` once it does for
  *         clarity's sake.
  *
+ * - A byte vector for handling chunk data during `effSetChunk()`. We can't
+     reuse the regular string handling here since the data may contain null
+     bytes and `std::string::as_c_str()` might cut off everything after the
+     first null byte.
  * - An X11 window handle.
  * - Specific data structures from `aeffextx.h`. For instance an event with the
  *   opcode `effProcessEvents` the hosts passes a `VstEvents` struct containing
@@ -226,15 +230,19 @@ struct WantsString {};
  *
  * - Some empty buffer for the plugin to write its own data to, for instance for
  *   a plugin to report its name or the label for a certain parameter. There are
- *   two separate cases here:
+ *   two separate cases here. This is typically a short null terminated
+ *   C-string. We'll assume this as the default case when none of the above
+ *   options apply.
  *
  *   - Either the plugin writes arbitrary data and uses its return value to
  *     indicate how much data was written (i.e. for the `effGetChunk` opcode).
+ *     For this we use a vector of bytes instead of a string since
  *   - Or the plugin will write a short null terminated C-string there. We'll
  *     assume that this is the default if none of the above options apply.
  */
 using EventPayload = std::variant<std::nullptr_t,
                                   std::string,
+                                  std::vector<uint8_t>,
                                   size_t,
                                   AEffect,
                                   DynamicVstEvents,
@@ -251,9 +259,10 @@ void serialize(S& s, EventPayload& payload) {
           bitsery::ext::StdVariant{
               [](S&, std::nullptr_t&) {},
               [](S& s, std::string& string) {
-                  // `binary_buffer_size` and not `max_string_length` because we
-                  // also use this to send back large chunk data
-                  s.text1b(string, binary_buffer_size);
+                  s.text1b(string, max_string_length);
+              },
+              [](S& s, std::vector<uint8_t>& buffer) {
+                  s.container1b(buffer, binary_buffer_size);
               },
               [](S& s, size_t& window_handle) { s.value8b(window_handle); },
               [](S& s, AEffect& effect) { s.object(effect); },
@@ -312,16 +321,16 @@ struct Event {
  *
  * - Nothing, on which case only the return value from the callback function
  *   gets passed along.
- * - Some buffer stored in a `std::string`. This is typically read from and
- *   written as C-style string, but in the case of `effGetChunk` this is some
- *   blob of binary data that should be written to `HostBridge::chunk_data`
- *   instenad.
+ * - A (short) string.
+ * - Some binary blob stored as a byte vector. During `effGetChunk` this will
+     contain some chunk data that should be written to `HostBridge::chunk_data`.
  * - A specific struct in response to an event such as `audioMasterGetTime` or
  *   `audioMasterIOChanged`.
  * - An X11 window pointer for the editor window.
  */
 using EventResposnePayload = std::variant<std::nullptr_t,
                                           std::string,
+                                          std::vector<uint8_t>,
                                           AEffect,
                                           VstIOProperties,
                                           VstParameterProperties,
@@ -334,10 +343,10 @@ void serialize(S& s, EventResposnePayload& payload) {
           bitsery::ext::StdVariant{
               [](S&, std::nullptr_t&) {},
               [](S& s, std::string& string) {
-                  // `binary_buffer_size` and not `max_string_length`
-                  // because we also use this to send back large chunk
-                  // data
-                  s.text1b(string, binary_buffer_size);
+                  s.text1b(string, max_string_length);
+              },
+              [](S& s, std::vector<uint8_t>& buffer) {
+                  s.container1b(buffer, binary_buffer_size);
               },
               [](S& s, AEffect& effect) { s.object(effect); },
               [](S& s, VstIOProperties& props) { s.object(props); },

src/plugin/host-bridge.cpp

@@ -206,11 +206,13 @@ class DispatchDataConverter : DefaultDataConverter {
             case effGetChunk:
                 return WantsChunkBuffer();
                 break;
-            case effSetChunk:
+            case effSetChunk: {
+                const uint8_t* chunk_data = static_cast<const uint8_t*>(data);
+
                 // When the host passes a chunk it will use the value parameter
                 // to tell us its length
-                return std::string(static_cast<const char*>(data), value);
-                break;
+                return std::vector<uint8_t>(chunk_data, chunk_data + value);
+            } break;
             case effProcessEvents:
                 return DynamicVstEvents(*static_cast<const VstEvents*>(data));
                 break;
@@ -243,8 +245,8 @@ class DispatchDataConverter : DefaultDataConverter {
                 // Write the chunk data to some publically accessible place in
                 // `HostBridge` and write a pointer to that struct to the data
                 // pointer
-
-                const auto buffer = std::get<std::string>(response.payload);
+                const auto buffer =
+                    std::get<std::vector<uint8_t>>(response.payload);
                 chunk.assign(buffer.begin(), buffer.end());
 
                 *static_cast<void**>(data) = chunk.data();