Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multi-channel plugins #7459

Open
wants to merge 68 commits into
base: master
Choose a base branch
from

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented Aug 17, 2024

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.

Supercedes "L/R routing" from #7247.

Multi-channel instrument:
multi_channel_vst_instrument

Stereo, mono, and multi-channel effects:
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
The term "track channel" is borrowed from REAPER. A track channel is a mono audio pipeline.
For Instrument Tracks, track channels run from the instrument output, through each effect plugin in the effects chain, then are routed to a mixer channel.
For Mixer Channels, there are also track channels, but the audio source is the mixer input instead of an instrument.

Currently LMMS only has 2 track channels - the main unnamed L/R pair. In the future, I hope LMMS will support user-created track channels beyond the main pair similar to REAPER. They are a prerequisite for audio routing.

Pin connector rules

  • Reading from a track channel (i.e routing an LMMS channel to a plugin input) does not have any effect on the track channel's audio data
  • Writing to a track channel (i.e. routing a plugin output to an LMMS channel) overwrites whatever audio data was already in that track channel
  • If a plugin does not write to a track channel, the track channel is unmodified (bypass)
  • If multiple inputs are routed to a single output channel, they are summed together (i.e. multiple LMMS channels to 1 plugin input or multiple plugin outputs to 1 LMMS channel)

Changes

  • PluginPinConnector: Pin connector model + audio routing methods
  • PluginPinConnectorView: Pin connector window
  • PluginAudioPort: ...
  • AudioPluginInterface: ...
  • AudioPluginBufferInterface: ...
  • Added pin connector button to every effect in effects chains
  • Added pin connector button to TrackOperationsWidget's menu for instruments that support it
  • lmms::unreachable: A stand-in for C++23's std::unreachable
  • Pin connector integration for Vestige, ZynAddSubFX, and all effects
  • Multi-channel support for VSTs
  • The Remote Plugin code was using both interleaved and non-interleaved audio data layouts but reinterpret-casting data to SampleFrame* parameters (which is interleaved data), so I replaced those with float*, which can be any data layout
  • ...

Future

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 using short as 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

  • Fix saving/loading pin connector settings
  • Replace lmms::Span with std::span
  • Use SharedMemory::size()?
  • Optimize routing for SampleFrame-based plugins with default connections
  • Move Vestige pin connector button to Instrument Track's menu?
  • Decide whether to change buffer update method parameters, move frame count to common location, etc. Consider for a follow-up PR
  • Disable pin automation for now? (Can easily add back in later, but cannot remove after users start using it)
  • Rename some classes? (AudioPort --> ExternalAudioPort? ...) Decide on bus / track channel terminology
  • Improve GUI? (layouts, window size, custom icons)
  • Address review comments

@JohannesLorenz JohannesLorenz self-requested a review August 18, 2024 16:38
@JohannesLorenz
Copy link
Contributor

While still doing a review, I wonder if the SampleFrame redesign can go into a separate PR? However, then would we have 2 PRs based on another (alternatively, simply squashing this PR and splitting it up into 2 atomic commits might also make the review more easy, I guess).

@messmerd messmerd removed the rework required Pull requests which must be reworked to make it able to merge or review label Dec 15, 2024
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review finished.

After this review, follow-ups would be possible:

  • optional second functional code review
  • style review
  • test review

For testing, I would at least test the following:

  • All instruments and effects must be tested, especially the pin-connector (if available) and the inputs and outputs shall be checked.
  • Both VST mono and stereo effects and instruments should be tested
  • The "pin connector rules" (see the original post on top of this thread) shall be tested with these different types of instruments/effects
  • Loading/Saving must be tested carefully, especially with automated and controlled pins

src/core/PluginPinConnector.cpp Outdated Show resolved Hide resolved
src/gui/PluginPinConnectorView.cpp Show resolved Hide resolved
@@ -193,7 +193,7 @@ class LMMS_EXPORT PluginPinConnector
void loadSettings(const QDomElement& elem) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments from messmerd#2 should be solved (unrelated to this code line here).

plugins/VstEffect/VstEffectControls.cpp Outdated Show resolved Hide resolved
include/PluginPinConnector.h Outdated Show resolved Hide resolved
include/AudioData.h Show resolved Hide resolved
#define LMMS_AUDIO_DATA_H

#include <cassert>
#include <type_traits>
Copy link
Contributor

@JohannesLorenz JohannesLorenz Dec 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems to have many useless includes. While many of them might be caused in the past, at least for those files where you touch the includes (or for all files that you touch?), they should be cleaned in this PR.

Checking for includes can be done as follows (I once need to set up a wiki page for this, or even integrate it):

  1. Run flags="include-what-you-use;-Xiwyu;--mapping_file=/usr/share/include-what-you-use/qt5_11.imp;-Xiwyu;--keep=*/xmmintrin.h;-Xiwyu;--keep=*/lmmsconfig.h;-Xiwyu;--keep=*/weak_libjack.h;-Xiwyu;--keep=*/sys/*;-Xiwyu;--keep=*/debug.h;-Xiwyu;--keep=*/SDL/*;-Xiwyu;--keep=*/alsa/*;-Xiwyu;--keep=*/FL/x.h;-Xiwyu;--keep=*/MidiApple.h;-Xiwyu;--keep=*/MidiWinMM.h;-Xiwyu;--keep=*/AudioSoundIo.h" (assuming you use Qt5, you are on Linux and the mapping file is correct)
  2. Run cmake -DCMAKE_C_INCLUDE_WHAT_YOU_USE="$flags" -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE="$flags" ..
  3. Run make >make.txt 2>&1 (and wait a whole while, this can take half an hour)
  4. Copy the following script into your build dir and run it:
#!/bin/bash

cat make.txt |
awk '/should remove these lines:$/ {print; flag=1; next} /^The full include-list for/ {flag=0} flag && NF' | # print all from "should remove these lines" until "The full include list"
awk '/ should / {if(found) { found=0 } should=$LINE} /- .+/ {if(found==0) print should; found=1; print}' | # print only if at least one nonempty removal suggestion is found |
awk '/ should / {ignore=0} /globals.h should/ {ignore=1} (!ignore)' # ignore globals.h - this is just an example for zyn, you can use this for LMMS with other files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note about this: I plan to do a complete #include-cleanup on master. You can still do it hear, but you can also merge without this check to master and I will fix it there, anyways.

include/PluginPinConnector.h Show resolved Hide resolved
- AudioPluginInterface --> AudioPlugin
- detail::AudioProcessorImpl --> detail::AudioPlugin
- DefaultEffectPluginInterface --> DefaultEffect
- DefaultMidiInstrumentPluginInterface -> DefaultMidiInstrument
In order to support multiple inputs/outputs correctly, all track
channels routed to a single plugin input need to be summed together
rather than averaged, and likewise for all plugin outputs routed to a
single track channel.

This change in behavior is accompanied by a change to the default
connections for mono plugins so that it does not double the audio
amplitude. Only the left track (rather than both) are connected to the
plugin input in this case, matching what REAPER does.

I also updated the unit tests and added a new test specifically to test
the new "summing" behavior.
@JohannesLorenz
Copy link
Contributor

One note about the class renaming: AudioPlugin still seems a bit confusing, it's more like AudioPluginWithPinConnector. Though, the last name is really long, and we might just say "For us, an audio plugin is a plugin with a pin connector" even though a pin connector is not strictly required for audio plugins.

However, given that you consider inheritance changes, this whole discussion might already be deprecated...

* Add PluginAudioPort class (WIP)

Refactors how the pin connector and audio buffers are incorporated into
AudioPlugin, how buffer updates occur, and how buffer customization
works.

Also uses more class type NTTP to simplify more of the template
parameters.

* Add ConfigurableAudioPort; Fix build

* Some fixes (WIP)

* Update (WIP)

* Replace AudioDataLayout with bool

* Remove SampleType

Reasons:
- It's clunky to use
- It bloats the PR
- Without a strong typedef it does nothing that can't be accomplished
with a comment

* Update tests

* Fix ZynAddSubFX when switching back to local

* Minor cleanup
- Add pin connector action to `TrackOperationsWidget`'s menu. The button
is disabled for instruments that don't support the pin connector.
- Revert all changes to `ManageVestigeInstrumentView`
- Minor changes to how the pin connector view is instantiated and closed
@messmerd messmerd changed the title Add Plugin Pin Connector Add support for multi-channel plugins Jan 31, 2025
Conflicts:
- src/core/RemotePlugin.cpp
Also removed lmms::Span and made some small improvements to the
RemotePlugin audio port
@sakertooth
Copy link
Contributor

sakertooth commented Feb 5, 2025

From first glance, it looks like we need a "what's this?" for the Pin connector action in the context menu. This is just my opinion, but I think users may just see a matrix and not understand what is going on at all without additional explanation in the DAW or somewhere else. It's not as clear as the other actions that need no explanation, like "Remove this track" or something.

@sakertooth
Copy link
Contributor

If instead you think the commit message is fine then I wouldn't bother.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just slowly trying to pick it apart.

include/SampleFrame.h Outdated Show resolved Hide resolved
include/SampleFrame.h Outdated Show resolved Hide resolved
@JohannesLorenz
Copy link
Contributor

Info: Reviewed until here (b2b8b3b). No further comments.

If all comments are solved, I will recommend intensive testing and will try to rebase #7387 on this as a verification measure.

@JohannesLorenz
Copy link
Contributor

@sakertooth Do you mind checking this comment about renaming of the classes and giving your opinion? Thanks on advance.

Allows the removal of ZynConfig, VstEffectConfig, and VestigeConfig
constants
Also move `AudioDataType` specialization for `SampleFrame` to
AudioData.h.
Fixed new build error in SampleThumbnail.cpp due to a change this PR
made to `SampleFrame`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mono VST plugins output only to the left channel Support audio processing with more than 2 channels
3 participants