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

soundserver_cubeb: fix cross-thread access of the sound out DMA buffer #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mihaip
Copy link
Contributor

@mihaip mihaip commented Aug 15, 2024

Instead of directly reading from the sound out DMA buffer in the cubeb callback, we have a polling task that copies it to a thread-safe ring buffer (found in the cubeb sources). That buffer can be read from the cubeb callback safely.

@maximumspatium
Copy link
Collaborator

I'll look at this tonight.

@mihaip mihaip force-pushed the upstream-ringbuffer branch from 944a47a to d46a25c Compare August 16, 2024 04:51
Instead of directly reading from the sound out DMA buffer in the cubeb
callback, we have a polling task that copies it to a thread-safe ring
buffer (found in the cubeb sources). That buffer can be read from the
cubeb callback safely.

Required removing `using namespace std` in timermanager.h since it was
leading to Windows compilation failures due to ambiguity with the `byte`
type (having `using namespace` in header files is generally discouraged)

Results in compilation failures on windows, and generally discouraged
in header files.
@mihaip mihaip force-pushed the upstream-ringbuffer branch from d46a25c to a8f7d72 Compare August 16, 2024 05:05
@joevt
Copy link
Contributor

joevt commented Aug 17, 2024

This is necessary only if the cubeb callback can read the sound out DMA buffer after the guest OS thinks the sound hardware is done with the sound out DMA buffer?

@mihaip
Copy link
Contributor Author

mihaip commented Aug 17, 2024

At least on a macOS host the cubeb callback runs on a separate thread, and can be invoked at arbitrary times. The main thread (where the rest of DPPC runs) can be in any state, including mid-update of the DMA channel fields. That would lead to inconsistent results at best, or corruption at worst.

The DMA channel read also has other side effects (e.g. it may register a timer in the PDM/AMIC implementation), which is another source of threading issues. I put some examples of the issues that Thread Sanitizer flagged in Discord: https://discord.com/channels/394224641654259712/441411517297065994/1273848926067232842

The approach here is to do all DMA channel acces on the main thread, and to have it keep a (thread safe) buffer filled that the cubeb callback/thread can read from. This being done via polling is not ideal, but I'm less familiar with DMA internals (ideally there could be a callback reigsteree whenever data is added).

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.

3 participants