Work around media crashtest hangs in cubeb_winmm.c

RESOLVED FIXED in mozilla14

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

(Depends on: 1 bug)

Trunk
mozilla14
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 612082 [details] [diff] [review]
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.
Attachment #612082 - Flags: review?(cpearce)
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.
Attachment #612082 - Flags: review?(cpearce) → review+
(Assignee)

Comment 3

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/0e390be0b88f
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/0e390be0b88f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 747793
(Assignee)

Updated

5 years ago
Depends on: 793024
Depends on: 802037
You need to log in before you can comment on or make changes to this bug.