Closed Bug 941873 Opened 6 years ago Closed 6 years ago

MediaStreamGraph keeps consuming CPU for AudioContexts in BFCache

Categories

(Core :: Web Audio, defect, P1, critical)

25 Branch
x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
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

People

(Reporter: karlt, Assigned: roc)

References

Details

(Keywords: memory-leak, perf)

Attachments

(3 files)

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.
Attached file 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.
Hmm, yeah, we should probably pause, since we pause other things such as timers and rAF handlers too.
Blocks: 937962
Moving to Roc per our meeting yesterday.
Assignee: karlt → roc
Blocks: 927044
This fixes the worst of the damage and is the right thing to do anyway.
Attachment #8358234 - Flags: review?(karlt)
Attachment #8358234 - Flags: review?(karlt) → review+
Duplicate of this bug: 927044
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.
Depends on: 961996
Severity: normal → critical
Attached patch v2Splinter Review
Had to add a check of mFinished when computed "blocked". See comment.
Attachment #8358234 - Attachment is obsolete: true
Attachment #8367121 - Flags: review?(karlt)
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.
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Blocks: 966873
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+
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.
No longer depends on: 970773
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.