742154 - Work around media crashtest hangs in cubeb_winmm.c

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

The winmm backend hangs during shutdown after running the media crashtests.  Despite spending a significant amount of time debugging this, I don't yet understand what the cause of this problem is.  The symptoms are as follows:

Browser hangs during shutdown after running media crashtests.  Main thread is waiting for decode related threads to terminate.  The decoders are all waiting on their audio threads to terminate.  The audio threads are blocked in cubeb_stream_destroy waiting for the enqueued buffer count to reach zero.  The buffers should become free either by completing playback, or by waveOutReset returning the buffers.  cubeb_buffer_callback is never called to release these buffers, and the dwFlags of the buffers remain WHDR_PREPARED | WHDR_INQUEUE.  In all observed cases, the enqueued buffers are the original NBUFS queued in cubeb_stream_init, and have not been returned via cubeb_buffer_callback even once.

An alternate situation appears where the audio threads are blocked in nsBufferedAudioStream::Write waiting on more space in mBuffer after starting the stream.  As above, the original NBUFS are queued and have never been returned via cubeb_buffer_callback.  cubeb_stream_start has been successfully called on these streams, 

This bug also appears in alternate versions of the code using the CALLBACK_EVENT and CALLBACK_THREAD mechanisms of waveOutOpen.  It has also been observed in a version where the buffers are not submitted for playback until cubeb_stream_start is called (after waveOutRestart succeeds).

To work around this, a limit of 32 active streams was chosen.  This limit is somewhat arbitrary, but it should be noted that a limit of 64 active streams still resulted in this deadlock-type situation occuring.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

- Re-enables cubeb (where's it's built, which is only Windows for now).
- Adds an error state to nsBufferedAudioStream and associated handling.  Allows cubeb's callback to signal that a stream is broken.
- Import latest cubeb changes for cubeb_winmm.c and cubeb.h:
  - Add CUBEB_STREAM_MAX workaround discussed in this bug. :-(
  - Remove unnecessary unprepare/prepare in drain path.
  - Handle various waveOut API errors rather than asserting.
  - Fix bytes_per_frame for CUBEB_SAMPLE_S16LE.
  - Tidy up work loop in cubeb_buffer_thread.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 612082 [details] [diff] [review]
patch v0

Review of attachment 612082 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but 8 lines of context in patches to make reviewing patches easier please!

::: media/libcubeb/src/cubeb_winmm.c
@@ +125,2 @@
>    if (got < 0) {
>      /* XXX handle this case */

Maybe you should at least leave the critical section in this case.

Comment 3

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0e390be0b88f

Comment 4

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0e390be0b88f