From 6c0979c506590ba4a2a1023af901f0c285c9c190 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sat, 6 May 2023 22:42:46 +0200 Subject: [PATCH] Also cache CLAP parameter infos --- CHANGELOG.md | 6 +- src/common/logging/clap.cpp | 29 ++++------ src/common/logging/clap.h | 13 +++-- src/common/serialization/clap.h | 3 +- src/common/serialization/clap/ext/params.h | 42 +++++++------- .../bridges/clap-impls/plugin-proxy.cpp | 51 +++++++++++++---- src/plugin/bridges/clap-impls/plugin-proxy.h | 26 +++++++++ src/plugin/bridges/clap.cpp | 7 ++- src/wine-host/bridges/clap.cpp | 56 +++++++++++-------- 9 files changed, 146 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b64ba55a..d9c9f80f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,9 +10,9 @@ Versioning](https://semver.org/spec/v2.0.0.html). ### Changed -- Parameter information for VST3 plugins is now queried all at once. This should - work around a bug in _Kontakt_ that would cause loading patches with lots of - exposed parameters to become very slow in **REAPER** +- Parameter information for VST3 and CLAP plugins is now queried all at once. + This should work around a bug in VST3 _Kontakt_ that would cause loading + patches with lots of exposed parameters to become very slow in **REAPER** ([#236](https://github.com/robbert-vdh/yabridge/issues/236)). - When dragging plugin windows around, yabridge now waits for the mouse buttons to be released before informing Wine about the window's new screen diff --git a/src/common/logging/clap.cpp b/src/common/logging/clap.cpp index 7a07c430..5305267d 100644 --- a/src/common/logging/clap.cpp +++ b/src/common/logging/clap.cpp @@ -60,8 +60,9 @@ bool ClapLogger::log_request(bool is_host_plugin, }); } -bool ClapLogger::log_request(bool is_host_plugin, - const clap::factory::plugin_factory::Create& request) { +bool ClapLogger::log_request( + bool is_host_plugin, + const clap::factory::plugin_factory::Create& request) { return log_request_base(is_host_plugin, [&](auto& message) { message << "clap_plugin_factory::create(host = , " "plugin_id = \"" @@ -328,20 +329,12 @@ bool ClapLogger::log_request( }); } -bool ClapLogger::log_request(bool is_host_plugin, - const clap::ext::params::plugin::Count& request) { - return log_request_base(is_host_plugin, [&](auto& message) { - message << request.instance_id << ": clap_plugin_params::count()"; - }); -} - bool ClapLogger::log_request( bool is_host_plugin, - const clap::ext::params::plugin::GetInfo& request) { + const clap::ext::params::plugin::GetInfos& request) { return log_request_base(is_host_plugin, [&](auto& message) { message << request.instance_id - << ": clap_plugin_params::get_info(param_index = " - << request.param_index << ", *param_info)"; + << ": clap_plugin_params::get_info(..., *param_info) (batched)"; }); } @@ -942,13 +935,13 @@ void ClapLogger::log_response( void ClapLogger::log_response( bool is_host_plugin, - const clap::ext::params::plugin::GetInfoResponse& response) { + const clap::ext::params::plugin::GetInfosResponse& response, + bool from_cache) { log_response_base(is_host_plugin, [&](auto& message) { - if (response.result) { - message << "true, name << "\">"; - } else { - message << "false"; + message << " for " << response.infos.size() + << " parameters"; + if (from_cache) { + message << " (from cache)"; } }); } diff --git a/src/common/logging/clap.h b/src/common/logging/clap.h index 6496e729..40bd172b 100644 --- a/src/common/logging/clap.h +++ b/src/common/logging/clap.h @@ -80,8 +80,10 @@ class ClapLogger { // log message for the response together with the request. // Main thread control messages - bool log_request(bool is_host_plugin, const clap::factory::plugin_factory::List&); - bool log_request(bool is_host_plugin, const clap::factory::plugin_factory::Create&); + bool log_request(bool is_host_plugin, + const clap::factory::plugin_factory::List&); + bool log_request(bool is_host_plugin, + const clap::factory::plugin_factory::Create&); bool log_request(bool is_host_plugin, const clap::plugin::Init&); bool log_request(bool is_host_plugin, const clap::plugin::Destroy&); bool log_request(bool is_host_plugin, const clap::plugin::Activate&); @@ -127,9 +129,7 @@ class ClapLogger { bool log_request(bool is_host_plugin, const clap::ext::note_ports::plugin::Get&); bool log_request(bool is_host_plugin, - const clap::ext::params::plugin::Count&); - bool log_request(bool is_host_plugin, - const clap::ext::params::plugin::GetInfo&); + const clap::ext::params::plugin::GetInfos&); bool log_request(bool is_host_plugin, const clap::ext::params::plugin::GetValue&); bool log_request(bool is_host_plugin, @@ -229,7 +229,8 @@ class ClapLogger { void log_response(bool is_host_plugin, const clap::ext::note_ports::plugin::GetResponse&); void log_response(bool is_host_plugin, - const clap::ext::params::plugin::GetInfoResponse&); + const clap::ext::params::plugin::GetInfosResponse&, + bool from_cache = false); void log_response(bool is_host_plugin, const clap::ext::params::plugin::GetValueResponse&); void log_response(bool is_host_plugin, diff --git a/src/common/serialization/clap.h b/src/common/serialization/clap.h index 17dfaa35..4fb04bf6 100644 --- a/src/common/serialization/clap.h +++ b/src/common/serialization/clap.h @@ -83,8 +83,7 @@ using ClapMainThreadControlRequest = clap::ext::note_name::plugin::Get, clap::ext::note_ports::plugin::Count, clap::ext::note_ports::plugin::Get, - clap::ext::params::plugin::Count, - clap::ext::params::plugin::GetInfo, + clap::ext::params::plugin::GetInfos, clap::ext::params::plugin::GetValue, clap::ext::params::plugin::ValueToText, clap::ext::params::plugin::TextToValue, diff --git a/src/common/serialization/clap/ext/params.h b/src/common/serialization/clap/ext/params.h index 77e6c23f..2d8c8dfd 100644 --- a/src/common/serialization/clap/ext/params.h +++ b/src/common/serialization/clap/ext/params.h @@ -79,45 +79,41 @@ struct ParamInfo { namespace plugin { /** - * Message struct for `clap_plugin_params::count()`. - */ -struct Count { - using Response = PrimitiveResponse; - - native_size_t instance_id; - - template - void serialize(S& s) { - s.value8b(instance_id); - } -}; - -/** - * The response to the `clap::ext::params::plugin::GetInfo` message defined + * The response to the `clap::ext::params::plugin::GetInfos` message defined * below. */ -struct GetInfoResponse { - std::optional result; +struct GetInfosResponse { + /** + * All of the plugin's parameter infos. If the plugin somehow returned an + * error for a parameter that should be in range, then this contains a + * nullopt value. + */ + std::vector> infos; template void serialize(S& s) { - s.ext(result, bitsery::ext::InPlaceOptional()); + s.container(infos, 1 << 16, [](S& s, auto& v) { + s.ext(v, bitsery::ext::InPlaceOptional{}); + }); } }; /** - * Message struct for `clap_plugin_params::get_info()`. + * Message struct for querying the information for all parameters using + * `clap_plugin_params::count()` and `clap_plugin_params::get_info()`. This + * information is then cached until the plugin tells the host that the + * parameters have changed. No specific plugins or hosts seem to require this at + * the moment, but this mimics the behavior of the VST3 bridge which needed this + * to work around a Kontakt bug. */ -struct GetInfo { - using Response = GetInfoResponse; +struct GetInfos { + using Response = GetInfosResponse; native_size_t instance_id; - uint32_t param_index; template void serialize(S& s) { s.value8b(instance_id); - s.value4b(param_index); } }; diff --git a/src/plugin/bridges/clap-impls/plugin-proxy.cpp b/src/plugin/bridges/clap-impls/plugin-proxy.cpp index d192b2b8..a61d230b 100644 --- a/src/plugin/bridges/clap-impls/plugin-proxy.cpp +++ b/src/plugin/bridges/clap-impls/plugin-proxy.cpp @@ -146,6 +146,11 @@ clap_plugin_proxy::clap_plugin_proxy(ClapPluginBridge& bridge, // getting that many of them pending_callbacks_(128) {} +void clap_plugin_proxy::clear_param_info_cache() { + std::lock_guard lock(param_info_cache_mutex_); + param_info_cache_.clear(); +} + bool CLAP_ABI clap_plugin_proxy::plugin_init(const struct clap_plugin* plugin) { assert(plugin && plugin->plugin_data); auto self = static_cast(plugin->plugin_data); @@ -719,10 +724,17 @@ clap_plugin_proxy::ext_note_ports_get(const clap_plugin_t* plugin, uint32_t CLAP_ABI clap_plugin_proxy::ext_params_count(const clap_plugin_t* plugin) { assert(plugin && plugin->plugin_data); - auto self = static_cast(plugin->plugin_data); + auto self = static_cast(plugin->plugin_data); - return self->bridge_.send_main_thread_message( - clap::ext::params::plugin::Count{.instance_id = self->instance_id()}); + // The infos for all of a plugin's parameters is queried in a batch and then + // cached. This was needed in the VST3 bridge to work around a bug in + // Kontakt (see the similarly named function there for more information), + // and the CAP bridge does the same thing for consistency's sake. This cache + // is cleared when the plugin asks the host to rescan its parameters. + self->maybe_query_parameter_info(); + + std::lock_guard lock(self->param_info_cache_mutex_); + return self->param_info_cache_.size(); } bool CLAP_ABI @@ -730,16 +742,18 @@ clap_plugin_proxy::ext_params_get_info(const clap_plugin_t* plugin, uint32_t param_index, clap_param_info_t* param_info) { assert(plugin && plugin->plugin_data && param_info); - auto self = static_cast(plugin->plugin_data); + auto self = static_cast(plugin->plugin_data); - const clap::ext::params::plugin::GetInfoResponse response = - self->bridge_.send_main_thread_message( - clap::ext::params::plugin::GetInfo{ - .instance_id = self->instance_id(), - .param_index = param_index}); - if (response.result) { - response.result->reconstruct(*param_info); + // See above + self->maybe_query_parameter_info(); + std::lock_guard lock(self->param_info_cache_mutex_); + if (param_index > self->param_info_cache_.size()) { + return false; + } + + if (const auto& info = self->param_info_cache_[param_index]) { + info->reconstruct(*param_info); return true; } else { return false; @@ -912,3 +926,18 @@ bool CLAP_ABI clap_plugin_proxy::ext_voice_info_get(const clap_plugin_t* plugin, return false; } } + +void clap_plugin_proxy::maybe_query_parameter_info() { + std::lock_guard lock(param_info_cache_mutex_); + + // We'll assume that the plugin has at least one parameter. If it does not + // have any parameters then everything will work as expected, except that + // the parameter count is not cached. + if (param_info_cache_.empty()) { + const clap::ext::params::plugin::GetInfosResponse response = + bridge_.send_main_thread_message( + clap::ext::params::plugin::GetInfos{.instance_id = + instance_id()}); + param_info_cache_ = std::move(response.infos); + } +} diff --git a/src/plugin/bridges/clap-impls/plugin-proxy.h b/src/plugin/bridges/clap-impls/plugin-proxy.h index d791fb47..b465284a 100644 --- a/src/plugin/bridges/clap-impls/plugin-proxy.h +++ b/src/plugin/bridges/clap-impls/plugin-proxy.h @@ -36,6 +36,7 @@ #include #include +#include "../../common/serialization/clap/ext/params.h" #include "../../common/serialization/clap/plugin.h" // Forward declaration to avoid circular includes @@ -157,6 +158,13 @@ class clap_plugin_proxy { return response_future; } + /** + * Clear the parameter information cache. Needs to be called when the plugin + * calls `clap_host_params::rescan()`. This cache is used to fetch + * information for all parameters at once. + */ + void clear_param_info_cache(); + /** * The `clap_host_t*` passed when creating the instance. Any callbacks made * by the proxied plugin instance must go through here. @@ -298,6 +306,14 @@ class clap_plugin_proxy { clap_voice_info_t* info); private: + /** + * Query information for all of the plugin's parameters and writes the + * results to `param_info_cache_` if necessary. Otherwise does nothing. + * Acquires a lock on the struct in the process, so it must not be locked + * before calling this function. + */ + void maybe_query_parameter_info(); + ClapPluginBridge& bridge_; size_t instance_id_; clap::plugin::Descriptor descriptor_; @@ -377,4 +393,14 @@ class clap_plugin_proxy { * socket needs to make a main thread function call, it will */ rigtorp::MPMCQueue pending_callbacks_; + + /** + * Caches the info structs for all of a plugin's parameters. This is queried + * all at once the first time the interacts with the param extension. When + * the plugin asks the host to rescan its parameters, this cache is cleared. + * The `std::optional` is used to handle the case where a plugin may return + * `false` when querying info for a parameter that should be in range. + */ + std::vector> param_info_cache_; + std::mutex param_info_cache_mutex_; }; diff --git a/src/plugin/bridges/clap.cpp b/src/plugin/bridges/clap.cpp index 62f4e0d9..91247fdf 100644 --- a/src/plugin/bridges/clap.cpp +++ b/src/plugin/bridges/clap.cpp @@ -251,8 +251,13 @@ ClapPluginBridge::ClapPluginBridge(const ghc::filesystem::path& plugin_path) run_on_main_thread( plugin_proxy, - [&, host = plugin_proxy.host_, + [&, plugin_proxy = &plugin_proxy, + host = plugin_proxy.host_, params = plugin_proxy.host_extensions_.params]() { + // Parameter information is cached and fetched in + // bulk as an optimization + plugin_proxy->clear_param_info_cache(); + params->rescan(host, request.flags); }) .wait(); diff --git a/src/wine-host/bridges/clap.cpp b/src/wine-host/bridges/clap.cpp index 9e67a68a..d8e58f7b 100644 --- a/src/wine-host/bridges/clap.cpp +++ b/src/wine-host/bridges/clap.cpp @@ -241,11 +241,11 @@ void ClapBridge::run() { if (plugin) { register_plugin_instance(plugin, std::move(host_proxy)); - return clap::factory::plugin_factory::CreateResponse{ - .instance_id = instance_id}; + return clap::factory::plugin_factory:: + CreateResponse{.instance_id = instance_id}; } else { - return clap::factory::plugin_factory::CreateResponse{ - .instance_id = std::nullopt}; + return clap::factory::plugin_factory:: + CreateResponse{.instance_id = std::nullopt}; } }) .get(); @@ -716,30 +716,40 @@ void ClapBridge::run() { .result = std::nullopt}; } }, - [&](const clap::ext::params::plugin::Count& request) - -> clap::ext::params::plugin::Count::Response { + [&](const clap::ext::params::plugin::GetInfos& request) + -> clap::ext::params::plugin::GetInfos::Response { const auto& [instance, _] = get_instance(request.instance_id); // We'll ignore the main thread requirement for simple array - // lookups to avoid the synchronisation costs in hot code paths - return instance.extensions.params->count(instance.plugin.get()); - }, - [&](const clap::ext::params::plugin::GetInfo& request) - -> clap::ext::params::plugin::GetInfo::Response { - const auto& [instance, _] = get_instance(request.instance_id); + // lookups to avoid the synchronisation costs in hot code paths. + // We also deviate from yabridge's usual bridging approach by + // querying the infos for all parameters at once and then + // caching them until told to invalidate that cache. This was + // required to get acceptable performance when loading certain + // patches in Kontakt's VST3 version. To be consistent, CLAP + // bridging uses a similar caching mechanism. + // TODO: Also do this for VST2, will require replacing the VST2 + // bridging with an approach similar to VST3 and CLAP + const uint32_t num_parameters = + instance.extensions.params->count(instance.plugin.get()); - // We'll ignore the main thread requirement for simple array - // lookups to avoid the synchronisation costs in hot code paths - clap_param_info_t param_info{}; - if (instance.extensions.params->get_info(instance.plugin.get(), - request.param_index, - ¶m_info)) { - return clap::ext::params::plugin::GetInfoResponse{ - .result = param_info}; - } else { - return clap::ext::params::plugin::GetInfoResponse{ - .result = std::nullopt}; + std::vector> infos; + infos.reserve(num_parameters); + for (uint32_t i = 0; i < num_parameters; i++) { + // This should never fail, but we can't make things up and + // we don't want to change parameter orders around so we'll + // store a nullopt if the plugin returns an error here + clap_param_info_t info{}; + if (instance.extensions.params->get_info( + instance.plugin.get(), i, &info)) { + infos.push_back(std::move(info)); + } else { + infos.push_back(std::nullopt); + } } + + return clap::ext::params::plugin::GetInfosResponse{ + .infos = std::move(infos)}; }, [&](const clap::ext::params::plugin::GetValue& request) -> clap::ext::params::plugin::GetValue::Response {