Semaphore shm split patch

[Narfinger]

Splitted the semaphore and SharedMemory from Remoteplugin.h into separate wrapper header files so RemotePlugin.h does not have that many #ifdefs.

[tresf]

Thanks. You'll want to re-base against master so that @M374LX's Mixer cleanup doesn't show up in the commit.

To put this PR in context, this is inspired by #1875 and supersedes #2304 (closed without merging).

The intention is to make the code more readable and (AFAIK) allow the easy enabling of QSharedMemory on Linux (Linux would normally use shm exclusively, but this has been causing some issues on Arch Linux as well as various others).

[tresf]

@Narfinger it looks like Linux has an unused variable per Travis:

In file included from /home/travis/build/LMMS/lmms/include/RemotePlugin.h:88:0,
                 from /home/travis/build/LMMS/lmms/src/core/RemotePlugin.cpp:31:
/home/travis/build/LMMS/lmms/include/SemaphoreWrapper.h: In member function ‘void SemaphoreWrapper::release()’:
/home/travis/build/LMMS/lmms/include/SemaphoreWrapper.h:83:16: error: unused variable ‘err’ [-Werror=unused-variable]
cc1plus: all warnings being treated as errors
make[3]: *** [src/CMakeFiles/lmmsobjs.dir/core/RemotePlugin.cpp.o] Error 1
make[2]: *** [src/CMakeFiles/lmmsobjs.dir/all] Error 2
make[1]: *** [tests/CMakeFiles/tests.dir/rule] Error 2
make: *** [tests] Error 2

[Narfinger]

Yes I noticed it too. Let's see if I got everything or if Travis still has something it doesn't like.

[tresf]

Minor, but I noticed some of our abreviated classnames use all-uppercase and some dont...

For example: AudioSdl.h vs. BBEditor.h (which will eventually be PatternEditor.h)

@lukas-w should the SHMWrapper be ShmWrapper to be consistent? I hope I'm not nitpicking. :)

[Narfinger]

I can change that if you want. Just tell me.

[lukas-w]

should the SHMWrapper be ShmWrapper to be consistent?

I can't see any consistency in our existing code, so either way would be fine. Though ShmWrapper is mostly preferred and also a bit better arguably.

[Narfinger]

I renamed the files, so if travis passes this is ready to be merged.

[tresf]

@Narfinger thanks, this makes the code much easier to read.

From a functionality perspective, we'll need some testing on the Linux and Windows side prior to merging this.

Before we merge it, we should squash down the commits as well.

[Narfinger]

I just merged the commits. Sadly I cannot test this because I don't have windows and I am effected by the archlinux VST Bug.

[tresf]

Sadly I cannot test this because I don't have windows and I am effected by the archlinux VST Bug.

Wine can be used for testing too. The Windows installers are actually built on Linux currently, but someone else can chime in with test results if you prefer.

Since we didn't touch the CMakeLists.txt at all, will this fix the Arch VST issue, or is this just a preliminary code cleanup step?

P.S. Sorry, accidentally clicked close on mobile.

[tresf]

@Narfinger,

@Umcaruje was asking if he could use this patch on his system to fix a similar VST crash but we're not sure how to enable it. Please let us know so we can decide what's needed prior to merge. 👍

[Narfinger]

I am slightly confused. This branch just splits the two things into managable chunks. I did not yet implement the switching to qt part. In essence this is just using defining the #define USE_QT_SHMEM and #define USE_QT_SEMAPHORES additionally, you need to link and include the correct Qt parts in plugins/vst_base/CMakeList.txt

[tresf]

This branch just splits the two things into managable chunks. I did not yet implement the switching to qt part.

Ok that clears it up. Once we have good test results, we can merge. Thanks for clarification.

[tresf]

32-bt VSTs on Ubuntu 12.04 x64 load but LMMS stops responding afterwards... This does not occur on the master branch, which I have compiled and running side-by-side.

[Narfinger]

Can you test it for ab64c67 (which is the commit before my change). I really cannot imagine that this changed any functionality. As I mentioned earlier, I sadly cannot help to debug this feature as I only have access to an archlinux system.

[tresf]

Can you test it for ab64c67 (which is the commit before my change)

The master I tested was pulled today... I'm not sure what you are asking.

As I mentioned earlier, I sadly cannot help to debug this feature as I only have access to an archlinux system.

If I can get an Arch VM up and running to confirm the Arch bug, surely you can do so with an Ubuntu 12.04 box to confirm this one.

[tresf]

Can you test it for ab64c67 (which is the commit before my change)

Here is a test of your exact request:

At ab64c67, test passes:

At 5300d78, test fails (mouse input freezes, LMMS can only be closed with ALT+F4, Tab, Space):

Edit: Edited screenshots to remove personal information. :)

[Narfinger]

Ok I will try to see what is going on.
Btw. I tested loading of plugins on Ubuntu 14.04 where it seems to work.

[Umcaruje]

Since #2739 was merged, #1875 has been reported as fixed. Is there still a need for this PR?

[tresf]

Is there still a need for this PR?

Well, according to the issue description, yes the software could benefit from a reduce source complexity, but we can reopen at a later time. 👍

Splitted the semaphore and SharedMemory from Remoteplugin.h into separate wrapper header files so RemotePlugin.h does not have that many #ifdefs.