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

Add clarification note about threading while GUI is alive #255

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

russellmcc
Copy link
Contributor

Fixes #195.

@CLAassistant
Copy link

CLAassistant commented Dec 22, 2022

CLA assistant check
All committers have signed the CLA.

@abique
Copy link
Contributor

abique commented Dec 22, 2022

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
Copy link
Contributor Author

russellmcc commented Dec 22, 2022

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: #195 (comment)

@abique
Copy link
Contributor

abique commented Dec 22, 2022

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
  • on windows and macos it should be the thread on which the gui and timer events happens

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

@robbert-vdh
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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:

  • If the main-thread cannot change, we'd not have is_main_thread(), a plugin could just identify the thread via TLS or gettid(), etc.
  • If we had UI toolkit negotiation (CLAP doesn't atm, but LV2 has it), a headless synthesis engine like the one Anklang has will need to implement main-thread migration. I.e. the plugin initially runs in the Synth-main-thread, then requests e.g. the Gtk+2 API, Anklang can provide that by loading a Gtk+2 ELF module which runs its event loop in a dedicated thread. In order for the plugin to use the API, it's main-thread will have to become the Gtk+2 thread.
  • Integration with QT-4, QT-4, etc could look similar.
* 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
Copy link
Contributor Author

russellmcc commented Dec 23, 2022

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
Copy link
Contributor

abique commented Dec 26, 2022

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

@abique
Copy link
Contributor

abique commented Dec 26, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

russellmcc commented Jan 2, 2023

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
Copy link
Contributor

abique commented Jan 3, 2023

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"

  • when loading/saving the state from a background thread, the host guarantee that it won't make any other calls into the plugin on the main thread
  • the plugin guarantee that it will take care of concurrency if it receives an OS event during the operation

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:

  • the main-thread is symbolic; the host can even have one main-thread for each plugin instances
  • the main-thread can't change
    • I believe that it is too dangerous to change the main-thread's underlying os-thread, or we'd need a hard requirement that only the host can initiate a call on the main-thread, and the plugin must pay extra concurrency attention if it is called back by OS events.
  • gui may only be shown if the main-thread = gui-thread

What do you think?

@abique
Copy link
Contributor

abique commented Jan 3, 2023

Maybe the background thread can also apply to plugin activation?

@tim-janik
Copy link
Contributor

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:

  • Plugins have to handle audio-threads migrating often (e.g. in hosts with thread pools)
  • Plugins have to handle audio-thread == main-thread (e.g. Python loading a CLAP plugin)
  • Plugins can rely on the main-thread not migrating (eases plugin impls)
  • Plugins can set a flag that they handle main-thread migrations well (allows hosts to concurrently load state)

@abique
Copy link
Contributor

abique commented Jan 4, 2023

  • 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
Copy link
Contributor

  • 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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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 abique changed the base branch from main to next March 10, 2023 15:09
@abique
Copy link
Contributor

abique commented Mar 10, 2023

LGTM

@russellmcc
Copy link
Contributor Author

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 abique merged commit 1797cf4 into free-audio:next Mar 10, 2023
@abique
Copy link
Contributor

abique commented Mar 10, 2023

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
Copy link
Contributor Author

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

@tim-janik
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

Clarity on whether the "main thread" is a "symbolic thread"
5 participants