bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Remove use of FLAG_BLOCK_INPUT/FLAG_BLOCK_OUTPUT

RESOLVED FIXED in Firefox 43

Status

()

Core
Audio/Video: MediaStreamGraph
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

40 bytes, text/x-review-board-request
jwwang
: review+
Details | Review
40 bytes, text/x-review-board-request
jwwang
: review+
Details | Review
40 bytes, text/x-review-board-request
jwwang
: review+
Details | Review
40 bytes, text/x-review-board-request
jwwang
: review+
Details | Review
40 bytes, text/x-review-board-request
eeejay
: review+
Details | Review
40 bytes, text/x-review-board-request
karlt
: review+
Details | Review
40 bytes, text/x-review-board-request
padenot
: review+
Details | Review
40 bytes, text/x-review-board-request
padenot
: review+
Details | Review
Comment hidden (empty)
I'm also going to remove some cases of explicit blocking here.
Created attachment 8656385 [details]
MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_* from OutputStreamData::Connect. r=jwwang

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.
Attachment #8656385 - Flags: review?(jwwang)
Created attachment 8656386 [details]
MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_OUTPUT from MediaRecorder. r=jwwang

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.
Attachment #8656386 - Flags: review?(jwwang)
Created attachment 8656387 [details]
MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from MediaRecorder. r=jwwang

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.
Attachment #8656387 - Flags: review?(jwwang)
Created attachment 8656388 [details]
MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from MediaStreamAudioSourceNode. r=jwwang

Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from MediaStreamAudioSourceNode. r=jwwang

There's no reason why WebAudio should block an incoming MediaStream.
Attachment #8656388 - Flags: review?(jwwang)
Created attachment 8656389 [details]
MozReview Request: Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan

Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot
Attachment #8656389 - Flags: review?(padenot)
Created attachment 8656390 [details]
MozReview Request: Bug 1201393. Create an iterator for MediaStreamGraph to iterate over all its streams. r=karlt

Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot

We don't need AudioNodes to block each other anymore.
Attachment #8656390 - Flags: review?(padenot)
Created attachment 8656391 [details]
MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot

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.
Attachment #8656391 - Flags: review?(jwwang)
Created attachment 8656392 [details]
MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot

Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan
Attachment #8656392 - Flags: review?(karlt)
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
Attachment #8656385 - Flags: review?(jwwang) → review+
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
Attachment #8656386 - Flags: review?(jwwang) → review+
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
Attachment #8656387 - Flags: review?(jwwang) → review+
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
Attachment #8656388 - Flags: review?(jwwang) → review+
Comment on attachment 8656391 [details]
MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot

https://reviewboard.mozilla.org/r/18181/#review16267
Attachment #8656391 - Flags: review?(jwwang) → review+
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.
Attachment #8656391 - Flags: review+ → review-
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
Attachment #8656389 - Attachment description: MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot → MozReview Request: 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
Attachment #8656390 - Attachment description: MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot → MozReview Request: Bug 1201393. Create an iterator for MediaStreamGraph to iterate over all its streams. r=karlt
Attachment #8656390 - Flags: review?(padenot) → review?(karlt)
Attachment #8656391 - Attachment description: MozReview Request: Bug 1201393. Remove explicit blocking when generating a MediaStream from a media element. r=jwwang → MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot
Attachment #8656391 - Flags: review?(padenot)
Attachment #8656391 - Flags: review?(jwwang)
Attachment #8656391 - Flags: review-
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.
Attachment #8656392 - Attachment description: MozReview Request: Bug 1201393. Remove irrelevant ProcessedMediaStream for nsSpeechTask. r=eitan → MozReview Request: Bug 1201393. Remove usage of FLAG_BLOCK_INPUT from AudioParam/AudioNode. r=padenot
Attachment #8656392 - Flags: review?(karlt) → review?(padenot)
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
Attachment #8656389 - Flags: review?(padenot) → review?(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.
Attachment #8656389 - Flags: review?(eitan) → review+
(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.
Attachment #8656390 - Flags: review?(karlt) → review+
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
Attachment #8656392 - Flags: review?(padenot) → review+
Comment on attachment 8656391 [details]
MozReview Request: Bug 1201393. Make suspended MediaStreams implicitly always block. r=padenot

https://reviewboard.mozilla.org/r/18181/#review16517
Attachment #8656391 - Flags: review?(padenot) → review+
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
(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.

Updated

3 years ago
Blocks: 1204871
Blocks: 1117871

Updated

2 years ago
Depends on: 1274797
You need to log in before you can comment on or make changes to this bug.