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 initial draft for background operation (loading the state and activate the plugin from a background thread) #343

Draft
wants to merge 9 commits into
base: next
Choose a base branch
from

Conversation

abique
Copy link
Contributor

@abique abique commented Aug 19, 2023

The idea behind this design is to let the host run some expensive operation on a background thread to reduce the load on the main thread.

I've chosen an approach with a clear transition between the main thread to the background thread bgop-thread, because I think it is easier to work that way rather than having to be extra defensive everywhere and expect a call to happen at anytime from a random thread.

Copy link
Contributor

@russellmcc russellmcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this design and it seems fit for purpose to me.

One potential downside of this design: plug-ins may have timers, etc., running on the main-thread, which means correctly implementing this design may be difficult. For example, they would have to stop any calls to the host from timers, and make sure there are no data races between the main thread and the bgop thread. I don't see anything here that would be impossible to implement, it just seems quite challenging with a lot of room for error to me.

include/clap/ext/draft/background-operation.h Outdated Show resolved Hide resolved
include/clap/ext/draft/background-operation.h Outdated Show resolved Hide resolved
include/clap/ext/draft/background-operation.h Outdated Show resolved Hide resolved
include/clap/ext/draft/background-operation.h Outdated Show resolved Hide resolved

4. Perform the background operations

plugin_state->load(...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must these calls be made on the [bgop-thread]? My reading is yes, the thread must be the same, but I think this should be made explicit as the concept of "symbolic" threads exists elsewhere in the CLAP spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the plug-in needs to call the host on the main thread as part of an operation here? It's not allowed to main main-thread calls so some things may be difficult for the plug-in author to write

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good question, I'm not sure what's the best answer is:

  1. allow callbacks on bgop-thread
  2. delay the callbacks until finished()

My gut feeling is that 1. would be the right answer. I don't think it is realistic to have the plugin asking what interfaces it can call from the host on the background thread, so it'd have to be ALL OF THEM.

@tim-janik
Copy link
Contributor

I wonder whether the following use case is supposed to be covered by some kind of CLAP "background thread" extension or not:

The liquidsfz CLAP plugin implementation (planned) may be busy playing one sample (sample bank, soundfont) at a particular point in time, while the user selects a new sample from the file system the plugin is supposed to switch to.
In order to avoid discontinuities, the new soundfont needs to be loaded, paged in, extracted, parsed, interpreted in a background thread (without realtime/lowlatency scheduling), and once that is done, the currently executing synthesis logic can switch to readily prepared state reflecting the new soundfont. This may take several seconds, and during that time, synthesis callbacks, main thread timers, etc need to continue to function as usual.

AFAICS, the current proposed background-ops spec conflicts with this use case, i.e. main thread calls are suspended in the current proposal and there is no easy way for the plugin to pass a time consuming low-priority callback to the host that is supposed to be executed asynchronously.

Is the above use case something that should be accommodated for by a future version of this proposal?
Or should be handled by another (future) extension?
Or is it something that is already decided to be clearly out of scope of anything CLAP could potentially offer API for?

@swesterfeld
Copy link

I wonder whether the following use case is supposed to be covered by some kind of CLAP "background thread" extension or not:...

For reference, for the LV2 plugin, liquidsfz is using the LV2 worker extension to do the necessary expensive operations in background (like parsing and disk I/O) when the user selects a new sfz file:

I haven't seen anything like this for CLAP, so I wonder what the correct way to do it would be.

Should the plugin could use _host.requestRestart() once the user has selected a new sfz file, and then perform its loading stuff during re-activation of the plugin? But this would indeed block the main thread for potentially a long time.

So maybe in combination with implementing this extension everything would work similar to the LV2 worker thread?
Or should liquidsfz simply start and manage its own worker thread (like it would probably have to do for VST, where there are no worker threads anyway)?

@abique abique changed the title Add initial draft for background threads Add initial draft for background operation (loading the state and activate the plugin from a background thread) Aug 23, 2023
@abique
Copy link
Contributor Author

abique commented Aug 23, 2023

The liquidsfz CLAP plugin implementation (planned) may be busy playing one sample (sample bank, soundfont) at a particular point in time, while the user selects a new sample from the file system the plugin is supposed to switch to.
In order to avoid discontinuities, the new soundfont needs to be loaded, paged in, extracted, parsed, interpreted in a background thread (without realtime/lowlatency scheduling), and once that is done, the currently executing synthesis logic can switch to readily prepared state reflecting the new soundfont. This may take several seconds, and during that time, synthesis callbacks, main thread timers, etc need to continue to function as usual.

The plugin can deal with this without requiring host support.

  1. create your own background thread
  2. compute/prepare the working data
  3. message the working data back to the dsp
  4. swap the working data
  5. message back the previous working data to the background thread
  6. destroy / de-allocate resources

@tim-janik
Copy link
Contributor

@abique wrote:

The idea behind this design is to let the host run some expensive operation on a background thread to reduce the load on the main thread.

I've chosen an approach with a clear transition between the main thread to the background thread bgop-thread, because I think it is easier to work that way rather than having to be extra defensive everywhere and expect a call to happen at anytime from a random thread.

@abique wrote:

What happens when the plug-in needs to call the host on the main thread as part of an operation here? It's not allowed to main main-thread calls so some things may be difficult for the plug-in author to write

This is good question, I'm not sure what's the best answer is:

1. allow callbacks on `bgop-thread`

2. delay the callbacks until `finished()`

My gut feeling is that 1. would be the right answer. I don't think it is realistic to have the plugin asking what interfaces it can call from the host on the background thread, so it'd have to be ALL OF THEM.

I fail to see how host and plugins can reliably implement this "thread switching", given the requirements of all the other APIs. A few examples:

  1. Timers: If a plugin does [main-thread] register_timer(), are timer calls supposed to be suspended between [bgop] started() + finished()? Or do you expect the host to "transfer" the timer callback invocations, and/or timer registration to the bgop somehow? What is the scope of a timer_id now, is it per-thread (e.g. the event source ID if each thread runs its own event loop) or global? If the latter, do you expect the host to implement [bgop] unregister_timer() for a timer_id that was registered in the [main-thread]? What about timers registered in [bgop] started(), should they be automatically unregistered upon [bgop] finished()?

  2. Exec requests: What about request_restart(), request_process(), request_callback()? Are pending requests ignored if they happen directly before [bgop] started()? Or should they be "carried" into the [bgop] thread? Or should they be queued but suspended until after [bgop] finished()? The same questions arise for request_restart(), request_process(), request_callback() being called from within the [bgop] thread right before [bgop] finished(), how should that be handled by the host?

  3. Processing: This ties in to question number (2) to some extend, since you said "ALL OF THEM [API calls]". Can a plugin reasonably expect to be able to request processing from the [bgop] thread, and get calls from the [audio-thread]. If that is true, then the whole [bgop] proposal is really nothing more than the plugin allowing to say that it's ok to move the [main-thread] it is called from. And in a previous discussion we determined that this is a bad idea at least under Win/Mac and potentially complicated under Linux.

I could probably go on with other (callback related) APIs that raise similar questions, but i hope you get where I am confused now. I guess my main problems with this proposal can be summed up as follows:

  • Most of the host and plugin implementations suddenly become a lot more complex if a "background_operation" thread suddenly may replace the [main-thread]. This potentially requires modifications to virtually all current [main-thread] entry points if a plugin uses hazard pointers, TLS or otherwise maintains shared resources. Additionally the plugin implementation has to be very careful with Windows UI calls, depending on whether it is currently called from the [main-thread] or if the host decided to call the same entry points from a "background_operation" thread.

  • Introducing a "suspended main-thread" or similar concept raises a lot of questions with all the places in the current API where (cached) state is maintained one way or the other. At the very least, it needs to be cleared up if the state (e.g. timer_id or posix_fd, etc) that is currently labeled [main-thread] is from now on a) per-thread and is destroyed when a bgop starts or finishes, b) global - are main-thread resources alterable during bgop or "frozen"/"suspended"/whatever, and do bgop resources auto-cleanup?

  • What do is_main_thread() and is_audio_thread() return during bgop, and can process() somehow be called from it?

@abique
Copy link
Contributor Author

abique commented Aug 24, 2023

Yeah I get your point.

Maybe 2. would be easier: the host performs just one call on the bgop-thread then returns to main, and the plugin waits for finished() to process all its callbacks as well as the host.
So the workflow is more like "hold everything, we do one background operation and then resume".

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

Successfully merging this pull request may close these issues.

5 participants