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

Relax constraints on callbacks by dropping Sync requirement #69

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kinetiknz
Copy link
Contributor

A cubeb backend may run user callbacks on any thread it chooses to, but must never run the same user callback on multiple threads concurrently. If I understand correctly, the Send constraint is sufficient to represent the appropriate constraint for this, making Sync unnecessary.

Fixes #68

…ent.

A cubeb backend may run user callbacks on any thread it chooses to, but
must never run the same user callback on multiple threads concurrently.
If I understand correctly, the `Send` constraint is sufficient to
represent the appropriate constraint for this, making `Sync`
unnecessary.

Fixes mozilla#68
@kinetiknz kinetiknz requested a review from a team April 27, 2022 03:29
@kinetiknz kinetiknz self-assigned this Apr 27, 2022
@eyeplum
Copy link
Contributor

eyeplum commented Apr 27, 2022

I noticed that the state changed callback and device changed callback can be called from threads other than the audio I/O thread.

E.g. with the macOS CoreAudio backend:

  • The state changed callback seems to be called from the thread that starts and stops the audio stream (main thread in my case); in case of error, it seems to be called from a thread owned by libcubeb (with label "com.mozilla.cubeb")
  • The device changed callback seems to be called from a hardware notification thread (with label "HALC_ProxyNoticication Queue")

This makes me think that maybe the other callbacks need to be kept as Sync?

@ChunMinChang
Copy link
Member

ChunMinChang commented Apr 29, 2022

A cubeb backend may run user callbacks on any thread it chooses to, but must never run the same user callback on multiple threads concurrently.

This seems true for data callback, but I guess cubeb can fire two different state callbacks at a time on the Mac platform. Suppose we unplug the device after the stream start in the following procedure:

  1. The stream starts successfully
  2. Before firing started state callback on the caller's thread, the device is unplugged, then cubeb will try to reinit the stream. If the device is the last device in the system, then we will fire an error state callback on the stream-task thread.
  3. The started state callback can be fired on the caller's thread at the same time when error state callback on the stream-task thread.

We should probably fix this on the cubeb-coreaudio and check it we have similar problems on other backends.

We could remove Sync for data callback for now and others later, what do you think?

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.

Is it possible to relax the data callback of Stream to not be Sync?
3 participants