[On Hold] Allow multiple SDL handles per device

aeikum commented

2018-11-19 11:48:43 -06:00

(Migrated from github.com)

This fixes audio in WWE2k19, which first opens an 11kHz xaudio device, and then a separate 48kHz device and expects low latency on the latter.

This removes the global tracking of platform devices and instead tracks them in the FAudio object. This shouldn't break any external platform backends(?), but they might want to change to use the new platform pointer instead of a global list, for consistency.

The game uses GetState to maintain 4 buffers in the queue, so this debugging was useful there.

This fixes audio in WWE2k19, which first opens an 11kHz xaudio device, and then a separate 48kHz device and expects low latency on the latter. This removes the global tracking of platform devices and instead tracks them in the FAudio object. This shouldn't break any external platform backends(?), but they might want to change to use the new platform pointer instead of a global list, for consistency. The game uses GetState to maintain 4 buffers in the queue, so this debugging was useful there.

flibitijibibo commented

2018-11-19 12:07:03 -06:00

(Migrated from github.com)

I believe VS2010 has this (I think?) but just in case, we do have SDL_PRIu64 if we need to map it like the other stdlib bits:

https://hg.libsdl.org/SDL/file/9dda3f3e9794/include/SDL_stdinc.h#l179

As for the rest, I do like this because AFAIK XAudio2 has a thread for each graph, even if multiple graphs use a single device. The only question is how safe this is - @rcgordon, we wanted to make sure that it was safe to open a single render device multiple times; the only place where I'm certain it produces an error is when we only have a default device, which hits this block here:

https://hg.libsdl.org/SDL/file/144400e4630d/src/audio/SDL_audio.c#l1285

This covers most but not all of our backends; dsound/wasapi/alsa/pulse are okay, but coreaudio is not (I believe a secret platform is affected too). We're wondering if it would make sense to have a fallback that produces SDL_AudioStreams for multiple streams on a single device. For example, in WWE2k19's case, we could open up two instances with 2 different sample rates on the same device, and SDL would produce an SDL_AudioStream for any instance with a sample rate that doesn't match the native device. It would mean opening the native device with the defaults 100% of the time (so no matter what spec.freq says, a device reporting 48KHz would always be 48KHz) but it would make audio support a lot easier to work with not just in strange cases like this one, but other scenarios where different middlewares may want to access the device (for example, FMOD could open up the device for general playback, but Bink might want to open the same device for playing back a specific video file).

I believe VS2010 has this (I think?) but just in case, we do have SDL_PRIu64 if we need to map it like the other stdlib bits: https://hg.libsdl.org/SDL/file/9dda3f3e9794/include/SDL_stdinc.h#l179 As for the rest, I do like this because AFAIK XAudio2 has a thread for each graph, even if multiple graphs use a single device. The only question is how safe this is - @rcgordon, we wanted to make sure that it was safe to open a single render device multiple times; the only place where I'm certain it produces an error is when we only have a default device, which hits this block here: https://hg.libsdl.org/SDL/file/144400e4630d/src/audio/SDL_audio.c#l1285 This covers most but not all of our backends; dsound/wasapi/alsa/pulse are okay, but coreaudio is not (I believe a secret platform is affected too). We're wondering if it would make sense to have a fallback that produces SDL_AudioStreams for multiple streams on a single device. For example, in WWE2k19's case, we could open up two instances with 2 different sample rates on the same device, and SDL would produce an SDL_AudioStream for any instance with a sample rate that doesn't match the native device. It would mean opening the native device with the defaults 100% of the time (so no matter what `spec.freq` says, a device reporting 48KHz would always be 48KHz) but it would make audio support a lot easier to work with not just in strange cases like this one, but other scenarios where different middlewares may want to access the device (for example, FMOD could open up the device for general playback, but Bink might want to open the same device for playing back a specific video file).

flibitijibibo commented

2018-11-19 14:10:45 -06:00

(Migrated from github.com)

Minor correction: CoreAudio is okay with this patch, but only on macOS:

https://hg.libsdl.org/SDL/file/144400e4630d/src/audio/coreaudio/SDL_coreaudio.m#l842

I'm surprised this isn't available on iOS as well, but I don't know what CoreAudio is like over there. Either way, the iOS/tvOS builds would still be affected by this.

Minor correction: CoreAudio is okay with this patch, but only on macOS: https://hg.libsdl.org/SDL/file/144400e4630d/src/audio/coreaudio/SDL_coreaudio.m#l842 I'm surprised this isn't available on iOS as well, but I don't know what CoreAudio is like over there. Either way, the iOS/tvOS builds would still be affected by this.

flibitijibibo commented

2018-11-21 09:11:00 -06:00

(Migrated from github.com)

I merged this into a devices branch in the main FAudio repo:

https://github.com/FNA-XNA/FAudio/tree/devices

This all seems fine to me but the SDL ambiguity complicates things a bit. For most platforms this should be fine, but some like iOS/Android (CC @TheSpydog @0x0ade) might complain about it. We should fix this in SDL first. Once that's dealt with in Hg, this'll go in FAudio's master.

I merged this into a `devices` branch in the main FAudio repo: https://github.com/FNA-XNA/FAudio/tree/devices This all seems fine to me but the SDL ambiguity complicates things a bit. For most platforms this should be fine, but some like iOS/Android (CC @TheSpydog @0x0ade) might complain about it. We should fix this in SDL first. Once that's dealt with in Hg, this'll go in FAudio's `master`.

flibitijibibo commented

2018-11-27 21:12:08 -06:00

(Migrated from github.com)

Just to be 100% sure we checked this on iOS and it does throw an error due to OnlyHasDefaultDevice. If we can fix the backend to not have that bool set, it should be okay... I’m pretty sure CoreAudio on iOS is okay with multiple contexts on one device.

icculus commented

2018-11-28 00:14:45 -06:00

(Migrated from github.com)

I think iOS probably actually supports multiple devices, since you could theoretically be hooked up to multiple bluetooth speakers/headphones/whatnot, but I haven't researched this at all.

Regardless of that, it seems to me that just removing the check about opening the default device a second time is the correct thing to do.

SDL has this comment (which I assume I wrote):

     * Also make sure that the simple case prevents multiple simultaneous
     *  opens of the default system device.

...but I don't know why that was important, if it ever was. I might have just been overzealous when redesigning this for SDL2. Clearly we don't care about opening the default device multiple times in the !OnlyHasDefaultOutputDevice case.

We're wondering if it would make sense to have a fallback that produces SDL_AudioStreams for multiple streams on a single device.

To be clear, we don't promise the backend will allow a second opening of the same device, so FAudio might need this fallback in any case if it's a widespread problem in the wild...but I'd rather not put that code into SDL itself, if we can avoid it.

I think iOS probably actually supports multiple devices, since you could theoretically be hooked up to multiple bluetooth speakers/headphones/whatnot, but I haven't researched this at all. Regardless of that, it seems to me that just removing the check about opening the default device a second time is the correct thing to do. SDL has this comment (which I assume I wrote): ``` * Also make sure that the simple case prevents multiple simultaneous * opens of the default system device. ``` ...but I don't know why that was important, if it ever was. I might have just been overzealous when redesigning this for SDL2. Clearly we don't care about opening the default device multiple times in the !OnlyHasDefaultOutputDevice case. > We're wondering if it would make sense to have a fallback that produces SDL_AudioStreams for multiple streams on a single device. To be clear, we don't promise the backend will allow a second opening of the same device, so FAudio might need this fallback in any case if it's a widespread problem in the wild...but I'd rather not put that code into SDL itself, if we can avoid it.

flibitijibibo commented

2018-11-28 09:57:50 -06:00

(Migrated from github.com)

Yeah, the AudioStream juggling would suck to have to maintain, so that's only if we get desperate.

But that's not looking to be the case here; Caleb just tested bypassing the check on iOS and it's okay. If this is also the case for Android then I think that'd be enough for us to use this patch by default.

Yeah, the AudioStream juggling would suck to have to maintain, so that's only if we get desperate. But that's not looking to be the case here; Caleb just tested bypassing the check on iOS and it's okay. If this is also the case for Android then I think that'd be enough for us to use this patch by default.

flibitijibibo commented

2018-11-28 12:29:38 -06:00

(Migrated from github.com)

Checked Android and it doesn't work, but we know the reason why: The Java portion of the SDL audio manager only supports one track at a time:

https://hg.libsdl.org/SDL/file/261716de5769/android-project/app/src/main/java/org/libsdl/app/SDLAudioManager.java

Here's exactly what happens: OpenAudioDevice runs once and creates mAudioTrack...

https://hg.libsdl.org/SDL/file/261716de5769/android-project/app/src/main/java/org/libsdl/app/SDLAudioManager.java#l205

... then when it calls OpenAudioDevice again, the null check fails and both devices use the same Track. The result is that this, for example, gets called twice per period...

https://hg.libsdl.org/SDL/file/261716de5769/android-project/app/src/main/java/org/libsdl/app/SDLAudioManager.java#l244

... so you get two contexts running one after the other, which makes some... interesting results.

If this got changed to manage multiple AudioTrack/AudioRecord instances, I think it would work. @0x0ade will have further details as he breaks this apart some more. @rcgordon, is this change something Sam would be willing to put up with?

Checked Android and it doesn't work, but we know the reason why: The Java portion of the SDL audio manager only supports one track at a time: https://hg.libsdl.org/SDL/file/261716de5769/android-project/app/src/main/java/org/libsdl/app/SDLAudioManager.java Here's exactly what happens: OpenAudioDevice runs once and creates mAudioTrack... https://hg.libsdl.org/SDL/file/261716de5769/android-project/app/src/main/java/org/libsdl/app/SDLAudioManager.java#l205 ... then when it calls OpenAudioDevice again, the null check fails and both devices use the same Track. The result is that this, for example, gets called twice per period... https://hg.libsdl.org/SDL/file/261716de5769/android-project/app/src/main/java/org/libsdl/app/SDLAudioManager.java#l244 ... so you get two contexts running one after the other, which makes some... interesting results. If this got changed to manage multiple AudioTrack/AudioRecord instances, I _think_ it would work. @0x0ade will have further details as he breaks this apart some more. @rcgordon, is this change something Sam would be willing to put up with?

flibitijibibo commented

2018-11-30 14:31:46 -06:00

(Migrated from github.com)

This got hit by today's logging changes - we can add that log line to master right away and reduce this to just the device changes.

flibitijibibo commented

2018-12-05 10:55:05 -06:00

(Migrated from github.com)

Merging this a little early - our only Android consumer is FNADroid at the moment and Maik's willing to have a fork that just reverts this change, and Ryan's looking into this on the SDL side. Sucks that we couldn't get it working all at once but we know it's possible to do, just need to wait for Hg to have the fix...

Rows
Columns

[On Hold] Allow multiple SDL handles per device #72