941873 - MediaStreamGraph keeps consuming CPU for AudioContexts in BFCache

Status: VERIFIED FIXED

(Core :: Web Audio, defect, P1)

Description

[Karl Tomlinson (:karlt)]

I don't know exactly what the BFCache timeout is but is seems to be something like 30 minutes.

I think we need to pause instead of mute the graph when in BF cache.
It will need to run again to shutdown.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: convolved-chord.html

Load this in a tab and then load another page in the same tab.

Expected results: cpu drops after loading the second page.
Actual: cpu use continues for many minutes.

FWIW Chrome behaves as expected.

Comment 2

[(no longer active)]

Hmm, yeah, we should probably pause, since we pause other things such as timers and rAF handlers too.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And media elements!

Comment 4

[Maire Reavy [:mreavy]]

Moving to Roc per our meeting yesterday.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: mostly-fixed (for 28)

This fixes the worst of the damage and is the right thing to do anyway.

Comment 7

[Karl Tomlinson (:karlt)]

The effects of this bug on memory use and latency are described in bug 927044 comment 11.  Tracking because this is the same kind of bug as bug 936784 was in 26.

Comment 8

[Karl Tomlinson (:karlt)]

The patch required a bit of merging with changes from bug 943461.
A try run showed up bug 961996.
https://tbpl.mozilla.org/?tree=Try&rev=d4662ffe8dde

Things are clear with bug 961996 fixed.
https://tbpl.mozilla.org/?tree=Try&rev=5c0c832cd65f
https://tbpl.mozilla.org/?tree=Try&rev=98aa934d47dd

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: v2

Had to add a check of mFinished when computed "blocked". See comment.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8367121 [details] [diff] [review]
v2

># HG changeset patch
># User Robert O'Callahan <[e-mail]>
># Date 1389780450 -46800
>#      Wed Jan 15 23:07:30 2014 +1300
># Node ID 025356490b78f771abe1fe94d14493ce03f4c143
># Parent 7e3ceb9ea7b7266fd9fc04bd98a855e4f6bf037d
>Bug 941873. Blocked AudioNodeStreams should not run processing. r=karl
>
>diff --git a/content/media/AudioNodeStream.cpp b/content/media/AudioNodeStream.cpp
>--- a/content/media/AudioNodeStream.cpp
>+++ b/content/media/AudioNodeStream.cpp
>@@ -402,17 +402,22 @@ AudioNodeStream::ProduceOutput(GraphTime
> {
>   EnsureTrack(AUDIO_TRACK, mSampleRate);
>   // No more tracks will be coming
>   mBuffer.AdvanceKnownTracksTime(STREAM_TIME_MAX);
> 
>   uint16_t outputCount = std::max(uint16_t(1), mEngine->OutputCount());
>   mLastChunks.SetLength(outputCount);
> 
>-  if (mMuted || IsFinishedOnGraphThread()) {
>+  // Consider this stream blocked if it has already finished output. Normally
>+  // mBlocked would reflect this, but due to rounding errors our audio track may

How does finishing influence mBlocked?

>-  if (!IsFinishedOnGraphThread()) {
>-    // Don't output anything after we've finished!
>+  if (!blocked) {
>+    // Don't output anything while blocked
>     AdvanceOutputSegment();
>     if (mMarkAsFinishedAfterThisBlock && (aFlags & ALLOW_FINISH)) {
>       // This stream was finished the last time that we looked at it, and all
>       // of the depending streams have finished their output as well, so now
>       // it's time to mark this stream as finished.
>       FinishOutput();
>     }
>   }

Are you intentionally not finishing mMarkAsFinishedAfterThisBlock streams that are subsequently blocked?

My attempt at merging the previous patch was in https://tbpl.mozilla.org/?tree=Try&rev=ad22c80c32c5.  See bug 961996.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Karl Tomlinson (back Jan 28 :karlt) from comment #10)
> >-  if (mMuted || IsFinishedOnGraphThread()) {
> >+  // Consider this stream blocked if it has already finished output. Normally
> >+  // mBlocked would reflect this, but due to rounding errors our audio track may
> 
> How does finishing influence mBlocked?

A finished stream is blocked after it's finished.

> >-  if (!IsFinishedOnGraphThread()) {
> >-    // Don't output anything after we've finished!
> >+  if (!blocked) {
> >+    // Don't output anything while blocked
> >     AdvanceOutputSegment();
> >     if (mMarkAsFinishedAfterThisBlock && (aFlags & ALLOW_FINISH)) {
> >       // This stream was finished the last time that we looked at it, and all
> >       // of the depending streams have finished their output as well, so now
> >       // it's time to mark this stream as finished.
> >       FinishOutput();
> >     }
> >   }
> 
> Are you intentionally not finishing mMarkAsFinishedAfterThisBlock streams
> that are subsequently blocked?

Yes, although it shouldn't matter.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8367121 [details] [diff] [review]
v2

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to Karl Tomlinson (back Jan 28 :karlt) from comment #10)
> > How does finishing influence mBlocked?
> 
> A finished stream is blocked after it's finished.

I've found the code in MediaStreamGraphImpl::RecomputeBlockingAt() now.

> > Are you intentionally not finishing mMarkAsFinishedAfterThisBlock streams
> > that are subsequently blocked?
> 
> Yes, although it shouldn't matter.

I wondered whether it was a small optimization to finish as soon as possible, but now that mMarkAsFinishedAfterThisBlock may mark as finished in the call for this block, i don't know of any way (except finishing) that blocking may change between recomputation of blocking, and so it doesn't matter.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8d1eb31b2d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b8d1eb31b2d

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8358234 [details] [diff] [review]
mostly-fixed (for 28)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): web audio
User impact if declined: see comment 7
Testing completed (on m-c, etc.): on m-c, in a modified form.
Risk to taking this patch (and alternatives if risky): Big enough change in behaviour for some risk, but not expecting problems.  Affects web audio only.
String or IDL/UUID changes made by this patch: none.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8358234 [details] [diff] [review]
mostly-fixed (for 28)

Early in the beta cycle so we can take this with time to backout if there are unforseen problems.

Comment 17

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b532c1989306

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/b532c1989306

Comment 19

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0		
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0		

Reproduced the initial issue on Firefox 27RC, verified as fixed using Firefox 28beta2 and latest Aurora on Ubuntu 12.04 x32 and Mac OS X 10.9.1.

Note: dependency Bug 970773 is still reproducible on all of the above.