Add support for multi-channel plugins#7459
Conversation
|
While still doing a review, I wonder if the |
Conflicts: - include/Effect.h - include/InstrumentTrackView.h - include/RemotePlugin.h - plugins/GranularPitchShifter/GranularPitchShifterEffect.h - src/core/Effect.cpp - src/core/RemotePlugin.cpp - src/gui/tracks/InstrumentTrackView.cpp
JohannesLorenz
left a comment
JohannesLorenz
left a comment
There was a problem hiding this comment.
Reviewed until 8824180
It's not used yet, but it makes it consistent with effects and is a step towards allowing the same functionality that effects have.
|
Hey, uh, sorry to bother you all, but what about Ring Mod Sidechain plugins like Kilohearts' Compactor? I don't seem to find any tutorials for these kinds of plugins in a way that they work inside LMMS |
|
@RainbowShatter |
This is slightly more thread-safe since the lock now covers the calls to `Effect::isSleeping()` and `AudioPorts::active()`, which matches what we did prior to PR LMMS#7484.
- Add `initialized` method to `AudioPorts::Buffer` interface - Make `AudioPorts::active()` mean `AudioPortsModel` and the buffers are fully initialized and ready to be used by the audio processor's process method (`processImpl`) - Make `AudioPorts::buffers()` simply return the buffers if it is able to - Rename `bufferPropertiesChanged` virtual method to `bufferPropertiesChanging` in `AudioPortsModel`. This allows implementations of the virtual method to to check what the old channel counts were, which may be useful. - Make the cached data in `AudioPortsModel` private and add protected getters - Simplify the `RemotePluginAudioPorts` implementation now that "active" and "initialized" are well-defined - Fix minor bug where audio ports for VST plugins were activated before the RemotePlugin was fully initialized - Simplify `AudioPortsModel::Matrix::setTrackChannelCount` and `AudioPortsModel::Matrix::setChannelCount`
Make Effect's running/sleeping terminology more consistent and understandable. Now a "running" plugin is a plugin that is processing audio, and a "sleeping" plugin is a plugin put to sleep by auto-quit. "Running" implies the plugin is enabled, not sleeping (awake), is okay, and not set to "don't run".
The AudioBus now keeps track of which of its track channels are known to be silent. This information can be used to avoid zeroing buffers unnecessarily. But more importantly, it also allowed me to rework the auto-quit system for effects. With this commit, sleeping effects determine whether they need to wake up by simply checking if there are any non-quiet track channels routed into their inputs. This is an O(1) task now that AudioBus keeps track of that info. The previous auto-quit system was hacky and made every effect in the chain wake up at once, even if there was no input noise routed to one of their inputs. With the new system, effects can wake up independently of each other and only when needed. - AudioBus moved to its own .h/.cpp files - Added a bitset to AudioBus to store which track channels are currently known to be quiet - Added AudioBus methods for checking for input noise, sanitizing buffers, clearing buffers, and updating silence flags - Integrated the new AudioBus functionality into AudioPlugin, AudioPorts::Router, AudioBusHandle, EffectChain, Mixer, and other places where needed in order for the silence flags of track channels to propagate and update correctly - Moved some AudioPorts::Router functionality to AudioBus - Make Effect::startRunning and Effect::stopRunning protected now that they are not used outside the effect - Updated the unit tests
JohannesLorenz
left a comment
JohannesLorenz
left a comment
There was a problem hiding this comment.
Reviewed until 413ea6f . Only editorials/questions.
| * NOTE: The following five members (`kind`, `interleaved`, `inputs`, `outputs`, and `inplace`) | ||
| * determine the signature of the process method that will need to be implemented. | ||
| * | ||
| * For an `Effect` or a MIDI-based `Instrument`, the signature will one of two options: |
There was a problem hiding this comment.
| * For an `Effect` or a MIDI-based `Instrument`, the signature will one of two options: | |
| * For an `Effect` or a MIDI-based `Instrument`, the signature will be one of two options: |
| * Or: | ||
| * ProcessStatus processImpl(InputOutputBuffers inOut); | ||
| * | ||
| * Explanation: |
There was a problem hiding this comment.
Maybe replace this by "whereas the buffer types are determined as follows:" or "Explanation of the buffer arguments" or something similar, that makes more clear that this will explain the buffer arguments?
| * | ||
| * (Note that when `inplace == true`, the channel count is `std::max(inputs, outputs)` and the channel counts | ||
| * are not allowed to differ unless one of them is zero. This cannot be an instrument because the output | ||
| * count is zero.) |
There was a problem hiding this comment.
Wait, you mean, "because the input count is 2"?
| //! Compile-time customizations for audio ports | ||
| struct AudioPortsSettings | ||
| { | ||
| //! The audio data type used by the processor |
There was a problem hiding this comment.
I know this is a non-blocker for this PR, but @sakertooth for your reminder, there are still questions open to you in this comment.
| class AudioPlugin<Instrument, settings, AudioPortsT> | ||
| : public Instrument | ||
| , public AudioProcessingMethod<Instrument, settings> | ||
| , public ProcessMutex |
There was a problem hiding this comment.
I find this use of inheritation a bit weird. Of course, an audio plugin is usually not a ProcessMutex, it just can have one (member instead of inheritance? Or inherit as HasProcessMutex? ProcessLockable? etc)?
The same could also be said about that AudioPlugin is no a ...Method, maybe HasAudioProcessingMethod would be clearer.
Though in both cases, this is totally subjective, and my comment here should be seen as optional.
There was a problem hiding this comment.
I find this use of inheritation a bit weird. Of course, an audio plugin is usually not a ProcessMutex, it just can have one (member instead of inheritance
I agree. While this PR probably works fine, the design/architecture is somewhat unintuitive. When I see AudioPlugin, I would be thinking along the lines of a base class for all kinds of plugins, e.g VST, LV2, etc. I think I mentioned this previously in the PR.
I think what happened is that the current design of the codebase was causing a lot of difficulty so @messmerd seems to have tried to circumvent and circle around the code to be able to even add the feature. Could be wrong though, @messmerd feel free to pitch in.
What's good is that the feature is there, but we are probably are going to have to figure out things from a maintenance perspective later.
Edit: Just want to clarify that I am okay with merge right now because this brings an important feature and its working. We can work things out later if its too much effort to try and resolve some of the design confusions currently.
There was a problem hiding this comment.
I'm free to change my mental model to at least try to understand though, but it will be hard.
| */ | ||
| void init() | ||
| { | ||
| if (!AudioPortsModel::initialized()) { return; } |
There was a problem hiding this comment.
I assume this line is to protect against virtual calls from CTOR? Shouldn't this bring a warning then, or better even throw? Or is there a use case to handle this gracefully?
Same question a few lines below.
|
Hopefully there is a way we can split this PR further. I wasn't able to review this properly I think because of the size. Its a good and needed feature, but I can't properly determine the extent of what the PR changes (without spending quite a bit of time). |
6 participants
This PR adds support for instrument and effect plugins with multiple input/output channels, as well as a new "Plugin Pin Connector" feature to control how audio is routed in and out of plugins. The plugin pin connector is based on a REAPER feature of the same name.
Supersedes "L/R routing" from #7247.
Multi-channel instrument:
Stereo, mono, and multi-channel effects:
The Plugin Pin Connector window provides automatable matrix widgets that provide users with full control over how audio is routed in and out of a plugin, including plugins with multiple input or output channels.
Pin connector support has been implemented for VST instruments, ZynAddSubFX, and all effects plugins, though only VST plugins currently support multiple channels. LV2, CLAP, LADSPA (?), and Carla plugins could also offer support for multiple channels, but that work hasn't been done yet.
Use cases
Track channel explanation
Pin connector rules
Changes
AudioBus: A non-owning span ofstd::span<const SampleFrame>, i.e. an audio bus or collection of track channel pairs.AudioPlugin: Interface for audio plugins which provides audio ports and compile-time customizations.AudioPorts: Interface for audio ports. Consists of anAudioPortsModel, anAudioPorts::Bufferinterface, and anAudioPorts::Router. Also has active/inactive states to support plugins with a "plugin not loaded" state.AudioPorts::Buffer: Interface for accessing input/output audio buffers. Defined according toAudioPortsConfigNTTP.AudioPorts::Router: Uses the model to perform routing of audio in and out of the audio processor.AudioPortsModel: The model forPinConnector. Manages the runtime information of an audio port such as channel counts, channel names, and pin connections.AudioPortsSettings: A simple struct with data describing an audio port (number of channels, data type, in-place, etc.). Used at compile-time as a NTTP for several class templates.ConfigurableAudioPorts: A customAudioPortsimplementation that offers the ability to switch between betweenRemotePlugin's shared memory audio buffer or a local audio buffer at runtime.CustomAudioPorts: A convenience class that can be inherited from to implement a customAudioPort. This class is not strictly necessary to have, but takes care of some boilerplate.PluginAudioPorts: General purpose implementation ofAudioPortswhich fits the needs of most audio plugins.PluginAudioPortsBuffer: General purpose implementation ofAudioPorts::Bufferwhich fits the needs of most audio plugins.PinConnector: The GUI forAudioPortsModel. Based on the Plug-in Pin Connector from REAPER.RemotePluginAudioPorts: A customAudioPortsimplementation forRemotePlugin. Note that due toRemotePluginnot being a class template, there is noRemotePluginAudioPortsBufferyet. OtherwiseRemotePluginwould have aRemotePluginAudioPortsBufferandRemotePluginAudioPortswould simply get it from it for itsaudioBuffers()implementation. Instead,RemotePluginAudioPortsimplements the audio buffer itself which is a bit strange but works.RemotePluginAudioPortsController: A class that allowsRemotePluginto communicate withRemotePluginAudioPorts. This is currently needed becauseRemotePluginis not (yet) a class template that accepts aAudioPortsConfigNTTP like most other new classes, so it can't just store a pointer to theRemotePluginAudioPortsclass template itself.TrackOperationsWidget's menu for instruments that support itlmms::unreachable: A stand-in for C++23'sstd::unreachableSampleFrame*parameters (which is interleaved data), so I replaced those withfloat*, which can be any data layouttests/src/core/AudioPortsTest.cppFuture
This PR only adds multi-channel support to VST plugins, but there are already plans to extended support to LV2, CLAP, and possibly Carla and LADSPA after this PR is merged. For CLAP, the pin connector could also provide a dropdown for selecting the audio port configuration.
For consistency's sake, we may eventually want all instruments to provide a pin connector, including
NotePlayHandle-based instruments which are currently unsupported due to threading issues when accessing the buffer. However, other than SlicerT which may benefit from having outputs for individual slices, or plugins like SID whose emulator is in reality usingshortas its sample type and has only one channel (the handling of which could be simplified by the pin connector), there is not as strong a motivation for adding pin connector support like there is for MIDI-based instruments like VST, LV2, CLAP, etc. which all support a variable number of channels and other related features.Before full audio routing functionality is introduced, we may want to provide support for adding new track channels to Instrument Tracks and Mixer Channels.
This would enable full use of band splitter plugins and drum machine plugins with multiple output channels without the pain involved in implementing full track-channel-to-track-channel audio routing. The Pin Connector is already designed to work with track channels >= 2, but since LMMS only has a fixed number of 2 track channels currently, we have only witnessed a mere 2% of its power.
TODO
lmms::Spanwithstd::spanSharedMemory::size()?SampleFrame-based plugins with default connectionsDecide whether to change buffer update method parameters, move frame count to common location, etc.Consider for a follow-up PRPinConnectorAudioPortsModel used processor channels / track channels, ...)TODO (for follow-up PRs)
AudioPluginsupport forNotePlayHandle-based pluginsWith the "direct routing" optimization, it may be possible to implement this without any changes required, so long as the pin connector is disabled. But for pin connector support, it may require a custom buffer type for play handles.
AudioPortsSettingswithinputs == 0oroutputs == 0be implicitly in-place. (Will requireRemotePluginAudioPortsrefactor to support in-place processing due to Zyn using it. Only doable afterRemotePluginClientis compiled in C++20 mode.)std::span<SampleFrame>withInterleavedBufferView<float, 2>?BusInfoto provide info about busses / channel groups. This could be used to identify which channels are the main channels and which are the aux (sidechain) channels, andAudioPortsModelcould then use that information when setting the default pin connections. It could also be used to improve the pin connector GUI by marking and potentially naming channel groups.RoutingInfoprovides), the wet/dry mixing for effects could be moved outside the process method, at least for plugins without in-place processing.shortbufferRemotePlugina template and addRemotePluginAudioBufferinstead of hacky stuff I'm doing currently (depends onRemoteVstPluginworking when built with C++20)