From cfb171c9910c8438bd496a750282c7b19ebe9edf Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sat, 30 Jan 2021 00:20:35 +0100 Subject: [PATCH] Remove cache_time_info and always cache time info It sort of goes against yabridge's principles to not do these unnecessary `audioMasterGetTime()` calls if the plugin does that, but it also hurts the user experience to not have this as a default. --- CHANGELOG.md | 7 +++++++ README.md | 5 +---- src/common/configuration.cpp | 8 +------- src/common/configuration.h | 15 --------------- src/plugin/bridges/common.h | 3 --- src/wine-host/bridges/vst2.cpp | 17 +++++++---------- src/wine-host/bridges/vst2.h | 18 +++++++++++++----- 7 files changed, 29 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24e50e10..889a0f4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,13 @@ TODO: Add an updated screenshot with some fancy VST3-only plugins to the readme with, and it then applies the change conditionally to be able to support building with both older and newer versions of Wine. +### Removed + +- The `cache_time_info` option for VST2 plugins has been removed and this + behaviour is now always enabled. This option would cache VST2 time information + during a single processing cycle in case the plugin asks for this information + more than once. + ### Fixed - VST2 plugin editor resizing in **REAPER** would not cause the FX window to be diff --git a/README.md b/README.md index 96192a8e..8b539f3b 100644 --- a/README.md +++ b/README.md @@ -288,7 +288,7 @@ plugin._ | Option | Values | Description | | --------------------- | -------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `cache_time_info` | `{true,false}` | Compatibility option for plugins that call `audioMasterGetTime()` multiple times during a single processing cycle. With this option subsequent calls during a single audio processing cycle will reuse the value returned by the first call to this function. This is a bug in the plugin, and this option serves as a temporary workaround until the plugin fixes the issue. | +| `cache_time_info` | `{true,false}` | Compatibility option for plugins that call `audioMasterGetTime()` multiple times during a single processing cycle. With this option subsequent calls during a single audio processing cycle will reuse the value returned by the first call to this function. This is a bug in the plugin, and this option serves as a temporary workaround until the plugin fixes the issue. _This is enabled by default on the master branch._ | | `editor_double_embed` | `{true,false}` | Compatibility option for plugins that rely on the absolute screen coordinates of the window they're embedded in. Since the Wine window gets embedded inside of a window provided by your DAW, these coordinates won't match up and the plugin would end up drawing in the wrong location without this option. Currently the only known plugins that require this option are _PSPaudioware_ plugins with expandable GUIs, such as E27. Defaults to `false`. | | `editor_force_dnd` | `{true,false}` | This option forcefully enables drag-and-drop support in _REAPER_. Because REAPER's FX window supports drag-and-drop itself, dragging a file onto a plugin editor will cause the drop to be intercepted by the FX window. This makes it impossible to drag files onto plugins in REAPER under normal circumstances. Setting this option to `true` will strip drag-and-drop support from the FX window, thus allowing files to be dragged onto the plugin again. Defaults to `false`. _This option is only availble on the master branch._ | | `editor_xembed` | `{true,false}` | Use Wine's XEmbed implementation instead of yabridge's normal window embedding method. Some plugins will have redrawing issues when using XEmbed and editor resizing won't always work properly with it, but it could be useful in certain setups. You may need to use [this Wine patch](https://github.com/psycha0s/airwave/blob/master/fix-xembed-wine-windows.patch) if you're getting blank editor windows. Defaults to `false`. _This option is only availble on the master branch._ | @@ -324,9 +324,6 @@ editor_double_embed = true ["Analog Lab 3.so"] editor_xembed = true -["SWAM Cello 64bit.so"] -cache_time_info = true - ["sforzando VST_x64.so"] editor_force_dnd = true frame_rate = 24 diff --git a/src/common/configuration.cpp b/src/common/configuration.cpp index 9594fddc..6c81a260 100644 --- a/src/common/configuration.cpp +++ b/src/common/configuration.cpp @@ -78,13 +78,7 @@ Configuration::Configuration(const fs::path& config_path, // their defaults. At this point I'd really wish C++ could do pattern // matching. for (const auto& [key, value] : table) { - if (key == "cache_time_info") { - if (const auto parsed_value = value.as_boolean()) { - cache_time_info = parsed_value->get(); - } else { - invalid_options.push_back(key); - } - } else if (key == "editor_double_embed") { + if (key == "editor_double_embed") { if (const auto parsed_value = value.as_boolean()) { editor_double_embed = parsed_value->get(); } else { diff --git a/src/common/configuration.h b/src/common/configuration.h index f0866e13..55b77643 100644 --- a/src/common/configuration.h +++ b/src/common/configuration.h @@ -74,20 +74,6 @@ class Configuration { Configuration(const boost::filesystem::path& config_path, const boost::filesystem::path& yabridge_path); - /** - * If set to `true`, then after an `audioMasterGetTime()` call all - * subsequent calls to that function during a single processing cycle will - * reuse the results from the first call. In theory it would be a bug on the - * host's side if this did _not_ return the same value every time, but since - * yabridge tries to not have any behaviour of its own and since this - * function should only be called at most once every processing cycle this - * option is not enabled by default. An example of a situation where this - * does become necessary is the SWAM Cello plugin, which calls - * `audioMasterGetTime()` for every sample instead of only once. This would - * tank performance without this caching behaviour. - */ - bool cache_time_info = false; - /** * If this is set to `true`, then the plugin editor should be embedded in * yet another window. This would result in an embedding sequence of @@ -180,7 +166,6 @@ class Configuration { template void serialize(S& s) { - s.value1b(cache_time_info); s.value1b(editor_double_embed); s.value1b(editor_force_dnd); s.value1b(editor_xembed); diff --git a/src/plugin/bridges/common.h b/src/plugin/bridges/common.h index b0fa8706..1ebdfec4 100644 --- a/src/plugin/bridges/common.h +++ b/src/plugin/bridges/common.h @@ -168,9 +168,6 @@ class PluginBridge { init_msg << "other options: "; std::vector other_options; - if (config.cache_time_info) { - other_options.push_back("hack: time info cache"); - } if (config.editor_double_embed) { other_options.push_back("editor: double embed"); } diff --git a/src/wine-host/bridges/vst2.cpp b/src/wine-host/bridges/vst2.cpp index e7da986b..ce55485c 100644 --- a/src/wine-host/bridges/vst2.cpp +++ b/src/wine-host/bridges/vst2.cpp @@ -178,9 +178,7 @@ Vst2Bridge::Vst2Bridge(MainContext& main_context, // value returned by this function during an audio // processing cycle will be reused for the rest of the // cycle. - if (config.cache_time_info) { - time_info.reset(); - } + cached_time_info.reset(); // Since the host should only be calling one of `process()`, // processReplacing()` or `processDoubleReplacing()`, we can all @@ -548,15 +546,14 @@ intptr_t Vst2Bridge::host_callback(AEffect* effect, void* data, float option) { // HACK: Workaround for a bug in SWAM Cello where it would call - // `audioMasterGetTime()` once for every sample. When this option is - // enabled `time_info` should be reset in the process function. The - // `time_info` value is assigned inside of - // `HostCallbackDataConverter::write()`. - if (config.cache_time_info && time_info) { - return reinterpret_cast(&*time_info); + // `audioMasterGetTime()` once for every sample. The `time_info` value + // is assigned inside of `HostCallbackDataConverter::write()`. At the + // beginning of the processing cycle this value will be reset. + if (cached_time_info) { + return reinterpret_cast(&*cached_time_info); } - HostCallbackDataConverter converter(effect, time_info); + HostCallbackDataConverter converter(effect, cached_time_info); return sockets.vst_host_callback.send_event(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 5c747b39..c7be2554 100644 --- a/src/wine-host/bridges/vst2.h +++ b/src/wine-host/bridges/vst2.h @@ -77,12 +77,20 @@ class Vst2Bridge : public HostBridge { intptr_t host_callback(AEffect*, int, int, intptr_t, void*, float); /** - * With the `audioMasterGetTime` host callback the plugin expects the return - * value from the calblack to be a pointer to a VstTimeInfo struct. If the - * host did not support a certain time info query, than we'll store the - * returned null pointer as a nullopt. + * The time information returned by the host after the plugin calls + * `audioMasterGetTime()`. We'll have to return a pointer to this, so we'll + * store it here. If the host returned a null pointer, then we'll just store + * a nullopt. + * + * We'll keep this information around for the entire processing cycle, in + * case a plugin somehow calls this function more than once, like the SWAM + * Cello plugin does. + * + * XXX: We don't have any logging for when the plugin calls + * `audioMasterGetTime()` more than once, but printing some message + * 44100 times per second also doesn't sound great */ - std::optional time_info; + std::optional cached_time_info; private: /**