MediaStreamGraph keeps consuming CPU for AudioContexts in BFCache

VERIFIED FIXED in Firefox 28, Firefox OS v1.3

Status

()

P1
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: karlt, Assigned: roc)

Tracking

({memory-leak, perf})

25 Branch
mozilla29
x86_64
Linux
memory-leak, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 wontfix, firefox27 wontfix, firefox28+ verified, firefox29+ verified, firefox-esr24 unaffected, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 8336375 [details]
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

5 years ago
Hmm, yeah, we should probably pause, since we pause other things such as timers and rAF handlers too.
And media elements!
(Reporter)

Updated

5 years ago
Blocks: 937962
Moving to Roc per our meeting yesterday.
Assignee: karlt → roc
(Reporter)

Updated

5 years ago
Blocks: 927044
Created attachment 8358234 [details] [diff] [review]
mostly-fixed (for 28)

This fixes the worst of the damage and is the right thing to do anyway.
Attachment #8358234 - Flags: review?(karlt)
(Reporter)

Updated

5 years ago
Attachment #8358234 - Flags: review?(karlt) → review+
(Reporter)

Updated

5 years ago
Duplicate of this bug: 927044
(Reporter)

Comment 7

5 years ago
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.
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-firefox26: --- → wontfix
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox-esr24: --- → unaffected
tracking-firefox28: --- → ?
tracking-firefox29: --- → ?
Keywords: mlk
(Reporter)

Updated

5 years ago
Depends on: 961996
(Reporter)

Updated

5 years ago
Severity: normal → critical
tracking-firefox28: ? → +
tracking-firefox29: ? → +
Created attachment 8367121 [details] [diff] [review]
v2

Had to add a check of mFinished when computed "blocked". See comment.
Attachment #8358234 - Attachment is obsolete: true
Attachment #8367121 - Flags: review?(karlt)
(Reporter)

Comment 10

5 years ago
Comment on attachment 8367121 [details] [diff] [review]
v2

># HG changeset patch
># User Robert O'Callahan <robert@ocallahan.org>
># 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.
(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.
(Reporter)

Comment 12

5 years ago
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.
Attachment #8367121 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/9b8d1eb31b2d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox29: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Reporter)

Updated

5 years ago
Blocks: 966873
(Reporter)

Comment 15

5 years ago
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.
Attachment #8358234 - Attachment description: mostly-fixed → mostly-fixed (for 28)
Attachment #8358234 - Attachment is obsolete: false
Attachment #8358234 - Flags: approval-mozilla-beta?
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.
Attachment #8358234 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Reporter)

Comment 17

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/b532c1989306
status-firefox28: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/b532c1989306
status-b2g-v1.2: affected → wontfix
status-b2g-v1.3: affected → fixed
status-b2g-v1.4: --- → fixed
status-firefox27: affected → wontfix
Keywords: verifyme
(Reporter)

Updated

5 years ago
Depends on: 970773
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.
status-firefox28: fixed → verified
status-firefox29: fixed → verified
Keywords: verifyme
(Reporter)

Updated

5 years ago
No longer depends on: 970773
Status: RESOLVED → VERIFIED
status-b2g-v1.3T: --- → fixed
You need to log in before you can comment on or make changes to this bug.