From a1e7142f17d8474616a4383fc117783f63f11fc1 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 25 Oct 2020 13:00:50 +0100 Subject: [PATCH 01/40] Mark max_win32_messages as [[maybe_unused]] ccls/clangd doesn't know that it's being used elsewhere. --- src/wine-host/editor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wine-host/editor.h b/src/wine-host/editor.h index dfb3d92e..e7d58ec8 100644 --- a/src/wine-host/editor.h +++ b/src/wine-host/editor.h @@ -44,7 +44,7 @@ * - Melda plugins when having multiple editor windows open within a single * plugin group */ -constexpr int max_win32_messages = 20; +constexpr int max_win32_messages [[maybe_unused]] = 20; /** * Used to store the maximum width and height of a screen. From 4b4b19bbd8692449da43e23ec5e1f70b3c47a35e Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 25 Oct 2020 13:38:21 +0100 Subject: [PATCH 02/40] Mention multiple socket endpoints in architecture --- docs/architecture.md | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 580cf4c8..cb8c6496 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1,5 +1,7 @@ # Architecture + + The project consists of two components: a Linux native VST plugin (`libyabridge.so`) and a VST host that runs under Wine (`yabridge-host.exe`/`yabridge-host.exe.so`, and @@ -38,22 +40,24 @@ as the _Windows VST plugin_. The whole process works as follows: - The Wine prefix the plugin is located in. If the `WINEPREFIX` environment variable is specified, then that will be used instead. -3. The plugin then sets up a Unix domain socket endpoint to communicate with the - Wine VST host somewhere in a temporary directory and starts listening on it. - I chose to communicate over Unix domain sockets rather than using shared - memory directly because this way you get low latency communication with - without any busy waits or manual synchronisation for free. The added benefit - is that it also makes it possible to send arbitrarily large chunks of data - without having to split it up first. This is useful for transmitting audio - and preset data which may have any arbitrary size. +3. The plugin then sets up several Unix domain socket endpoints to communicate + with the Wine VST host somewhere in a temporary directory and starts + listening on them. We'll use multiple sockets so we can easily handle + multiple data streams from different threads using blocking synchronous + operations. This greatly simplifies the way communication works without + compromising on latency. The different sockets will be described below. We + communicate over Unix domain sockets rather than using shared memory directly + because this way we get low latency communication without any manual + synchronisation for free, while being able to send messages of arbitrary + length without having to split them up first. This is useful for transmitting + audio and preset data which can be any arbitrary size. 4. The plugin launches the Wine VST host in the detected wine prefix, passing - the name of the `.dll` file it should be loading and the path to the Unix - domain socket that was just created as its arguments. -5. Communication gets set up using multiple sockets over the end point created - previously. This allows us to easily handle multiple data streams from - different threads using blocking read operations for synchronization. Doing - this greatly simplifies the way communication works without compromising on - latency. The following types of events each get their own socket: + the name of the `.dll` file it should be loading and the base directory for + the Unix domain sockets that are going to be communciated over as its + arguments. +5. The Wine VST host connects to the sockets and communication between the + plugin and the Wine VST host gets set up. The following types of events each + get their own socket: - Calls from the native VST host to the plugin's `dispatcher()` function. These get forwarded to the Windows VST plugin through the Wine VST host. From 4b533425144fe21daba159f01f34565d3bfaec45 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 25 Oct 2020 16:50:58 +0100 Subject: [PATCH 03/40] :boom: Encapsulate and rework all socket logic This is a pretty huge change that will be important for being able to handle nested or mutually recursive `dispatch()` and `audioMaster()` calls. This sadly all had to be done in a single commit, so here's a summary: - `src/common/sockets.h:Sockets` contains all sockets on both the plugin and the Wine host side, and is used to both listen on and connect to the sockets. - Sockets and other temporary files respect `$XDG_RUNTIME_DIR` instead of being dumped in `/tmp`. - All sockets now have a unique endpoint in `/run/user//yabridge--/`. This is important for when we want to have multiple socket connections for handling `dispatch()` and `audioMaster()`. - Because of the above, we no longer clean up the socket endpoint files after the connection gets established during initialization. Instead we'll remove the socket base directory when shutting down. --- meson.build | 2 + src/common/communication.cpp | 127 ++++++++++++++++++++++++++ src/common/communication.h | 147 ++++++++++++++++++++++++++++++ src/common/serialization.cpp | 3 +- src/common/serialization.h | 7 +- src/common/utils.cpp | 13 +++ src/common/utils.h | 11 +++ src/plugin/host-process.cpp | 37 ++++---- src/plugin/host-process.h | 24 +++-- src/plugin/plugin-bridge.cpp | 91 ++++++++---------- src/plugin/plugin-bridge.h | 41 +-------- src/plugin/utils.cpp | 68 +++----------- src/plugin/utils.h | 31 +++---- src/wine-host/bridges/group.cpp | 6 +- src/wine-host/bridges/vst2.cpp | 55 +++++------ src/wine-host/bridges/vst2.h | 51 ++--------- src/wine-host/individual-host.cpp | 10 +- 17 files changed, 439 insertions(+), 285 deletions(-) create mode 100644 src/common/communication.cpp diff --git a/meson.build b/meson.build index 5942e132..bade536b 100644 --- a/meson.build +++ b/meson.build @@ -92,6 +92,7 @@ shared_library( 'src/common/configuration.cpp', 'src/common/logging.cpp', 'src/common/serialization.cpp', + 'src/common/communication.cpp', 'src/common/utils.cpp', 'src/plugin/host-process.cpp', 'src/plugin/plugin.cpp', @@ -116,6 +117,7 @@ host_sources = [ 'src/common/configuration.cpp', 'src/common/logging.cpp', 'src/common/serialization.cpp', + 'src/common/communication.cpp', 'src/common/utils.cpp', 'src/wine-host/bridges/vst2.cpp', 'src/wine-host/editor.cpp', diff --git a/src/common/communication.cpp b/src/common/communication.cpp new file mode 100644 index 00000000..391eb709 --- /dev/null +++ b/src/common/communication.cpp @@ -0,0 +1,127 @@ +// yabridge: a Wine VST bridge +// Copyright (C) 2020 Robbert van der Helm +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +#include "communication.h" + +#include + +#include "utils.h" + +namespace fs = boost::filesystem; + +/** + * Used for generating random identifiers. + */ +constexpr char alphanumeric_characters[] = + "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; + +Sockets::Sockets(boost::asio::io_context& io_context, + const boost::filesystem::path& endpoint_base_dir, + bool listen) + : base_dir(endpoint_base_dir), + io_context(io_context), + host_vst_dispatch(io_context), + host_vst_dispatch_midi_events(io_context), + vst_host_callback(io_context), + host_vst_parameters(io_context), + host_vst_process_replacing(io_context), + host_vst_control(io_context), + host_vst_dispatch_endpoint( + (base_dir / "host_vst_dispatch.sock").string()), + host_vst_dispatch_midi_events_endpoint( + (base_dir / "host_vst_dispatch_midi_events.sock").string()), + vst_host_callback_endpoint( + (base_dir / "vst_host_callback.sock").string()), + host_vst_parameters_endpoint( + (base_dir / "host_vst_parameters.sock").string()), + host_vst_process_replacing_endpoint( + (base_dir / "host_vst_process_replacing.sock").string()), + host_vst_control_endpoint((base_dir / "host_vst_control.sock").string()) { + if (listen) { + fs::create_directory(base_dir); + + acceptors = Acceptors{ + .host_vst_dispatch{io_context, host_vst_dispatch_endpoint}, + .host_vst_dispatch_midi_events{ + io_context, host_vst_dispatch_midi_events_endpoint}, + .vst_host_callback{io_context, vst_host_callback_endpoint}, + .host_vst_parameters{io_context, host_vst_parameters_endpoint}, + .host_vst_process_replacing{io_context, + host_vst_process_replacing_endpoint}, + .host_vst_control{io_context, host_vst_control_endpoint}, + }; + } +} + +Sockets::~Sockets() { + // Only clean if we're the ones who have created these files, although it + // should not cause any harm to also do this on the Wine side + if (acceptors) { + try { + fs::remove_all(base_dir); + } catch (const fs::filesystem_error&) { + // There should not be any filesystem errors since only one side + // removes the files, but if we somehow can't delete the file then + // we can just silently ignore this + } + } +} + +void Sockets::connect() { + if (acceptors) { + acceptors->host_vst_dispatch.accept(host_vst_dispatch); + acceptors->host_vst_dispatch_midi_events.accept( + host_vst_dispatch_midi_events); + acceptors->vst_host_callback.accept(vst_host_callback); + acceptors->host_vst_parameters.accept(host_vst_parameters); + acceptors->host_vst_process_replacing.accept( + host_vst_process_replacing); + acceptors->host_vst_control.accept(host_vst_control); + } else { + host_vst_dispatch.connect(host_vst_dispatch_endpoint); + host_vst_dispatch_midi_events.connect( + host_vst_dispatch_midi_events_endpoint); + vst_host_callback.connect(vst_host_callback_endpoint); + host_vst_parameters.connect(host_vst_parameters_endpoint); + host_vst_process_replacing.connect(host_vst_process_replacing_endpoint); + host_vst_control.connect(host_vst_control_endpoint); + } +} + +boost::filesystem::path generate_endpoint_base(const std::string& plugin_name) { + fs::path temp_directory = get_temporary_directory(); + + std::random_device random_device; + std::mt19937 rng(random_device()); + fs::path candidate_endpoint; + do { + std::string random_id; + std::sample( + alphanumeric_characters, + alphanumeric_characters + strlen(alphanumeric_characters) - 1, + std::back_inserter(random_id), 8, rng); + + // We'll get rid of the file descriptors immediately after accepting the + // sockets, so putting them inside of a subdirectory would only leave + // behind an empty directory + std::ostringstream socket_name; + socket_name << "yabridge-" << plugin_name << "-" << random_id; + + candidate_endpoint = temp_directory / socket_name.str(); + } while (fs::exists(candidate_endpoint)); + + return candidate_endpoint; +} diff --git a/src/common/communication.h b/src/common/communication.h index 22ac3516..21e5ee47 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -22,9 +22,11 @@ #ifdef __WINE__ #include "../wine-host/boost-fix.h" #endif +#include #include #include #include +#include template using OutputAdapter = bitsery::OutputBufferAdapter; @@ -32,6 +34,151 @@ using OutputAdapter = bitsery::OutputBufferAdapter; template using InputAdapter = bitsery::InputBufferAdapter; +/** + * Manages all the sockets used for communicating between the plugin and the + * Wine host. Every plugin will get its own directory (the socket endpoint base + * directory), and all socket endpoints are created within this directory. This + * is usually `/run/user//yabridge--/`. + * + * On the plugin side this class should be initialized with `listen` set to + * `true` before launching the Wine VST host. This will start listening on the + * sockets, and the call to `connect()` will then accept any incoming + * connections. + */ +class Sockets { + public: + /** + * Sets up the sockets using the specified base directory. The sockets won't + * be active until `connect()` gets called. + * + * @param io_context The IO context the sockets should be bound to. Relevant + * when doing asynchronous operations. + * @param endpoint_base_dir The base directory that will be used for the + * Unix domain sockets. + * @param listen If `true`, start listening on the sockets. Incoming + * connections will be accepted when `connect()` gets called. This should + * be set to `true` on the plugin side, and `false` on the Wine host side. + * + * @see Sockets::connect + */ + Sockets(boost::asio::io_context& io_context, + const boost::filesystem::path& endpoint_base_dir, + bool listen); + + /** + * Cleans up the directory containing the socket endpoints when yabridge + * shuts down if it still exists. + */ + ~Sockets(); + + /** + * Depending on the value of the `listen` argument passed to the + * constructor, either accept connections made to the sockets on the Linux + * side or connect to the sockets on the Wine side + */ + void connect(); + + /** + * The base directory for our socket endpoints. All `*_endpoint` variables + * below are files within this directory. + */ + const boost::filesystem::path base_dir; + + /** + * The IO context that the sockets will be created on. This is only relevant + * for asynchronous operations. + */ + boost::asio::io_context& io_context; + + // The naming convention for these sockets is `__`. For + // instance the socket named `host_vst_dispatch` forwards + // `AEffect.dispatch()` calls from the native VST host to the Windows VST + // plugin (through the Wine VST host). + + /** + * The socket that forwards all `dispatcher()` calls from the VST host to + * the plugin. + */ + boost::asio::local::stream_protocol::socket host_vst_dispatch; + /** + * Used specifically for the `effProcessEvents` opcode. This is needed + * because the Win32 API is designed to block during certain GUI + * interactions such as resizing a window or opening a dropdown. Without + * this MIDI input would just stop working at times. + */ + boost::asio::local::stream_protocol::socket host_vst_dispatch_midi_events; + /** + * The socket that forwards all `audioMaster()` calls from the Windows VST + * plugin to the host. + */ + boost::asio::local::stream_protocol::socket vst_host_callback; + /** + * Used for both `getParameter` and `setParameter` since they mostly + * overlap. + */ + boost::asio::local::stream_protocol::socket host_vst_parameters; + /** + * Used for processing audio usign the `process()`, `processReplacing()` and + * `processDoubleReplacing()` functions. + */ + boost::asio::local::stream_protocol::socket host_vst_process_replacing; + /** + * A control socket that sends data that is not suitable for the other + * sockets. At the moment this is only used to, on startup, send the Windows + * VST plugin's `AEffect` object to the native VST plugin, and to then send + * the configuration (from `config`) back to the Wine host. + */ + boost::asio::local::stream_protocol::socket host_vst_control; + + private: + const boost::asio::local::stream_protocol::endpoint + host_vst_dispatch_endpoint; + const boost::asio::local::stream_protocol::endpoint + host_vst_dispatch_midi_events_endpoint; + const boost::asio::local::stream_protocol::endpoint + vst_host_callback_endpoint; + const boost::asio::local::stream_protocol::endpoint + host_vst_parameters_endpoint; + const boost::asio::local::stream_protocol::endpoint + host_vst_process_replacing_endpoint; + const boost::asio::local::stream_protocol::endpoint + host_vst_control_endpoint; + + /** + * All of our socket acceptors. We have to create these before launching the + * Wine process. + */ + struct Acceptors { + boost::asio::local::stream_protocol::acceptor host_vst_dispatch; + boost::asio::local::stream_protocol::acceptor + host_vst_dispatch_midi_events; + boost::asio::local::stream_protocol::acceptor vst_host_callback; + boost::asio::local::stream_protocol::acceptor host_vst_parameters; + boost::asio::local::stream_protocol::acceptor + host_vst_process_replacing; + boost::asio::local::stream_protocol::acceptor host_vst_control; + }; + + /** + * If the `listen` constructor argument was set to `true`, when we'll + * prepare a set of socket acceptors that listen on the socket endpoints. + */ + std::optional acceptors; +}; + +/** + * Generate a unique base directory that can be used as a prefix for all Unix + * domain socket endpoints used in `PluginBridge`/`Vst2Bridge`. This will + * usually return `/run/user//yabridge--/`. + * + * Sockets for group hosts are handled separately. See + * `../plugin/utils.h:generate_group_endpoint` for more information on those. + * + * @param plugin_name The name of the plugin we're generating endpoints for. + * Used as a visual indication of what plugin is using this endpoint. + */ +boost::filesystem::path generate_endpoint_base(const std::string& plugin_name); + /** * Serialize an object using bitsery and write it to a socket. This will write * both the size of the serialized object and the object itself over the socket. diff --git a/src/common/serialization.cpp b/src/common/serialization.cpp index 4a5e14d4..d49d480d 100644 --- a/src/common/serialization.cpp +++ b/src/common/serialization.cpp @@ -110,5 +110,6 @@ AEffect& update_aeffect(AEffect& plugin, const AEffect& updated_plugin) { } bool GroupRequest::operator==(const GroupRequest& rhs) const { - return plugin_path == rhs.plugin_path && socket_path == rhs.socket_path; + return plugin_path == rhs.plugin_path && + endpoint_base_dir == rhs.endpoint_base_dir; } diff --git a/src/common/serialization.h b/src/common/serialization.h index 3679d528..c53612a5 100644 --- a/src/common/serialization.h +++ b/src/common/serialization.h @@ -598,14 +598,14 @@ struct AudioBuffers { */ struct GroupRequest { std::string plugin_path; - std::string socket_path; + std::string endpoint_base_dir; bool operator==(const GroupRequest& rhs) const; template void serialize(S& s) { s.text1b(plugin_path, 4096); - s.text1b(socket_path, 4096); + s.text1b(endpoint_base_dir, 4096); } }; @@ -614,7 +614,8 @@ struct std::hash { std::size_t operator()(GroupRequest const& params) const noexcept { std::hash hasher{}; - return hasher(params.plugin_path) ^ (hasher(params.socket_path) << 1); + return hasher(params.plugin_path) ^ + (hasher(params.endpoint_base_dir) << 1); } }; diff --git a/src/common/utils.cpp b/src/common/utils.cpp index e62b5a0a..1b2d0e64 100644 --- a/src/common/utils.cpp +++ b/src/common/utils.cpp @@ -17,6 +17,19 @@ #include "utils.h" #include +#include + +namespace bp = boost::process; +namespace fs = boost::filesystem; + +fs::path get_temporary_directory() { + bp::environment env = boost::this_process::environment(); + if (!env["XDG_RUNTIME_DIR"].empty()) { + return env["XDG_RUNTIME_DIR"].to_string(); + } else { + return fs::temp_directory_path(); + } +} bool set_realtime_priority() { sched_param params{.sched_priority = 5}; diff --git a/src/common/utils.h b/src/common/utils.h index 7543294e..a21fb0dd 100644 --- a/src/common/utils.h +++ b/src/common/utils.h @@ -16,6 +16,17 @@ #pragma once +#ifdef __WINE__ +#include "../wine-host/boost-fix.h" +#endif +#include + +/** + * Return the path to the directory for story temporary files. This will be + * `$XDG_RUNTIME_DIR` if set, and `/tmp` otherwise. + */ +boost::filesystem::path get_temporary_directory(); + /** * Set the scheduling policy to `SCHED_FIFO` with priority 10 for this process. * We explicitly don't do this for wineserver itself since from my testing that diff --git a/src/plugin/host-process.cpp b/src/plugin/host-process.cpp index 83d78c23..0473abb3 100644 --- a/src/plugin/host-process.cpp +++ b/src/plugin/host-process.cpp @@ -83,7 +83,7 @@ void HostProcess::async_log_pipe_lines(patched_async_pipe& pipe, IndividualHost::IndividualHost(boost::asio::io_context& io_context, Logger& logger, fs::path plugin_path, - fs::path socket_endpoint) + const Sockets& sockets) : HostProcess(io_context, logger), plugin_arch(find_vst_architecture(plugin_path)), host_path(find_vst_host(plugin_arch, false)), @@ -93,7 +93,7 @@ IndividualHost::IndividualHost(boost::asio::io_context& io_context, #else plugin_path, #endif - socket_endpoint, + sockets.base_dir, bp::env = set_wineprefix(), bp::std_out = stdout_pipe, bp::std_err = stderr_pipe @@ -127,17 +127,15 @@ void IndividualHost::terminate() { host.wait(); } -GroupHost::GroupHost( - boost::asio::io_context& io_context, - Logger& logger, - fs::path plugin_path, - fs::path socket_endpoint, - std::string group_name, - boost::asio::local::stream_protocol::socket& host_vst_dispatch) +GroupHost::GroupHost(boost::asio::io_context& io_context, + Logger& logger, + fs::path plugin_path, + Sockets& sockets, + std::string group_name) : HostProcess(io_context, logger), plugin_arch(find_vst_architecture(plugin_path)), host_path(find_vst_host(plugin_arch, true)), - host_vst_dispatch(host_vst_dispatch) { + sockets(sockets) { #ifdef WITH_WINEDBG if (plugin_path.string().find(' ') != std::string::npos) { logger.log("Warning: winedbg does not support paths containing spaces"); @@ -167,6 +165,7 @@ GroupHost::GroupHost( wine_prefix = fs::path(host_env.at("HOME").to_string()) / ".wine"; } + const fs::path endpoint_base_dir = sockets.base_dir; const fs::path group_socket_path = generate_group_endpoint(group_name, wine_prefix, plugin_arch); try { @@ -175,9 +174,10 @@ GroupHost::GroupHost( boost::asio::local::stream_protocol::socket group_socket(io_context); group_socket.connect(group_socket_path.string()); - write_object(group_socket, - GroupRequest{.plugin_path = plugin_path.string(), - .socket_path = socket_endpoint.string()}); + write_object( + group_socket, + GroupRequest{.plugin_path = plugin_path.string(), + .endpoint_base_dir = endpoint_base_dir.string()}); const auto response = read_object(group_socket); host_pid = response.pid; @@ -199,7 +199,7 @@ GroupHost::GroupHost( // meantime. group_host_connect_handler = std::jthread([&, group_socket_path, plugin_path, - socket_endpoint]() { + endpoint_base_dir]() { using namespace std::literals::chrono_literals; // TODO: Replace this polling with inotify @@ -214,8 +214,9 @@ GroupHost::GroupHost( write_object( group_socket, - GroupRequest{.plugin_path = plugin_path.string(), - .socket_path = socket_endpoint.string()}); + GroupRequest{ + .plugin_path = plugin_path.string(), + .endpoint_base_dir = endpoint_base_dir.string()}); const auto response = read_object(group_socket); @@ -262,7 +263,7 @@ void GroupHost::terminate() { // There's no need to manually terminate group host processes as they will // shut down automatically after all plugins have exited. Manually closing // the dispatch socket will cause the associated plugin to exit. - host_vst_dispatch.shutdown( + sockets.host_vst_dispatch.shutdown( boost::asio::local::stream_protocol::socket::shutdown_both); - host_vst_dispatch.close(); + sockets.host_vst_dispatch.close(); } diff --git a/src/plugin/host-process.h b/src/plugin/host-process.h index 5330adf2..840b3a74 100644 --- a/src/plugin/host-process.h +++ b/src/plugin/host-process.h @@ -26,6 +26,7 @@ #include #include "../common/logging.h" +#include "../common/communication.h" #include "utils.h" /** @@ -117,7 +118,7 @@ class IndividualHost : public HostProcess { * handled on. * @param logger The `Logger` instance the redirected STDIO streams will be * written to. - * @param socket_endpoint The endpoint that should be used to communicate + * @param sockets The socket endpoints that will be used for communication * with the plugin. * * @throw std::runtime_error When `plugin_path` does not point to a valid @@ -126,7 +127,7 @@ class IndividualHost : public HostProcess { IndividualHost(boost::asio::io_context& io_context, Logger& logger, boost::filesystem::path plugin_path, - boost::filesystem::path socket_endpoint); + const Sockets& sockets); PluginArchitecture architecture() override; boost::filesystem::path path() override; @@ -160,19 +161,16 @@ class GroupHost : public HostProcess { * handled on. * @param logger The `Logger` instance the redirected STDIO streams will be * written to. - * @param socket_endpoint The endpoint that should be used to communicate - * with the plugin. + * @param sockets The socket endpoints that will be used for communication + * with the plugin. When the plugin shuts down, we'll terminate the + * dispatch socket contained in this object. * @param group_name The name of the plugin group. - * @param host_vst_dispatch The socket used to communicate - * `AEffect::dispatcher()` events with this plugin. Will be closed as to - * shut down the plugin. */ GroupHost(boost::asio::io_context& io_context, Logger& logger, boost::filesystem::path plugin_path, - boost::filesystem::path socket_endpoint, - std::string group_name, - boost::asio::local::stream_protocol::socket& host_vst_dispatch); + Sockets& socket_endpoint, + std::string group_name); PluginArchitecture architecture() override; boost::filesystem::path path() override; @@ -191,10 +189,10 @@ class GroupHost : public HostProcess { pid_t host_pid; /** - * The associated dispatch socket for the plugin we're hosting. This is used - * to terminate the plugin. + * The associated sockets for the plugin we're hosting. This is used to + * terminate the plugin. */ - boost::asio::local::stream_protocol::socket& host_vst_dispatch; + Sockets& sockets; /** * A thread that waits for the group host to have started and then ask it to diff --git a/src/plugin/plugin-bridge.cpp b/src/plugin/plugin-bridge.cpp index 886abeec..0c0c3003 100644 --- a/src/plugin/plugin-bridge.cpp +++ b/src/plugin/plugin-bridge.cpp @@ -54,32 +54,26 @@ PluginBridge::PluginBridge(audioMasterCallback host_callback) // bridge will crash otherwise plugin(), io_context(), - socket_endpoint(generate_plugin_endpoint().string()), - socket_acceptor(io_context, socket_endpoint), - host_vst_dispatch(io_context), - host_vst_dispatch_midi_events(io_context), - vst_host_callback(io_context), - host_vst_parameters(io_context), - host_vst_process_replacing(io_context), - host_vst_control(io_context), + sockets(io_context, + generate_endpoint_base( + vst_plugin_path.filename().replace_extension("").string()), + true), host_callback_function(host_callback), logger(Logger::create_from_environment( - create_logger_prefix(socket_endpoint.path()))), + create_logger_prefix(sockets.base_dir))), wine_version(get_wine_version()), - vst_host( - config.group - ? std::unique_ptr( - std::make_unique(io_context, - logger, - vst_plugin_path, - socket_endpoint.path(), - *config.group, - host_vst_dispatch)) - : std::unique_ptr( - std::make_unique(io_context, + vst_host(config.group + ? std::unique_ptr( + std::make_unique(io_context, logger, vst_plugin_path, - socket_endpoint.path()))), + sockets, + *config.group)) + : std::unique_ptr( + std::make_unique(io_context, + logger, + vst_plugin_path, + sockets))), has_realtime_priority(set_realtime_priority()), wine_io_handler([&]() { io_context.run(); }) { log_init_message(); @@ -107,24 +101,13 @@ PluginBridge::PluginBridge(audioMasterCallback host_callback) }); #endif - // It's very important that these sockets are connected to in the same - // order in the Wine VST host - socket_acceptor.accept(host_vst_dispatch); - socket_acceptor.accept(host_vst_dispatch_midi_events); - socket_acceptor.accept(vst_host_callback); - socket_acceptor.accept(host_vst_parameters); - socket_acceptor.accept(host_vst_process_replacing); - socket_acceptor.accept(host_vst_control); - + // This will block until all sockets have been connected to by the Wine VST + // host + sockets.connect(); #ifndef WITH_WINEDBG host_guard_handler.request_stop(); #endif - // There's no need to keep the socket endpoint file around after accepting - // all the sockets, and RAII won't clean these files up for us - socket_acceptor.close(); - fs::remove(socket_endpoint.path()); - // Set up all pointers for our `AEffect` struct. We will fill this with data // from the VST plugin loaded in Wine at the end of this constructor. plugin.ptr3 = this; @@ -144,8 +127,8 @@ PluginBridge::PluginBridge(audioMasterCallback host_callback) // TODO: Think of a nicer way to structure this and the similar // handler in `Vst2Bridge::handle_dispatch_midi_events` receive_event( - vst_host_callback, std::pair(logger, false), - [&](Event& event) { + sockets.vst_host_callback, + std::pair(logger, false), [&](Event& event) { // MIDI events sent from the plugin back to the host are // a special case here. They have to sent during the // `processReplacing()` function or else the host will @@ -181,13 +164,14 @@ PluginBridge::PluginBridge(audioMasterCallback host_callback) // call these during its initialization. Any further updates will be sent // over the `dispatcher()` socket. This would happen whenever the plugin // calls `audioMasterIOChanged()` and after the host calls `effOpen()`. - const auto initialization_data = read_object(host_vst_control); + const auto initialization_data = + read_object(sockets.host_vst_control); const auto initialized_plugin = std::get(initialization_data.payload); // After receiving the `AEffect` values we'll want to send the configuration // back to complete the startup process - write_object(host_vst_control, config); + write_object(sockets.host_vst_control, config); update_aeffect(plugin, initialized_plugin); } @@ -452,10 +436,10 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, intptr_t return_value = 0; try { // TODO: Add some kind of timeout? - return_value = - send_event(host_vst_dispatch, dispatch_mutex, converter, - std::pair(logger, true), opcode, - index, value, data, option); + return_value = send_event( + sockets.host_vst_dispatch, dispatch_mutex, converter, + std::pair(logger, true), opcode, index, + value, data, option); } catch (const boost::system::system_error& a) { // Thrown when the socket gets closed because the VST plugin // loaded into the Wine process crashed during shutdown @@ -478,7 +462,7 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, // thread and socket to pass MIDI events. Otherwise plugins will // stop receiving MIDI data when they have an open dropdowns or // message box. - return send_event(host_vst_dispatch_midi_events, + return send_event(sockets.host_vst_dispatch_midi_events, dispatch_midi_events_mutex, converter, std::pair(logger, true), opcode, index, value, data, option); @@ -538,7 +522,7 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, // and loading plugin state it's much better to have bitsery or our // receiving function temporarily allocate a large enough buffer rather than // to have a bunch of allocated memory sitting around doing nothing. - return send_event(host_vst_dispatch, dispatch_mutex, converter, + return send_event(sockets.host_vst_dispatch, dispatch_mutex, converter, std::pair(logger, true), opcode, index, value, data, option); } @@ -555,11 +539,11 @@ void PluginBridge::do_process(T** inputs, T** outputs, int sample_frames) { } const AudioBuffers request{input_buffers, sample_frames}; - write_object(host_vst_process_replacing, request, process_buffer); + write_object(sockets.host_vst_process_replacing, request, process_buffer); // Write the results back to the `outputs` arrays - const auto response = - read_object(host_vst_process_replacing, process_buffer); + const auto response = read_object( + sockets.host_vst_process_replacing, process_buffer); const auto& response_buffers = std::get>>(response.buffers); @@ -608,8 +592,8 @@ float PluginBridge::get_parameter(AEffect* /*plugin*/, int index) { // called at the same time since they share the same socket { std::lock_guard lock(parameters_mutex); - write_object(host_vst_parameters, request); - response = read_object(host_vst_parameters); + write_object(sockets.host_vst_parameters, request); + response = read_object(sockets.host_vst_parameters); } logger.log_get_parameter_response(*response.value); @@ -625,9 +609,9 @@ void PluginBridge::set_parameter(AEffect* /*plugin*/, int index, float value) { { std::lock_guard lock(parameters_mutex); - write_object(host_vst_parameters, request); + write_object(sockets.host_vst_parameters, request); - response = read_object(host_vst_parameters); + response = read_object(sockets.host_vst_parameters); } logger.log_set_parameter_response(); @@ -647,7 +631,8 @@ void PluginBridge::log_init_message() { << std::endl; init_msg << "realtime: '" << (has_realtime_priority ? "yes" : "no") << "'" << std::endl; - init_msg << "socket: '" << socket_endpoint.path() << "'" << std::endl; + init_msg << "sockets: '" << sockets.base_dir.string() << "'" + << std::endl; init_msg << "wine prefix: '"; // If the Wine prefix is manually overridden, then this should be made diff --git a/src/plugin/plugin-bridge.h b/src/plugin/plugin-bridge.h index 6b3fc57e..ad0d62a2 100644 --- a/src/plugin/plugin-bridge.h +++ b/src/plugin/plugin-bridge.h @@ -25,6 +25,7 @@ #include "../common/configuration.h" #include "../common/logging.h" +#include "../common/communication.h" #include "host-process.h" /** @@ -123,45 +124,7 @@ class PluginBridge { void log_init_message(); boost::asio::io_context io_context; - boost::asio::local::stream_protocol::endpoint socket_endpoint; - boost::asio::local::stream_protocol::acceptor socket_acceptor; - - // The naming convention for these sockets is `__`. For - // instance the socket named `host_vst_dispatch` forwards - // `AEffect.dispatch()` calls from the native VST host to the Windows VST - // plugin (through the Wine VST host). - - /** - * The socket that forwards all `dispatcher()` calls from the VST host to - * the plugin. - */ - boost::asio::local::stream_protocol::socket host_vst_dispatch; - /** - * Used specifically for the `effProcessEvents` opcode. This is needed - * because the Win32 API is designed to block during certain GUI - * interactions such as resizing a window or opening a dropdown. Without - * this MIDI input would just stop working at times. - */ - boost::asio::local::stream_protocol::socket host_vst_dispatch_midi_events; - /** - * The socket that forwards all `audioMaster()` calls from the Windows VST - * plugin to the host. - */ - boost::asio::local::stream_protocol::socket vst_host_callback; - /** - * Used for both `getParameter` and `setParameter` since they mostly - * overlap. - */ - boost::asio::local::stream_protocol::socket host_vst_parameters; - boost::asio::local::stream_protocol::socket host_vst_process_replacing; - - /** - * A control socket that sends data that is not suitable for the other - * sockets. At the moment this is only used to, on startup, send the Windows - * VST plugin's `AEffect` object to the native VST plugin, and to then send - * the configuration (from `config`) back to the Wine host. - */ - boost::asio::local::stream_protocol::socket host_vst_control; + Sockets sockets; /** * The thread that handles host callbacks. diff --git a/src/plugin/utils.cpp b/src/plugin/utils.cpp index 651bbaa5..86ca0e82 100644 --- a/src/plugin/utils.cpp +++ b/src/plugin/utils.cpp @@ -22,37 +22,27 @@ #include #include #include -#include #include // Generated inside of the build directory #include #include "../common/configuration.h" +#include "../common/utils.h" namespace bp = boost::process; namespace fs = boost::filesystem; -/** - * Used for generating random identifiers. - */ -constexpr char alphanumeric_characters[] = - "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; - -std::string create_logger_prefix(const fs::path& socket_path) { - // Use the socket filename as the logger prefix, but strip the `yabridge-` - // part since that's redundant - std::string socket_name = - socket_path.filename().replace_extension().string(); +std::string create_logger_prefix(const fs::path& endpoint_base_dir) { + // Use the name of the base directory used for our sockets as the logger + // prefix, but strip the `yabridge-` part since that's redundant + std::string endpoint_name = endpoint_base_dir.filename().string(); constexpr std::string_view socket_prefix("yabridge-"); - assert(socket_name.starts_with(socket_prefix)); - socket_name = socket_name.substr(socket_prefix.size()); + assert(endpoint_name.starts_with(socket_prefix)); + endpoint_name = endpoint_name.substr(socket_prefix.size()); - std::ostringstream prefix; - prefix << "[" << socket_name << "] "; - - return prefix.str(); + return "[" + endpoint_name + "] "; } std::optional find_wineprefix() { @@ -183,39 +173,7 @@ boost::filesystem::path generate_group_endpoint( } socket_name << ".sock"; - return fs::temp_directory_path() / socket_name.str(); -} - -fs::path generate_plugin_endpoint() { - const auto plugin_name = - find_vst_plugin().filename().replace_extension("").string(); - - std::random_device random_device; - std::mt19937 rng(random_device()); - fs::path candidate_endpoint; - do { - std::string random_id; - std::sample( - alphanumeric_characters, - alphanumeric_characters + strlen(alphanumeric_characters) - 1, - std::back_inserter(random_id), 8, rng); - - // We'll get rid of the file descriptors immediately after accepting the - // sockets, so putting them inside of a subdirectory would only leave - // behind an empty directory - std::ostringstream socket_name; - socket_name << "yabridge-" << plugin_name << "-" << random_id - << ".sock"; - - candidate_endpoint = fs::temp_directory_path() / socket_name.str(); - } while (fs::exists(candidate_endpoint)); - - // TODO: Should probably try creating the endpoint right here and catch any - // exceptions since this could technically result in a race condition - // when two instances of yabridge decide to use the same endpoint name - // at the same time - - return candidate_endpoint; + return get_temporary_directory() / socket_name.str(); } fs::path get_this_file_location() { @@ -243,7 +201,7 @@ std::string get_wine_version() { bp::environment env = boost::this_process::environment(); if (!env["WINELOADER"].empty()) { - wine_command = env.get("WINELOADER"); + wine_command = env["WINELOADER"].to_string(); } bp::ipstream output; @@ -271,13 +229,13 @@ std::string get_wine_version() { std::string join_quoted_strings(std::vector& strings) { bool is_first = true; - std::ostringstream joined_strigns{}; + std::ostringstream joined_strings{}; for (const auto& option : strings) { - joined_strigns << (is_first ? "'" : ", '") << option << "'"; + joined_strings << (is_first ? "'" : ", '") << option << "'"; is_first = false; } - return joined_strigns.str(); + return joined_strings.str(); } Configuration load_config_for(const fs::path& yabridge_path) { diff --git a/src/plugin/utils.h b/src/plugin/utils.h index a9d3fb92..8e2d6fbb 100644 --- a/src/plugin/utils.h +++ b/src/plugin/utils.h @@ -46,15 +46,17 @@ class patched_async_pipe : public boost::process::async_pipe { enum class PluginArchitecture { vst_32, vst_64 }; /** - * Create a logger prefix based on the unique socket path for easy - * identification. The socket path contains both the plugin's name and a unique - * identifier. + * Create a logger prefix based on the endpoint base directory used for the + * sockets for easy identification. This will result in a prefix of the form + * `[-] `. * - * @param socket_path The path to the socket endpoint in use. + * @param endpoint_base_dir A directory name generated by + * `generate_endpoint_base()`. * * @return A prefix string for log messages. */ -std::string create_logger_prefix(const boost::filesystem::path& socket_path); +std::string create_logger_prefix( + const boost::filesystem::path& endpoint_base_dir); /** * Determine the architecture of a VST plugin (or rather, a .dll file) based on @@ -117,10 +119,11 @@ std::optional find_wineprefix(); /** * Generate the group socket endpoint name used based on the name of the group, * the Wine prefix in use and the plugin architecture. The resulting format is - * `/tmp/yabridge-group---.sock`. In - * this socket name the `wine_prefix_id` is a numerical hash based on the Wine - * prefix in use. This way the same group name can be used for multiple Wine - * prefixes and for both 32 and 64 bit plugins without clashes. + * in the form + * `/run/user//yabridge-group---.sock`. + * In this socket name the `wine_prefix_id` is a numerical hash based on the + * Wine prefix in use. This way the same group name can be used for multiple + * Wine prefixes and for both 32 and 64 bit plugins without clashes. * * @param group_name The name of the plugin group. * @param wine_prefix The name of the Wine prefix in use. This should be @@ -140,16 +143,6 @@ boost::filesystem::path generate_group_endpoint( const boost::filesystem::path& wine_prefix, const PluginArchitecture architecture); -/** - * Generate a unique name for the Unix domain socket endpoint based on the VST - * plugin's name. This will also generate the parent directory if it does not - * yet exist since we're using this in the constructor's initializer list. - * - * @return A path to a not yet existing Unix domain socket endpoint. - * @throw std::runtime_error If no matching .dll file could be found. - */ -boost::filesystem::path generate_plugin_endpoint(); - /** * Return a path to this `.so` file. This can be used to find out from where * this link to or copy of `libyabridge.so` was loaded. diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index bb01b6bb..b50fdbe4 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -209,10 +209,12 @@ void GroupBridge::accept_requests() { // this has to be done on the same thread that's handling messages, // and all window messages have to be handled from the same thread. logger.log("Received request to host '" + request.plugin_path + - "' using socket '" + request.socket_path + "'"); + "' using socket endpoint base directory '" + + request.endpoint_base_dir + "'"); try { auto bridge = std::make_unique( - plugin_context, request.plugin_path, request.socket_path); + plugin_context, request.plugin_path, + request.endpoint_base_dir); logger.log("Finished initializing '" + request.plugin_path + "'"); diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index e2647cb3..7d5ad91f 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -67,16 +67,10 @@ Vst2Bridge& get_bridge_instance(const AEffect* plugin) { Vst2Bridge::Vst2Bridge(boost::asio::io_context& main_context, std::string plugin_dll_path, - std::string socket_endpoint_path) + std::string endpoint_base_dir) : io_context(main_context), plugin_handle(LoadLibrary(plugin_dll_path.c_str()), FreeLibrary), - socket_endpoint(socket_endpoint_path), - host_vst_dispatch(io_context), - host_vst_dispatch_midi_events(io_context), - vst_host_callback(io_context), - host_vst_parameters(io_context), - host_vst_process_replacing(io_context), - host_vst_control(io_context) { + sockets(io_context, endpoint_base_dir, false) { // Got to love these C APIs if (!plugin_handle) { throw std::runtime_error("Could not load the Windows .dll file at '" + @@ -101,14 +95,7 @@ Vst2Bridge::Vst2Bridge(boost::asio::io_context& main_context, "'."); } - // It's very important that these sockets are accepted to in the same order - // in the Linux plugin - host_vst_dispatch.connect(socket_endpoint); - host_vst_dispatch_midi_events.connect(socket_endpoint); - vst_host_callback.connect(socket_endpoint); - host_vst_parameters.connect(socket_endpoint); - host_vst_process_replacing.connect(socket_endpoint); - host_vst_control.connect(socket_endpoint); + sockets.connect(); // Initialize after communication has been set up // We'll try to do the same `get_bridge_isntance` trick as in @@ -132,13 +119,14 @@ Vst2Bridge::Vst2Bridge(boost::asio::io_context& main_context, // of this object will be sent over the `dispatcher()` socket. This would be // done after the host calls `effOpen()`, and when the plugin calls // `audioMasterIOChanged()`. - write_object(host_vst_control, EventResult{.return_value = 0, - .payload = *plugin, - .value_payload = std::nullopt}); + write_object(sockets.host_vst_control, + EventResult{.return_value = 0, + .payload = *plugin, + .value_payload = std::nullopt}); // After sending the AEffect struct we'll receive this instance's // configuration as a response - config = read_object(host_vst_control); + config = read_object(sockets.host_vst_control); // This works functionally identically to the `handle_dispatch()` function, // but this socket will only handle MIDI events and it will handle them @@ -160,7 +148,7 @@ void Vst2Bridge::handle_dispatch() { while (true) { try { receive_event( - host_vst_dispatch, std::nullopt, + sockets.host_vst_dispatch, std::nullopt, passthrough_event( plugin, [&](AEffect* plugin, int opcode, int index, intptr_t value, @@ -194,7 +182,8 @@ void Vst2Bridge::handle_dispatch_midi_events() { while (true) { try { receive_event( - host_vst_dispatch_midi_events, std::nullopt, [&](Event& event) { + sockets.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 @@ -255,19 +244,19 @@ void Vst2Bridge::handle_parameters() { // through on this socket since they have a lot of overlap. The // presence of the `value` field tells us which one we're dealing // with. - auto request = read_object(host_vst_parameters); + auto request = read_object(sockets.host_vst_parameters); if (request.value) { // `setParameter` plugin->setParameter(plugin, request.index, *request.value); ParameterResult response{std::nullopt}; - write_object(host_vst_parameters, response); + write_object(sockets.host_vst_parameters, response); } else { // `getParameter` float value = plugin->getParameter(plugin, request.index); ParameterResult response{value}; - write_object(host_vst_parameters, response); + write_object(sockets.host_vst_parameters, response); } } catch (const boost::system::system_error&) { // The plugin has cut off communications, so we can shut down this @@ -288,8 +277,8 @@ void Vst2Bridge::handle_process_replacing() { while (true) { try { - auto request = read_object(host_vst_process_replacing, - process_buffer); + auto request = read_object( + sockets.host_vst_process_replacing, process_buffer); // Let the plugin process the MIDI events that were received since // the last buffer, and then clean up those events. This approach // should not be needed but Kontakt only stores pointers to rather @@ -348,8 +337,8 @@ void Vst2Bridge::handle_process_replacing() { AudioBuffers response{output_buffers_single_precision, request.sample_frames}; - write_object(host_vst_process_replacing, response, - process_buffer); + write_object(sockets.host_vst_process_replacing, + response, process_buffer); }, [&](std::vector>& input_buffers) { // Exactly the same as the above, but for double @@ -373,8 +362,8 @@ void Vst2Bridge::handle_process_replacing() { AudioBuffers response{output_buffers_double_precision, request.sample_frames}; - write_object(host_vst_process_replacing, response, - process_buffer); + write_object(sockets.host_vst_process_replacing, + response, process_buffer); }}, request.buffers); @@ -419,7 +408,7 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin, // When hosting multiple plugins in a group process, all plugins // should get a unique window class const std::string window_class = - "yabridge plugin " + socket_endpoint.path(); + "yabridge plugin " + sockets.base_dir.string(); Editor& editor_instance = editor.emplace( config, window_class, x11_handle, plugin); @@ -581,7 +570,7 @@ intptr_t Vst2Bridge::host_callback(AEffect* effect, } HostCallbackDataConverter converter(effect, time_info); - return send_event(vst_host_callback, host_callback_mutex, converter, + return send_event(sockets.vst_host_callback, host_callback_mutex, converter, std::nullopt, opcode, index, value, data, option); } diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 3d0815af..162907e4 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -31,6 +31,7 @@ #include "../../common/configuration.h" #include "../../common/logging.h" +#include "../../common/communication.h" #include "../editor.h" #include "../utils.h" @@ -46,7 +47,7 @@ struct EditorOpening {}; * plugin and provides host callback function for the plugin to talk back. * * @remark Because of Win32 API limitations, all window handling has to be done - * from the same thread. Most plugins won't have any issues when using + * from a single thread. Most plugins won't have any issues when using * multiple message loops, but the Melda plugins for instance will only update * their GUIs from the message loop of the thread that created the first * instance. This is why we pass an IO context to this class so everything @@ -64,8 +65,8 @@ class Vst2Bridge { * also be run from this context. * @param plugin_dll_path A (Unix style) path to the VST plugin .dll file to * load. - * @param socket_endpoint_path A (Unix style) path to the Unix socket - * endpoint the native VST plugin created to communicate over. + * @param endpoint_base_dir The base directory used for the socket + * endpoints. See `Sockets` for more information. * * @note The object has to be constructed from the same thread that calls * `main_context.run()`. @@ -75,7 +76,7 @@ class Vst2Bridge { */ Vst2Bridge(boost::asio::io_context& main_context, std::string plugin_dll_path, - std::string socket_endpoint_path); + std::string endpoint_base_dir); /** * Returns true if the message loop should be skipped. This happens when the @@ -189,47 +190,9 @@ class Vst2Bridge { AEffect* plugin; /** - * The UNIX domain socket endpoint used for communicating to this specific - * bridged plugin. + * All sockets used for communicating with this specific plugin. */ - boost::asio::local::stream_protocol::endpoint socket_endpoint; - - // The naming convention for these sockets is `__`. For - // instance the socket named `host_vst_dispatch` forwards - // `AEffect.dispatch()` calls from the native VST host to the Windows VST - // plugin (through the Wine VST host). - - /** - * The socket that forwards all `dispatcher()` calls from the VST host to - * the plugin. - */ - boost::asio::local::stream_protocol::socket host_vst_dispatch; - /** - * Used specifically for the `effProcessEvents` opcode. This is needed - * because the Win32 API is designed to block during certain GUI - * interactions such as resizing a window or opening a dropdown. Without - * this MIDI input would just stop working at times. - */ - boost::asio::local::stream_protocol::socket host_vst_dispatch_midi_events; - /** - * The socket that forwards all `audioMaster()` calls from the Windows VST - * plugin to the host. - */ - boost::asio::local::stream_protocol::socket vst_host_callback; - /** - * Used for both `getParameter` and `setParameter` since they mostly - * overlap. - */ - boost::asio::local::stream_protocol::socket host_vst_parameters; - boost::asio::local::stream_protocol::socket host_vst_process_replacing; - - /** - * A control socket that sends data that is not suitable for the other - * sockets. At the moment this is only used to, on startup, send the Windows - * VST plugin's `AEffect` object to the native VST plugin, and to then send - * the configuration (from `config`) back to the Wine host. - */ - boost::asio::local::stream_protocol::socket host_vst_control; + Sockets sockets; /** * The thread that specifically handles `effProcessEvents` opcodes so the diff --git a/src/wine-host/individual-host.cpp b/src/wine-host/individual-host.cpp index 48e4f847..3f93e46b 100644 --- a/src/wine-host/individual-host.cpp +++ b/src/wine-host/individual-host.cpp @@ -39,7 +39,7 @@ void async_handle_events(boost::asio::steady_timer& timer, Vst2Bridge& bridge); /** * This is the default VST host application. It will load the specified VST2 - * plugin, and then connect back to the `libyabridge.so` instace that spawned + * plugin, and then connect back to the `libyabridge.so` instance that spawned * this over the socket. * * The explicit calling convention is needed to work around a bug introduced in @@ -48,9 +48,9 @@ void async_handle_events(boost::asio::steady_timer& timer, Vst2Bridge& bridge); int __cdecl main(int argc, char* argv[]) { set_realtime_priority(); - // We pass the name of the VST plugin .dll file to load and the Unix domain - // socket to connect to in plugin/bridge.cpp as the first two arguments of - // this process. + // We pass the name of the VST plugin .dll file to load and the base + // directory for the Unix domain socket endpoints to connect to as the first + // two arguments of this process in plugin/bridge.cpp. if (argc < 3) { std::cerr << "Usage: " #ifdef __i386__ @@ -58,7 +58,7 @@ int __cdecl main(int argc, char* argv[]) { #else << yabridge_individual_host_name #endif - << " " << std::endl; + << " " << std::endl; return 1; } From 54ed69c408474442b90d82462ada0c3f6205cb44 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 25 Oct 2020 21:25:31 +0100 Subject: [PATCH 04/40] Merge events.h into communication.h --- src/common/communication.cpp | 53 +++++ src/common/communication.h | 354 ++++++++++++++++++++++++++++ src/common/events.h | 408 --------------------------------- src/plugin/plugin-bridge.cpp | 1 - src/wine-host/bridges/vst2.cpp | 1 - 5 files changed, 407 insertions(+), 410 deletions(-) delete mode 100644 src/common/events.h diff --git a/src/common/communication.cpp b/src/common/communication.cpp index 391eb709..4aebeeb4 100644 --- a/src/common/communication.cpp +++ b/src/common/communication.cpp @@ -101,6 +101,59 @@ void Sockets::connect() { } } +EventPayload DefaultDataConverter::read(const int /*opcode*/, + const int /*index*/, + const intptr_t /*value*/, + const void* data) const { + if (!data) { + return nullptr; + } + + // This is a simple fallback that will work in almost every case. + // Because some plugins don't zero out their string buffers when sending + // host callbacks, we will explicitely list all callbacks that expect a + // string in `DispatchDataConverter` adn `HostCallbackDataConverter`. + const char* c_string = static_cast(data); + if (c_string[0] != 0) { + return std::string(c_string); + } else { + return WantsString{}; + } +} + +std::optional DefaultDataConverter::read_value( + const int /*opcode*/, + const intptr_t /*value*/) const { + return std::nullopt; +} + +void DefaultDataConverter::write(const int /*opcode*/, + void* data, + const EventResult& response) const { + // The default behavior is to handle this as a null terminated C-style + // string + std::visit(overload{[&](const auto&) {}, + [&](const std::string& s) { + char* output = static_cast(data); + + // We use std::string for easy transport but in + // practice we're always writing null terminated + // C-style strings + std::copy(s.begin(), s.end(), output); + output[s.size()] = 0; + }}, + response.payload); +} + +void DefaultDataConverter::write_value(const int /*opcode*/, + intptr_t /*value*/, + const EventResult& /*response*/) const {} + +intptr_t DefaultDataConverter::return_value(const int /*opcode*/, + const intptr_t original) const { + return original; +} + boost::filesystem::path generate_endpoint_base(const std::string& plugin_name) { fs::path temp_directory = get_temporary_directory(); diff --git a/src/common/communication.h b/src/common/communication.h index 21e5ee47..6df4ca16 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -28,6 +28,8 @@ #include #include +#include "logging.h" + template using OutputAdapter = bitsery::OutputBufferAdapter; @@ -166,6 +168,61 @@ class Sockets { std::optional acceptors; }; +/** + * Encodes the base behavior for reading from and writing to the `data` argument + * for event dispatch functions. This provides base functionality for these + * kinds of events. The `dispatch()` function will require some more specific + * structs. + */ +class DefaultDataConverter { + public: + virtual ~DefaultDataConverter(){}; + + /** + * Read data from the `data` void pointer into a an `EventPayload` value + * that can be serialized and conveys the meaning of the event. + */ + virtual EventPayload read(const int opcode, + const int index, + const intptr_t value, + const void* data) const; + + /** + * Read data from the `value` pointer into a an `EventPayload` value that + * can be serialized and conveys the meaning of the event. This is only used + * for the `effSetSpeakerArrangement` and `effGetSpeakerArrangement` events. + */ + virtual std::optional read_value(const int opcode, + const intptr_t value) const; + + /** + * Write the reponse back to the `data` pointer. + */ + virtual void write(const int opcode, + void* data, + const EventResult& response) const; + + /** + * Write the reponse back to the `value` pointer. This is only used during + * the `effGetSpeakerArrangement` event. + */ + virtual void write_value(const int opcode, + intptr_t value, + const EventResult& response) const; + + /** + * This function can override a callback's return value based on the opcode. + * This is used in one place to return a pointer to a `VstTime` object + * that's contantly being updated. + * + * @param opcode The opcode for the current event. + * @param original The original return value as returned by the callback + * function. + */ + virtual intptr_t return_value(const int opcode, + const intptr_t original) const; +}; + /** * Generate a unique base directory that can be used as a prefix for all Unix * domain socket endpoints used in `PluginBridge`/`Vst2Bridge`. This will @@ -261,3 +318,300 @@ inline T read_object(Socket& socket, return object; } + +/** + * Serialize and send an event over a socket. This is used for both the host -> + * plugin 'dispatch' events and the plugin -> host 'audioMaster' host callbacks + * since they follow the same format. See one of those functions for details on + * 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 `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 + * threads at once. + * @param data_converter Some struct that knows how to read data from and write + * data back to the `data` void pointer. For host callbacks this parameter + * contains either a string or a null pointer while `dispatch()` calls might + * contain opcode specific structs. See the documentation for `EventPayload` + * for more information. The `DefaultDataConverter` defined above handles the + * basic behavior that's sufficient for host callbacks. + * @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. + * + * @relates receive_event + * @relates passthrough_event + */ +template +intptr_t send_event(boost::asio::local::stream_protocol::socket& socket, + std::mutex& write_mutex, + D& data_converter, + std::optional> logging, + int opcode, + int index, + intptr_t value, + void* data, + float option) { + // Encode the right payload types for this event. Check the documentation + // for `EventPayload` for more information. These types are converted to + // C-style data structures in `passthrough_event()` so they can be passed to + // a plugin or callback function. + const EventPayload payload = + data_converter.read(opcode, index, value, data); + const std::optional value_payload = + data_converter.read_value(opcode, value); + + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event(is_dispatch, opcode, index, value, payload, option, + value_payload); + } + + const Event event{.opcode = opcode, + .index = index, + .value = value, + .option = option, + .payload = payload, + .value_payload = value_payload}; + + // Prevent two threads from writing over the socket at the same time and + // messages getting out of order. This is needed because we can't prevent + // the plugin or the host from calling `dispatch()` or `audioMaster()` from + // multiple threads. + EventResult response; + { + std::lock_guard lock(write_mutex); + write_object(socket, event); + response = read_object(socket); + } + + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event_response(is_dispatch, opcode, response.return_value, + response.payload, response.value_payload); + } + + data_converter.write(opcode, data, response); + data_converter.write_value(opcode, value, response); + + return data_converter.return_value(opcode, response.return_value); +} + +/** + * 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 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 +void receive_event(boost::asio::local::stream_protocol::socket& socket, + std::optional> logging, + F callback) { + auto event = read_object(socket); + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event(is_dispatch, event.opcode, event.index, event.value, + event.payload, event.option, event.value_payload); + } + + EventResult response = callback(event); + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event_response(is_dispatch, event.opcode, + response.return_value, response.payload, + response.value_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 +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 the beginning with null values to both prevent it from + // being read as some arbitrary C-style string, and to make sure that + // `*static_cast(string_buffer.data)` will be a null pointer if + // the plugin is supposed to write a pointer there but doesn't (such as + // with `effEditGetRect`/`WantsVstRect`). + std::array string_buffer; + std::fill(string_buffer.begin(), string_buffer.begin() + sizeof(size_t), + 0); + + auto read_payload_fn = overload{ + [&](const std::nullptr_t&) -> void* { return nullptr; }, + [&](const std::string& s) -> void* { + return const_cast(s.c_str()); + }, + [&](const std::vector& buffer) -> void* { + return const_cast(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. + return reinterpret_cast( + static_cast(window_handle)); + }, + [&](const AEffect&) -> void* { return nullptr; }, + [&](DynamicVstEvents& events) -> void* { + return &events.as_c_events(); + }, + [&](DynamicSpeakerArrangement& speaker_arrangement) -> void* { + return &speaker_arrangement.as_c_speaker_arrangement(); + }, + [&](WantsAEffectUpdate&) -> void* { + // The host will never actually ask for an updated `AEffect` + // object since that should not be a thing. This is purely a + // meant as a workaround for plugins that initialize their + // `AEffect` object after the plugin has already finished + // initializing. + return nullptr; + }, + [&](WantsChunkBuffer&) -> void* { return string_buffer.data(); }, + [&](VstIOProperties& props) -> void* { return &props; }, + [&](VstMidiKeyName& key_name) -> void* { return &key_name; }, + [&](VstParameterProperties& props) -> void* { return &props; }, + [&](WantsVstRect&) -> void* { return string_buffer.data(); }, + [&](const WantsVstTimeInfo&) -> void* { return nullptr; }, + [&](WantsString&) -> void* { return string_buffer.data(); }}; + + // Almost all events pass data through the `data` argument. There are + // two events, `effSetParameter` and `effGetParameter` that also pass + // data through the value argument. + void* data = std::visit(read_payload_fn, event.payload); + intptr_t value = event.value; + if (event.value_payload) { + value = reinterpret_cast( + std::visit(read_payload_fn, *event.value_payload)); + } + + const intptr_t return_value = callback( + plugin, event.opcode, event.index, value, data, event.option); + + // Only write back data when needed, this depends on the event payload + // type + auto write_payload_fn = overload{ + [&](auto) -> EventResultPayload { return nullptr; }, + [&](const AEffect& updated_plugin) -> EventResultPayload { + // This is a bit of a special case! Instead of writing some + // return value, we will update values on the native VST + // plugin's `AEffect` object. This is triggered by the + // `audioMasterIOChanged` callback from the hosted VST plugin. + update_aeffect(*plugin, updated_plugin); + + return nullptr; + }, + [&](DynamicSpeakerArrangement& speaker_arrangement) + -> EventResultPayload { return speaker_arrangement; }, + [&](WantsChunkBuffer&) -> EventResultPayload { + // In this case the plugin will have written its data 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 + const uint8_t* chunk_data = *static_cast(data); + return std::vector(chunk_data, + chunk_data + return_value); + }, + [&](VstIOProperties& props) -> EventResultPayload { return props; }, + [&](VstMidiKeyName& key_name) -> EventResultPayload { + return key_name; + }, + [&](VstParameterProperties& props) -> EventResultPayload { + return props; + }, + [&](WantsAEffectUpdate&) -> EventResultPayload { return *plugin; }, + [&](WantsVstRect&) -> EventResultPayload { + // The plugin should have written a pointer to a VstRect struct + // into the data pointer. I haven't seen this fail yet, but + // since some hosts will call `effEditGetRect()` before + // `effEditOpen()` I can assume there are plugins that don't + // handle this correctly. + VstRect* editor_rect = *static_cast(data); + if (!editor_rect) { + return nullptr; + } + + return *editor_rect; + }, + [&](WantsVstTimeInfo&) -> EventResultPayload { + // Not sure why the VST API has twenty different ways of + // returning structs, but in this case the value returned from + // the callback function is actually a pointer to a + // `VstTimeInfo` struct! It can also be a null pointer if the + // host doesn't support this. + const auto time_info = + reinterpret_cast(return_value); + if (!time_info) { + return nullptr; + } else { + return *time_info; + } + }, + [&](WantsString&) -> EventResultPayload { + return std::string(static_cast(data)); + }}; + + // As mentioned about, the `effSetSpeakerArrangement` and + // `effGetSpeakerArrangement` events are the only two events that use + // the value argument as a pointer to write data to. Additionally, the + // `effGetSpeakerArrangement` expects the plugin to write its own data + // to this value. Hence why we need to encode the response here + // separately. + const EventResultPayload response_data = + std::visit(write_payload_fn, event.payload); + std::optional value_response_data = std::nullopt; + if (event.value_payload) { + value_response_data = + std::visit(write_payload_fn, *event.value_payload); + } + + EventResult response{.return_value = return_value, + .payload = response_data, + .value_payload = value_response_data}; + + return response; + }; +} diff --git a/src/common/events.h b/src/common/events.h deleted file mode 100644 index 68864b13..00000000 --- a/src/common/events.h +++ /dev/null @@ -1,408 +0,0 @@ -// yabridge: a Wine VST bridge -// Copyright (C) 2020 Robbert van der Helm -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with this program. If not, see . - -#pragma once - -#include - -#include "communication.h" -#include "logging.h" - -/** - * Encodes the base behavior for reading from and writing to the `data` argument - * for event dispatch functions. This provides base functionality for these - * kinds of events. The `dispatch()` function will require some more specific - * structs. - */ -class DefaultDataConverter { - public: - virtual ~DefaultDataConverter(){}; - - /** - * Read data from the `data` void pointer into a an `EventPayload` value - * that can be serialized and conveys the meaning of the event. - */ - virtual EventPayload read(const int /*opcode*/, - const int /*index*/, - const intptr_t /*value*/, - const void* data) const { - if (!data) { - return nullptr; - } - - // This is a simple fallback that will work in almost every case. - // Because some plugins don't zero out their string buffers when sending - // host callbacks, we will explicitely list all callbacks that expect a - // string in `DispatchDataConverter` adn `HostCallbackDataConverter`. - const char* c_string = static_cast(data); - if (c_string[0] != 0) { - return std::string(c_string); - } else { - return WantsString{}; - } - } - - /** - * Read data from the `value` pointer into a an `EventPayload` value that - * can be serialized and conveys the meaning of the event. This is only used - * for the `effSetSpeakerArrangement` and `effGetSpeakerArrangement` events. - */ - virtual std::optional read_value( - const int /*opcode*/, - const intptr_t /*value*/) const { - return std::nullopt; - } - - /** - * Write the reponse back to the `data` pointer. - */ - virtual void write(const int /*opcode*/, - void* data, - const EventResult& response) const { - // The default behavior is to handle this as a null terminated C-style - // string - std::visit(overload{[&](const auto&) {}, - [&](const std::string& s) { - char* output = static_cast(data); - - // We use std::string for easy transport but in - // practice we're always writing null terminated - // C-style strings - std::copy(s.begin(), s.end(), output); - output[s.size()] = 0; - }}, - response.payload); - } - - /** - * Write the reponse back to the `value` pointer. This is only used during - * the `effGetSpeakerArrangement` event. - */ - virtual void write_value(const int /*opcode*/, - intptr_t /*value*/, - const EventResult& /*response*/) const {} - - /** - * This function can override a callback's return value based on the opcode. - * This is used in one place to return a pointer to a `VstTime` object - * that's contantly being updated. - * - * @param opcode The opcode for the current event. - * @param original The original return value as returned by the callback - * function. - */ - virtual intptr_t return_value(const int /*opcode*/, - const intptr_t original) const { - return original; - } -}; - -/** - * Serialize and send an event over a socket. This is used for both the host -> - * plugin 'dispatch' events and the plugin -> host 'audioMaster' host callbacks - * since they follow the same format. See one of those functions for details on - * 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 `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 - * threads at once. - * @param data_converter Some struct that knows how to read data from and write - * data back to the `data` void pointer. For host callbacks this parameter - * contains either a string or a null pointer while `dispatch()` calls might - * contain opcode specific structs. See the documentation for `EventPayload` - * for more information. The `DefaultDataConverter` defined above handles the - * basic behavior that's sufficient for host callbacks. - * @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. - * - * @relates receive_event - * @relates passthrough_event - */ -template -intptr_t send_event(boost::asio::local::stream_protocol::socket& socket, - std::mutex& write_mutex, - D& data_converter, - std::optional> logging, - int opcode, - int index, - intptr_t value, - void* data, - float option) { - // Encode the right payload types for this event. Check the documentation - // for `EventPayload` for more information. These types are converted to - // C-style data structures in `passthrough_event()` so they can be passed to - // a plugin or callback function. - const EventPayload payload = - data_converter.read(opcode, index, value, data); - const std::optional value_payload = - data_converter.read_value(opcode, value); - - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event(is_dispatch, opcode, index, value, payload, option, - value_payload); - } - - const Event event{.opcode = opcode, - .index = index, - .value = value, - .option = option, - .payload = payload, - .value_payload = value_payload}; - - // Prevent two threads from writing over the socket at the same time and - // messages getting out of order. This is needed because we can't prevent - // the plugin or the host from calling `dispatch()` or `audioMaster()` from - // multiple threads. - EventResult response; - { - std::lock_guard lock(write_mutex); - write_object(socket, event); - response = read_object(socket); - } - - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event_response(is_dispatch, opcode, response.return_value, - response.payload, response.value_payload); - } - - data_converter.write(opcode, data, response); - data_converter.write_value(opcode, value, response); - - return data_converter.return_value(opcode, response.return_value); -} - -/** - * 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 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 -void receive_event(boost::asio::local::stream_protocol::socket& socket, - std::optional> logging, - F callback) { - auto event = read_object(socket); - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event(is_dispatch, event.opcode, event.index, event.value, - event.payload, event.option, event.value_payload); - } - - EventResult response = callback(event); - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event_response(is_dispatch, event.opcode, - response.return_value, response.payload, - response.value_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 -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 the beginning with null values to both prevent it from - // being read as some arbitrary C-style string, and to make sure that - // `*static_cast(string_buffer.data)` will be a null pointer if - // the plugin is supposed to write a pointer there but doesn't (such as - // with `effEditGetRect`/`WantsVstRect`). - std::array string_buffer; - std::fill(string_buffer.begin(), string_buffer.begin() + sizeof(size_t), - 0); - - auto read_payload_fn = overload{ - [&](const std::nullptr_t&) -> void* { return nullptr; }, - [&](const std::string& s) -> void* { - return const_cast(s.c_str()); - }, - [&](const std::vector& buffer) -> void* { - return const_cast(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. - return reinterpret_cast( - static_cast(window_handle)); - }, - [&](const AEffect&) -> void* { return nullptr; }, - [&](DynamicVstEvents& events) -> void* { - return &events.as_c_events(); - }, - [&](DynamicSpeakerArrangement& speaker_arrangement) -> void* { - return &speaker_arrangement.as_c_speaker_arrangement(); - }, - [&](WantsAEffectUpdate&) -> void* { - // The host will never actually ask for an updated `AEffect` - // object since that should not be a thing. This is purely a - // meant as a workaround for plugins that initialize their - // `AEffect` object after the plugin has already finished - // initializing. - return nullptr; - }, - [&](WantsChunkBuffer&) -> void* { return string_buffer.data(); }, - [&](VstIOProperties& props) -> void* { return &props; }, - [&](VstMidiKeyName& key_name) -> void* { return &key_name; }, - [&](VstParameterProperties& props) -> void* { return &props; }, - [&](WantsVstRect&) -> void* { return string_buffer.data(); }, - [&](const WantsVstTimeInfo&) -> void* { return nullptr; }, - [&](WantsString&) -> void* { return string_buffer.data(); }}; - - // Almost all events pass data through the `data` argument. There are - // two events, `effSetParameter` and `effGetParameter` that also pass - // data through the value argument. - void* data = std::visit(read_payload_fn, event.payload); - intptr_t value = event.value; - if (event.value_payload) { - value = reinterpret_cast( - std::visit(read_payload_fn, *event.value_payload)); - } - - const intptr_t return_value = callback( - plugin, event.opcode, event.index, value, data, event.option); - - // Only write back data when needed, this depends on the event payload - // type - auto write_payload_fn = overload{ - [&](auto) -> EventResultPayload { return nullptr; }, - [&](const AEffect& updated_plugin) -> EventResultPayload { - // This is a bit of a special case! Instead of writing some - // return value, we will update values on the native VST - // plugin's `AEffect` object. This is triggered by the - // `audioMasterIOChanged` callback from the hosted VST plugin. - update_aeffect(*plugin, updated_plugin); - - return nullptr; - }, - [&](DynamicSpeakerArrangement& speaker_arrangement) - -> EventResultPayload { return speaker_arrangement; }, - [&](WantsChunkBuffer&) -> EventResultPayload { - // In this case the plugin will have written its data 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 - const uint8_t* chunk_data = *static_cast(data); - return std::vector(chunk_data, - chunk_data + return_value); - }, - [&](VstIOProperties& props) -> EventResultPayload { return props; }, - [&](VstMidiKeyName& key_name) -> EventResultPayload { - return key_name; - }, - [&](VstParameterProperties& props) -> EventResultPayload { - return props; - }, - [&](WantsAEffectUpdate&) -> EventResultPayload { return *plugin; }, - [&](WantsVstRect&) -> EventResultPayload { - // The plugin should have written a pointer to a VstRect struct - // into the data pointer. I haven't seen this fail yet, but - // since some hosts will call `effEditGetRect()` before - // `effEditOpen()` I can assume there are plugins that don't - // handle this correctly. - VstRect* editor_rect = *static_cast(data); - if (!editor_rect) { - return nullptr; - } - - return *editor_rect; - }, - [&](WantsVstTimeInfo&) -> EventResultPayload { - // Not sure why the VST API has twenty different ways of - // returning structs, but in this case the value returned from - // the callback function is actually a pointer to a - // `VstTimeInfo` struct! It can also be a null pointer if the - // host doesn't support this. - const auto time_info = - reinterpret_cast(return_value); - if (!time_info) { - return nullptr; - } else { - return *time_info; - } - }, - [&](WantsString&) -> EventResultPayload { - return std::string(static_cast(data)); - }}; - - // As mentioned about, the `effSetSpeakerArrangement` and - // `effGetSpeakerArrangement` events are the only two events that use - // the value argument as a pointer to write data to. Additionally, the - // `effGetSpeakerArrangement` expects the plugin to write its own data - // to this value. Hence why we need to encode the response here - // separately. - const EventResultPayload response_data = - std::visit(write_payload_fn, event.payload); - std::optional value_response_data = std::nullopt; - if (event.value_payload) { - value_response_data = - std::visit(write_payload_fn, *event.value_payload); - } - - EventResult response{.return_value = return_value, - .payload = response_data, - .value_payload = value_response_data}; - - return response; - }; -} diff --git a/src/plugin/plugin-bridge.cpp b/src/plugin/plugin-bridge.cpp index 0c0c3003..3217f75a 100644 --- a/src/plugin/plugin-bridge.cpp +++ b/src/plugin/plugin-bridge.cpp @@ -21,7 +21,6 @@ #include #include "../common/communication.h" -#include "../common/events.h" #include "../common/utils.h" #include "utils.h" diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 7d5ad91f..35e59c49 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -21,7 +21,6 @@ #include #include "../../common/communication.h" -#include "../../common/events.h" /** * A function pointer to what should be the entry point of a VST plugin. From 74c3cab0465d5b44574f44433b0b021c87500182 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 25 Oct 2020 23:08:11 +0100 Subject: [PATCH 05/40] Move event handling logic to a dedicated class Now all pieces are in place to allow handling events over multiple socket connections. --- docs/architecture.md | 40 +- src/common/communication.cpp | 161 ++++---- src/common/communication.h | 683 ++++++++++++++++++--------------- src/plugin/host-process.cpp | 2 - src/plugin/plugin-bridge.cpp | 76 ++-- src/plugin/plugin-bridge.h | 16 +- src/wine-host/bridges/vst2.cpp | 138 +++---- src/wine-host/bridges/vst2.h | 9 +- 8 files changed, 581 insertions(+), 544 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index cb8c6496..6be60502 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -107,25 +107,27 @@ as the _Windows VST plugin_. The whole process works as follows: are located in `src/common/communication.h`. The actual binary serialization is handled using [bitsery](https://github.com/fraillt/bitsery). - Actually sending and receiving the events happens in the `send_event()` and - `receive_event()` functions. When calling either `dispatch()` or - `audioMaster()`, the caller will oftentimes either pass along some kind of - data structure through the void pointer function argument, or they expect the - function's return value to be a pointer to some kind of struct provided by - the plugin or host. The behaviour for reading from and writing into these - void pointers and returning pointers to objects when needed is encapsulated - in the `DispatchDataConverter` and `HostCallbackDataCovnerter` classes for - the `dispatcher()` and `audioMaster()` functions respectively. For operations - involving the plugin editor there is also some extra glue in - `Vst2Bridge::dispatch_wrapper`. On the receiving end of the function calls, - the `passthrough_event()` function which calls the callback functions and - handles the marshalling between our data types created by the - `*DataConverter` classes and the VST API's different pointer types. This - behaviour is separated from `receive_event()` so we can handle MIDI events - separately. This is needed because a select few plugins only store pointers - to the received events rather than copies of the objects. Because of this, - the received event data must live at least until the next audio buffer gets - processed so it needs to be stored temporarily. + TODO: Rewrite this after the socket changes are done + + Actually sending and receiving the events happens in the + `EventHandler::send()` and `EventHandler::receive()` functions. When calling + either `dispatch()` or `audioMaster()`, the caller will oftentimes either + pass along some kind of data structure through the void pointer function + argument, or they expect the function's return value to be a pointer to some + kind of struct provided by the plugin or host. The behaviour for reading from + and writing into these void pointers and returning pointers to objects when + needed is encapsulated in the `DispatchDataConverter` and + `HostCallbackDataCovnerter` classes for the `dispatcher()` and + `audioMaster()` functions respectively. For operations involving the plugin + editor there is also some extra glue in `Vst2Bridge::dispatch_wrapper`. On + the receiving end of the function calls, the `passthrough_event()` function + which calls the callback functions and handles the marshalling between our + data types created by the `*DataConverter` classes and the VST API's + different pointer types. This behaviour is separated from `receive_event()` + so we can handle MIDI events separately. This is needed because a select few + plugins only store pointers to the received events rather than copies of the + objects. Because of this, the received event data must live at least until + the next audio buffer gets processed so it needs to be stored temporarily. 6. The Wine VST host loads the Windows VST plugin and starts forwarding messages over the sockets described above. diff --git a/src/common/communication.cpp b/src/common/communication.cpp index 4aebeeb4..8b0a8a2a 100644 --- a/src/common/communication.cpp +++ b/src/common/communication.cpp @@ -28,79 +28,6 @@ namespace fs = boost::filesystem; constexpr char alphanumeric_characters[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; -Sockets::Sockets(boost::asio::io_context& io_context, - const boost::filesystem::path& endpoint_base_dir, - bool listen) - : base_dir(endpoint_base_dir), - io_context(io_context), - host_vst_dispatch(io_context), - host_vst_dispatch_midi_events(io_context), - vst_host_callback(io_context), - host_vst_parameters(io_context), - host_vst_process_replacing(io_context), - host_vst_control(io_context), - host_vst_dispatch_endpoint( - (base_dir / "host_vst_dispatch.sock").string()), - host_vst_dispatch_midi_events_endpoint( - (base_dir / "host_vst_dispatch_midi_events.sock").string()), - vst_host_callback_endpoint( - (base_dir / "vst_host_callback.sock").string()), - host_vst_parameters_endpoint( - (base_dir / "host_vst_parameters.sock").string()), - host_vst_process_replacing_endpoint( - (base_dir / "host_vst_process_replacing.sock").string()), - host_vst_control_endpoint((base_dir / "host_vst_control.sock").string()) { - if (listen) { - fs::create_directory(base_dir); - - acceptors = Acceptors{ - .host_vst_dispatch{io_context, host_vst_dispatch_endpoint}, - .host_vst_dispatch_midi_events{ - io_context, host_vst_dispatch_midi_events_endpoint}, - .vst_host_callback{io_context, vst_host_callback_endpoint}, - .host_vst_parameters{io_context, host_vst_parameters_endpoint}, - .host_vst_process_replacing{io_context, - host_vst_process_replacing_endpoint}, - .host_vst_control{io_context, host_vst_control_endpoint}, - }; - } -} - -Sockets::~Sockets() { - // Only clean if we're the ones who have created these files, although it - // should not cause any harm to also do this on the Wine side - if (acceptors) { - try { - fs::remove_all(base_dir); - } catch (const fs::filesystem_error&) { - // There should not be any filesystem errors since only one side - // removes the files, but if we somehow can't delete the file then - // we can just silently ignore this - } - } -} - -void Sockets::connect() { - if (acceptors) { - acceptors->host_vst_dispatch.accept(host_vst_dispatch); - acceptors->host_vst_dispatch_midi_events.accept( - host_vst_dispatch_midi_events); - acceptors->vst_host_callback.accept(vst_host_callback); - acceptors->host_vst_parameters.accept(host_vst_parameters); - acceptors->host_vst_process_replacing.accept( - host_vst_process_replacing); - acceptors->host_vst_control.accept(host_vst_control); - } else { - host_vst_dispatch.connect(host_vst_dispatch_endpoint); - host_vst_dispatch_midi_events.connect( - host_vst_dispatch_midi_events_endpoint); - vst_host_callback.connect(vst_host_callback_endpoint); - host_vst_parameters.connect(host_vst_parameters_endpoint); - host_vst_process_replacing.connect(host_vst_process_replacing_endpoint); - host_vst_control.connect(host_vst_control_endpoint); - } -} - EventPayload DefaultDataConverter::read(const int /*opcode*/, const int /*index*/, const intptr_t /*value*/, @@ -154,6 +81,94 @@ intptr_t DefaultDataConverter::return_value(const int /*opcode*/, return original; } +EventHandler::EventHandler( + boost::asio::io_context& io_context, + boost::asio::local::stream_protocol::endpoint endpoint, + bool listen) + : endpoint(endpoint), socket(io_context) { + if (listen) { + fs::create_directories(fs::path(endpoint.path()).parent_path()); + acceptor.emplace(io_context, endpoint); + } +} + +void EventHandler::connect() { + if (acceptor) { + acceptor->accept(socket); + } else { + socket.connect(endpoint); + } +} + +void EventHandler::close() { + socket.shutdown(boost::asio::local::stream_protocol::socket::shutdown_both); + socket.close(); +} + +Sockets::Sockets(boost::asio::io_context& io_context, + const boost::filesystem::path& endpoint_base_dir, + bool listen) + : base_dir(endpoint_base_dir), + host_vst_dispatch(io_context, + (base_dir / "host_vst_dispatch.sock").string(), + listen), + host_vst_dispatch_midi_events( + io_context, + (base_dir / "host_vst_dispatch_midi_events.sock").string(), + listen), + vst_host_callback(io_context, + (base_dir / "vst_host_callback.sock").string(), + listen), + host_vst_parameters(io_context), + host_vst_process_replacing(io_context), + host_vst_control(io_context), + host_vst_parameters_endpoint( + (base_dir / "host_vst_parameters.sock").string()), + host_vst_process_replacing_endpoint( + (base_dir / "host_vst_process_replacing.sock").string()), + host_vst_control_endpoint((base_dir / "host_vst_control.sock").string()) { + if (listen) { + fs::create_directories(base_dir); + + acceptors = Acceptors{ + .host_vst_parameters{io_context, host_vst_parameters_endpoint}, + .host_vst_process_replacing{io_context, + host_vst_process_replacing_endpoint}, + .host_vst_control{io_context, host_vst_control_endpoint}, + }; + } +} + +Sockets::~Sockets() { + // Only clean if we're the ones who have created these files, although it + // should not cause any harm to also do this on the Wine side + if (acceptors) { + try { + fs::remove_all(base_dir); + } catch (const fs::filesystem_error&) { + // There should not be any filesystem errors since only one side + // removes the files, but if we somehow can't delete the file then + // we can just silently ignore this + } + } +} + +void Sockets::connect() { + host_vst_dispatch.connect(); + host_vst_dispatch_midi_events.connect(); + vst_host_callback.connect(); + if (acceptors) { + acceptors->host_vst_parameters.accept(host_vst_parameters); + acceptors->host_vst_process_replacing.accept( + host_vst_process_replacing); + acceptors->host_vst_control.accept(host_vst_control); + } else { + host_vst_parameters.connect(host_vst_parameters_endpoint); + host_vst_process_replacing.connect(host_vst_process_replacing_endpoint); + host_vst_control.connect(host_vst_control_endpoint); + } +} + boost::filesystem::path generate_endpoint_base(const std::string& plugin_name) { fs::path temp_directory = get_temporary_directory(); diff --git a/src/common/communication.h b/src/common/communication.h index 6df4ca16..6e147af8 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -36,206 +36,6 @@ using OutputAdapter = bitsery::OutputBufferAdapter; template using InputAdapter = bitsery::InputBufferAdapter; -/** - * Manages all the sockets used for communicating between the plugin and the - * Wine host. Every plugin will get its own directory (the socket endpoint base - * directory), and all socket endpoints are created within this directory. This - * is usually `/run/user//yabridge--/`. - * - * On the plugin side this class should be initialized with `listen` set to - * `true` before launching the Wine VST host. This will start listening on the - * sockets, and the call to `connect()` will then accept any incoming - * connections. - */ -class Sockets { - public: - /** - * Sets up the sockets using the specified base directory. The sockets won't - * be active until `connect()` gets called. - * - * @param io_context The IO context the sockets should be bound to. Relevant - * when doing asynchronous operations. - * @param endpoint_base_dir The base directory that will be used for the - * Unix domain sockets. - * @param listen If `true`, start listening on the sockets. Incoming - * connections will be accepted when `connect()` gets called. This should - * be set to `true` on the plugin side, and `false` on the Wine host side. - * - * @see Sockets::connect - */ - Sockets(boost::asio::io_context& io_context, - const boost::filesystem::path& endpoint_base_dir, - bool listen); - - /** - * Cleans up the directory containing the socket endpoints when yabridge - * shuts down if it still exists. - */ - ~Sockets(); - - /** - * Depending on the value of the `listen` argument passed to the - * constructor, either accept connections made to the sockets on the Linux - * side or connect to the sockets on the Wine side - */ - void connect(); - - /** - * The base directory for our socket endpoints. All `*_endpoint` variables - * below are files within this directory. - */ - const boost::filesystem::path base_dir; - - /** - * The IO context that the sockets will be created on. This is only relevant - * for asynchronous operations. - */ - boost::asio::io_context& io_context; - - // The naming convention for these sockets is `__`. For - // instance the socket named `host_vst_dispatch` forwards - // `AEffect.dispatch()` calls from the native VST host to the Windows VST - // plugin (through the Wine VST host). - - /** - * The socket that forwards all `dispatcher()` calls from the VST host to - * the plugin. - */ - boost::asio::local::stream_protocol::socket host_vst_dispatch; - /** - * Used specifically for the `effProcessEvents` opcode. This is needed - * because the Win32 API is designed to block during certain GUI - * interactions such as resizing a window or opening a dropdown. Without - * this MIDI input would just stop working at times. - */ - boost::asio::local::stream_protocol::socket host_vst_dispatch_midi_events; - /** - * The socket that forwards all `audioMaster()` calls from the Windows VST - * plugin to the host. - */ - boost::asio::local::stream_protocol::socket vst_host_callback; - /** - * Used for both `getParameter` and `setParameter` since they mostly - * overlap. - */ - boost::asio::local::stream_protocol::socket host_vst_parameters; - /** - * Used for processing audio usign the `process()`, `processReplacing()` and - * `processDoubleReplacing()` functions. - */ - boost::asio::local::stream_protocol::socket host_vst_process_replacing; - /** - * A control socket that sends data that is not suitable for the other - * sockets. At the moment this is only used to, on startup, send the Windows - * VST plugin's `AEffect` object to the native VST plugin, and to then send - * the configuration (from `config`) back to the Wine host. - */ - boost::asio::local::stream_protocol::socket host_vst_control; - - private: - const boost::asio::local::stream_protocol::endpoint - host_vst_dispatch_endpoint; - const boost::asio::local::stream_protocol::endpoint - host_vst_dispatch_midi_events_endpoint; - const boost::asio::local::stream_protocol::endpoint - vst_host_callback_endpoint; - const boost::asio::local::stream_protocol::endpoint - host_vst_parameters_endpoint; - const boost::asio::local::stream_protocol::endpoint - host_vst_process_replacing_endpoint; - const boost::asio::local::stream_protocol::endpoint - host_vst_control_endpoint; - - /** - * All of our socket acceptors. We have to create these before launching the - * Wine process. - */ - struct Acceptors { - boost::asio::local::stream_protocol::acceptor host_vst_dispatch; - boost::asio::local::stream_protocol::acceptor - host_vst_dispatch_midi_events; - boost::asio::local::stream_protocol::acceptor vst_host_callback; - boost::asio::local::stream_protocol::acceptor host_vst_parameters; - boost::asio::local::stream_protocol::acceptor - host_vst_process_replacing; - boost::asio::local::stream_protocol::acceptor host_vst_control; - }; - - /** - * If the `listen` constructor argument was set to `true`, when we'll - * prepare a set of socket acceptors that listen on the socket endpoints. - */ - std::optional acceptors; -}; - -/** - * Encodes the base behavior for reading from and writing to the `data` argument - * for event dispatch functions. This provides base functionality for these - * kinds of events. The `dispatch()` function will require some more specific - * structs. - */ -class DefaultDataConverter { - public: - virtual ~DefaultDataConverter(){}; - - /** - * Read data from the `data` void pointer into a an `EventPayload` value - * that can be serialized and conveys the meaning of the event. - */ - virtual EventPayload read(const int opcode, - const int index, - const intptr_t value, - const void* data) const; - - /** - * Read data from the `value` pointer into a an `EventPayload` value that - * can be serialized and conveys the meaning of the event. This is only used - * for the `effSetSpeakerArrangement` and `effGetSpeakerArrangement` events. - */ - virtual std::optional read_value(const int opcode, - const intptr_t value) const; - - /** - * Write the reponse back to the `data` pointer. - */ - virtual void write(const int opcode, - void* data, - const EventResult& response) const; - - /** - * Write the reponse back to the `value` pointer. This is only used during - * the `effGetSpeakerArrangement` event. - */ - virtual void write_value(const int opcode, - intptr_t value, - const EventResult& response) const; - - /** - * This function can override a callback's return value based on the opcode. - * This is used in one place to return a pointer to a `VstTime` object - * that's contantly being updated. - * - * @param opcode The opcode for the current event. - * @param original The original return value as returned by the callback - * function. - */ - virtual intptr_t return_value(const int opcode, - const intptr_t original) const; -}; - -/** - * Generate a unique base directory that can be used as a prefix for all Unix - * domain socket endpoints used in `PluginBridge`/`Vst2Bridge`. This will - * usually return `/run/user//yabridge--/`. - * - * Sockets for group hosts are handled separately. See - * `../plugin/utils.h:generate_group_endpoint` for more information on those. - * - * @param plugin_name The name of the plugin we're generating endpoints for. - * Used as a visual indication of what plugin is using this endpoint. - */ -boost::filesystem::path generate_endpoint_base(const std::string& plugin_name); - /** * Serialize an object using bitsery and write it to a socket. This will write * both the size of the serialized object and the object itself over the socket. @@ -320,126 +120,395 @@ inline T read_object(Socket& socket, } /** - * Serialize and send an event over a socket. This is used for both the host -> - * plugin 'dispatch' events and the plugin -> host 'audioMaster' host callbacks - * since they follow the same format. See one of those functions for details on - * 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 `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 - * threads at once. - * @param data_converter Some struct that knows how to read data from and write - * data back to the `data` void pointer. For host callbacks this parameter - * contains either a string or a null pointer while `dispatch()` calls might - * contain opcode specific structs. See the documentation for `EventPayload` - * for more information. The `DefaultDataConverter` defined above handles the - * basic behavior that's sufficient for host callbacks. - * @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. - * - * @relates receive_event - * @relates passthrough_event + * Encodes the base behavior for reading from and writing to the `data` argument + * for event dispatch functions. This provides base functionality for these + * kinds of events. The `dispatch()` function will require some more specific + * structs. */ -template -intptr_t send_event(boost::asio::local::stream_protocol::socket& socket, - std::mutex& write_mutex, - D& data_converter, - std::optional> logging, - int opcode, - int index, - intptr_t value, - void* data, - float option) { - // Encode the right payload types for this event. Check the documentation - // for `EventPayload` for more information. These types are converted to - // C-style data structures in `passthrough_event()` so they can be passed to - // a plugin or callback function. - const EventPayload payload = - data_converter.read(opcode, index, value, data); - const std::optional value_payload = - data_converter.read_value(opcode, value); +class DefaultDataConverter { + public: + virtual ~DefaultDataConverter(){}; - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event(is_dispatch, opcode, index, value, payload, option, - value_payload); - } + /** + * Read data from the `data` void pointer into a an `EventPayload` value + * that can be serialized and conveys the meaning of the event. + */ + virtual EventPayload read(const int opcode, + const int index, + const intptr_t value, + const void* data) const; - const Event event{.opcode = opcode, - .index = index, - .value = value, - .option = option, - .payload = payload, - .value_payload = value_payload}; + /** + * Read data from the `value` pointer into a an `EventPayload` value that + * can be serialized and conveys the meaning of the event. This is only used + * for the `effSetSpeakerArrangement` and `effGetSpeakerArrangement` events. + */ + virtual std::optional read_value(const int opcode, + const intptr_t value) const; - // Prevent two threads from writing over the socket at the same time and - // messages getting out of order. This is needed because we can't prevent - // the plugin or the host from calling `dispatch()` or `audioMaster()` from - // multiple threads. - EventResult response; - { - std::lock_guard lock(write_mutex); - write_object(socket, event); - response = read_object(socket); - } + /** + * Write the reponse back to the `data` pointer. + */ + virtual void write(const int opcode, + void* data, + const EventResult& response) const; - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event_response(is_dispatch, opcode, response.return_value, - response.payload, response.value_payload); - } + /** + * Write the reponse back to the `value` pointer. This is only used during + * the `effGetSpeakerArrangement` event. + */ + virtual void write_value(const int opcode, + intptr_t value, + const EventResult& response) const; - data_converter.write(opcode, data, response); - data_converter.write_value(opcode, value, response); - - return data_converter.return_value(opcode, response.return_value); -} + /** + * This function can override a callback's return value based on the opcode. + * This is used in one place to return a pointer to a `VstTime` object + * that's contantly being updated. + * + * @param opcode The opcode for the current event. + * @param original The original return value as returned by the callback + * function. + */ + virtual intptr_t return_value(const int opcode, + const intptr_t original) const; +}; /** - * 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. + * For most of our sockets we can just send out our messages on the writing + * side, and do a simple blocking loop on the reading side. The `dispatch()` and + * `audioMaster()` calls are different. Not only do they have they come with + * complex payload values, they can also be called simultaneously from multiple + * threads, and `audioMaster()` and `dispatch()` calls can even be mutually + * recursive. Luckily this does not happen very often, but it does mean that our + * simple 'one-socket-per-function' model doesn't work anymore. Because setting + * up new sockets is quite expensive and this is seldom needed, this works + * slightly differently: * - * @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 callback The function used to generate a response out of an event. + * - We'll keep a single long lived socket connection. This works the exact same + * way as every other socket defined in the `Sockets` class. + * - Aside from that the listening side will have a second thread asynchronously + * listening for new connections on the socket endpoint. * - * @tparam F A function type in the form of `EventResponse(Event)`. - * - * @relates send_event - * @relates passthrough_event + * The `EventHandler::send()` is used to send events. If the socket is currently + * being written to, we'll first create a new socket connection as described + * above. Similarly, the `EventHandler::receive()` method first sets up + * asynchronous listeners for the socket endpoint, and then block and handle + * events until the main socket is closed. */ -template -void receive_event(boost::asio::local::stream_protocol::socket& socket, - std::optional> logging, - F callback) { - auto event = read_object(socket); - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event(is_dispatch, event.opcode, event.index, event.value, - event.payload, event.option, event.value_payload); +class EventHandler { + public: + /** + * Sets up a single main socket for this type of events. The sockets won't + * be active until `connect()` gets called. + * + * @param io_context The IO context the sockets should be bound to. Relevant + * when doing asynchronous operations. + * @param endpoint The socket endpoint used for this event handler. + * @param listen If `true`, start listening on the sockets. Incoming + * connections will be accepted when `connect()` gets called. This should + * be set to `true` on the plugin side, and `false` on the Wine host side. + * + * @see Sockets::connect + */ + EventHandler(boost::asio::io_context& io_context, + boost::asio::local::stream_protocol::endpoint endpoint, + bool listen); + + /** + * Depending on the value of the `listen` argument passed to the + * constructor, either accept connections made to the sockets on the Linux + * side or connect to the sockets on the Wine side + */ + void connect(); + + /** + * Close the socket. Both sides that are actively listening will be thrown a + * `boost::system_error` when this happens. + */ + void close(); + + /** + * Serialize and send an event over a socket. This is used for both the host + * -> plugin 'dispatch' events and the plugin -> host 'audioMaster' host + * callbacks since they follow the same format. See one of those functions + * for details on the parameters and return value of this function. + * + * As described above, if this function is currently being called from + * another thread, then this will create a new socket connection and send + * the event there instead. + * + * @param data_converter Some struct that knows how to read data from and + * write data back to the `data` void pointer. For host callbacks this + * parameter contains either a string or a null pointer while `dispatch()` + * calls might contain opcode specific structs. See the documentation for + * `EventPayload` for more information. The `DefaultDataConverter` defined + * above handles the basic behavior that's sufficient for host callbacks. + * @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. + * + * @relates EventHandler::receive + * @relates passthrough_event + */ + template + intptr_t send(D& data_converter, + std::optional> logging, + int opcode, + int index, + intptr_t value, + void* data, + float option) { + // TODO: Create a new socket if the mutex is locked + + // Encode the right payload types for this event. Check the + // documentation for `EventPayload` for more information. These types + // are converted to C-style data structures in `passthrough_event()` so + // they can be passed to a plugin or callback function. + const EventPayload payload = + data_converter.read(opcode, index, value, data); + const std::optional value_payload = + data_converter.read_value(opcode, value); + + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event(is_dispatch, opcode, index, value, payload, option, + value_payload); + } + + const Event event{.opcode = opcode, + .index = index, + .value = value, + .option = option, + .payload = payload, + .value_payload = value_payload}; + + // Prevent two threads from writing over the socket at the same time and + // messages getting out of order. This is needed because we can't + // prevent the plugin or the host from calling `dispatch()` or + // `audioMaster()` from multiple threads. + EventResult response; + { + std::lock_guard lock(write_mutex); + write_object(socket, event); + response = read_object(socket); + } + + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event_response(is_dispatch, opcode, + response.return_value, response.payload, + response.value_payload); + } + + data_converter.write(opcode, data, response); + data_converter.write_value(opcode, value, response); + + return data_converter.return_value(opcode, response.return_value); } - EventResult response = callback(event); - if (logging) { - auto [logger, is_dispatch] = *logging; - logger.log_event_response(is_dispatch, event.opcode, - response.return_value, response.payload, - response.value_payload); + /** + * Spawn a new thread to listen for extra connections to `endpoint`, and + * then a blocking loop that handles events from the primary `socket`. + * + * The specified function will be used to create an `EventResult` from an + * `Event`. This is almost always a wrapper around `passthrough_event()`, + * which converts the `EventPayload` into a format used by VST2, calls + * either `dispatch()` or `audioMaster()` depending on the socket, and then + * serializes the result back into an `EventResultPayload`. + * + * This function will also be used separately for receiving MIDI data, as + * some plugins will need pointers to received MIDI data to stay alive until + * the next audio buffer gets processed. + * + * @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 callback The function used to generate a response out of an event. + * + * @tparam F A function type in the form of `EventResponse(Event)`. + * + * @relates EventHandler::send + * @relates passthrough_event + */ + template + void receive(std::optional> logging, F callback) { + // TODO: Listen for new incoming connections + + while (true) { + try { + auto event = read_object(socket); + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event(is_dispatch, event.opcode, event.index, + event.value, event.payload, event.option, + event.value_payload); + } + + EventResult response = callback(event); + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event_response( + is_dispatch, event.opcode, response.return_value, + response.payload, response.value_payload); + } + + write_object(socket, response); + } catch (const boost::system::system_error&) { + // This happens when the sockets got closed because the plugin + // is being shut down + break; + } + } } - write_object(socket, response); -} + private: + boost::asio::local::stream_protocol::endpoint endpoint; + boost::asio::local::stream_protocol::socket socket; + + /** + * This acceptor will be used once synchronously on the listening side + * during `Sockets::connect()`. When `EventHandler::receive()` is called + * this will be replaced by a new acceptor bound to a new IO context to + * receive any additional incoming connections. + */ + std::optional acceptor; + + /** + * A mutex that locks the main `socket`. If this is locked, then any new + * events will be sent over a new socket instead. + */ + std::mutex write_mutex; +}; + +/** + * Manages all the sockets used for communicating between the plugin and the + * Wine host. Every plugin will get its own directory (the socket endpoint base + * directory), and all socket endpoints are created within this directory. This + * is usually `/run/user//yabridge--/`. + * + * On the plugin side this class should be initialized with `listen` set to + * `true` before launching the Wine VST host. This will start listening on the + * sockets, and the call to `connect()` will then accept any incoming + * connections. + */ +class Sockets { + public: + /** + * Sets up the sockets using the specified base directory. The sockets won't + * be active until `connect()` gets called. + * + * @param io_context The IO context the sockets should be bound to. Relevant + * when doing asynchronous operations. + * @param endpoint_base_dir The base directory that will be used for the + * Unix domain sockets. + * @param listen If `true`, start listening on the sockets. Incoming + * connections will be accepted when `connect()` gets called. This should + * be set to `true` on the plugin side, and `false` on the Wine host side. + * + * @see Sockets::connect + */ + Sockets(boost::asio::io_context& io_context, + const boost::filesystem::path& endpoint_base_dir, + bool listen); + + /** + * Cleans up the directory containing the socket endpoints when yabridge + * shuts down if it still exists. + */ + ~Sockets(); + + /** + * Depending on the value of the `listen` argument passed to the + * constructor, either accept connections made to the sockets on the Linux + * side or connect to the sockets on the Wine side + */ + void connect(); + + /** + * The base directory for our socket endpoints. All `*_endpoint` variables + * below are files within this directory. + */ + const boost::filesystem::path base_dir; + + // The naming convention for these sockets is `__`. For + // instance the socket named `host_vst_dispatch` forwards + // `AEffect.dispatch()` calls from the native VST host to the Windows VST + // plugin (through the Wine VST host). + + /** + * The socket that forwards all `dispatcher()` calls from the VST host to + * the plugin. + */ + EventHandler host_vst_dispatch; + /** + * Used specifically for the `effProcessEvents` opcode. This is needed + * because the Win32 API is designed to block during certain GUI + * interactions such as resizing a window or opening a dropdown. Without + * this MIDI input would just stop working at times. + */ + EventHandler host_vst_dispatch_midi_events; + /** + * The socket that forwards all `audioMaster()` calls from the Windows VST + * plugin to the host. + */ + EventHandler vst_host_callback; + /** + * Used for both `getParameter` and `setParameter` since they mostly + * overlap. + */ + boost::asio::local::stream_protocol::socket host_vst_parameters; + /** + * Used for processing audio usign the `process()`, `processReplacing()` and + * `processDoubleReplacing()` functions. + */ + boost::asio::local::stream_protocol::socket host_vst_process_replacing; + /** + * A control socket that sends data that is not suitable for the other + * sockets. At the moment this is only used to, on startup, send the Windows + * VST plugin's `AEffect` object to the native VST plugin, and to then send + * the configuration (from `config`) back to the Wine host. + */ + boost::asio::local::stream_protocol::socket host_vst_control; + + private: + const boost::asio::local::stream_protocol::endpoint + host_vst_parameters_endpoint; + const boost::asio::local::stream_protocol::endpoint + host_vst_process_replacing_endpoint; + const boost::asio::local::stream_protocol::endpoint + host_vst_control_endpoint; + + /** + * All of our socket acceptors. We have to create these before launching the + * Wine process. + */ + struct Acceptors { + boost::asio::local::stream_protocol::acceptor host_vst_parameters; + boost::asio::local::stream_protocol::acceptor + host_vst_process_replacing; + boost::asio::local::stream_protocol::acceptor host_vst_control; + }; + + /** + * If the `listen` constructor argument was set to `true`, when we'll + * prepare a set of socket acceptors that listen on the socket endpoints. + */ + std::optional acceptors; +}; + +/** + * Generate a unique base directory that can be used as a prefix for all Unix + * domain socket endpoints used in `PluginBridge`/`Vst2Bridge`. This will + * usually return `/run/user//yabridge--/`. + * + * Sockets for group hosts are handled separately. See + * `../plugin/utils.h:generate_group_endpoint` for more information on those. + * + * @param plugin_name The name of the plugin we're generating endpoints for. + * Used as a visual indication of what plugin is using this endpoint. + */ +boost::filesystem::path generate_endpoint_base(const std::string& plugin_name); /** * Create a callback function that takes an `Event` object, decodes the data @@ -460,9 +529,9 @@ void receive_event(boost::asio::local::stream_protocol::socket& socket, * `audioMasterCallback`. * * @return A `EventResult(Event)` callback function that can be passed to - * `receive_event`. + * `EditorHandler::receive()`. * - * @relates receive_event + * @relates EditorHandler::receive */ template auto passthrough_event(AEffect* plugin, F callback) { diff --git a/src/plugin/host-process.cpp b/src/plugin/host-process.cpp index 0473abb3..48ae621c 100644 --- a/src/plugin/host-process.cpp +++ b/src/plugin/host-process.cpp @@ -263,7 +263,5 @@ void GroupHost::terminate() { // There's no need to manually terminate group host processes as they will // shut down automatically after all plugins have exited. Manually closing // the dispatch socket will cause the associated plugin to exit. - sockets.host_vst_dispatch.shutdown( - boost::asio::local::stream_protocol::socket::shutdown_both); sockets.host_vst_dispatch.close(); } diff --git a/src/plugin/plugin-bridge.cpp b/src/plugin/plugin-bridge.cpp index 3217f75a..3ddc2867 100644 --- a/src/plugin/plugin-bridge.cpp +++ b/src/plugin/plugin-bridge.cpp @@ -121,41 +121,31 @@ PluginBridge::PluginBridge(audioMasterCallback host_callback) // instead of asynchronous IO since communication has to be handled in // lockstep anyway host_callback_handler = std::jthread([&]() { - while (true) { - try { - // TODO: Think of a nicer way to structure this and the similar - // handler in `Vst2Bridge::handle_dispatch_midi_events` - receive_event( - sockets.vst_host_callback, - std::pair(logger, false), [&](Event& event) { - // MIDI events sent from the plugin back to the host are - // a special case here. They have to sent during the - // `processReplacing()` function or else the host will - // ignore them. Because of this we'll temporarily save - // any MIDI events we receive here, and then we'll - // actually send them to the host at the end of the - // `process_replacing()` function. - if (event.opcode == audioMasterProcessEvents) { - std::lock_guard lock(incoming_midi_events_mutex); + // TODO: Think of a nicer way to structure this and the similar + // handler in `Vst2Bridge::handle_dispatch_midi_events` + sockets.vst_host_callback.receive( + std::pair(logger, false), [&](Event& event) { + // MIDI events sent from the plugin back to the host are a + // special case here. They have to sent during the + // `processReplacing()` function or else the host will ignore + // them. Because of this we'll temporarily save any MIDI events + // we receive here, and then we'll actually send them to the + // host at the end of the `process_replacing()` function. + if (event.opcode == audioMasterProcessEvents) { + std::lock_guard lock(incoming_midi_events_mutex); - incoming_midi_events.push_back( - std::get(event.payload)); - EventResult response{.return_value = 1, - .payload = nullptr, - .value_payload = std::nullopt}; + incoming_midi_events.push_back( + std::get(event.payload)); + EventResult response{.return_value = 1, + .payload = nullptr, + .value_payload = std::nullopt}; - return response; - } else { - return passthrough_event( - &plugin, host_callback_function)(event); - } - }); - } catch (const boost::system::system_error&) { - // This happens when the sockets got closed because the plugin - // is being shut down - break; - } - } + return response; + } else { + return passthrough_event(&plugin, + host_callback_function)(event); + } + }); }); // Read the plugin's information from the Wine process. This can only be @@ -435,10 +425,9 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, intptr_t return_value = 0; try { // TODO: Add some kind of timeout? - return_value = send_event( - sockets.host_vst_dispatch, dispatch_mutex, converter, - std::pair(logger, true), opcode, index, - value, data, option); + return_value = sockets.host_vst_dispatch.send( + converter, std::pair(logger, true), opcode, + index, value, data, option); } catch (const boost::system::system_error& a) { // Thrown when the socket gets closed because the VST plugin // loaded into the Wine process crashed during shutdown @@ -461,10 +450,9 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, // thread and socket to pass MIDI events. Otherwise plugins will // stop receiving MIDI data when they have an open dropdowns or // message box. - return send_event(sockets.host_vst_dispatch_midi_events, - dispatch_midi_events_mutex, converter, - std::pair(logger, true), opcode, - index, value, data, option); + return sockets.host_vst_dispatch_midi_events.send( + converter, std::pair(logger, true), opcode, + index, value, data, option); break; case effCanDo: { const std::string query(static_cast(data)); @@ -521,9 +509,9 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, // and loading plugin state it's much better to have bitsery or our // receiving function temporarily allocate a large enough buffer rather than // to have a bunch of allocated memory sitting around doing nothing. - return send_event(sockets.host_vst_dispatch, dispatch_mutex, converter, - std::pair(logger, true), opcode, index, - value, data, option); + return sockets.host_vst_dispatch.send( + converter, std::pair(logger, true), opcode, index, value, + data, option); } template diff --git a/src/plugin/plugin-bridge.h b/src/plugin/plugin-bridge.h index ad0d62a2..e62fc750 100644 --- a/src/plugin/plugin-bridge.h +++ b/src/plugin/plugin-bridge.h @@ -23,9 +23,9 @@ #include #include +#include "../common/communication.h" #include "../common/configuration.h" #include "../common/logging.h" -#include "../common/communication.h" #include "host-process.h" /** @@ -132,16 +132,10 @@ class PluginBridge { std::jthread host_callback_handler; /** - * A binary semaphore to prevent race conditions from the dispatch function - * being called by two threads at once. See `send_event()` for more - * information. - */ - std::mutex dispatch_mutex; - std::mutex dispatch_midi_events_mutex; - /** - * A similar semaphore as the `dispatch_*` semaphores in the rare case that - * `getParameter()` and `setParameter()` are being called at the same time - * since they use the same socket. + * A mutex to prevent multiple simultaneous calls to `getParameter()` and + * `setParameter()`. This likely won't happen, but better safe than sorry. + * For `dispatch()` and `audioMaster()` there's some more complex logic for + * this in `EventHandler`. */ std::mutex parameters_mutex; diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 35e59c49..93adef5b 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -144,96 +144,74 @@ bool Vst2Bridge::should_skip_message_loop() const { } void Vst2Bridge::handle_dispatch() { - while (true) { - try { - receive_event( - sockets.host_vst_dispatch, std::nullopt, - passthrough_event( - plugin, - [&](AEffect* plugin, int opcode, int index, intptr_t value, - void* data, float option) -> intptr_t { - // Instead of running `plugin->dispatcher()` (or - // `dispatch_wrapper()`) directly, we'll run the - // function within the IO context so all events will be - // executed on the same thread as the one that runs the - // Win32 message loop - std::promise dispatch_result; - boost::asio::dispatch(io_context, [&]() { - const intptr_t result = dispatch_wrapper( - plugin, opcode, index, value, data, option); + sockets.host_vst_dispatch.receive( + std::nullopt, + passthrough_event( + plugin, + [&](AEffect* plugin, int opcode, int index, intptr_t value, + void* data, float option) -> intptr_t { + // Instead of running `plugin->dispatcher()` (or + // `dispatch_wrapper()`) directly, we'll run the function within + // the IO context so all events will be executed on the same + // thread as the one that runs the Win32 message loop + std::promise dispatch_result; + boost::asio::dispatch(io_context, [&]() { + const intptr_t result = dispatch_wrapper( + plugin, opcode, index, value, data, option); - dispatch_result.set_value(result); - }); + dispatch_result.set_value(result); + }); - // The message loop and X11 event handling will be run - // separately on a timer - return dispatch_result.get_future().get(); - })); - } catch (const boost::system::system_error&) { - // The plugin has cut off communications, so we can shut down this - // host application - break; - } - } + // The message loop and X11 event handling will be run + // separately on a timer + return dispatch_result.get_future().get(); + })); } void Vst2Bridge::handle_dispatch_midi_events() { - while (true) { - try { - receive_event( - sockets.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. 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); + sockets.host_vst_dispatch_midi_events.receive( + 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. We're not using + // `passthrough_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); - next_audio_buffer_midi_events.push_back( - std::get(event.payload)); - DynamicVstEvents& events = - next_audio_buffer_midi_events.back(); + next_audio_buffer_midi_events.push_back( + std::get(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); + // 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 = return_value, - .payload = nullptr, - .value_payload = std::nullopt}; + EventResult response{.return_value = return_value, + .payload = nullptr, + .value_payload = std::nullopt}; - return response; - } else { - using namespace std::placeholders; + return response; + } else { + using namespace std::placeholders; - std::cerr << "[Warning] Received non-MIDI " - "event on MIDI processing thread" - << std::endl; + std::cerr << "[Warning] Received non-MIDI " + "event on MIDI processing thread" + << std::endl; - // Maybe this should just be a hard error instead, since - // it should never happen - return passthrough_event( - plugin, - std::bind(&Vst2Bridge::dispatch_wrapper, this, _1, + // Maybe this should just be a hard error instead, since it + // should never happen + return passthrough_event( + plugin, std::bind(&Vst2Bridge::dispatch_wrapper, this, _1, _2, _3, _4, _5, _6))(event); - } - }); - } catch (const boost::system::system_error&) { - // The plugin has cut off communications, so we can shut down this - // host application - break; - } - } + } + }); } void Vst2Bridge::handle_parameters() { @@ -569,8 +547,8 @@ intptr_t Vst2Bridge::host_callback(AEffect* effect, } HostCallbackDataConverter converter(effect, time_info); - return send_event(sockets.vst_host_callback, host_callback_mutex, converter, - std::nullopt, opcode, index, value, data, option); + return sockets.vst_host_callback.send(converter, std::nullopt, opcode, + index, value, data, option); } intptr_t VST_CALL_CONV host_callback_proxy(AEffect* effect, diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 162907e4..38adc306 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -29,9 +29,9 @@ #include #include +#include "../../common/communication.h" #include "../../common/configuration.h" #include "../../common/logging.h" -#include "../../common/communication.h" #include "../editor.h" #include "../utils.h" @@ -210,13 +210,6 @@ class Vst2Bridge { */ Win32Thread process_replacing_handler; - /** - * A binary semaphore to prevent race conditions from the host callback - * function being called by two threads at once. See `send_event()` for more - * information. - */ - std::mutex host_callback_mutex; - /** * A scratch buffer for sending and receiving data during `process` and * `processReplacing` calls. From d82f8463d9a1cce1e6f58ba071ad81414befa3fb Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 11:51:13 +0100 Subject: [PATCH 06/40] Use simple numerical IDs for plugins in groups We'll be using a similar approach to identify threads for event handlers. --- src/common/serialization.cpp | 5 ----- src/common/serialization.h | 2 -- src/wine-host/bridges/group.cpp | 23 +++++++++++------------ src/wine-host/bridges/group.h | 31 ++++++++++++++++++++----------- src/wine-host/bridges/vst2.cpp | 3 ++- src/wine-host/bridges/vst2.h | 5 +++++ 6 files changed, 38 insertions(+), 31 deletions(-) diff --git a/src/common/serialization.cpp b/src/common/serialization.cpp index d49d480d..cb9662d3 100644 --- a/src/common/serialization.cpp +++ b/src/common/serialization.cpp @@ -108,8 +108,3 @@ AEffect& update_aeffect(AEffect& plugin, const AEffect& updated_plugin) { return plugin; } - -bool GroupRequest::operator==(const GroupRequest& rhs) const { - return plugin_path == rhs.plugin_path && - endpoint_base_dir == rhs.endpoint_base_dir; -} diff --git a/src/common/serialization.h b/src/common/serialization.h index c53612a5..18829b46 100644 --- a/src/common/serialization.h +++ b/src/common/serialization.h @@ -600,8 +600,6 @@ struct GroupRequest { std::string plugin_path; std::string endpoint_base_dir; - bool operator==(const GroupRequest& rhs) const; - template void serialize(S& s) { s.text1b(plugin_path, 4096); diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index b50fdbe4..1b0e9e49 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -111,15 +111,15 @@ GroupBridge::~GroupBridge() { stdio_context.stop(); } -void GroupBridge::handle_plugin_dispatch(const GroupRequest request) { +void GroupBridge::handle_plugin_dispatch(size_t plugin_id) { // At this point the `active_plugins` map will already contain the // intialized plugin's `Vst2Bridge` instance and this thread's handle - auto& [thread, bridge] = active_plugins.at(request); + auto& [thread, bridge] = active_plugins[plugin_id]; // Blocks this thread until the plugin shuts down, handling all events on // the main IO context bridge->handle_dispatch(); - logger.log("'" + request.plugin_path + "' has exited"); + logger.log("'" + bridge->vst_plugin_path.string() + "' has exited"); // After the plugin has exited we'll remove this thread's plugin from the // active plugins. This is done within the IO context because the call to @@ -127,11 +127,11 @@ void GroupBridge::handle_plugin_dispatch(const GroupRequest request) { // potentially corrupt our heap. This way we can also properly join the // thread again. If no active plugins remain, then we'll terminate the // process. - boost::asio::post(plugin_context, [&, request]() { + boost::asio::post(plugin_context, [this, plugin_id]() { std::lock_guard lock(active_plugins_mutex); // The join is implicit because we're using std::jthread - active_plugins.erase(request); + active_plugins.erase(plugin_id); }); // Defer actually shutting down the process to allow for fast plugin @@ -201,10 +201,6 @@ void GroupBridge::accept_requests() { const auto request = read_object(socket); write_object(socket, GroupResponse{boost::this_process::get_id()}); - // Collisions in the generated socket names should be very rare, but - // it could in theory happen - assert(!active_plugins.contains(request)); - // The plugin has to be initiated on the IO context's thread because // this has to be done on the same thread that's handling messages, // and all window messages have to be handled from the same thread. @@ -220,10 +216,13 @@ void GroupBridge::accept_requests() { // Start listening for dispatcher events sent to the plugin's // socket on another thread. The actual event handling will - // still occur within this IO context. - active_plugins[request] = + // still be posted to this IO context so that every plugin's + // primary event handling happens on the main thread. Since this + // is only used within this context we don't need any locks. + const size_t plugin_id = next_plugin_id.fetch_add(1); + active_plugins[plugin_id] = std::pair(std::jthread([&, request]() { - handle_plugin_dispatch(request); + handle_plugin_dispatch(plugin_id); }), std::move(bridge)); } catch (const std::runtime_error& error) { diff --git a/src/wine-host/bridges/group.h b/src/wine-host/bridges/group.h index c926cd9c..cf7690e8 100644 --- a/src/wine-host/bridges/group.h +++ b/src/wine-host/bridges/group.h @@ -22,6 +22,7 @@ #include #include +#include #include #include "vst2.h" @@ -132,21 +133,21 @@ class GroupBridge { * `accept_requests` since it has to be initiated inside of the IO context's * thread. * - * Once the plugin has exited, this thread will then remove itself from the - * `active_plugins` map. If this causes the vector to become empty, we will - * terminate this process. This check will be delayed by a few seconds to - * prevent having to constantly restart the group process during plugin - * scanning. + * Once the plugin has exited, this thread will then be joined to the main + * thread and removed from the `active_plugins` from the main IO context. If + * this causes the vector to become empty, we will terminate this process. + * This check is delayed by a few seconds to prevent having to constantly + * restart the group process during plugin scanning. * - * @param request Information about the plugin to launch, i.e. the path to - * the plugin and the path of the socket endpoint that will be used for - * communication. + * @param plugin_id The ID of this plugin in the `active_plugins` map. The + * thread can fetch the plugin's `Vst2Bridge` instance from that map using + * this identifier. * * @note In the case that the process starts but no plugin gets initiated, * then the process will never exit on its own. This should not happen * though. */ - void handle_plugin_dispatch(const GroupRequest request); + void handle_plugin_dispatch(size_t plugin_id); /** * Listen for new requests to spawn plugins within this process and handle @@ -257,11 +258,19 @@ class GroupBridge { * along with their plugin instance. After a plugin has exited or its * initialization has failed, the thread handling it will remove itself from * this map. This is to keep track of the amount of plugins currently - * running with their associated thread handles. + * running with their associated thread handles. The key that identifies the + * thread and plugin is a unique plugin ID obtained by doing a fetch-and-add + * on `next_plugin_id`. */ - std::unordered_map>> active_plugins; + /** + * A counter for the next unique plugin ID. When hosting a new plugin we'll + * do a fetch-and-add to ensure that every thread gets its own unique + * identifier. + */ + std::atomic_size_t next_plugin_id; /** * A mutex to prevent two threads from simultaneously accessing the plugins * map, and also to prevent `handle_plugin_dispatch()` from terminating the diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 93adef5b..846ae6f8 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -67,7 +67,8 @@ Vst2Bridge& get_bridge_instance(const AEffect* plugin) { Vst2Bridge::Vst2Bridge(boost::asio::io_context& main_context, std::string plugin_dll_path, std::string endpoint_base_dir) - : io_context(main_context), + : vst_plugin_path(plugin_dll_path), + io_context(main_context), plugin_handle(LoadLibrary(plugin_dll_path.c_str()), FreeLibrary), sockets(io_context, endpoint_base_dir, false) { // Got to love these C APIs diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 38adc306..0a0ada14 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -150,6 +150,11 @@ class Vst2Bridge { */ std::optional time_info; + /** + * The path to the .dll being loaded in the Wine VST host. + */ + const boost::filesystem::path vst_plugin_path; + private: /** * A wrapper around `plugin->dispatcher` that handles the opening and From 2f6883977f1650ce7b1969317187957c8360a977 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 12:13:27 +0100 Subject: [PATCH 07/40] Create secondary sockets for sending nested events --- src/common/communication.cpp | 2 +- src/common/communication.h | 34 +++++++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/common/communication.cpp b/src/common/communication.cpp index 8b0a8a2a..9a2bdc5a 100644 --- a/src/common/communication.cpp +++ b/src/common/communication.cpp @@ -85,7 +85,7 @@ EventHandler::EventHandler( boost::asio::io_context& io_context, boost::asio::local::stream_protocol::endpoint endpoint, bool listen) - : endpoint(endpoint), socket(io_context) { + : io_context(io_context), endpoint(endpoint), socket(io_context) { if (listen) { fs::create_directories(fs::path(endpoint.path()).parent_path()); acceptor.emplace(io_context, endpoint); diff --git a/src/common/communication.h b/src/common/communication.h index 6e147af8..4c0dbae5 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -259,8 +259,6 @@ class EventHandler { intptr_t value, void* data, float option) { - // TODO: Create a new socket if the mutex is locked - // Encode the right payload types for this event. Check the // documentation for `EventPayload` for more information. These types // are converted to C-style data structures in `passthrough_event()` so @@ -283,15 +281,26 @@ class EventHandler { .payload = payload, .value_payload = value_payload}; - // Prevent two threads from writing over the socket at the same time and - // messages getting out of order. This is needed because we can't - // prevent the plugin or the host from calling `dispatch()` or - // `audioMaster()` from multiple threads. + // A socket only handles a single request at a time as to prevent + // messages from arriving out of order. For throughput reasons we prefer + // to do most communication over a single main socket (`socket`), and + // we'll lock `write_mutex` while doing so. In the event that the mutex + // is already locked and thus the main socket is currently in use by + // another thread, then we'll spawn a new socket to handle the request. EventResult response; { - std::lock_guard lock(write_mutex); - write_object(socket, event); - response = read_object(socket); + std::unique_lock lock(write_mutex, std::try_to_lock); + if (lock.owns_lock()) { + write_object(socket, event); + response = read_object(socket); + } else { + boost::asio::local::stream_protocol::socket secondary_socket( + io_context); + secondary_socket.connect(endpoint); + + write_object(secondary_socket, event); + response = read_object(secondary_socket); + } } if (logging) { @@ -363,6 +372,13 @@ class EventHandler { } private: + /** + * The main IO context. New sockets created during `send()` will be bound to + * this context. In `receive()` we'll create a new IO context since we want + * to do all listening there on a dedicated thread. + */ + boost::asio::io_context& io_context; + boost::asio::local::stream_protocol::endpoint endpoint; boost::asio::local::stream_protocol::socket socket; From 523ac64d11a7113172331ff6877d3e980b862f8f Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 12:45:38 +0100 Subject: [PATCH 08/40] Add a missing lock to reading from active_plugins I've never seen this cause memory errors, but there exist a rare situation where it could. --- src/wine-host/bridges/group.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index 1b0e9e49..ebb24028 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -114,7 +114,11 @@ GroupBridge::~GroupBridge() { void GroupBridge::handle_plugin_dispatch(size_t plugin_id) { // At this point the `active_plugins` map will already contain the // intialized plugin's `Vst2Bridge` instance and this thread's handle - auto& [thread, bridge] = active_plugins[plugin_id]; + Vst2Bridge* bridge; + { + std::lock_guard lock(active_plugins_mutex); + bridge = active_plugins[plugin_id].second.get(); + } // Blocks this thread until the plugin shuts down, handling all events on // the main IO context From 81efa6febe5689a9f6686c98f3b24f039d15fdd9 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 12:58:03 +0100 Subject: [PATCH 09/40] Listen for incoming secondary event requests --- src/common/communication.h | 126 +++++++++++++++++++++++++++++++++++-- 1 file changed, 120 insertions(+), 6 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index 4c0dbae5..1e5e2529 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -16,6 +16,10 @@ #pragma once +#include +#include +#include + #include #include @@ -331,18 +335,80 @@ class EventHandler { * the next audio buffer gets processed. * * @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. + * this is for sending `dispatch()` events or host callbacks. Optional + * since it doesn't have to be done on both sides. * @param callback The function used to generate a response out of an event. * * @tparam F A function type in the form of `EventResponse(Event)`. * + * TODO: Add a boolean flag indicating that this is being called + * from an off thread + * * @relates EventHandler::send * @relates passthrough_event */ template void receive(std::optional> logging, F callback) { - // TODO: Listen for new incoming connections + assert(acceptor.has_value()); + + // As described above we'll handle incoming requests for `socket` on + // this thread. We'll also listen for incoming connections on `endpoint` + // on another thread. For any incoming connection we'll spawn a new + // thread to handle the request. When `socket` closes and this loop + // breaks, the listener and any still active threads will be cleaned up + // before this function exits. + boost::asio::io_context secondary_context{}; + + // This works the exact same was as `active_plugins` and + // `next_plugin_id` in `GroupBridge` + std::map active_secondary_requests{}; + std::atomic_size_t next_request_id{}; + std::mutex active_secondary_requests_mutex; + accept_requests( + *acceptor, logging, + [&](boost::asio::local::stream_protocol::socket secondary_socket) { + const size_t request_id = next_request_id.fetch_add(1); + + std::lock_guard lock(active_secondary_requests_mutex); + active_secondary_requests[request_id] = + std::jthread([&, request_id]() { + // TODO: Factor this out + auto event = read_object(secondary_socket); + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event(is_dispatch, event.opcode, + event.index, event.value, + event.payload, event.option, + event.value_payload); + } + + EventResult response = callback(event); + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log_event_response(is_dispatch, event.opcode, + response.return_value, + response.payload, + response.value_payload); + } + + write_object(secondary_socket, response); + + // When we have processed this request, we'll join the + // thread again with the thread that's handling + // `secondary_context`. + boost::asio::post(secondary_context, [&, request_id]() { + std::lock_guard lock( + active_secondary_requests_mutex); + + // The join is implicit because we're using + // std::jthread + active_secondary_requests.erase(request_id); + }); + }); + }); + + std::jthread secondary_requests_handler( + [&]() { secondary_context.run(); }); while (true) { try { @@ -369,9 +435,57 @@ class EventHandler { break; } } + + // When the socket gets terminated we'll stop accepting new connections + // and thus terminate `secondary_requests_handler`. Dropping + // `active_secondary_requests` will wait for all threads to join. + secondary_context.stop(); } private: + /** + * Used in `receive()` to asynchronously listen for secondary socket + * connections. After `callback()` returns this function will continue to be + * called until the IO context gets stopped. + * + * @param acceptor The acceptor we will be listening on. + * @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 callback A function that handles the new socket connection. + * + * @tparam F A function in the form + * `void(boost::asio::local::stream_protocol::socket)` to handle a new + * incoming connection. + */ + template + void accept_requests( + boost::asio::local::stream_protocol::acceptor& acceptor, + std::optional> logging, + F callback) { + acceptor.async_accept( + [&](const boost::system::error_code& error, + boost::asio::local::stream_protocol::socket secondary_socket) { + // + if (error.failed()) { + if (logging) { + auto [logger, is_dispatch] = *logging; + logger.log("Failure while accepting connections: " + + error.message()); + } else { + std::cerr << "Failure while accepting connections: " + << error.message() << std::endl; + } + + return; + } + + callback(std::move(secondary_socket)); + + accept_requests(acceptor, logging, callback); + }); + } + /** * The main IO context. New sockets created during `send()` will be bound to * this context. In `receive()` we'll create a new IO context since we want @@ -384,9 +498,9 @@ class EventHandler { /** * This acceptor will be used once synchronously on the listening side - * during `Sockets::connect()`. When `EventHandler::receive()` is called - * this will be replaced by a new acceptor bound to a new IO context to - * receive any additional incoming connections. + * during `Sockets::connect()`. When `EventHandler::receive()` is then + * called, we'll asynchronously listen for new incoming socket connections + * on `endpoint` using this same acceptor. */ std::optional acceptor; From 8d7826f1dfc64eff81253e0f0b5129039fb3ebf6 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 13:45:19 +0100 Subject: [PATCH 10/40] Handle incoming events from off-threads separately On the Wine side we want to handle most events on the main UI thread. We'll assume any events coming in from a secondary socket are safe and can be handled directly. --- src/common/communication.cpp | 6 ++++ src/common/communication.h | 28 ++++++++++++------- src/plugin/plugin-bridge.cpp | 5 ++-- src/wine-host/bridges/vst2.cpp | 51 +++++++++++++++++++++------------- 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/src/common/communication.cpp b/src/common/communication.cpp index 9a2bdc5a..543202ae 100644 --- a/src/common/communication.cpp +++ b/src/common/communication.cpp @@ -95,6 +95,12 @@ EventHandler::EventHandler( void EventHandler::connect() { if (acceptor) { acceptor->accept(socket); + + // As mentioned in `acceptor's` docstring, this acceptor will be + // recreated in `receive()` on another context, and potentially on the + // other side of the connection in the case of `vst_host_callback` + acceptor.reset(); + fs::remove(endpoint.path()); } else { socket.connect(endpoint); } diff --git a/src/common/communication.h b/src/common/communication.h index 1e5e2529..638cc94b 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -338,19 +338,17 @@ class EventHandler { * this is for sending `dispatch()` events or host callbacks. Optional * since it doesn't have to be done on both sides. * @param callback The function used to generate a response out of an event. + * See the definition of `F` for more information. * - * @tparam F A function type in the form of `EventResponse(Event)`. - * - * TODO: Add a boolean flag indicating that this is being called - * from an off thread + * @tparam F A function type in the form of `EventResponse(Event, bool)`. + * The boolean flag is `true` when this event was received on the main + * socket, and `false` otherwise. * * @relates EventHandler::send * @relates passthrough_event */ template void receive(std::optional> logging, F callback) { - assert(acceptor.has_value()); - // As described above we'll handle incoming requests for `socket` on // this thread. We'll also listen for incoming connections on `endpoint` // on another thread. For any incoming connection we'll spawn a new @@ -358,6 +356,9 @@ class EventHandler { // breaks, the listener and any still active threads will be cleaned up // before this function exits. boost::asio::io_context secondary_context{}; + // The previous acceptor has already been shut down by + // `EventHandler::connect()` + acceptor.emplace(secondary_context, endpoint); // This works the exact same was as `active_plugins` and // `next_plugin_id` in `GroupBridge` @@ -382,7 +383,7 @@ class EventHandler { event.value_payload); } - EventResult response = callback(event); + EventResult response = callback(event, false); if (logging) { auto [logger, is_dispatch] = *logging; logger.log_event_response(is_dispatch, event.opcode, @@ -420,7 +421,7 @@ class EventHandler { event.value_payload); } - EventResult response = callback(event); + EventResult response = callback(event, true); if (logging) { auto [logger, is_dispatch] = *logging; logger.log_event_response( @@ -499,8 +500,12 @@ class EventHandler { /** * This acceptor will be used once synchronously on the listening side * during `Sockets::connect()`. When `EventHandler::receive()` is then - * called, we'll asynchronously listen for new incoming socket connections - * on `endpoint` using this same acceptor. + * called, we'll recreate the acceptor asynchronously listen for new + * incoming socket connections on `endpoint` using this same acceptor. This + * is important, because on the case of `vst_host_callback` the acceptor is + * first accepts an initial socket on the plugin side (like all sockets), + * but all additional incoming connections of course have to be listened for + * on the plugin side. */ std::optional acceptor; @@ -650,6 +655,9 @@ boost::filesystem::path generate_endpoint_base(const std::string& plugin_name); * * This is the receiving analogue of the `*DataCovnerter` objects. * + * TODO: Now that `EventHandler::receive` replaced `receive_event()`, refactor + * this to just handle the event directly rather than returning a lambda + * * @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 diff --git a/src/plugin/plugin-bridge.cpp b/src/plugin/plugin-bridge.cpp index 3ddc2867..6acfdc11 100644 --- a/src/plugin/plugin-bridge.cpp +++ b/src/plugin/plugin-bridge.cpp @@ -121,10 +121,9 @@ PluginBridge::PluginBridge(audioMasterCallback host_callback) // instead of asynchronous IO since communication has to be handled in // lockstep anyway host_callback_handler = std::jthread([&]() { - // TODO: Think of a nicer way to structure this and the similar - // handler in `Vst2Bridge::handle_dispatch_midi_events` sockets.vst_host_callback.receive( - std::pair(logger, false), [&](Event& event) { + std::pair(logger, false), + [&](Event& event, bool /*on_main_thread*/) { // MIDI events sent from the plugin back to the host are a // special case here. They have to sent during the // `processReplacing()` function or else the host will ignore diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 846ae6f8..6d639f98 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -146,32 +146,43 @@ bool Vst2Bridge::should_skip_message_loop() const { void Vst2Bridge::handle_dispatch() { sockets.host_vst_dispatch.receive( - std::nullopt, - passthrough_event( - plugin, - [&](AEffect* plugin, int opcode, int index, intptr_t value, - void* data, float option) -> intptr_t { - // Instead of running `plugin->dispatcher()` (or - // `dispatch_wrapper()`) directly, we'll run the function within - // the IO context so all events will be executed on the same - // thread as the one that runs the Win32 message loop - std::promise dispatch_result; - boost::asio::dispatch(io_context, [&]() { - const intptr_t result = dispatch_wrapper( - plugin, opcode, index, value, data, option); + std::nullopt, [&](Event& event, bool on_main_thread) { + // TODO: As per the TODO in `passthrough_event`, this can use a + // round of refactoring now that we never use its returned + // lambda directly anymore + return passthrough_event( + plugin, + [&](AEffect* plugin, int opcode, int index, intptr_t value, + void* data, float option) -> intptr_t { + // On the main thread, instead of running + // `plugin->dispatcher()` (or `dispatch_wrapper()`) + // directly, we'll run the function within the IO context so + // all events will be executed on the same thread as the one + // that runs the Win32 message loop. We'll assume every + // event that comes in while the main thread is already + // handling an event in the IO context can safely be handled + // on an off thread. + if (on_main_thread) { + std::promise dispatch_result; + boost::asio::dispatch(io_context, [&]() { + const intptr_t result = dispatch_wrapper( + plugin, opcode, index, value, data, option); - dispatch_result.set_value(result); - }); + dispatch_result.set_value(result); + }); - // The message loop and X11 event handling will be run - // separately on a timer - return dispatch_result.get_future().get(); - })); + return dispatch_result.get_future().get(); + } else { + return dispatch_wrapper(plugin, opcode, index, value, + data, option); + } + })(event); + }); } void Vst2Bridge::handle_dispatch_midi_events() { sockets.host_vst_dispatch_midi_events.receive( - std::nullopt, [&](Event& event) { + std::nullopt, [&](Event& event, bool /*on_main_thread*/) { 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 From cde7b4ec67c387b687b74b1598c23f92be99d2e3 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 15:48:43 +0100 Subject: [PATCH 11/40] Properly move the sockets to the handler threads At times like this you really wish you were writing Rust right now. --- src/common/communication.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index 638cc94b..a240bf2c 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -116,7 +116,7 @@ inline T read_object(Socket& socket, {buffer.begin(), size}, object); if (BOOST_UNLIKELY(!success)) { - throw std::runtime_error("Deserialization failure in call:" + + throw std::runtime_error("Deserialization failure in call: " + std::string(__PRETTY_FUNCTION__)); } @@ -370,9 +370,12 @@ class EventHandler { [&](boost::asio::local::stream_protocol::socket secondary_socket) { const size_t request_id = next_request_id.fetch_add(1); + // We have to make sure to keep moving these sockets into the + // threads that will handle them std::lock_guard lock(active_secondary_requests_mutex); - active_secondary_requests[request_id] = - std::jthread([&, request_id]() { + active_secondary_requests[request_id] = std::jthread( + [&, request_id](boost::asio::local::stream_protocol::socket + secondary_socket) { // TODO: Factor this out auto event = read_object(secondary_socket); if (logging) { @@ -405,7 +408,8 @@ class EventHandler { // std::jthread active_secondary_requests.erase(request_id); }); - }); + }, + std::move(secondary_socket)); }); std::jthread secondary_requests_handler( @@ -465,9 +469,9 @@ class EventHandler { std::optional> logging, F callback) { acceptor.async_accept( - [&](const boost::system::error_code& error, + [&, logging, callback]( + const boost::system::error_code& error, boost::asio::local::stream_protocol::socket secondary_socket) { - // if (error.failed()) { if (logging) { auto [logger, is_dispatch] = *logging; From ac17539ef304603a8784ebdd833c608cd5d7096b Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 17:32:37 +0100 Subject: [PATCH 12/40] Drop all additional IO contexts Not really needed (since the only other thing happening in the IO context is processing stdio from the Wine process) and it was causing some impossible to debug malloc failures in Boost.Asio. --- src/common/communication.h | 29 ++++++++++++++--------------- src/common/logging.h | 5 +++-- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index a240bf2c..d364f0a9 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -206,8 +206,9 @@ class EventHandler { * Sets up a single main socket for this type of events. The sockets won't * be active until `connect()` gets called. * - * @param io_context The IO context the sockets should be bound to. Relevant - * when doing asynchronous operations. + * @param io_context The IO context the sockets should be bound to. + * Additional incoming connections will be handled asynchronously within + * this IO context. * @param endpoint The socket endpoint used for this event handler. * @param listen If `true`, start listening on the sockets. Incoming * connections will be accepted when `connect()` gets called. This should @@ -355,10 +356,9 @@ class EventHandler { // thread to handle the request. When `socket` closes and this loop // breaks, the listener and any still active threads will be cleaned up // before this function exits. - boost::asio::io_context secondary_context{}; // The previous acceptor has already been shut down by // `EventHandler::connect()` - acceptor.emplace(secondary_context, endpoint); + acceptor.emplace(io_context, endpoint); // This works the exact same was as `active_plugins` and // `next_plugin_id` in `GroupBridge` @@ -400,7 +400,7 @@ class EventHandler { // When we have processed this request, we'll join the // thread again with the thread that's handling // `secondary_context`. - boost::asio::post(secondary_context, [&, request_id]() { + boost::asio::post(io_context, [&, request_id]() { std::lock_guard lock( active_secondary_requests_mutex); @@ -412,9 +412,6 @@ class EventHandler { std::move(secondary_socket)); }); - std::jthread secondary_requests_handler( - [&]() { secondary_context.run(); }); - while (true) { try { auto event = read_object(socket); @@ -441,10 +438,11 @@ class EventHandler { } } - // When the socket gets terminated we'll stop accepting new connections - // and thus terminate `secondary_requests_handler`. Dropping - // `active_secondary_requests` will wait for all threads to join. - secondary_context.stop(); + // After the main socket gets terminated (during shutdown) we'll make + // sure all outstanding jobs have been processed and then drop all work + // from the IO context + std::lock_guard lock(active_secondary_requests_mutex); + io_context.stop(); } private: @@ -492,9 +490,10 @@ class EventHandler { } /** - * The main IO context. New sockets created during `send()` will be bound to - * this context. In `receive()` we'll create a new IO context since we want - * to do all listening there on a dedicated thread. + * The main IO context for this application. New sockets created during + * `send()` will be bound to this context, and in `receive()` we'll + * asynchronously listen for additional incoming connections through this + * context. */ boost::asio::io_context& io_context; diff --git a/src/common/logging.h b/src/common/logging.h index 2c260a5b..62723722 100644 --- a/src/common/logging.h +++ b/src/common/logging.h @@ -97,8 +97,9 @@ class Logger { void log_get_parameter_response(float vlaue); void log_set_parameter(int index, float value); void log_set_parameter_response(); - // If is_dispatch is true, then use opcode names from the plugin's dispatch - // function. Otherwise use names for the host callback function opcodes. + // If `is_dispatch` is `true`, then use opcode names from the plugin's + // dispatch function. Otherwise use names for the host callback function + // opcodes. void log_event(bool is_dispatch, int opcode, int index, From e067bbbfbc99cb70f34e01c7cd7a352ac3d3a486 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 17:44:01 +0100 Subject: [PATCH 13/40] Don't stop the whole IO context This would break plugin groups since the different plugins share a single IO context. --- src/common/communication.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index d364f0a9..658a8fab 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -442,7 +442,7 @@ class EventHandler { // sure all outstanding jobs have been processed and then drop all work // from the IO context std::lock_guard lock(active_secondary_requests_mutex); - io_context.stop(); + acceptor.reset(); } private: @@ -471,13 +471,14 @@ class EventHandler { const boost::system::error_code& error, boost::asio::local::stream_protocol::socket secondary_socket) { if (error.failed()) { + // On the Wine side it's expected that the main socket + // connection will be dropped during shutdown, so we can + // silently ignore any related socket errors on the Wine + // side if (logging) { auto [logger, is_dispatch] = *logging; logger.log("Failure while accepting connections: " + error.message()); - } else { - std::cerr << "Failure while accepting connections: " - << error.message() << std::endl; } return; From 816a2cbe019e2d8759a84ab76f696c09bc9fd39e Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 17:54:41 +0100 Subject: [PATCH 14/40] [yabridgectl] Update Wine error detection The usage string has changed, better to just match part of it so this won't cause issues again in the future. --- tools/yabridgectl/src/utils.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/yabridgectl/src/utils.rs b/tools/yabridgectl/src/utils.rs index 026dc59d..cbc025c5 100644 --- a/tools/yabridgectl/src/utils.rs +++ b/tools/yabridgectl/src/utils.rs @@ -31,9 +31,9 @@ use textwrap::Wrapper; use crate::config::{Config, KnownConfig}; /// (Part of) the expected output when running `yabridge-host.exe`. Used to verify that everything's -/// working correctly. -const YABRIDGE_HOST_EXPECTED_OUTPUT: &str = - "Usage: yabridge-host.exe "; +/// working correctly. We'll only match this prefix so we can modify the exact output at a later +/// moment without causing issues. +const YABRIDGE_HOST_EXPECTED_OUTPUT_PREFIX: &str = "Usage: yabridge-"; /// Wrapper around [`std::fs::copy()`](std::fs::copy) with a human readable error message. pub fn copy, Q: AsRef>(from: P, to: Q) -> Result { @@ -220,7 +220,7 @@ pub fn verify_wine_setup(config: &mut Config) -> Result<()> { let mut success = false; let mut last_error: Option<&str> = None; for line in stderr.lines() { - if line == YABRIDGE_HOST_EXPECTED_OUTPUT { + if line.starts_with(YABRIDGE_HOST_EXPECTED_OUTPUT_PREFIX) { success = true; break; } From 4c490808c0584e9ad4d4de46aa2882293116474d Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 18:00:10 +0100 Subject: [PATCH 15/40] Add a changelog entry for the socket rework --- CHANGELOG.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fbcdacbb..783a344b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,22 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +TODO: Expand on this + +- The way yabridge communicates between its plugin and the Wine host process has + been completely revamped. This was necessary to allow yabridge to handle + nested or mutually recursive function calls. What this boils down to is that + yabridge became even faster, more responsive, and can now handle a few edge + case scenarios that previously required workarounds. This means that yabridge + will now work out of the box with _REAPER_ and _Renoise_ and the + `hack_reaper_update_display` workaround for is no longer needed. I have been + testing this extensively to make sure that the change does not introduce any + regressions, but please let me know if this does break anything for you. + ## [1.7.1] - 2020-10-23 ### Fixed From c95e8aa63c97c90d2e7b1e9989bab9f23fe1e40c Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 18:00:24 +0100 Subject: [PATCH 16/40] Add a TODO for removing hack_reaper_update_display --- src/common/configuration.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/common/configuration.h b/src/common/configuration.h index 68d9e357..01dc91a8 100644 --- a/src/common/configuration.h +++ b/src/common/configuration.h @@ -92,6 +92,8 @@ class Configuration { * will automatically return 0 without being sent to the host. This is a * HACK to work around implementations issues in REAPER and Renoise, see #29 * and #32. + * + * TODO: Remove for yabridge 1.8.0 */ bool hack_reaper_update_display = false; From ca2b95e7aae66912c9ca51df8168a6cced5d16f5 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 20:00:26 +0100 Subject: [PATCH 17/40] Handle dispatch() directly during event handling When the message loop is active and we get an incoming dispatch() event, we'll just handle it directly. In practice this would only be needed when the event is a response to an `audioMaster()` call made during the event loop, but we can't know that. This allows the `getProgram()` during `audioMasterUpdateDisplay()` in REAPER and Renoise to work correctly. Hopefully this doesn't cause random rare breakage. --- src/common/communication.h | 2 +- src/wine-host/bridges/group.cpp | 27 ++-------- src/wine-host/bridges/group.h | 10 +--- src/wine-host/bridges/vst2.cpp | 19 +++---- src/wine-host/bridges/vst2.h | 4 +- src/wine-host/individual-host.cpp | 41 +++------------ src/wine-host/utils.cpp | 10 ++++ src/wine-host/utils.h | 87 +++++++++++++++++++++++++++++++ 8 files changed, 122 insertions(+), 78 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index 658a8fab..10ea422c 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -364,7 +364,7 @@ class EventHandler { // `next_plugin_id` in `GroupBridge` std::map active_secondary_requests{}; std::atomic_size_t next_request_id{}; - std::mutex active_secondary_requests_mutex; + std::mutex active_secondary_requests_mutex{}; accept_requests( *acceptor, logging, [&](boost::asio::local::stream_protocol::socket secondary_socket) { diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index ebb24028..e8620ed0 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -29,11 +29,6 @@ namespace fs = boost::filesystem; using namespace std::literals::chrono_literals; -/** - * The delay between calls to the event loop at a more than cinematic 30 fps. - */ -constexpr std::chrono::duration event_loop_interval = 1000ms / 30; - /** * Listen on the specified endpoint if no process is already listening there, * otherwise throw. This is needed to handle these three situations: @@ -94,10 +89,9 @@ GroupBridge::GroupBridge(boost::filesystem::path group_socket_path) stdout_redirect(stdio_context, STDOUT_FILENO), stderr_redirect(stdio_context, STDERR_FILENO), group_socket_endpoint(group_socket_path.string()), - group_socket_acceptor( - create_acceptor_if_inactive(plugin_context, group_socket_endpoint)), - events_timer(plugin_context), - shutdown_timer(plugin_context) { + group_socket_acceptor(create_acceptor_if_inactive(plugin_context.context, + group_socket_endpoint)), + shutdown_timer(plugin_context.context) { // Write this process's original STDOUT and STDERR streams to the logger // TODO: This works for output generated by plugins, but not for debug // messages generated by wineserver. Is it possible to catch those? @@ -131,7 +125,7 @@ void GroupBridge::handle_plugin_dispatch(size_t plugin_id) { // potentially corrupt our heap. This way we can also properly join the // thread again. If no active plugins remain, then we'll terminate the // process. - boost::asio::post(plugin_context, [this, plugin_id]() { + boost::asio::post(plugin_context.context, [this, plugin_id]() { std::lock_guard lock(active_plugins_mutex); // The join is implicit because we're using std::jthread @@ -240,16 +234,7 @@ void GroupBridge::accept_requests() { } void GroupBridge::async_handle_events() { - // Try to keep a steady framerate, but add in delays to let other events get - // handled if the GUI message handling somehow takes very long. - events_timer.expires_at( - std::max(events_timer.expiry() + event_loop_interval, - std::chrono::steady_clock::now() + 5ms)); - events_timer.async_wait([&](const boost::system::error_code& error) { - if (error.failed()) { - return; - } - + plugin_context.async_handle_events([&]() { { // Always handle X11 events std::lock_guard lock(active_plugins_mutex); @@ -279,8 +264,6 @@ void GroupBridge::async_handle_events() { DispatchMessage(&msg); } } - - async_handle_events(); }); } diff --git a/src/wine-host/bridges/group.h b/src/wine-host/bridges/group.h index cf7690e8..52b43138 100644 --- a/src/wine-host/bridges/group.h +++ b/src/wine-host/bridges/group.h @@ -217,7 +217,7 @@ class GroupBridge { * operations that may involve the Win32 mesasge loop (e.g. initialization * and most `AEffect::dispatcher()` calls) should be run on. */ - boost::asio::io_context plugin_context; + PluginContext plugin_context; /** * A seperate IO context that handles the STDIO redirect through * `StdIoCapture`. This is seperated the `plugin_context` above so that @@ -279,14 +279,6 @@ class GroupBridge { */ std::mutex active_plugins_mutex; - /** - * A timer used to repeatedly handle the Win32 message loop and the X11 - * events. - * - 8 @see async_handle_events - */ - boost::asio::steady_timer events_timer; - /** * A timer to defer shutting down the process, allowing for fast plugin * scanning without having to start a new group host process for each diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 6d639f98..f205fa26 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -64,13 +64,13 @@ Vst2Bridge& get_bridge_instance(const AEffect* plugin) { return *static_cast(plugin->ptr1); } -Vst2Bridge::Vst2Bridge(boost::asio::io_context& main_context, +Vst2Bridge::Vst2Bridge(PluginContext& main_context, std::string plugin_dll_path, std::string endpoint_base_dir) : vst_plugin_path(plugin_dll_path), - io_context(main_context), + plugin_context(main_context), plugin_handle(LoadLibrary(plugin_dll_path.c_str()), FreeLibrary), - sockets(io_context, endpoint_base_dir, false) { + sockets(plugin_context.context, endpoint_base_dir, false) { // Got to love these C APIs if (!plugin_handle) { throw std::runtime_error("Could not load the Windows .dll file at '" + @@ -158,13 +158,14 @@ void Vst2Bridge::handle_dispatch() { // `plugin->dispatcher()` (or `dispatch_wrapper()`) // directly, we'll run the function within the IO context so // all events will be executed on the same thread as the one - // that runs the Win32 message loop. We'll assume every - // event that comes in while the main thread is already - // handling an event in the IO context can safely be handled - // on an off thread. - if (on_main_thread) { + // that runs the Win32 message loop. In some scenarios we'll + // receive incoming calls from multiple threads or we'll + // receive calls while we're currently stuck in the Win32 + // message loop. In those cases we'll assume that these + // events can be safely handled directlyfrom another thread. + if (on_main_thread && !plugin_context.event_loop_active) { std::promise dispatch_result; - boost::asio::dispatch(io_context, [&]() { + boost::asio::dispatch(plugin_context.context, [&]() { const intptr_t result = dispatch_wrapper( plugin, opcode, index, value, data, option); diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 0a0ada14..7f12527f 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -74,7 +74,7 @@ class Vst2Bridge { * @throw std::runtime_error Thrown when the VST plugin could not be loaded, * or if communication could not be set up. */ - Vst2Bridge(boost::asio::io_context& main_context, + Vst2Bridge(PluginContext& main_context, std::string plugin_dll_path, std::string endpoint_base_dir); @@ -172,7 +172,7 @@ class Vst2Bridge { * message handling can be performed from a single thread, even when hosting * multiple plugins. */ - boost::asio::io_context& io_context; + PluginContext& plugin_context; /** * The configuration for this instance of yabridge based on the `.so` file diff --git a/src/wine-host/individual-host.cpp b/src/wine-host/individual-host.cpp index 3f93e46b..087372f0 100644 --- a/src/wine-host/individual-host.cpp +++ b/src/wine-host/individual-host.cpp @@ -24,19 +24,6 @@ #include "../common/utils.h" #include "bridges/vst2.h" -using namespace std::literals::chrono_literals; - -/** - * The delay between calls to the event loop at a more than cinematic 30 fps. - */ -constexpr std::chrono::duration event_loop_interval = 1000ms / 30; - -/** - * Handle both Win32 and X11 events on a timer. This is more or less a - * simplified version of `GroupBridge::async_handle_events`. - */ -void async_handle_events(boost::asio::steady_timer& timer, Vst2Bridge& bridge); - /** * This is the default VST host application. It will load the specified VST2 * plugin, and then connect back to the `libyabridge.so` instance that spawned @@ -80,10 +67,10 @@ int __cdecl main(int argc, char* argv[]) { // setup is slightly more convoluted than it has to be, but doing it this // way we don't need to differentiate between individually hosted plugins // and plugin groups when it comes to event handling. - boost::asio::io_context io_context{}; + PluginContext plugin_context{}; std::unique_ptr bridge; try { - bridge = std::make_unique(io_context, plugin_dll_path, + bridge = std::make_unique(plugin_context, plugin_dll_path, socket_endpoint_path); } catch (const std::runtime_error& error) { std::cerr << "Error while initializing Wine VST host:" << std::endl; @@ -101,25 +88,9 @@ int __cdecl main(int argc, char* argv[]) { // Handle Win32 messages and X11 events on a timer, just like in // `GroupBridge::async_handle_events()`` - boost::asio::steady_timer events_timer(io_context); - async_handle_events(events_timer, *bridge); - - io_context.run(); -} - -void async_handle_events(boost::asio::steady_timer& timer, Vst2Bridge& bridge) { - // Try to keep a steady framerate, but add in delays to let other events get - // handled if the GUI message handling somehow takes very long. - timer.expires_at(std::max(timer.expiry() + event_loop_interval, - std::chrono::steady_clock::now() + 5ms)); - timer.async_wait([&](const boost::system::error_code& error) { - if (error.failed()) { - return; - } - - bridge.handle_x11_events(); - bridge.handle_win32_events(); - - async_handle_events(timer, bridge); + plugin_context.async_handle_events([&]() { + bridge->handle_x11_events(); + bridge->handle_win32_events(); }); + plugin_context.run(); } diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index cc26ffb4..3ef90392 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -16,6 +16,16 @@ #include "utils.h" +PluginContext::PluginContext() : context(), events_timer(context) {} + +void PluginContext::run() { + context.run(); +} + +void PluginContext::stop() { + context.stop(); +} + Win32Thread::Win32Thread() : handle(nullptr, nullptr) {} Win32Timer::Win32Timer(HWND window_handle, diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index 14826e85..20d52bf3 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -16,6 +16,8 @@ #pragma once +#include "boost-fix.h" + #include #include @@ -26,6 +28,91 @@ #define WIN32_LEAN_AND_MEAN #include +#include + +/** + * The delay between calls to the event loop at an even more than cinematic 30 + * fps. + */ +constexpr std::chrono::duration event_loop_interval = + std::chrono::milliseconds(1000) / 30; + +/** + * A wrapper around `boost::asio::io_context()`. A single instance is shared for + * all plugins in a plugin group so that most events can be handled on the main + * thread, which can be required because all GUI related operations have to be + * handled from the same thread. If during the Win32 message loop the plugin + * performs a host callback and the host then calls a function on the plugin in + * response, then this IO context will still be busy with the message loop + * which. To prevent a deadlock in this situation, we'll allow different threads + * to handle `dispatch()` calls while the message loop is running. + */ +class PluginContext { + public: + PluginContext(); + + /** + * Run the IO context. This rest of this class assumes that this is only + * done from a single thread. + */ + void run(); + + /** + * Drop all future work from the IO context. This does not necessarily mean + * that the thread that called `plugin_context.run()` immediatly returns. + */ + void stop(); + + /** + * Start a timer to handle events every `event_loop_interval` milliseconds. + * `message_loop_active()` will return `true` while `handler` is being + * executed. + * + * @param handler The function that should be executed in the IO context + * when the timer ticks. This should be a function that handles both the + * X11 events and the Win32 message loop. + */ + template + void async_handle_events(F handler) { + // Try to keep a steady framerate, but add in delays to let other events + // get handled if the GUI message handling somehow takes very long. + events_timer.expires_at(std::max( + events_timer.expiry() + event_loop_interval, + std::chrono::steady_clock::now() + std::chrono::milliseconds(5))); + events_timer.async_wait( + [&, handler](const boost::system::error_code& error) { + if (error.failed()) { + return; + } + + event_loop_active = true; + handler(); + event_loop_active = false; + + async_handle_events(handler); + }); + } + + /** + * Is `true` if the context is currently handling the Win32 message loop and + * incoming `dispatch()` events should be handled on their own thread (as + * posting them to the IO context will thus block). + */ + std::atomic_bool event_loop_active; + + /** + * The raw IO context. Can and should be used directly for everything that's + * not the event handling loop. + */ + boost::asio::io_context context; + + private: + /** + * The timer used to periodically handle X11 events and Win32 messages. + */ + boost::asio::steady_timer events_timer; +}; + /** * A simple RAII wrapper around the Win32 thread API. * From e12a56978d9f2224e9b2db005412e44f549da7af Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 20:39:16 +0100 Subject: [PATCH 18/40] Reintroduce the additional IO contexts Having these bound to the main context was not a good idea since that would prevent sockets from being accepted on the Wine side while the message loop is running. Partial revert of ac17539ef304603a8784ebdd833c608cd5d7096b --- src/common/communication.h | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index 10ea422c..671ebaec 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -206,9 +206,9 @@ class EventHandler { * Sets up a single main socket for this type of events. The sockets won't * be active until `connect()` gets called. * - * @param io_context The IO context the sockets should be bound to. - * Additional incoming connections will be handled asynchronously within - * this IO context. + * @param io_context The IO context the main socket should be bound to. A + * new IO context will be created for accepting the additional incoming + * connections. * @param endpoint The socket endpoint used for this event handler. * @param listen If `true`, start listening on the sockets. Incoming * connections will be accepted when `connect()` gets called. This should @@ -356,9 +356,11 @@ class EventHandler { // thread to handle the request. When `socket` closes and this loop // breaks, the listener and any still active threads will be cleaned up // before this function exits. + boost::asio::io_context secondary_context{}; + // The previous acceptor has already been shut down by // `EventHandler::connect()` - acceptor.emplace(io_context, endpoint); + acceptor.emplace(secondary_context, endpoint); // This works the exact same was as `active_plugins` and // `next_plugin_id` in `GroupBridge` @@ -400,7 +402,7 @@ class EventHandler { // When we have processed this request, we'll join the // thread again with the thread that's handling // `secondary_context`. - boost::asio::post(io_context, [&, request_id]() { + boost::asio::post(secondary_context, [&, request_id]() { std::lock_guard lock( active_secondary_requests_mutex); @@ -412,6 +414,9 @@ class EventHandler { std::move(secondary_socket)); }); + std::jthread secondary_requests_handler( + [&]() { secondary_context.run(); }); + while (true) { try { auto event = read_object(socket); @@ -442,6 +447,7 @@ class EventHandler { // sure all outstanding jobs have been processed and then drop all work // from the IO context std::lock_guard lock(active_secondary_requests_mutex); + secondary_context.stop(); acceptor.reset(); } @@ -491,10 +497,9 @@ class EventHandler { } /** - * The main IO context for this application. New sockets created during - * `send()` will be bound to this context, and in `receive()` we'll - * asynchronously listen for additional incoming connections through this - * context. + * The main IO context. New sockets created during `send()` will be bound to + * this context. In `receive()` we'll create a new IO context since we want + * to do all listening there on a dedicated thread. */ boost::asio::io_context& io_context; From 016ceccc180dec5267e1052961ffe99fa9a76124 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 20:59:27 +0100 Subject: [PATCH 19/40] Mark several opcodes as unsafe These potentially perform GUI operations and should always be handled on the main thread. --- src/wine-host/bridges/vst2.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index f205fa26..d745ce02 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include "../../common/communication.h" @@ -38,6 +39,14 @@ Vst2Bridge* current_bridge_instance = nullptr; */ std::mutex current_bridge_instance_mutex; +/** + * Opcodes that should always be handled on the main thread because they may + * involve GUI operations. + */ +const std::set unsafe_opcodes{effOpen, effClose, effEditGetRect, + effEditOpen, effEditClose, effEditIdle, + effEditTop}; + intptr_t VST_CALL_CONV host_callback_proxy(AEffect*, int, int, intptr_t, void*, float); @@ -163,7 +172,8 @@ void Vst2Bridge::handle_dispatch() { // receive calls while we're currently stuck in the Win32 // message loop. In those cases we'll assume that these // events can be safely handled directlyfrom another thread. - if (on_main_thread && !plugin_context.event_loop_active) { + if (unsafe_opcodes.contains(opcode) || + (on_main_thread && !plugin_context.event_loop_active)) { std::promise dispatch_result; boost::asio::dispatch(plugin_context.context, [&]() { const intptr_t result = dispatch_wrapper( From 5b00ddb0c4dcfc4a0748e1a80eee1f00edaf1f30 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 22:13:55 +0100 Subject: [PATCH 20/40] Fall back to waiting when socket is not yet ready This can happen with plugin groups. --- src/common/communication.h | 30 +++++++++++++++++++++++++----- src/wine-host/bridges/group.cpp | 2 +- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index 671ebaec..5664f551 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -179,6 +179,11 @@ class DefaultDataConverter { }; /** + * So, this is a bit of a mess. The TL;DR is that we want to use a single long + * living socket connection for `dispatch()` and another one for `audioMaster()` + * for performance reasons, but when the socket is already being written to we + * create new connections on demand. + * * For most of our sockets we can just send out our messages on the writing * side, and do a simple blocking loop on the reading side. The `dispatch()` and * `audioMaster()` calls are different. Not only do they have they come with @@ -299,12 +304,27 @@ class EventHandler { write_object(socket, event); response = read_object(socket); } else { - boost::asio::local::stream_protocol::socket secondary_socket( - io_context); - secondary_socket.connect(endpoint); + try { + boost::asio::local::stream_protocol::socket + secondary_socket(io_context); + secondary_socket.connect(endpoint); - write_object(secondary_socket, event); - response = read_object(secondary_socket); + write_object(secondary_socket, event); + response = read_object(secondary_socket); + } catch (const boost::system::system_error&) { + // So, what do we do when noone is listening on the endpoint + // yet? This can happen with plugin groups when the Wine + // host process does an `audioMaster()` call before the + // plugin is listening. If that happens we'll fall back to a + // synchronous request. This is not very pretty, so if + // anyone can think of a better way to structure all of this + // while still mainting a long living primary socket please + // let me know. + std::lock_guard lock(write_mutex); + + write_object(socket, event); + response = read_object(socket); + } } } diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index e8620ed0..8a42c56d 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -219,7 +219,7 @@ void GroupBridge::accept_requests() { // is only used within this context we don't need any locks. const size_t plugin_id = next_plugin_id.fetch_add(1); active_plugins[plugin_id] = - std::pair(std::jthread([&, request]() { + std::pair(std::jthread([this, plugin_id]() { handle_plugin_dispatch(plugin_id); }), std::move(bridge)); From e51c7f7ae3149c411188fc3e1ba78af7680e9fd5 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 26 Oct 2020 22:22:21 +0100 Subject: [PATCH 21/40] Get rid of hack_reaper_update_display It is now no longer necessary. --- CHANGELOG.md | 10 ++++++++-- src/common/configuration.cpp | 6 ------ src/common/configuration.h | 11 ----------- src/plugin/plugin-bridge.cpp | 27 --------------------------- src/wine-host/bridges/vst2.cpp | 10 ---------- 5 files changed, 8 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 783a344b..00113654 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,6 @@ Versioning](https://semver.org/spec/v2.0.0.html). ### Added -TODO: Expand on this - - The way yabridge communicates between its plugin and the Wine host process has been completely revamped. This was necessary to allow yabridge to handle nested or mutually recursive function calls. What this boils down to is that @@ -22,6 +20,14 @@ TODO: Expand on this testing this extensively to make sure that the change does not introduce any regressions, but please let me know if this does break anything for you. + TODO: Expand on this + +### Removed + +- The `hack_reaper_update_display` option is now obsolete and has been removed. + + TODO: Remove all mentions of `hack_reaper_update_display` from the readme. + ## [1.7.1] - 2020-10-23 ### Fixed diff --git a/src/common/configuration.cpp b/src/common/configuration.cpp index b8c5161e..791df004 100644 --- a/src/common/configuration.cpp +++ b/src/common/configuration.cpp @@ -84,12 +84,6 @@ Configuration::Configuration(const fs::path& config_path, } else { invalid_options.push_back(key); } - } else if (key == "hack_reaper_update_display") { - if (const auto parsed_value = value.as_boolean()) { - hack_reaper_update_display = parsed_value->get(); - } else { - invalid_options.push_back(key); - } } else if (key == "group") { if (const auto parsed_value = value.as_string()) { group = parsed_value->get(); diff --git a/src/common/configuration.h b/src/common/configuration.h index 01dc91a8..275d8410 100644 --- a/src/common/configuration.h +++ b/src/common/configuration.h @@ -87,16 +87,6 @@ class Configuration { */ bool editor_double_embed = false; - /** - * If this is set to true, then any calls to `audioMasterUpdateDisplay()` - * will automatically return 0 without being sent to the host. This is a - * HACK to work around implementations issues in REAPER and Renoise, see #29 - * and #32. - * - * TODO: Remove for yabridge 1.8.0 - */ - bool hack_reaper_update_display = false; - /** * The name of the plugin group that should be used for the plugin this * configuration object was created for. If not set, then the plugin should @@ -130,7 +120,6 @@ class Configuration { template void serialize(S& s) { s.value1b(editor_double_embed); - s.value1b(hack_reaper_update_display); s.ext(group, bitsery::ext::StdOptional(), [](S& s, auto& v) { s.text1b(v, 4096); }); s.ext(matched_file, bitsery::ext::StdOptional(), diff --git a/src/plugin/plugin-bridge.cpp b/src/plugin/plugin-bridge.cpp index 6acfdc11..bbd8b757 100644 --- a/src/plugin/plugin-bridge.cpp +++ b/src/plugin/plugin-bridge.cpp @@ -473,29 +473,6 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, logger.log(" when using REAPER."); logger.log(""); - // Since the user is using REAPER, also show a reminder that the - // REAPER workaround should be enabled when it is not yet - // enabled since it may be easy to miss - if (!config.hack_reaper_update_display) { - logger.log( - " With using REAPER you will have to enable the"); - logger.log( - " 'hack_reaper_update_display' option to prevent"); - logger.log( - " certain plugins from crashing. To do so, create a"); - logger.log( - " new file named 'yabridge.toml' next to your"); - logger.log(" plugins with the following contents:"); - logger.log(""); - logger.log( - " # " - "https://github.com/robbert-vdh/" - "yabridge#runtime-dependencies-and-known-issues"); - logger.log(" [\"*\"]"); - logger.log(" hack_reaper_update_display = true"); - logger.log(""); - } - logger.log_event_response(true, opcode, -1, nullptr, std::nullopt); return -1; @@ -660,10 +637,6 @@ void PluginBridge::log_init_message() { if (config.editor_double_embed) { other_options.push_back("editor: double embed"); } - if (config.hack_reaper_update_display) { - other_options.push_back( - "hack: REAPER audioMasterUpdateDisplay() workaround"); - } if (!other_options.empty()) { init_msg << join_quoted_strings(other_options) << std::endl; } else { diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index d745ce02..648ffe36 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -559,16 +559,6 @@ intptr_t Vst2Bridge::host_callback(AEffect* effect, intptr_t value, void* data, float option) { - // HACK: Sadly this is needed to work around a mutual recursion issue with - // REAPER and Renoise. See #29 and #32. - // TODO: We don't have access to the verbosity level here, but it would be - // nice to log that this is being skipped when `YABRIDGE_DEBUG_LEVEL - // >= 2`. - if (config.hack_reaper_update_display && - opcode == audioMasterUpdateDisplay) { - return 0; - } - HostCallbackDataConverter converter(effect, time_info); return sockets.vst_host_callback.send(converter, std::nullopt, opcode, index, value, data, option); From dc72dd97a59983b4ee0c8248ca764f13c44c5b6c Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 11:01:55 +0100 Subject: [PATCH 22/40] Reword the socket rework changelog entry --- CHANGELOG.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00113654..7b04c8e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,15 +10,16 @@ Versioning](https://semver.org/spec/v2.0.0.html). ### Added -- The way yabridge communicates between its plugin and the Wine host process has - been completely revamped. This was necessary to allow yabridge to handle - nested or mutually recursive function calls. What this boils down to is that - yabridge became even faster, more responsive, and can now handle a few edge - case scenarios that previously required workarounds. This means that yabridge - will now work out of the box with _REAPER_ and _Renoise_ and the - `hack_reaper_update_display` workaround for is no longer needed. I have been - testing this extensively to make sure that the change does not introduce any - regressions, but please let me know if this does break anything for you. +- The way communication works in yabridge has been completely redesigned. This + was necessary to allow yabridge to handle nested and mutually recursive + function calls, as well as several other edge cases. What this boils down to + is that yabridge became even faster, more responsive, and can now handle a few + edge case scenarios that would previously require workarounds. This means that + yabridge no longer requires the `hack_reaper_update_display` workaround for + _REAPER_ and _Renoise_, and the loading issues in Bitwig Studio 3.3 beta 1 + have also been resolved. I have been testing this extensively to make sure + that the change does not introduce any regressions, but please let me know if + this does break anything for you. TODO: Expand on this From 058eb9f2ee152d293d07978781f7183fc3b5a4ce Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 11:56:00 +0100 Subject: [PATCH 23/40] Fix winedbg warning --- src/plugin/host-process.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugin/host-process.cpp b/src/plugin/host-process.cpp index 48ae621c..fa46d623 100644 --- a/src/plugin/host-process.cpp +++ b/src/plugin/host-process.cpp @@ -104,7 +104,7 @@ IndividualHost::IndividualHost(boost::asio::io_context& io_context, #endif )) { #ifdef WITH_WINEDBG - if (plugin_path.string().find(' ') != std::string::npos) { + if (plugin_path.filename().string().find(' ') != std::string::npos) { logger.log("Warning: winedbg does not support paths containing spaces"); } #endif From 1681ec976745c9725486eddcc8317b66b0c21bc2 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 17:04:40 +0100 Subject: [PATCH 24/40] Add support for lambdas to Win32Thread --- src/wine-host/utils.cpp | 23 ++++++++++++++ src/wine-host/utils.h | 67 ++++++++++++++++++++++++++++------------- 2 files changed, 69 insertions(+), 21 deletions(-) diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index 3ef90392..7d8d86c8 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -18,6 +18,29 @@ PluginContext::PluginContext() : context(), events_timer(context) {} +uint32_t WINAPI win32_thread_trampoline(std::function* entry_point) { + (*entry_point)(); + delete entry_point; + + return 0; +} + +Win32Thread::~Win32Thread() { + // Imitate std::jthread's joining behaviour, the thread handle will be + // cleaned up by the `std::unique_ptr` + if (handle) { + WaitForSingleObject(handle.get(), INFINITE); + } +} + +Win32Thread::Win32Thread(Win32Thread&& o) : handle(std::move(o.handle)) {} + +Win32Thread& Win32Thread::operator=(Win32Thread&& o) { + handle = std::move(o.handle); + + return *this; +} + void PluginContext::run() { context.run(); } diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index 20d52bf3..b5c04e6e 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -114,23 +114,29 @@ class PluginContext { }; /** - * A simple RAII wrapper around the Win32 thread API. + * A proxy function that calls `Win32Thread::entry_point` since `CreateThread()` + * is not usable with lambdas directly. Calling the passed function will invoke + * the lambda with the arguments passed during `Win32Thread`'s constructor. This + * function deallocates the function after it's finished executing. * - * These threads are implemented using `CreateThread` rather than `std::thread` - * because in some cases `std::thread` in winelib causes very hard to debug data - * races within plugins such as Serum. This might be caused by calling - * conventions being handled differently. + * We can't store the function pointer in the `Win32Thread` object because + * moving a `Win32Thread` object would then cause issues. * - * This somewhat mimicks `std::thread`, with the following differences: + * @param win32_thread_trampoline A `std::function*` pointer to a + * function pointer, great. + */ +uint32_t WINAPI win32_thread_trampoline(std::function* entry_point); + +/** + * A simple RAII wrapper around the Win32 thread API that imitates + * `std::jthread`. * - * - The threads will immediatly be killed silently when a `Win32Thread` object - * goes out of scope. This is the desired behavior in our case since the host - * will have already saved chunk data before closing the plugin and this - * ensures that the plugin shuts down quickly. - * - This does not accept lambdas because we're calling a C function that - * expects a function pointer of type `LPTHREAD_START_ROUTINE`. GCC supports - * converting stateless lambdas to this format, but clang (as used for IDE - * tooling) does not. + * `std::thread` directly uses pthreads. This means that, like with + * `CreateThread()`, some thread local information does not get initialized + * which can lead to memory errors. + * + * TODO: Once these changes are complete, check if we can drop `PluginContext` + * again and execute all 'safe' opcodes on the calling thread. * * @note This should be used instead of `std::thread` or `std::jthread` whenever * the thread directly calls third party library code, i.e. `LoadLibrary()`, @@ -145,19 +151,38 @@ class Win32Thread { Win32Thread(); /** - * Constructor that immediately starts running the thread + * Constructor that immediately starts running the thread. This works + * equivalently to `std::jthread`. * * @param entry_point The thread entry point that should be run. * @param parameter The parameter passed to the entry point function. - - * @tparam F A function type that should be convertible to a - * `LPTHREAD_START_ROUTINE` function pointer. */ - template - Win32Thread(F entry_point, void* parameter) - : handle(CreateThread(nullptr, 0, entry_point, parameter, 0, nullptr), + template + Win32Thread(Function&& f, Args&&... args) + : handle(CreateThread( + nullptr, + 0, + reinterpret_cast( + win32_thread_trampoline), + new std::function( + [f = std::move(f), ... args = std::move(args)]() { + std::invoke(f, args...); + }), + 0, + nullptr), CloseHandle) {} + /** + * Join the thread on destruction, just like `std::jthread` does. + */ + ~Win32Thread(); + + Win32Thread(const Win32Thread&) = delete; + Win32Thread& operator=(const Win32Thread&) = delete; + + Win32Thread(Win32Thread&&); + Win32Thread& operator=(Win32Thread&&); + private: /** * The handle for the thread that is running, will be a null pointer if this From 4038e198fe9d866add76f08b888e24d4fe686351 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 17:16:20 +0100 Subject: [PATCH 25/40] Replace the Win32Thread proxy functions Now that we can use lambdas instead. --- src/wine-host/bridges/vst2.cpp | 404 ++++++++++++++++----------------- src/wine-host/bridges/vst2.h | 8 - 2 files changed, 192 insertions(+), 220 deletions(-) diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 648ffe36..ecac52ce 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -50,13 +50,6 @@ const std::set unsafe_opcodes{effOpen, effClose, effEditGetRect, intptr_t VST_CALL_CONV host_callback_proxy(AEffect*, int, int, intptr_t, void*, float); -// We need to use the `CreateThread` WinAPI functions instead of `std::thread` -// to use the correct calling conventions within threads. Otherwise we'll get -// some rare and impossible to debug data races in some particular plugins. -uint32_t WINAPI handle_dispatch_midi_events_proxy(void*); -uint32_t WINAPI handle_parameters_proxy(void*); -uint32_t WINAPI handle_process_replacing_proxy(void*); - /** * Fetch the Vst2Bridge instance stored in one of the two pointers reserved * for the host of the hosted VST plugin. This is sadly needed as a workaround @@ -140,13 +133,200 @@ Vst2Bridge::Vst2Bridge(PluginContext& main_context, // This works functionally identically to the `handle_dispatch()` function, // but this socket will only handle MIDI events and it will handle them // eagerly. This is needed because of Win32 API limitations. - dispatch_midi_events_handler = - Win32Thread(handle_dispatch_midi_events_proxy, this); + dispatch_midi_events_handler = Win32Thread([&]() { + sockets.host_vst_dispatch_midi_events.receive( + std::nullopt, [&](Event& event, bool /*on_main_thread*/) { + 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. We're + // not using `passthrough_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); - parameters_handler = Win32Thread(handle_parameters_proxy, this); + next_audio_buffer_midi_events.push_back( + std::get(event.payload)); + DynamicVstEvents& events = + next_audio_buffer_midi_events.back(); - process_replacing_handler = - Win32Thread(handle_process_replacing_proxy, this); + // 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 = return_value, + .payload = nullptr, + .value_payload = std::nullopt}; + + return response; + } else { + using namespace std::placeholders; + + std::cerr << "[Warning] Received non-MIDI " + "event on MIDI processing thread" + << std::endl; + + // Maybe this should just be a hard error instead, since it + // should never happen + return passthrough_event( + plugin, std::bind(&Vst2Bridge::dispatch_wrapper, this, + _1, _2, _3, _4, _5, _6))(event); + } + }); + }); + + parameters_handler = Win32Thread([&]() { + while (true) { + try { + // Both `getParameter` and `setParameter` functions are passed + // through on this socket since they have a lot of overlap. The + // presence of the `value` field tells us which one we're + // dealing with. + auto request = + read_object(sockets.host_vst_parameters); + if (request.value) { + // `setParameter` + plugin->setParameter(plugin, request.index, *request.value); + + ParameterResult response{std::nullopt}; + write_object(sockets.host_vst_parameters, response); + } else { + // `getParameter` + float value = plugin->getParameter(plugin, request.index); + + ParameterResult response{value}; + write_object(sockets.host_vst_parameters, response); + } + } catch (const boost::system::system_error&) { + // The plugin has cut off communications, so we can shut down + // this host application + break; + } + } + }); + + process_replacing_handler = Win32Thread([&]() { + // These are used as scratch buffers to prevent unnecessary allocations. + // Since don't know in advance whether the host will call + // `processReplacing` or `processDoubleReplacing` we'll just create + // both. + std::vector> output_buffers_single_precision( + plugin->numOutputs); + std::vector> output_buffers_double_precision( + plugin->numOutputs); + + while (true) { + try { + auto request = read_object( + sockets.host_vst_process_replacing, process_buffer); + // Let the plugin process the MIDI events that were received + // since the last buffer, and then clean up those events. This + // approach should not be needed but Kontakt only stores + // pointers to rather than copies of the events. + std::lock_guard lock(next_buffer_midi_events_mutex); + + // Since the host should only be calling one of `process()`, + // processReplacing()` or `processDoubleReplacing()`, we can all + // handle them over the same socket. We pick which one to call + // depending on the type of data we got sent and the plugin's + // reported support for these functions. + std::visit( + overload{ + [&](std::vector>& input_buffers) { + // The process functions expect a `float**` for + // their inputs and their outputs + std::vector inputs; + for (auto& buffer : input_buffers) { + inputs.push_back(buffer.data()); + } + + // We reuse the buffers to avoid some unnecessary + // heap allocations, so we need to make sure the + // buffers are large enough since plugins can change + // their output configuration. The type we're using + // here (single precision floats vs double + // precisioon doubles) should be the same as the one + // we're sending in our response. + std::vector outputs; + output_buffers_single_precision.resize( + plugin->numOutputs); + for (auto& buffer : + output_buffers_single_precision) { + buffer.resize(request.sample_frames); + outputs.push_back(buffer.data()); + } + + // Any plugin made in the last fifteen years or so + // should support `processReplacing`. In the off + // chance it does not we can just emulate this + // behavior ourselves. + if (plugin->processReplacing) { + plugin->processReplacing(plugin, inputs.data(), + outputs.data(), + request.sample_frames); + } else { + // If we zero out this buffer then the behavior + // is the same as `processReplacing`` + for (std::vector& buffer : + output_buffers_single_precision) { + std::fill(buffer.begin(), buffer.end(), + 0.0); + } + + plugin->process(plugin, inputs.data(), + outputs.data(), + request.sample_frames); + } + + AudioBuffers response{ + output_buffers_single_precision, + request.sample_frames}; + write_object(sockets.host_vst_process_replacing, + response, process_buffer); + }, + [&](std::vector>& input_buffers) { + // Exactly the same as the above, but for double + // precision audio + std::vector inputs; + for (auto& buffer : input_buffers) { + inputs.push_back(buffer.data()); + } + + std::vector outputs; + output_buffers_double_precision.resize( + plugin->numOutputs); + for (auto& buffer : + output_buffers_double_precision) { + buffer.resize(request.sample_frames); + outputs.push_back(buffer.data()); + } + + plugin->processDoubleReplacing( + plugin, inputs.data(), outputs.data(), + request.sample_frames); + + AudioBuffers response{ + output_buffers_double_precision, + request.sample_frames}; + write_object(sockets.host_vst_process_replacing, + response, process_buffer); + }}, + request.buffers); + + next_audio_buffer_midi_events.clear(); + } catch (const boost::system::system_error&) { + // The plugin has cut off communications, so we can shut down + // this host application + break; + } + } + }); } bool Vst2Bridge::should_skip_message_loop() const { @@ -191,191 +371,6 @@ void Vst2Bridge::handle_dispatch() { }); } -void Vst2Bridge::handle_dispatch_midi_events() { - sockets.host_vst_dispatch_midi_events.receive( - std::nullopt, [&](Event& event, bool /*on_main_thread*/) { - 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. We're not using - // `passthrough_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); - - next_audio_buffer_midi_events.push_back( - std::get(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 = return_value, - .payload = nullptr, - .value_payload = std::nullopt}; - - return response; - } else { - using namespace std::placeholders; - - std::cerr << "[Warning] Received non-MIDI " - "event on MIDI processing thread" - << std::endl; - - // Maybe this should just be a hard error instead, since it - // should never happen - return passthrough_event( - plugin, std::bind(&Vst2Bridge::dispatch_wrapper, this, _1, - _2, _3, _4, _5, _6))(event); - } - }); -} - -void Vst2Bridge::handle_parameters() { - while (true) { - try { - // Both `getParameter` and `setParameter` functions are passed - // through on this socket since they have a lot of overlap. The - // presence of the `value` field tells us which one we're dealing - // with. - auto request = read_object(sockets.host_vst_parameters); - if (request.value) { - // `setParameter` - plugin->setParameter(plugin, request.index, *request.value); - - ParameterResult response{std::nullopt}; - write_object(sockets.host_vst_parameters, response); - } else { - // `getParameter` - float value = plugin->getParameter(plugin, request.index); - - ParameterResult response{value}; - write_object(sockets.host_vst_parameters, response); - } - } catch (const boost::system::system_error&) { - // The plugin has cut off communications, so we can shut down this - // host application - break; - } - } -} - -void Vst2Bridge::handle_process_replacing() { - // These are used as scratch buffers to prevent unnecessary allocations. - // Since don't know in advance whether the host will call `processReplacing` - // or `processDoubleReplacing` we'll just create both. - std::vector> output_buffers_single_precision( - plugin->numOutputs); - std::vector> output_buffers_double_precision( - plugin->numOutputs); - - while (true) { - try { - auto request = read_object( - sockets.host_vst_process_replacing, process_buffer); - // Let the plugin process the MIDI events that were received since - // the last buffer, and then clean up those events. This approach - // should not be needed but Kontakt only stores pointers to rather - // than copies of the events. - std::lock_guard lock(next_buffer_midi_events_mutex); - - // Since the host should only be calling one of `process()`, - // processReplacing()` or `processDoubleReplacing()`, we can all - // handle them over the same socket. We pick which one to call - // depending on the type of data we got sent and the plugin's - // reported support for these functions. - std::visit( - overload{ - [&](std::vector>& input_buffers) { - // The process functions expect a `float**` for their - // inputs and their outputs - std::vector inputs; - for (auto& buffer : input_buffers) { - inputs.push_back(buffer.data()); - } - - // We reuse the buffers to avoid some unnecessary heap - // allocations, so we need to make sure the buffers are - // large enough since plugins can change their output - // configuration. The type we're using here (single - // precision floats vs double precisioon doubles) should - // be the same as the one we're sending in our response. - std::vector outputs; - output_buffers_single_precision.resize( - plugin->numOutputs); - for (auto& buffer : output_buffers_single_precision) { - buffer.resize(request.sample_frames); - outputs.push_back(buffer.data()); - } - - // Any plugin made in the last fifteen years or so - // should support `processReplacing`. In the off chance - // it does not we can just emulate this behavior - // ourselves. - if (plugin->processReplacing) { - plugin->processReplacing(plugin, inputs.data(), - outputs.data(), - request.sample_frames); - } else { - // If we zero out this buffer then the behavior is - // the same as `processReplacing`` - for (std::vector& buffer : - output_buffers_single_precision) { - std::fill(buffer.begin(), buffer.end(), 0.0); - } - - plugin->process(plugin, inputs.data(), - outputs.data(), - request.sample_frames); - } - - AudioBuffers response{output_buffers_single_precision, - request.sample_frames}; - write_object(sockets.host_vst_process_replacing, - response, process_buffer); - }, - [&](std::vector>& input_buffers) { - // Exactly the same as the above, but for double - // precision audio - std::vector inputs; - for (auto& buffer : input_buffers) { - inputs.push_back(buffer.data()); - } - - std::vector outputs; - output_buffers_double_precision.resize( - plugin->numOutputs); - for (auto& buffer : output_buffers_double_precision) { - buffer.resize(request.sample_frames); - outputs.push_back(buffer.data()); - } - - plugin->processDoubleReplacing(plugin, inputs.data(), - outputs.data(), - request.sample_frames); - - AudioBuffers response{output_buffers_double_precision, - request.sample_frames}; - write_object(sockets.host_vst_process_replacing, - response, process_buffer); - }}, - request.buffers); - - next_audio_buffer_midi_events.clear(); - } catch (const boost::system::system_error&) { - // The plugin has cut off communications, so we can shut down this - // host application - break; - } - } -} - intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin, int opcode, int index, @@ -573,18 +568,3 @@ intptr_t VST_CALL_CONV host_callback_proxy(AEffect* effect, return get_bridge_instance(effect).host_callback(effect, opcode, index, value, data, option); } - -uint32_t WINAPI handle_dispatch_midi_events_proxy(void* instance) { - static_cast(instance)->handle_dispatch_midi_events(); - return 0; -} - -uint32_t WINAPI handle_parameters_proxy(void* instance) { - static_cast(instance)->handle_parameters(); - return 0; -} - -uint32_t WINAPI handle_process_replacing_proxy(void* instance) { - static_cast(instance)->handle_process_replacing(); - return 0; -} diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 7f12527f..55e1cd3d 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -128,14 +128,6 @@ class Vst2Bridge { */ void handle_win32_events(); - // These functions are the entry points for the `*_handler` threads - // defined below. They're defined here because we can't use lambdas with - // WinAPI's `CreateThread` which is needed to support the proper call - // conventions the VST plugins expect. - void handle_dispatch_midi_events(); - void handle_parameters(); - void handle_process_replacing(); - /** * Forward the host callback made by the plugin to the host and return the * results. From 59b57f48daf2d97611deb11268b008e0856f57ad Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 17:30:18 +0100 Subject: [PATCH 26/40] Add a changelog entry for the thread rework --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b04c8e7..1eef173f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,12 @@ Versioning](https://semver.org/spec/v2.0.0.html). TODO: Remove all mentions of `hack_reaper_update_display` from the readme. +### Changed + +- As part of the communication rework the way the Wine process handles threading + has also been completely reworked. although this should not cause any + noticeable changes. + ## [1.7.1] - 2020-10-23 ### Fixed From a6b5951d81f9776ea5bd9d516bc59adbd411c977 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 17:38:13 +0100 Subject: [PATCH 27/40] Replace std::jthread with Win32Thread everywhere --- src/wine-host/bridges/group.cpp | 7 ++++--- src/wine-host/bridges/group.h | 4 ++-- src/wine-host/individual-host.cpp | 15 +++++++-------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index 8a42c56d..7cc08034 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -98,7 +98,7 @@ GroupBridge::GroupBridge(boost::filesystem::path group_socket_path) async_log_pipe_lines(stdout_redirect.pipe, stdout_buffer, "[STDOUT] "); async_log_pipe_lines(stderr_redirect.pipe, stderr_buffer, "[STDERR] "); - stdio_handler = std::jthread([&]() { stdio_context.run(); }); + stdio_handler = Win32Thread([&]() { stdio_context.run(); }); } GroupBridge::~GroupBridge() { @@ -128,7 +128,8 @@ void GroupBridge::handle_plugin_dispatch(size_t plugin_id) { boost::asio::post(plugin_context.context, [this, plugin_id]() { std::lock_guard lock(active_plugins_mutex); - // The join is implicit because we're using std::jthread + // The join is implicit because we're using Win32Thread (which mimics + // std::jthread) active_plugins.erase(plugin_id); }); @@ -219,7 +220,7 @@ void GroupBridge::accept_requests() { // is only used within this context we don't need any locks. const size_t plugin_id = next_plugin_id.fetch_add(1); active_plugins[plugin_id] = - std::pair(std::jthread([this, plugin_id]() { + std::pair(Win32Thread([this, plugin_id]() { handle_plugin_dispatch(plugin_id); }), std::move(bridge)); diff --git a/src/wine-host/bridges/group.h b/src/wine-host/bridges/group.h index 52b43138..5c2a8d90 100644 --- a/src/wine-host/bridges/group.h +++ b/src/wine-host/bridges/group.h @@ -244,7 +244,7 @@ class GroupBridge { /** * A thread that runs the `stdio_context` loop. */ - std::jthread stdio_handler; + Win32Thread stdio_handler; boost::asio::local::stream_protocol::endpoint group_socket_endpoint; /** @@ -263,7 +263,7 @@ class GroupBridge { * on `next_plugin_id`. */ std::unordered_map>> + std::pair>> active_plugins; /** * A counter for the next unique plugin ID. When hosting a new plugin we'll diff --git a/src/wine-host/individual-host.cpp b/src/wine-host/individual-host.cpp index 087372f0..e7782b3f 100644 --- a/src/wine-host/individual-host.cpp +++ b/src/wine-host/individual-host.cpp @@ -60,13 +60,12 @@ int __cdecl main(int argc, char* argv[]) { << std::endl; // As explained in `Vst2Bridge`, the plugin has to be initialized in the - // same thread as the one that calls `io_context.run()`. And for some - // reason, a lot of plugins have memory corruption issues when executing - // `LoadLibrary()` or some of their functions from within a `std::thread` - // (although the WinAPI `CreateThread()` does not have these issues). This - // setup is slightly more convoluted than it has to be, but doing it this - // way we don't need to differentiate between individually hosted plugins - // and plugin groups when it comes to event handling. + // same thread as the one that calls `io_context.run()`. This setup is + // slightly more convoluted than it has to be, but doing it this way we + // don't need to differentiate between individually hosted plugins and + // plugin groups when it comes to event handling. + // TODO: Update documentation once we figure out if we can safely replace + // PluginContext again with a normal `io_context`. PluginContext plugin_context{}; std::unique_ptr bridge; try { @@ -84,7 +83,7 @@ int __cdecl main(int argc, char* argv[]) { // We'll listen for `dispatcher()` calls on a different thread, but the // actual events will still be executed within the IO context - std::jthread dispatch_handler([&]() { bridge->handle_dispatch(); }); + Win32Thread dispatch_handler([&]() { bridge->handle_dispatch(); }); // Handle Win32 messages and X11 events on a timer, just like in // `GroupBridge::async_handle_events()`` From bafc36614b6e2e478459681866ffa02409761a2b Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 18:28:23 +0100 Subject: [PATCH 28/40] Properly forward arguments in Win32Thread --- src/wine-host/utils.cpp | 8 ------- src/wine-host/utils.h | 47 ++++++++++++++++++++++++----------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index 7d8d86c8..981bd3f2 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -25,14 +25,6 @@ uint32_t WINAPI win32_thread_trampoline(std::function* entry_point) { return 0; } -Win32Thread::~Win32Thread() { - // Imitate std::jthread's joining behaviour, the thread handle will be - // cleaned up by the `std::unique_ptr` - if (handle) { - WaitForSingleObject(handle.get(), INFINITE); - } -} - Win32Thread::Win32Thread(Win32Thread&& o) : handle(std::move(o.handle)) {} Win32Thread& Win32Thread::operator=(Win32Thread&& o) { diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index b5c04e6e..df8c263b 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -127,9 +127,18 @@ class PluginContext { */ uint32_t WINAPI win32_thread_trampoline(std::function* entry_point); +/** + * Taken from the C++ reference: + * https://en.cppreference.com/w/cpp/thread/jthread + */ +template +std::decay_t decay_copy(T&& v) { + return std::forward(v); +} + /** * A simple RAII wrapper around the Win32 thread API that imitates - * `std::jthread`. + * `std::thread`. * * `std::thread` directly uses pthreads. This means that, like with * `CreateThread()`, some thread local information does not get initialized @@ -152,30 +161,30 @@ class Win32Thread { /** * Constructor that immediately starts running the thread. This works - * equivalently to `std::jthread`. + * equivalently to `std::thread`. * * @param entry_point The thread entry point that should be run. * @param parameter The parameter passed to the entry point function. */ template Win32Thread(Function&& f, Args&&... args) - : handle(CreateThread( - nullptr, - 0, - reinterpret_cast( - win32_thread_trampoline), - new std::function( - [f = std::move(f), ... args = std::move(args)]() { - std::invoke(f, args...); - }), - 0, - nullptr), - CloseHandle) {} - - /** - * Join the thread on destruction, just like `std::jthread` does. - */ - ~Win32Thread(); + : handle( + CreateThread(nullptr, + 0, + reinterpret_cast( + win32_thread_trampoline), + // We'll capture the function by move since the + // lambda will often go out of scope in the time that + // the thread is starting. Alternatively we could + // wait for the thread to be up before continuing, + // that may be a bit safer. + new std::function([&, f = std::move(f)]() { + std::invoke( + f, decay_copy(std::forward(args))...); + }), + 0, + nullptr), + CloseHandle) {} Win32Thread(const Win32Thread&) = delete; Win32Thread& operator=(const Win32Thread&) = delete; From 28886e7073331a46d8ffa64e7421606bb8fb7f0f Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 18:29:38 +0100 Subject: [PATCH 29/40] Replace all threads with Win32Thread in Wine host --- src/common/communication.cpp | 94 ---------------------------- src/common/communication.h | 118 ++++++++++++++++++++++++++++++----- src/plugin/host-process.cpp | 4 +- src/plugin/host-process.h | 8 +-- src/plugin/plugin-bridge.h | 2 +- src/wine-host/bridges/vst2.h | 2 +- 6 files changed, 111 insertions(+), 117 deletions(-) diff --git a/src/common/communication.cpp b/src/common/communication.cpp index 543202ae..1ad60440 100644 --- a/src/common/communication.cpp +++ b/src/common/communication.cpp @@ -81,100 +81,6 @@ intptr_t DefaultDataConverter::return_value(const int /*opcode*/, return original; } -EventHandler::EventHandler( - boost::asio::io_context& io_context, - boost::asio::local::stream_protocol::endpoint endpoint, - bool listen) - : io_context(io_context), endpoint(endpoint), socket(io_context) { - if (listen) { - fs::create_directories(fs::path(endpoint.path()).parent_path()); - acceptor.emplace(io_context, endpoint); - } -} - -void EventHandler::connect() { - if (acceptor) { - acceptor->accept(socket); - - // As mentioned in `acceptor's` docstring, this acceptor will be - // recreated in `receive()` on another context, and potentially on the - // other side of the connection in the case of `vst_host_callback` - acceptor.reset(); - fs::remove(endpoint.path()); - } else { - socket.connect(endpoint); - } -} - -void EventHandler::close() { - socket.shutdown(boost::asio::local::stream_protocol::socket::shutdown_both); - socket.close(); -} - -Sockets::Sockets(boost::asio::io_context& io_context, - const boost::filesystem::path& endpoint_base_dir, - bool listen) - : base_dir(endpoint_base_dir), - host_vst_dispatch(io_context, - (base_dir / "host_vst_dispatch.sock").string(), - listen), - host_vst_dispatch_midi_events( - io_context, - (base_dir / "host_vst_dispatch_midi_events.sock").string(), - listen), - vst_host_callback(io_context, - (base_dir / "vst_host_callback.sock").string(), - listen), - host_vst_parameters(io_context), - host_vst_process_replacing(io_context), - host_vst_control(io_context), - host_vst_parameters_endpoint( - (base_dir / "host_vst_parameters.sock").string()), - host_vst_process_replacing_endpoint( - (base_dir / "host_vst_process_replacing.sock").string()), - host_vst_control_endpoint((base_dir / "host_vst_control.sock").string()) { - if (listen) { - fs::create_directories(base_dir); - - acceptors = Acceptors{ - .host_vst_parameters{io_context, host_vst_parameters_endpoint}, - .host_vst_process_replacing{io_context, - host_vst_process_replacing_endpoint}, - .host_vst_control{io_context, host_vst_control_endpoint}, - }; - } -} - -Sockets::~Sockets() { - // Only clean if we're the ones who have created these files, although it - // should not cause any harm to also do this on the Wine side - if (acceptors) { - try { - fs::remove_all(base_dir); - } catch (const fs::filesystem_error&) { - // There should not be any filesystem errors since only one side - // removes the files, but if we somehow can't delete the file then - // we can just silently ignore this - } - } -} - -void Sockets::connect() { - host_vst_dispatch.connect(); - host_vst_dispatch_midi_events.connect(); - vst_host_callback.connect(); - if (acceptors) { - acceptors->host_vst_parameters.accept(host_vst_parameters); - acceptors->host_vst_process_replacing.accept( - host_vst_process_replacing); - acceptors->host_vst_control.accept(host_vst_control); - } else { - host_vst_parameters.connect(host_vst_parameters_endpoint); - host_vst_process_replacing.connect(host_vst_process_replacing_endpoint); - host_vst_control.connect(host_vst_control_endpoint); - } -} - boost::filesystem::path generate_endpoint_base(const std::string& plugin_name) { fs::path temp_directory = get_temporary_directory(); diff --git a/src/common/communication.h b/src/common/communication.h index 5664f551..4df0966a 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -18,7 +18,6 @@ #include #include -#include #include #include @@ -204,7 +203,11 @@ class DefaultDataConverter { * above. Similarly, the `EventHandler::receive()` method first sets up * asynchronous listeners for the socket endpoint, and then block and handle * events until the main socket is closed. + * + * @tparam Thread The thread implementation to use. On the Linux side this + * should be `std::jthread` and on the Wine side this should be `Win32Thread`. */ +template class EventHandler { public: /** @@ -223,20 +226,44 @@ class EventHandler { */ EventHandler(boost::asio::io_context& io_context, boost::asio::local::stream_protocol::endpoint endpoint, - bool listen); + bool listen) + : io_context(io_context), endpoint(endpoint), socket(io_context) { + if (listen) { + boost::filesystem::create_directories( + boost::filesystem::path(endpoint.path()).parent_path()); + acceptor.emplace(io_context, endpoint); + } + } /** * Depending on the value of the `listen` argument passed to the * constructor, either accept connections made to the sockets on the Linux * side or connect to the sockets on the Wine side */ - void connect(); + void connect() { + if (acceptor) { + acceptor->accept(socket); + + // As mentioned in `acceptor's` docstring, this acceptor will be + // recreated in `receive()` on another context, and potentially on + // the other side of the connection in the case of + // `vst_host_callback` + acceptor.reset(); + boost::filesystem::remove(endpoint.path()); + } else { + socket.connect(endpoint); + } + } /** * Close the socket. Both sides that are actively listening will be thrown a * `boost::system_error` when this happens. */ - void close(); + void close() { + socket.shutdown( + boost::asio::local::stream_protocol::socket::shutdown_both); + socket.close(); + } /** * Serialize and send an event over a socket. This is used for both the host @@ -384,7 +411,7 @@ class EventHandler { // This works the exact same was as `active_plugins` and // `next_plugin_id` in `GroupBridge` - std::map active_secondary_requests{}; + std::map active_secondary_requests{}; std::atomic_size_t next_request_id{}; std::mutex active_secondary_requests_mutex{}; accept_requests( @@ -395,7 +422,7 @@ class EventHandler { // We have to make sure to keep moving these sockets into the // threads that will handle them std::lock_guard lock(active_secondary_requests_mutex); - active_secondary_requests[request_id] = std::jthread( + active_secondary_requests[request_id] = Thread( [&, request_id](boost::asio::local::stream_protocol::socket secondary_socket) { // TODO: Factor this out @@ -427,15 +454,14 @@ class EventHandler { active_secondary_requests_mutex); // The join is implicit because we're using - // std::jthread + // std::jthread/Win32Thread active_secondary_requests.erase(request_id); }); }, std::move(secondary_socket)); }); - std::jthread secondary_requests_handler( - [&]() { secondary_context.run(); }); + Thread secondary_requests_handler([&]() { secondary_context.run(); }); while (true) { try { @@ -555,7 +581,11 @@ class EventHandler { * `true` before launching the Wine VST host. This will start listening on the * sockets, and the call to `connect()` will then accept any incoming * connections. + * + * @tparam Thread The thread implementation to use. On the Linux side this + * should be `std::jthread` and on the Wine side this should be `Win32Thread`. */ +template class Sockets { public: /** @@ -574,20 +604,78 @@ class Sockets { */ Sockets(boost::asio::io_context& io_context, const boost::filesystem::path& endpoint_base_dir, - bool listen); + bool listen) + : base_dir(endpoint_base_dir), + host_vst_dispatch(io_context, + (base_dir / "host_vst_dispatch.sock").string(), + listen), + host_vst_dispatch_midi_events( + io_context, + (base_dir / "host_vst_dispatch_midi_events.sock").string(), + listen), + vst_host_callback(io_context, + (base_dir / "vst_host_callback.sock").string(), + listen), + host_vst_parameters(io_context), + host_vst_process_replacing(io_context), + host_vst_control(io_context), + host_vst_parameters_endpoint( + (base_dir / "host_vst_parameters.sock").string()), + host_vst_process_replacing_endpoint( + (base_dir / "host_vst_process_replacing.sock").string()), + host_vst_control_endpoint( + (base_dir / "host_vst_control.sock").string()) { + if (listen) { + boost::filesystem::create_directories(base_dir); + + acceptors = Acceptors{ + .host_vst_parameters{io_context, host_vst_parameters_endpoint}, + .host_vst_process_replacing{ + io_context, host_vst_process_replacing_endpoint}, + .host_vst_control{io_context, host_vst_control_endpoint}, + }; + } + } /** * Cleans up the directory containing the socket endpoints when yabridge * shuts down if it still exists. */ - ~Sockets(); + ~Sockets() { + // Only clean if we're the ones who have created these files, although + // it should not cause any harm to also do this on the Wine side + if (acceptors) { + try { + boost::filesystem::remove_all(base_dir); + } catch (const boost::filesystem::filesystem_error&) { + // There should not be any filesystem errors since only one side + // removes the files, but if we somehow can't delete the file + // then we can just silently ignore this + } + } + } /** * Depending on the value of the `listen` argument passed to the * constructor, either accept connections made to the sockets on the Linux * side or connect to the sockets on the Wine side */ - void connect(); + void connect() { + host_vst_dispatch.connect(); + host_vst_dispatch_midi_events.connect(); + vst_host_callback.connect(); + if (acceptors) { + acceptors->host_vst_parameters.accept(host_vst_parameters); + acceptors->host_vst_process_replacing.accept( + host_vst_process_replacing); + acceptors->host_vst_control.accept(host_vst_control); + } else { + host_vst_parameters.connect(host_vst_parameters_endpoint); + host_vst_process_replacing.connect( + host_vst_process_replacing_endpoint); + host_vst_control.connect(host_vst_control_endpoint); + } + } /** * The base directory for our socket endpoints. All `*_endpoint` variables @@ -604,19 +692,19 @@ class Sockets { * The socket that forwards all `dispatcher()` calls from the VST host to * the plugin. */ - EventHandler host_vst_dispatch; + EventHandler host_vst_dispatch; /** * Used specifically for the `effProcessEvents` opcode. This is needed * because the Win32 API is designed to block during certain GUI * interactions such as resizing a window or opening a dropdown. Without * this MIDI input would just stop working at times. */ - EventHandler host_vst_dispatch_midi_events; + EventHandler host_vst_dispatch_midi_events; /** * The socket that forwards all `audioMaster()` calls from the Windows VST * plugin to the host. */ - EventHandler vst_host_callback; + EventHandler vst_host_callback; /** * Used for both `getParameter` and `setParameter` since they mostly * overlap. diff --git a/src/plugin/host-process.cpp b/src/plugin/host-process.cpp index fa46d623..49c0492a 100644 --- a/src/plugin/host-process.cpp +++ b/src/plugin/host-process.cpp @@ -83,7 +83,7 @@ void HostProcess::async_log_pipe_lines(patched_async_pipe& pipe, IndividualHost::IndividualHost(boost::asio::io_context& io_context, Logger& logger, fs::path plugin_path, - const Sockets& sockets) + const Sockets& sockets) : HostProcess(io_context, logger), plugin_arch(find_vst_architecture(plugin_path)), host_path(find_vst_host(plugin_arch, false)), @@ -130,7 +130,7 @@ void IndividualHost::terminate() { GroupHost::GroupHost(boost::asio::io_context& io_context, Logger& logger, fs::path plugin_path, - Sockets& sockets, + Sockets& sockets, std::string group_name) : HostProcess(io_context, logger), plugin_arch(find_vst_architecture(plugin_path)), diff --git a/src/plugin/host-process.h b/src/plugin/host-process.h index 840b3a74..5826c987 100644 --- a/src/plugin/host-process.h +++ b/src/plugin/host-process.h @@ -25,8 +25,8 @@ #include #include -#include "../common/logging.h" #include "../common/communication.h" +#include "../common/logging.h" #include "utils.h" /** @@ -127,7 +127,7 @@ class IndividualHost : public HostProcess { IndividualHost(boost::asio::io_context& io_context, Logger& logger, boost::filesystem::path plugin_path, - const Sockets& sockets); + const Sockets& sockets); PluginArchitecture architecture() override; boost::filesystem::path path() override; @@ -169,7 +169,7 @@ class GroupHost : public HostProcess { GroupHost(boost::asio::io_context& io_context, Logger& logger, boost::filesystem::path plugin_path, - Sockets& socket_endpoint, + Sockets& socket_endpoint, std::string group_name); PluginArchitecture architecture() override; @@ -192,7 +192,7 @@ class GroupHost : public HostProcess { * The associated sockets for the plugin we're hosting. This is used to * terminate the plugin. */ - Sockets& sockets; + Sockets& sockets; /** * A thread that waits for the group host to have started and then ask it to diff --git a/src/plugin/plugin-bridge.h b/src/plugin/plugin-bridge.h index e62fc750..468c8fd6 100644 --- a/src/plugin/plugin-bridge.h +++ b/src/plugin/plugin-bridge.h @@ -124,7 +124,7 @@ class PluginBridge { void log_init_message(); boost::asio::io_context io_context; - Sockets sockets; + Sockets sockets; /** * The thread that handles host callbacks. diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 55e1cd3d..ba02d8d0 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -189,7 +189,7 @@ class Vst2Bridge { /** * All sockets used for communicating with this specific plugin. */ - Sockets sockets; + Sockets sockets; /** * The thread that specifically handles `effProcessEvents` opcodes so the From 1a18ea861444381e7245eed5ee208fd16057c638 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 22:52:57 +0100 Subject: [PATCH 30/40] Add a dependency for function2 std::function requires the function to be CopyConstructable and thus does not allow you to capture by move, which is exactly what we need. --- README.md | 1 + meson.build | 5 +++++ subprojects/function2-patch-4.1.0.tar.xz | Bin 0 -> 2184 bytes subprojects/function2.wrap | 10 ++++++++++ 4 files changed, 16 insertions(+) create mode 100644 subprojects/function2-patch-4.1.0.tar.xz create mode 100644 subprojects/function2.wrap diff --git a/README.md b/README.md index 465933d6..b296679f 100644 --- a/README.md +++ b/README.md @@ -499,6 +499,7 @@ the following dependencies: The following dependencies are included in the repository as a Meson wrap: - bitsery +- function2 - tomlplusplus The project can then be compiled as follows: diff --git a/meson.build b/meson.build index bade536b..b0eae740 100644 --- a/meson.build +++ b/meson.build @@ -73,6 +73,7 @@ boost_filesystem_dep = dependency( static : with_static_boost ) bitsery_dep = subproject('bitsery').get_variable('bitsery_dep') +function2_dep = subproject('function2').get_variable('function2_dep') threads_dep = dependency('threads') tomlplusplus_dep = subproject('tomlplusplus', version : '2.1.0').get_variable('tomlplusplus_dep') # The built in threads dependency does not know how to handle winegcc @@ -141,6 +142,7 @@ executable( boost_dep, boost_filesystem_dep, bitsery_dep, + function2_dep, tomlplusplus_dep, wine_threads_dep, xcb_dep @@ -158,6 +160,7 @@ executable( boost_dep, boost_filesystem_dep, bitsery_dep, + function2_dep, tomlplusplus_dep, wine_threads_dep, xcb_dep @@ -203,6 +206,7 @@ if with_bitbridge boost_dep, boost_filesystem_dep, bitsery_dep, + function2_dep, tomlplusplus_dep, wine_threads_dep, xcb_dep @@ -227,6 +231,7 @@ if with_bitbridge boost_dep, boost_filesystem_dep, bitsery_dep, + function2_dep, tomlplusplus_dep, wine_threads_dep, xcb_dep diff --git a/subprojects/function2-patch-4.1.0.tar.xz b/subprojects/function2-patch-4.1.0.tar.xz new file mode 100644 index 0000000000000000000000000000000000000000..141ec0d3fc8711c79075c927c7438cdc18220809 GIT binary patch literal 2184 zcmV;32zU4WH+ooF000E$*0e?f03iVu0001VFXf})PyYxvv3N&l%~`x6G^{^S`1 zn*1a?>89t8VP4QTLf5&1YHA}#TMD`n)pUW}s{{#_as2K$0DmkiDYpd$izB4PO$C{6 z(mfVr@kib9#u&eVhbH3B1Q7e8X z^vGJjnT>JF$O_!yT_yc^NGYI?*f``RB;o*nW3wBX|Mx%1C6JBeB~y^)r4q5HM&Mhn z3`iPCRz_SqJzi^I7J$Z9k17!>BV4@qY7{}^h0`n$V1I(8a(!kxcs0J|AMzB z5mE7=k=jvMw|$!D*mzMa+o*g1_s$GX{AfVeIS7`30d04_4 zS}AUmpkhJ>7hSMmjg=cu4LtM{ak9($ugyPm&wr0CI2waJ;vw10T7VGTp{J@DY=)m> z;kBz?dRnHxSXt-u?yUs}Hah>^s1EsjQ5!IVBK;pQW@H5~GNPpK9rqwxc^!lc1Rc6? zNh+A6w}skuGp9d?HA9Z_y$v4u@gA<~!Wray%qd;-?9fIn+h{K$!w-(=MXvGNifc&2 z1Bzggyg9BHnXE|cZ4W-^eL&6?u|yA)29bKlNNMuNiJfRW+WINbn{#eYt_!s3$Q6b6 z<*>2i`&}yXPL(T8I@I`7O(*0(ZzAWwZDAXdRh<*;p3US>GhDtn-G^;9LW$OHR=M)pqQb8K*E9<(OnK&l>svy-km(zL;|$UfOzHdJWgqi+NLPHY}^I?OyTnFqvr`Ve)DRu;=#AngwBnbM&_F)3J5R z`8=;w+=5xsy__i{k9JQCW1CCC?sERv6HE<*m0m`B4F!!+*slGxr+=}*Mp3R9r$I~ zc77Tgkst#X%cnq-0nGKDOd;e4U16zP=h0fSp^&cJE~@tqE@mVK+dxz;UbDpdK_kIT zOi)pyzRoVU_fcDpHLM9O5CKa4nTOM0j2Ru|x=9v4PUR$M4`~hLOard#D&d+qb%y8KR7zXLZdYJWCszR8^)PjU0?CS zPO^BQHM^-v`w&fAVm5BuzyeMJ{e`aH0a*|A$Uw|JgByHb&DZh0H3S--y>N}5mVr7R z;fVId!_2HuI!xClr=M$K)#2El%-H1`M@`DOJ8d#;*wp-}&6pqD7wwv4N{ zZc#(60*F{{$QnS}qX)&dOs90CT2P%=Q2m|;Kku9!M_{G1-mqCy+jYfZBLF{ z5np$U`#rWU+bf>U`;lYa!p$+Lk~R`+&?Z-76;fw5YGF6~p!uOrHR>!bJ=5I1Ow7Y! zGDX#R+iO!iIv)tZYgF+=XWe0lv{ekC@*f{P00pk#WpaB4B@LwLW#Ao{g K000001X)@I+$sP7 literal 0 HcmV?d00001 diff --git a/subprojects/function2.wrap b/subprojects/function2.wrap new file mode 100644 index 00000000..bb59c231 --- /dev/null +++ b/subprojects/function2.wrap @@ -0,0 +1,10 @@ +[wrap-file] +directory = function2-4.1.0 + +source_url = https://github.com/Naios/function2/archive/4.1.0.tar.gz +source_filename = function2-4.1.0.tar.gz +source_hash = c3aaeaf93bf90c0f4505a18f1094b51fe28881ce202c3bf78ec4efb336c51981 + +patch_url = file:./subprojects/function2-patch-4.1.0.tar.xz +patch_filename = function2-patch-4.1.0.tar.xz +patch_hash = 4b966afd862413ea1f3d96484e74401992ec958f1ee2b4cc161f3cb7c36fe7ba From eb8d4ae1d89822efc7ac57ade208afbd5f251e6e Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 22:53:59 +0100 Subject: [PATCH 31/40] Fix Win32Thread not capturing by move std::function does not allow non-movable lambdas, so capturing by move doesn't work there. And the old solution of course has issues with dangling pointers (but because this is C++ the compiler still thinks it's A-Ok). --- src/wine-host/utils.cpp | 3 ++- src/wine-host/utils.h | 50 ++++++++++++++++++----------------------- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index 981bd3f2..172996c0 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -18,7 +18,8 @@ PluginContext::PluginContext() : context(), events_timer(context) {} -uint32_t WINAPI win32_thread_trampoline(std::function* entry_point) { +uint32_t WINAPI +win32_thread_trampoline(fu2::unique_function* entry_point) { (*entry_point)(); delete entry_point; diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index df8c263b..81f5c8e1 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -29,6 +29,7 @@ #include #include +#include /** * The delay between calls to the event loop at an even more than cinematic 30 @@ -122,19 +123,11 @@ class PluginContext { * We can't store the function pointer in the `Win32Thread` object because * moving a `Win32Thread` object would then cause issues. * - * @param win32_thread_trampoline A `std::function*` pointer to a - * function pointer, great. + * @param entry_point A `fu2::unique_function*` pointer to a function + * pointer, great. */ -uint32_t WINAPI win32_thread_trampoline(std::function* entry_point); - -/** - * Taken from the C++ reference: - * https://en.cppreference.com/w/cpp/thread/jthread - */ -template -std::decay_t decay_copy(T&& v) { - return std::forward(v); -} +uint32_t WINAPI +win32_thread_trampoline(fu2::unique_function* entry_point); /** * A simple RAII wrapper around the Win32 thread API that imitates @@ -166,24 +159,25 @@ class Win32Thread { * @param entry_point The thread entry point that should be run. * @param parameter The parameter passed to the entry point function. */ - template + template Win32Thread(Function&& f, Args&&... args) : handle( - CreateThread(nullptr, - 0, - reinterpret_cast( - win32_thread_trampoline), - // We'll capture the function by move since the - // lambda will often go out of scope in the time that - // the thread is starting. Alternatively we could - // wait for the thread to be up before continuing, - // that may be a bit safer. - new std::function([&, f = std::move(f)]() { - std::invoke( - f, decay_copy(std::forward(args))...); - }), - 0, - nullptr), + CreateThread( + nullptr, + 0, + reinterpret_cast( + win32_thread_trampoline), + // `std::function` does not support functions with move + // captures the function has to be copy-constructable. + // Function2's unique_function lets us capture and move our + // arguments to the lambda so we don't end up with dangling + // references. + new fu2::unique_function( + [f = std::move(f), ... args = std::move(args)]() mutable { + f(std::move(args)...); + }), + 0, + nullptr), CloseHandle) {} Win32Thread(const Win32Thread&) = delete; From fac820c25ab2dc19e305636cd72e559db71f9cbf Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Tue, 27 Oct 2020 23:18:59 +0100 Subject: [PATCH 32/40] Execute all non-unsafe opcodes on calling thread This will require more testing of course, but I think it should be safe. This would increase the potential maximal throughput in group hosts significantly. --- src/wine-host/bridges/vst2.cpp | 19 +++++++------------ src/wine-host/utils.h | 3 --- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index ecac52ce..4093dd21 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -335,7 +335,7 @@ bool Vst2Bridge::should_skip_message_loop() const { void Vst2Bridge::handle_dispatch() { sockets.host_vst_dispatch.receive( - std::nullopt, [&](Event& event, bool on_main_thread) { + std::nullopt, [&](Event& event, bool /*on_main_thread*/) { // TODO: As per the TODO in `passthrough_event`, this can use a // round of refactoring now that we never use its returned // lambda directly anymore @@ -343,17 +343,12 @@ void Vst2Bridge::handle_dispatch() { plugin, [&](AEffect* plugin, int opcode, int index, intptr_t value, void* data, float option) -> intptr_t { - // On the main thread, instead of running - // `plugin->dispatcher()` (or `dispatch_wrapper()`) - // directly, we'll run the function within the IO context so - // all events will be executed on the same thread as the one - // that runs the Win32 message loop. In some scenarios we'll - // receive incoming calls from multiple threads or we'll - // receive calls while we're currently stuck in the Win32 - // message loop. In those cases we'll assume that these - // events can be safely handled directlyfrom another thread. - if (unsafe_opcodes.contains(opcode) || - (on_main_thread && !plugin_context.event_loop_active)) { + // Certain functions will most definitely involve the GUI or + // the Win32 message loop. These functions have to be + // performed on the thread that is running the IO context, + // since this is also where the plugins were instantiated + // and where the Win32 message loop is handled. + if (unsafe_opcodes.contains(opcode)) { std::promise dispatch_result; boost::asio::dispatch(plugin_context.context, [&]() { const intptr_t result = dispatch_wrapper( diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index 81f5c8e1..b442ed0d 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -137,9 +137,6 @@ win32_thread_trampoline(fu2::unique_function* entry_point); * `CreateThread()`, some thread local information does not get initialized * which can lead to memory errors. * - * TODO: Once these changes are complete, check if we can drop `PluginContext` - * again and execute all 'safe' opcodes on the calling thread. - * * @note This should be used instead of `std::thread` or `std::jthread` whenever * the thread directly calls third party library code, i.e. `LoadLibrary()`, * `FreeLibrary()`, the plugin's entry point, or any of the `AEffect::*()` From bece654c2df3cfb3a6106f8037a1a7d567b6e299 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 01:02:56 +0100 Subject: [PATCH 33/40] Rename PluginContext to MainContext for clarity --- src/wine-host/bridges/group.cpp | 18 +++++++++--------- src/wine-host/bridges/group.h | 12 ++++++------ src/wine-host/bridges/vst2.cpp | 8 ++++---- src/wine-host/bridges/vst2.h | 4 ++-- src/wine-host/individual-host.cpp | 10 +++++----- src/wine-host/utils.cpp | 18 +++++++++--------- src/wine-host/utils.h | 23 ++++++++++++----------- 7 files changed, 47 insertions(+), 46 deletions(-) diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index 7cc08034..bb4eb6d5 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -84,14 +84,14 @@ StdIoCapture::~StdIoCapture() { GroupBridge::GroupBridge(boost::filesystem::path group_socket_path) : logger(Logger::create_from_environment( create_logger_prefix(group_socket_path))), - plugin_context(), + main_context(), stdio_context(), stdout_redirect(stdio_context, STDOUT_FILENO), stderr_redirect(stdio_context, STDERR_FILENO), group_socket_endpoint(group_socket_path.string()), - group_socket_acceptor(create_acceptor_if_inactive(plugin_context.context, + group_socket_acceptor(create_acceptor_if_inactive(main_context.context, group_socket_endpoint)), - shutdown_timer(plugin_context.context) { + shutdown_timer(main_context.context) { // Write this process's original STDOUT and STDERR streams to the logger // TODO: This works for output generated by plugins, but not for debug // messages generated by wineserver. Is it possible to catch those? @@ -125,7 +125,7 @@ void GroupBridge::handle_plugin_dispatch(size_t plugin_id) { // potentially corrupt our heap. This way we can also properly join the // thread again. If no active plugins remain, then we'll terminate the // process. - boost::asio::post(plugin_context.context, [this, plugin_id]() { + boost::asio::post(main_context.context, [this, plugin_id]() { std::lock_guard lock(active_plugins_mutex); // The join is implicit because we're using Win32Thread (which mimics @@ -147,7 +147,7 @@ void GroupBridge::handle_plugin_dispatch(size_t plugin_id) { if (active_plugins.size() == 0) { logger.log( "All plugins have exited, shutting down the group process"); - plugin_context.stop(); + main_context.stop(); } }); } @@ -158,7 +158,7 @@ void GroupBridge::handle_incoming_connections() { logger.log( "Group host is up and running, now accepting incoming connections"); - plugin_context.run(); + main_context.run(); } bool GroupBridge::should_skip_message_loop() { @@ -188,7 +188,7 @@ void GroupBridge::accept_requests() { logger.log("Error while listening for incoming connections:"); logger.log(error.message()); - plugin_context.stop(); + main_context.stop(); } // Read the parameters, and then host the plugin in this process @@ -208,7 +208,7 @@ void GroupBridge::accept_requests() { request.endpoint_base_dir + "'"); try { auto bridge = std::make_unique( - plugin_context, request.plugin_path, + main_context, request.plugin_path, request.endpoint_base_dir); logger.log("Finished initializing '" + request.plugin_path + "'"); @@ -235,7 +235,7 @@ void GroupBridge::accept_requests() { } void GroupBridge::async_handle_events() { - plugin_context.async_handle_events([&]() { + main_context.async_handle_events([&]() { { // Always handle X11 events std::lock_guard lock(active_plugins_mutex); diff --git a/src/wine-host/bridges/group.h b/src/wine-host/bridges/group.h index 5c2a8d90..13f4fcf1 100644 --- a/src/wine-host/bridges/group.h +++ b/src/wine-host/bridges/group.h @@ -179,7 +179,7 @@ class GroupBridge { * event handling and message loop interaction also has to be done from that * thread, which is why we initialize the plugin here and use the * `handle_dispatch()` function to run events within the same - * `plugin_context`. + * `main_context`. * * @see handle_plugin_dispatch */ @@ -217,13 +217,13 @@ class GroupBridge { * operations that may involve the Win32 mesasge loop (e.g. initialization * and most `AEffect::dispatcher()` calls) should be run on. */ - PluginContext plugin_context; + MainContext main_context; /** * A seperate IO context that handles the STDIO redirect through - * `StdIoCapture`. This is seperated the `plugin_context` above so that - * STDIO capture does not get blocked by blocking GUI operations. Since - * every GUI related operation should be run from the same thread, we can't - * just add another thread to the main IO context. + * `StdIoCapture`. This is separated from the `main_context` above so that + * STDIO capture does not get blocked by GUI operations. Since every GUI + * related operation should be run from the same thread, we can't just add + * another thread to the main IO context. */ boost::asio::io_context stdio_context; diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index 4093dd21..4f2f7601 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -66,13 +66,13 @@ Vst2Bridge& get_bridge_instance(const AEffect* plugin) { return *static_cast(plugin->ptr1); } -Vst2Bridge::Vst2Bridge(PluginContext& main_context, +Vst2Bridge::Vst2Bridge(MainContext& main_context, std::string plugin_dll_path, std::string endpoint_base_dir) : vst_plugin_path(plugin_dll_path), - plugin_context(main_context), + main_context(main_context), plugin_handle(LoadLibrary(plugin_dll_path.c_str()), FreeLibrary), - sockets(plugin_context.context, endpoint_base_dir, false) { + sockets(main_context.context, endpoint_base_dir, false) { // Got to love these C APIs if (!plugin_handle) { throw std::runtime_error("Could not load the Windows .dll file at '" + @@ -350,7 +350,7 @@ void Vst2Bridge::handle_dispatch() { // and where the Win32 message loop is handled. if (unsafe_opcodes.contains(opcode)) { std::promise dispatch_result; - boost::asio::dispatch(plugin_context.context, [&]() { + boost::asio::dispatch(main_context.context, [&]() { const intptr_t result = dispatch_wrapper( plugin, opcode, index, value, data, option); diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index ba02d8d0..2bda44bb 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -74,7 +74,7 @@ class Vst2Bridge { * @throw std::runtime_error Thrown when the VST plugin could not be loaded, * or if communication could not be set up. */ - Vst2Bridge(PluginContext& main_context, + Vst2Bridge(MainContext& main_context, std::string plugin_dll_path, std::string endpoint_base_dir); @@ -164,7 +164,7 @@ class Vst2Bridge { * message handling can be performed from a single thread, even when hosting * multiple plugins. */ - PluginContext& plugin_context; + MainContext& main_context; /** * The configuration for this instance of yabridge based on the `.so` file diff --git a/src/wine-host/individual-host.cpp b/src/wine-host/individual-host.cpp index e7782b3f..1fc812e7 100644 --- a/src/wine-host/individual-host.cpp +++ b/src/wine-host/individual-host.cpp @@ -65,11 +65,11 @@ int __cdecl main(int argc, char* argv[]) { // don't need to differentiate between individually hosted plugins and // plugin groups when it comes to event handling. // TODO: Update documentation once we figure out if we can safely replace - // PluginContext again with a normal `io_context`. - PluginContext plugin_context{}; + // MainContext again with a normal `io_context`. + MainContext main_context{}; std::unique_ptr bridge; try { - bridge = std::make_unique(plugin_context, plugin_dll_path, + bridge = std::make_unique(main_context, plugin_dll_path, socket_endpoint_path); } catch (const std::runtime_error& error) { std::cerr << "Error while initializing Wine VST host:" << std::endl; @@ -87,9 +87,9 @@ int __cdecl main(int argc, char* argv[]) { // Handle Win32 messages and X11 events on a timer, just like in // `GroupBridge::async_handle_events()`` - plugin_context.async_handle_events([&]() { + main_context.async_handle_events([&]() { bridge->handle_x11_events(); bridge->handle_win32_events(); }); - plugin_context.run(); + main_context.run(); } diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index 172996c0..ce0562d6 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -16,7 +16,15 @@ #include "utils.h" -PluginContext::PluginContext() : context(), events_timer(context) {} +MainContext::MainContext() : context(), events_timer(context) {} + +void MainContext::run() { + context.run(); +} + +void MainContext::stop() { + context.stop(); +} uint32_t WINAPI win32_thread_trampoline(fu2::unique_function* entry_point) { @@ -34,14 +42,6 @@ Win32Thread& Win32Thread::operator=(Win32Thread&& o) { return *this; } -void PluginContext::run() { - context.run(); -} - -void PluginContext::stop() { - context.stop(); -} - Win32Thread::Win32Thread() : handle(nullptr, nullptr) {} Win32Timer::Win32Timer(HWND window_handle, diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index b442ed0d..c2c64c5f 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -39,18 +39,16 @@ constexpr std::chrono::duration event_loop_interval = std::chrono::milliseconds(1000) / 30; /** - * A wrapper around `boost::asio::io_context()`. A single instance is shared for - * all plugins in a plugin group so that most events can be handled on the main - * thread, which can be required because all GUI related operations have to be - * handled from the same thread. If during the Win32 message loop the plugin - * performs a host callback and the host then calls a function on the plugin in - * response, then this IO context will still be busy with the message loop - * which. To prevent a deadlock in this situation, we'll allow different threads - * to handle `dispatch()` calls while the message loop is running. + * A wrapper around `boost::asio::io_context()` to serve as the application's + * main IO context. A single instance is shared for all plugins in a plugin + * group so that several important events can be handled on the main thread, + * which can be required because in the Win32 model all GUI related operations + * have to be handled from the same thread. This will be run from the + * application's main thread. */ -class PluginContext { +class MainContext { public: - PluginContext(); + MainContext(); /** * Run the IO context. This rest of this class assumes that this is only @@ -60,7 +58,7 @@ class PluginContext { /** * Drop all future work from the IO context. This does not necessarily mean - * that the thread that called `plugin_context.run()` immediatly returns. + * that the thread that called `main_context.run()` immediatly returns. */ void stop(); @@ -98,6 +96,9 @@ class PluginContext { * Is `true` if the context is currently handling the Win32 message loop and * incoming `dispatch()` events should be handled on their own thread (as * posting them to the IO context will thus block). + * + * TODO: No longer used after the thread rework, we can probably just drop + * this if everything works out */ std::atomic_bool event_loop_active; From f39ee82bd40f9ebc82b7e1e0693e5841e2acdb5b Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 01:21:56 +0100 Subject: [PATCH 34/40] Update socket rework changelog entry --- CHANGELOG.md | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1eef173f..d9f9dafc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,18 +10,22 @@ Versioning](https://semver.org/spec/v2.0.0.html). ### Added -- The way communication works in yabridge has been completely redesigned. This - was necessary to allow yabridge to handle nested and mutually recursive - function calls, as well as several other edge cases. What this boils down to - is that yabridge became even faster, more responsive, and can now handle a few - edge case scenarios that would previously require workarounds. This means that - yabridge no longer requires the `hack_reaper_update_display` workaround for - _REAPER_ and _Renoise_, and the loading issues in Bitwig Studio 3.3 beta 1 - have also been resolved. I have been testing this extensively to make sure - that the change does not introduce any regressions, but please let me know if - this does break anything for you. +- The way communication works in yabridge has been completely redesigned to be + asynchronous and to use additional threads as necessary. This was needed to + allow yabridge to handle nested and mutually recursive function calls as well + as several other edge cases. What this boils down to is that yabridge became + even faster, more responsive, and can now handle a few edge case scenarios + that would previously require workarounds. This means that yabridge no longer + requires the `hack_reaper_update_display` workaround for _REAPER_ and + _Renoise_, that the loading issues in Bitwig Studio 3.3 beta 1 have also been + resolved, and that certain plugins like Kontakt no longer interrupt playback + in Bitwig while their editor is opening. I have been testing this extensively + to make sure that the change does not introduce any regressions, but please + let me know if this does break anything for you. - TODO: Expand on this + TODO: Expand on this + TODO: Remove known issue about opening Kontakt and certain other plugins + causing playback to stall, since this is no longer the case ### Removed @@ -32,8 +36,7 @@ Versioning](https://semver.org/spec/v2.0.0.html). ### Changed - As part of the communication rework the way the Wine process handles threading - has also been completely reworked. although this should not cause any - noticeable changes. + has also been completely reworked. ## [1.7.1] - 2020-10-23 From e70b042f9fb6e33e2f61dcdedd26f2c771c9fb29 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 19:52:11 +0100 Subject: [PATCH 35/40] Update the versions of the tested DAWs --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index b296679f..b44bb246 100644 --- a/README.md +++ b/README.md @@ -38,10 +38,10 @@ compatibility while also staying easy to debug and maintain. Yabridge has been tested under the following VST hosts using Wine Staging 5.9: - Bitwig Studio 3.2 -- Carla 2.1 -- Ardour 6.2 +- Carla 2.2 +- Ardour 6.3 - Mixbus 6.0.702 -- REAPER 6.09[\*](#runtime-dependencies-and-known-issues) +- REAPER 6.15[\*](#runtime-dependencies-and-known-issues) - Renoise 3.2.1[\*](#runtime-dependencies-and-known-issues) Please let me know if there are any issues with other VST hosts. From 93e01dacef5a31f66f7ca6fb9445d2fa84023df8 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 20:25:10 +0100 Subject: [PATCH 36/40] Explicitly close all sockets on shutdown This way we're sure to break out of any blocking loops. --- src/common/communication.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/common/communication.h b/src/common/communication.h index 4df0966a..82045ea8 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -260,8 +260,10 @@ class EventHandler { * `boost::system_error` when this happens. */ void close() { + // The shutdown can fail when the socket is already closed + boost::system::error_code err; socket.shutdown( - boost::asio::local::stream_protocol::socket::shutdown_both); + boost::asio::local::stream_protocol::socket::shutdown_both, err); socket.close(); } @@ -653,6 +655,24 @@ class Sockets { // then we can just silently ignore this } } + + // Manually close all sockets so we break out of any blocking operations + // that may still be active + host_vst_dispatch.close(); + host_vst_dispatch_midi_events.close(); + vst_host_callback.close(); + + // These shutdowns can fail when the socket has already been closed, but + // that's not an issue in our case + constexpr auto shutdown_type = + boost::asio::local::stream_protocol::socket::shutdown_both; + boost::system::error_code err; + host_vst_parameters.shutdown(shutdown_type, err); + host_vst_process_replacing.shutdown(shutdown_type, err); + host_vst_control.shutdown(shutdown_type, err); + host_vst_parameters.close(); + host_vst_process_replacing.close(); + host_vst_control.close(); } /** From d81759c9290c983034b63245d718922bb08f7ae6 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 20:40:19 +0100 Subject: [PATCH 37/40] Add an explicit wait to our thread wrapper --- src/wine-host/utils.cpp | 6 ++++++ src/wine-host/utils.h | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index ce0562d6..484a9fe8 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -42,6 +42,12 @@ Win32Thread& Win32Thread::operator=(Win32Thread&& o) { return *this; } +void Win32Thread::wait() { + if (handle) { + WaitForSingleObject(handle.get(), INFINITE); + } +} + Win32Thread::Win32Thread() : handle(nullptr, nullptr) {} Win32Timer::Win32Timer(HWND window_handle, diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index c2c64c5f..10acf6a6 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -184,6 +184,12 @@ class Win32Thread { Win32Thread(Win32Thread&&); Win32Thread& operator=(Win32Thread&&); + /** + * Threads in Win32 don't join like threads on other platforms, but it may + * still be useful to wait for a thread to terminate. + */ + void wait(); + private: /** * The handle for the thread that is running, will be a null pointer if this From dd9957159af5f51a3303387a5684b7f93fb0a5a1 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 21:05:14 +0100 Subject: [PATCH 38/40] Move the sockets after the threads in Vst2Bridge This way the sockets can be destroyed before the threads, and we can safely wait for the threads to shut down. I initially had Win32Thread imitate std::jthread's join-on-destruct, but that was causing group processes to hang. Now we can safely add that back again, and this will fix some spurious segfaults during plugin scans when using plugin groups containing a massive amount of plugins. --- src/wine-host/bridges/vst2.h | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/wine-host/bridges/vst2.h b/src/wine-host/bridges/vst2.h index 2bda44bb..514c682d 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -186,11 +186,6 @@ class Vst2Bridge { */ AEffect* plugin; - /** - * All sockets used for communicating with this specific plugin. - */ - Sockets sockets; - /** * The thread that specifically handles `effProcessEvents` opcodes so the * plugin can still receive MIDI during GUI interaction to work around Win32 @@ -207,6 +202,15 @@ class Vst2Bridge { */ Win32Thread process_replacing_handler; + /** + * All sockets used for communicating with this specific plugin. + * + * NOTE: This is defined **after** the threads on purpose. This way the + * sockets will be closed first, and we can then safely wait for the + * threads to exit. + */ + Sockets sockets; + /** * A scratch buffer for sending and receiving data during `process` and * `processReplacing` calls. From 08cd7cf8abd1d755a0796613d857224ecbba142c Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 21:11:34 +0100 Subject: [PATCH 39/40] Join Win32Threads on destruct, like std::jthread --- src/wine-host/utils.cpp | 2 +- src/wine-host/utils.h | 26 +++++++++++++++----------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/wine-host/utils.cpp b/src/wine-host/utils.cpp index 484a9fe8..15b477c4 100644 --- a/src/wine-host/utils.cpp +++ b/src/wine-host/utils.cpp @@ -42,7 +42,7 @@ Win32Thread& Win32Thread::operator=(Win32Thread&& o) { return *this; } -void Win32Thread::wait() { +Win32Thread::~Win32Thread() { if (handle) { WaitForSingleObject(handle.get(), INFINITE); } diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index 10acf6a6..3fd01100 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -132,11 +132,15 @@ win32_thread_trampoline(fu2::unique_function* entry_point); /** * A simple RAII wrapper around the Win32 thread API that imitates - * `std::thread`. + * `std::jthread`, including implicit joining (or waiting, since this is Win32) + * on destruction. * - * `std::thread` directly uses pthreads. This means that, like with - * `CreateThread()`, some thread local information does not get initialized - * which can lead to memory errors. + * `std::thread` uses pthreads directly in Winelib (since this is technically a + * regular Linux application). This means that when using + * `std::thread`/`std::jthread` directly, some thread local information that + * `CreateThread()` would normally set does not get initialized. This could then + * lead to memory errors. This wrapper aims to be equivalent to `std::jthread`, + * but using the Win32 API instead. * * @note This should be used instead of `std::thread` or `std::jthread` whenever * the thread directly calls third party library code, i.e. `LoadLibrary()`, @@ -152,7 +156,7 @@ class Win32Thread { /** * Constructor that immediately starts running the thread. This works - * equivalently to `std::thread`. + * equivalently to `std::jthread`. * * @param entry_point The thread entry point that should be run. * @param parameter The parameter passed to the entry point function. @@ -178,18 +182,18 @@ class Win32Thread { nullptr), CloseHandle) {} + /** + * Join (or wait on, since this is WIn32) the thread on shutdown, just like + * `std::jthread` does. + */ + ~Win32Thread(); + Win32Thread(const Win32Thread&) = delete; Win32Thread& operator=(const Win32Thread&) = delete; Win32Thread(Win32Thread&&); Win32Thread& operator=(Win32Thread&&); - /** - * Threads in Win32 don't join like threads on other platforms, but it may - * still be useful to wait for a thread to terminate. - */ - void wait(); - private: /** * The handle for the thread that is running, will be a null pointer if this From 264f6ab8b59dd6a492cd5f5cb04b2b7fbd5a1e01 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 28 Oct 2020 21:19:50 +0100 Subject: [PATCH 40/40] Mention that all plugin group crashes are fixed --- CHANGELOG.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9f9dafc..aa24889b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,16 +27,23 @@ Versioning](https://semver.org/spec/v2.0.0.html). TODO: Remove known issue about opening Kontakt and certain other plugins causing playback to stall, since this is no longer the case +### Changed + +- As part of the communication rework the way the Wine process handles threading + has also been completely reworked. + ### Removed - The `hack_reaper_update_display` option is now obsolete and has been removed. TODO: Remove all mentions of `hack_reaper_update_display` from the readme. -### Changed +### Fixed -- As part of the communication rework the way the Wine process handles threading - has also been completely reworked. +- Fixed a very long standing issue when using plugins groups where unloading a + plugin could cause a crash. In practice this was only reproducible during the + plugin scanning process when hosting a very large number of plugins in a + single group. ## [1.7.1] - 2020-10-23