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

Remove Sync restriction from jack::ProcessHandler #121

Closed
techninja1008 opened this issue Apr 11, 2020 · 4 comments
Closed

Remove Sync restriction from jack::ProcessHandler #121

techninja1008 opened this issue Apr 11, 2020 · 4 comments

Comments

@techninja1008
Copy link

Based on the Jack Wiki, the ProcessHandler should only be called from the audio processing thread, therefore meaning that the current Sync restriction should be lifted. I am aware this was originally put into place due to #104, however I have done further testing (specifically, mapping out which threads certain handlers are called on) and have found the following results:

ThreadId(1) => {SampleRate}
ThreadId(2) => {ThreadInit}
ThreadId(3) => {BufferSize, AudioProcessing, ThreadInit}
ThreadId(4) => {GraphReorder, XRun, ThreadInit}

Output is from a program I threw together - source in gist

As you can see, thread_init is called from all three created threads, whereas the audio processing callback is only called on the one (presumably RT audio) thread. I am working to see if I can get this solidified as a guarantee in the API that this callback will only be called on one thread at any one time.

@ErikNatanael
Copy link

Thanks for looking at this, I ran into this problem when trying to implement JACK as a backend for CPAL (RustAudio/cpal#389). A Mutex would be required to access the closure doing the audio processing to comply with the CPAL API.

To me it makes no sense for the audio process to be called from two different threads at the same time unless the audio processing is entirely stateless. A multithreaded audio processing context would require a very conscious design (such as Supernova for SuperCollider). If the processing has state one of the threads has to block until the other thread releases that state. So we write code that assumes the Mutex guarding the state will never be contended, defeating the purpose of having the ProcessHandler be Sync in the first place. Mutex::try_lock() doesn't help since if we can't get the state we can't produce audio so we either block/spin or output silence; both are as I understand it unacceptable for a realtime audio process.

@Windfisch
Copy link
Contributor

Windfisch commented Jun 25, 2020

I've just had a discussion on IRC in #lad (removed unrelated discussion and squashed correction messages)

04:46 < Windfisch> just to be clear: client1's processcallback runs concurrent to itself? to client1's process callback?
04:48 < falktx> Windfisch: not to itself, but to other clients
04:48 < Windfisch> ha!
04:48 < Windfisch> thank you! this is what i wanted to know :D
04:48 < falktx> the same client will not run twice in multiple threads!
04:49 < falktx> you can think of 1 thread per client
04:49 < Windfisch> okay. so this is what enables me to safely access nonthreadsafe per-client state :)
04:50 < falktx> sure. but once you try to access any shared/global state, you need protection
04:50 < Windfisch> surely, global state must be synchronized upon, but per client state is fine because there is only one thread of execution for a client
04:50 < Windfisch> sure.
04:50 < falktx> do note though, that for buffer-size callbacks, they will happen in a separate non-audio thread
04:51 < Windfisch> may i paste these last few lines of chatlog into the github discussion on rust-jack?
04:51 < falktx> but not concurrently while audio is playing for that specific client
04:51 < falktx> sure

Seems to me that we can lift the Sync restriction.

@Windfisch
Copy link
Contributor

I've checked the source code. I am absolutely sure that the ProcessHandler only must be Send, not Sync. I am not sure if the same holds true for the NotificationHandler.

Process, Sync and Timebase callbacks are (transitively) called through
bool JackSocketClientChannel::Execute() (posix/JackSocketClientChannel.cpp) or
bool JackWinNamedPipeClientChannel::Execute() (windows/JackWinNamedPipeClientChannel.cpp).
Both basically get run from a single real time thread.

All others are handled as ClientNotify (or Shutdown and Init in rare cases). I have no idea which thread(s) can call this.

Windfisch added a commit to Windfisch/rust-jack that referenced this issue Jun 25, 2020
Windfisch added a commit to Windfisch/rust-jack that referenced this issue Jun 25, 2020
@Windfisch
Copy link
Contributor

See PR #127 which implements this.

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

No branches or pull requests

4 participants