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

Race in FACTAudioEngine_Initialize #234

Closed
j-r opened this issue Jan 8, 2021 · 13 comments
Closed

Race in FACTAudioEngine_Initialize #234

j-r opened this issue Jan 8, 2021 · 13 comments

Comments

@j-r
Copy link

j-r commented Jan 8, 2021

On my (old and slow) system Hyperdimension Neptunia ReBirth1 exits silently (probably catching the crash) pretty soon after initializing (see +xact3 log in linked wine bug).

This problem vanishes when single stepping through FACTAudioEngine_Initialize or randomly sprinkling some FAudio_sleep(1000) into it.

Have no idea where to look now.

@flibitijibibo
Copy link
Member

Here's where the thread is made: https://github.com/FNA-XNA/FAudio/blob/master/src/FACT.c#L254

The backing FAudio thread should be fine, FACT would surprise me though since there's a massive mutex protecting everything.

@j-r
Copy link
Author

j-r commented Jan 8, 2021

Thanks! It seems that one sleep between CreateMasteringVoice and PlatformCreateThread is needed. Perhaps the device needs some time to settle down?

@flibitijibibo
Copy link
Member

Does it still die if you put the sleep after submix creation? FAudio's create functions pretty much all block so it surprises me that it causes any issues to begin with... for reference, the thread is stuck waiting on the mutex that the main thread holds until everything's started:

https://github.com/FNA-XNA/FAudio/blob/master/src/FACT_internal.c#L1640

@flibitijibibo
Copy link
Member

flibitijibibo commented Jan 8, 2021

On the subject of threads I do think I've found something: If the game registers global callbacks and the API thread executes them, we may be running into a situation where the native thread isn't compatible with the Wine thread. This is one reason we had EngineProcedureEXT:

https://github.com/FNA-XNA/FAudio/blob/master/extensions/EngineProcedureEXT.txt

We may need to amend this extension to include an export for the FACT engine thread. If these calls from the log...

0024:trace:xact3:IXACT3EngineImpl_RegisterNotification (0x17c6e8)->(0x31f784)
0024:trace:xact3:unwrap_notificationdesc Type 17
0024:trace:xact3:IXACT3EngineImpl_RegisterNotification (0x17c6e8)->(0x31f784)
0024:trace:xact3:unwrap_notificationdesc Type 6
0024:trace:xact3:IXACT3EngineImpl_RegisterNotification (0x17c6e8)->(0x31f784)
0024:trace:xact3:unwrap_notificationdesc Type 3
0024:trace:xact3:IXACT3EngineImpl_RegisterNotification (0x17c6e8)->(0x31f784)
0024:trace:xact3:unwrap_notificationdesc Type 1

... have structs where these four guys are always NULL...

https://github.com/FNA-XNA/FAudio/blob/master/include/FACT.h#L290-L293

... that's exactly what we're looking at. Even if this isn't the case for this exact game it probably should get checked out, so CC'ing @aeikum since he wrote the previous extension.

@j-r
Copy link
Author

j-r commented Jan 10, 2021

Thanks for the pointers. The problem is the thread priority call at

FAudio_PlatformThreadPriority(FAUDIO_THREAD_PRIORITY_HIGH);

If it is commented out, the problem vanishes. I played around a little more with the sleep and it has to be after

device = SDL_OpenAudioDevice(

Makes it look like an internal SDL problem. It might be an instance of https://bugzilla.libsdl.org/show_bug.cgi?id=5093.

@flibitijibibo
Copy link
Member

Does the game produce multiple sound devices? If not, it's likely we're looking at something else... but having thread priority affect the outcome reaffirms the existence of a race somewhere.

@j-r
Copy link
Author

j-r commented Jan 10, 2021

Does the game produce multiple sound devices? If not, it's likely we're looking at something else... but having thread priority affect the outcome reaffirms the existence of a race somewhere.

My resasoning is like this: we know that the problem (which is of yet not fully diagnosed, but possibly a crash - perhaps inside SDL - that I fail to observe accurately, because I'm not smart enough to look at the right place:-) is triggered by calling SDL_SetPriority very shortly after SDL_OpenAudioDevice. I don't think the actual thread priority matters at all (because the problem also gets hidden by some random amount of waiting befor SDL_SetPriority), but looking at the linked bug it mentions that the crash happens because of a race in the internal thread priority manipulation code path. Here we have AFAICT only a single audio device, but also a seperate call to SDL_SetPriority that might exercise the same path.

I have no clear idea how to verify that, though. So perhaps there are other more promising tests to do. I'm open to suggestions.

(Unfortunately my holiday is ending today, but daycare facilities will remain closed over here, so probably I won't have time to work further on this for at least some weeks.)

@alesliehughes
Copy link
Contributor

Some applications create multiple xaudio devies, which inturn calls SDL_OpenAudioDevice multiple times. There was an issue, when SDL failed to open the device, using pulse, it would attempt to teardown the device, leading to a crash.

SDL 2.0.14 has a fix for the above scenario.

With respect to the notification, wine-staging has a proof of concept patch for the XACTNOTIFICATIONTYPE_WAVEBANKPREPARED. This patch can be applied directly on top of wine if you want to give it a go.

https://github.com/wine-staging/wine-staging/tree/master/patches/xactengine3_7-Notification

@orbea
Copy link
Contributor

orbea commented Mar 2, 2021

@flibitijibibo I am not sure if this is the same as this issue, but would you mind looking at this wine issue?

https://bugs.winehq.org/show_bug.cgi?id=50757

It only crashes/freezes when wine is built against FAudio and seems related to to the referenced wine issue (https://bugs.winehq.org/show_bug.cgi?id=50416).

I also bisected it to this wine commit. wine-mirror/wine@127ef80

@flibitijibibo
Copy link
Member

It's not us, FNA uses this API and its callbacks with no issue - you may be looking at native<->Wine thread corruption issues. It is indeed a separate issue based on the bisect.

@flibitijibibo flibitijibibo changed the title Race(?) in FACTAudioEngine_Initialize (s. https://bugs.winehq.org/show_bug.cgi?id=50416) Race(?) in FACTAudioEngine_Initialize Mar 5, 2021
@flibitijibibo
Copy link
Member

SDL bug was moved to GitHub: libsdl-org/SDL#3643

@flibitijibibo flibitijibibo changed the title Race(?) in FACTAudioEngine_Initialize Race in FACTAudioEngine_Initialize Mar 5, 2021
@InfoTeddy
Copy link

Looks like the SDL bug has been fixed.

@flibitijibibo
Copy link
Member

Indeed it has - update to SDL 2.0.16 when it's released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants