-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: master
Are you sure you want to change the base?
Conversation
While still doing a review, I wonder if the |
There was a problem hiding this 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
@@ -193,7 +193,7 @@ class LMMS_EXPORT PluginPinConnector | |||
void loadSettings(const QDomElement& elem) override; |
There was a problem hiding this comment.
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).
#define LMMS_AUDIO_DATA_H | ||
|
||
#include <cassert> | ||
#include <type_traits> |
There was a problem hiding this comment.
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):
- 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) - Run
cmake -DCMAKE_C_INCLUDE_WHAT_YOU_USE="$flags" -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE="$flags" ..
- Run
make >make.txt 2>&1
(and wait a whole while, this can take half an hour) - 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
There was a problem hiding this comment.
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.
- 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.
One note about the class renaming: 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
Conflicts: - src/core/RemotePlugin.cpp
Also removed lmms::Span and made some small improvements to the RemotePlugin audio port
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. |
If instead you think the commit message is fine then I wouldn't bother. |
There was a problem hiding this 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.
@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`
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:

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
PluginPinConnector
: Pin connector model + audio routing methodsPluginPinConnectorView
: Pin connector windowPluginAudioPort
: ...AudioPluginInterface
: ...AudioPluginBufferInterface
: ...TrackOperationsWidget
's menu for instruments that support itlmms::unreachable
: A stand-in for C++23'sstd::unreachable
SampleFrame*
parameters (which is interleaved data), so I replaced those withfloat*
, which can be any data layoutFuture
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 usingshort
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
lmms::Span
withstd::span
SharedMemory::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 PR