search

mackron / miniaudio

Audio playback and capture library written in C, in a single source file.
https://miniaud.io
Other
4.07k stars 361 forks source link

Fix potential device stop/uninit sync issue #730

Closed day-garwood closed 1 year ago

day-garwood commented 1 year ago

During the device uninitialisation process, there was an issue with the order of multithreading synchronisation under certain circumstances. This led to scenarios where the device was trying to start when it should be shutting down. This commit ensures proper synchronisation and ordering, preventing such conflicts.

mackron commented 1 year ago

Thanks. This requires a more detailed explanation.

You mentioned certain circumstances - what are the specific circumstances we're talking about here? When you say it's trying to start when it should be shutting down, what are you doing to produce this exactly? Are you trying to start the device in one thread while shutting it down in another thread? As soon as you enter ma_device_uninit(), the ma_device object needs to be considered destroyed, even before it returns. Therefore it's invalid usage to call ma_device_uninit() while any other ma_device function is still in progress, including ma_device_start() and ma_device_stop(). If this is indeed what you're doing, that is invalid usage and is not something that will be addressed in miniaudio.

day-garwood commented 1 year ago

I'm not working on multiple threads - I'm actually brand new to multithreaded programming hence the reason I've probably messed up on diagnosing this one. I actually found the issue while trying to debug the odd goings on of Issue 717 using my minimalistic test program which simply uses engine_init, engine_start, a few sound functions, and engine_uninit. On the other hand, on revisiting my MiniAudio debug logs this morning, I see the issue isn't with device_uninit but actually appears in device_stop - you already try to wait for stopEvent there but for some reason that's not always happening, hence the out of sync problem when calling uninit. The check and wait I put into uninit allowed it to work here, but looking at it again it's only really a sticky-tape solution - if that also fails then we would potentially hit the same problem. The real solution would probably be to debug why wait might not actually wait in some circumstances. My apologies. Hopefully I'll be able to contribute something meaningful at some point.

mackron commented 1 year ago

How did you determine the stop event wasn't getting waited on? If the device is already stopped, ma_sound_stop() will abort early and bypass the stop event entirely. For a synchronous backend, which is the case for WASAPI, the stop event will always be waited on in ma_device_stop() if it get's past that early check, and it'll always be waited on while it's in the ma_device_state_stopping state (it sets the state to stopping, and then waits on the stop event - there is no branching or early termination happening at this point in the process).

I think it's highly unlike there's a bug in the start and stop logic at this point, although anything is possible I suppose. This code has existed since the very beginning of miniaudio, and looking at the code now there's certainly nothing obviously wrong. I don't think 717 is a threading issue. I've posted a comment over there.

day-garwood commented 1 year ago

I've been doing a lot of debug logging on MiniAudio, firstly to learn how the library works, the flow of execution etc, and secondly to try and find a fix for these strange issues. Here are excerpts of my logs. This shows MiniAudio in normal operation, and this shows what happens after the computer has been put to sleep. This is exactly the same code run, just in different scenarios. As you'll see, after sleep the order of operations is out of sync which suggests to me that for some reason ma_event_wait isn't actually waiting. What's more, doing some quick checks as I write this suggests that ma_event_wait is returning success rather than some other error. So my guess is that either this is a Windows API bug we need to try and get around somehow (are events affected when the device goes into a sleep state, for instance?), or the engine is indeed being put into some weird state when the machine is put to sleep (possibly because of the constant WASAPI reroute/fail sequence). Again, at this point I'm back to speculating. A further test as I write this suggests that this threading issue only occurs when I use ma_engine_start or ma_device_start, and again only after a sleep state; a full reinitialisation with engine_uninit/engine_init doesn't produce these problems. Apologies, I realise this is a bit long-winded, so here's a summary of the state of affairs at this point:

  1. The original issue (issue 717): Without any state checking/starting/stopping, no sound when returning from sleep. That's literally just an init, play, sleep/wake, play (attempt), uninit. That seems to be because the device is now in a stopped state.
  2. Restarting with engine_start between waking and playing the next sound causes the threading issue. Init, play, sleep/wake, start, play, uninit (attempt). In this scenario, seems there's a problem while waiting for the stop event on the device for the first time.
  3. A full engine reinitialisation cycle works perfectly. Init, play, sleep/wake, uninit, init, play, uninit. Hope this helps. Cheers.
mackron commented 1 year ago

Let's close this one and resume this in the 717 discussion.