free-audio / clap

Audio Plugin API
https://cleveraudio.org/
MIT License
1.76k stars 97 forks source link

Add clarification note about threading while GUI is alive #255

Closed russellmcc closed 1 year ago

russellmcc commented 1 year ago

Fixes #195.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

abique commented 1 year ago

On Windows and Mac, it has to always be on the OS main thread, not only when the GUI is shown, because if you are using an OS timer you want it to tick on the main thread, even if the GUI isn't shown.

russellmcc commented 1 year ago

I just modified it to make it if the GUI is ever shown, then all calls must happen on the main thread. But maybe even if the gui is never shown we also need the calls on the main thread? This would seem to make background thread processing hard for hosts and would disagree with @baconpaul's comment about the "without gui" case here: https://github.com/free-audio/clap/issues/195#issuecomment-1295923255

abique commented 1 year ago

I don't know about other hosts, but for clap-host and bitwig studio, we have just one main thread, it is actually the one in which the program's int main(int argc, char **argv); was called, and it never changes.

I'd like to read what other thinks, but I believe that we should be even stricter:

@baconpaul @tim-janik @robbert-vdh @Bremmers

robbert-vdh commented 1 year ago

I don't see how this clarifications clarifies anything. All functions marked [main-thread] need to be called from the operating system's main thread. So, the function the application's main() function or equivalent (like WinMain) was called on. There are situations where you have some more liberty to do other things, like on Linux with X11 you'd be able to designate another thread as a main thread (but you'd still need to treat it exactly the same as on Windows and macOS). But as a rule of thumb, the main thread should be the main thread.

tim-janik commented 1 year ago

I don't see how this clarifications clarifies anything. All functions marked [main-thread] need to be called from the operating system's main thread. So, the function the application's main() function or equivalent (like WinMain) was called on. There are situations where you have some more liberty to do other things, like on Linux with X11 you'd be able to designate another thread as a main thread (but you'd still need to treat it exactly the same as on Windows and macOS). But as a rule of thumb, the main thread should be the main thread.

Anklang has one [main-thread] (the one int main() was called from) which is also where it loads, activates/deactivates plugins from. If plugins need GUI embedding, Anklang loads a gtk2wrap ELF module and starts another thread dedicated to run a Gtk+2 GUI. It does not however call into the plugin from this thread, but it is indeed possible for the [main-thread] the plugins see and the (OS) GUI thread that runs the UI event processing main loop to be different threads.

Also, keep in mind the use case of a simple script (e.g. python) making use of CLAP plugins. Here, [main-thread] will remain stable but [audio-thread] will always equal [main-thread]. In contrast, on a heavily multi-threaded DAW [audio-thread] might change frequently, e.g. picking an idle thread from a pool.

tim-janik commented 1 year ago

I don't know about other hosts, but for clap-host and bitwig studio, we have just one main thread, it is actually the one in which the program's int main(int argc, char **argv); was called, and it never changes.

I'd like to read what other thinks, but I believe that we should be even stricter:

* the main-thread's os-thread must never change during the whole plugin lifetime

I've tried to think this through, here is why I'm not sure that's a good idea:

* on windows and macos it should be the thread on which the gui and timer events happens

IIRC, the Windows UI thread cannot change, but it doesn't have to be the one where main() got called, right? I have no idea about the MAc though...

russellmcc commented 1 year ago

I think the most important thing is to have a clear set of rules such that race conditions can't happen when a conforming host loads a conforming plug-in.

This is an area where existing plug-in formats take different approaches, so there's not necessarily an "obvious" way to resolve this.

For example, in AU, the host is explicitly allowed to call the equivalent of load on any thread (see this document). This is also the case in at least one other common format. Hosts can use this rule to speed up session loading time by running plug-in state loading concurrently. The downside of this approach is that plug-ins are perhaps harder to write as there would then be 3 "threads" to deal with (the existing two "symbolic" threads, plus the OS gui thread).

In VST3 the opposite choice is made and most calls must happen on the OS main thread on mac and windows. I'm sure I've seen hosts bend these rules a little, but they are explicitly stated.

I'm not sure what approach is right for CLAP, but I do think the project should make the rules explicit so there is no confusion. I'm happy to attempt to write up any consensus decision the project comes up with - is this PR a good place to discuss this?

abique commented 1 year ago

I wonder if gui.h is the right place for describing the thread rules.

abique commented 1 year ago

I think, that any thread specification should be added to thead-check.h and eventually gui.h can refer to thread-check.h.

Makes sense?

russellmcc commented 1 year ago

Makes sense to me! I’ll wait for a decision on what the thread rules should actually be and then modify this PR to put the rules in the proper place.

russellmcc commented 1 year ago

Happy new year! I've modified the rules to try to match @abique's suggestion. I think this is similar to the VST3 specification.

The trade-off as I see it is this: doing things this way makes it tricker for the host to correctly load and host CLAP plug-ins, and it makes loading sessions take longer for the user. But, it makes it easier to write plug-ins. I'm not really qualified to know how best to make this trade-off for the CLAP project.

abique commented 1 year ago

Please excuse me, I now realize that I did not pay enough attention to your original issue.

My biggest concern is that indeed, you can't run the initialization phase in a background thread with that spec. Also if the game industry picks-up on CLAP, they may want to keep the main thread responsive and they possibly want to run this plugin communication in another thread, maybe even have one "main-thread" per-plugin instance? Though, most games have a loading phase and after that, everything happens on the audio thread so it might be OK also with using the process's main-thread.

I've thought a lot about this today and as soon as the plugin starts to use OS events which happens on the process's "main-thread", then it can modify its own state and callback into the host at anytime, out of control and lead to concurrency issues if you decide to load a state in background.

I like the current setting where we avoid the concurrency issues.

One option would be to add a flag to the plugin: "thread-safe state loading and saving"

Then for the other issues: having one main-thread for each plugin instances, I think that the way out would be to introduce a way to flag the plugin as "symbolic main-thread ready". This would imply a set of rules:

What do you think?

abique commented 1 year ago

Maybe the background thread can also apply to plugin activation?

tim-janik commented 1 year ago

gui may only be shown if the main-thread = gui-thread

There can be different event-loops and threads for different GUI implementations in a program, at least on Linux.

We're best off leaving the GUI thread specification out of this, we cannot foresee future GUI/host threading capabilities anyway. I can see why plugin authors may want to rely on the main thread not changing for some plugins (those implementing their own hazard pointers for instance), but for the audio-thread they already have to deal with migrations. To have simple plugins and allow background loading, the following rules would suffice:

abique commented 1 year ago
  • Plugins can set a flag that they handle main-thread migrations well (allows hosts to concurrently load state)

Main thread migration is much harder than allowing to load a state in background. You may have timers, I/O, gui, ... which you'd need to cancel/move. I think only headless plugins will be able to support that today.

tim-janik commented 1 year ago
  • Plugins can set a flag that they handle main-thread migrations well (allows hosts to concurrently load state)

Main thread migration is much harder than allowing to load a state in background. You may have timers, I/O, gui, ... which you'd need to cancel/move. I think only headless plugins will be able to support that today.

Ok, then to clarify the idea here:

1) A plugin should be able to indicate that clap_plugin_state->load may be called from another thread than main-thread? Does the host have to make sure it's not calling the plugin from the main-thread during this time? (I.e implement some kind of mutex to ensure this?) 2) Then, does the same apply to clap_plugin_state->save? 3) Doesn't that essentially add another type to CLAP's threading model, so we should revisit a bunch of APIs wrg to this new thread type? E.g. I presume the plugin may call clap_host->request_callback() during load from a background-thread, but may it call other "main-thread" functions like clap_host_params->rescan() and register_timer() ? 4) Related, should other IO intensive operations be able to run in a background-thread as well, such as scanning plugin preset files from the HDD?

robbert-vdh commented 1 year ago

I can see why plugin authors may want to rely on the main thread not changing for some plugins

Because that could break the entire plugin on Windows or macOS. That seems like a valid enough reason for me. You cannot migrate the main thread on Windows and macOS. If you have created a window and a timer on thread X, then there simply is no way to move those to thread Y. And macOS is even more strict about this than Windows.

russellmcc commented 1 year ago

I think I understand this proposal enough to write up a draft except for a few points of clarification (most are related to the points that @tim-janik brought up in this comment).

To summarize the proposal as I understand it:

Two new interfaces are added: 1) the ability for the plug-in to opt-in to having a non-gui main thread 2) the ability for the plug-in to opt-in to asynchronous loading and saving state

A few new rules are added: 1) The main-thread must never change throughout the lifetime of the plug-in. 2) If the plug-in does not opt-in to non-gui main thread, OR the host plans to open the gui (ever), the main thread must match the OS gui thread on macOS and windows. 3) If the plug-in opts-in, the host may load and save state on a background thread that is NOT the main thread.

The things I still don't fully understand yet: A) Where should the new opt-in interfaces go? B) During async loads and saves, what host calls is the plug-in allowed to call from the load and save? All of the main-thread APIs? C) During async loads and saves, is the host allowed to call into the plug-in on the main-thread while the load and save call are happening? D) During async loads and saves, is the plug-in allowed to call into the host on the main-thread (e.g. from a timer)?

tim-janik commented 1 year ago

I can see why plugin authors may want to rely on the main thread not changing for some plugins

Because that could break the entire plugin on Windows or macOS.

Yes, we are in agreement, I can see that ;-)

tim-janik commented 1 year ago

A) Where should the new opt-in interfaces go?

I'm not sure we have a strong use case for main-thread migration atm. For now it could suffice to document that main-thread == gui-thread on mac & windows out of necessity and that this cannot be relied on on linux/unix.

B) During async loads and saves, what host calls is the plug-in allowed to call from the load and save? All of the main-thread APIs?

We need @abique's idea on this I guess.

Note, there is also http://github.com/free-audio/clap/blob/main/include/clap/ext/draft/state-context.h i.e. a "replacement" draft for the current state load/save API. That could be a place to add async state IO opt-in hints if people think that is really a good idea.

tim-janik commented 1 year ago

The things I still don't fully understand yet:

I just re-read the discussion and think I have some answers now. ;-)

A) Where should the new opt-in interfaces go?

As said previously, most straight forward could be to extend /state-context.h to indicate that load/save from a background thread are ok.

B) During async loads and saves, what host calls is the plug-in allowed to call from the load and save? All of the main-thread APIs?

As a host writer, I'd say most definitely not all main-thread APIs. E.g. adding timers currently relies on being called on the main-thread, b/c the timer is then also added to the current threads (main-thread) timer event loop. But we do have clap_host->request_callback() which is labeled "[thread-safe]", i.e. may be called from any thread. That'd include the background thread for async state IO, and it can be used to defer/schedule main-thread calls.

C) During async loads and saves, is the host allowed to call into the plug-in on the main-thread while the load and save call are happening?

Yes, it should be allowed. If the plugin cannot handle main-thread execution and background state IO concurrently, it MUST NOT advertise being async state IO capable.

D) During async loads and saves, is the plug-in allowed to call into the host on the main-thread (e.g. from a timer)?

Yes. First, the plugin only gets to call into the host in the main-thread, because the host called into the plugin from the main-thread before or after async state IO was triggered. The host MUST NOT use the state IO API from a background thread, if it cannot support concurrent main thread execution.

abique commented 1 year ago

LGTM

russellmcc commented 1 year ago

Thanks @tim-janik, that all makes sense and I think is quite coherent with the existing threading design of clap.

I think this PR is good to go as-is, I'll open a separate PR for the async loading extension and we can continue to discuss the details there. At some point I probably will also open another PR for the "non-gui main thread" extension we discussed although it seems like there's some disagreement about whether that one is worth it.

abique commented 1 year ago

I think this PR is good to go as-is, I'll open a separate PR for the async loading extension and we can continue to discuss the details there. At some point I probably will also open another PR for the "non-gui main thread" extension we discussed although it seems like there's some disagreement about whether that one is worth it.

I think there are two things worth doing in a background thread:

  1. load/save
  2. activate

The plugin shall implement bg thread if there is an actual benefit (ie the load/activate is very long). Calling those functions via a bg thread should go through specific interfaces: activate_from_background_thread() ...

russellmcc commented 1 year ago

I've opened #310 as a first draft, although it doesn't yet support activating

tim-janik commented 1 year ago

I think there are two things worth doing in a background thread:

1. load/save

2. activate

The plugin shall implement bg thread if there is an actual benefit (ie the load/activate is very long). Calling those functions via a bg thread should go through specific interfaces: activate_from_background_thread() ...

Can you give an example for a lengthy activate() call that would warrant moving into a bg thread? If we're talking about sample loading or simialr work here, it might be better to introduce something like a background worker facility to prepare for activation, instead of modifying/extending the current activation logic.