Closed Bug 1189506 Opened 9 years ago Closed 9 years ago

Remove blocking from MSG

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: mreavy, Assigned: roc)

References

(Depends on 1 open bug)

Details

Attachments

(45 files)

MozReview Request: Bug 1189506. Give AudioContext non-owning pointers to all its AudioNodes. r=karl
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Put AudioContext::State inline. r=karl
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Pass AudioContext to AudioNodeStream::Create. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Make AudioContext responsible for tracking all nodes which need to be suspended and resumed. r=padenot
40 bytes, text/x-review-board-request
padenot
: review+
Details
MozReview Request: Bug 1189506. Make suspending/resuming streams more reusable. r=padenot
40 bytes, text/x-review-board-request
padenot
: review+
Details
MozReview Request: Bug 1189506. Don't bother blocking captured media-element MediaStreams while we're not decoding. r=jwwang
40 bytes, text/x-review-board-request
jwwang
: review+
Details
MozReview Request: Bug 1189506. Convert ChangeExplicitBlockerCount to MediaStream::Suspend/Resume. r=padenot
40 bytes, text/x-review-board-request
padenot
: review+
Details
MozReview Request: Bug 1189506. Remove usage of FLAG_BLOCK_OUTPUT from MediaManager. r=jesup
40 bytes, text/x-review-board-request
jesup
: review+
Details
MozReview Request: Bug 1189506. Remove aFlags parameter from AllocateInputPort. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove MediaInputPort::mFlags. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Simplify blocking code now that stream blocking decision are always independent of other streams. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove MediaStream::mBlockInThisPhase. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove mExplicitBlockerCount and related code since it's always zero now. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
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
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Non-SourceMediaStreams in processing can only block by being finished. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Fix multi-track MediaStream audio output. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Simplify PlayAudio based on the fact that track time units == stream time units. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove misleading comment. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove unused MediaStreamGraph::GetBufferedTicks. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Replace MediaStream::mBlocked with simpler MediaStream::mStartBlocking. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove no-longer-used TimeVarying.h. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Inline RecomputeBlocking. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Inline ComputeStreamBlockTime. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove unused NotifyConsumptionChanged. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove unused mFlushSourcesNow/mFlushSourcesOnNextIteration. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Factor out code from OneIteration into helper methods. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Move setting of mStateComputedTime to OneIteration so it's near setting mProcessedTime. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. No need to pass aNextCurrentTime to UpdateCurrentTimeForStreams. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Inline StreamNotifyOutput/StreamNotifyFinished. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Rename StreamTimeToGraphTime/GraphTimeToStreamTime to ...WithBlocking. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Set mStartBlocking in UpdateCurrentTimeForStreams to indicate that blocking time has been processed. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Create StreamTimeToGraphTime/GraphTimeToStreamTime that don't take account of blocking, and call them from AudioNodeStream. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove INCLUDE_TRAILING_BLOCKED_INTERVAL from PlayVideo. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Call StreamTimeToGraphTime in MediaStreamGraphImpl::UpdateCurrentTimeForStreams, since we know blocking has been taken account of already there. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Call StreamTimeToGraphTime in DecodedStreamGraphListener::NotifyOutput, since there's no blocking pending there. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Call GraphTimeToStreamTime in AudioNodeExternalInputStream. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Call GraphTimeToStreamTime in ExtractPendingInput since we know no blocking time has been determined yet. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Use mProcessedTime in some places instead of passing aFrom. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Use mStateComputedTime in some places instead of passing aTo. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Use mProcessedTime/mStateComputedTime in ProduceDataForStreamsBlockByBlock. karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Use GraphTimeToStreamTime in PrepareUpdatesToMainThreadState. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Relax assertions a bit. karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Make PlayVideo use GraphTimeToStreamTime/StreamTimeToGraphTime and remove StreamTimeToGraphTimeWithBlocking. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Make PlayAudio use GraphTimeToStreamTime. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details
MozReview Request: Bug 1189506. Remove invalid assertion. r=karlt
40 bytes, text/x-review-board-request
karlt
: review+
Details 
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
Rank: 10
Priority: -- → P1
Blocks: webaudioperf_parity
No longer blocks: webaudioperf
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!
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
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?
Flags: needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
(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?
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(rjesup)
Assignee: nobody → roc
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?
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.
Flags: needinfo?(padenot)
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.
Flags: needinfo?(padenot)
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. 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.
Attachment #8659494 - Flags: review?(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.
Attachment #8659496 - Flags: review?(jwwang)
Bug 1189506. Convert ChangeExplicitBlockerCount to MediaStream::Suspend/Resume. r=padenot
Attachment #8659497 - Flags: review?(padenot)
Bug 1189506. Simplify blocking code now that stream blocking decision are always independent of other streams. r=karlt
Attachment #8659501 - Flags: review?(karlt)
Bug 1189506. Remove mExplicitBlockerCount and related code since it's always zero now. r=karlt
Attachment #8659503 - Flags: review?(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
Attachment #8659504 - Flags: review?(karlt)
Bug 1189506. Non-SourceMediaStreams in processing can only block by being finished. r=karlt
Attachment #8659505 - Flags: review?(karlt)
Bug 1189506. Simplify PlayAudio based on the fact that track time units == stream time units. r=karlt
Attachment #8659507 - Flags: review?(karlt)
Bug 1189506. Replace MediaStream::mBlocked with simpler MediaStream::mStartBlocking. r=karlt
Attachment #8659510 - Flags: review?(karlt)
Bug 1189506. Move setting of mStateComputedTime to OneIteration so it's near setting mProcessedTime. r=karlt
Attachment #8659517 - Flags: review?(karlt)
Bug 1189506. No need to pass aNextCurrentTime to UpdateCurrentTimeForStreams. r=karlt
Attachment #8659518 - Flags: review?(karlt)
Bug 1189506. Rename StreamTimeToGraphTime/GraphTimeToStreamTime to ...WithBlocking. r=karlt
Attachment #8659520 - Flags: review?(karlt)
Bug 1189506. Set mStartBlocking in UpdateCurrentTimeForStreams to indicate that blocking time has been processed. r=karlt
Attachment #8659521 - Flags: review?(karlt)
Bug 1189506. Create StreamTimeToGraphTime/GraphTimeToStreamTime that don't take account of blocking, and call them from AudioNodeStream. r=karlt
Attachment #8659522 - Flags: review?(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.
Attachment #8659523 - Flags: review?(karlt)
Bug 1189506. Call StreamTimeToGraphTime in MediaStreamGraphImpl::UpdateCurrentTimeForStreams, since we know blocking has been taken account of already there. r=karlt
Attachment #8659524 - Flags: review?(karlt)
Bug 1189506. Call StreamTimeToGraphTime in DecodedStreamGraphListener::NotifyOutput, since there's no blocking pending there. r=karlt
Attachment #8659525 - Flags: review?(karlt)
Bug 1189506. Call GraphTimeToStreamTime in ExtractPendingInput since we know no blocking time has been determined yet. r=karlt
Attachment #8659527 - Flags: review?(karlt)
Bug 1189506. Use mStateComputedTime in some places instead of passing aTo. r=karlt
Attachment #8659529 - Flags: review?(karlt)
Bug 1189506. Use mProcessedTime/mStateComputedTime in ProduceDataForStreamsBlockByBlock. karlt
Attachment #8659530 - Flags: review?(karlt)
Bug 1189506. Use GraphTimeToStreamTime in PrepareUpdatesToMainThreadState. r=karlt
Attachment #8659532 - Flags: review?(karlt)
Bug 1189506. Make PlayVideo use GraphTimeToStreamTime/StreamTimeToGraphTime and remove StreamTimeToGraphTimeWithBlocking. r=karlt
Attachment #8659534 - Flags: review?(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.
Attachment #8659539 - Flags: review?(karlt)
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
Attachment #8659496 - Flags: review?(jwwang) → review+
Attachment #8659498 - Flags: review?(rjesup) → review+
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
Attachment #8659494 - Flags: review?(padenot) → review+
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/
Attachment #8659495 - Flags: review?(padenot) → review+
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
Attachment #8659497 - Flags: review?(padenot) → review+
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
Attachment #8659491 - Flags: review?(karlt) → review+
Comment on attachment 8659492 [details]
MozReview Request: Bug 1189506. Put AudioContext::State inline. r=karl

https://reviewboard.mozilla.org/r/18847/#review16975
Attachment #8659492 - Flags: review?(karlt) → review+
Comment on attachment 8659493 [details]
MozReview Request: Bug 1189506. Pass AudioContext to AudioNodeStream::Create. r=karlt

https://reviewboard.mozilla.org/r/18849/#review16977
Attachment #8659493 - Flags: review?(karlt) → review+
Attachment #8659499 - Flags: review?(karlt) → review+
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
Attachment #8659500 - Flags: review?(karlt) → review+
Comment on attachment 8659502 [details]
MozReview Request: Bug 1189506. Remove MediaStream::mBlockInThisPhase. r=karlt

https://reviewboard.mozilla.org/r/18867/#review16993
Attachment #8659502 - Flags: review?(karlt) → review+
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
Attachment #8659503 - Flags: review?(karlt) → review+
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
Attachment #8659505 - Flags: review?(karlt) → review+
Comment on attachment 8659506 [details]
MozReview Request: Bug 1189506. Fix multi-track MediaStream audio output. r=karlt

https://reviewboard.mozilla.org/r/18875/#review17001
Attachment #8659506 - Flags: review?(karlt) → review+
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
Attachment #8659507 - Flags: review?(karlt) → review+
Comment on attachment 8659508 [details]
MozReview Request: Bug 1189506. Remove misleading comment. r=karlt

https://reviewboard.mozilla.org/r/18879/#review17123
Attachment #8659508 - Flags: review?(karlt) → review+
Comment on attachment 8659509 [details]
MozReview Request: Bug 1189506. Remove unused MediaStreamGraph::GetBufferedTicks. r=karlt

https://reviewboard.mozilla.org/r/18881/#review17125
Attachment #8659509 - Flags: review?(karlt) → review+
Comment on attachment 8659511 [details]
MozReview Request: Bug 1189506. Remove no-longer-used TimeVarying.h. r=karlt

https://reviewboard.mozilla.org/r/18885/#review17149
Attachment #8659511 - Flags: review?(karlt) → review+
Comment on attachment 8659512 [details]
MozReview Request: Bug 1189506. Inline RecomputeBlocking. r=karlt

https://reviewboard.mozilla.org/r/18887/#review17153
Attachment #8659512 - Flags: review?(karlt) → review+
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.
Attachment #8659501 - Flags: review?(karlt) → review+
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.
Attachment #8659504 - Flags: review?(karlt) → review+
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.
Attachment #8659510 - Flags: review?(karlt) → review+
Comment on attachment 8659513 [details]
MozReview Request: Bug 1189506. Inline ComputeStreamBlockTime. r=karlt

https://reviewboard.mozilla.org/r/18889/#review17161
Attachment #8659513 - Flags: review?(karlt) → review+
Attachment #8659514 - Flags: review?(karlt) → review+
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
Attachment #8659515 - Flags: review?(karlt) → review+
Attachment #8659516 - Flags: review?(karlt)
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
Attachment #8659516 - Flags: review+
Attachment #8659517 - Flags: review?(karlt) → review+
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
Attachment #8659518 - Flags: review?(karlt) → review+
Attachment #8659519 - Flags: review?(karlt) → review+
Comment on attachment 8659519 [details]
MozReview Request: Bug 1189506. Inline StreamNotifyOutput/StreamNotifyFinished. r=karlt

https://reviewboard.mozilla.org/r/18901/#review17175
Attachment #8659520 - Flags: review?(karlt) → review+
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
Attachment #8659521 - Flags: review?(karlt) → review+
Attachment #8659523 - Flags: review?(karlt) → review+
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.
Attachment #8659525 - Flags: review?(karlt) → review+
Attachment #8659526 - Flags: review?(karlt) → review+
Comment on attachment 8659526 [details]
MozReview Request: Bug 1189506. Call GraphTimeToStreamTime in AudioNodeExternalInputStream. r=karlt

https://reviewboard.mozilla.org/r/18915/#review17187
Attachment #8659527 - Flags: review?(karlt) → review+
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.
Attachment #8659528 - Flags: review?(karlt) → review+
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
Attachment #8659529 - Flags: review?(karlt) → review+
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.
Attachment #8659530 - Flags: review?(karlt) → review+
Comment on attachment 8659532 [details]
MozReview Request: Bug 1189506. Use GraphTimeToStreamTime in PrepareUpdatesToMainThreadState. r=karlt

https://reviewboard.mozilla.org/r/18925/#review17197
Attachment #8659532 - Flags: review?(karlt) → review+
Attachment #8659533 - Flags: review?(karlt) → review+
Attachment #8659524 - Flags: review?(karlt) → review+
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
Attachment #8659534 - Flags: review?(karlt) → review+
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
Attachment #8659535 - Flags: review?(karlt) → review+
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?
Attachment #8659539 - Flags: review?(karlt) → review+
Attachment #8659522 - Flags: review?(karlt) → review+
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/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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1198386
Depends on: 1209524
Depends on: 1216417
Depends on: 1223670
No longer depends on: 1223670
Depends on: 1257407
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: