I'm also going to remove some cases of explicit blocking here.

Bug 1201393. Remove usage of FLAG_BLOCK_* from OutputStreamData::Connect. r=jwwang We don't want to block stream decoding on the output MediaStream, or vice versa.

Bug 1201393. Remove usage of FLAG_BLOCK_OUTPUT from MediaRecorder. r=jwwang Blocking on the source stream will be treated as silence/no video change.

Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from MediaRecorder. r=jwwang FLAG_BLOCK_INPUT is not needed on mPipeStream because nothing should cause mPipeStream to block.

Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from MediaStreamAudioSourceNode. r=jwwang There's no reason why WebAudio should block an incoming MediaStream.

Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot

Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot We don't need AudioNodes to block each other anymore.

Bug 1201393. Remove explicit blocking when generating a MediaStream from a media element. r=jwwang Since we're not trying to eliminate "dead air" from the generated stream anymore, there's no need for this blocking code.

Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan

Comment on attachment 8656385 [details] MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_* from OutputStreamData::Connect. r=jwwang https://reviewboard.mozilla.org/r/18169/#review16259

Comment on attachment 8656386 [details] MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_OUTPUT from MediaRecorder. r=jwwang https://reviewboard.mozilla.org/r/18171/#review16261

Comment on attachment 8656387 [details] MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from MediaRecorder. r=jwwang https://reviewboard.mozilla.org/r/18173/#review16263

Comment on attachment 8656388 [details] MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from MediaStreamAudioSourceNode. r=jwwang https://reviewboard.mozilla.org/r/18175/#review16265

Comment on attachment 8656391 [details] MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot https://reviewboard.mozilla.org/r/18181/#review16267

Since blocking is removed, the position reported by DecodedStream::GetPosition() will keep increasing even when the media element is paused, right? How will HTMLMediaElement.currentTime handle this after it is resumed from pause?

You're quite right, that's a bug in attachment 8656391 [details]. I'll just drop that patch out for now.

Comment on attachment 8656389 [details] MozReview Request: Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan

Comment on attachment 8656390 [details] MozReview Request: Bug 1201393. Create an iterator for MediaStreamGraph to iterate over all its streams. r=karlt Bug 1201393. Create an iterator for MediaStreamGraph to iterate over all its streams. r=karlt

Comment on attachment 8656391 [details] MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot To make this work, we have to iterate over suspended MediaStreams in a few more places. We don't need START_TIME_DELAYED anymore since blocking takes care of that. I think it's good to allow suspended MediaStreams to notify the main thread that they're finished; we might need that later when we have non-AudioNode streams being suspended.

Comment on attachment 8656392 [details] MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot We don't need AudioNodes to block each other anymore.

Comment on attachment 8656389 [details] MozReview Request: Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan

Comment on attachment 8656389 [details] MozReview Request: Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan https://reviewboard.mozilla.org/r/18177/#review16317 I'm fine with this patch, although I would have done it a bit differently. ::: dom/media/webspeech/synth/nsSpeechTask.h:52 (Diff revision 2) > + void InitIndirectAudio(); It seems like this could still be one function with a boolean argument. ::: dom/media/webspeech/synth/nsSynthVoiceRegistry.cpp:772 (Diff revision 2) > - if (!mStream) { > + aTask->InitDirectAudio(); If this was one function, there would be no need for if/else, but simply passing `serviceType == nsISpeechService::SERVICETYPE_INDIRECT_AUDIO` as an argument.

(In reply to Eitan Isaacson [:eeejay] from comment #23) > I'm fine with this patch, although I would have done it a bit differently. Thanks. Generally I think we try to avoid functions taking boolean parameters. They're hard to read at the call site, although not in this case I guess.

https://reviewboard.mozilla.org/r/18181/#review16375 ::: dom/media/MediaStreamGraph.cpp (Diff revision 2) > - if (!stream->IsSuspended()) { > - bool streamHasOutput = blockedTime < aNextCurrentTime - aPrevCurrentTime; > + bool streamHasOutput = blockedTime < aNextCurrentTime - aPrevCurrentTime; > - NS_ASSERTION(!streamHasOutput || !stream->mNotifiedFinished, > + NS_ASSERTION(!streamHasOutput || !stream->mNotifiedFinished, > - "Shouldn't have already notified of finish *and* have output!"); > + "Shouldn't have already notified of finish *and* have output!"); > > - if (streamHasOutput) { > + if (streamHasOutput) { > - StreamNotifyOutput(stream); > + StreamNotifyOutput(stream); > - } > + } > > - if (stream->mFinished && !stream->mNotifiedFinished) { > + if (stream->mFinished && !stream->mNotifiedFinished) { > - StreamReadyToFinish(stream); > + StreamReadyToFinish(stream); > - } > + } I assume that mFinished won't change on suspended streams. Is stream time advancing on suspended streams so that GetAllTracksEnd() might be reached in StreamReadyToFinish()? If not, then the only reason to process main thread updates on suspended streams is the fact that updates are not dispatched each iteration?

Comment on attachment 8656390 [details] MozReview Request: Bug 1201393. Create an iterator for MediaStreamGraph to iterate over all its streams. r=karlt https://reviewboard.mozilla.org/r/18179/#review16373 The changes here are good if you can avoid IsSuspended(), thanks. ::: dom/media/MediaStreamGraph.cpp:393 (Diff revision 2) > - if (runningAndSuspendedPair[array] == &mStreams) { > + if (!stream->IsSuspended()) { I don't see IsSuspended() declared at http://reviewboard-hg.mozilla.org/gecko/file/4248871f6cd7/dom/media/MediaStreamGraph.h Looks like this will be removed in https://reviewboard.mozilla.org/r/18181/diff/2 so move UpdateCurrentTimeForStreams() changes to that patch to avoid using IsSuspended(), but see my question on that patch. ::: dom/media/MediaStreamGraphImpl.h:555 (Diff revision 2) > + nsTArray<MediaStream*>* Array() > + { > + return mArrayNum == 0 ? &mGraph->mStreams : &mGraph->mSuspendedStreams; > + } Can you make Array() private, please, to clarify that it is just part of the implementation? The member variables can also be private. ::: dom/media/MediaStreamGraphImpl.h:577 (Diff revision 2) > + StreamSet Streams() { return StreamSet(*this); } Streams() returns different streams from what are in mStreams, so I wonder about calling this all streams, but then maybe it would be better to rename mStreams to mRunningStreams, which is a different issue.

Try green on Linux debug.

https://reviewboard.mozilla.org/r/18179/#review16373 > I don't see IsSuspended() declared at > http://reviewboard-hg.mozilla.org/gecko/file/4248871f6cd7/dom/media/MediaStreamGraph.h > > Looks like this will be removed in > https://reviewboard.mozilla.org/r/18181/diff/2 > so move UpdateCurrentTimeForStreams() changes to that patch to avoid using IsSuspended(), but see my question on that patch. Good idea. > Can you make Array() private, please, to clarify that it is just part of the implementation? > The member variables can also be private. Sure > Streams() returns different streams from what are in mStreams, so I wonder about calling this all streams, but then maybe it would be better to rename mStreams to mRunningStreams, which is a different issue. Sure, I'll rename it to AllStreams.

https://reviewboard.mozilla.org/r/18179/#review16373 > Good idea. Actually to fix this more simply without getting tangled up with other changes, I just split the loop so that the !IsSuspended work is done in a separate loop over mStreams.

https://reviewboard.mozilla.org/r/18179/#review16373 > Actually to fix this more simply without getting tangled up with other changes, I just split the loop so that the !IsSuspended work is done in a separate loop over mStreams. No I won't, that doesn't work. I'll replace !IsSuspended with mSuspendedStreams.IndexOf(stream) == mSuspendedStreams.NoIndex.

(In reply to Karl Tomlinson (ni?:karlt) from comment #25) > I assume that mFinished won't change on suspended streams. Is stream time > advancing on suspended streams so that GetAllTracksEnd() might be reached in > StreamReadyToFinish()? That's one possibility. > If not, then the only reason to process main thread updates on suspended > streams is the fact that updates are not dispatched each iteration? That's also a potential issue. Basically I don't want to have to think about this too much. Skipping updates on suspended streams is fragile when it's non-obvious, and here I don't think it's obvious. Also keep in mind that most AudioNodes already suppress main-thread updates so those suspended nodes we notify on shouldn't cost us much.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > https://reviewboard.mozilla.org/r/18179/#review16373 > > > Actually to fix this more simply without getting tangled up with other changes, I just split the loop so that the !IsSuspended work is done in a separate loop over mStreams. > > No I won't, that doesn't work. I'll replace !IsSuspended with > mSuspendedStreams.IndexOf(stream) == mSuspendedStreams.NoIndex. I'm keen to avoid new n^2 costs involving suspended streams. If this is being removed in the next patch, then it doesn't matter. But I expect it is better to have no test than a test that makes this n^2.

Comment on attachment 8656392 [details] MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot https://reviewboard.mozilla.org/r/18183/#review16515

Comment on attachment 8656391 [details] MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot https://reviewboard.mozilla.org/r/18181/#review16517

(In reply to Karl Tomlinson (ni?:karlt) from comment #32) > But I expect it is better to have no test than a test that makes this n^2. We already have that in UpdateStreamOrder, which calls StreamSuspended in a couple of places during its loop. Note that the O(N^2) cost is only when AudioContext.suspend/resume are being used, and I have patches to get rid of this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/05bae6d384d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/75832092cd8d https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c818c29872 https://hg.mozilla.org/integration/mozilla-inbound/rev/0f56b9b904a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc8220b9561 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0794169172 https://hg.mozilla.org/integration/mozilla-inbound/rev/61079e59ef35 https://hg.mozilla.org/integration/mozilla-inbound/rev/321303b7d428

https://hg.mozilla.org/mozilla-central/rev/05bae6d384d9 https://hg.mozilla.org/mozilla-central/rev/75832092cd8d https://hg.mozilla.org/mozilla-central/rev/e3c818c29872 https://hg.mozilla.org/mozilla-central/rev/0f56b9b904a2 https://hg.mozilla.org/mozilla-central/rev/fcc8220b9561 https://hg.mozilla.org/mozilla-central/rev/2d0794169172 https://hg.mozilla.org/mozilla-central/rev/61079e59ef35 https://hg.mozilla.org/mozilla-central/rev/321303b7d428