Refactor DOMMediaStream to accommodate Clone(), AddTrack(), RemoveTrack(), etc.

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

(Blocks 1 bug)

41 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox44 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

40 bytes, text/x-review-board-request
pehrsons
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
pehrsons
: review+
Details
40 bytes, text/x-review-board-request
pehrsons
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
pehrsons
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
Assignee

Description

4 years ago
Today's DOMMediaStream architecture makes it difficult to do certain operations. Consider for instance:

> navigator.getUserMedia({audio:true, video: true}).then(stream => {
>   var t = stream.getTracks()[0];
>   stream.removeTrack(t);
> }

How do you remove a track from a stream when the track is native to that stream? Note that it may still exist in other streams.


> navigator.getUserMedia({video: true}).then(stream => {
>   var t = stream.getTracks()[0];
>   var t2 = t.clone();
>   t.stop();
> }

How do you clone a track so that it keeps playing after having stopped the original? Let's say that the DOMMediaStream is backed by a SourceMediaStream directly.
Assignee

Comment 1

4 years ago
What I propose doing is to let DOMMediaStream contain a 3-staged stream chain with the following streams:
1. mInputStream
     Stream of the type indicated by DOMMediaStream::CreateSourceStream/CreateTrackUnionStream.
     A producer can create its DOM stream, then get the input stream and feed it with data.
2. mOwnedStream
     TrackUnionStream containing tracks owned by this stream.
     MediaStreamTracks reference tracks in this stream.
3. mPlaybackStream
     TrackUnionStream containing tracks carried by this stream (returned by getTracks()).
     addTrack(t) can be set up simply by connecting t's owning stream to this playback stream with
     a MediaInputPort locked to t's TrackID.

This also implies that we can lock a MediaInputPort to a certain input track id. Only that track should then be forwarded to the consumer. Forwarding all tracks might still be OK, e.g., when not routing through a DOMMediaStream, so we should support both.

This can hopefully also lead to simplification in other places, where we for instance no longer need to create one SourceMediaStream and have that routed to several TrackUnionStream-backed DOMMediaStreams (as is the case with MediaManager, HTMLMediaElement::CaptureStream, etc.)

In an ideal world DOMMediaStream::CreateSourceStream should be enough (we can use Clone(), AddTrack() to cover connected cases when we have them), but perhaps for WebAudio we'll still need DOMMediaStream::CreateTrackUnionStream when piping from an internal stream to the DOM stream.

It hopefully also helps to shrink the API surface for stream producers as they no longer have to allocate and manage MediaInputPorts directly. A nice side effect!

=====

For a simple graphical overview:

addTrack()ed case:

DOMStream A
          Input        Owned          Playback
           t1 ---------> t1 ------------> t1     <- MediaStreamTrack X (pointing to DOMStream A, track 1)
                             \
                              ----
DOMStream B                        \
          Input        Owned        \ Playback
                                     ---> t1     <- MediaStreamTrack X (pointing to DOMStream A, track 1)

-----

removeTrack()ed case:


DOMStream A
          Input        Owned          Playback
           t1 ---------> t1                      <- No tracks

-----

clone()d case:


DOMStream A
          Input        Owned          Playback
           t1 ---------> t1 ------------> t1     <- MediaStreamTrack X (pointing to DOMStream A, track 1)
              \
               ----
DOMStream B         \
          Input      \ Owned          Playback
                      -> t1 ------------> t1     <- MediaStreamTrack Y (pointing to DOMStream B, track 1)

=====

I have some patches with this architecture mostly working (short of HTMLMediaElement::CaptureStream, and perhaps something I recently broke in WebSpeech) just so I could see that it would follow through. I'd like some feedback on the basic idea though, so roc, jesup, padenot, any thoughts you have are welcome. If there's someone else you think should take a look, please forward an ni? Thanks!

I'll clean up my patches and then post them on rb in case you want to see the nitty-grits.
Flags: needinfo?(roc)
Flags: needinfo?(rjesup)
Flags: needinfo?(padenot)
Assignee

Comment 2

4 years ago
Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges to allow for tracking the input. r=
Assignee

Comment 3

4 years ago
Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r=
Assignee

Comment 4

4 years ago
Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r=
Assignee

Comment 5

4 years ago
Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r=
Assignee

Comment 6

4 years ago
Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r=
Assignee

Comment 7

4 years ago
Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r=
Assignee

Comment 8

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges to allow for tracking the input. r=
Assignee

Comment 9

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r=
Assignee

Comment 10

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r=
Assignee

Comment 11

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r=
Assignee

Comment 12

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r=
Assignee

Comment 13

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r=

Comment 14

4 years ago
Could anyone update when this could be made available
Assignee

Updated

4 years ago
Depends on: 1172394
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #1)
> What I propose doing is to let DOMMediaStream contain a 3-staged stream
> chain with the following streams:
> 1. mInputStream
>      Stream of the type indicated by
> DOMMediaStream::CreateSourceStream/CreateTrackUnionStream.
>      A producer can create its DOM stream, then get the input stream and
> feed it with data.
> 2. mOwnedStream
>      TrackUnionStream containing tracks owned by this stream.
>      MediaStreamTracks reference tracks in this stream.
> 3. mPlaybackStream
>      TrackUnionStream containing tracks carried by this stream (returned by
> getTracks()).
>      addTrack(t) can be set up simply by connecting t's owning stream to
> this playback stream with
>      a MediaInputPort locked to t's TrackID.

I don't understand the need for mOwnedStream. Can you explain when we'd need a MediaStreamTrack to refer to a track in mOwnedStream that's not available in mInputStream? Is it because the streams in mInputStream don't block but the ones in mOwnedStream might?
Flags: needinfo?(roc)
Assignee

Comment 16

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #1)
> > What I propose doing is to let DOMMediaStream contain a 3-staged stream
> > chain with the following streams:
> > 1. mInputStream
> >      Stream of the type indicated by
> > DOMMediaStream::CreateSourceStream/CreateTrackUnionStream.
> >      A producer can create its DOM stream, then get the input stream and
> > feed it with data.
> > 2. mOwnedStream
> >      TrackUnionStream containing tracks owned by this stream.
> >      MediaStreamTracks reference tracks in this stream.
> > 3. mPlaybackStream
> >      TrackUnionStream containing tracks carried by this stream (returned by
> > getTracks()).
> >      addTrack(t) can be set up simply by connecting t's owning stream to
> > this playback stream with
> >      a MediaInputPort locked to t's TrackID.
> 
> I don't understand the need for mOwnedStream. Can you explain when we'd need
> a MediaStreamTrack to refer to a track in mOwnedStream that's not available
> in mInputStream? Is it because the streams in mInputStream don't block but
> the ones in mOwnedStream might?

If a track is available in mOwnedStream it should always be available in mInputStream under the same TrackID.


Consider a mixed addTrack() (Stream B) and clone() (Stream C) case without mOwnedStream:

DOMStream A
          Input        Playback
           t1 ----------> t1   <- MediaStreamTrack X (Stream A, track 1)
              \
               \-----
DOMStream B     \    \
          Input  \    \ Playback
                  \    -> t1   <- MediaStreamTrack X (Stream A, track 1)
                   \
                    \
DOMStream C          \
          Input       \ Playback
                       -> t1   <- MediaStreamTrack Y (Stream A, track 1)


When you call `x.stop();`, track X shall end in streams A and B whereas track Y shall still be live in stream C.

I found it most convenient to let track X in this case be represented by a single MediaInputPort in its owning stream. To stop() X we only have to remove this input port and the rest will happen automatically. I like the separation of concerns we get here.

It should however be possible to store a list of MediaInputPorts for track X, probably on X's owning stream (A in this case). When someone calls stop() on X, we can remove all these ports and be at the same stage as in the case with mOwnedStream. This means to removeTrack() X from stream B, we need to go through A, which I find a bit surprising.

So essentially,
Case 1, (with mOwnedStream): Each DOMMediaStream owns all MediaInputPorts to its owned and playback streams.
Case 2, (without mOwnedStream): Each DOMMediaStream owns all MediaInputPorts from its input stream to some playback stream.

In case 2, as a bonus we can probably get ride of the TrackPort class I created in Part 3.

I don't really have an opinion on which I prefer. Since I wrote the code for case 1, I'm probably biased anyway. What do people think?
Flags: needinfo?(roc)
I get it. I didn't realize the clone case would work like that. Your original design sounds fine.
Flags: needinfo?(roc)
I agree with roc.
Flags: needinfo?(rjesup)
Assignee

Updated

4 years ago
Blocks: 1056652
Yeah same. Now we need to implement all the infrastructure to make this work.
Flags: needinfo?(padenot)
Assignee

Comment 20

4 years ago
jib, jesup, I noticed your discussion on MediaStreams, clone() and applyConstraints() on IRC earlier today. If you look at comment 1 and comment 16, what are your thoughts regarding applyConstraints() and my proposed way of handling clone()?

I basically want to provide a generic way of handling cloning of tracks and streams. Completely contained in DOMMediaStream, thus kept separate from the source.

Should we need support from the source for constraints on (both original and cloned) tracks we could get rid of the first stream ("input") of the three I proposed in DOMMediaStream (see comment 16) and let the source provide a new, separate stream.
Downside: all sources need explicit support for cloning.

We could also hook streams up with their source on top of my proposed three-staged stream chain (per comment 1), just to allow passing constraints back to the source. Then we could let the source adapt it's stream to accommodate all the clones' constraints - which will then be applied at the sinks instead.
Downside: Complex, maybe?
Upside: clone() + applyConstraints() wouldn't mean a performance hit on its own, only after being given to a sink. Think lazy constraints. Sinks could optimize for the constraints to avoid making raw copies of video frames for instance, or fall back to some shared adapter that will transform the video according to the constraints.


Just some thoughts. You two probably know a lot more about applyConstraints() than me, so it'd be good to get some input on that before I get back to work on this bug.
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
https://reviewboard.mozilla.org/r/10161/#review15527

(This is not a review, I just found it easier to code-comment in mozReview).

Thanks for the drawings, as they help me tremendously! I think we have a huge terminology problem here, as every time I read "stream" or "track" I have to guess whether it's a DOM object or a graph object being discussed, and if I guess wrong then I may think I agree with you when I don't or vice versa (not to mention the comprehension problem for someone reading this cold).

Before even commenting on applyConstraints, let me catch up on the model. I see a couple of problems I think. First:

::: dom/media/DOMMediaStream.h:81
(Diff revision 2)
> + *                     addTrack()ed case:
> + * DOMStream A
> + *           Input        Owned          Playback
> + *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack X
> + *                                                     (pointing to t1 in A)
> + *                                 --------> t2     <- MediaStreamTrack Y
> + *                                /                    (pointing to t1 in B)
> + * DOMStream B                   /
> + *           Input        Owned /        Playback
> + *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack Y
> + *                                                     (pointing to t1 in B)

If A has track X and I do do A.addTrack(Y) ...

::: dom/media/DOMMediaStream.h:99
(Diff revision 2)
> + *                     clone()d case:
> + * DOMStream A
> + *           Input        Owned          Playback
> + *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack X
> + *               \                                     (pointing to t1 in A)
> + *                -----
> + * DOMStream B         \
> + *           Input      \ Owned          Playback
> + *                       -> t1 ------------> t1     <- MediaStreamTrack Y
> + *                                                     (pointing to t1 in B)

... and then I do: var C = A.clone(),

then I would expect C to contain both X and Y.

However, when I try to follow your model, I end up with the following, which seems wrong, because t2 is missing from C (basically I've smashed your two drawings together):

 * DOMStream C          
 *           Input        Owned          Playback
 *                       -> t1 ------------> t1     <- MediaStreamTrack Z
 *                      /           (where's t2?!)     (pointing to t1 in C)
 *                     /
 *                -----
 * DOMStream A   /
 *           Input        Owned          Playback
 *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack X
 *                                                     (pointing to t1 in A)
 *                                 --------> t2     <- MediaStreamTrack Y
 *                                /                    (pointing to t1 in B)
 * DOMStream B                   /
 *           Input        Owned /        Playback
 *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack Y
 *                                                     (pointing to t1 in B)

Even if there's a fix for that, I dislike the "asymmetry based on origin-story" here. As I mentioned on IRC, I think the add/remove/clone transitions are a red herring when reviewing the model itself. Taking a step back, if for a single video source V, I have track A with mute and a 10 fps constraint, and track B and C with a 30 fps constraint, it shouldn't matter how tracks A, B and C were conceived, whether they were cloned, moved or came from different gUM requests. I worry in your model this is not true, and that would be a problem I think.

It would be a problem, because the DOM objects are light consumer abstractions, representing needs (settings) that are either identical (which we must optimize for), or different, but we must assume that content can and will shuffle these around until the structure is not recognizable anymore. If we have some dependency on how things came about, then that will almost certainly break. It's also needless complexity that I don't see buys us anything.

It seems clear to me that these objects must be totally divorced from the underlying things (that are also confusingly called streams and tracks). To use a lame analogy, they're mere instructions for the old phone operator for how the wires on the old operator board should be hooked up. I THINK your model largely does this already by its use of loose couplings, but the prose confuses me. If that's all there is, me being confused, then I think we need to divorce the two terminologies here.

Lastly, I'm not sure any of this belongs in DOMStream anymore. The reason I say that is that if I look at recent spec changes in both Media Capture and WebRTC, basically all the meat has moved out of MediaStream and into MediaStreamTrack, to the point where MediaStream is actually stateless (it seems to me it could actually be implemented as a label that also doubles as its id).

I'm not sure where I would move this to, most likely the MediaStreamTrack objects themselves?
Flags: needinfo?(jib)
See Also: → 1083354
Assignee

Comment 22

4 years ago
(In reply to Jan-Ivar Bruaroey [:jib] from comment #21)
> https://reviewboard.mozilla.org/r/10161/#review15527
> 
> (This is not a review, I just found it easier to code-comment in mozReview).
> 
> Thanks for the drawings, as they help me tremendously! I think we have a
> huge terminology problem here, as every time I read "stream" or "track" I
> have to guess whether it's a DOM object or a graph object being discussed,
> and if I guess wrong then I may think I agree with you when I don't or vice
> versa (not to mention the comprehension problem for someone reading this
> cold).
> 
> Before even commenting on applyConstraints, let me catch up on the model. I
> see a couple of problems I think. First:
> 
> ::: dom/media/DOMMediaStream.h:81
> (Diff revision 2)
> > + *                     addTrack()ed case:
> > + * DOMStream A
> > + *           Input        Owned          Playback
> > + *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack X
> > + *                                                     (pointing to t1 in A)
> > + *                                 --------> t2     <- MediaStreamTrack Y
> > + *                                /                    (pointing to t1 in B)
> > + * DOMStream B                   /
> > + *           Input        Owned /        Playback
> > + *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack Y
> > + *                                                     (pointing to t1 in B)
> 
> If A has track X and I do do A.addTrack(Y) ...
> 
> ::: dom/media/DOMMediaStream.h:99
> (Diff revision 2)
> > + *                     clone()d case:
> > + * DOMStream A
> > + *           Input        Owned          Playback
> > + *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack X
> > + *               \                                     (pointing to t1 in A)
> > + *                -----
> > + * DOMStream B         \
> > + *           Input      \ Owned          Playback
> > + *                       -> t1 ------------> t1     <- MediaStreamTrack Y
> > + *                                                     (pointing to t1 in B)
> 
> ... and then I do: var C = A.clone(),
> 
> then I would expect C to contain both X and Y.
> 
> However, when I try to follow your model, I end up with the following, which
> seems wrong, because t2 is missing from C (basically I've smashed your two
> drawings together):

Here's what it'd look like (I changed Z to X' and created Y' because I ran out of letters):
 * DOMStream C          
 *           Input        Owned          Playback
 *                   -----> t1 ------------> t1     <- MediaStreamTrack X'
 *                  /                                  (pointing to t1 in C)
 *                 /     -> t2 ------------> t2     <- MediaStreamTrack Y'
 *                /     /                              (pointing to t2 in C)
 * DOMStream A   /     /
 *           Input    /   Owned          Playback
 *            t1 ----^----> t1 ------------> t1     <- MediaStreamTrack X
 *                  /                                  (pointing to t1 in A)
 *                 /               --------> t2     <- MediaStreamTrack Y
 *                /               /                    (pointing to t1 in B)
 * DOMStream B   /               /
 *           Input        Owned /        Playback
 *            t1 ---------> t1 ------------> t1     <- MediaStreamTrack Y
 *                                                     (pointing to t1 in B)

> Even if there's a fix for that, I dislike the "asymmetry based on
> origin-story" here. As I mentioned on IRC, I think the add/remove/clone
> transitions are a red herring when reviewing the model itself. Taking a step
> back, if for a single video source V, I have track A with mute and a 10 fps
> constraint, and track B and C with a 30 fps constraint, it shouldn't matter
> how tracks A, B and C were conceived, whether they were cloned, moved or
> came from different gUM requests. I worry in your model this is not true,
> and that would be a problem I think.

It depends on how we implement constraints. That's what I wanted to get to in comment 20.
E.g., for your example; mute (enabled = false?) is already handled on the track itself so it shouldn't be a problem, but getting track A from gUM at 10 FPS, cloning that to track B and constraining it to 30 FPS, how should that work? Are we constrained by whatever came out of gUM already, or should we tell the source that we need 30 FPS and constrain A to 10 FPS? Both are doable.

If we get 30 FPS from gUM to track A, clone A to B and apply a 10 FPS constraint to B, then clone B to C and apply a 20 FPS constraint to C, it wouldn't be a problem because, from my model above:
* Cloning happens at the internal "Input" stream (at A for both clones B and C).
* Constraints would happen on the internal "Owned" stream (because that's what's tied to the DOM MediaStreamTrack instances)
* I.e., A, B and C all have access to the raw internal stream from gUM as "Input" on A, then A constrains it to 30FPS at "Owned", B to 10 FPS at "Owned" and C to 20 FPS at "Owned".

After writing the above I did look a bit at the spec for applyConstraints() and it seems like constraints are always applied at the source (e.g., the hardware behind gUM, or as close to it as possible). That would be fairly easy to fix by letting DOMStreams know about their source through some common interface class - and we can throw out some sub classes of DOMMediaStream at the same time, like DOMLocalMediaStream and nsDOMUserMediaStream.

> It would be a problem, because the DOM objects are light consumer
> abstractions, representing needs (settings) that are either identical (which
> we must optimize for), or different, but we must assume that content can and
> will shuffle these around until the structure is not recognizable anymore.
> If we have some dependency on how things came about, then that will almost
> certainly break. It's also needless complexity that I don't see buys us
> anything.
> 
> It seems clear to me that these objects must be totally divorced from the
> underlying things (that are also confusingly called streams and tracks). To
> use a lame analogy, they're mere instructions for the old phone operator for
> how the wires on the old operator board should be hooked up. I THINK your
> model largely does this already by its use of loose couplings, but the prose
> confuses me. If that's all there is, me being confused, then I think we need
> to divorce the two terminologies here.

I agree that the terminology is confusing, but it's not a new problem.

> Lastly, I'm not sure any of this belongs in DOMStream anymore. The reason I
> say that is that if I look at recent spec changes in both Media Capture and
> WebRTC, basically all the meat has moved out of MediaStream and into
> MediaStreamTrack, to the point where MediaStream is actually stateless (it
> seems to me it could actually be implemented as a label that also doubles as
> its id).
> 
> I'm not sure where I would move this to, most likely the MediaStreamTrack
> objects themselves?

DOMStreams are modeled after our internal MediaStreams, as in: a boiled-down MediaStream is a StreamBuffer object containing a set of tracks (identified by TrackID).

A DOMStream has a reference to the internal MediaStream and its collection of MediaStreamTracks have a reference to their parent DOMStream and the underlying TrackID. Putting the logic in MediaStreamTrack instead would cause the DOM model and the internal model to get out of sync, making things even more confusing.

I'm sort of getting us half the way there by locking MediaInputPorts to specific TrackIDs, but I don't think we should refactor our internal streams into tracks right now. We have so many bits depending on them. We can perhaps do something as part of padenot's throwing away buffering from MediaStreamGraph, I think those go more hand in hand.


So, did I make it any clearer?
Flags: needinfo?(jib)
Assignee

Comment 23

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc
This allows for tracking the input track of an added track (for
ProcessedMediaStream tracks; SourceMediaStream tracks don't have input
tracks) directly in the NotifyQueuedTrackChanges handler, which will be
necessary for locking MediaInputPorts to specific tracks.
Attachment #8615070 - Attachment description: MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges to allow for tracking the input. r= → MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc
Attachment #8615070 - Flags: review?(roc)
Assignee

Comment 24

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Attachment #8615071 - Attachment description: MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r= → MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Attachment #8615071 - Flags: review?(roc)
Assignee

Comment 25

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Attachment #8615072 - Attachment description: MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r= → MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
Attachment #8615072 - Flags: review?(roc)
Assignee

Comment 26

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r?roc
This cleans up the excessive stream chain we'd get if the playback
stream was still a DOMMediaStream (1 instead of 3 extra streams) plus
gets rid of the overhead of an extra DOMMediaStream that we don't need.
Attachment #8615073 - Attachment description: MozReview Request: Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r= → MozReview Request: Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r?roc
Attachment #8615073 - Flags: review?(roc)
Assignee

Comment 27

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Attachment #8615074 - Attachment description: MozReview Request: Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r= → MozReview Request: Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Attachment #8615074 - Flags: review?(rjesup)
Assignee

Comment 28

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r?roc
Attachment #8615075 - Attachment description: MozReview Request: Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r= → MozReview Request: Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r?roc
Attachment #8615075 - Flags: review?(roc)
Assignee

Comment 29

4 years ago
Bug 1170958 - Part 7. Add DOMMediaStream::InputStreamListener. r?roc
A DOMMediaStream's input stream is either a ProcessedMediaStream (like
for media element capture) or a SourceMediaStream (like for gUM). When
producers like these create new tracks after the stream has already been
created (the initial set should be available to JS synchronously), it is
nice if the DOMMediaStream picks them up automatically and create the
corresponding MediaStreamTracks.

The InputStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Attachment #8655253 - Flags: review?(roc)
Assignee

Comment 30

4 years ago
Bug 1170958 - Part 8. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Attachment #8655354 - Flags: review?(roc)
Attachment #8655354 - Flags: review?(rjesup)
After re-reading the specs it seems to me that input/output is still rooted in streams, and I've filed issues on WebRTC on the pieces that confused me into thinking otherwise. e.g. https://github.com/w3c/webrtc-pc/issues/288

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #22)
> Here's what it'd look like

Thanks for the explanation. I think I understand now how this will work, even though the asymmetry still seems odd to me. e.g. I move the sole track from A into an empty B I created:

 * DOMStream A
 *           Input        Owned          Playback
 *            t1 ---------> t1
 *                            \
 *                             \
 *                              \
 * DOMStream B                   \
 *           Input        Owned   \      Playback
 *                                 --------> t1     <- MediaStreamTrack X
 *                                                     (pointing to t1 in A)

Why even keep DOMStream A (the container) around anymore? If there are historical reasons for that then fine. If not, might reference counting here sufficiently take care of ownership?

                                 it1
                                 /
                          ot1 <--
 * DOMStream B           /
 *           Playback   /
 *              t1 <----                            <- MediaStreamTrack X
 *                                                     (ref-pointing to ot1 on heap)

> E.g., for your example; mute (enabled = false?) is already handled on the
> track itself so it shouldn't be a problem, but getting track A from gUM at
> 10 FPS, cloning that to track B and constraining it to 30 FPS, how should
> that work? Are we constrained by whatever came out of gUM already, or

No we're not. That is, we're only constrained to the camera chosen. Any mode of that camera is fair game, including any modes we "make up" that it has (as long as we never "make up pixels", e.g. halving fps is ok, doubling is not).

> should we tell the source that we need 30 FPS and constrain A to 10 FPS?

Yes I think we should. Note that we must disclose such "new modes" in MediaCapabilities.

> After writing the above I did look a bit at the spec for applyConstraints()
> and it seems like constraints are always applied at the source (e.g., the
> hardware behind gUM, or as close to it as possible). That would be fairly
> easy to fix by letting DOMStreams know about their source through some
> common interface class - and we can throw out some sub classes of
> DOMMediaStream at the same time, like DOMLocalMediaStream and
> nsDOMUserMediaStream.

Good!

> Putting the logic in MediaStreamTrack instead would cause the DOM model and
> the internal model to get out of sync, making things even more confusing.

How are they in sync now?

> I'm sort of getting us half the way there by locking MediaInputPorts to
> specific TrackIDs, but I don't think we should refactor our internal streams
> into tracks right now. We have so many bits depending on them. We can
> perhaps do something as part of padenot's throwing away buffering from
> MediaStreamGraph, I think those go more hand in hand.
> 
> 
> So, did I make it any clearer?

Yes, thanks.
Flags: needinfo?(jib)
Assignee

Comment 32

4 years ago
(In reply to Jan-Ivar Bruaroey [:jib] from comment #31)
> After re-reading the specs it seems to me that input/output is still rooted
> in streams, and I've filed issues on WebRTC on the pieces that confused me
> into thinking otherwise. e.g. https://github.com/w3c/webrtc-pc/issues/288
> 
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #22)
> > Here's what it'd look like
> 
> Thanks for the explanation. I think I understand now how this will work,
> even though the asymmetry still seems odd to me. e.g. I move the sole track
> from A into an empty B I created:
> 
>  * DOMStream A
>  *           Input        Owned          Playback
>  *            t1 ---------> t1
>  *                            \
>  *                             \
>  *                              \
>  * DOMStream B                   \
>  *           Input        Owned   \      Playback
>  *                                 --------> t1     <- MediaStreamTrack X
>  *                                                     (pointing to t1 in A)
> 
> Why even keep DOMStream A (the container) around anymore? If there are
> historical reasons for that then fine. If not, might reference counting here
> sufficiently take care of ownership?

DOMStream A 'owns' track X, so e.g., X.stop() will result in A.stopTrack(X's internal TrackID). The TrackID that X carries is only valid in A; when added to B it may get assigned another value if its original ID was already assigned to another track.


>                                  it1
>                                  /
>                           ot1 <--
>  * DOMStream B           /
>  *           Playback   /
>  *              t1 <----                            <- MediaStreamTrack X
>  *                                                     (ref-pointing to ot1
> on heap)

The problem here is that there's no ot1 (Owned Track?) object on the heap. There's a MediaStream (the MSG-side class) and a TrackID (int32_t). The MediaStream is referenced from a DOMStream, which in turn is referenced from MediaStreamTracks.


> > Putting the logic in MediaStreamTrack instead would cause the DOM model and
> > the internal model to get out of sync, making things even more confusing.
> 
> How are they in sync now?

DOMMediaStream owns a MediaStream (stream <-> stream)
MediaStreamTrack owns a TrackID and refers to a DOMMediaStream (track <-> track)

Shifting logic to MediaStreamTrack we'd get:
DOMMediaStream owns nothing in the underlying layer
MediaStreamTrack owns (references?) a MediaStream (and a TrackID?) (track <-> stream) - some question marks in there since there are multiple ways to do it.
Assignee

Comment 33

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc
This allows for tracking the input track of an added track (for
ProcessedMediaStream tracks; SourceMediaStream tracks don't have input
tracks) directly in the NotifyQueuedTrackChanges handler, which will be
necessary for locking MediaInputPorts to specific tracks.
Assignee

Comment 34

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Assignee

Comment 35

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Assignee

Comment 36

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r?roc
This cleans up the excessive stream chain we'd get if the playback
stream was still a DOMMediaStream (1 instead of 3 extra streams) plus
gets rid of the overhead of an extra DOMMediaStream that we don't need.
Assignee

Comment 37

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Assignee

Comment 38

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r?roc
Assignee

Comment 39

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Add DOMMediaStream::InputStreamListener. r?roc
A DOMMediaStream's input stream is either a ProcessedMediaStream (like
for media element capture) or a SourceMediaStream (like for gUM). When
producers like these create new tracks after the stream has already been
created (the initial set should be available to JS synchronously), it is
nice if the DOMMediaStream picks them up automatically and create the
corresponding MediaStreamTracks.

The InputStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Assignee

Comment 40

4 years ago
Comment on attachment 8655354 [details]
MozReview Request: Bug 1170958 - Part 8. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 8. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Assignee

Comment 41

4 years ago
That spam was caused by a fresh rebase and a couple of test failure fixes. It should function quite well now.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20cbf0ca27cb
Assignee

Comment 42

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Assignee

Comment 43

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r?roc
This cleans up the excessive stream chain we'd get if the playback
stream was still a DOMMediaStream (1 instead of 3 extra streams) plus
gets rid of the overhead of an extra DOMMediaStream that we don't need.
Assignee

Comment 44

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Assignee

Comment 45

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r?roc
Assignee

Comment 46

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Add DOMMediaStream::InputStreamListener. r?roc
A DOMMediaStream's input stream is either a ProcessedMediaStream (like
for media element capture) or a SourceMediaStream (like for gUM). When
producers like these create new tracks after the stream has already been
created (the initial set should be available to JS synchronously), it is
nice if the DOMMediaStream picks them up automatically and create the
corresponding MediaStreamTracks.

The InputStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Assignee

Comment 47

4 years ago
Comment on attachment 8655354 [details]
MozReview Request: Bug 1170958 - Part 8. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 8. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Assignee

Comment 48

4 years ago
Did a fix for CameraPreviewMediaStream that should solve the oranges on B2G.
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe1bad07229
And one for some missing mochitests on Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c2a4d52643d
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

https://reviewboard.mozilla.org/r/10163/#review16179

I just landed a change to eliminate mPlaybackStream so you'll probably just want to remove this.
Attachment #8615073 - Flags: review?(roc)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #32)
> DOMStream A 'owns' track X, so e.g., X.stop() will result in A.stopTrack(X's
> internal TrackID). The TrackID that X carries is only valid in A; when added
> to B it may get assigned another value if its original ID was already
> assigned to another track.

Right, these would be the historical reasons. There is nothing in the spec (anymore) that requires this setup AFAIK, e.g. no exposed stream.stopTrack() API, external track id's are globally unique [1], and lifespan ownership is loosely coupled (e.g. add/remove/refcounting).

> >                                  it1
> >                                  /
> >                           ot1 <--
> >  * DOMStream B           /
> >  *           Playback   /
> >  *              t1 <----                            <- MediaStreamTrack X
> >  *                                                     (ref-pointing to ot1
> > on heap)
> 
> The problem here is that there's no ot1 (Owned Track?) object on the heap.
> There's a MediaStream (the MSG-side class) and a TrackID (int32_t). The
> MediaStream is referenced from a DOMStream, which in turn is referenced from
> MediaStreamTracks.

Right so ot1 == MediaStream + TrackID in today's internal setup. I'm merely challenging design-wise why the original create-time DOMStream needs to proxy as owner for these objects through a list anymore.

> > > Putting the logic in MediaStreamTrack instead would cause the DOM model and
> > > the internal model to get out of sync, making things even more confusing.
> > 
> > How are they in sync now?
> 
> DOMMediaStream owns a MediaStream (stream <-> stream)
> MediaStreamTrack owns a TrackID and refers to a DOMMediaStream (track <->
> track)
> 
> Shifting logic to MediaStreamTrack we'd get:
> DOMMediaStream owns nothing in the underlying layer
> MediaStreamTrack owns (references?) a MediaStream (and a TrackID?) (track
> <-> stream) - some question marks in there since there are multiple ways to
> do it.

I misunderstood, I thought by "DOM model" you meant the exposed API. To be least confusing, I think our model should be in sync with the spec, which by my understanding is:

  MediaStream API references ref-counted MediaStreamTrack objects
  MediaStreamTrack API owns/is internal track (be it MediaStream + TrackID, or ?)

That said, I'm sure there are lots of internal reasons for where things are today, and I think what you have here should work, and is a good step. It is confusing to read however.

[1] http://jsfiddle.net/m8fv5bLt
Hmm... After we remove stream blocking in bug 1189506 (another couple of weeks?), I think we could fix this bug by simply having one MediaStream per track. And then (obviously) clean up by getting rid of the stream/track distinction at the MSG level. Would that work? Is it worth going through this intermediate state?
Flags: needinfo?(pehrsons)
Actually MSG MediaStreams containing multiple tracks do have an important function: they let us add and remove tracks off the main thread. So scrap that idea (though after fixing bug 1189506 we can simplify MediaStream a lot, and maybe at some point it would make sense to have MSG Track objects that are peers of MediaStreams and can be belong to multiple MediaStreams).
Flags: needinfo?(pehrsons)
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

https://reviewboard.mozilla.org/r/10157/#review16245
Attachment #8615070 - Flags: review?(roc) → review+
Assignee

Comment 54

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> Actually MSG MediaStreams containing multiple tracks do have an important
> function: they let us add and remove tracks off the main thread. So scrap
> that idea (though after fixing bug 1189506 we can simplify MediaStream a
> lot, and maybe at some point it would make sense to have MSG Track objects
> that are peers of MediaStreams and can be belong to multiple MediaStreams).

I was thinking of something like one MSG-MediaStream per track as a future goal involving quite a lot of work (i.e., yes it would work) - probably with issues we didn't consider (like below :-) ). At the same time, addTrack/removeTrack is a pretty highly requested API that would be nice to get without waiting for MSG to be cleaned up.

There's some work in landing this of course, but I don't think it will be more work throwing this out than throwing what we had before out when later cleaning up.


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> Actually MSG MediaStreams containing multiple tracks do have an important
> function: they let us add and remove tracks off the main thread.

Ah right, though the only real user of that now afaik is media element capture. I.e., when seeking, restarting, etc. the media element.


> So scrap
> that idea (though after fixing bug 1189506 we can simplify MediaStream a
> lot, and maybe at some point it would make sense to have MSG Track objects
> that are peers of MediaStreams and can be belong to multiple MediaStreams).

It's not clear to me how much we'll be able to do, but sounds good! :-)
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

https://reviewboard.mozilla.org/r/10159/#review16247

::: dom/media/DOMMediaStream.cpp:311
(Diff revision 4)
> -  InitStreamCommon(aGraph->CreateAudioCaptureStream(this));
> +  TrackID audioCaptureTrackId = 1;

Use AUDIO_TRACK instead of 1

::: dom/media/DecodedStream.cpp:255
(Diff revision 4)
> -  mPort = mStream->AllocateInputPort(aStream,
> +  mPort = mStream->AllocateInputPort(aStream, TRACK_INVALID,

Clearer to introduce TRACK_ANY and use it here and elsewhere.

::: dom/media/MediaRecorder.cpp:538
(Diff revision 4)
> +                                                      TRACK_INVALID,

Use TRACK_ANY here. When you rebase this you'll find that the flags parameter is always 0, so we can make TRACK_ANY the default parameter value and omit it in most places.

::: dom/media/TrackUnionStream.cpp:95
(Diff revision 4)
> +          // This input port was locked to another track. Ignore.

Add a method to MediaInputPort to encapsulate this check, e.g. MediaInputPort::PassTrackThrough().
Attachment #8615071 - Flags: review?(roc) → review+
> Ah right, though the only real user of that now afaik is media element capture. I.e., when seeking, restarting, etc. the
> media element.

I think even now a track in a media resource can end at any time, and that will update in the MediaStream asynchronously. Of course that's not too important since a track ending early can be treated just like an underrun and it doesn't matter if the track is removed late.

I guess the question is whether we will need to support dynamic track addition (from media resources, gUM, and WebRTC) off the main thread in the future. I suspect we will, but I'm uncertain.

   * aTrackID can be set to TRACK_INVALID to forward all tracks from aStream,
   * though this is discouraged as it will prevent future removal of those
   * tracks from a DOMMediaStream.

So this is worrying, because we need to use TRACK_INVALID/TRACK_ALL to get off-main-thread dynamic track addition and removal.

So I guess the hard case is going to be:
1) We're playing a media resource (or gUM, or WebRTC) and capturing it to a DOM MediaStream
2) JS enumerates the DOM tracks, and removes one.
3) A new track appears in the source.
4) The new track needs to appear in the DOM MediaStream

The current case that I think can already happen:
1) We're playing a media resource (or gUM, or WebRTC) and capturing it to a DOM MediaStream
2) JS enumerates the DOM tracks, and removes one.
3) A different track ends in the source.
4) The ended track needs to be removed from the DOM MediaStream.

How can we handle those scenarios in your model? I guess you could even write a test for the second case.
Flags: needinfo?(pehrsons)
Assignee

Comment 57

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #56)
> So I guess the hard case is going to be:
> 1) We're playing a media resource (or gUM, or WebRTC) and capturing it to a
> DOM MediaStream
> 2) JS enumerates the DOM tracks, and removes one.
> 3) A new track appears in the source.
> 4) The new track needs to appear in the DOM MediaStream

The DOM stream owned by gUM or WebRTC has a listener on its input stream that will create a MediaStreamTrack and the appropriate MediaInputPorts to forward it to the playback stream. So in such cases it takes a dispatch to main thread and again back to the MSG thread to get media data to start flowing. See part 7.

Hmm, what about more complex cases; Should a stream cloned from a media element capture also be getting dynamically added/removed tracks? I think that would be easy enough to fix.


> The current case that I think can already happen:
> 1) We're playing a media resource (or gUM, or WebRTC) and capturing it to a
> DOM MediaStream
> 2) JS enumerates the DOM tracks, and removes one.
> 3) A different track ends in the source.
> 4) The ended track needs to be removed from the DOM MediaStream.

This is fine. JS removed track X from the playback stream. It can map track Y ending to the appropriate MediaStreamTrack and remove that. That said I don't see any existing code for actually removing the track..? It's possible anyhow; see part 3.


> How can we handle those scenarios in your model? I guess you could even
> write a test for the second case.

I'll note that on the addTrack/removeTrack bug; bug 1103188.
Flags: needinfo?(pehrsons)
Assignee

Comment 58

4 years ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #57)
> So in such cases it takes a dispatch to
> main thread and again back to the MSG thread to get media data to start
> flowing.

This we can probably fix. I'll give it a shot.
Yeah, it'd be nice to avoid. We can make the MediaInputPort::PassTrackThrough gate more complicated, e.g. allow it to pass through all but a specific set of tracks.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #55)
> Use TRACK_ANY here. When you rebase this you'll find that the flags
> parameter is always 0, so we can make TRACK_ANY the default parameter value
> and omit it in most places.

Actually those patches haven't landed yet. I'll get them landed.
Assignee

Comment 61

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> Actually those patches haven't landed yet. I'll get them landed.

Whoa, am I getting more bitrot? ;-)

Which bug are they part of?
Assignee

Comment 62

4 years ago
https://reviewboard.mozilla.org/r/10159/#review16247

> Use AUDIO_TRACK instead of 1

I did this to avoid having a track id hardcoded in a header. And defining AUDIO_TRACK in DOMMediaStream.cpp leaks it to all the other sub classes which I don't want.

It's only needed for creating the track so why have it as an enum or a #define?
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

https://reviewboard.mozilla.org/r/10165/#review16287

::: dom/media/MediaManager.cpp
(Diff revision 5)
> -                 reinterpret_cast<int64_t>(trackunion->GetInputStream()));

Is the logging here no longer needed?  We haven't played with the latency stuff recently, but it should still work (though it's tricky to get graphs out of).  It was used to let us correlate items in the logs so we could track and graph the latency through the pipeline
Attachment #8615074 - Flags: review?(rjesup) → review+
Comment on attachment 8655354 [details]
MozReview Request: Bug 1170958 - Part 8. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

https://reviewboard.mozilla.org/r/17911/#review16289
Attachment #8655354 - Flags: review?(rjesup) → review+
Assignee

Comment 65

4 years ago
https://reviewboard.mozilla.org/r/10165/#review16287

> Is the logging here no longer needed?  We haven't played with the latency stuff recently, but it should still work (though it's tricky to get graphs out of).  It was used to let us correlate items in the logs so we could track and graph the latency through the pipeline

I don't know much about the LatencyLogger. I removed it since we now only have one stream (`domStream`) here, so the relationship between `stream` and `trackunion` is gone.

Is the latency logger meant to be hooked up between all MSG MediaStreams? If so I can add a patch that adds it wherever there's wiring going on in DOMMediaStream.cpp.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #65)
> I don't know much about the LatencyLogger. I removed it since we now only
> have one stream (`domStream`) here, so the relationship between `stream` and
> `trackunion` is gone.

So 'correlation' log event is no longer needed, then.

> Is the latency logger meant to be hooked up between all MSG MediaStreams? If
> so I can add a patch that adds it wherever there's wiring going on in
> DOMMediaStream.cpp.

The main idea is that you can trace data from capture, to MSG, into PeerConnection, out of PeerConnection, through AudioStream and cubeb, and add up the latencies as samples pass through the pipelines.  There's a python tool (most recent version not checked in) that generates semi-pretty graphs of all this.
Flags: needinfo?(rjesup)
Assignee

Comment 67

4 years ago
(In reply to Randell Jesup [:jesup] from comment #66)
> So 'correlation' log event is no longer needed, then.

There's still a chain of streams wired up inside a DOMMediaStream though, so it might be needed there?

And I suspect the LatencyLogger might not suppoert a case where we lock an input port to a single track and pipe that to another stream. Should we invent something for that? Who owns the LatencyLogger?
Flags: needinfo?(rjesup)
Assignee

Comment 68

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc
This allows for tracking the input track of an added track (for
ProcessedMediaStream tracks; SourceMediaStream tracks don't have input
tracks) directly in the NotifyQueuedTrackChanges handler, which will be
necessary for locking MediaInputPorts to specific tracks.
Attachment #8615070 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8615071 - Flags: review+ → review?(roc)
Assignee

Comment 69

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Assignee

Comment 70

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Assignee

Comment 71

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Attachment #8615073 - Attachment description: MozReview Request: Bug 1170958 - Part 4. Use raw TrackUnionStream instead of DOMMediaStream as playback stream in media element. r?roc → MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Assignee

Comment 72

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Attachment #8615074 - Attachment description: MozReview Request: Bug 1170958 - Part 5. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup → MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Attachment #8615074 - Flags: review+ → review?(roc)
Assignee

Updated

4 years ago
Attachment #8615075 - Attachment description: MozReview Request: Bug 1170958 - Part 6. Improve logging of MediaStreams and playback. r?roc → MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
Assignee

Comment 73

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
A DOMMediaStream's owned stream is piped from the input stream which is
under the control of the DOMMediaStream's owner/producer (like
captureStream or gUM). When producers like these create new tracks after
the stream has already been created (the initial set should be available
to JS synchronously), it is nice if the DOMMediaStream picks them up
automatically and create the corresponding MediaStreamTracks.

The OwnedStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Assignee

Comment 74

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Attachment #8655253 - Attachment description: MozReview Request: Bug 1170958 - Part 7. Add DOMMediaStream::InputStreamListener. r?roc → MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
Assignee

Updated

4 years ago
Attachment #8655354 - Attachment is obsolete: true
Attachment #8655354 - Flags: review?(roc)
Assignee

Comment 75

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Carries r=roc (comment 53).
Attachment #8615070 - Flags: review+
Assignee

Comment 76

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Carries r=jesup (comment 63).

Comment 63 seems confused with which patch it applies to because I later removed an earlier commit..
Attachment #8615073 - Flags: review+
Assignee

Comment 77

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Carries r=jesup (comment 64).
Attachment #8655253 - Flags: review+
https://reviewboard.mozilla.org/r/10159/#review16247

> I did this to avoid having a track id hardcoded in a header. And defining AUDIO_TRACK in DOMMediaStream.cpp leaks it to all the other sub classes which I don't want.
> 
> It's only needed for creating the track so why have it as an enum or a #define?

It can be local if you like. It's just easier to read an identifier than "1", which leaves the reader wondering what that means.
Assignee

Comment 80

4 years ago
https://reviewboard.mozilla.org/r/10159/#review16247

> It can be local if you like. It's just easier to read an identifier than "1", which leaves the reader wondering what that means.

`audioCaptureTrackId` ?

I mean, it needs to be set to 1 somewhere, so why not as close as possible?
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #80)
> https://reviewboard.mozilla.org/r/10159/#review16247
> 
> > It can be local if you like. It's just easier to read an identifier than "1", which leaves the reader wondering what that means.
> 
> `audioCaptureTrackId` ?
> 
> I mean, it needs to be set to 1 somewhere, so why not as close as possible?

I agree with roc - naked constants in calls can be confusing, especially if there are hidden correlations/restrictions on them.  #define, or class-local const value, etc all will do.
Flags: needinfo?(rjesup)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #67)
> (In reply to Randell Jesup [:jesup] from comment #66)
> > So 'correlation' log event is no longer needed, then.
> 
> There's still a chain of streams wired up inside a DOMMediaStream though, so
> it might be needed there?

Quite likely.

> And I suspect the LatencyLogger might not suppoert a case where we lock an
> input port to a single track and pipe that to another stream. Should we
> invent something for that? Who owns the LatencyLogger?

The latencylogger only really is meant to support a subset of cases; though in theory it could be expanded to do more.  It is designed to capture this with little added latency/overhead.  As we're driving latency and buffers out of MSG it becomes less critical; it was useful when we had capture delay, MSG buffering/self-clocking, and then AudioStream buffered, and there was cubeb/driver delay, and we didn't have MSG bypass.

Feel free to add new LatencyLogger enum entries for new correlations; then the data is there for the processing script.  I'll try to dig the old one out for reference.  (ugly hacked-up python; generates PDFs with graphs)
Assignee

Comment 83

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc
This allows for tracking the input track of an added track (for
ProcessedMediaStream tracks; SourceMediaStream tracks don't have input
tracks) directly in the NotifyQueuedTrackChanges handler, which will be
necessary for locking MediaInputPorts to specific tracks.
Attachment #8615070 - Flags: review+
Assignee

Comment 84

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Assignee

Comment 85

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Assignee

Comment 86

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Attachment #8615073 - Flags: review+
Assignee

Comment 87

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Assignee

Comment 88

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
A DOMMediaStream's owned stream is piped from the input stream which is
under the control of the DOMMediaStream's owner/producer (like
captureStream or gUM). When producers like these create new tracks after
the stream has already been created (the initial set should be available
to JS synchronously), it is nice if the DOMMediaStream picks them up
automatically and create the corresponding MediaStreamTracks.

The OwnedStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Assignee

Comment 89

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Attachment #8655253 - Flags: review+
Assignee

Comment 90

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Carries r=roc
Attachment #8615070 - Flags: review+
Assignee

Comment 91

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Carries r=jesup
Attachment #8615073 - Flags: review+
Assignee

Comment 92

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Carries r=jesup
Attachment #8655253 - Flags: review+
Assignee

Comment 93

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Assignee

Comment 94

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Assignee

Updated

4 years ago
Attachment #8615073 - Flags: review+
Assignee

Comment 95

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Assignee

Comment 96

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Assignee

Comment 97

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
A DOMMediaStream's owned stream is piped from the input stream which is
under the control of the DOMMediaStream's owner/producer (like
captureStream or gUM). When producers like these create new tracks after
the stream has already been created (the initial set should be available
to JS synchronously), it is nice if the DOMMediaStream picks them up
automatically and create the corresponding MediaStreamTracks.

The OwnedStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Assignee

Comment 98

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Attachment #8655253 - Flags: review+
Assignee

Comment 99

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Fixed a bug in part 2 where tracks blocked in the input port weren't detected ended.

Carries r=jesup.
Attachment #8615073 - Flags: review+
Assignee

Comment 100

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Carries r=jesup.
Attachment #8655253 - Flags: review+
Assignee

Comment 101

4 years ago
Latest try push shows us as blocked by bug 1072313 and bug 1196689: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4ffe2565274. Same story for bug 1161913.
Depends on: 1072313, 1196689
Flags: needinfo?(mreavy)
(In reply to Randell Jesup [:jesup] from comment #81)
> I agree with roc - naked constants in calls can be confusing, especially if
> there are hidden correlations/restrictions on them.  #define, or class-local
> const value, etc all will do.

Even a function-local const would do.
Assignee

Comment 103

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #102)
> Even a function-local const would do.

Good, because that's what I put in there :-)
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

https://reviewboard.mozilla.org/r/10159/#review16467

::: dom/media/MediaStreamGraph.h:1050
(Diff revision 7)
> +    mBlockedTracks.AppendElement(aTrackId);

I think it would be better to use the control API message passing mechanism so we can guarantee that changes to mBlockedTracks can be synchronized with other operations on the MSG from the main thread. Then we also don't need mMutex.
Attachment #8615071 - Flags: review?(roc)
Assignee

Comment 105

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Attachment #8615071 - Flags: review?(roc)
Assignee

Comment 106

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Assignee

Comment 107

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Attachment #8615073 - Flags: review+
Assignee

Comment 108

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Assignee

Comment 109

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
A DOMMediaStream's owned stream is piped from the input stream which is
under the control of the DOMMediaStream's owner/producer (like
captureStream or gUM). When producers like these create new tracks after
the stream has already been created (the initial set should be available
to JS synchronously), it is nice if the DOMMediaStream picks them up
automatically and create the corresponding MediaStreamTracks.

The OwnedStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Assignee

Comment 110

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Attachment #8655253 - Flags: review+
Assignee

Comment 111

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Carries r=jesup.
Attachment #8615073 - Flags: review+
Assignee

Comment 112

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Carries r=jesup.
Attachment #8655253 - Flags: review+
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

https://reviewboard.mozilla.org/r/10161/#review16469

::: dom/media/DOMMediaStream.h:78
(Diff revision 8)
> + *        A MediaStreamTrack reference a track by TrackID in this stream.

I'm not sure how to parse this line.

::: dom/media/DOMMediaStream.h:104
(Diff revision 8)
> + *            t1 ---------> t1                      <- No tracks

You need to explain somewhere here the need for "Owned". I always have a hard time getting my head around it.

One thing I don't understand with this model: suppose we take the addTrack() case above and then clone DOMStream A. How does that work? If we just clone A's Owned stream then we don't get a clone of track Y.

Similarly, if we follow the removeTrack() case and then clone DOMStream A, how do we ensure the cloned stream has no tracks?

I'm actually starting to feel that the alternative approach where we don't have "Owned" might be better, sorry :-(. I'll discuss more in the bug.

::: dom/media/DOMMediaStream.h:178
(Diff revision 8)
> +  MediaStreamTrack* FindCarriedDOMTrack(MediaStream* aOwningStream, TrackID aTrackID);

I think it would be simpler to use the term "Playback" here instead of "Carried", since they mean the same thing.

::: dom/media/DOMMediaStream.h:312
(Diff revision 8)
> +   * is no atomic registration of input ports to a stream.

I do think we should support dynamic off-main-thread track creation and destruction. Apart from anything else, it would be nice to avoid input MediaStream sources having to create DOM tracks when they add tracks. This is tied into the architecture issue.
Attachment #8615072 - Flags: review?(roc)
Reading the spec again, it seems like the spec doesn't really contemplate gUM/PeerConnection/<video> dynamically adding or removing tracks. In particular, a track's ending doesn't remove it from the MediaStream track set, AFAICT. Martin, do you know if the media-capture TF is intentionally steering away from host-driven dynamic track changes? Maybe someone should bring this up?
Flags: needinfo?(martin.thomson)
Assignee

Comment 115

4 years ago
https://reviewboard.mozilla.org/r/10161/#review16469

> I'm not sure how to parse this line.

Should be something like "Each MediaStreamTrack references a DOMMediaStream and a TrackID that is valid in that DOMMediaStream's Owned stream". I'll fix that.

> You need to explain somewhere here the need for "Owned". I always have a hard time getting my head around it.
> 
> One thing I don't understand with this model: suppose we take the addTrack() case above and then clone DOMStream A. How does that work? If we just clone A's Owned stream then we don't get a clone of track Y.
> 
> Similarly, if we follow the removeTrack() case and then clone DOMStream A, how do we ensure the cloned stream has no tracks?
> 
> I'm actually starting to feel that the alternative approach where we don't have "Owned" might be better, sorry :-(. I'll discuss more in the bug.

DOMStream A has a list of MediaStreamTracks it currently contains (the ones that are in its playback stream). These can be owned by A or someone else.

When cloning A we follow the spec and iterate over the MediaStreamTracks that A contains ('carries' per my current patch), for each track we create an input port (locked to the track's TrackID) from the track's owning DOMStream's mInputStream (where the track's TrackID is valid) to the new clone's mOwnedStream.

If we're cloning a clone we can trace down the original input MSG-stream it came from through the port connecting the original input stream with the first clone's owned stream.

It works the same way for the case with a removed track. We iterate over the MediaStreamTracks the DOMStream currently carries, instead of the tracks in any of its MSG streams.

> I do think we should support dynamic off-main-thread track creation and destruction. Apart from anything else, it would be nice to avoid input MediaStream sources having to create DOM tracks when they add tracks. This is tied into the architecture issue.

Right, I have fixed that (you also need part 7 for it to fully work). Probably just forgot to update the comments here.
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
An update on the two intermittents blocking this: 
* Edwin is actively working bug 1196689 (it's just taking longer than he would like).  He is close though. 
* I talked with k17e (Anthony) today, and he realizes bug 1072313 needs to get fixed;  he plans to put jya on it this week.  
* jya is usually fast so I'm hopeful we can have both issues resolved in the next couple of weeks.  
* I also plan to offer help to Edwin on 1196689 since it's a GMP issue and my team has some knowledge there.
Flags: needinfo?(mreavy)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #114)
> Reading the spec again, it seems like the spec doesn't really contemplate
> gUM/PeerConnection/<video> dynamically adding or removing tracks. In
> particular, a track's ending doesn't remove it from the MediaStream track
> set, AFAICT. Martin, do you know if the media-capture TF is intentionally
> steering away from host-driven dynamic track changes? Maybe someone should
> bring this up?

I don't see much evidence of moving away from host-driven changes.  RTCPeerConnection is still likely to produce these sorts of changes.  I'm going to be proposing something like that for HTMLMediaElement.captureStream() as well, though that is indirectly content-driven when you get down to it.

That the spec doesn't properly address this is a problem.  That there is no single controller for changes makes the MediaStream a little messier than might be ideal.  The add and remove operations are idempotent, so it's not a huge issue though.
Flags: needinfo?(martin.thomson)
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

https://reviewboard.mozilla.org/r/10165/#review16651
Attachment #8615074 - Flags: review?(roc) → review+
Assignee

Comment 119

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #113)
> I'm actually starting to feel that the alternative approach where we don't
> have "Owned" might be better, sorry :-(. I'll discuss more in the bug.

Something you'd like to elaborate on here? I'm waiting for more feedback before putting in more work.
Flags: needinfo?(roc)
Sorry, no. Your explanation is fine.

> Right, I have fixed that (you also need part 7 for it to fully work). Probably just forgot to update the comments here.

OK, please update the comments so it's clear how that works.
Flags: needinfo?(roc)
Assignee

Comment 121

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc
This allows for tracking the input track of an added track (for
ProcessedMediaStream tracks; SourceMediaStream tracks don't have input
tracks) directly in the NotifyQueuedTrackChanges handler, which will be
necessary for locking MediaInputPorts to specific tracks.
Attachment #8615070 - Flags: review+
Assignee

Comment 122

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Assignee

Comment 123

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Attachment #8615072 - Flags: review?(roc)
Assignee

Comment 124

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Attachment #8615073 - Flags: review+
Assignee

Comment 125

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Attachment #8615074 - Flags: review+
Assignee

Comment 126

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
A DOMMediaStream's owned stream is piped from the input stream which is
under the control of the DOMMediaStream's owner/producer (like
captureStream or gUM). When producers like these create new tracks after
the stream has already been created (the initial set should be available
to JS synchronously), it is nice if the DOMMediaStream picks them up
automatically and create the corresponding MediaStreamTracks.

The OwnedStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Assignee

Comment 127

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Attachment #8655253 - Flags: review+
Assignee

Comment 128

4 years ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #115)
> you also need part 7 for it to fully work
That should be part 6. 

Patches rebased and updated per roc's comments. I went through all comments in the code so they should be up to date now. Also added a more complicated example to the visualized-DOMMediaStream-cases-comment - it's basically comment 115 combined and visualized.
Assignee

Comment 129

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Carries r=roc.
Attachment #8615070 - Flags: review+
Assignee

Comment 130

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Carries r=jesup.
Attachment #8615073 - Flags: review+
Assignee

Comment 131

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Carries r=roc.
Attachment #8615074 - Flags: review+
Assignee

Comment 132

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Carries r=jesup.
Attachment #8655253 - Flags: review+
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

https://reviewboard.mozilla.org/r/10159/#review17101
Attachment #8615071 - Flags: review?(roc) → review+
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

https://reviewboard.mozilla.org/r/10161/#review17121

I still don't see how new tracks arriving in the input stream are automatically forwarded to all the clones, off the main thread. Can you explain that in comments somewhere?

::: dom/media/DOMMediaStream.cpp:55
(Diff revision 10)
> +                     bool aOwnsInputPort,

Use an enum instead of a bool so the call sites are readable.

::: dom/media/DOMMediaStream.cpp:97
(Diff revision 10)
> +  bool mOwnsInputPort;

Document this, and put it last in the class to avoid holes.

::: dom/media/DOMMediaStream.cpp:419
(Diff revision 10)
> +                                               MediaInputPort::FLAG_BLOCK_OUTPUT);

Don't pass FLAG_BLOCK_OUTPUT here. We're getting rid of inter-stream blocking. And don't call BlockStreamIfNeeded.

::: dom/media/DOMMediaStream.cpp:425
(Diff revision 10)
> +                                                     MediaInputPort::FLAG_BLOCK_OUTPUT);

Here too.

::: dom/media/DOMMediaStream.cpp:471
(Diff revision 10)
> -    mStream->AsSourceStream()->EndTrack(aTrackID);
> +    mInputStream->AsSourceStream()->EndTrack(aTrackID);

Won't this stop the track in all clones, incorrectly?

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:463
(Diff revision 10)
> +                                                MediaSegment::AUDIO);

I dont like having to do this. Why can't the DOMMediaStream observe tracks appearing in its input stream and create the owned DOM tracks itself?

ll don't see
Assignee

Comment 135

4 years ago
https://reviewboard.mozilla.org/r/10161/#review17121

I based it on the spec which says "Clone each track in this MediaStream object and add the result to streamClone's track set.; Return streamClone.", but then again the spec might not cover dynamically added tracks that well..

It's easily done with a TRACK_ANY input port, same as for the original stream. I'll put it in.

> Won't this stop the track in all clones, incorrectly?

No, because clone is not implemented yet so we retain the old behavior. I plan to first implement addTrack, removeTrack; then mediastream constructors, then clone.

> I dont like having to do this. Why can't the DOMMediaStream observe tracks appearing in its input stream and create the owned DOM tracks itself?

It can (with part 6), though I think it's much nicer for JS developers to know about the tracks up front. For WebRTC they're defined in the SDP so when we get here we already know about them, no reason not to tell JS in my opinion.

I'll remove these since they're not needed with part 6 and as such should go to another bug.
https://reviewboard.mozilla.org/r/10161/#review17121

> No, because clone is not implemented yet so we retain the old behavior. I plan to first implement addTrack, removeTrack; then mediastream constructors, then clone.

OK

> It can (with part 6), though I think it's much nicer for JS developers to know about the tracks up front. For WebRTC they're defined in the SDP so when we get here we already know about them, no reason not to tell JS in my opinion.
> 
> I'll remove these since they're not needed with part 6 and as such should go to another bug.

Right, I just looked at Part 6 and realized that. Great.
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

https://reviewboard.mozilla.org/r/10167/#review17133
Attachment #8615075 - Flags: review?(roc) → review+
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

https://reviewboard.mozilla.org/r/17895/#review17135
Attachment #8655253 - Flags: review?(roc) → review+
Assignee

Comment 139

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Attachment #8615072 - Flags: review?(roc)
Assignee

Comment 140

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Attachment #8615073 - Flags: review+
Assignee

Comment 141

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Attachment #8615074 - Flags: review+
Assignee

Comment 142

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
A DOMMediaStream's owned stream is piped from the input stream which is
under the control of the DOMMediaStream's owner/producer (like
captureStream or gUM). When producers like these create new tracks after
the stream has already been created (the initial set should be available
to JS synchronously), it is nice if the DOMMediaStream picks them up
automatically and create the corresponding MediaStreamTracks.

The OwnedStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.
Attachment #8615075 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8655253 - Flags: review+
Assignee

Comment 143

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Assignee

Comment 144

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Carries r=jesup.
Attachment #8615073 - Flags: review+
Assignee

Comment 145

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Carries r=roc.
Attachment #8615074 - Flags: review+
Assignee

Comment 146

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Carries r=roc.
Attachment #8615075 - Flags: review+
Assignee

Comment 147

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Carries r=roc,jesup.
Attachment #8655253 - Flags: review+
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

https://reviewboard.mozilla.org/r/10161/#review17151
Attachment #8615072 - Flags: review?(roc) → review+
Assignee

Updated

4 years ago
Blocks: 1204761
Assignee

Comment 149

4 years ago
Bug 1170958 - Part 8. Don't create owned MediaStreamTracks in MetadataLoaded. r?roc
When play()ing a media element after it has ended, MediaDecoder will
again call MetadataLoaded(). When capturing the media to a
DOMMediaStream, that will attempt to create new MediaStreamTracks in the
stream with the original TrackIDs. That won't work, since the original
tracks with the same TrackIDs have already ended.

We solve it by only explicitly creating MediaStreamTracks in the stream
in captureStream(), and only if they're already known. Otherwise the
tracks will be created asynchronously when available in the underlying
stream.
Attachment #8661081 - Flags: review?(roc)
Assignee

Comment 150

4 years ago
Part 8 is based on this try push (a new assert I introduced caught it): https://treeherder.mozilla.org/#/jobs?repo=try&revision=5444a0b88721
Comment on attachment 8661081 [details]
MozReview Request: Bug 1170958 - Part 8. Don't create owned MediaStreamTracks in MetadataLoaded. r?roc

https://reviewboard.mozilla.org/r/19285/#review17211
Assignee

Updated

4 years ago
Attachment #8615070 - Flags: review+ → review?(roc)
Assignee

Comment 152

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc
This allows for tracking the input track of an added track (for
ProcessedMediaStream tracks; SourceMediaStream tracks don't have input
tracks) directly in the NotifyQueuedTrackChanges handler, which will be
necessary for locking MediaInputPorts to specific tracks.
Assignee

Comment 153

4 years ago
Comment on attachment 8615071 [details]
MozReview Request: Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc

Bug 1170958 - Part 2. Allow MediaInputPort to lock to a specific input track. r?roc
Locking to specific tracks lets us dynamically remove and add single
tracks to a ProcessedMediaStream.
Assignee

Comment 154

4 years ago
Comment on attachment 8615072 [details]
MozReview Request: Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc

Bug 1170958 - Part 3. Refactor DOMMediaStream to contain a 3-stage track chain. r?roc
This lets us separate tracks by ownership like so:
* Input    - Owned by the producer of the DOMMediaStream (gUM etc.)
* Owned    - Contains Input tracks (per above) or tracks cloned tracks
             if this DOMMediaStream is a clone.
* Playback - Contains Owned tracks plus tracks addTrack()ed to this
             DOMMediaStream minus tracks removeTrack()ed from this
             DOMMediaStream.
Assignee

Comment 155

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup
Simplifies the structure of MediaManager somewhat. Possible since
MediaManager owns both the SourceMediaStream and the DOMMediaStream.
Attachment #8615073 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8615074 - Flags: review?(roc)
Attachment #8615074 - Flags: review?(rjesup)
Attachment #8615074 - Flags: review+
Assignee

Comment 156

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc
Assignee

Comment 157

4 years ago
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc
A DOMMediaStream's owned stream is piped from the input stream which is
under the control of the DOMMediaStream's owner/producer (like
captureStream or gUM). When producers like these create new tracks after
the stream has already been created (the initial set should be available
to JS synchronously), it is nice if the DOMMediaStream picks them up
automatically and create the corresponding MediaStreamTracks.

The OwnedStreamListener added here does just that; creates an owned
MediaStreamTrack when a track appeared in the stream that didn't already
have a MediaStreamTrack.

It also moves the logic for ended tracks from the PlaybackStreamListener
to the OwnedStreamListener as we previously would see a track end in the
playbak stream after removeTrack() and that would be interpreted as the
track ending at the source.
Attachment #8615075 - Flags: review+ → review?(roc)
Assignee

Comment 158

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup
TrackUnionStream guarantees that TrackIDs are maintained if no tracks
have claimed them before.

In the gUM case, we have a SourceMediaStream which we wholly own (the
DOMMediaStream's Input stream), piped into a TrackUnionStream which
no-one external is able to add tracks to (the DOMMediaStream's Owned
stream) - addTrack()ed tracks are added to the DOMMediaStream's Playback
stream.

The MediaStreamTracks being enabled/disable refer to a TrackID in the
DOMMediaStream's Owned stream.

Alas, we don't need to forward a track's enabled state, we can just do
it on the source.
Attachment #8655253 - Flags: review+ → review?(roc)
Assignee

Comment 159

4 years ago
Comment on attachment 8661081 [details]
MozReview Request: Bug 1170958 - Part 8. Don't create owned MediaStreamTracks in MetadataLoaded. r?roc

Bug 1170958 - Part 8. Don't create owned MediaStreamTracks in MetadataLoaded. r?roc
When play()ing a media element after it has ended, MediaDecoder will
again call MetadataLoaded(). When capturing the media to a
DOMMediaStream, that will attempt to create new MediaStreamTracks in the
stream with the original TrackIDs. That won't work, since the original
tracks with the same TrackIDs have already ended.

We solve it by only explicitly creating MediaStreamTracks in the stream
in captureStream(), and only if they're already known. Otherwise the
tracks will be created asynchronously when available in the underlying
stream.
Assignee

Comment 160

4 years ago
Comment on attachment 8615070 [details]
MozReview Request: Bug 1170958 - Part 1. Add input stream and track as args to NotifyQueuedTrackChanges. r?roc

Something went seriously bananas here, sorry for the spam! Mozreview reflects the correct state..

Carries r=roc.
Attachment #8615070 - Flags: review?(roc) → review+
Assignee

Comment 161

4 years ago
Comment on attachment 8615073 [details]
MozReview Request: Bug 1170958 - Part 4. Feed a SourceMediaStream-backed dom stream instead of a raw SourceMediaStream in MediaManager. r?jesup

Carries r=jesup.
Attachment #8615073 - Flags: review+
Assignee

Comment 162

4 years ago
Comment on attachment 8615074 [details]
MozReview Request: Bug 1170958 - Part 5. Improve logging of MediaStreams and playback. r?roc

Carries r=roc.
Attachment #8615074 - Flags: review?(roc)
Attachment #8615074 - Flags: review?(rjesup)
Attachment #8615074 - Flags: review+
Assignee

Comment 163

4 years ago
Comment on attachment 8655253 [details]
MozReview Request: Bug 1170958 - Part 7. Remove ProcessedMediaStream::ForwardTrackEnabled. r?roc,jesup

Carries r=roc,jesup.
Attachment #8655253 - Flags: review?(roc) → review+
Comment on attachment 8615075 [details]
MozReview Request: Bug 1170958 - Part 6. Add DOMMediaStream::OwnedStreamListener. r?roc

https://reviewboard.mozilla.org/r/10167/#review18187
Attachment #8615075 - Flags: review?(roc) → review+
BTW Andreas, with mozreview I find that it's not necessary to label commit messages with "Part X" anymore.
Assignee

Updated

4 years ago
No longer depends on: 1172394
Assignee

Comment 166

4 years ago
Bug 1170958 - Destroy track-locked MediaInputPorts when the track ends. r?roc
This is needed to make tests pass until we have bug 1208316 implemented.
Attachment #8665807 - Flags: review?(roc)
Comment on attachment 8665807 [details]
MozReview Request: Bug 1170958 - Destroy track-locked MediaInputPorts when the track ends. r?roc

https://reviewboard.mozilla.org/r/20327/#review18287
Attachment #8665807 - Flags: review?(roc) → review+
Assignee

Updated

4 years ago
Blocks: 1208371
Assignee

Updated

4 years ago
Blocks: 1208373
Assignee

Comment 171

4 years ago
See bug 1103188 comment 85.
Flags: needinfo?(pehrsons)
You need to log in before you can comment on or make changes to this bug.