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

Synchronize with remote plugins using local sockets #2739

Merged
merged 1 commit into from Jun 4, 2016

Conversation

@jasp00
Copy link
Member

jasp00 commented Apr 26, 2016

This fixes #2724 and possibly #1875.

@tresf
Copy link
Member

tresf commented Apr 27, 2016

Albeit off-topic, I had to kick-start this job 3 times for Travis to execute it properly.

@lukas-w should we consider travis_retry-ing our commands?

@jasp00 jasp00 force-pushed the jasp00:sync-socket branch from b4cb735 to 5a74b7f Apr 27, 2016
@jasp00
Copy link
Member Author

jasp00 commented Apr 27, 2016

The Apple builds are fixed.

@tresf
Copy link
Member

tresf commented May 1, 2016

@tobydox do you have time to review this?

@Narfinger, FYI, this will throw #2390 into even further conflict once merged.

@Umcaruje
Copy link
Member

Umcaruje commented May 1, 2016

Hmm, I just compiled this branch, and now when I load up a VST using vestige (Synth1) I get this outputted in the terminal:

Unexpected poll error.
Error while writing.
QProcess: Destroyed while process is still running.

I'm on elementary OS Freya 64-bit. LMMS info:

Version 1.1.90-g5a74b7f (Linux/x86_64, Qt 4.8.6, GCC 4.8.4)
@jasp00
Copy link
Member Author

jasp00 commented May 1, 2016

This error is expected on 64-bit: you need the 32-bit RemoteVstPlugin (and 32-bit dependencies). This comes from a 32-bit build; your distro would provide an i386 package to an amd64 system, see #2735. Theoretically, RemoteVstPlugin can be compiled on amd64 with 32-bit Wine, but I have not tried that method.

@jasp00
Copy link
Member Author

jasp00 commented May 1, 2016

Theoretically, RemoteVstPlugin can be compiled on amd64 with 32-bit Wine, but I have not tried that method.

You may be unable to try this. In Debian, 32-bit Wine tools depend on 32-bit GCC, which conflict with 64-bit GCC.

@Narfinger
Copy link

Narfinger commented May 2, 2016

It still might be good to merge this if this fixes the archlinux bug. And after some testing the QT and sem_t code can be removed to cleanup the #ifdef mess.

Am I understanding this correctly that the current source code cannot be compiled on 64bit to get a working lmms+vestige? Does this first need to be merged with #2735?

@liushuyu
Copy link
Member

liushuyu commented May 2, 2016

It still might be good to merge this if this fixes the archlinux bug.

I and many of my friends can help test this out. I (we) will test this very
soon.

@tresf
Copy link
Member

tresf commented May 2, 2016

Does this first need to be merged with #2735?

From my understanding, #2735 is to allow linking against an already existing RemoteVstPlugin, such as in the event that it was provided by another package. If this is the case, it means compilation for LMMS could go without the Wine dependencies and instead just link against the library.

To my understanding, #2735 is to help Debian packaging and is quite benign (adds a new optional cmake parameter).

Am I understanding this correctly that the current source code cannot be compiled on 64bit to get a working lmms+vestige?

Seems to be slightly correct statement, more of a 32 vs. 64 thing and I don't like reading comments like this...

You may be unable to [compile RemoteVstPlugin with 32-bit wine]. In Debian, 32-bit Wine tools depend on 32-bit GCC, which conflict with 64-bit GCC.

I think #2724, needs a better explanation in regards to what situation we've gotten into with Wine. It appears @jasp00 has a pretty good grasp on why things have gone awry but I'm still missing the big picture (so I can only assume others are too). This is why I've asked @tobydox to review. He knows the VST stuff better than anyone. 👍

Here's a copy of the description from #2724:

I am having locking problems with VST on amd64. It looks like sem_t cannot be shared between 32-bit and 64-bit processes.
https://sourceware.org/bugzilla/show_bug.cgi?id=17980

Inter-process communication should use new sem32_* functions, pipes or sockets. You may consider endianness too (VST on powerpc). Do you want me to code this feature?

@jasp00
Copy link
Member Author

jasp00 commented May 2, 2016

Am I understanding this correctly that the current source code cannot be compiled on 64bit to get a working lmms+vestige?

You can (and it works). In Debian, you need some trickery, see https://bugs.debian.org/823264 .
Developing with Wine in Debian has some issues (you have to set PATH and RUNPATH); it is not very friendly, although LMMS is the only project that links with Wine.

@Narfinger
Copy link

Narfinger commented May 3, 2016

I don't quite understand. I am on archlinux and have multilib stuff installed but the standard compile does not work. It gives the same error as in @Umcaruje comment.

@jasp00
Copy link
Member Author

jasp00 commented May 3, 2016

Do you have RemoteVstPlugin in your installation folder, such as /usr/lib/lmms?

@jasp00 jasp00 force-pushed the jasp00:sync-socket branch from 5a74b7f to eaa082f May 20, 2016
@jasp00
Copy link
Member Author

jasp00 commented May 20, 2016

I have added a test to detect the missing RemoteVstPlugin. The console output should be more helpful. @Umcaruje or @Narfinger, could you confirm this was the error?

@liushuyu
Copy link
Member

liushuyu commented May 21, 2016

I tested this patch and found that this could surely fix #1875 ! 👍 😄

My configuration when testing:
Arch Linux (Base version 229, update 2016-05-20)
Wine 1.9.9-staging

screenshot_20160521_121329

@Umcaruje
Copy link
Member

Umcaruje commented May 21, 2016

@jasp00 I can't build the branch using ninja on elementary OS Freya x64:

[480/481] Linking CXX shared module plugins/libzynaddsubfx.so
FAILED: : && /usr/bin/c++  -fPIC -fno-exceptions -Wall -Werror=unused-function -Wno-sign-compare -Wno-strict-overflow -Wno-array-bounds  -fPIC -DPIC -g -DLMMS_DEBUG   -shared -Wl,-soname,libzynaddsubfx.so -o plugins/libzynaddsubfx.so plugins/zynaddsubfx/CMakeFiles/zynaddsubfx.dir/ZynAddSubFx.cpp.o plugins/zynaddsubfx/CMakeFiles/zynaddsubfx.dir/moc_ZynAddSubFx.cxx.o  -Wl,--enable-new-dtags -L.. -lZynAddSubFxCore -Wl,-rpath,"\$ORIGIN" && :
/usr/bin/ld: cannot find -lZynAddSubFxCore
collect2: error: ld returned 1 exit status

This is probably not relevant to this patch, but to your cmake patches. I'll disable zyn and build without it, to test this out.

@Umcaruje
Copy link
Member

Umcaruje commented May 21, 2016

Ok, wow, great job @jasp00! I managed to load several VST's into LMMS, and all worked like a charm, without any errors. I even loaded xfer Serum in, and it worked, but the GUI was all dark (that's a different issue though). So I tried Synth1, T-Force Alpha Plus, DVS Saxophone and some other VST's and all worked nicely.

@Umcaruje
Copy link
Member

Umcaruje commented May 21, 2016

Ok, I found a weird issue, when I enable VST sync to host playback in my settings, I can't load any VST plugins, and I get

Error while reading.
Remote plugin did not connect.
Error while writing.
QProcess: Destroyed while process is still running.

Outputted in my terminal.

When I disable it, this is the output:

VST sync support disabled in your configuration
RemotePluginClient::shmget: No such file or directory
RemoteVstPlugin.cpp::shmget: No such file or directory
RemoteVstPlugin.cpp: Failed to initialize shared memory for VST synchronization.
 (VST-host synchronization will be disabled)
unique ID: S1Vs
RemotePlugin::DebugMessage: inputs: 0  output: 2
RemotePlugin::DebugMessage: creating editor
RemotePlugin::DebugMessage: editor successfully created
RemotePlugin::DebugMessage: failed getting shared memory
@jasp00 jasp00 force-pushed the jasp00:sync-socket branch from eaa082f to ae8b827 May 22, 2016
@jasp00
Copy link
Member Author

jasp00 commented May 22, 2016

I can't build the branch using ninja on elementary OS Freya x64

@Umcaruje, this looks like a bug in CMake 2.8. Should I work around this bug?

@jasp00
Copy link
Member Author

jasp00 commented May 22, 2016

I found a weird issue, when I enable VST sync to host playback in my settings, I can't load any VST plugins

Fixed.

@liushuyu
Copy link
Member

liushuyu commented May 22, 2016

I tested this again and found this worked for me (I am using Arch Linux which means this even fixed #1875 ) . Should we merge this ? @tresf @Umcaruje

@Umcaruje
Copy link
Member

Umcaruje commented May 22, 2016

@Umcaruje, this looks like a bug in CMake 2.8. Should I work around this bug?

Well, I just upgraded my cmake to 3.2 and ninja to the latest version and I still get the error:

[968/1012] Linking CXX shared module plugins/libzynaddsubfx.so
FAILED: plugins/libzynaddsubfx.so 
: && /usr/bin/c++  -fPIC -fno-exceptions -Wall -Werror=unused-function -Wno-sign-compare -Wno-strict-overflow -Wno-array-bounds  -fPIC -DPIC -O2 -Wno-write-strings -Wno-deprecated-declarations -fpermissive -O3 -DNDEBUG   -shared -Wl,-soname,libzynaddsubfx.so -o plugins/libzynaddsubfx.so plugins/zynaddsubfx/CMakeFiles/zynaddsubfx.dir/ZynAddSubFx.cpp.o plugins/zynaddsubfx/CMakeFiles/zynaddsubfx.dir/moc_ZynAddSubFx.cxx.o  -Wl,--enable-new-dtags -L.. -lZynAddSubFxCore -Wl,-rpath,"\$ORIGIN" && :
/usr/bin/ld: cannot find -lZynAddSubFxCore
collect2: error: ld returned 1 exit status
@Umcaruje
Copy link
Member

Umcaruje commented May 22, 2016

Ok VST's load now with sync on. I got a crash when closing the window of T-Force Alpha Plus but I'll try to find steps to reproduce and post a backtrace tommorow.

@jasp00 jasp00 mentioned this pull request May 23, 2016
@liushuyu
Copy link
Member

liushuyu commented May 23, 2016

I got a crash when closing the window of T-Force Alpha Plus but I'll try to find steps to reproduce and post a backtrace tommorow.

Did you get a SIGPIPE? Some of my VST plugins also crashed and gave me SIGPIPE or SIGSEGV

@jasp00 jasp00 force-pushed the jasp00:sync-socket branch 2 times, most recently from aca6040 to 573bfdf May 28, 2016
@jasp00
Copy link
Member Author

jasp00 commented May 28, 2016

I have fixed some concurrency issues with the last commit.

@liushuyu
Copy link
Member

liushuyu commented May 28, 2016

I have tested the new commit. It seemed that the issues were eliminated. But I am not sure if it will appear again later.

@liushuyu
Copy link
Member

liushuyu commented May 29, 2016

@Umcaruje So how is your result? Maybe we could merge it now?

@Umcaruje
Copy link
Member

Umcaruje commented May 29, 2016

Ok, so I have a crash.
Edit: removed bug description, moved away to #2802

Another thing I noticed is that the parameter window also has a fixed width, which ends up having a lot of blank space when dealing with VST's with few parameters.

@jasp00
Copy link
Member Author

jasp00 commented May 29, 2016

Yes, there is a crash and a lot of blank space in the window. There is another crash when removing the track while still playing. But these are issues independent from this pull request.

@Umcaruje
Copy link
Member

Umcaruje commented May 29, 2016

Yes, there is a crash and a lot of blank space in the window. There is another crash when removing the track while still playing. But these are issues independent from this pull request.

Ok, I'll open up new issues for those, as they are out of scope 👍

I tested again, and can conclude that VST's work stable and like a charm, this is good to be merged 🎉

@tresf
Copy link
Member

tresf commented May 29, 2016

[...] fixed width [...]

Yes. Somewhat related/FYI #1184 (comment)

@jasp00 jasp00 force-pushed the jasp00:sync-socket branch from 573bfdf to fa7d7a1 May 29, 2016
@jasp00
Copy link
Member Author

jasp00 commented May 30, 2016

Fixed loading on Windows.

@jasp00 jasp00 merged commit 85d1b20 into LMMS:master Jun 4, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jasp00 jasp00 deleted the jasp00:sync-socket branch Jun 4, 2016
@jasp00 jasp00 mentioned this pull request Jun 4, 2016
@falkTX
Copy link
Contributor

falkTX commented Aug 9, 2016

can someone do a rebase of this patch against the stable branch please?

I'm not sure how long the next version of lmms is going to take, so I'd like to backport the fix into the current stable release.

@jasp00
Copy link
Member Author

jasp00 commented Aug 12, 2016

This is not exactly a backport. This is the Debian patch against version 1.1.3; it was developed before this pull request.

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.