1208371 - Implement MediaStream/MediaStreamTrack.clone()

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

A cloned stream will wire up a MediaInputPort from the original DOM stream's input stream to the clone's owned stream.

A cloned track will have to create a dummy DOMMediaStream on which to do the same wiring. The dummy is kept alive by a ref from the track-clone but not exposed to js.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc

Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make AudioCaptureStream startable. r?padenot

Bug 1208371 - Make AudioCaptureStream startable. r?padenot
This allows us to add the JS-side MediaStreamTrack before the MSG-side
track.

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc

Bug 1208371 - Expose TrackPort in DOMMediaStream.h r?roc

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc
So it can be forward declared.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

Bug 1208371 - Introduce MediaStreamTrack logs. r?roc

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc
This lets a MediaStreamTrack communicate with its source/producer on the
main thread. It's for now used for stopping a track at the source and
retrieving some metadata, but it could also be a link between actual
sinks of a track and the source, to for instance let the source optimize
by scaling down the resolution when all sinks want lowres-video.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc

Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc
This allows DOMMediaStream to assign MediaStreamTrackSources to
dynamically created MediaStreamTracks.

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc

Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc
For original tracks, the input TrackID is the same as in its
owned stream.
For cloned tracks, the input TrackID comes from the original track,
since no guarantees about TrackIDs in a cloned DOMMediaStream's owned
stream can be given
(imagine e.g., `new MediaStream([trackID1FromStreamX, trackID1FromStreamY]).clone()`).

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc

Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc
This let's us use FindOwnedDOMTrack before the TrackID in mOwnedStream
is known.

This is necessary for a stream clone with multiple tracks whose original
TrackID is the same.

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc

Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

Bug 1208371 - Introduce an unbound state for MediaStreamTracks r?roc,jib
The need for this comes from the following JS code example:
var stream = new MediaStream();
stream.addTrack(gUMVideoTrack1); // TrackID 1 internally
stream.addTrack(gUMVideoTrack2); // TrackID 1 internally
var clone = stream.clone();

I.e., just after cloning a stream, we don't know which TrackIDs the
cloned tracks will have. We have to wait for the MSG to handle the
new MediaInputPorts and notify us of the new TrackIDs.

This will require us to proxy certain operations in MediaStreamTrack in
future patches when the track is in an unbound state, for instance
adding a listener to the underlying stream/track.

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Count the users of a MediaStream to ease Destroy() responsibility. r?roc

Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

Comment 15

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add convenience method for checking if TrackID is explicit. r?roc

Bug 1208371 - Allow MediaInputPorts to be created before the source's TrackID is known. r?roc
If the tracks of a cloned stream are added to another stream, they may
not have been assigned a TrackID yet. AddTrack() still requires a
MediaInputPort to be set up so we're forced to use what we have;
the input MediaStream and input TrackID to the cloned track.

This patch allows allocation of such input ports and gives
TrackUnionStream the ability handle them. These input ports are
guaranteed to be processed after their sources have been created, so there's
no risk for them not finding the source track.

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Allow MediaInputPorts mapped to a destination TrackID. r?roc

Bug 1208371 - Allow allocating input ports based on not-yet-initialized input ports. r?roc

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

Bug 1208371 - Pass MediaStreamTrack to ApplyConstraintsToTrack instead of TrackID. r?jib

Comment 18

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Let DOMMediaStream base its principal on the tracks it contains. r?mt

Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

Comment 19

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

Bug 1208371 - Implement MediaStream.clone() r?smaug,jib,roc

Comment 20

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

Bug 1208371 - Forward applyConstraints() to original track for clones. r?jib

Comment 21

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make it possible to look up stream id by track in PeerConnectionImpl. r?jib

Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib

Comment 22

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback and wrap methods in getters. r?jib

Comment 23

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

Bug 1208371 - Test MediaStream.clone(). r?jib

Comment 24

[Andreas Pehrson [:pehrsons]]

This is not quite finished yet, but I thought I'd get started with the reviews. I still need to add a test for track cloning, and for some more corner cases, including making sure that principals are correctly combined.

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676162 [details]
MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

https://reviewboard.mozilla.org/r/22575/#review20101

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8676177 [details]
MozReview Request: Bug 1208371 - Let DOMMediaStream base its principal on the tracks it contains. r?mt

MediaStreamTrack*
+DOMMediaStream::CreateClonedDOMTrack(MediaStreamTrack& aClonedTrack)
+{
+  MOZ_RELEASE_ASSERT(mOwnedStream);
+  MOZ_RELEASE_ASSERT(mPlaybackStream);
+
+  TrackID inputTrackID = aClonedTrack.GetInputTrackID();
+  MediaStream* inputStream = aClonedTrack.GetInputStream();
+
+  CombineWithPrincipal(aClonedTrack.GetInputDOMStream()->GetPrincipal());
+
+  MOZ_ASSERT(FindOwnedDOMTrack(inputStream, inputTrackID) == nullptr);
+
+  MediaStreamTrackSource& source = aClonedTrack.GetSource();
+  MediaStreamTrack* track;
Please use RefPtr here and make the method to return already_AddRefed<MediaStreamTrack>

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib


-  : mLogicalStreamStartTime(0), mWindow(aWindow),
+  : mLogicalStreamStartTime(0), mWindow(aWindow), mClonedDOMStream(nullptr)
No need to initialize RefPtr member variables to null (they are initialized to null implicitly)



+  protected:
+    virtual ~ClonedStreamSourceGetter() {}
+    DOMMediaStream* const mStream;
+  };
I'm not familiar with this code, but raw pointers as member variables are scary so it needs some comment 
explaining why it can't ever point to deleted object and perhaps MOZ_NON_OWNING_REF annotation.



r+ for the .webidl change.

Comment 28

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676164 [details]
MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

https://reviewboard.mozilla.org/r/22579/#review20103

I'll have to come back to this one at the end, once I understand why we need to keep track of the original track.

::: dom/media/MediaStreamTrack.h:77
(Diff revision 1)
> +  RefPtr<MediaStreamTrack> mClonedTrack;

I think you have it right in the patch title, and this should be mOriginalTrack. "ClonedTrack" may be linguistically accurate, but is an odd label and makes me thing of the track-clone.

When I eject two discs from a duplicator, I write "original" on one and "copy" on the other. I don't write "copied" on the first.

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22581/#review20105

::: dom/html/HTMLCanvasElement.cpp:685
(Diff revision 1)
> -  stream->CreateOwnDOMTrack(videoTrackId, MediaSegment::VIDEO);
> +  stream->CreateOwnDOMTrack(videoTrackId, MediaSegment::VIDEO, new BasicNonStoppableTrackSource());

Unstoppable? :)

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22585/#review20129

Here's an example: In the description for this patch it says: "For cloned tracks, the input TrackID comes from the original track,"

Here I think you're using "cloned track" to mean the track clone.

Comment 31

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22585/#review20131

::: dom/media/MediaStreamTrack.h:141
(Diff revision 1)
> +   * Returns the TrackID this MediaStreamTrack has in it's original owning DOM
> +   * stream's input stream.

I hope we can refactor this soon, so we don't have to understand sentences like that. TrackIDs from different domains share the same type, so the possibility for bugs seems high.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676157 [details]
MozReview Request: Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc

https://reviewboard.mozilla.org/r/22567/#review20209

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676160 [details]
MozReview Request: Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc

https://reviewboard.mozilla.org/r/22571/#review20211

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676161 [details]
MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

https://reviewboard.mozilla.org/r/22573/#review20213

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676163 [details]
MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

https://reviewboard.mozilla.org/r/22577/#review20217

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676165 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

https://reviewboard.mozilla.org/r/22581/#review20219

Please split the move to MediaEnginePhotoCallback out of this patch to a separate patch.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676168 [details]
MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc

https://reviewboard.mozilla.org/r/22583/#review20221

::: dom/media/DOMMediaStream.h:75
(Diff revision 1)
> +class MediaStreamTrackSourceGetter

This needs documentation.

::: dom/media/DOMMediaStream.h:81
(Diff revision 1)
> +  NS_IMETHOD_(MozExternalRefCountType) Release(void) = 0;

Why are these pure virtual? Can't we declare inline refcounting on the class?

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676169 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc

https://reviewboard.mozilla.org/r/22585/#review20223

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676170 [details]
MozReview Request: Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc

https://reviewboard.mozilla.org/r/22587/#review20225

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676171 [details]
MozReview Request: Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc

https://reviewboard.mozilla.org/r/22589/#review20227

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676172 [details]
MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

https://reviewboard.mozilla.org/r/22591/#review20231

> I.e., just after cloning a stream, we don't know which TrackIDs the
> cloned tracks will have. We have to wait for the MSG to handle the
> new MediaInputPorts and notify us of the new TrackIDs.

Is there no way to ensure that the main thread can compute the same track ID assignments as the MSG?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676173 [details]
MozReview Request: Bug 1208371 - Count the users of a MediaStream to ease Destroy() responsibility. r?roc

https://reviewboard.mozilla.org/r/22593/#review20233

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676177 [details]
MozReview Request: Bug 1208371 - Let DOMMediaStream base its principal on the tracks it contains. r?mt

https://reviewboard.mozilla.org/r/22601/#review20235

::: dom/media/MediaStreamTrack.cpp:134
(Diff revision 1)
> +  newStream->InitPlaybackStreamCommon(GetStream()->GetPlaybackStream()->Graph());

Factor out Graph() call

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

https://reviewboard.mozilla.org/r/22603/#review20237

::: dom/media/DOMMediaStream.cpp:629
(Diff revision 1)
> +    DOMMediaStream* const mStream;

How do we know ClonedStreamSourceGetter outlives mStream?

Comment 45

[Andreas Pehrson [:pehrsons]]

(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8676177 [details]
> MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone().
> r?smaug,jib,roc
> 
> MediaStreamTrack*
> +DOMMediaStream::CreateClonedDOMTrack(MediaStreamTrack& aClonedTrack)
> +{
> +  MOZ_RELEASE_ASSERT(mOwnedStream);
> +  MOZ_RELEASE_ASSERT(mPlaybackStream);
> +
> +  TrackID inputTrackID = aClonedTrack.GetInputTrackID();
> +  MediaStream* inputStream = aClonedTrack.GetInputStream();
> +
> +  CombineWithPrincipal(aClonedTrack.GetInputDOMStream()->GetPrincipal());
> +
> +  MOZ_ASSERT(FindOwnedDOMTrack(inputStream, inputTrackID) == nullptr);
> +
> +  MediaStreamTrackSource& source = aClonedTrack.GetSource();
> +  MediaStreamTrack* track;
> Please use RefPtr here and make the method to return
> already_AddRefed<MediaStreamTrack>

Similarly to CreateOwnDOMTrack() I think it should be fine to return a raw pointer here - the returned track is kept alive by both mOwnedTracks and mTracks. With a raw pointer we can safely ignore the result when calling CreateClonedDOMTrack, but with already_AddRefed we can't, we'd have to put it in a RefPtr and let that go out of scope.

Comment 46

[Paul Adenot (:padenot)]

Comment on attachment 8676159 [details]
MozReview Request: Bug 1208371 - Make AudioCaptureStream startable. r?padenot

https://reviewboard.mozilla.org/r/22569/#review20261

Comment 47

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676172 [details]
MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

https://reviewboard.mozilla.org/r/22591/#review20425

Sorry this is taking so long, but the complexity seems to be rising here, and I'm having trouble keeping all the concepts in my head "owning", "input", "bound". I'm still unsure what input is. So I'll need more time to wrap my head around this. I also feel compelled to "fight back" against this complexity (see annoying questions and ideas below).

::: dom/media/MediaStreamTrack.h:148
(Diff revision 1)
> -  TrackID GetTrackID() const { return mTrackID; }
> +  DOMMediaStream* GetStream() const { return mOwningDOMStream; }

The fact that a MediaStreamTrack has an "owning" object very much seems like an implementation detail.

I'm wondering... What would it take to make mOwningDOMStream private to MediaStreamTrack?

I'm looking for ways to encapsulate complexity, and it occurs to me that if MediaStreamTrack were not to expose this detail about itself, even to other implementation objects, then it might simplify things.

MediaStreamTrack would need to proxy some methods to mOwningDOMStream obviously, but how many?

::: dom/media/MediaStreamTrack.h:191
(Diff revision 1)
> +  // Returns true if this MediaStreamTrack has been bound,
> +  // i.e., linked to it's underlying MSG-MediaStream and TrackID.

MSG-MediaStream... That's a nice name (giving me ideas below)

::: dom/media/MediaStreamTrack.h:197
(Diff revision 1)
> +  TrackID BoundTrackID() const

This should be GetBoundTrackID() according to your own comment above.

::: dom/media/MediaStreamTrack.h:213
(Diff revision 1)
> -  RefPtr<DOMMediaStream> mOwningStream;
> +  RefPtr<DOMMediaStream> mOwningDOMStream;
> -  TrackID mTrackID;
>    TrackID mInputTrackID;
>    const RefPtr<MediaStreamTrackSource> mSource;
> +  MediaStream* mBoundOwningStream;
> +  TrackID mBoundTrackID;

I see what you're having to do here to avoid confusion about dom "stream" vs. msg "stream", with the introduction of "mBoundOwningStream" being msg vs. mOwningStream which is dom.

It's not just this line, it's throughout that I find this hard to follow. The fact that we have MediaStream* and MediaStreamTrack* and that they're in different domains, is coming to bear I think.

I humbly suggest the time may have come to undertake a two-step plan either before or shortly after this bug to:

 1. Rename MediaStream to MSGMediaStream or MsgStream (or MediaStreamNode?)
 2. Rename DOMMediaStream to MediaStream sometime soon.

Once that is done, mBoundOwningStream -> mBoundMsgStream, with no need for "Owning" (since we never bind to non-owned things I presume?)

::: dom/media/imagecapture/CaptureTask.cpp:60
(Diff revision 1)
>    RefPtr<DOMMediaStream> domStream = track->GetStream();
> -  domStream->AddPrincipalChangeObserver(this);
> +  MOZ_RELEASE_ASSERT(domStream);
> +  track->GetStream()->AddPrincipalChangeObserver(this);
>  
> -  RefPtr<MediaStream> stream = domStream->GetPlaybackStream();
> -  stream->AddListener(this);
> +  MOZ_RELEASE_ASSERT(domStream->GetOwnedStream());
> +  track->GetStream()->GetOwnedStream()->AddListener(this);

The addref here seems redundant.

Comment 48

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22591/#review20429

::: dom/media/MediaStreamTrack.h:212
(Diff revision 1)
>  
> -  RefPtr<DOMMediaStream> mOwningStream;
> +  RefPtr<DOMMediaStream> mOwningDOMStream;

Also, can't mOwningDOMStream be const?

Comment 49

[Andreas Pehrson [:pehrsons]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> Comment on attachment 8676172 [details]
> MozReview Request: Bug 1208371 - Introduce an unbound state for
> MediaStreamTracks r?roc,jib
> 
> https://reviewboard.mozilla.org/r/22591/#review20231
> 
> > I.e., just after cloning a stream, we don't know which TrackIDs the
> > cloned tracks will have. We have to wait for the MSG to handle the
> > new MediaInputPorts and notify us of the new TrackIDs.
> 
> Is there no way to ensure that the main thread can compute the same track ID
> assignments as the MSG?

Well, can I do it behind a Mutex in TrackUnionStream? It might block TrackUnionStream::AddTrack() for a bit but will lead to a cleaner solution.

Otherwise we could perhaps get away with reserving the upper half of the TrackID address space for tracks from TRACK_ANY input ports, and the lower half for tracks from explicit ports. It would break the invariant that TrackIDs are preserved when possible, however, and I'm not sure how to work around that.

Please do tell if you have other ideas.

Comment 50

[Andreas Pehrson [:pehrsons]]

(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8676178 [details]
> MozReview Request: Bug 1208371 - Implement MediaStream.clone()
> r?smaug,jib,roc
> 
> 
> -  : mLogicalStreamStartTime(0), mWindow(aWindow),
> +  : mLogicalStreamStartTime(0), mWindow(aWindow), mClonedDOMStream(nullptr)
> No need to initialize RefPtr member variables to null (they are initialized
> to null implicitly)

Fixed.


> +  protected:
> +    virtual ~ClonedStreamSourceGetter() {}
> +    DOMMediaStream* const mStream;
> +  };
> I'm not familiar with this code, but raw pointers as member variables are
> scary so it needs some comment 
> explaining why it can't ever point to deleted object and perhaps
> MOZ_NON_OWNING_REF annotation.

I switched this to a WeakPtr to make it more clear. Even with a comment in this case it was quite tricky to see the relationship.

Comment 51

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22579/#review20103

> I think you have it right in the patch title, and this should be mOriginalTrack. "ClonedTrack" may be linguistically accurate, but is an odd label and makes me thing of the track-clone.
> 
> When I eject two discs from a duplicator, I write "original" on one and "copy" on the other. I don't write "copied" on the first.

mOriginalTrack it is.

Comment 52

[Andreas Pehrson [:pehrsons]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> Comment on attachment 8676168 [details]
> MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface.
> r?roc
> 
> https://reviewboard.mozilla.org/r/22583/#review20221
> 
> ::: dom/media/DOMMediaStream.h:75
> (Diff revision 1)
> > +class MediaStreamTrackSourceGetter
> 
> This needs documentation.

Done.


> ::: dom/media/DOMMediaStream.h:81
> (Diff revision 1)
> > +  NS_IMETHOD_(MozExternalRefCountType) Release(void) = 0;
> 
> Why are these pure virtual? Can't we declare inline refcounting on the class?

I removed the refcounting and instead keep them in a UniquePtr in their parent DOMMediaStream.

Comment 53

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #47)
> Comment on attachment 8676172 [details]
> MozReview Request: Bug 1208371 - Introduce an unbound state for
> MediaStreamTracks r?roc,jib
> 
> https://reviewboard.mozilla.org/r/22591/#review20425
> 
> Sorry this is taking so long, but the complexity seems to be rising here,
> and I'm having trouble keeping all the concepts in my head "owning",
> "input", "bound". I'm still unsure what input is. So I'll need more time to
> wrap my head around this. I also feel compelled to "fight back" against this
> complexity (see annoying questions and ideas below).

Appreciated! I found myself a bit stuck in this complexity a couple of times while writing the patches. Though I think I managed to clean it up a little bit after everything got wired up and into a working state. All your "annoying" questions are great help towards making it even cleaner. I'll reply to your other questions in mozreview.

Comment 54

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22591/#review20425

> The fact that a MediaStreamTrack has an "owning" object very much seems like an implementation detail.
> 
> I'm wondering... What would it take to make mOwningDOMStream private to MediaStreamTrack?
> 
> I'm looking for ways to encapsulate complexity, and it occurs to me that if MediaStreamTrack were not to expose this detail about itself, even to other implementation objects, then it might simplify things.
> 
> MediaStreamTrack would need to proxy some methods to mOwningDOMStream obviously, but how many?

I'll try to hide most of it away and see where that takes me.

We shouldn't have to proxy much at all anymore with the MediaStreamTrackSource interface. For now, there's at least ApplyConstraints going through DOMMediaStream, but that can be moved to MediaStreamTrackSource.

> This should be GetBoundTrackID() according to your own comment above.

Right, changed the comment.

> I see what you're having to do here to avoid confusion about dom "stream" vs. msg "stream", with the introduction of "mBoundOwningStream" being msg vs. mOwningStream which is dom.
> 
> It's not just this line, it's throughout that I find this hard to follow. The fact that we have MediaStream* and MediaStreamTrack* and that they're in different domains, is coming to bear I think.
> 
> I humbly suggest the time may have come to undertake a two-step plan either before or shortly after this bug to:
> 
>  1. Rename MediaStream to MSGMediaStream or MsgStream (or MediaStreamNode?)
>  2. Rename DOMMediaStream to MediaStream sometime soon.
> 
> Once that is done, mBoundOwningStream -> mBoundMsgStream, with no need for "Owning" (since we never bind to non-owned things I presume?)

I completely agree that it would be great to solve the naming issue. I can create another patch or two that does this.

We'd also end up with,
MSGMediaStream
MSGProcessedMediaStream
MSGTrackUnionStream
MSGSourceStream

> The addref here seems redundant.

Fixed.

Comment 55

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22591/#review20429

> Also, can't mOwningDOMStream be const?

No, because of cycle collection.

Comment 56

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22591/#review20425

> Right, changed the comment.

Not the function name?

> I completely agree that it would be great to solve the naming issue. I can create another patch or two that does this.
> 
> We'd also end up with,
> MSGMediaStream
> MSGProcessedMediaStream
> MSGTrackUnionStream
> MSGSourceStream

Hmm, not ProcessedMSGMediaStream, TrackUnionMSGStream and SourceMSGStream? Bike-shedding time!

Did anyone like MediaStreamNode? i.e. the "nodes in a graph" in a MediaStreamGraph? I think the root of the confusion is that both concepts are "streams" colloquially, and this would address that (if unfairly since they came first). It would also highlight that the implementation objects are in fact nodes in a graph, and maybe also that a single "stream" of data can run through more than one of these objects. It's also easier to compose on:

MediaStreamNode
ProcessedMediaStreamNode
TrackUnionStreamNode
SourceStreamNode

Thoughts?

Comment 57

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

https://reviewboard.mozilla.org/r/22599/#review20519

with fix.

::: dom/media/MediaManager.cpp:3168
(Diff revision 1)
> -  if (!(((aIsAudio && mAudioDevice) ||
> -         (!aIsAudio && mVideoDevice)) && !mStopped))
> +  if (mStopped || ((aTrackID == kAudioTrack && !mAudioDevice) &&
> +                   (aTrackID == kVideoTrack && !mVideoDevice)))

One inversion too many.

  if (mStopped || ((aTrackID == kAudioTrack && !mAudioDevice) ||
                   (aTrackID == kVideoTrack && !mVideoDevice)))

Comment 58

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

https://reviewboard.mozilla.org/r/22599/#review20523

Comment 59

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676177 [details]
MozReview Request: Bug 1208371 - Let DOMMediaStream base its principal on the tracks it contains. r?mt

https://reviewboard.mozilla.org/r/22601/#review20525

lgtm with nits, unless you bite on refactoring away the dom-owning dummy.

::: dom/media/DOMMediaStream.h:499
(Diff revision 1)
> +  /**
> +   * Creates a MediaStreamTrack cloned from aClonedTrack, adds it to mTracks
> +   * and returns it.
> +   */
> +  MediaStreamTrack* CreateClonedDOMTrack(MediaStreamTrack& aClonedTrack);

aOriginalTrack

and maybe CreateDOMTrackClone ?

::: dom/media/DOMMediaStream.cpp:831
(Diff revision 1)
>  MediaStreamTrack*
> +DOMMediaStream::CreateClonedDOMTrack(MediaStreamTrack& aClonedTrack)

Given where this is called from, I would make this return already_AddRefed<MediaStreamTrack>

::: dom/media/DOMMediaStream.cpp:845
(Diff revision 1)
> +  MediaStreamTrack* track;

I would use RefPtr<> here and .forget() at the end, or this is one early return away from a leak.

::: dom/media/DOMMediaStream.cpp:862
(Diff revision 1)
> +  RefPtr<TrackPort> trackPort =
> +    new TrackPort(inputPort, track, TrackPort::InputPortOwnership::OWNED);
> +  mOwnedTracks.AppendElement(trackPort.forget());

Why not:

    mOwnedTracks.AppendElement(new TrackPort(...))

?

::: dom/media/MediaStreamTrack.cpp:127
(Diff revision 1)
> +  // MediaStreamTracks are currently governed by streams, so we need a dummy
> +  // DOMMediaStream to own our track clone.

Ugh, yeah. I wonder does it need to be domstream-owned? Could it just inherit the other track's msg-stream-node thing and be done?

::: dom/media/MediaStreamTrack.cpp:136
(Diff revision 1)
> +  RefPtr<MediaStreamTrack> newTrack =
> +    newStream->CreateClonedDOMTrack(*this);
> +
> +  return newTrack.forget();

With alredy_addRefed, this would become:

    return newStream->CreateClonedDOMTrack(*this);

Comment 60

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

https://reviewboard.mozilla.org/r/22603/#review20527

I still need to understand this better.

::: dom/media/DOMMediaStream.h:586
(Diff revision 1)
> +  RefPtr<DOMMediaStream> mClonedDOMStream;

mOriginalDOMStream

::: dom/media/DOMMediaStream.cpp:324
(Diff revision 1)
> -  : mLogicalStreamStartTime(0), mWindow(aWindow),
> +  : mLogicalStreamStartTime(0), mWindow(aWindow), mClonedDOMStream(nullptr),

redundant

::: dom/media/DOMMediaStream.cpp:641
(Diff revision 1)
> +  // Set up an input port from our input stream to the new DOM stream's owned
> +  // stream, to allow for dynamically added tracks at the source to appear in
> +  // the clone. The clone may use mInputStream as its own mInputStream but
> +  // ownership remains with us.

(refactor thoughts) Is destroying mInputStream the main reason we need a pointer to the original?

If so, could we get rid of that dependency with some open/close methods on the input stream instead?

::: dom/media/DOMMediaStream.cpp:645
(Diff revision 1)
> +  DOMMediaStream* originalStream = mClonedDOMStream ? mClonedDOMStream.get() : this;
> +  newStream->mClonedDOMStream = originalStream;

inline auto?

::: dom/media/DOMMediaStream.cpp:653
(Diff revision 1)
> +  // Set up locked input ports for all tracks already existing.
> +  // This will ensure they can be removed at any time, even if
> +  // their TrackIDs are not yet known.
> +  // Due to this, we have to track which tracks are owned by us, so that we
> +  // don't set up those tracks multiple times through the generic port.

I'm not understanding locked ports.

::: dom/media/DOMMediaStream.cpp:672
(Diff revision 1)
> +    newStream->CreateClonedDOMTrack(track);

oh I see about the RefPtr now. I might still vote for it though. Something called "Create" usually returns something.

Maybe if it were named CloneAndAdd() or something

Comment 61

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676179 [details]
MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

https://reviewboard.mozilla.org/r/22605/#review20529

::: dom/media/MediaStreamTrack.cpp:121
(Diff revision 1)
> +  if (mClonedTrack) {
> +    return mClonedTrack->ApplyConstraints(aConstraints, aRv);
> +  } else {
> -  return GetStream()->ApplyConstraintsToTrack(*this, aConstraints, aRv);
> +    return GetStream()->ApplyConstraintsToTrack(*this, aConstraints, aRv);
> -}
> +  }

Why is this necessary?

Comment 62

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676180 [details]
MozReview Request: Bug 1208371 - Make it possible to look up stream id by track in PeerConnectionImpl. r?jib

https://reviewboard.mozilla.org/r/22607/#review20531

Comment 63

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

https://reviewboard.mozilla.org/r/22609/#review20533

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #56)
> Did anyone like MediaStreamNode? i.e. the "nodes in a graph" in a
> MediaStreamGraph? I think the root of the confusion is that both concepts
> are "streams" colloquially, and this would address that (if unfairly since
> they came first). It would also highlight that the implementation objects
> are in fact nodes in a graph, and maybe also that a single "stream" of data
> can run through more than one of these objects. It's also easier to compose
> on:
> 
> MediaStreamNode
> ProcessedMediaStreamNode
> TrackUnionStreamNode
> SourceStreamNode

Not "Node" since we have Web Audio nodes and that would get confusing.

Maybe GraphMediaStream... etc. Or RTMediaStream... etc for "real time" (although for offline MSGs, they're not).

Maybe GraphMediaStream would get confused with MediaStreamGraph.

Maybe AsyncMediaStream?

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #49)
> Well, can I do it behind a Mutex in TrackUnionStream? It might block
> TrackUnionStream::AddTrack() for a bit but will lead to a cleaner solution.

I don't want to bring Mutexes in here.

> Otherwise we could perhaps get away with reserving the upper half of the
> TrackID address space for tracks from TRACK_ANY input ports, and the lower
> half for tracks from explicit ports. It would break the invariant that
> TrackIDs are preserved when possible, however, and I'm not sure how to work
> around that.

I was hoping for a deterministic track assignment algorithm so that the main thread and the MSG thread agree on track IDs.

Maybe we don't need to preserve track IDs. It seemed like a nice thing to have but off the top of my head I don't know of anything that actually needs it.

Seems to me we should be able to set things up so that when the DOM track exists first, the main thread assigns the track ID and propagates that to the MSG. When the MSG track exists first, the MSG assigns the track ID and propagates that to the main thread.

Comment 66

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

https://reviewboard.mozilla.org/r/22611/#review20603

Found two minor issues, the rest are ideas.

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:22
(Diff revision 1)
> +      is(stream.getAudioTracks().length, clone.getAudioTracks().length,
> +         "All audio tracks should get cloned");
> +      is(stream.getVideoTracks().length, clone.getVideoTracks().length,
> +         "All video tracks should get cloned");

The clone's value should be the first argument since that's what we're testing, and that's the value we want dumped in the alert message if it's wrong.

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:29
(Diff revision 1)
> +      // XXX Check stream ID and all cloned tracks' IDs are following the spec

bug # or coming follow-on patch?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:45
(Diff revision 1)
> +        var clonedStream = stream.clone();

On naming: maybe streamClone, since clonedStream has multiple connotations?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:47
(Diff revision 1)
> +        isnot(clonedTrack, track,
> +              "Cloned track should be different from the original");

I would move this up to the first test, to do it on both audio and video.

Also maybe check kind == kind, enabled == enabled and id != id?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:64
(Diff revision 1)
> +        // Not part of clonedStream. Does not get stopped by the playback test.
> +        otherTrack.stop();

Maybe throw in stream.stop() as well for good measure?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:70
(Diff revision 1)
> +    }))

These APIs are extremely flexible now, so I feel like we should throw in some sick tests here as well. E.g.

Test for cycle-collection leaks (e.g. mOriginalTrack):

    var b = a.clone().clone().clone().clone().clone().clone().clone().clone();
    var atracks = a.getTracks(); 
    atracks.forEach(t => (a.removeTrack(t), b.addTrack(t)));

Have tracks multiply, if we support that:

    var atracks = a.getTracks(); 
    for (var i = 0; i < 10; i++) {
      atracks.forEach(t => a.addTrack(t.clone()));
    }
    var b = a.clone();

What do you think?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:77
(Diff revision 1)
> +      var osc1k = osc1kOriginal.clone();
> +      var audioTrack1k = osc1k.getTracks()[0];

osc1k is unused. i.e. why not:

    var audioTrack1kClone = osc1kOriginal.clone().getTracks()[0];

Also, labeling the clone 'clone' would help at least me remember what's being tested (I lost track below).

Lastly, could we just clone the track here? e.g.

    var audioTrack1kClone = audioTrack1kOriginal.clone();

then we could get rid of var osc1kOriginal as well.

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:82
(Diff revision 1)
> +      var osc5k = osc5kOriginal.clone();
> +      var audioTrack5k = osc5k.getTracks()[0];

same applies here.

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:101
(Diff revision 1)
> +            stream.getTracks().forEach(t => t.stop());
> +            return analyser.waitForAnalysisSuccess(array =>
> +                     array[analyser.binIndexForFrequency(50)] < 50 &&
> +                     array[analyser.binIndexForFrequency(1000)] > 200 &&

Hmm, after stop(), shouldn't this be

    array[analyser.binIndexForFrequency(1000)] < 50 &&

?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:106
(Diff revision 1)
> +                     // WebAudioDestination streams do not handle stop()
> +                     // array[analyser.binIndexForFrequency(5000)] > 200 &&

Is that a bug? If so, bug #. But again, shouldn't it be < 50 ?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:127
(Diff revision 1)
> +        var stream = new MediaStream([audioTrack1k, audioTrack5k]);
> +        var clonedStream = stream.clone();

stream is unused. i.e. we could do

    var streamClone = new MediaStream([audioTrack1k, audioTrack5k]).clone();

Less chance of analysing the wrong var below this way I think.

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:141
(Diff revision 1)
> +        var stream = new MediaStream([audioTrack1k, audioTrack5k]);
> +        var clonedStream = stream.clone();
> +        var newStream = new MediaStream();
> +        clonedStream.getTracks().forEach(t => newStream.addTrack(t));

Why not:

    var stream = new MediaStream(new MediaStream([audioTrack1k, audioTrack5k]).clone().getTracks());

?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:158
(Diff revision 1)
> +        var clonedStream = stream.clone();
> +        stream.getTracks().forEach(t => stream.removeTrack(t));
> +        clonedStream.getTracks().forEach(t => stream.addTrack(t));
> +
> +        var analyser = new AudioStreamAnalyser(ac, stream);
> +        analyser.enableDebugCanvas();
> +        return analyser.waitForAnalysisSuccess(array =>
> +                 array[analyser.binIndexForFrequency(50)] < 50 &&
> +                 array[analyser.binIndexForFrequency(1000)] > 200 &&
> +                 array[analyser.binIndexForFrequency(3000)] < 50 &&
> +                 array[analyser.binIndexForFrequency(5000)] > 200 &&
> +                 array[analyser.binIndexForFrequency(10000)] < 50)
> +          .then(() => analyser.disableDebugCanvas());

What if we mixed it up here and moved just one of the tracks from the clone to the original and then analysed both?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:172
(Diff revision 1)
> +    }));

Would it be useful to throw in an analyser test that checks that a clone can be enabled/disabled separately from the original?

Comment 67

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22579/#review20103

After looking at the other patches, it seems to me like this dependency could be refactored out, but I don't see anything wrong either, so marking at Ship it to not block on refactor.

Comment 68

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676164 [details]
MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

https://reviewboard.mozilla.org/r/22579/#review20629

Comment 69

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22601/#review20525

> aOriginalTrack
> 
> and maybe CreateDOMTrackClone ?

We already have CreateOwnDOMTrack and with that in mind I think CreateClonedDOMTrack makes more sense.

> I would use RefPtr<> here and .forget() at the end, or this is one early return away from a leak.

Done

> Why not:
> 
>     mOwnedTracks.AppendElement(new TrackPort(...))
> 
> ?

We already had this pattern in CreateOwnDOMTrack, but if you don't mind I'll change both.

> Ugh, yeah. I wonder does it need to be domstream-owned? Could it just inherit the other track's msg-stream-node thing and be done?

I can imagine it working by just creating a new MSG-side TrackUnionStream and let that be the track clone's owning stream. The input stream would be inherited.

It might create separate code paths for stream and track cloning though, so I'm not sure I want to do it right now.

Comment 70

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22603/#review20527

> (refactor thoughts) Is destroying mInputStream the main reason we need a pointer to the original?
> 
> If so, could we get rid of that dependency with some open/close methods on the input stream instead?

Yep, I have a patch that does something like this. It lets us get rid of the pointer to the original.

> I'm not understanding locked ports.

Track-locked maybe?

A port here is a MediaInputPort (MSG-side) that forwards tracks from one stream to another. Normally it forwards all tracks but it can also be locked to a specific TrackID in the input stream.

Comment 71

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22605/#review20529

> Why is this necessary?

Because of DOMMediaStream sub classes (hello nsDOMUserMediaStream). It should really go through the MediaStreamTrackSource though, I'll move it there for the next version of this patch.

Comment 72

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #69)
> https://reviewboard.mozilla.org/r/22601/#review20525
> 
> > aOriginalTrack
> > 
> > and maybe CreateDOMTrackClone ?
> 
> We already have CreateOwnDOMTrack and with that in mind I think
> CreateClonedDOMTrack makes more sense.

Earlier, this patch used "cloned track" simultaneously to mean the track being cloned and the clone. It could mean both, which is why I think we should avoid the notion.

How about CreateCloneDOMTrack?

Comment 73

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #70)
> Track-locked maybe?
> 
> A port here is a MediaInputPort (MSG-side) that forwards tracks from one
> stream to another. Normally it forwards all tracks but it can also be locked
> to a specific TrackID in the input stream.

Cool. A comment to that effect should do then, unless I missed it.

Comment 74

[Andreas Pehrson [:pehrsons]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #49)
> > Well, can I do it behind a Mutex in TrackUnionStream? It might block
> > TrackUnionStream::AddTrack() for a bit but will lead to a cleaner solution.
> 
> I don't want to bring Mutexes in here.
> 
> > Otherwise we could perhaps get away with reserving the upper half of the
> > TrackID address space for tracks from TRACK_ANY input ports, and the lower
> > half for tracks from explicit ports. It would break the invariant that
> > TrackIDs are preserved when possible, however, and I'm not sure how to work
> > around that.
> 
> I was hoping for a deterministic track assignment algorithm so that the main
> thread and the MSG thread agree on track IDs.
> 
> Maybe we don't need to preserve track IDs. It seemed like a nice thing to
> have but off the top of my head I don't know of anything that actually needs
> it.
> 
> Seems to me we should be able to set things up so that when the DOM track
> exists first, the main thread assigns the track ID and propagates that to
> the MSG. When the MSG track exists first, the MSG assigns the track ID and
> propagates that to the main thread.

I was able to make it deterministic by allowing the main thread caller to provide a TrackID mapping as long as no TRACK_ANY input port was added prior (if one exists, there might be a race where the desired TrackID gets taken by a track coming from the source stream rather than the main thread). This solves my use case and simplifies the code by quite a bit. :-)

Comment 75

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22611/#review20603

> bug # or coming follow-on patch?

Actually spec just says that using a UUID is good practice so I'll just test that it's non-empty here.

> These APIs are extremely flexible now, so I feel like we should throw in some sick tests here as well. E.g.
> 
> Test for cycle-collection leaks (e.g. mOriginalTrack):
> 
>     var b = a.clone().clone().clone().clone().clone().clone().clone().clone();
>     var atracks = a.getTracks(); 
>     atracks.forEach(t => (a.removeTrack(t), b.addTrack(t)));
> 
> Have tracks multiply, if we support that:
> 
>     var atracks = a.getTracks(); 
>     for (var i = 0; i < 10; i++) {
>       atracks.forEach(t => a.addTrack(t.clone()));
>     }
>     var b = a.clone();
> 
> What do you think?

I think it's a great idea!

You can't add a track if it already exists, but you can clone it and then add it to the original, and loop that a couple of times :-)

> osc1k is unused. i.e. why not:
> 
>     var audioTrack1kClone = osc1kOriginal.clone().getTracks()[0];
> 
> Also, labeling the clone 'clone' would help at least me remember what's being tested (I lost track below).
> 
> Lastly, could we just clone the track here? e.g.
> 
>     var audioTrack1kClone = audioTrack1kOriginal.clone();
> 
> then we could get rid of var osc1kOriginal as well.

I want to do track cloning in another test, but I'll take the rest.

> Hmm, after stop(), shouldn't this be
> 
>     array[analyser.binIndexForFrequency(1000)] < 50 &&
> 
> ?

Yes, this was an error on my part.

> Is that a bug? If so, bug #. But again, shouldn't it be < 50 ?

I'm not sure. I'm planning to take this discussion when we implement the `remote` attribute for tracks, as that's what the stop() algorithm looks at.

> What if we mixed it up here and moved just one of the tracks from the clone to the original and then analysed both?

Sure.

> Would it be useful to throw in an analyser test that checks that a clone can be enabled/disabled separately from the original?

Certainly.

Comment 76

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #72)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #69)
> > https://reviewboard.mozilla.org/r/22601/#review20525
> > 
> > > aOriginalTrack
> > > 
> > > and maybe CreateDOMTrackClone ?
> > 
> > We already have CreateOwnDOMTrack and with that in mind I think
> > CreateClonedDOMTrack makes more sense.
> 
> Earlier, this patch used "cloned track" simultaneously to mean the track
> being cloned and the clone. It could mean both, which is why I think we
> should avoid the notion.
> 
> How about CreateCloneDOMTrack?



Or CreateDOMTrack()/CloneDOMTrack() for CreateOwnDOMTrack()/CreateClonedDOMTrack()?

I'm about to update the patches here but I haven't resolved this yet. It's on my list.

Comment 77

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #73)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #70)
> > Track-locked maybe?
> > 
> > A port here is a MediaInputPort (MSG-side) that forwards tracks from one
> > stream to another. Normally it forwards all tracks but it can also be locked
> > to a specific TrackID in the input stream.
> 
> Cool. A comment to that effect should do then, unless I missed it.

https://dxr.mozilla.org/mozilla-central/rev/1fbc958f75576446a57cf93406db87b51b12911d/dom/media/MediaStreamGraph.h#921
https://dxr.mozilla.org/mozilla-central/rev/1fbc958f75576446a57cf93406db87b51b12911d/dom/media/MediaStreamGraph.h#1066

:-)

Comment 78

[Andreas Pehrson [:pehrsons]]

I'm about to push another set of patches to mozreview, but ordering has changed somewhat and that is messing with reviewboard. I'm letting the first couple of patches keep their r+ as they're unchanged, but I'm requesting new reviews for most other ones. Sorry for any and all inconvenience caused.

This is probably from my workflow of using git and then moving patches over to hg in one go. I don't know of a solution to that problem :/

Comment 79

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676157 [details]
MozReview Request: Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc

Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc

Comment 80

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676159 [details]
MozReview Request: Bug 1208371 - Make AudioCaptureStream startable. r?padenot

Bug 1208371 - Make AudioCaptureStream startable. r?padenot
This allows us to add the JS-side MediaStreamTrack before the MSG-side
track.

Comment 81

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676160 [details]
MozReview Request: Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc

Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc
So it can be forward declared.

Comment 82

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676161 [details]
MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

Comment 83

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676162 [details]
MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

Bug 1208371 - Introduce MediaStreamTrack logs. r?roc

Comment 84

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676163 [details]
MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

Comment 85

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676164 [details]
MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc
So it may be forward declared.

Comment 86

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676165 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

Bug 1208371 - Make DOMMediaStream support WeakPtrs. r?roc

Comment 87

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676168 [details]
MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc

Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc
This lets a MediaStreamTrack communicate with its source/producer on the
main thread. It's for now used for stopping a track at the source and
retrieving some metadata, but it could also be a link between actual
sinks of a track and the source, to for instance let the source optimize
by scaling down the resolution when all sinks want lowres-video.

Comment 88

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676169 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc

Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc
This allows DOMMediaStream to assign MediaStreamTrackSources to
dynamically created MediaStreamTracks.

Comment 89

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676170 [details]
MozReview Request: Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc

Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc
For original tracks, the input TrackID is the same as in its
owned stream.
For cloned tracks, the input TrackID comes from the original track,
since no guarantees about TrackIDs in a cloned DOMMediaStream's owned
stream can be given
(imagine e.g., `new MediaStream([trackID1FromStreamX, trackID1FromStreamY]).clone()`).

Comment 90

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676171 [details]
MozReview Request: Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc

Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc
This let's us use FindOwnedDOMTrack before the TrackID in mOwnedStream
is known.

This is necessary for a stream clone with multiple tracks whose original
TrackID is the same.

Comment 91

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676172 [details]
MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

Comment 92

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676173 [details]
MozReview Request: Bug 1208371 - Count the users of a MediaStream to ease Destroy() responsibility. r?roc

Bug 1208371 - Count the users of a MediaStream to ease Destroy() responsibility. r?roc

Comment 93

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676174 [details]
MozReview Request: Bug 1208371 - Add convenience method for checking if TrackID is explicit. r?roc

Bug 1208371 - Add convenience method for checking if TrackID is explicit. r?roc

Comment 94

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676175 [details]
MozReview Request: Bug 1208371 - Allow MediaInputPorts mapped to a destination TrackID. r?roc

Bug 1208371 - Allow MediaInputPorts mapped to a destination TrackID. r?roc
This lets us know the track's TrackID in the destination stream before
the input port has been processed.
For sanity we only allow mapping to a destination TrackID if the
destination stream does not have any TRACK_ANY input ports already
assigned to it as that can cause intermittent TrackID collisions.

Comment 95

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

Comment 96

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676177 [details]
MozReview Request: Bug 1208371 - Let DOMMediaStream base its principal on the tracks it contains. r?mt

Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

Comment 97

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

Bug 1208371 - Add a PrincipalChangeObserver interface to MediaStreamTrack. r?roc
Also adds MediaStreamTrack::GetPrincipal, so we can start moving
consumers towards checking principals per consumed track, and producers
to set principals per track instead of per stream.

For compatibility with modules consuming whole streams we can move
DOMMediaStream over to listening for principal changes on all its
tracks, plus update its principal when its set of tracks changes.

Comment 98

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676179 [details]
MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

Bug 1208371 - Add an MSGListener interface to MediaStreamTrack. r?roc
This creates a convenience Wrapper MSGListener around
MediaStreamListener to make MediaStreamTrack more self contained. The
convenience lies in that the MediaStreamTrack will handle filtering out
the right TrackID in the MediaStreamListener callback and only forward
the relevant ones to the MSGListener instances.

Comment 99

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676180 [details]
MozReview Request: Bug 1208371 - Make it possible to look up stream id by track in PeerConnectionImpl. r?jib

Bug 1208371 - Make it possible to look up stream id by track in PeerConnectionImpl. r?jib
This attempts to get rid of uses of MediaStreamTrack::GetStream() in
PeerConnectionImpl but does unfortunately not go all the way. There's
still a use case in ReplaceTrack() so we handle it for now by making
PeerConnectionImpl a friend of MediaStreamTrack.

Comment 100

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

Comment 101

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

Bug 1208371 - Remove MediaStreamTrack::GetStream/GetParentObject. r?jib

Comment 102

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Route ApplyConstraints through MediaStreamTrackSource. r?jib

Bug 1208371 - Route ApplyConstraints through MediaStreamTrackSource. r?jib

Comment 103

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make it possible to block tracks in a MediaInputPort initally. r?roc

Bug 1208371 - Make it possible to block tracks in a MediaInputPort initally. r?roc

Comment 104

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

Comment 105

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

Bug 1208371 - Implement MediaStream.clone() r?smaug,jib,roc

Comment 106

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib

Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib

Comment 107

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Forward input stream and track id on regular track changes for union streams. r?roc

Bug 1208371 - Forward input stream and track id on regular track changes for union streams. r?roc

Comment 108

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback. r?jib

Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback and wrap methods in getters. r?jib

Comment 109

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Test DOMMediaStream::Clone(). r?jib

Bug 1208371 - Test MediaStream.clone(). r?jib

Comment 110

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

(MozReview is being silly here, since I r+'ed the previous versions of these patches already.)

Comment 111

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676163 [details]
MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

https://reviewboard.mozilla.org/r/22577/#review21293

Comment 112

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22601/#review21389

::: dom/media/MediaStreamTrack.cpp:118
(Diff revision 2)
> +  return mOwningStream->GetOwnedStream()->Graph();

A lot of owning going on here.

Comment 113

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

https://reviewboard.mozilla.org/r/22611/#review21393

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2088
(Diff revision 2)
>  PeerConnectionImpl::GetRemoteTrackId(const std::string streamId,
> -                                     TrackID numericTrackId,
> +                                     const MediaStreamTrack& aTrack,
>                                       std::string* trackId) const
>  {
>    if (IsClosed()) {
>      return NS_ERROR_UNEXPECTED;
>    }
>  
> -  return mMedia->GetRemoteTrackId(streamId, numericTrackId, trackId);
> +  return mMedia->GetRemoteTrackId(streamId, aTrack.mTrackID, trackId);

These changes are great! I'd love to push this change deeper, into the same function in PeerConnectionMedia and SourceStreamInfo, as there's a chance we'd reach the bottom and get rid of the id here entirely (but it doesn't need to be in this bug).

The distinction of a "numericTrackId" wont be missed. :)

[1] http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?rev=e8c7dfe727cd#760

Comment 114

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681202 [details]
MozReview Request: Bug 1208371 - Route ApplyConstraints through MediaStreamTrackSource. r?jib

https://reviewboard.mozilla.org/r/23771/#review21397

::: dom/media/MediaManager.cpp:3157
(Diff revision 1)
> -  if (!(((aIsAudio && mAudioDevice) ||
> -         (!aIsAudio && mVideoDevice)) && !mStopped))
> +  if (mStopped || ((aTrackID == kAudioTrack && !mAudioDevice) ||
> +                   (aTrackID == kVideoTrack && !mVideoDevice)))
>    {
>      LOG(("gUM track %d applyConstraints, but we don't have type %s",
> -         aTrackID, aIsAudio ? "audio" : "video"));
> +         aTrackID, aTrackID == kAudioTrack ? "audio" : "video"));
>      p->Resolve(false);
>      return p.forget();
>    }
>  
>    // XXX to support multiple tracks of a type in a stream, this should key off
>    // the TrackID and not just the type
> -  RefPtr<AudioDevice> audioDevice = aIsAudio ? mAudioDevice.get() : nullptr;
> -  RefPtr<VideoDevice> videoDevice = !aIsAudio ? mVideoDevice.get() : nullptr;
> +  RefPtr<AudioDevice> audioDevice =
> +    aTrackID == kAudioTrack ? mAudioDevice.get() : nullptr;
> +  RefPtr<VideoDevice> videoDevice =
> +    aTrackID == kVideoTrack ? mVideoDevice.get() : nullptr;

Now that this is cleaner, it seems obvious that this is just:

    if (mStopped || (!audioDevice && !videoDevice))

(if we move the if below the RefPtrs)

Comment 115

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #76)
> Or CreateDOMTrack()/CloneDOMTrack() for
> CreateOwnDOMTrack()/CreateClonedDOMTrack()?

+1

Comment 116

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681204 [details]
MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23775/#review21443

Crashes I think.

::: dom/media/DOMMediaStream.h:524
(Diff revision 1)
> +  already_AddRefed<MediaStreamTrack> CreateClonedDOMTrack(MediaStreamTrack& aClonedTrack,
> +                                                          TrackID aCloneTrackID);

CloneDOMTrack sounds good.

::: dom/media/DOMMediaStream.cpp:816
(Diff revision 1)
> +  TrackID inputTrackID = aOriginalTrack.mInputTrackID;
> +  MediaStream* inputStream = aOriginalTrack.GetInputStream();
> +
> +  CombineWithPrincipal(aOriginalTrack.GetPrincipal());
> +
> +  MediaStreamTrackSource& source = aOriginalTrack.GetSource();
> +  RefPtr<MediaStreamTrack> track;
> +  if (aOriginalTrack.AsAudioStreamTrack()) {
> +    track = new AudioStreamTrack(this, aCloneTrackID, inputTrackID, &source);
> +  } else if (aOriginalTrack.AsVideoStreamTrack()) {
> +    track = new VideoStreamTrack(this, aCloneTrackID, inputTrackID, &source);
> +  } else {
> +    MOZ_CRASH("Unhandled track type");
> +  }
> +
> +  track->mOriginalTrack = &aOriginalTrack;

What if we're cloning a clone?

Do all of these still make sense?

mOriginalTrack will in this case not point to an original track but to another clone.

::: dom/media/MediaStreamTrack.cpp:136
(Diff revision 1)
> +  MediaStreamTrackSourceGetter* getter = nullptr; // No dynamically added tracks
> +  RefPtr<DOMMediaStream> newStream =
> +    new DOMMediaStream(mOwningStream->GetParentObject(), getter);

nit: The getter var exposition just confused me ("is it an out arg?") I would just use nullptr in the arg here, with comment.

::: dom/media/MediaStreamTrack.cpp:150
(Diff revision 1)
>    MediaStreamTrack* originalTrack =
>      mOriginalTrack ? mOriginalTrack.get() : this;
>    MOZ_RELEASE_ASSERT(originalTrack->mOwningStream);

Wont this crash if I clone another clone?

Comment 117

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23777/#review21445

I don't know what this patch is doing, or why new concepts like weakptrs are needed.

According to the spec [1] this should be as simple as:

 1. Let streamClone be a newly constructed MediaStream object.

 2. Initialize streamClone's id attribute to a newly generated value.

 3. Clone each track in this MediaStream object and add the result to streamClone's track set.


[1] http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStream-clone-MediaStream

Comment 118

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681206 [details]
MozReview Request: Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib

https://reviewboard.mozilla.org/r/23779/#review21447

Comment 119

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681208 [details]
MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback. r?jib

https://reviewboard.mozilla.org/r/23783/#review21449

::: dom/media/tests/mochitest/mediaStreamPlayback.js:26
(Diff revision 1)
> -MediaStreamPlayback.prototype = {
> +MediaStreamPlayback.prototype = Object.create(MediaStreamPlayback.prototype, {

Is this creating itself? Seems bad.

Did you copy an inheritance pattern by mistake?

Comment 120

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681209 [details]
MozReview Request: Bug 1208371 - Test DOMMediaStream::Clone(). r?jib

https://reviewboard.mozilla.org/r/23785/#review21451

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:33
(Diff revision 1)
> +      is(clone.getAudioTracks().length, stream.getAudioTracks().length,
> +         "All audio tracks should get cloned");
> +      is(clone.getVideoTracks().length, stream.getVideoTracks().length,
> +         "All video tracks should get cloned");
> +      stream.getTracks().forEach(t => ok(!clone.getTracks().includes(t),
> +                                         "The clone should contain an original track"));
> +      checkTrackCloneAgainstOriginal(clone.getAudioTracks()[0], stream.getAudioTracks()[0]);
> +      checkTrackCloneAgainstOriginal(clone.getVideoTracks()[0], stream.getVideoTracks()[0]);
> +
> +      isnot(clone.id.length, 0, "Stream clone should have an id string");
> +      isnot(clone.getAudioTracks()[0].id.length, 0,
> +            "Audio track clone should have an id string");
> +      isnot(clone.getVideoTracks()[0].id.length, 0,
> +            "Audio track clone should have an id string");

Would love to see a checkStreamCloneAgainstOriginal function for symmetry (I also see one new use for it below).

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:91
(Diff revision 1)
> +      var inceptionClone = stream.clone().clone().clone().clone().clone()
> +                                 .clone().clone().clone().clone().clone();

maybe checkStreamCloneAgainstOriginal(inceptionClone, stream); here?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html:107
(Diff revision 1)
> +      var LOOPS = 3;

const?

Comment 121

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22601/#review21389

> A lot of owning going on here.

I removed the mOwningStream since tracks also have GetOwnedStream() at this point. A little less owning.

Comment 122

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676164 [details]
MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

https://reviewboard.mozilla.org/r/22579/#review21475

Comment 123

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676165 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

https://reviewboard.mozilla.org/r/22581/#review21489

Comment 124

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/22579/#review21491

Comment 125

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22611/#review21393

> These changes are great! I'd love to push this change deeper, into the same function in PeerConnectionMedia and SourceStreamInfo, as there's a chance we'd reach the bottom and get rid of the id here entirely (but it doesn't need to be in this bug).
> 
> The distinction of a "numericTrackId" wont be missed. :)
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?rev=e8c7dfe727cd#760

I followed your suggestion but didn't go all the way to completely get rid of it.

I'll also just note that we still need some TrackIDs, e.g., to assign to received tracks.

Comment 126

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23771/#review21397

> Now that this is cleaner, it seems obvious that this is just:
> 
>     if (mStopped || (!audioDevice && !videoDevice))
> 
> (if we move the if below the RefPtrs)

Done.

Comment 127

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23775/#review21443

> What if we're cloning a clone?
> 
> Do all of these still make sense?
> 
> mOriginalTrack will in this case not point to an original track but to another clone.

Nice catch!

I can only see the mOriginalTrack assignment being wrong here; it should be aOriginalTrack.mOriginalTrack if there's one, or aOriginalTrack otherwise. I fixed this and did some renaming to avoid confusion here.

> nit: The getter var exposition just confused me ("is it an out arg?") I would just use nullptr in the arg here, with comment.

Done.

> Wont this crash if I clone another clone?

No, all tracks are owned by a DOMMediaStream.

Comment 128

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676168 [details]
MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc

https://reviewboard.mozilla.org/r/22583/#review21587

::: dom/media/MediaStreamTrack.h:26
(Diff revision 2)
> + * Common interface for all MediaStreamTrack producers.

A little more detail here would help. E.g. main-thread-only, who owns it, etc.

::: dom/media/MediaStreamTrack.h:39
(Diff revision 2)
> +   */

Please finish these comments!

Comment 129

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/22585/#review21589

Comment 130

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/22593/#review21591

Comment 131

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676174 [details]
MozReview Request: Bug 1208371 - Add convenience method for checking if TrackID is explicit. r?roc

https://reviewboard.mozilla.org/r/22595/#review21593

Comment 132

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676175 [details]
MozReview Request: Bug 1208371 - Allow MediaInputPorts mapped to a destination TrackID. r?roc

https://reviewboard.mozilla.org/r/22597/#review21597

Glad we were able to come up with a reasonably simple solution here!

::: dom/media/MediaStreamGraph.h:940
(Diff revision 2)
> + * know that this TrackID in the destination stream is available. We assert

"When we do this, we must know"

Comment 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

https://reviewboard.mozilla.org/r/22599/#review21601

Comment 134

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

https://reviewboard.mozilla.org/r/22603/#review21603

Glad to see us moving in this direction.

::: dom/media/MediaStreamTrack.h:188
(Diff revision 2)
> +   * XXX

What did you intend to write here?

::: dom/media/MediaStreamTrack.h:193
(Diff revision 2)
> +    virtual void PrincipalChanged(MediaStreamTrack* aTrack) = 0;

If you intend to follow the same ownership regime as DOMMediaStream::PrincipalChangeObserver, please comment here what that regime is. (Yes, I know it's not documented in DOMMediaStream::PrincipalChangeObserver, but it should be!)

Namely, the invariant is that a PrincipalChangeObserver must be removed from the stream by whoever added it before the observer object dies.

::: dom/media/MediaStreamTrack.h:212
(Diff revision 2)
> +  class PrincipalChangeObserverForwarder :

Explain what this class does

l

Comment 135

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676179 [details]
MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

https://reviewboard.mozilla.org/r/22605/#review21605

::: dom/media/MediaStreamTrack.h:201
(Diff revision 2)
> +   * XXX

Please fix this comment

::: dom/media/MediaStreamTrack.h:203
(Diff revision 2)
> +  class MSGListener

Can we call this MSGTrackListener to make it a little more clear?

Comment 136

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

https://reviewboard.mozilla.org/r/22609/#review21609

Comment 137

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8681203 [details]
MozReview Request: Bug 1208371 - Make it possible to block tracks in a MediaInputPort initally. r?roc

https://reviewboard.mozilla.org/r/23773/#review21611

Comment 138

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8681204 [details]
MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23775/#review21613

::: dom/media/DOMMediaStream.cpp:829
(Diff revision 1)
> +  }

It would be cleaner to give MediaStreamTrack a CloneInternal() method.

Comment 139

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23777/#review21615

Comment 140

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8681207 [details]
MozReview Request: Bug 1208371 - Forward input stream and track id on regular track changes for union streams. r?roc

https://reviewboard.mozilla.org/r/23781/#review21617

Comment 141

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23777/#review21445

Yeah but then we have some extra things to deal with, like connecting streams with input ports so that the data in the tracks we just cloned flows to the right place.

See comment 50 for the WeakPtr.

Comment 142

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23783/#review21449

> Is this creating itself? Seems bad.
> 
> Did you copy an inheritance pattern by mistake?

Hmm, I probably copied it from LocalMediaStreamPlayback. I'll just restore the old prototype and skip the getters altogether.

Comment 143

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23785/#review21451

> Would love to see a checkStreamCloneAgainstOriginal function for symmetry (I also see one new use for it below).

Good idea.

> maybe checkStreamCloneAgainstOriginal(inceptionClone, stream); here?

I also put it in the first, basic clone(), test.

Comment 144

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/23777/#review21445

Thanks for the link to comment 50. Are you saying the WeakPtrs serve no technical purpose here other than to demote use-after-free refs to nullptr crashes ? Unsure how I feel about that.

On the "extra things do deal with", don't we already have to deal with those when users do:

    var clone = new MediaStream(stream);

? If so, then I see no reason to maintain a redundant codepath. Seems like premature optimization.

Comment 145

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

https://reviewboard.mozilla.org/r/22611/#review21793

Comment 146

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22601/#review20525

> We already have CreateOwnDOMTrack and with that in mind I think CreateClonedDOMTrack makes more sense.

Still hopin' for CloneDOMTrack.

Comment 147

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #144)
> https://reviewboard.mozilla.org/r/23777/#review21445
> 
> Thanks for the link to comment 50. Are you saying the WeakPtrs serve no
> technical purpose here other than to demote use-after-free refs to nullptr
> crashes ? Unsure how I feel about that.

No, it's demoting use-after-free refs to "Ah it's been destructed, let's return early" -- in the case where we have a TrackSourceGetter on a clone referring back to the original. The clone is indirectly keeping the original alive but since it's non-obvious the WeakPtr provides extra safety. There is no ref from the original to the getter so we can't tell it to Forget() its mStream pointer like we do for the MediaStreamListeners.

For the listeners (OwnedStreamListener and PlaybackStreamListener) I wanted to use the WeakPtr as well but that didn't work because of garbage collection doing actual object destruction asynchronously, i.e., it'd call DOMMediaStream::Destroy() first, then dispatch a task to remove the last (GC-held) refs to the DOMMediaStream objects. In between this, the MSG would have a runnable run on the main thread which would call into the live DOMMediaStream (by WeakPtr), triggering asserts due to certain members being null (like mOwnedStream which should always exist).


> On the "extra things do deal with", don't we already have to deal with those
> when users do:
> 
>     var clone = new MediaStream(stream);

Hmm, how?

This is also not a clone, the new stream just adds all the tracks from the one passed in to its constructor. The tracks keep the original alive by strong refs.

Comment 148

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22583/#review21587

> A little more detail here would help. E.g. main-thread-only, who owns it, etc.

Done. Sorry for missing this when pushing the latest revision.

Comment 149

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #147)
> The clone is indirectly keeping the original alive

Why not use a strong pointer then, and let the cycle-collector break it?

> This is also not a clone, the new stream just adds all the tracks from the
> one passed in to its constructor. The tracks keep the original alive by
> strong refs.

That's what we want I think. Are you working off some other definition of 'clone' than http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStream-clone-MediaStream ?

Comment 150

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry, I meant:

    var clone = new MediaStream(stream.getTracks().map(track => track.clone());

Comment 151

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22603/#review21603

> What did you intend to write here?

It is now:

```
Get this track's principal.

This is currently the same principal as for the track's owning
DOMMediaStream.
```

Comment 152

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22603/#review21603

> If you intend to follow the same ownership regime as DOMMediaStream::PrincipalChangeObserver, please comment here what that regime is. (Yes, I know it's not documented in DOMMediaStream::PrincipalChangeObserver, but it should be!)
> 
> Namely, the invariant is that a PrincipalChangeObserver must be removed from the stream by whoever added it before the observer object dies.

Comments added. I also threw in the same comments for DOMMediaStream::Add/RemovePrincipalChangeObserver.

Comment 153

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #149)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #147)
> > The clone is indirectly keeping the original alive
> 
> Why not use a strong pointer then, and let the cycle-collector break it?

I couldn't get a native cycle collected inherited class working. :/


> Are you working off some other definition of
> 'clone' than
> http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStream-
> clone-MediaStream ?

No. It feels like we're going in circles around each other here.. I'd like to understand what you mean in comment 144 by "don't we already have to deal with those". Which, specifically?

Comment 154

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23775/#review21613

> It would be cleaner to give MediaStreamTrack a CloneInternal() method.

Done!

Comment 155

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/22591/#review20425

> Hmm, not ProcessedMSGMediaStream, TrackUnionMSGStream and SourceMSGStream? Bike-shedding time!
> 
> Did anyone like MediaStreamNode? i.e. the "nodes in a graph" in a MediaStreamGraph? I think the root of the confusion is that both concepts are "streams" colloquially, and this would address that (if unfairly since they came first). It would also highlight that the implementation objects are in fact nodes in a graph, and maybe also that a single "stream" of data can run through more than one of these objects. It's also easier to compose on:
> 
> MediaStreamNode
> ProcessedMediaStreamNode
> TrackUnionStreamNode
> SourceStreamNode
> 
> Thoughts?

I'll create a new bug for the renaming.

Comment 156

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676157 [details]
MozReview Request: Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22567/diff/2-3/

Comment 157

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676159 [details]
MozReview Request: Bug 1208371 - Make AudioCaptureStream startable. r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22569/diff/2-3/

Comment 158

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676160 [details]
MozReview Request: Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22571/diff/2-3/

Comment 159

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676161 [details]
MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22573/diff/2-3/

Comment 160

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676162 [details]
MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22575/diff/2-3/

Comment 161

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676163 [details]
MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22577/diff/2-3/

Comment 162

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676164 [details]
MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22579/diff/2-3/

Comment 163

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676165 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22581/diff/2-3/

Comment 164

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676168 [details]
MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22583/diff/2-3/

Comment 165

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676169 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22585/diff/2-3/

Comment 166

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676170 [details]
MozReview Request: Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22587/diff/2-3/

Comment 167

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676171 [details]
MozReview Request: Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22589/diff/2-3/

Comment 168

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676172 [details]
MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22591/diff/2-3/

Comment 169

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676173 [details]
MozReview Request: Bug 1208371 - Count the users of a MediaStream to ease Destroy() responsibility. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22593/diff/2-3/

Comment 170

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676174 [details]
MozReview Request: Bug 1208371 - Add convenience method for checking if TrackID is explicit. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22595/diff/2-3/

Comment 171

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676175 [details]
MozReview Request: Bug 1208371 - Allow MediaInputPorts mapped to a destination TrackID. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22597/diff/2-3/

Comment 172

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22599/diff/2-3/

Comment 173

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676177 [details]
MozReview Request: Bug 1208371 - Let DOMMediaStream base its principal on the tracks it contains. r?mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22601/diff/2-3/

Comment 174

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22603/diff/2-3/

Comment 175

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676179 [details]
MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22605/diff/2-3/

Comment 176

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676157 [details]
MozReview Request: Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22567/diff/2-3/

Comment 177

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676180 [details]
MozReview Request: Bug 1208371 - Make it possible to look up stream id by track in PeerConnectionImpl. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22607/diff/2-3/

Comment 178

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22609/diff/2-3/

Comment 179

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22611/diff/2-3/

Comment 180

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681202 [details]
MozReview Request: Bug 1208371 - Route ApplyConstraints through MediaStreamTrackSource. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23771/diff/1-2/

Comment 181

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681203 [details]
MozReview Request: Bug 1208371 - Make it possible to block tracks in a MediaInputPort initally. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23773/diff/1-2/

Comment 182

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681204 [details]
MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23775/diff/1-2/

Comment 183

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23777/diff/1-2/

Comment 184

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681206 [details]
MozReview Request: Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23779/diff/1-2/

Comment 185

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681207 [details]
MozReview Request: Bug 1208371 - Forward input stream and track id on regular track changes for union streams. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23781/diff/1-2/

Comment 186

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681208 [details]
MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23783/diff/1-2/

Comment 187

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681209 [details]
MozReview Request: Bug 1208371 - Test DOMMediaStream::Clone(). r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23785/diff/1-2/

Comment 188

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Rename CreateOwnDOMTrack/CreateClonedDOMTrack to CreateDOMTrack/CloneDOMTrack. r?jib

Bug 1208371 - Rename CreateOwnDOMTrack/CreateClonedDOMTrack to CreateDOMTrack/CloneDOMTrack. r?jib

Comment 189

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #153)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #149)
> > Are you working off some other definition of 'clone' than
> > http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStream-clone-MediaStream ?
> 
> No. It feels like we're going in circles around each other here.. I'd like
> to understand what you mean in comment 144 by "don't we already have to deal
> with those". Which, specifically?

It is needed for what I mentioned in comment 150:

    var clone = new MediaStream(stream.getTracks().map(track => track.clone());

This already works, is a shoe-in for clone, and the spec says we MUST implement clone this way.

clone

    Clones the given MediaStream and all its tracks.

    When the MediaStream.clone() method is invoked, the User Agent MUST run the following steps:

        Let streamClone be a newly constructed MediaStream object.

        Initialize streamClone's id attribute to a newly generated value.

        Clone each track in this MediaStream object and add the result to streamClone's track set.

In https://reviewboard.mozilla.org/r/23777/#review21445 you seem to be saying there are arguments for deviating from the spec here, but I don't understand what they are, since comment 150 already works. Fewer code-paths is good.

Comment 190

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/23777/#review21445

> var clone = new MediaStream(stream);

This was a typo mistake on my part. Here I meant:

    var clone = new MediaStream(stream.getTracks().map(track => track.clone());

Comment 191

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681208 [details]
MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback. r?jib

https://reviewboard.mozilla.org/r/23783/#review21933

Comment 192

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23777/#review21937

::: dom/media/DOMMediaStream.cpp:576
(Diff revision 2)
> +already_AddRefed<DOMMediaStream>
> +DOMMediaStream::Clone()
> +{
> +  class ClonedStreamSourceGetter : public MediaStreamTrackSourceGetter
> +  {
> +  public:
> +    ClonedStreamSourceGetter(DOMMediaStream* aStream)
> +      : mStream(aStream) {}

(Repeating issue for mozReview's benefit, since top descriptions apparently don't count as issues.)

According to spec [1] clone MUST be implemented as:

    Let streamClone be a newly constructed MediaStream object.

    Initialize streamClone's id attribute to a newly generated value.

    Clone each track in this MediaStream object and add the result to streamClone's track set.

So this seems implementable entirely using existing code, i.e. the c++ that covers:

    var clone = new MediaStream(stream.getTracks().map(track => track.clone());

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStream-clone-MediaStream

::: dom/media/DOMMediaStream.cpp:724
(Diff revision 2)
> +  mOwnedStream->RegisterUser();

Is the register/unregister thing needed if we do it the spec way?

Comment 193

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8683551 [details]
MozReview Request: Bug 1208371 - Rename CreateOwnDOMTrack/CreateClonedDOMTrack to CreateDOMTrack/CloneDOMTrack. r?jib

https://reviewboard.mozilla.org/r/24365/#review21939

Comment 194

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676162 [details]
MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

https://reviewboard.mozilla.org/r/22575/#review21959

Comment 195

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676168 [details]
MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc

https://reviewboard.mozilla.org/r/22583/#review21961

Comment 196

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676172 [details]
MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

https://reviewboard.mozilla.org/r/22591/#review21963

Comment 197

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676179 [details]
MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

https://reviewboard.mozilla.org/r/22605/#review21973

Comment 198

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

https://reviewboard.mozilla.org/r/22603/#review21975

Comment 199

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/22603/#review21977

Comment 200

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23777/#review21937

> (Repeating issue for mozReview's benefit, since top descriptions apparently don't count as issues.)
> 
> According to spec [1] clone MUST be implemented as:
> 
>     Let streamClone be a newly constructed MediaStream object.
> 
>     Initialize streamClone's id attribute to a newly generated value.
> 
>     Clone each track in this MediaStream object and add the result to streamClone's track set.
> 
> So this seems implementable entirely using existing code, i.e. the c++ that covers:
> 
>     var clone = new MediaStream(stream.getTracks().map(track => track.clone());
> 
> [1] http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStream-clone-MediaStream

Right, I see what you mean now. Good that you brought this up.

The thing we are doing differently here is also setting up a generic MediaInputPort between the original and the clone if the original has an input stream, to ensure that dynamically added tracks show up in both.

Nothing in mediacapture-main gets tracks dynamically added by the user agent. There are "addtrack" and "removetrack" events for a MediaStream, but clone() seems to  have been overseen in this regard. At least I see value in having dynamically added tracks appear in the clones as well, see more below.

PeerConnections can do it, but are regulated in another spec, [2].

Our main use case for this is HTMLMediaElement::CaptureStream where one gets a stream that sees its entire track set swapped out when the media element changes source. See mediacapture-fromelement at [3].

There's some backstory here, though not too much, see bug 1170958 comment 57, bug 1170958 comment 114, and bug 1170958 comment 117.

I think we should decide whether to set up a generic input port when cloning streams and if needed bring this to the spec.

I vote for setting it up, but I guess jib and mt are the main peers for the spec here so what do you think we should do?

I'll also note that there's no real value to stream cloning without this, since one can do the same thing in JS with a MediaStream constructor and track cloning, per jib's example above.


[2] http://w3c.github.io/webrtc-pc/
[3] https://w3c.github.io/mediacapture-fromelement/index.html

> Is the register/unregister thing needed if we do it the spec way?

Yeah, DOMMediaStream::mInputStream is still shared across track clones.

Comment 201

[Andreas Pehrson [:pehrsons]]

Also, some security considerations.

(Bug 942367) Martin, how should we handle DOMMediaStream::mPeerIdentity on stream.clone()? addTrack/removeTrack?

(Bug 937718) Paul, how should we handle DOMMediaStream::mCORSMode on stream.clone()? addTrack/removeTrack?

Comment 202

[Andreas Pehrson [:pehrsons]]

There's apparently some magic in `GetParentObject()` that I couldn't find docs for. Restoring it for MediaStreamTrack to return the owning window solved most of these: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73778cb5a31

Comment 203

[Paul Adenot (:padenot)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #201)
> Also, some security considerations.
> 
> (Bug 942367) Martin, how should we handle DOMMediaStream::mPeerIdentity on
> stream.clone()? addTrack/removeTrack?
> 
> (Bug 937718) Paul, how should we handle DOMMediaStream::mCORSMode on
> stream.clone()? addTrack/removeTrack?

I'm usually asking Martin himself when touching CORS things, so I guess he can answer both questions here.

Comment 204

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/23777/#review21937

> Right, I see what you mean now. Good that you brought this up.
> 
> The thing we are doing differently here is also setting up a generic MediaInputPort between the original and the clone if the original has an input stream, to ensure that dynamically added tracks show up in both.
> 
> Nothing in mediacapture-main gets tracks dynamically added by the user agent. There are "addtrack" and "removetrack" events for a MediaStream, but clone() seems to  have been overseen in this regard. At least I see value in having dynamically added tracks appear in the clones as well, see more below.
> 
> PeerConnections can do it, but are regulated in another spec, [2].
> 
> Our main use case for this is HTMLMediaElement::CaptureStream where one gets a stream that sees its entire track set swapped out when the media element changes source. See mediacapture-fromelement at [3].
> 
> There's some backstory here, though not too much, see bug 1170958 comment 57, bug 1170958 comment 114, and bug 1170958 comment 117.
> 
> I think we should decide whether to set up a generic input port when cloning streams and if needed bring this to the spec.
> 
> I vote for setting it up, but I guess jib and mt are the main peers for the spec here so what do you think we should do?
> 
> I'll also note that there's no real value to stream cloning without this, since one can do the same thing in JS with a MediaStream constructor and track cloning, per jib's example above.
> 
> 
> [2] http://w3c.github.io/webrtc-pc/
> [3] https://w3c.github.io/mediacapture-fromelement/index.html

Thanks, I hadn't considered that. Essentially: "do addtrack and removetrack events fire on cloned streams?"

Good question. Starting with removetrack was helpful for me:

The spec allows UAs to pull tracks at will [1], so when a PeerConnection renegotiates away a media source, or an audio element with capture disables an audio source, removetrack clearly MUST be fired on all streams (originals and clones) that used to contain a track (original or clone!) from that source.

But addtrack is tied to the stream, so here it seems we have a choice. I no longer think it's a mediacapture spec issue, since [1] seems to allow UAs to add tracks anywhere to any stream at any time.

Our options seem to be:

1. addtrack only fires on originals. People must re-acquire host stream, or re-clone original
   to get updates. Maybe remove .clone() on MediaStream to avoid confusion.

   pros: Simple. Why clone if things keep behaving like the original?
   cons: Awkward to deal with .captureStream and remote streams?

2. addtrack fires on clones as well. Watch out for surprise tracks popping up, and wonder
   if stream.clone() and new MediaStream(stream) do the same thing.

   pros: Symmetry, powerful
   cons: Two kinds of clones, pc.addStream(stream) still wont get future tracks.

Lets say it's (1). What problems would that cause? What good use-cases does it hamper or break? Listing those may help.

I understand that with (2) we get it both ways, with new MediaStream(stream.getTracks()) being the opt-out, but at a complexity cost.

My main concern is: what's most predictable?

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#mediastreamtrackevent

Comment 205

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23777/#review22035

I think we should still resolve comment 204, but if we file a follow-up bug for that, and if Martin agrees, then I'm OK with landing this.

Comment 206

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681204 [details]
MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23775/#review22037

Comment 207

[Martin Thomson [:mt:]]

(In reply to Paul Adenot (:padenot) from comment #203)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #201)
> > Also, some security considerations.
> > 
> > (Bug 942367) Martin, how should we handle DOMMediaStream::mPeerIdentity on
> > stream.clone()? addTrack/removeTrack?
> > 
> > (Bug 937718) Paul, how should we handle DOMMediaStream::mCORSMode on
> > stream.clone()? addTrack/removeTrack?
> 
> I'm usually asking Martin himself when touching CORS things, so I guess he
> can answer both questions here.

The best option would be to move these properties to the tracks.  They are properly track properties.

Comment 208

[Andreas Pehrson [:pehrsons]]

Martin, please also see comment 200.

Comment 209

[Martin Thomson [:mt:]]

Hmm, I don't think that you need to concern yourself with propagating add/remove on cloned streams.  The best option (to my mind) would be to clone with the current state of the stream.  That's going to be the least surprising option in many cases.  For other cases, applications can perform their own propagation of added and removed tracks.

Comment 210

[Andreas Pehrson [:pehrsons]]

There are some issues with that approach that I'd like to highlight and want to hear your comments on Martin. Also ni-ing jib in case he has any new thoughts.

If applications perform their own propagation there's a risk that data goes missing because events are async. I.e., there's a gap from when a track first appears in a stream until that track is exposed to JS.

This is not so much of a problem for realtime streams (gUM) but if you're capturing a media element you may get a noticable part missing in the very start after changing sources.

I'd also like to argue that if a clone propagates UA-added/removed tracks they're easy to avoid should one want it (`new MediaStream(stream.getTracks().forEach(t => t.clone))`).

Likewise if we don't propagate to a clone, stream cloning could just be done like the snippet above. If we stay on this route I think the api for stream cloning should be removed from the spec to keep it simple since it doesn't add any value.

Like jib said in comment 204 these are our two options, and I believe the missed data I mentioned above is the main downside to doing js-side propagation. Regarding predictability I think both cases make sense and I don't think surprises is that much of a concern if the behavior is just explained in docs. All developers read docs, right?

Comment 211

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #210)
> if you're capturing a media element you may get a noticable part missing
> in the very start after changing sources.

Doesn't this presume a sink that automatically picks up on new tracks in a stream (otherwise a visit to JS seems inevitable)? What's an example of this? PeerConnection is track-based now, so it doesn't seem to qualify.

Also, in these cases, what use-case is not covered by using the original stream?

> I'd also like to argue that if a clone propagates UA-added/removed tracks
> they're easy to avoid should one want it (`new
> MediaStream(stream.getTracks().forEach(t => t.clone))`).

Sure, though that's not an argument for keeping it.

> Likewise if we don't propagate to a clone, stream cloning could just be done
> like the snippet above. If we stay on this route I think the api for stream
> cloning should be removed from the spec to keep it simple since it doesn't
> add any value.

It's hard to argue that stream.clone() is in the spec for any reason other than convenience, given that it's defined as a redundancy.

That said, I think removing stream.clone() is an opportunity to reduce confusion if hosts are going to be removing tracks from clones yet never replenish them with new ones.

Comment 212

[Martin Thomson [:mt:]]

I think that we should take this discussion to the media capture list.  I can see now why the model Andreas suggests has some value, but if we don't have clarity in the spec, I don't see any value in doing the extra work.

Is it possible to include that part of this work in a follow-up bug?  Then we can track that extra piece independently.

Comment 213

[Martin Thomson [:mt:]]

Opened: https://github.com/w3c/mediacapture-main/issues/271

Comment 214

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Great to raise the issue, but I don't think, as I mentioned in comment 204, that we're blocked on that spec here, since from my reading [1] seems to allow UAs to add tracks anywhere to any stream at any time.

The way I read it, another spec can add tracks even to clones from e.g. new MediaStream(stream) if it wants, i.e. how a clone is created might not be significant.

In other words, I'm not sure the language on stream.clone() precludes hosts from adding tracks to clones (but I could be wrong). 

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#mediastreamtrackevent

Comment 215

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #214)
> Great to raise the issue, but I don't think, as I mentioned in comment 204,
> that we're blocked on that spec here, since from my reading [1] seems to
> allow UAs to add tracks anywhere to any stream at any time.

I don't think that having one browser propagate track changes to clones and another not doing so is a recipe for interoperability.  We should decide, collectively, what the best option is and do that.  Right now, the status quo is to not propagate changes, though I will acknowledge the uncertainty that the text you cite implies.

Comment 216

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Agree, I'm saying this might be specified in the specs that introduce host-managed tracks, rather than in the mediastream spec which has none.

Comment 217

[Andreas Pehrson [:pehrsons]]

Thanks for bringing it to the spec guys.

While moving the principal stuff I ran into an issue that I'd like some advice on how best to resolve. Martin, perhaps?

I now have patches that move nsPrincipal, PeerIdentity and CORSMode from DOMMediaStream to MediaStreamTrack. DOMMediaStream still has a principal because of APIs that consumes it - it is essentially a combination of all its current tracks and their principal changes from the point the stream started to be consumed.

Now WebRTC had special access to updating the principal on a stream, not by combining but by replacing it. With that moved to the track the track gets the new principal and signals it to the stream, which combines it with its old one, still having the stream content inaccessible to script.

I could fix this by always having the streams principal be the combination of all of its tracks' one. But then what would happen if we remove the only video track (with system principal) from a stream played in a media element? We'd make its last rendered frame accessible to anyone (given that the remaining audio tracks are).

We had a similar bug for the same reason where the MediaRecorder would throw a security error because the stream's contents were inaccessible. Any thoughts on how to solve this?

I see an ideal world where this info is passed along with the actual audio/video data. That would simplify a lot. But its a major change and principals with all their nsCOMPtrs and stuff look scary to pass to and use on the MSG thread, etc.

Comment 218

[Martin Thomson [:mt:]]

I think that - where possible - we should be relying on track principals for access checks.  Obviously there are a number of places where we can't do that, but I would rather fail than create a situation where the last video frame is leaked.  What you describe (a cumulative combining at DOMMediaStream) will fail more than is ideal, but that is not necessarily bad.

That means that you should take at least a cursory look at the sites where the DOMMediaStream principal is checked to see if it can (or should) be changed to check the tracks instead.  For instance, rendering a video tag to canvas could be trivially simplified; MediaRecorder can also trivially look at tracks.

Note, I don't think that this needs to be moved to the MSG thread, but some care will need to be taken at the point where changes are made.

Comment 219

[Andreas Pehrson [:pehrsons]]

(In reply to Martin Thomson [:mt:] from comment #218)
> I think that - where possible - we should be relying on track principals for
> access checks.  Obviously there are a number of places where we can't do
> that, but I would rather fail than create a situation where the last video
> frame is leaked.  What you describe (a cumulative combining at
> DOMMediaStream) will fail more than is ideal, but that is not necessarily
> bad.
> 
> That means that you should take at least a cursory look at the sites where
> the DOMMediaStream principal is checked to see if it can (or should) be
> changed to check the tracks instead.  For instance, rendering a video tag to
> canvas could be trivially simplified;

Point taken, and I have an idea that I think works. I hope I have it up for review to you before the end of the week. Though to make it trivial with media elements we have to implement explicit track selection. There's an implementation of tracks behind a pref today but afaik it doesn't do anything to rendering.


> MediaRecorder can also trivially look at tracks.

This on the other hand should be trivial already today.


> Note, I don't think that this needs to be moved to the MSG thread, but some
> care will need to be taken at the point where changes are made.

Comment 220

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676162 [details]
MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22575/diff/3-4/

Comment 221

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676168 [details]
MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22583/diff/3-4/

Comment 222

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676169 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22585/diff/3-4/

Comment 223

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676170 [details]
MozReview Request: Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22587/diff/3-4/

Comment 224

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676171 [details]
MozReview Request: Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22589/diff/3-4/

Comment 225

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676172 [details]
MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22591/diff/3-4/

Comment 226

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676173 [details]
MozReview Request: Bug 1208371 - Count the users of a MediaStream to ease Destroy() responsibility. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22593/diff/3-4/

Comment 227

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676174 [details]
MozReview Request: Bug 1208371 - Add convenience method for checking if TrackID is explicit. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22595/diff/3-4/

Comment 228

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676175 [details]
MozReview Request: Bug 1208371 - Allow MediaInputPorts mapped to a destination TrackID. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22597/diff/3-4/

Comment 229

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22599/diff/3-4/

Comment 230

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676165 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22581/diff/3-4/

Comment 231

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22603/diff/3-4/

Comment 232

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add principal APIs to MediaStreamTrack. r?mt

Review commit: https://reviewboard.mozilla.org/r/28729/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28729/

Comment 233

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Create MediaStreamTrackSource::Sink that forwards principal changes from the source. r?mt

Review commit: https://reviewboard.mozilla.org/r/28731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28731/

Comment 234

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add a generic PrincipalChangeObserver interface. r?mt

Review commit: https://reviewboard.mozilla.org/r/28733/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28733/

Comment 235

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Add an interface DecoderPrincipalChangeObserver to HTMLMediaElement. r?roc

Review commit: https://reviewboard.mozilla.org/r/28735/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28735/

Comment 236

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make HTMLMediaElement::CaptureStream pass its principal to MediaStreamTrack. r?mt

Review commit: https://reviewboard.mozilla.org/r/28737/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28737/

Comment 237

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make MediaManager pass its principal to MediaStreamTrack for gUM. r?mt

Review commit: https://reviewboard.mozilla.org/r/28739/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28739/

Comment 238

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make ImageCapture listen to principal changes of MediaStreamTrack instead. r?mt

Review commit: https://reviewboard.mozilla.org/r/28741/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28741/

Comment 239

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make PeerConnectionImpl pass its principal to MediaStreamTrack through a new RemoteTrackSource. r?mt

Review commit: https://reviewboard.mozilla.org/r/28743/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28743/

Comment 240

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1208371 - Make remaining DOMMediaStream principal sources use MediaStreamTrack. r?mt

Review commit: https://reviewboard.mozilla.org/r/28745/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28745/

Comment 241

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676177 [details]
MozReview Request: Bug 1208371 - Let DOMMediaStream base its principal on the tracks it contains. r?mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22601/diff/3-4/

Comment 242

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676179 [details]
MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22605/diff/3-4/

Comment 243

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676180 [details]
MozReview Request: Bug 1208371 - Make it possible to look up stream id by track in PeerConnectionImpl. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22607/diff/3-4/

Comment 244

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22609/diff/3-4/

Comment 245

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22611/diff/3-4/

Comment 246

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681202 [details]
MozReview Request: Bug 1208371 - Route ApplyConstraints through MediaStreamTrackSource. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23771/diff/2-3/

Comment 247

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681203 [details]
MozReview Request: Bug 1208371 - Make it possible to block tracks in a MediaInputPort initally. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23773/diff/2-3/

Comment 248

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681204 [details]
MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23775/diff/2-3/

Comment 249

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23777/diff/2-3/

Comment 250

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681206 [details]
MozReview Request: Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23779/diff/2-3/

Comment 251

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681207 [details]
MozReview Request: Bug 1208371 - Forward input stream and track id on regular track changes for union streams. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23781/diff/2-3/

Comment 252

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681208 [details]
MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23783/diff/2-3/

Comment 253

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681209 [details]
MozReview Request: Bug 1208371 - Test DOMMediaStream::Clone(). r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23785/diff/2-3/

Comment 254

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8683551 [details]
MozReview Request: Bug 1208371 - Rename CreateOwnDOMTrack/CreateClonedDOMTrack to CreateDOMTrack/CloneDOMTrack. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24365/diff/1-2/

Comment 255

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8700527 [details]
MozReview Request: Bug 1208371 - Make ImageCapture listen to principal changes of MediaStreamTrack instead. r?mt

Review commit: https://reviewboard.mozilla.org/r/28741/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28741/

Comment 256

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8676179 [details]
MozReview Request: Bug 1208371 - Move ImageCapture to a MediaStreamTrackListener. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22605/diff/3-4/

Comment 257

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681202 [details]
MozReview Request: Bug 1208371 - Route ApplyConstraints through MediaStreamTrackSource. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23771/diff/3-4/