From 7a55fc3ec0ceeb1edcdee1063a9bbc40f98bdafa Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 23 Dec 2020 15:31:32 +0100 Subject: [PATCH] Significantly clean up mutual recursion workaround This has much fewer moving parts, and it's probably more understandable. There was also a race condition in the previous implementation, so there's that. --- .../bridges/vst3-impls/plug-frame-proxy.cpp | 79 +---------- .../bridges/vst3-impls/plug-frame-proxy.h | 48 ------- src/wine-host/bridges/vst3.cpp | 53 +++----- src/wine-host/bridges/vst3.h | 125 +++++++++++++++++- src/wine-host/utils.h | 32 +---- 5 files changed, 146 insertions(+), 191 deletions(-) diff --git a/src/wine-host/bridges/vst3-impls/plug-frame-proxy.cpp b/src/wine-host/bridges/vst3-impls/plug-frame-proxy.cpp index 888e1ef3..3ceff0de 100644 --- a/src/wine-host/bridges/vst3-impls/plug-frame-proxy.cpp +++ b/src/wine-host/bridges/vst3-impls/plug-frame-proxy.cpp @@ -47,80 +47,11 @@ Vst3PlugFrameProxyImpl::resizeView(Steinberg::IPlugView* /*view*/, // assume `view` is the `IPlugView*` returned by the last call to // `IEditController::createView()` - // HACK: This ia bit of a weird one and requires special handling. A - // plugin will call this function from the Win32 message loop so - // the call blocks the loop. The host will then check with the - // plugin if it can actually resize itself to `*newSize`, and it - // will then call `IPlugView::onSize()` with the new size. The - // issue is that the `IPlugView::onSize()` call also has to be - // called from within the UI thread, but that thread is currently - // being blocked by the call to this function. - // As a workaround, we'll send the message for the call to - // `IPlugFrame::resizeView()` on another thread. We then wait for - // either that request to finish immediately (meaning the host - // hasn't resized the window), or for `IPlugView::onSize()` to be - // called by the host. If the host does call `IPlugView::onSize()` - // while the other thread is handling `IPlugFrame::resizeView()`, - // then we'll awaken this thread using a condition variable so we - // can do the actual call to `IPlugView::onSize()` from here, from - // within the Win32 loop. - // TODO: Can we someone use Boost.Asio strands to make this cleaner? - { - std::lock_guard lock(on_size_interrupt_mutex); - on_size_interrupt_waiting = true; - on_size_interrupt.reset(); - on_size_interrupt_result.reset(); - } - - std::promise resize_result_promise{}; - std::future resize_result_future = - resize_result_promise.get_future(); - Win32Thread resize_thread([&]() { - const tresult result = bridge.send_message(YaPlugFrame::ResizeView{ - .owner_instance_id = owner_instance_id(), - .new_size = *newSize}); - - resize_result_promise.set_value(result); - - { - std::lock_guard lock(on_size_interrupt_mutex); - if (BOOST_LIKELY(!on_size_interrupt_waiting)) { - return; - } - - // If the call to `IPlugFrame::resizeView()` finish without the - // host calling `IPlugView::onSize()` (e.g. when the call - // failed), then we'll have to manually unblock the thread below - // by providing a noop function. - on_size_interrupt = []() { return Steinberg::kResultOk; }; - } - on_size_interrupt_cv.notify_one(); - - // We don't need this result value, but we should still wait for - // it - std::unique_lock lock(on_size_interrupt_mutex); - on_size_interrupt_cv.wait( - lock, [&]() { return on_size_interrupt_result.has_value(); }); - }); - - // Wait for `IPlugView::onSize()` to be called by the host and handle - // the call here on this thread, or wait for the call to - // `IPlugFrame::resizeView()` in the above thread to finish. In the last - // case we'll execute a noop function. - std::unique_lock lock(on_size_interrupt_mutex); - on_size_interrupt_cv.wait( - lock, [&]() { return on_size_interrupt.has_value(); }); - - // When we get a function through one of the two above means, we'll - // store the result and then awake the calling thread again so it can - // access the result - on_size_interrupt_result = (*on_size_interrupt)(); - on_size_interrupt_waiting = false; - - lock.unlock(); - on_size_interrupt_cv.notify_one(); - - return resize_result_future.get(); + // We have to use this special sending function here so we can handle + // calls to `IPlugView::onSize()` from this same thread (the UI thread). + // See the docstring for more information. + return bridge.send_mutually_recursive_message(YaPlugFrame::ResizeView{ + .owner_instance_id = owner_instance_id(), .new_size = *newSize}); } else { std::cerr << "WARNING: Null pointer passed to 'IPlugFrame::resizeView()'" diff --git a/src/wine-host/bridges/vst3-impls/plug-frame-proxy.h b/src/wine-host/bridges/vst3-impls/plug-frame-proxy.h index e43d7e9b..e15066a6 100644 --- a/src/wine-host/bridges/vst3-impls/plug-frame-proxy.h +++ b/src/wine-host/bridges/vst3-impls/plug-frame-proxy.h @@ -30,58 +30,10 @@ class Vst3PlugFrameProxyImpl : public Vst3PlugFrameProxy { tresult PLUGIN_API queryInterface(const Steinberg::TUID _iid, void** obj) override; - /** - * This is needed to be able to handle a call to `IPlugView::onSize()` from - * the UI thread while the plugin is currently calling - * `IPlugFrame::resizeView()` from that same thread.. This is probably the - * hackiest (and most error prone, probably) part of this VST3 - * implementation. The details on how this works and why it is necessary are - * explained in the comment in `YaPlugFrameProxyImpl::resizeView()`. - * - * If there is currently a call to `resizeView()` being processed, then this - * will run `on_size` from the same thread that's currently processing a - * call to `resizeView()` and return the result from the function call. - * Otherwise this will return a nullopt and `on_size` should be passed to - * `main_context.run_in_context()`. - */ - template - std::optional maybe_run_on_size_from_ui_thread(F on_size) { - { - std::lock_guard lock(on_size_interrupt_mutex); - if (!on_size_interrupt_waiting) { - return std::nullopt; - } - - on_size_interrupt = std::move(on_size); - } - on_size_interrupt_cv.notify_one(); - - // Since `on_size` is run from another thread, we now have to wait to be - // woken up again when the result is ready - std::unique_lock lock(on_size_interrupt_mutex); - on_size_interrupt_cv.wait( - lock, [&]() { return on_size_interrupt_result.has_value(); }); - - return *on_size_interrupt_result; - } - // From `IPlugFrame` tresult PLUGIN_API resizeView(Steinberg::IPlugView* view, Steinberg::ViewRect* newSize) override; private: Vst3Bridge& bridge; - - /** - * A function that will be used to run `IPlugView::onSize()` on the thread - * that originally called `IPlugFrame::resizeView()` to work around a Win32 - * limitation, along with its result, and whether or not we're currently - * waiting for this function to be provided by some other thread. See the - * comment in `onSize()` for more information. - */ - bool on_size_interrupt_waiting = false; - std::optional> on_size_interrupt; - std::optional on_size_interrupt_result; - std::condition_variable on_size_interrupt_cv; - std::mutex on_size_interrupt_mutex; }; diff --git a/src/wine-host/bridges/vst3.cpp b/src/wine-host/bridges/vst3.cpp index 6d9e91f6..756f948e 100644 --- a/src/wine-host/bridges/vst3.cpp +++ b/src/wine-host/bridges/vst3.cpp @@ -69,7 +69,7 @@ void Vst3Bridge::run() { -> Vst3PlugViewProxy::Destruct::Response { // XXX: Not sure if his has to be run form the UI thread main_context - .run_in_context([&]() { + .run_in_context([&]() { // When the pointer gets dropped by the host, we want to // drop it here as well, along with the `IPlugFrame` // proxy object it may have received in @@ -275,7 +275,7 @@ void Vst3Bridge::run() { -> YaEditController::CreateView::Response { // Instantiate the object from the GUI thread main_context - .run_in_context([&]() { + .run_in_context([&]() { object_instances[request.instance_id].plug_view = Steinberg::owned( object_instances[request.instance_id] @@ -409,31 +409,20 @@ void Vst3Bridge::run() { .result = result, .updated_size = request.size}; }, [&](YaPlugView::OnSize& request) -> YaPlugView::OnSize::Response { - auto call_on_size = [&]() { - const auto result = - object_instances[request.owner_instance_id] + // HACK: This function has to be run from the UI thread since + // the plugin probably wants to redraw when it gets + // resized. The issue here is that this function can be + // called in response to a call to + // `IPlugFrame::resizeView()`. That function is always + // called from the UI thread, so we need some way to run + // code on the same thread that's currently waiting for a + // response to the message it sent. See the docstring of + // this function for more information on how this works. + return do_mutual_recursion_or_handle_in_main_context( + [&]() { + return object_instances[request.owner_instance_id] .plug_view->onSize(&request.new_size); - return result; - }; - - // HACK: As explained in the docstring of - // `YaPlugFrameProxyImpl::resizeView()`, calls to - // `IPlugView::onSize()` have to be handled from the UI - // thread, even if that thread is currently making a call - // to `IPlugFrame::resizeView()`. We use some special - // handling to execute `call_on_size()` on that thread if - // it's currently waiting for a call to - // `IPlugFrame::resizeView()`, and we'll just run in - // within the main context as usual otherwise. - if (auto result = - object_instances[request.owner_instance_id] - .plug_frame_proxy_impl - ->maybe_run_on_size_from_ui_thread(call_on_size)) { - return *result; - } else { - return main_context.run_in_context(call_on_size) - .get(); - } + }); }, [&](const YaPlugView::OnFocus& request) -> YaPlugView::OnFocus::Response { @@ -450,15 +439,9 @@ void Vst3Bridge::run() { // pass that to the `setFrame()` function. The lifetime of this // object is tied to that of the actual `IPlugFrame` object // we're passing this proxy to. - // We'll also store an unmanaged pointer to the actual - // implementation so we can do some sneakiness for - // `IPlugView::onSize()`. - object_instances[request.owner_instance_id] - .plug_frame_proxy_impl = new Vst3PlugFrameProxyImpl( - *this, std::move(request.plug_frame_args)); object_instances[request.owner_instance_id].plug_frame_proxy = - Steinberg::owned(object_instances[request.owner_instance_id] - .plug_frame_proxy_impl); + Steinberg::owned(new Vst3PlugFrameProxyImpl( + *this, std::move(request.plug_frame_args))); // TODO: Does this have to be run from the UI thread? Figure out // if it does @@ -711,7 +694,7 @@ void Vst3Bridge::unregister_object_instance(size_t instance_id) { // executed and when the actual host application context on // the plugin side gets deallocated. main_context - .run_in_context([&, instance_id]() { + .run_in_context([&, instance_id]() { std::lock_guard lock(object_instances_mutex); object_instances.erase(instance_id); }) diff --git a/src/wine-host/bridges/vst3.h b/src/wine-host/bridges/vst3.h index 9b7c5bf8..0f5927dc 100644 --- a/src/wine-host/bridges/vst3.h +++ b/src/wine-host/bridges/vst3.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include @@ -75,13 +76,6 @@ struct InstanceInterfaces { */ Steinberg::IPtr plug_frame_proxy; - /** - * An unmanaged raw pointer for the actual implementation behind - * `plug_frame_proxy`. This is needed for some special handling for - * `IPlugView::onSize()`. - */ - Vst3PlugFrameProxyImpl* plug_frame_proxy_impl; - /** * The base object we cast from. */ @@ -160,6 +154,106 @@ class Vst3Bridge : public HostBridge { return sockets.vst_host_callback.send_message(object, std::nullopt); } + /** + * Spawn a new thread and call `send_message()` from there, and then handle + * functions passed by calls to + * `do_mutual_recursion_or_handle_in_main_context()` on this thread until + * the original message we're trying to send has succeeded. This is a very + * specific solution to a very specific problem. When a plugin wants to + * resize itself, it will call `IPlugFrame::resizeView()` from within the + * WIn32 message loop. The host will then call `IPlugView::onSize()` on the + * plugin's `IPlugView` to actually resize the plugin. The issue is that + * that call to `IPlugView::onSize()` has to be handled from the UI thread, + * but in this sequence that thread is being blocked by a call to + * `IPlugFrame::resizeView()`. + * + * The hacky solution here is to send the message from another thread, and + * to then allow this thread to execute other functions submitted to an IO + * context. + */ + template + typename T::Response send_mutually_recursive_message(const T& object) { + using TResponse = typename T::Response; + + // This IO context will accept incoming calls from + // `do_mutual_recursion_or_handle_in_main_context()` until we receive a + // response + { + std::unique_lock lock(mutual_recursion_context_mutex); + + // In case some other thread is already calling + // `send_mutually_recursive_message()` (which should never be the + // case since this should only be called from the UI thread), we'll + // wait for that function to finish + if (mutual_recursion_context) { + mutual_recursion_context_cv.wait(lock, [&]() { + return !mutual_recursion_context.has_value(); + }); + } + + mutual_recursion_context.emplace(); + } + + // We will call the function from another thread so we can handle calls + // to from this thread + std::promise response_promise{}; + Win32Thread sending_thread([&]() { + const TResponse response = send_message(object); + + // Stop accepting additional work to be run from the calling thread + // once we receive a response + { + std::lock_guard lock(mutual_recursion_context_mutex); + mutual_recursion_context->stop(); + mutual_recursion_context.reset(); + } + mutual_recursion_context_cv.notify_one(); + + response_promise.set_value(response); + }); + + // Accept work from the other thread until we receive a response, at + // which point the context will be stopped + auto work_guard = + boost::asio::make_work_guard(*mutual_recursion_context); + mutual_recursion_context->run(); + + return response_promise.get_future().get(); + } + + /** + * Crazy functions ask for crazy naming. This is the other part of + * `send_mutually_recursive_message()`. If another thread is currently + * calling that function (from the UI thread), then we'll execute `f` from + * the UI thread using the IO context started in the above function. + * Otherwise `f` will be run on the UI thread through `main_context` as + * usual. + * + * @see Vst3Bridge::send_mutually_recursive_message + */ + template + T do_mutual_recursion_or_handle_in_main_context(F f) { + std::packaged_task do_call(f); + std::future do_call_response = do_call.get_future(); + + // If the above function is currently being called from some thread, + // then we'll submit the task to the IO context created there so it can + // be handled on that same thread. Otherwise we'll just submit it to the + // main IO context. Neither of these two functions block until `do_call` + // finish executing. + { + std::lock_guard lock(mutual_recursion_context_mutex); + if (mutual_recursion_context) { + boost::asio::dispatch(*mutual_recursion_context, + std::move(do_call)); + } else { + main_context.schedule_task(std::move(do_call)); + } + } + + return do_call_response.get(); + } + private: /** * Generate a nique instance identifier using an atomic fetch-and-add. This @@ -232,4 +326,21 @@ class Vst3Bridge : public HostBridge { */ std::map object_instances; std::mutex object_instances_mutex; + + /** + * The IO context used in `send_mutually_recursive_message()` to be able to + * execute functions from that same calling thread while we're waiting for a + * response. See the docstring there for more information. When this doesn't + * contain an IO context, this function is not being called and + * `do_mutual_recursion_or_handle_in_main_context()` should post the task + * directly to the main IO context. + */ + std::optional mutual_recursion_context; + std::mutex mutual_recursion_context_mutex; + /** + * Used to make sure only a single call to + * `send_mutually_recursive_message()` at a time can be processed (this + * should never happen, but better safe tha nsorry). + */ + std::condition_variable mutual_recursion_context_cv; }; diff --git a/src/wine-host/utils.h b/src/wine-host/utils.h index c87fc652..ef6e6355 100644 --- a/src/wine-host/utils.h +++ b/src/wine-host/utils.h @@ -71,33 +71,11 @@ class MainContext { */ template std::future run_in_context(F fn) { - std::promise result{}; - std::future future = result.get_future(); - boost::asio::dispatch(context, - [result = std::move(result), fn]() mutable { - result.set_value(fn()); - }); + std::packaged_task call_fn(std::move(fn)); + std::future response = call_fn.get_future(); + boost::asio::dispatch(context, std::move(call_fn)); - return future; - } - - /** - * The same as the above, but without returning a value. This allows us to - * wait for the task to have been run. - * - * @overload - */ - template - std::future run_in_context(F fn) { - std::promise result{}; - std::future future = result.get_future(); - boost::asio::dispatch(context, - [result = std::move(result), fn]() mutable { - fn(); - result.set_value(); - }); - - return future; + return response; } /** @@ -107,7 +85,7 @@ class MainContext { */ template void schedule_task(F fn) { - boost::asio::post(context, fn); + boost::asio::post(context, std::move(fn)); } /**