FNA-XNA / FAudio

FAudio - Accuracy-focused XAudio reimplementation for open platforms
https://fna-xna.github.io/
Other
534 stars 72 forks source link

Fix deadlock in voice callbacks #337

Closed xtsm closed 3 months ago

xtsm commented 3 months ago

This is an attempt to fix a Wine bug. Downstream PR: https://gitlab.winehq.org/wine/wine/-/merge_requests/5424

Deadlock was being caused by the fact that some private FAudio routines hold internal mutexes while calling application callbacks. This may result in a deadlock because app might have a huge global mutex that is locked both by callbacks and by some code that e.g. submits new buffers to XAudio.

flibitijibibo commented 3 months ago

All makes sense to me... something I wish I'd logged better for each mutex is why we lock/unlock each mutex at specific times, above each lock/unlock call. Would be cool to wedge that in, but no need for you to pay for my mistakes :P

Merging this now since we're early in the release cycle, any further testing is appreciated!

Romans-I-XVI commented 2 months ago

This change breaks music playback in FNA NativeAOT-GDKX projects.

Music will play for 1 second and then break, if set to loop the track will restart and play for 1 second again (sounding like a broken record). Additionally, for some reason playback gets further in a Release build, but still breaks after a few seconds.

flibitijibibo commented 2 months ago

Reverted for now... https://github.com/FNA-XNA/FAudio/commit/6ee58c148e27d6e2d12ef5d1a866ff269509fc3b

... but it'll help to have a test program so that we can confirm a fix for Wine that also keeps FNA working too.

xtsm commented 2 months ago

Yea I could try debugging that but unfortunately I don't know what "FNA NativeAOT-GDKX" means so if someone told me how to reproduce that problem it would be pretty cool

flibitijibibo commented 2 months ago

GDKX refers to the Xbox environment, however I would expect this to happen on desktop Win32 and probably Linux/macOS as well - we should be able to make a test program that runs on desktop and see the same issue.

flibitijibibo commented 2 months ago

Turns out the problematic sample can be reproduced with XNA_Song, so here's the test program in its entirety:

/* cc test.c -lFAudio */

#include <stdint.h>
#include <unistd.h>

extern void XNA_SongInit();
extern void XNA_SongQuit();
extern float XNA_PlaySong(const char *name);
extern uint32_t XNA_GetSongEnded();

int main(int argc, char **argv)
{
    XNA_SongInit();
    XNA_PlaySong("test.ogg");
    while (!XNA_GetSongEnded())
    {
        /* 0.5 seconds */
        usleep(500000);
    }
    XNA_SongQuit();
    return 0;
}
xtsm commented 2 months ago

Managed to reproduce the problem with this code, thank you. I see the issue now: XNA_GetSongEnded determines its output by checking if the buffer queue is empty. Meanwhile the new code doesn't lock the buffer queue while calling BufferEnd cb, making the "empty buffer queue while still playing stuff" state valid for concurrent threads to observe. Can't tell if that's an XNA_Song problem or a FAudio problem though. Is there a reason why this state shouldn't be valid? Because it looks okay to me and checking the size of buffer queue to determine the end of the song seems strange. Maybe we could rely on some, i dunno, "end of stream" condition?