From 42032c5c2dfda0aa3844a67adc091a0994418822 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sat, 7 Nov 2020 18:17:21 +0100 Subject: [PATCH] Fix the old accumulative process() function --- CHANGELOG.md | 3 +++ src/plugin/plugin-bridge.cpp | 42 ++++++++++++++++++++++++++---------- src/plugin/plugin-bridge.h | 19 +++++++++++++++- 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 908b068a..33a14335 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,9 @@ Versioning](https://semver.org/spec/v2.0.0.html). plugin could cause a crash. In practice this was only reproducible during the plugin scanning process when rapidly adding and removing a large number of plugins in a single group. +- Fixed the implementation of the accumulative `process()` function. As far as + I'm aware no VST hosts made in the last few decades event use this, but it + just feels wrong to have an incorrect implementation as part of yabridge. ## [1.7.1] - 2020-10-23 diff --git a/src/plugin/plugin-bridge.cpp b/src/plugin/plugin-bridge.cpp index 761bbd1c..bb49d412 100644 --- a/src/plugin/plugin-bridge.cpp +++ b/src/plugin/plugin-bridge.cpp @@ -16,6 +16,8 @@ #include "plugin-bridge.h" +#include + // Generated inside of the build directory #include #include @@ -490,7 +492,7 @@ intptr_t PluginBridge::dispatch(AEffect* /*plugin*/, data, option); } -template +template void PluginBridge::do_process(T** inputs, T** outputs, int sample_frames) { // The inputs and outputs arrays should be `[num_inputs][sample_frames]` and // `[num_outputs][sample_frames]` floats large respectfully. @@ -513,8 +515,23 @@ void PluginBridge::do_process(T** inputs, T** outputs, int sample_frames) { assert(response_buffers.size() == static_cast(plugin.numOutputs)); for (int channel = 0; channel < plugin.numOutputs; channel++) { - std::copy(response_buffers[channel].begin(), - response_buffers[channel].end(), outputs[channel]); + if constexpr (replacing) { + std::copy(response_buffers[channel].begin(), + response_buffers[channel].end(), outputs[channel]); + } else { + // The old `process()` function expects the plugin to add its output + // to the accumulated values in `outputs`. Since no host is ever + // going to call this anyways we won't even bother with a separate + // implementation and we'll just add `processReplacing()` results to + // `outputs`. + std::transform(std::execution::unseq, + response_buffers[channel].begin(), + response_buffers[channel].end(), outputs[channel], + outputs[channel], + [](const T& new_value, T& current_value) -> T { + return new_value + current_value; + }); + } } // Plugins are allowed to send MIDI events during processing using a host @@ -532,18 +549,25 @@ void PluginBridge::do_process(T** inputs, T** outputs, int sample_frames) { incoming_midi_events.clear(); } +void PluginBridge::process(AEffect* /*plugin*/, + float** inputs, + float** outputs, + int sample_frames) { + do_process(inputs, outputs, sample_frames); +} + void PluginBridge::process_replacing(AEffect* /*plugin*/, float** inputs, float** outputs, int sample_frames) { - do_process(inputs, outputs, sample_frames); + do_process(inputs, outputs, sample_frames); } void PluginBridge::process_double_replacing(AEffect* /*plugin*/, double** inputs, double** outputs, int sample_frames) { - do_process(inputs, outputs, sample_frames); + do_process(inputs, outputs, sample_frames); } float PluginBridge::get_parameter(AEffect* /*plugin*/, int index) { @@ -697,12 +721,8 @@ void process_proxy(AEffect* plugin, float** inputs, float** outputs, int sample_frames) { - // FIXME: This is incorrect, and I only noticed just now. I'm 99% sure no - // hosts actually use this, but this will overwrite the buffer. On - // the plugin side we do properly handle plugins that only support - // the old cumulative process function. - return get_bridge_instance(*plugin).process_replacing( - plugin, inputs, outputs, sample_frames); + return get_bridge_instance(*plugin).process(plugin, inputs, outputs, + sample_frames); } void process_replacing_proxy(AEffect* plugin, diff --git a/src/plugin/plugin-bridge.h b/src/plugin/plugin-bridge.h index 468c8fd6..c9da720f 100644 --- a/src/plugin/plugin-bridge.h +++ b/src/plugin/plugin-bridge.h @@ -60,6 +60,18 @@ class PluginBridge { intptr_t value, void* data, float option); + /** + * This is the old, accumulative version of `processReplacing()`. As far as + * I'm aware no host from the last 20 years will use this (since it's not + * very practical), but we have to support this anyways. Because this is not + * used, we'll just reuse your `process_replacing()` implementation (which + * actually falls back to `process()` if the plugin somehow does not support + * the former). + */ + void process(AEffect* plugin, + float** inputs, + float** outputs, + int sample_frames); /** * Ask the VST plugin to process audio for us. If the plugin somehow does * not support `processReplacing()` and only supports the old `process()` @@ -90,11 +102,16 @@ class PluginBridge { * @tparam T The sample type. Should be either `float` for single preceision * audio processing called through `processReplacing`, or `double` for * double precision audio through `processDoubleReplacing`. + * @tparam replacing Whether or not `outputs` should be replaced by the new + * processed audio. This is the normal behaviour for `processReplacing()`. + * If this is set to `false` then the results are added to the existing + * values in `outputs`. No host will use this last behaviour anymore, but + * it's part of the VST2.4 spec so we have to support it. * * @see PluginBridge::process_replacing * @see PluginBridge::process_double_replacing */ - template + template void do_process(T** inputs, T** outputs, int sample_frames); /**