GNOME Bugzilla – Bug 761083
Core Audio deadlock
Last modified: 2018-11-03 15:07:23 UTC
Created attachment 319680 [details] [review] Fix deadlock in core audio sink Basically any call to AudioUnitSetProperty/AudioUnitGetProperty from within the audio thread can and will cause a deadlock at some point. If you need to do that, then it should be used through the AUGraph wrapper which adds thread safety. When the audiounit render callback is executing, it enters through HALB_IOThread::Entry. Looking at the disassembly shows that this function locks the HALB_Mutex. Now, concurrently 1 or more other threads can request to close the graph while it is running the render callback. Closing the graph calls into AUMethodStop which doesnt have a symbolized disassembly, but I found the code in a java binding which shows that it calls AUI_LOCK which is exactly #define AUI_LOCK CAMutex::Locker auLock(AUI->GetMutex()); After that, the HALB module tries to lock the HALB_mutex which has been locked before by our render callback. Audio thread: ---> HALB_IOThread::Entry ---> lock HALB mutex ---------> do some processing ----> lock AUI mutex [DEADLOCK] Main thread: ------------------> stop requested ----> AUMethodStop ---> lock AUI mutex -----> StopIOProc ---> lock on HALB mutex [DEADLOCK] ------------------------------------------------ time --------------------------------------------------------------------------------------> E.g. a stop() call triggers a render_notify which will try to remove the callback and in turn calls into AudioUnitSetProperty. If currently a render callback was executing, then this will cause a deadlock in the way i tried to sketch above with my 1337 ascii drawing skills. I worked around that by adding a mutex to synchronize render callbacks and calls to stop() from the main thread. So far it works but its not the ultimate solution. IMO, it should be avoided to call into AudioUnitSetProperty from render callbacks and any callbacks that are executed in the audio thread (that includes render_notify afaik). This is pretty much inline with what is already mentioned in the comments of the respective code parts in gst_core_audio_io_proc_stop() and gst_core_audio_render_notify() in gstosxcoreaudiocommon.c. I am currently unsure whats the best place to detach the render_notify and io callbacks, but certainly it must be in a function thats called from the main thread. For reference, the JUCE and WebRTC project had similar issues with the same root cause, i.e. calling AUGet/AUSetProperty from within a rendering callback. http://www.juce.com/forum/topic/coreaudio-possible-deadlock-when-moving-mavericks https://bugs.chromium.org/p/webrtc/issues/detail?id=4128 Probably the best solution would be to port it to the AUGraph API which adds thread safety. But the provided patch simply fixes the posiible deadlocks that can occur with the current implementation.
Review of attachment 319680 [details] [review]: Isn't also some change needed in gstosxaudiosink.c here? ::: sys/osxaudio/gstosxcoreaudiocommon.c @@ +39,3 @@ + if (!G_TRYLOCK (au_render_cb)) { + g_thread_yield (); Why? Will this be called again later so we can remove the callback? Why can't we just block on the mutex?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/253.