Last Comment Bug 752141 - Big pauses when playing sounds in BananaBread
: Big pauses when playing sounds in BananaBread
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: gecko-games
  Show dependency treegraph
 
Reported: 2012-05-04 21:15 PDT by Alon Zakai (:azakai)
Modified: 2012-05-08 10:18 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: don't hold media decoder monitor while finishing audio stream (5.53 KB, patch)
2012-05-06 16:57 PDT, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Review
Patch with `diff -w` (3.05 KB, patch)
2012-05-06 16:58 PDT, Chris Pearce (:cpearce)
kinetik: feedback+
Details | Diff | Review

Description Alon Zakai (:azakai) 2012-05-04 21:15:58 PDT
http://www.syntensity.com/static/bb/client.html

The game has some gc/recompilation pauses, every few seconds the frame rate drops. But aside from that, when you click to fire the gun, there is a very noticeable pause - longer and worse than the gc pauses - right after the sound plays, pretty much every time.

These are *NOT* gc pauses, no gc'ing shows up in the logs. Also, a CPU monitor with high frequency shows that the CPU is inactive during these pauses, as if sleeping as it waits for a lock or something like that. Commenting out the line that does .play() on the audio element prevents the stalls, so it seems to be audio related.

(If you want to comment it out, save a local copy of the html file and also the same url with .data instead of .html, that's all it needs. The relevant play() happens in _Mix_PlayChannel.)
Comment 1 Alon Zakai (:azakai) 2012-05-04 21:21:10 PDT
Bill, this sounds similar to something we saw in the past on a (private) game-related demo, I seem to recall you profiled it and found audio is to blame, and when audio was disabled the demo got much faster? Do you remember any other details that might help here?
Comment 2 Bill McCloskey (:billm) 2012-05-04 22:27:59 PDT
Yeah, the problem was that we were spending a lot of time waiting for a lock in the media code. It was a Linux-only problem, and the media people didn't seem to have much interest in fixing it. I'm not sure there's much that can be done.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-04 22:35:12 PDT
What lock are blocking on that can be held for ~seconds?
Comment 4 Chris Pearce (:cpearce) 2012-05-05 01:02:12 PDT
(In reply to Bill McCloskey (:billm) from comment #2)
> Yeah, the problem was that we were spending a lot of time waiting for a lock
> in the media code.

What's the bug number for this?

If we're stuck waiting for several seconds on a lock, we'll want to fix that.

> It was a Linux-only problem, and the media people didn't
> seem to have much interest in fixing it. I'm not sure there's much that can
> be done.

Matthew Gregan has been in the process of writing our own sound library (libcubeb) which should alleviate much of our sound issues. It's been slow going, in part because sound on Linux is a pretty broken.

libcubeb has landed for Windows and Mac, but it's disabled on Aurora+ so we can test it more. Linux still needs work I understand, so hasn't landed.
Comment 5 Alon Zakai (:azakai) 2012-05-06 12:22:32 PDT
(In reply to Chris Pearce (:cpearce) from comment #4)
> (In reply to Bill McCloskey (:billm) from comment #2)
> > Yeah, the problem was that we were spending a lot of time waiting for a lock
> > in the media code.
> 
> What's the bug number for this?
> 

Other people know more, but I suspect since the demo where we saw the problem was non-public, we couldn't open a bug on it.

But anyhow, we do have a usable testcase now in this bug.
Comment 6 Chris Pearce (:cpearce) 2012-05-06 16:36:20 PDT
This is easy to fix. We're holding the decoder monitor when we write silence to the audio stream in order to ensure we've written more than the block size in AudioLoop(), and that's blocking event runners acquiring the monitor on the main thread.
Comment 7 Chris Pearce (:cpearce) 2012-05-06 16:57:22 PDT
Created attachment 621478 [details] [diff] [review]
Patch: don't hold media decoder monitor while finishing audio stream

In addition to holding the media decoder monitor while writing silence to fill up the last block in the stream, we're *not* holding the monitor while reading mState and mStopAudioThread; we should be.

Tests pass locally, I've pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=6c8fb3cdfeb5

I'll upload a `diff -w` patch, so it's clearer what I've changed.
Comment 8 Chris Pearce (:cpearce) 2012-05-06 16:58:26 PDT
Created attachment 621479 [details] [diff] [review]
Patch with `diff -w`

Same as previous patch, but produced with `diff -w`, so it's clearer what's going on.
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-07 16:13:23 PDT
https://hg.mozilla.org/mozilla-central/rev/e103eee2cc24
Comment 11 Alon Zakai (:azakai) 2012-05-08 10:18:59 PDT
Works great, thanks for the quick resolution!

Note You need to log in before you can comment on or make changes to this bug.