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

Native linux VST support #6048

Merged
merged 24 commits into from
Mar 2, 2022
Merged

Native linux VST support #6048

merged 24 commits into from
Mar 2, 2022

Conversation

akimaze
Copy link
Contributor

@akimaze akimaze commented Jun 9, 2021

Hello,
With this changes finally we can use Linux VST(2) directly from Vestige. And yes ZynAddSubFX 3.0 works great also :)

Fixes #416

I know LMMS is in reorganization but that was just experiment to answer question "Can I do that?" And goes so good so I made PR ;) I know there is Carla Rack but I think a lot of users will like that feature (In Carla Rack you need open Carla UI and next plugin UI to change something what is a little inefficient (IMO)).

Limitations

  • only instrument support, but code for effects can be easily done (but in other PR)
  • currently only no embed plugin window support

How to use

  • set VST plugin embedding to "No embeding"
  • in vestige open window change file type to "SO-files" .so
  • select plugin .so file

open plugin

What works?

All needed features like show plugin UI, save/load presets (and preview in lmms browser), automation.
Some mono synth like Dexed works only in left channel but that is out of scope of this PR.

I tested a lot of synths (most of them from kxstudio repos (usr/lib/vst folder after installation) and some commercial) and works OK:

  • Vital
  • ZynAddSubFX
  • Surge
  • Helm
  • Tal Noise Maker
  • Obxd
  • Biotek 2
  • CollaB3
  • Collective
  • Carla Rack

Synths with issues:

  • Dexed - has only one output (mono)

The code

All native linux related code is in NATIVE_LINUX_VST. I'm a busy man so feel free to fix small issues, fork this code or do whatever is needed to merge/make better solution. I didn't noticed any thread issues but maybe it should be done better ;)

Some inspirations how to do X11 code from: https://github.com/ekenberg/vstminihost

Screenshots

synths_lmms_native_vst

synths2

@LmmsBot
Copy link

LmmsBot commented Jun 9, 2021

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://15794-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.195%2Bgf8b318d8c-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15794?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://15796-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.195%2Bgf8b318d8c-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15796?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/pge5hxcnijr1b8kj/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42717565"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/f7q3hqcrie7vvesg/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42717565"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://15795-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.195%2Bgf8b318d8c-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15795?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://15793-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.195%2Bgf8b318d8c-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15793?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "e71c737a829b8e3ff7cf939df24d7ae31fbd2c71"}

@MaciejMalczyk
Copy link
Contributor

MaciejMalczyk commented Jun 10, 2021

Does Carla Patchbay work with this? And how automation works?

@PhysSong
Copy link
Member

I'm still reading the code, but I think:

  • The #ifdefs are making the code hard to read. If we decide to support more platforms(such as macOS), that might become even worse.
    Ideally, platform-dependent code should be decoupled from the core code(and probably IPC stuff as well, but it's not relevant here).
  • For native VSTs, Qt is also available(and it's a cross platform framework). You may find relevant discussions in Add native VST support for Mac OS X (WIP) #3975.

@akimaze
Copy link
Contributor Author

akimaze commented Jun 11, 2021

Does Carla Patchbay work with this? And how automation works?

I never used Carla Patchbay so I can't test it properly but it loads. Below first message you can download app image and test it :)

@akimaze
Copy link
Contributor Author

akimaze commented Jun 11, 2021

Sorry for delay,

  • The #ifdefs are making the code hard to read. If we decide to support more platforms(such as macOS), that might become even worse.
    Ideally, platform-dependent code should be decoupled from the core code(and probably IPC stuff as well, but it's not relevant here).
  • For native VSTs, Qt is also available(and it's a cross platform framework). You may find relevant discussions in Add native VST support for Mac OS X (WIP) #3975.

I can agree with you. However, at this point I am not able to do such deep RemotePlugin refactoring. Designing this API is rather a task for someone who knows LMMS code better (and I don't think it has to be done in the first implementation). Maybe when someone implement MacOS we will know better how the API should look like. And then we just can refactor the code.

Doing code refactor at this point can also introduce errors in windows VST handling. And a good testing can turn this PR (and adding other platform support) into a really big task that will last for years. Therefore, one of my assumptions was to not change the Windows code as much as possible. And you can check that nothing changes - only one calling from class to object.

Another advantage is that you can easily compare implementations for different platforms (just scroll up/below). And the skeleton remains common.

include/aeffectx.h Outdated Show resolved Hide resolved
plugins/vst_base/VstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

Here's my review of this. I don't have any experience with X11, so I'm assuming that those bits of code are correct (or at least tested and working). I've got some things I'd like to work on regarding VST support, so it would be great if we could get this merged soonish.

include/RemotePlugin.h Outdated Show resolved Hide resolved
plugins/vst_base/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/vst_base/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/vst_base/NativeLinuxRemoteVstPlugin64.cmake Outdated Show resolved Hide resolved
plugins/vst_base/NativeLinuxRemoteVstPlugin64.cmake Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/vst_base/VstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/vstbase/CMakeLists.txt Outdated Show resolved Hide resolved
@DomClark
Copy link
Member

DomClark commented Oct 1, 2021

@akimaze Are you able to work on this soon, or if not, do you mind if we finish it off?

@akimaze
Copy link
Contributor Author

akimaze commented Oct 4, 2021

@akimaze Are you able to work on this soon, or if not, do you mind if we finish it off?

Sorry, I haven't free time currently to do that, I'll be glad if you finish this PR :)

@akimaze
Copy link
Contributor Author

akimaze commented Oct 4, 2021

Here's my review of this. I don't have any experience with X11, so I'm assuming that those bits of code are correct (or at least tested and working). I've got some things I'd like to work on regarding VST support, so it would be great if we could get this merged soonish.

@DomClark I'm not X11 programmer also. X11 code is inspired by https://github.com/ekenberg/vstminihost . I wrote it after reading this sources (license "use it as you wish").
As @PhysSong mentioned it can be replaced by Qt code.

@guglielmofratticioli
Copy link

nice one !
when I compiled myself on Debian make gave the error in /src/core/MemoryManager.cpp line: 53

MemoryManager::ThreadGuard::~ThreadGuard()
{
   if (--thread_guard_depth == 0) {
   	rpmalloc_thread_finalize();
   }
}

not enaught input arguments for function rmpalloc_thread_finalize();

in the main lmms repo that is called with 'true' arg -> rpmalloc_thread_finalize(true);

Copy link
Contributor

@Veratil Veratil left a comment

Choose a reason for hiding this comment

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

Along with these comments I'd say following the style conventions (such as no spaces before/after the parens in things like if (...)) unless other devs are okay with ignoring them in favor of auto styling later.

plugins/vst_base/NativeLinuxRemoteVstPlugin64.cmake Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/RemoteVstPlugin.cpp Outdated Show resolved Hide resolved
plugins/vst_base/VstPlugin.cpp Show resolved Hide resolved
@PhysSong PhysSong self-requested a review February 26, 2022 03:09
@PhysSong
Copy link
Member

I can't find any critical issues with this implementation. It looks like there is room for improvements and cleanups, though.
Note: I'm not familiar with X11 stuff, too.

@allejok96
Copy link
Contributor

Compiled fine, seems to work fine, only complaint I have as a user is that I must select *.so instead of *.dll in the browser every time, which I probably wouldn't notice if I wasn't told to look for it and so I'd just assume that only dll files can be loaded.

@DomClark DomClark merged commit a08e7f9 into LMMS:master Mar 2, 2022
@probonopd
Copy link

Some mono synth like Dexed works only in left channel but that is out of scope of this PR.

Is there be a follow-up ticket regarding this issue?

@DomClark
Copy link
Member

DomClark commented Mar 3, 2022

Only complaint I have as a user is that I must select *.so instead of *.dll in the browser every time

I will address this in a future PR (also removing *.exe from that list because it makes no sense).

Some mono synth like Dexed works only in left channel but that is out of scope of this PR.

Is there be a follow-up ticket regarding this issue?

I would hope a solution to #5108 would include better handling of instruments with a single output channel, but we can also open an issue specifically for those instruments. In the meantime, it's easy to work around with a stereo matrix effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LinuxVST support
9 participants