-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Operations in an audio event filter #8331
Comments
We're probably going to need to put in an SDL hint or something to tell the system to fake a disconnect event at the right moment, since we can't automate a physical device unplug, and pushing an event won't be enough, we'll need this to wreak havoc from the PulseAudio hotplug thread to actually test the thing that's most likely to explode. Or maybe a simple script that can tell the OS level "destroy this device" so SDL has to react to actual device losses/format changes/whatever, if that's possible. In the short term, I'm just going to manually unplug things to shake out the initial bugs. |
Okay, so the easiest (and arguably, most correct) solution is to destroy the disconnected devices before sending the event, which would match what this looks like when the event is retrieved in the normal event loop and prevents shenanigans...but SDL_GetAudioStreamDevice() will return 0, since it would no longer be bound to a device when the event filter runs. It's still safe to destroy the stream in the filter, and if you didn't do this from an event filter, the device ID would be zero when you get the event. But if you absolutely need that device ID and can't just react to the stream being bound to nothing in the filter's SDL_EVENT_AUDIO_DEVICE_REMOVED code, we can add a flag to the device to say "we're disconnecting and running an event filter, don't actually do anything with this device until the filter returns." |
I think having the state be the same as when the event would normally be processed in the event loop makes sense. If necessary I can cache off the device ID or treat getting an ID of 0 as meaning that the stream was disconnected. |
Give the latest a try and see if it works for you. |
Works great, thanks! |
- No more tapdance to either join the audio device thread or have it detach itself. Significant simplication of and fixes to the locking code to prevent deadlocks. - Physical devices now keep a refcount. Each logical device increments it, as does the existence of a device thread, etc. Last unref destroys the device and takes it out of the device_hash. Since there's a lot of moving parts that might be holding a reference to a physical device, this seemed like a safer way to protect the object. - Disconnected devices now continue to function as zombie devices. Playback devices will still consume data (and just throw it away), and capture devices will continue to produce data (which always be silence). This helps apps that don't handle disconnect events; the device still stops playing/capturing, but bound audio streams will still consume data so they don't allocate more data infinitely, and apps that depend on an audio callback firing regularly to make progress won't hang. Please note that disconnected audio devices must now be explicitly closed! They always _should_ have been, but before this commit, SDL3 would destroy the disconnected device for you (and manually closing afterwards was a safe no-op). Reference Issue #8331. Fixes #8386. (and probably others).
@slouken: just pushed a change to how disconnect events behave in event filters (devices continue to exist, streams continue to be bound, etc, and the device acts like it still works until you explicitly close it). Once you update, make sure this still works for you. |
… event. This prevents catastrophe if someone tries to close the device in an event filter in response to the event. Note that this means SDL_GetAudioStreamDevice() for any stream on this device will return 0 during the event filter! Fixes libsdl-org#8331.
Some things an application might want to do in a filter function processing audio device events:
These should probably be supported and tested in automated tests.
The text was updated successfully, but these errors were encountered: