From 4226ab6e430da810ac7d57ecf91f9ea25ca86613 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Mon, 28 Dec 2020 13:19:34 +0100 Subject: [PATCH] Pass pointers to `IMessage` objects around Instead of serializing the actual `YaMessage`, for the reasons mentioned in the comments. This was needed to stop iZotope VocalSynth 2 in Ardour from segfaulting when editing parameters, because that plugin is apparently being very naughty. --- src/common/logging/vst3.cpp | 10 +- src/common/serialization/vst3/README.md | 1 + src/common/serialization/vst3/message.cpp | 46 ++++++++ src/common/serialization/vst3/message.h | 105 +++++++++++++++--- .../vst3/plugin/connection-point.h | 13 ++- .../bridges/vst3-impls/plugin-proxy.cpp | 18 +-- src/plugin/bridges/vst3.cpp | 2 +- .../vst3-impls/connection-point-proxy.cpp | 13 +-- src/wine-host/bridges/vst3.cpp | 26 ++--- 9 files changed, 181 insertions(+), 53 deletions(-) diff --git a/src/common/logging/vst3.cpp b/src/common/logging/vst3.cpp index 8ec0c08f..a6dd672f 100644 --- a/src/common/logging/vst3.cpp +++ b/src/common/logging/vst3.cpp @@ -126,13 +126,15 @@ bool Vst3Logger::log_request(bool is_host_vst, bool Vst3Logger::log_request(bool is_host_vst, const YaConnectionPoint::Notify& request) { return log_request_base(is_host_vst, [&](auto& message) { + // We can safely print the pointer as long we don't dereference it message << request.instance_id - << ": IConnectionPoint::notify(message = (request.message).getMessageID()) { - message << "with ID = \"" << id << "\""; + const_cast(request.message_ptr).getMessageID()) { + message << " with ID = \"" << id << "\""; } else { - message << "without an ID"; + message << " without an ID"; } message << ">)"; }); diff --git a/src/common/serialization/vst3/README.md b/src/common/serialization/vst3/README.md index e1e1c1d8..8bc48c81 100644 --- a/src/common/serialization/vst3/README.md +++ b/src/common/serialization/vst3/README.md @@ -42,6 +42,7 @@ implemented for serialization purposes: | `YaAttributeList` | `IAttributeList` | | | `YaEventList` | `IEventList` | Comes with a lot of serialization wrappers around the related structs. | | `YaMessage` | `IMessage` | | +| `YaMessagePtr` | `IMessage` | Should be used in inter process communication to exchange messages | | `YaParameterChanges` | `IParameterChanges` | | | `YaParamValueQueue` | `IParamValueQueue` | | | `VectorStream` | `IBStream` | Used for serializing data streams. | diff --git a/src/common/serialization/vst3/message.cpp b/src/common/serialization/vst3/message.cpp index ca416036..ac285a2a 100644 --- a/src/common/serialization/vst3/message.cpp +++ b/src/common/serialization/vst3/message.cpp @@ -16,6 +16,52 @@ #include "message.h" +YaMessagePtr::YaMessagePtr(){FUNKNOWN_CTOR} + +YaMessagePtr::YaMessagePtr(IMessage& message) + : message_id(message.getMessageID() + ? std::make_optional(message.getMessageID()) + : std::nullopt), + original_message_ptr(static_cast( + reinterpret_cast(&message))){FUNKNOWN_CTOR} + + YaMessagePtr::~YaMessagePtr() { + FUNKNOWN_DTOR +} + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor" +IMPLEMENT_FUNKNOWN_METHODS(YaMessagePtr, + Steinberg::Vst::IMessage, + Steinberg::Vst::IMessage::iid) +#pragma GCC diagnostic pop + +Steinberg::Vst::IMessage* YaMessagePtr::get_original() const { + // See the docstrings on `YaMessage` and `YaMessagePtr` + return reinterpret_cast( + static_cast(original_message_ptr)); +} + +Steinberg::FIDString PLUGIN_API YaMessagePtr::getMessageID() { + if (message_id) { + return message_id->c_str(); + } else { + return nullptr; + } +} + +void PLUGIN_API YaMessagePtr::setMessageID(Steinberg::FIDString id /*in*/) { + if (id) { + message_id = id; + } else { + message_id.reset(); + } +} + +Steinberg::Vst::IAttributeList* PLUGIN_API YaMessagePtr::getAttributes() { + return &attribute_list; +} + YaMessage::YaMessage(){FUNKNOWN_CTOR} YaMessage::~YaMessage() { diff --git a/src/common/serialization/vst3/message.h b/src/common/serialization/vst3/message.h index e26c8b23..a4e1aa6c 100644 --- a/src/common/serialization/vst3/message.h +++ b/src/common/serialization/vst3/message.h @@ -19,6 +19,7 @@ #include #include +#include "../common.h" #include "attribute-list.h" #include "base.h" @@ -26,15 +27,94 @@ #pragma GCC diagnostic ignored "-Wnon-virtual-dtor" /** - * Wraps around `IMessage` for serialization purposes. We create instances of - * these in `IHostApplication::createInstance()` so the Windows VST3 plugin can - * send messages between objects. There is one huge caveat here: it is - * impossible to work with arbitrary `IMessage` objects, since there's no way to - * retrieve all of the keys in the attribute list. With this approach we support - * hosts that indirectly connect the processor and the controller through a - * proxy (like Ardour), but we still require a dynamic cast from the `IMessage*` - * passed to `YaConnectionPoint::notify()` to a `YaMessage*` for this to work - * for the above mentioned reason. + * A serialization wrapper around `IMessage`. As explained in `YaMessage`, we + * can't exchange the regular `YaMessage` object when dealing with + * `IConnectionPoint` connection proxies. Instead, we'll use this wrapper that + * only stores the ID (for logging purposes) and a pointer to the original + * object. That way we can pass the original message created by the plugin to + * the receiver without having to know what object the host's connection proxy + * is actually connecting us to. + * + * @note THis object should _not_ be passed to the plugin directly. The only + * purpose of this object is to be able to pass the original `IMessage*` + * object passed the connection proxy to the receiver, by wrapping a pointer + * to it in this object. `YaMessagePtr::get_original()` can be used to + * retrieve the original object. + */ +class YaMessagePtr : public Steinberg::Vst::IMessage { + public: + YaMessagePtr(); + + /** + * Create a proxy for this message. We'll store the message's ID for logging + * purposes as well as a pointer to it so we can retrieve the object after a + * round trip from the Wine plugin host, to the native plugin, to the host, + * back to the native plugin, and then finally back to the Wine plugin host + * again. + */ + explicit YaMessagePtr(IMessage& message); + + ~YaMessagePtr(); + + DECLARE_FUNKNOWN_METHODS + + /** + * Get back a pointer to the original `IMessage` object passed to the + * constructor. This should be used on the Wine plugin host side when + * handling `IConnectionPoint::notify`. + */ + Steinberg::Vst::IMessage* get_original() const; + + virtual Steinberg::FIDString PLUGIN_API getMessageID() override; + virtual void PLUGIN_API + setMessageID(Steinberg::FIDString id /*in*/) override; + virtual Steinberg::Vst::IAttributeList* PLUGIN_API getAttributes() override; + + template + void serialize(S& s) { + s.ext(message_id, bitsery::ext::StdOptional{}, + [](S& s, std::string& id) { s.text1b(id, 1024); }); + s.value8b(original_message_ptr); + } + + private: + /** + * The implementation that comes with the SDK returns a null pointer when + * the ID has not yet been set, so we'll do the same thing. + */ + std::optional message_id; + + /** + * The pointer to the message passed during the constructor, as a 64-bit + * unsigned integer. This way we can retrieve the original object after a + * round trip. + */ + native_size_t original_message_ptr = 0; + + /** + * An empty attribute list, in case the host checks this for some reason. + */ + YaAttributeList attribute_list; +}; + +/** + * A `IMessage` implementation the plugin can use to exchange messages with. We + * create instances of these in `IHostApplication::createInstance()` so the + * Windows VST3 plugin can send messages between objects. A plugin's controller + * or processor will fill the message with data and then try to send it to the + * connected object using `IConnectionPoint::notify()`. For directly connected + * objects this works exactly like you'd expect. When the host places a proxy + * between the two, it becomes a bit more interesting, and we'll have to proxy + * that proxy. In that case we won't send the actual `YaMessage` object from the + * Wine plugin host to the native plugin, and then back to the Wine plugin host. + * Instead, we'll send a thin wrapper that only stores a name and a pointer to + * the actual object. This is needed in case the plugin tries to store the + * `IMessage` object, thinking it's backed by a smart pointer. This means that + * the message we pass while handling `IConnectionPoint::notify` should live as + * long as the original message object, thus we'll use a pointer to get back the + * original message object. + * + * @relates YaMessagePtr */ class YaMessage : public Steinberg::Vst::IMessage { public: @@ -53,13 +133,6 @@ class YaMessage : public Steinberg::Vst::IMessage { setMessageID(Steinberg::FIDString id /*in*/) override; virtual Steinberg::Vst::IAttributeList* PLUGIN_API getAttributes() override; - template - void serialize(S& s) { - s.ext(message_id, bitsery::ext::StdOptional{}, - [](S& s, std::string& id) { s.text1b(id, 1024); }); - s.object(attribute_list); - } - private: /** * The implementation that comes with the SDK returns a null pointer when diff --git a/src/common/serialization/vst3/plugin/connection-point.h b/src/common/serialization/vst3/plugin/connection-point.h index 213cdcad..4dab808d 100644 --- a/src/common/serialization/vst3/plugin/connection-point.h +++ b/src/common/serialization/vst3/plugin/connection-point.h @@ -178,21 +178,24 @@ class YaConnectionPoint : public Steinberg::Vst::IConnectionPoint { * the Wine plugin host. Since `IAttributeList` does not have any way to * iterate over all values, we only support messages sent by plugins using * our own implementation of the interface, since there's no way to - * serialize them otherwise. This `IConnectionPoint::notify()` - * implementation is also only used with hosts that do not connect objects - * directly and use connection proxies instead. + * serialize them otherwise. Additionally, plugins may store the `IMessage` + * pointer for later usage, so we have to pass through a pointer to the + * original message so it has the same lifetime as the original message. + * This `IConnectionPoint::notify()` implementation is also only used with + * hosts that do not connect objects directly and use connection proxies + * instead. */ struct Notify { using Response = UniversalTResult; native_size_t instance_id; - YaMessage message; + YaMessagePtr message_ptr; template void serialize(S& s) { s.value8b(instance_id); - s.object(message); + s.object(message_ptr); } }; diff --git a/src/plugin/bridges/vst3-impls/plugin-proxy.cpp b/src/plugin/bridges/vst3-impls/plugin-proxy.cpp index 39221270..615710cb 100644 --- a/src/plugin/bridges/vst3-impls/plugin-proxy.cpp +++ b/src/plugin/bridges/vst3-impls/plugin-proxy.cpp @@ -250,14 +250,18 @@ tresult PLUGIN_API Vst3PluginProxyImpl::notify(Steinberg::Vst::IMessage* message) { // Since there is no way to enumerate over all values in an // `IAttributeList`, we can only support relaying messages that were sent by - // our own objects. This is only needed to support hosts that place a - // connection proxy between two objects instead of connecting them directly. - // If the objects are connected directly we also connected them directly on - // the Wine side, so we don't have to do any additional when those objects - // pass through messages. - if (auto message_impl = dynamic_cast(message)) { + // our own objects. Additionally, the `IMessage*` we end up passing to the + // plugin needs to have the same lifetime as the original object, because + // some plugins are being a bit naughty. That's why we pass around a pointer + // to the original message object. + // All of this is only needed to support hosts that place a connection proxy + // between two objects instead of connecting them directly. If the objects + // are connected directly we also connected them directly on the Wine side, + // so we don't have to do any additional when those objects pass through + // messages. + if (auto message_ptr = dynamic_cast(message)) { return bridge.send_message(YaConnectionPoint::Notify{ - .instance_id = instance_id(), .message = *message_impl}); + .instance_id = instance_id(), .message_ptr = *message_ptr}); } else { bridge.logger.log( "WARNING: Unknown message type passed to " diff --git a/src/plugin/bridges/vst3.cpp b/src/plugin/bridges/vst3.cpp index fcb9eee0..b59564ef 100644 --- a/src/plugin/bridges/vst3.cpp +++ b/src/plugin/bridges/vst3.cpp @@ -110,7 +110,7 @@ Vst3PluginBridge::Vst3PluginBridge() -> YaConnectionPoint::Notify::Response { return plugin_proxies.at(request.instance_id) .get() - .connection_point_proxy->notify(&request.message); + .connection_point_proxy->notify(&request.message_ptr); }, [&](const YaHostApplication::GetName& request) -> YaHostApplication::GetName::Response { diff --git a/src/wine-host/bridges/vst3-impls/connection-point-proxy.cpp b/src/wine-host/bridges/vst3-impls/connection-point-proxy.cpp index 55a03c8a..4949a1f8 100644 --- a/src/wine-host/bridges/vst3-impls/connection-point-proxy.cpp +++ b/src/wine-host/bridges/vst3-impls/connection-point-proxy.cpp @@ -55,15 +55,14 @@ Vst3ConnectionPointProxyImpl::disconnect(IConnectionPoint* /*other*/) { tresult PLUGIN_API Vst3ConnectionPointProxyImpl::notify(Steinberg::Vst::IMessage* message) { - // As explained in `YaMessage` and `Vst3PluginProxyImpl::notify`, we can - // only support our own `IMessage implementation here` - if (auto message_impl = dynamic_cast(message)) { - return bridge.send_message(YaConnectionPoint::Notify{ - .instance_id = owner_instance_id(), .message = *message_impl}); + if (message) { + return bridge.send_message( + YaConnectionPoint::Notify{.instance_id = owner_instance_id(), + .message_ptr = YaMessagePtr(*message)}); } else { - std::cerr << "WARNING: Unknown message type passed to " + std::cerr << "WARNING: Null pointer passed to " "'IConnectionPoint::notify()', ignoring" << std::endl; - return Steinberg::kNotImplemented; + return Steinberg::kInvalidArgument; } } diff --git a/src/wine-host/bridges/vst3.cpp b/src/wine-host/bridges/vst3.cpp index a308c4b9..7679ef20 100644 --- a/src/wine-host/bridges/vst3.cpp +++ b/src/wine-host/bridges/vst3.cpp @@ -223,21 +223,21 @@ void Vst3Bridge::run() { return result; } }, - [&](YaConnectionPoint::Notify& request) + [&](const YaConnectionPoint::Notify& request) -> YaConnectionPoint::Notify::Response { - // FIXME: This needs to be redesigned. We should only send a - // pointer to the `IMessage` instead of copying the - // actual message. What ends up happening is that iZotope - // VocalSynth 2 exchanges pointers to the processor and - // the controller. Then at some point during an - // `IAudioProcessor::process()` call after a parameter - // changes, it will try to access the pointers stored in - // that message. So to be able to support that, the - // message object we pass to notify here still has to be - // alive at that point. This goes 100% against the design - // of VST3, but there's nothing we can do about it. + // NOTE: We're using a few tricks here to pass through a pointer + // to the _original_ `IMessage` object passed to a + // connection proxy. This is needed because some plugins + // like iZotope VocalSynth 2 use these messages to + // exchange pointers between their objects so they can + // break out of VST3's separation, but they might also + // store the message object and not the actual pointers. + // We should thus be passing a (raw) pointer to the + // original object so we can pretend none of this wrapping + // and serializing has ever happened. return object_instances[request.instance_id] - .connection_point->notify(&request.message); + .connection_point->notify( + request.message_ptr.get_original()); }, [&](YaEditController::SetComponentState& request) -> YaEditController::SetComponentState::Response {