After discussion in dev-media, we decided to take out blocking support from MediaStreamGraph (MSG). For a detailed discussion of why we chose to do this (and other details), please see https://groups.google.com/forum/#!searchin/mozilla.dev.media/MediaStreams$20and$20%22blocking%22$20semantics/mozilla.dev.media/ijpluytUYS4/sSCuNfidKV4J

Hi Rob -- During Whistler, you volunteered to remove blocking from MSG. Is this something you can start work on now or soon so that we can land it in August? We need this to achieve web audio perf parity with competitors, and we've promised ourselves that we're going to achieve this (or even better, surpass parity) by end of Q3. So if you can't work on this now or soon, I'll need to find someone else. Thanks!

Here are the steps to fixing this: 1) Ensure that no streams ever block. 1a) When a SourceMediaStream underruns, instead of blocking, produce silence/no video. Remove the places where we call ChangeExplicitBlockerCount: 1b) When we're playing a MediaStream, and the element is paused, instead of blocking the input stream, disconnect its audio and video outputs so that updates are ignored. 1c) When a MediaStream has been captured from a media element, and the element decode pauses, instead of blocking the MediaStream just produce silence/no video frames. At this point, I think all streams will never block. (I might have missed something, but if I have, hopefully that wouldn't be too hard to fix). 1d) If we just do the above, then MediaRecorder will probably record silence instead of just skipping blocked intervals in a MediaStream. That's a slight regression so it's probably a good idea to mark audio/video chunks produced by steps 1a and 1c as "dead" so they can be ignored by MediaRecorder. 2) Remove all the blocking logic. At this point we just have a ton of dead code that we should be able to rip out fairly mechanically.

Removing blocking breaks dom/media/test/test_streams_element_capture.html, dom/media/test/test_streams_element_capture_createObjectURL.html and dom/media/test/test_streams_element_capture_reset.html. For example, in dom/media/test/test_streams_element_capture.html we connect a video element 'v' to 'vout', play 'v' to the end and then check that the currentTimes match and they're equal to the resource duration. If we eliminate blocking, these invariants no longer hold; at the beginning of playback (while starting decoding of 'v') and at the end of playback (after 'v' has ended) v is not making progress but vout still does. We could potentially preserve the tests' expected behavior by introducing an alternative concept: marking "dead air" in a track (silence/no video produced because the source was not producing anything) and, in a media element playing a MediaStream, don't advance currentTime while all tracks are dead air ... and cut those segments out of a MediaRecorder recording, too. But I'm not sure if we should actually do this. It's certainly a spec issue. Randell, what do you think? Has this been discussed anywhere while I wasn't looking?

Can the input stream just discard the data that are coming late after underflow? So the amount of data processed by MSG is not increased due to insertion of silent data.

I think that's not a good solution in general. For example if we're playing back a video and we spend 1s getting decoding going, we'd emit 1s of dead air and then chop off the first second of the video. I don't think people want that.

That's the nature of a live stream if we consider a capture of a media element a live stream. In fact, the consumer of the stream can't tell the difference whether the stream is backed by a camera or a media element.

Once again, this runs into the difference between live streams (stay current, always drop data if not available/late) and recorded streams, where offsetting late data (insert 'dead air') makes more sense

(In reply to JW Wang [:jwwang] from comment #7) > That's the nature of a live stream if we consider a capture of a media > element a live stream. In fact, the consumer of the stream can't tell the > difference whether the stream is backed by a camera or a media element. I'm not sure what you mean here. Doesn't this approach mean we should not discard any data, and instead we should modify the tests to accept that the currentTimes of the two video elements may not be in sync? (In reply to Randell Jesup [:jesup] from comment #8) > Once again, this runs into the difference between live streams (stay > current, always drop data if not available/late) and recorded streams, where > offsetting late data (insert 'dead air') makes more sense So what do you think we should do in these tests?

As Randell said, if we treat a media element capture as a live stream, we should discard late data to keep the duration the same. The tests won't need to be changed to pass. However, I think it also depends on the source a media element is playing. A capture of a media element playing an RTSP stream is more like a live stream. On the other hand, a capture of a media element playing a file is more like a recorded stream.

For RTSP the media element itself is discarding late data to stay live, so the same thing will happen to the MediaStream generated from that element, no matter what else we do. I'm leaning towards relaxing these tests so the media element currentTimess don't need to be in sync. More precisely, the vout currentTime will advance as quickly or more than the input media element currentTime. Does that sound OK?

That should be fine. Another concern is that the finial duration of v will change for insertion of dead air because the playback position is reported by the stream position. It will be solved when we don't stop AudioSink when capture happens as required by the spec and there is already a bug for that IIRC.

I agree also - relax the constraint to be >=. If there was some way to inquire as to the amount of dead air inserted, one could probably stick to source_time + dead_air_time == dest_time; but I'm not sure that's needed.

I'm having a problem with test_streams_element_capture.html with GStreamer enabled. With my modifications, we test that vout.currentTime >= test.duration. But this fails because the duration is 338.333ms but GStreamer only decodes 313.446ms of audio. I'm going to ignore this failure for now on the assumption it's not related to my patches.

(That's for small-shot-mp3.mp4. All the other tests pass.)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > I'm having a problem with test_streams_element_capture.html with GStreamer > enabled. With my modifications, we test that vout.currentTime >= > test.duration. But this fails because the duration is 338.333ms but > GStreamer only decodes 313.446ms of audio. > > I'm going to ignore this failure for now on the assumption it's not related > to my patches. Sounds like bug 1084185 comment 9? Which file is it?

small-shot-mp3.mp4

We'll need to reimplement AudioContext::Suspend/Resume. padenot, did you have anything in mind for that? How about we add an mSuspended flag to AudioNodeEngine, and just skip processing (outputting silence) when suspended? Then AudioContext::Suspend would send a control message to the MSG which would set mSuspended on all the AudioNodeEngines for that AudioContext. I guess we'd have to label each AudioNodeEngine with an ID for its AudioContext. Alternatively we could avoid scanning the entire list of MediaStreams by having AudioContext keep a weak pointer to each of its AudioNodes and send one message per AudioNode to the MSG --- but that seems unnecessarily complex since doing work per MediaStream is frequent anyway.

I talked to padenot on IRC. I ended up just making suspended streams always be treated as blocked in MSG for now. When we're ready to remove the actual MSG blocking code we'll make sure that the current time of suspended streams is accounted for properly.

With my patches so far, we have three uses of explicit blocking left: 1) nsSpeechTask uses a SourceMediaStream to play audio. It exposes Pause() and Resume() methods which block and unblock the stream. Cancel() and ForceEnd() also block the stream. 2) MediaRecorder's Pause() and Resume() methods block/unblock the TrackUnionStream that gets recorded. 3) AudioDestinationNode blocks/unblocks its stream when it's the only node in the AudioContext, IIRC so that we can stop running the MSG while there are dormant AudioContexts. There's some slightly complicated code in AudioDestinationNode to adjust for the blocked time. I looked into removing the TrackUnionStream from MediaRecorder and implementing Pause()/Resume() by just ignoring arriving data while paused ... but that gets complicated because we don't want the recorded tracks to get out of sync, and the amount of data delivered by NotifyQueuedTrackChanges can vary from moment to moment and track to track. So I think we should continue to support these cases with a direct API. I think we should generalize the suspended-streams abstraction and use it for all above cases. A suspended stream's current-time does not progress, it produces silence and no video, and it ignores its inputs.

Bug 1189506. Give AudioContext non-owning pointers to all its AudioNodes. r=karl

Bug 1189506. Put AudioContext::State inline. r=karl

Bug 1189506. Pass AudioContext to AudioNodeStream::Create. r=karlt

Bug 1189506. Make AudioContext responsible for tracking all nodes which need to be suspended and resumed. r=padenot This simplifies MediaStreamGraph by removing the need for it to be aware of which AudioContext a stream belongs to. This also makes it easier to reuse stream suspending for purposes other than AudioContext suspend/resume.

Bug 1189506. Make suspending/resuming streams more reusable. r=padenot

Bug 1189506. Don't bother blocking captured media-element MediaStreams while we're not decoding. r=jwwang They should just run in realtime since we're getting rid of blocking.

Bug 1189506. Convert ChangeExplicitBlockerCount to MediaStream::Suspend/Resume. r=padenot

Bug 1189506. Remove usage of FLAG_BLOCK_OUTPUT from MediaManager. r=jesup

Bug 1189506. Remove aFlags parameter from AllocateInputPort. r=karlt

Bug 1189506. Remove MediaInputPort::mFlags. r=karlt

Bug 1189506. Simplify blocking code now that stream blocking decision are always independent of other streams. r=karlt

Bug 1189506. Remove MediaStream::mBlockInThisPhase. r=karlt

Bug 1189506. Remove mExplicitBlockerCount and related code since it's always zero now. r=karlt

Bug 1189506. Simplify blocking calculations based on the observation that once a stream starts blocking in a given processing interval, it must stay blocked. r=karlt

Bug 1189506. Non-SourceMediaStreams in processing can only block by being finished. r=karlt

Bug 1189506. Fix multi-track MediaStream audio output. r=karlt

Bug 1189506. Simplify PlayAudio based on the fact that track time units == stream time units. r=karlt

Bug 1189506. Remove misleading comment. r=karlt

Bug 1189506. Remove unused MediaStreamGraph::GetBufferedTicks. r=karlt

Bug 1189506. Replace MediaStream::mBlocked with simpler MediaStream::mStartBlocking. r=karlt

Bug 1189506. Remove no-longer-used TimeVarying.h. r=karlt

Bug 1189506. Inline RecomputeBlocking. r=karlt

Bug 1189506. Inline ComputeStreamBlockTime. r=karlt

Bug 1189506. Remove unused NotifyConsumptionChanged. r=karlt

Bug 1189506. Remove unused mFlushSourcesNow/mFlushSourcesOnNextIteration. r=karlt

Bug 1189506. Factor out code from OneIteration into helper methods. r=karlt

Bug 1189506. Move setting of mStateComputedTime to OneIteration so it's near setting mProcessedTime. r=karlt

Bug 1189506. No need to pass aNextCurrentTime to UpdateCurrentTimeForStreams. r=karlt

Bug 1189506. Inline StreamNotifyOutput/StreamNotifyFinished. r=karlt

Bug 1189506. Rename StreamTimeToGraphTime/GraphTimeToStreamTime to ...WithBlocking. r=karlt

Bug 1189506. Set mStartBlocking in UpdateCurrentTimeForStreams to indicate that blocking time has been processed. r=karlt

Bug 1189506. Create StreamTimeToGraphTime/GraphTimeToStreamTime that don't take account of blocking, and call them from AudioNodeStream. r=karlt

Bug 1189506. Remove INCLUDE_TRAILING_BLOCKED_INTERVAL from PlayVideo. r=karlt A video frame whose timestamp is right at mStartBlocking should just be rendered then, not delayed until the end of blocking.

Bug 1189506. Call StreamTimeToGraphTime in MediaStreamGraphImpl::UpdateCurrentTimeForStreams, since we know blocking has been taken account of already there. r=karlt

Bug 1189506. Call StreamTimeToGraphTime in DecodedStreamGraphListener::NotifyOutput, since there's no blocking pending there. r=karlt

Bug 1189506. Call GraphTimeToStreamTime in AudioNodeExternalInputStream. r=karlt

Bug 1189506. Call GraphTimeToStreamTime in ExtractPendingInput since we know no blocking time has been determined yet. r=karlt

Bug 1189506. Use mProcessedTime in some places instead of passing aFrom. r=karlt

Bug 1189506. Use mStateComputedTime in some places instead of passing aTo. r=karlt

Bug 1189506. Use mProcessedTime/mStateComputedTime in ProduceDataForStreamsBlockByBlock. karlt

Bug 1189506. Use GraphTimeToStreamTime in PrepareUpdatesToMainThreadState. r=karlt

Bug 1189506. Relax assertions a bit. karlt

Bug 1189506. Make PlayVideo use GraphTimeToStreamTime/StreamTimeToGraphTime and remove StreamTimeToGraphTimeWithBlocking. r=karlt

Bug 1189506. Make PlayAudio use GraphTimeToStreamTime. r=karlt

Comment on attachment 8659491 [details] MozReview Request: Bug 1189506. Give AudioContext non-owning pointers to all its AudioNodes. r=karl Bug 1189506. Give AudioContext non-owning pointers to all its AudioNodes. r=karl

Comment on attachment 8659492 [details] MozReview Request: Bug 1189506. Put AudioContext::State inline. r=karl Bug 1189506. Put AudioContext::State inline. r=karl

Comment on attachment 8659493 [details] MozReview Request: Bug 1189506. Pass AudioContext to AudioNodeStream::Create. r=karlt Bug 1189506. Pass AudioContext to AudioNodeStream::Create. r=karlt

Comment on attachment 8659494 [details] MozReview Request: Bug 1189506. Make AudioContext responsible for tracking all nodes which need to be suspended and resumed. r=padenot Bug 1189506. Make AudioContext responsible for tracking all nodes which need to be suspended and resumed. r=padenot This simplifies MediaStreamGraph by removing the need for it to be aware of which AudioContext a stream belongs to. This also makes it easier to reuse stream suspending for purposes other than AudioContext suspend/resume.

Comment on attachment 8659495 [details] MozReview Request: Bug 1189506. Make suspending/resuming streams more reusable. r=padenot Bug 1189506. Make suspending/resuming streams more reusable. r=padenot

Comment on attachment 8659496 [details] MozReview Request: Bug 1189506. Don't bother blocking captured media-element MediaStreams while we're not decoding. r=jwwang Bug 1189506. Don't bother blocking captured media-element MediaStreams while we're not decoding. r=jwwang They should just run in realtime since we're getting rid of blocking.

Comment on attachment 8659497 [details] MozReview Request: Bug 1189506. Convert ChangeExplicitBlockerCount to MediaStream::Suspend/Resume. r=padenot Bug 1189506. Convert ChangeExplicitBlockerCount to MediaStream::Suspend/Resume. r=padenot

Comment on attachment 8659498 [details] MozReview Request: Bug 1189506. Remove usage of FLAG_BLOCK_OUTPUT from MediaManager. r=jesup Bug 1189506. Remove usage of FLAG_BLOCK_OUTPUT from MediaManager. r=jesup

Comment on attachment 8659499 [details] MozReview Request: Bug 1189506. Remove aFlags parameter from AllocateInputPort. r=karlt Bug 1189506. Remove aFlags parameter from AllocateInputPort. r=karlt

Comment on attachment 8659500 [details] MozReview Request: Bug 1189506. Remove MediaInputPort::mFlags. r=karlt Bug 1189506. Remove MediaInputPort::mFlags. r=karlt

Comment on attachment 8659501 [details] MozReview Request: Bug 1189506. Simplify blocking code now that stream blocking decision are always independent of other streams. r=karlt Bug 1189506. Simplify blocking code now that stream blocking decision are always independent of other streams. r=karlt

Comment on attachment 8659502 [details] MozReview Request: Bug 1189506. Remove MediaStream::mBlockInThisPhase. r=karlt Bug 1189506. Remove MediaStream::mBlockInThisPhase. r=karlt

Comment on attachment 8659503 [details] MozReview Request: Bug 1189506. Remove mExplicitBlockerCount and related code since it's always zero now. r=karlt Bug 1189506. Remove mExplicitBlockerCount and related code since it's always zero now. r=karlt

Comment on attachment 8659504 [details] MozReview Request: Bug 1189506. Simplify blocking calculations based on the observation that once a stream starts blocking in a given processing interval, it must stay blocked. r=karlt Bug 1189506. Simplify blocking calculations based on the observation that once a stream starts blocking in a given processing interval, it must stay blocked. r=karlt

Comment on attachment 8659505 [details] MozReview Request: Bug 1189506. Non-SourceMediaStreams in processing can only block by being finished. r=karlt Bug 1189506. Non-SourceMediaStreams in processing can only block by being finished. r=karlt

Comment on attachment 8659506 [details] MozReview Request: Bug 1189506. Fix multi-track MediaStream audio output. r=karlt Bug 1189506. Fix multi-track MediaStream audio output. r=karlt

Comment on attachment 8659507 [details] MozReview Request: Bug 1189506. Simplify PlayAudio based on the fact that track time units == stream time units. r=karlt Bug 1189506. Simplify PlayAudio based on the fact that track time units == stream time units. r=karlt

Comment on attachment 8659508 [details] MozReview Request: Bug 1189506. Remove misleading comment. r=karlt Bug 1189506. Remove misleading comment. r=karlt

Comment on attachment 8659509 [details] MozReview Request: Bug 1189506. Remove unused MediaStreamGraph::GetBufferedTicks. r=karlt Bug 1189506. Remove unused MediaStreamGraph::GetBufferedTicks. r=karlt

Comment on attachment 8659510 [details] MozReview Request: Bug 1189506. Replace MediaStream::mBlocked with simpler MediaStream::mStartBlocking. r=karlt Bug 1189506. Replace MediaStream::mBlocked with simpler MediaStream::mStartBlocking. r=karlt

Comment on attachment 8659511 [details] MozReview Request: Bug 1189506. Remove no-longer-used TimeVarying.h. r=karlt Bug 1189506. Remove no-longer-used TimeVarying.h. r=karlt

Comment on attachment 8659512 [details] MozReview Request: Bug 1189506. Inline RecomputeBlocking. r=karlt Bug 1189506. Inline RecomputeBlocking. r=karlt

Comment on attachment 8659513 [details] MozReview Request: Bug 1189506. Inline ComputeStreamBlockTime. r=karlt Bug 1189506. Inline ComputeStreamBlockTime. r=karlt

Comment on attachment 8659514 [details] MozReview Request: Bug 1189506. Remove unused NotifyConsumptionChanged. r=karlt Bug 1189506. Remove unused NotifyConsumptionChanged. r=karlt

Comment on attachment 8659515 [details] MozReview Request: Bug 1189506. Remove unused mFlushSourcesNow/mFlushSourcesOnNextIteration. r=karlt Bug 1189506. Remove unused mFlushSourcesNow/mFlushSourcesOnNextIteration. r=karlt

Comment on attachment 8659516 [details] MozReview Request: Bug 1189506. Factor out code from OneIteration into helper methods. r=karlt Bug 1189506. Factor out code from OneIteration into helper methods. r=karlt

Comment on attachment 8659517 [details] MozReview Request: Bug 1189506. Move setting of mStateComputedTime to OneIteration so it's near setting mProcessedTime. r=karlt Bug 1189506. Move setting of mStateComputedTime to OneIteration so it's near setting mProcessedTime. r=karlt

Comment on attachment 8659518 [details] MozReview Request: Bug 1189506. No need to pass aNextCurrentTime to UpdateCurrentTimeForStreams. r=karlt Bug 1189506. No need to pass aNextCurrentTime to UpdateCurrentTimeForStreams. r=karlt

Comment on attachment 8659519 [details] MozReview Request: Bug 1189506. Inline StreamNotifyOutput/StreamNotifyFinished. r=karlt Bug 1189506. Inline StreamNotifyOutput/StreamNotifyFinished. r=karlt

Comment on attachment 8659520 [details] MozReview Request: Bug 1189506. Rename StreamTimeToGraphTime/GraphTimeToStreamTime to ...WithBlocking. r=karlt Bug 1189506. Rename StreamTimeToGraphTime/GraphTimeToStreamTime to ...WithBlocking. r=karlt

Comment on attachment 8659521 [details] MozReview Request: Bug 1189506. Set mStartBlocking in UpdateCurrentTimeForStreams to indicate that blocking time has been processed. r=karlt Bug 1189506. Set mStartBlocking in UpdateCurrentTimeForStreams to indicate that blocking time has been processed. r=karlt

Comment on attachment 8659522 [details] MozReview Request: Bug 1189506. Create StreamTimeToGraphTime/GraphTimeToStreamTime that don't take account of blocking, and call them from AudioNodeStream. r=karlt Bug 1189506. Create StreamTimeToGraphTime/GraphTimeToStreamTime that don't take account of blocking, and call them from AudioNodeStream. r=karlt

Comment on attachment 8659523 [details] MozReview Request: Bug 1189506. Remove INCLUDE_TRAILING_BLOCKED_INTERVAL from PlayVideo. r=karlt Bug 1189506. Remove INCLUDE_TRAILING_BLOCKED_INTERVAL from PlayVideo. r=karlt A video frame whose timestamp is right at mStartBlocking should just be rendered then, not delayed until the end of blocking.

Comment on attachment 8659524 [details] MozReview Request: Bug 1189506. Call StreamTimeToGraphTime in MediaStreamGraphImpl::UpdateCurrentTimeForStreams, since we know blocking has been taken account of already there. r=karlt Bug 1189506. Call StreamTimeToGraphTime in MediaStreamGraphImpl::UpdateCurrentTimeForStreams, since we know blocking has been taken account of already there. r=karlt

Comment on attachment 8659525 [details] MozReview Request: Bug 1189506. Call StreamTimeToGraphTime in DecodedStreamGraphListener::NotifyOutput, since there's no blocking pending there. r=karlt Bug 1189506. Call StreamTimeToGraphTime in DecodedStreamGraphListener::NotifyOutput, since there's no blocking pending there. r=karlt

Comment on attachment 8659526 [details] MozReview Request: Bug 1189506. Call GraphTimeToStreamTime in AudioNodeExternalInputStream. r=karlt Bug 1189506. Call GraphTimeToStreamTime in AudioNodeExternalInputStream. r=karlt

Comment on attachment 8659527 [details] MozReview Request: Bug 1189506. Call GraphTimeToStreamTime in ExtractPendingInput since we know no blocking time has been determined yet. r=karlt Bug 1189506. Call GraphTimeToStreamTime in ExtractPendingInput since we know no blocking time has been determined yet. r=karlt

Comment on attachment 8659528 [details] MozReview Request: Bug 1189506. Use mProcessedTime in some places instead of passing aFrom. r=karlt Bug 1189506. Use mProcessedTime in some places instead of passing aFrom. r=karlt

Comment on attachment 8659529 [details] MozReview Request: Bug 1189506. Use mStateComputedTime in some places instead of passing aTo. r=karlt Bug 1189506. Use mStateComputedTime in some places instead of passing aTo. r=karlt

Comment on attachment 8659530 [details] MozReview Request: Bug 1189506. Use mProcessedTime/mStateComputedTime in ProduceDataForStreamsBlockByBlock. karlt Bug 1189506. Use mProcessedTime/mStateComputedTime in ProduceDataForStreamsBlockByBlock. karlt

Comment on attachment 8659532 [details] MozReview Request: Bug 1189506. Use GraphTimeToStreamTime in PrepareUpdatesToMainThreadState. r=karlt Bug 1189506. Use GraphTimeToStreamTime in PrepareUpdatesToMainThreadState. r=karlt

Comment on attachment 8659533 [details] MozReview Request: Bug 1189506. Relax assertions a bit. karlt Bug 1189506. Relax assertions a bit. karlt

Comment on attachment 8659534 [details] MozReview Request: Bug 1189506. Make PlayVideo use GraphTimeToStreamTime/StreamTimeToGraphTime and remove StreamTimeToGraphTimeWithBlocking. r=karlt Bug 1189506. Make PlayVideo use GraphTimeToStreamTime/StreamTimeToGraphTime and remove StreamTimeToGraphTimeWithBlocking. r=karlt

Comment on attachment 8659535 [details] MozReview Request: Bug 1189506. Make PlayAudio use GraphTimeToStreamTime. r=karlt Bug 1189506. Make PlayAudio use GraphTimeToStreamTime. r=karlt

Bug 1189506. Remove invalid assertion. r=karlt That's nothing stopping someone passing an AudioContext time of (e.g.) zero to AudioBufferSourceNode::Stop and OscillatorNode::Start/Stop, which can get turned into a negative time here. Those nodes can handle negative start/stop times just fine.

Comment on attachment 8659496 [details] MozReview Request: Bug 1189506. Don't bother blocking captured media-element MediaStreams while we're not decoding. r=jwwang https://reviewboard.mozilla.org/r/18855/#review16849

Comment on attachment 8659498 [details] MozReview Request: Bug 1189506. Remove usage of FLAG_BLOCK_OUTPUT from MediaManager. r=jesup https://reviewboard.mozilla.org/r/18859/#review16883

Comment on attachment 8659494 [details] MozReview Request: Bug 1189506. Make AudioContext responsible for tracking all nodes which need to be suspended and resumed. r=padenot https://reviewboard.mozilla.org/r/18851/#review16893 ::: dom/media/MediaStreamGraph.h:1281 (Diff revision 1) > - * aNodeStream is the stream of the DestinationNode of the AudioContext. > + * aStreams is the streams of all the AudioNodes of the AudioContext that s/is/are/

Comment on attachment 8659495 [details] MozReview Request: Bug 1189506. Make suspending/resuming streams more reusable. r=padenot https://reviewboard.mozilla.org/r/18853/#review16895

Comment on attachment 8659497 [details] MozReview Request: Bug 1189506. Convert ChangeExplicitBlockerCount to MediaStream::Suspend/Resume. r=padenot https://reviewboard.mozilla.org/r/18857/#review16897

Comment on attachment 8659491 [details] MozReview Request: Bug 1189506. Give AudioContext non-owning pointers to all its AudioNodes. r=karl https://reviewboard.mozilla.org/r/18845/#review16973

Comment on attachment 8659492 [details] MozReview Request: Bug 1189506. Put AudioContext::State inline. r=karl https://reviewboard.mozilla.org/r/18847/#review16975

Comment on attachment 8659493 [details] MozReview Request: Bug 1189506. Pass AudioContext to AudioNodeStream::Create. r=karlt https://reviewboard.mozilla.org/r/18849/#review16977

Comment on attachment 8659499 [details] MozReview Request: Bug 1189506. Remove aFlags parameter from AllocateInputPort. r=karlt https://reviewboard.mozilla.org/r/18861/#review16987

Comment on attachment 8659500 [details] MozReview Request: Bug 1189506. Remove MediaInputPort::mFlags. r=karlt https://reviewboard.mozilla.org/r/18863/#review16989

Comment on attachment 8659502 [details] MozReview Request: Bug 1189506. Remove MediaStream::mBlockInThisPhase. r=karlt https://reviewboard.mozilla.org/r/18867/#review16993

Comment on attachment 8659503 [details] MozReview Request: Bug 1189506. Remove mExplicitBlockerCount and related code since it's always zero now. r=karlt https://reviewboard.mozilla.org/r/18869/#review16995

Comment on attachment 8659505 [details] MozReview Request: Bug 1189506. Non-SourceMediaStreams in processing can only block by being finished. r=karlt https://reviewboard.mozilla.org/r/18873/#review16999

Comment on attachment 8659506 [details] MozReview Request: Bug 1189506. Fix multi-track MediaStream audio output. r=karlt https://reviewboard.mozilla.org/r/18875/#review17001

Comment on attachment 8659507 [details] MozReview Request: Bug 1189506. Simplify PlayAudio based on the fact that track time units == stream time units. r=karlt https://reviewboard.mozilla.org/r/18877/#review17003

Comment on attachment 8659508 [details] MozReview Request: Bug 1189506. Remove misleading comment. r=karlt https://reviewboard.mozilla.org/r/18879/#review17123

Comment on attachment 8659509 [details] MozReview Request: Bug 1189506. Remove unused MediaStreamGraph::GetBufferedTicks. r=karlt https://reviewboard.mozilla.org/r/18881/#review17125

Comment on attachment 8659511 [details] MozReview Request: Bug 1189506. Remove no-longer-used TimeVarying.h. r=karlt https://reviewboard.mozilla.org/r/18885/#review17149

Comment on attachment 8659512 [details] MozReview Request: Bug 1189506. Inline RecomputeBlocking. r=karlt https://reviewboard.mozilla.org/r/18887/#review17153

Comment on attachment 8659501 [details] MozReview Request: Bug 1189506. Simplify blocking code now that stream blocking decision are always independent of other streams. r=karlt https://reviewboard.mozilla.org/r/18865/#review16991 ::: dom/media/MediaStreamGraph.cpp (Diff revision 1) > -MediaStreamGraphImpl::AddBlockingRelatedStreamsToSet(nsTArray<MediaStream*>* aStreams, The declaration should also be removed.

Comment on attachment 8659504 [details] MozReview Request: Bug 1189506. Simplify blocking calculations based on the observation that once a stream starts blocking in a given processing interval, it must stay blocked. r=karlt https://reviewboard.mozilla.org/r/18871/#review16997 ::: dom/media/MediaStreamGraph.cpp:403 (Diff revision 1) > MediaStreamGraphImpl::WillUnderrun(MediaStream* aStream, GraphTime aTime, > - GraphTime aEndBlockingDecisions, GraphTime* aEnd) > + GraphTime aEndBlockingDecisions) aTime also is unused now, but removing that need not block these changes.

Comment on attachment 8659510 [details] MozReview Request: Bug 1189506. Replace MediaStream::mBlocked with simpler MediaStream::mStartBlocking. r=karlt https://reviewboard.mozilla.org/r/18883/#review17145 ::: dom/media/MediaStreamGraph.cpp:273 (Diff revision 1) > - return t; > + // XXX we generally shouldn't need to call this for aTime >= mStartBlocking! aTime is StreamTime while mStartBlocking is GraphTime. Explicitly using timeAssumingNoBlocking or mBufferStartTime + aTime would avoid any implication that the two are in the same units.

Comment on attachment 8659513 [details] MozReview Request: Bug 1189506. Inline ComputeStreamBlockTime. r=karlt https://reviewboard.mozilla.org/r/18889/#review17161

Comment on attachment 8659514 [details] MozReview Request: Bug 1189506. Remove unused NotifyConsumptionChanged. r=karlt https://reviewboard.mozilla.org/r/18891/#review17163

Comment on attachment 8659515 [details] MozReview Request: Bug 1189506. Remove unused mFlushSourcesNow/mFlushSourcesOnNextIteration. r=karlt https://reviewboard.mozilla.org/r/18893/#review17165

Comment on attachment 8659516 [details] MozReview Request: Bug 1189506. Factor out code from OneIteration into helper methods. r=karlt https://reviewboard.mozilla.org/r/18895/#review17167

Comment on attachment 8659516 [details] MozReview Request: Bug 1189506. Factor out code from OneIteration into helper methods. r=karlt https://reviewboard.mozilla.org/r/18895/#review17169

Comment on attachment 8659517 [details] MozReview Request: Bug 1189506. Move setting of mStateComputedTime to OneIteration so it's near setting mProcessedTime. r=karlt https://reviewboard.mozilla.org/r/18897/#review17171

Comment on attachment 8659518 [details] MozReview Request: Bug 1189506. No need to pass aNextCurrentTime to UpdateCurrentTimeForStreams. r=karlt https://reviewboard.mozilla.org/r/18899/#review17173

Comment on attachment 8659519 [details] MozReview Request: Bug 1189506. Inline StreamNotifyOutput/StreamNotifyFinished. r=karlt https://reviewboard.mozilla.org/r/18901/#review17175

Comment on attachment 8659520 [details] MozReview Request: Bug 1189506. Rename StreamTimeToGraphTime/GraphTimeToStreamTime to ...WithBlocking. r=karlt https://reviewboard.mozilla.org/r/18903/#review17177

Comment on attachment 8659521 [details] MozReview Request: Bug 1189506. Set mStartBlocking in UpdateCurrentTimeForStreams to indicate that blocking time has been processed. r=karlt https://reviewboard.mozilla.org/r/18905/#review17179

Comment on attachment 8659523 [details] MozReview Request: Bug 1189506. Remove INCLUDE_TRAILING_BLOCKED_INTERVAL from PlayVideo. r=karlt https://reviewboard.mozilla.org/r/18909/#review17183 This is debatable. If the blocking time is on the border of the display periods for 2 frames, then I would expected to see the first frame. But removing code wins here.

Comment on attachment 8659525 [details] MozReview Request: Bug 1189506. Call StreamTimeToGraphTime in DecodedStreamGraphListener::NotifyOutput, since there's no blocking pending there. r=karlt https://reviewboard.mozilla.org/r/18913/#review17185 ::: dom/media/mediasink/DecodedStream.cpp:38 (Diff revision 1) > - mLastOutputTime = mStream->StreamTimeToMicroseconds(mStream->GraphTimeToStreamTimeWithBlocking(aCurrentTime)); > + mLastOutputTime = mStream->StreamTimeToMicroseconds( > + mStream->GraphTimeToStreamTime(aCurrentTime)); > Bug 1189506. Call StreamTimeToGraphTime in DecodedStreamGraphListener::NotifyOutput, since there's no blocking pending there. r=karlt "GraphTimeToStreamTime" in the commit message.

Comment on attachment 8659526 [details] MozReview Request: Bug 1189506. Call GraphTimeToStreamTime in AudioNodeExternalInputStream. r=karlt https://reviewboard.mozilla.org/r/18915/#review17187

Comment on attachment 8659527 [details] MozReview Request: Bug 1189506. Call GraphTimeToStreamTime in ExtractPendingInput since we know no blocking time has been determined yet. r=karlt https://reviewboard.mozilla.org/r/18917/#review17189

Comment on attachment 8659528 [details] MozReview Request: Bug 1189506. Use mProcessedTime in some places instead of passing aFrom. r=karlt https://reviewboard.mozilla.org/r/18919/#review17191 ::: dom/media/MediaStreamGraph.cpp:1123 (Diff revision 1) > - ProduceDataForStreamsBlockByBlock(i, n->SampleRate(), aFrom, aTo); > + ProduceDataForStreamsBlockByBlock(i, n->SampleRate(), > + mProcessedTime, aTo); Convention is usually to align after '(', where that works. ::: dom/media/MediaStreamGraph.cpp:1234 (Diff revision 1) > + GraphTime processedTime = mProcessedTime; > mProcessedTime = stateEnd; > > - UpdateCurrentTimeForStreams(stateFrom); > + UpdateCurrentTimeForStreams(processedTime); Please rename processedTime to something like processedFrom or lastProcessedTime to clearly distinguish from mProcessedTime.

Comment on attachment 8659529 [details] MozReview Request: Bug 1189506. Use mStateComputedTime in some places instead of passing aTo. r=karlt https://reviewboard.mozilla.org/r/18921/#review17193

Comment on attachment 8659530 [details] MozReview Request: Bug 1189506. Use mProcessedTime/mStateComputedTime in ProduceDataForStreamsBlockByBlock. karlt https://reviewboard.mozilla.org/r/18923/#review17195 ::: dom/media/MediaStreamGraphImpl.h:315 (Diff revision 1) > * Produce data for all streams >= aStreamIndex for the given time interval. s/given/current/ or something.

Comment on attachment 8659532 [details] MozReview Request: Bug 1189506. Use GraphTimeToStreamTime in PrepareUpdatesToMainThreadState. r=karlt https://reviewboard.mozilla.org/r/18925/#review17197

Comment on attachment 8659533 [details] MozReview Request: Bug 1189506. Relax assertions a bit. karlt https://reviewboard.mozilla.org/r/18927/#review17199

Comment on attachment 8659524 [details] MozReview Request: Bug 1189506. Call StreamTimeToGraphTime in MediaStreamGraphImpl::UpdateCurrentTimeForStreams, since we know blocking has been taken account of already there. r=karlt https://reviewboard.mozilla.org/r/18911/#review17201

Comment on attachment 8659534 [details] MozReview Request: Bug 1189506. Make PlayVideo use GraphTimeToStreamTime/StreamTimeToGraphTime and remove StreamTimeToGraphTimeWithBlocking. r=karlt https://reviewboard.mozilla.org/r/18929/#review17203

Comment on attachment 8659535 [details] MozReview Request: Bug 1189506. Make PlayAudio use GraphTimeToStreamTime. r=karlt https://reviewboard.mozilla.org/r/18931/#review17205

Comment on attachment 8659539 [details] MozReview Request: Bug 1189506. Remove invalid assertion. r=karlt https://reviewboard.mozilla.org/r/18933/#review17213 ::: dom/media/webaudio/AudioNodeStream.cpp:664 (Diff revision 1) > // Round to nearest > StreamTime ticks = thisSeconds + 0.5; Yes, the assertion is incorrect, but that means that the rounding is incorrect when -ve. That doesn't matter for the Start/Stop methods because all values <= 0 behave similarly, but I suspect it does matter for AudioParams where event times may be < currentTime. Can you make this a warning for now, or otherwise address, please?

Comment on attachment 8659522 [details] MozReview Request: Bug 1189506. Create StreamTimeToGraphTime/GraphTimeToStreamTime that don't take account of blocking, and call them from AudioNodeStream. r=karlt https://reviewboard.mozilla.org/r/18907/#review17181 ::: dom/media/webaudio/AudioNodeStream.h:146 (Diff revision 1) > + * Only call this during MSG message processing. > */ > double FractionalTicksFromDestinationTime(AudioNodeStream* aDestination, > double aSeconds); > /** > * Convert a time in seconds on the destination stream to StreamTime > * on this stream. > + * Only call this during MSG message processing. > */ > StreamTime TicksFromDestinationTime(MediaStream* aDestination, > double aSeconds); > /** > * Get the destination stream time in seconds corresponding to a position on > - * this stream. > + * this stream. Only call this when the stream is not blocked before > + * aPosition. These seem more restricted than necessary and "not blocked before aPosition" may be misleading. It is fine if the stream has previously been blocked, but is now not blocked. The methods should not be used when in the processing stage and there is some blocking in this iteration before aPosition, but AudioNodeStreams are either blocked for the whole of OneIteration() or none of it. It is safe to call these methods either when outside the processing stage (including NotifyEvent(FINISHED)/NotifyOutput/NotifyBlockingChanged/) or when processing and not currently blocked. The likelihood of wanting to call these methods during processing while blocked seems slim given that most processing is skipped while blocked. Can we just depend on the assertions in StreamTimeToGraphTime/GraphTimeToStreamTime?

(In reply to Karl Tomlinson (ni?:karlt) from comment #156) > It is safe to call these methods either when outside the processing stage > (including NotifyEvent(FINISHED)/NotifyOutput/NotifyBlockingChanged/) or > when processing and not currently blocked. > > The likelihood of wanting to call these methods during processing while > blocked seems slim given that most processing is skipped while blocked. > > Can we just depend on the assertions in > StreamTimeToGraphTime/GraphTimeToStreamTime? Yes. I'll do that.

https://reviewboard.mozilla.org/r/18851/#review16893 > s/is/are/ done

https://reviewboard.mozilla.org/r/18865/#review16991 > The declaration should also be removed. done

https://reviewboard.mozilla.org/r/18883/#review17145 > aTime is StreamTime while mStartBlocking is GraphTime. Explicitly using timeAssumingNoBlocking or mBufferStartTime + aTime would avoid any implication that the two are in the same units. This code gets removed later in the patch queue anyway.

https://reviewboard.mozilla.org/r/18919/#review17191 > Convention is usually to align after '(', where that works. This gets fixed in a later patch anyway.

https://reviewboard.mozilla.org/r/18933/#review17213 > Yes, the assertion is incorrect, but that means that the rounding is incorrect when -ve. > > That doesn't matter for the Start/Stop methods because all values <= 0 behave similarly, but I suspect it does matter for AudioParams where event times may be < currentTime. > > Can you make this a warning for now, or otherwise address, please? Fixed by calling NS_round() instead.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #162) > Fixed by calling NS_round() instead. FWIW NS_round(), like round(), breaks ties by rounding away from zero. There is nothing special about 0 on this timeline apart from the floating point precision, so X = floor(x + 0.5) is more consistent. However, what gets rounded to XXX.5 depends on the magnitude of the value because of floating point precision, so, regardless of what we do, we can't provide the property that x1 and x2 = x1 + d where d is integer always maps to X1 and X2 such that X2 = X1 + d. NS_round() is much better than what we have, thanks. It's so much easier to review individual conceptual changes as posted here, so thanks so much for doing that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3353f2d77d21 https://hg.mozilla.org/integration/mozilla-inbound/rev/d01a693feb4b https://hg.mozilla.org/integration/mozilla-inbound/rev/49306680e27b https://hg.mozilla.org/integration/mozilla-inbound/rev/82ac981a8b0d https://hg.mozilla.org/integration/mozilla-inbound/rev/3a81598dc929 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d75cd4d9ea https://hg.mozilla.org/integration/mozilla-inbound/rev/2624ea64e7ab https://hg.mozilla.org/integration/mozilla-inbound/rev/90258efcf37f https://hg.mozilla.org/integration/mozilla-inbound/rev/d6361921977c https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e44fd15c5d https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3e38b1d426 https://hg.mozilla.org/integration/mozilla-inbound/rev/23f79deaebee https://hg.mozilla.org/integration/mozilla-inbound/rev/45bbd595ac18 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6d421cfc32 https://hg.mozilla.org/integration/mozilla-inbound/rev/63e927ca87fb https://hg.mozilla.org/integration/mozilla-inbound/rev/386e8ef862dc https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1060db7ddb https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd399259045 https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd6b03d2d39 https://hg.mozilla.org/integration/mozilla-inbound/rev/309eaacef687 https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd85ebb56de https://hg.mozilla.org/integration/mozilla-inbound/rev/3311210c41e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/b791ed14fbe4 https://hg.mozilla.org/integration/mozilla-inbound/rev/72f03839930a https://hg.mozilla.org/integration/mozilla-inbound/rev/a2fa219fe521 https://hg.mozilla.org/integration/mozilla-inbound/rev/71ba3cb1406e https://hg.mozilla.org/integration/mozilla-inbound/rev/f38cc960f179 https://hg.mozilla.org/integration/mozilla-inbound/rev/e09858e37a65 https://hg.mozilla.org/integration/mozilla-inbound/rev/67cadfca5bbd https://hg.mozilla.org/integration/mozilla-inbound/rev/09b5e7e7eced https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3d3c66fab5 https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d2999a8773 https://hg.mozilla.org/integration/mozilla-inbound/rev/04c37cfa2299 https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd584f42eee https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbbc7a0c7d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb138eeb4a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7a41713e1d https://hg.mozilla.org/integration/mozilla-inbound/rev/aecf80382905 https://hg.mozilla.org/integration/mozilla-inbound/rev/07ddd70da2c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d36f0caeae https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0b08207de9 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4bfb4017c00 https://hg.mozilla.org/integration/mozilla-inbound/rev/133b52029897 https://hg.mozilla.org/integration/mozilla-inbound/rev/724eb2edb328 https://hg.mozilla.org/integration/mozilla-inbound/rev/052e47bcd8ac

https://hg.mozilla.org/mozilla-central/rev/3353f2d77d21 https://hg.mozilla.org/mozilla-central/rev/d01a693feb4b https://hg.mozilla.org/mozilla-central/rev/49306680e27b https://hg.mozilla.org/mozilla-central/rev/82ac981a8b0d https://hg.mozilla.org/mozilla-central/rev/3a81598dc929 https://hg.mozilla.org/mozilla-central/rev/c8d75cd4d9ea https://hg.mozilla.org/mozilla-central/rev/2624ea64e7ab https://hg.mozilla.org/mozilla-central/rev/90258efcf37f https://hg.mozilla.org/mozilla-central/rev/d6361921977c https://hg.mozilla.org/mozilla-central/rev/e3e44fd15c5d https://hg.mozilla.org/mozilla-central/rev/9a3e38b1d426 https://hg.mozilla.org/mozilla-central/rev/23f79deaebee https://hg.mozilla.org/mozilla-central/rev/45bbd595ac18 https://hg.mozilla.org/mozilla-central/rev/ed6d421cfc32 https://hg.mozilla.org/mozilla-central/rev/63e927ca87fb https://hg.mozilla.org/mozilla-central/rev/386e8ef862dc https://hg.mozilla.org/mozilla-central/rev/3d1060db7ddb https://hg.mozilla.org/mozilla-central/rev/2bd399259045 https://hg.mozilla.org/mozilla-central/rev/7bd6b03d2d39 https://hg.mozilla.org/mozilla-central/rev/309eaacef687 https://hg.mozilla.org/mozilla-central/rev/7fd85ebb56de https://hg.mozilla.org/mozilla-central/rev/3311210c41e5 https://hg.mozilla.org/mozilla-central/rev/b791ed14fbe4 https://hg.mozilla.org/mozilla-central/rev/72f03839930a https://hg.mozilla.org/mozilla-central/rev/a2fa219fe521 https://hg.mozilla.org/mozilla-central/rev/71ba3cb1406e https://hg.mozilla.org/mozilla-central/rev/f38cc960f179 https://hg.mozilla.org/mozilla-central/rev/e09858e37a65 https://hg.mozilla.org/mozilla-central/rev/67cadfca5bbd https://hg.mozilla.org/mozilla-central/rev/09b5e7e7eced https://hg.mozilla.org/mozilla-central/rev/1e3d3c66fab5 https://hg.mozilla.org/mozilla-central/rev/c9d2999a8773 https://hg.mozilla.org/mozilla-central/rev/04c37cfa2299 https://hg.mozilla.org/mozilla-central/rev/1cd584f42eee https://hg.mozilla.org/mozilla-central/rev/fbbbc7a0c7d6 https://hg.mozilla.org/mozilla-central/rev/0bb138eeb4a3 https://hg.mozilla.org/mozilla-central/rev/fe7a41713e1d https://hg.mozilla.org/mozilla-central/rev/aecf80382905 https://hg.mozilla.org/mozilla-central/rev/07ddd70da2c9 https://hg.mozilla.org/mozilla-central/rev/c1d36f0caeae https://hg.mozilla.org/mozilla-central/rev/1c0b08207de9 https://hg.mozilla.org/mozilla-central/rev/a4bfb4017c00 https://hg.mozilla.org/mozilla-central/rev/133b52029897 https://hg.mozilla.org/mozilla-central/rev/724eb2edb328 https://hg.mozilla.org/mozilla-central/rev/052e47bcd8ac