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

Implement Cocoa event loop in Scsynth #4499

Open
wants to merge 7 commits into
base: develop
from

Conversation

@Spacechild1
Copy link
Contributor

commented Jul 23, 2019

Purpose and Motivation

Add the possibility to run a Cocoa event loop on Scsynth's main thread so we can show native GUI windows on the Server. 1 Currently, the main thread just waits for a semaphore, so it might just as well handle GUI events instead. This is very useful for projects like https://git.iem.at/pd/vstplugin. Note that we only need this for macOS because the event loop must run on the main thread (which I don't control). On Windows and Linux, the event loop can run on any thread and I can therefore create my own event loop.*)

Types of changes

  • cross platform EventLoop class (does nothing on Windows and Linux so far)
  • Cocoa event loop functions implemented in SC_Apple.hpp/.mm
  • called in scsynth_main.cpp (I didn't need to touch any other files!)
  • "-e" command line option to enable the event loop (disabled by default)
  • "eventLoop" member in ServerOption class (default: false)

The idea is that plugin clients can simply execute methods on the main thread with dispatch_sync/dispatch_async, e.g. to create/destroy windows https://developer.apple.com/documentation/dispatch/1453057-dispatch_async?language=objc

The reason for the "-e" argument is that with the Cocoa event loop, Scsynth will show up as an app in the taskbar and people might find this annoying. Probably "-e" should have an argument (0/1), so we can later change the default value...

EDIT: I've removed the "-e" argument because I've decided to always run the Cocoa event loop and not add an icon to the taskbar (I rather do this in VSTPlugin after the fact).

Disclaimer: I'm not a Cocoa expert my any means!

Testing

I've tested this with the latest develop branch of VSTPlugin (https://git.iem.at/pd/vstplugin/tree/develop) and the VST plugin GUI works fine.

Here's a pre-compiled binary for macOS: VSTPlugin.zip

And here's a test .scd file: VSTPlugin_editor_cocoa.scd.zip

Notes

The addition of the event loop shouldn't be noticable by the user. Also, there shouldn't be any impact on performance because the event loop would just sit idle (like the main thread already does), but this has to be tested!

I would like to see this included in the next SuperCollider release as a "private" feature which can be tested in the wild with the next version of VSTPlugin. If this turns out to work well, we can think about adding a Windows and Linux implementation and exposing the event loop via the plugin API.
(Windows and Linux don't have the concept of a "main thread", i.e. there is no equivalent to Cocoa's dispatch_get_main_queue, so at the very least we need method to return a handle to the GUI thread. We could also add methods for passing messages between GUI thread and RT thread, similar to SendMsgToEngine/SendMsgFromEngine).

Finally, note that this PR is not specifically about VSTPlugin! It allows any plugin to create native windows for all kinds of things: video playback, visualization, complex GUIs, etc.

  • Code is tested
  • This PR is ready for review

Christof Ressi added some commits Jul 23, 2019

Christof Ressi Christof Ressi
implement Cocoa event loop and run it in scsynth_main.cpp to show nat…
…ive GUI windows

* added "-e" command line option to enable the event loop (disabled by default)
* added "eventLoop" member to sclangs's ServerOption class
* didn't have to touch WorldOptions!
@scztt

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

I think there's definitely some more review and analysis needed for this PR, but at a glance I think it's doing roughly the correct thing. My immediate concerns for this would be:

  • Does making scsynth an NSApplication / have an event loop have any important effects? Performance / threading concerns? Priority? Does it have an icon in the dock? Will it process events that we don't want it to receive?

  • Will we be able to share most of this implementation with VST3? VST2 is very deprecated (IIRC Steinberg is pulling the SDK and not allowing licenses, if not now then any day) - we should at least make sure that we e.g. won't have to add ANOTHER new flag and set of different event loop hacks to support VST3 in the future.

  • Testing VST GUI's is laborious - what is our plan to try to get good test coverage before we merge this? This may be a simple change now, but it could get much more complex as we work around real-world plugin behavior. It would be nice to know that we had a good range of VST's smoke tested before we merged this and users start to expect it to work.

  • It's reasonable to have separate PR's for separate platforms, but it would be good to have at least a WIP for windows VST GUI before this was merged, lest we get stuck with a one-platform implementation (unless I'm totally wrong and Windows GUI is already working? If so, that would be wonderful - I haven't tested it!)

I can give a closer code review on this soon, and happy to help brainstorm / discuss the issues I mentioned above.

@scztt

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

One other code comment: I would somewhat prefer a solution where the #ifdef __APPLE__'s were removed and instead we had a mostly macro-free cpp file, with platform-specific includes and a no-op case for unsupported platforms. E.g.:

        case 'e':
            checkNumArgs(1);
            eventLoop = IsApple ? true : false;
            break;
// EventLoop_Mac.h
struct EventLoop {
    static void startRunLoop(function<void()> f) {
      auto thread = SC_Thread([&]() {
            f();
            SC::Apple::EventLoop::quit();
        });
        thread.detach();
        SC::Apple::EventLoop::run();
    }
}

// EventLoop_Win.h
struct EventLoop { 
    static void startRunLoop(function<void()> f) { f(); } // or assert, if we want to ensure this is unreachable
}

// ...
EventLoop::startRunLoop([&](){ World_WaitForQuit(world, true); });

Not sure if others have preferences here? In terms of code readability and maintainability, I find this kind of separation much better, even if it means sometimes you need to jump around to find out what's going on.

This could also be done simply as a run-loop function in run() for each platform. It's super reasonable that each platform would have different details in their run loop.

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Performance / threading concerns? Priority?

I don't think so. The main thread is already there, it just sleeps. Note that a GUI event loop also sleeps unless it receives events.

Does it have an icon in the dock?

Yes, that's actual reason why I made it optional because it might annoy people. I've updated the PR decription.

Will it process events that we don't want it to receive?

The event loop doesn't care about the events, it simply pops them from the queue and dispatches them. Plugins are supposed to create a window and set the window delegate which handles the input events. Furthermore, it can call methods on the window and send events. But the event loop itself doesn't know anything about it.

* Will we be able to share most of this implementation with VST3? VST2

Actually, this PR is not about VST. It simply allows any SuperCollider plugin to use the Cocoa event loop (e.g. to create native Windows). Again, the event loop is agnostic about the implementation of each Window.

* Testing VST GUI's is laborious - what is our plan to try to get good test coverage before we merge this?

Again, this PR is not specifically about VST and the event loop itself is "dumb". Any problems with VSTPlugin should go to the VSTPlugin bug tracker and will be handled by me.

* It's reasonable to have separate PR's for separate platforms, but it would be good to have at least a WIP for windows VST GUI before this was merged, lest we get stuck with a one-platform implementation (unless I'm totally wrong and Windows GUI is already working? If so, that would be wonderful - I haven't tested it!)

Yes, the Windows and Linux GUI is already working! Personally, I don't need the Windows and Linux equivalent of this PR because on those platforms the event loop can be created on any thread - by me! (Actually, there can even be more than one event loop! So Cocoa sucks...)

On the other hand it could be nice to add a Windows and Linux event loop, so that future plugins don't have to create their own event loops. But it would need an API extension because Windows and Linux don't have the concept of a "main thread", i.e. there is no equivalent to dispatch_get_main_queue. This means, plugins need a way to get a handle to the GUI thread. It could be void * member in World or a plugin API function. Anyway, we would have to increment the plugin API version.

I would really love to see the Cocoa event loop merged rather sooner than later. I would consider it an "experimental" and "private" feature because I'll be the only one using it, anyway... After a while of testing it in the real world with VSTPlugin we can think about exposing it via the plugin API as described above.

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

One other code comment: I would somewhat prefer a solution where the #ifdef __APPLE__'s were removed and instead we had a mostly macro-free cpp file, with platform-specific includes and a no-op case for unsupported platforms.

I agree that dummy implementations for Windows and Linux would be better and I think your examples looks quite nice. I'll go ahead and add this but I think we only need a single SC_EventLoop.hpp header with a couple of #ifdefs. We already have files for platform specific code.

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

BTW, I'm still looking for a way to hide the Scsynth icon from the docker, in which case we might not need the "-e" flag because the users wouldn't notice anything. On the other hand, it might be still a good idea to keep the event loop optional. Dunno... You tell me!

@muellmusik

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Actually, this PR is not about VST. It simply allows any SuperCollider plugin to use the Cocoa event loop (e.g. to create native Windows). Again, the event loop is agnostic about the implementation of each Window.

So this could, for instance, be used to support video playback in scsynth?

BTW, I'm still looking for a way to hide the Scsynth icon from the docker, in which case we might not need the "-m" flag because the users wouldn't notice anything. On the other hand, it might be still a good idea to keep the event loop optional. Dunno... You tell me!

If it's a GUI app, it should have a dock icon I think. This is important as it provides a conventional way to front the app etc. I think best not to contradict that. That said, at one point scsynth did have an icon, and some people found that annoying in the absence of a GUI. So the command line might be a good compromise.

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@muellmusik

So this could, for instance, be used to support video playback in scsynth?

totally!

If it's a GUI app, it should have a dock icon I think. This is important as it provides a conventional way to front the app etc. I think best not to contradict that.

I agree! For example, when you click on the icon, all its windows will move to the foreground. IIRC this was the reason why I added the icon in the first place in the original Pd version, I just forgot... (I must remember to document this!)

That said, at one point scsynth did have an icon, and some people found that annoying in the absence of a GUI. So the command line might be a good compromise.

Thanks for this insight! The question is: should the new flag only be about showing the icon (with the event loop running either way)? Or should it be about running the event loop + showing the icon?

I guess the option of not introducing any flag is out of question (because it would imply not showing the icon).

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Another option: VSTPlugin performs the foreground application transformation the first time a GUI window is requested.

Later this can be done automatically in the new API methods (e.g. whenever the GUI thread handle is requested for the first time by a plugin).

Then the only question would be if the event loop should always run or only if requested with the "-e" option... I think there is no performance hit with running an idle event loop but of course I'm not 100% sure.

@muellmusik

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

totally!

Great! That would also be an exciting feature!

Thanks for this insight! The question is: should the new flag only be about showing the icon (with the event loop running either way)? Or should it be about running the event loop + showing the icon?

Well, as I said, IMO whenever it is a GUI app it should show the dock icon.

Another option: VSTPlugin performs the foreground application transformation the first time a GUI window is requested.

Later this can be done automatically in the new API methods (e.g. whenever the GUI thread handle is requested for the first time by a plugin).

So the icon would appear the first time you created a GUI? That could be nice, although perhaps slightly unusual in behaviour. Personally I never minded scsynth having a dock icon, but others found it odd. If in the longer run GUI elements became more common there might be less objection.

Then the only question would be if the event loop should always run or only if requested with the "-e" option... I think there is no performance hit with running an idle event loop but of course I'm not 100% sure.

Well, if there's concern about another command line arg being bloat, and that could avoid it, that's a plus I'd guess. Also nice for users not to have to remember to launch it in GUI mode. I suspect also that the load is insignificant, but would be good to know. A compromise might be to boot it in GUI mode by default, so you only need to change an arg if you don't want the dock icon.

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

So the icon would appear the first time you created a GUI? That could be nice, although perhaps slightly unusual in behaviour.

FWIW, I've just tested this (VSTPlugin performing the transformation after the fact) and it works!

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

So the icon would appear the first time you created a GUI? That could be nice, although perhaps slightly unusual in behaviour.

Another option:

We can add another flag for DefineUnit, e.g.

enum { 
    kUnitDef_CantAliasInputsToOutputs = 1,
    kUnitDef_NeedGui = 2
 };

and scsynth does the transformation the first time it finds a plugin which sets this flag.
Until then, VSTPlugin could do the transformation in its setup function (instead of "on demand").

What is better? I'm not a Mac user, so I don't have an opinion...

@muellmusik

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

What is better? I'm not a Mac user, so I don't have an opinion...

I don't have a strong preference. People will definitely want to run 'GUI-less' in some contexts though, so that should be made possible on some level. (Could be a compile option.)

@telephon

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

People will definitely want to run 'GUI-less' in some contexts though, so that should be made possible on some level. (Could be a compile option.)

I don't think it should be a compile option, but a server option. E.g. when you do network music and you start 5 servers you don't want 5 icons popping up, do you? And you don't want to use a different program for that I think.

Christof Ressi added some commits Jul 25, 2019

Christof Ressi Christof Ressi
Remove "-e" argument and always run Cocoa event loop
We don't transform the process into a foreground application on the Server!
Christof Ressi Christof Ressi
@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

OK, I've decided to remove the "-e" flag and always run the Cocao event loop. This way the feature becomes 100% private. Mac users are invited to run their existing projects with this patch and observe any differences in performance. I think there should be no impact at all because the event loop just waits for a condition - like the current main thread already does.

Also, I don't do the foreground application transformation on the Server, I rather do it in VSTPlugin the first time a GUI editor is requested. This means that scsynth only gets an icon in the taskbar if there is at least one GUI window.

My suggestion would be to test this for a while with VSTPlugin and if it works out well, I can offer to add implementations for Windows and Linux and extend the plugin API, so the feature can be easily used by other plugins.

I've updated the PR description.

@geoffroymontel

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Hello @Spacechild1 ! Is your PR still WIP or Ready for Review ?

@Spacechild1 Spacechild1 changed the title [WIP] implement Cocoa event loop in Scsynth Implement Cocoa event loop in Scsynth Aug 6, 2019

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Hey, I've removed the [WIP] label, but I guess this PR still needs discussion. Personally, I think it could be merged as an experimental "private" feature which shouldn't affect normal operation.

I've added some testing instructions to the PR description, give it a try!

@geoffroymontel

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Hi !
Exciting feature !
I'm running macOS 10.12.6 with built-in soundcard

I've just tried this branch with the pre-compiled binary for macOS and I can't get the UI to open with Dexed VSTi

Here is a reproducer :

s.boot;
VSTPlugin.search;

returns

searching in '/Users/geoffroy/Library/Audio/Plug-Ins/VST'...
found 0 plugins
searching in '/Library/Audio/Plug-Ins/VST'...
/Library/Audio/Plug-Ins/VST/Dexed.vst
etc.

then

(
SynthDef(\dexed, { |out = 0|
	Out.ar(out, VSTPlugin.ar(nil, 2, id: \dexed));
}).add;
)

~synth = Synth(\dexed);
~dexed = VSTPluginController.new(~synth, \dexed);
~dexed.open("Dexed", editor: true);
~dexed.gui;

I can see the scsynth icon but the generic Qt GUI opens.
Did I miss something ?
Best !

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

~dexed.editor; ;-)

@geoffroymontel

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Ah ah :) sorry ! It's working like a charm ! Thanks a lot Christof for that, I hope it will make more people consider SC as a host !!

@Spacechild1

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Cool! Now I'd need to know if macOS users notice any sort of noticable performance regression when running their existing projects (which don't use VSTPlugin) on this branch. I think there shouldn't be any difference, but I would like to get some feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.