Closed Bug 1208371 Opened 4 years ago Closed 4 years ago

Implement MediaStream/MediaStreamTrack.clone()

Categories

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

33 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed
Blocking Flags:

People

(Reporter: pehrsons, Assigned: pehrsons, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(106 files, 3 obsolete files)

40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
padenot
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
jib
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
jib
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
jib
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
roc
: review+
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
roc
: review+
jib
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
roc
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
jib
: review+
smaug
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
jib
: review+
smaug
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
roc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
pkerr
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
jesup
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
jesup
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
jesup
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
jesup
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
jesup
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
jesup
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
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.
Bug 1208371 - Pass parent window to DOMMediaStream through constructor. r?roc
Attachment #8676157 - Flags: review?(roc)
Bug 1208371 - Make AudioCaptureStream startable. r?padenot
This allows us to add the JS-side MediaStreamTrack before the MSG-side
track.
Attachment #8676159 - Flags: review?(padenot)
Bug 1208371 - Expose TrackPort in DOMMediaStream.h r?roc
Attachment #8676160 - Flags: review?(roc)
Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc
So it can be forward declared.
Attachment #8676161 - Flags: review?(roc)
Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib
Attachment #8676162 - Flags: review?(jib)
Bug 1208371 - Introduce MediaStreamTrack logs. r?roc
Attachment #8676163 - Flags: review?(roc)
Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib
Attachment #8676164 - Flags: review?(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.
Attachment #8676165 - Flags: review?(roc)
Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc
This allows DOMMediaStream to assign MediaStreamTrackSources to
dynamically created MediaStreamTracks.
Attachment #8676168 - Flags: review?(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()`).
Attachment #8676169 - Flags: review?(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.
Attachment #8676170 - Flags: review?(roc)
Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc
Attachment #8676171 - Flags: review?(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.
Attachment #8676172 - Flags: review?(roc)
Attachment #8676172 - Flags: review?(jib)
Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc
Attachment #8676173 - Flags: review?(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.
Attachment #8676174 - Flags: review?(roc)
Bug 1208371 - Allow allocating input ports based on not-yet-initialized input ports. r?roc
Attachment #8676175 - Flags: review?(roc)
Bug 1208371 - Pass MediaStreamTrack to ApplyConstraintsToTrack instead of TrackID. r?jib
Attachment #8676176 - Flags: review?(jib)
Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc
Attachment #8676177 - Flags: review?(roc)
Attachment #8676177 - Flags: review?(jib)
Attachment #8676177 - Flags: review?(bugs)
Bug 1208371 - Implement MediaStream.clone() r?smaug,jib,roc
Attachment #8676178 - Flags: review?(roc)
Attachment #8676178 - Flags: review?(jib)
Attachment #8676178 - Flags: review?(bugs)
Bug 1208371 - Forward applyConstraints() to original track for clones. r?jib
Attachment #8676179 - Flags: review?(jib)
Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib
Attachment #8676180 - Flags: review?(jib)
Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback and wrap methods in getters. r?jib
Attachment #8676181 - Flags: review?(jib)
Bug 1208371 - Test MediaStream.clone(). r?jib
Attachment #8676182 - Flags: review?(jib)
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 on attachment 8676162 [details]
MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

https://reviewboard.mozilla.org/r/22575/#review20101
Attachment #8676162 - Flags: review?(jib) → review+
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>
Attachment #8676177 - Flags: review?(bugs) → review+
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.
Attachment #8676178 - Flags: review?(bugs) → review+
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.
Attachment #8676164 - Flags: review?(jib)
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? :)
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.
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 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 on attachment 8676160 [details]
MozReview Request: Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc

https://reviewboard.mozilla.org/r/22571/#review20211
Comment on attachment 8676161 [details]
MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

https://reviewboard.mozilla.org/r/22573/#review20213
Attachment #8676161 - Flags: review?(roc) → review+
Comment on attachment 8676163 [details]
MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

https://reviewboard.mozilla.org/r/22577/#review20217
Attachment #8676163 - Flags: review?(roc) → review+
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.
Attachment #8676165 - Flags: review?(roc)
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?
Attachment #8676168 - Flags: review?(roc)
Comment on attachment 8676169 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc

https://reviewboard.mozilla.org/r/22585/#review20223
Attachment #8676169 - Flags: review?(roc) → review+
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
Attachment #8676170 - Flags: review?(roc) → review+
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 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
Attachment #8676173 - Flags: review?(roc) → review+
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
Attachment #8676177 - Flags: review?(roc) → review+
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?
Attachment #8676178 - Flags: review?(roc)
(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 on attachment 8676159 [details]
MozReview Request: Bug 1208371 - Make AudioCaptureStream startable. r?padenot

https://reviewboard.mozilla.org/r/22569/#review20261
Attachment #8676159 - Flags: review?(padenot) → review+
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.
Attachment #8676172 - Flags: review?(jib)
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?
(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.
Flags: needinfo?(roc)
(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.
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.
(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.
(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.
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.
https://reviewboard.mozilla.org/r/22591/#review20429

> Also, can't mOwningDOMStream be const?

No, because of cycle collection.
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 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 on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

https://reviewboard.mozilla.org/r/22599/#review20523
Attachment #8676176 - Flags: review+
Attachment #8676177 - Flags: review?(jib) → review+
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 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
Attachment #8676178 - Flags: review?(jib)
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?
Attachment #8676179 - Flags: review?(jib)
Attachment #8676180 - Flags: review?(jib) → review+
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 on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

https://reviewboard.mozilla.org/r/22609/#review20533
Attachment #8676181 - Flags: review?(jib) → review+
(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?
Flags: needinfo?(roc)
(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.
Attachment #8676182 - Flags: review?(jib) → review+
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?
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 on attachment 8676164 [details]
MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

https://reviewboard.mozilla.org/r/22579/#review20629
Attachment #8676164 - Flags: review?(jib) → review+
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.
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.
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.
(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?
(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.
(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. :-)
Depends on: 1220047
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.
(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.
(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

:-)
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 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 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 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.
Attachment #8676160 - Attachment description: MozReview Request: Bug 1208371 - Expose TrackPort in DOMMediaStream.h r?roc → MozReview Request: Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc
Comment on attachment 8676161 [details]
MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib

Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib
Attachment #8676161 - Attachment description: MozReview Request: Bug 1208371 - Move OnTracksAvailableCallback out of DOMMediaStream. r?roc → MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib
Comment on attachment 8676162 [details]
MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib

Bug 1208371 - Introduce MediaStreamTrack logs. r?roc
Attachment #8676162 - Attachment description: MozReview Request: Bug 1208371 - Remove unused MediaManager::NotifyMediaStreamTrackEnded. r?jib → MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc
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
Attachment #8676163 - Attachment description: MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc → MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib
Attachment #8676163 - Flags: review?(jib)
Attachment #8676164 - Attachment description: MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib → MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc
Attachment #8676164 - Flags: review?(roc)
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 on attachment 8676165 [details]
MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib

Bug 1208371 - Make DOMMediaStream support WeakPtrs. r?roc
Attachment #8676165 - Attachment description: MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc → MozReview Request: Bug 1208371 - Make DOMMediaStream support WeakPtrs. r?roc
Attachment #8676165 - Flags: review?(roc)
Attachment #8676168 - Attachment description: MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc → MozReview Request: Bug 1208371 - Add a MediaStreamTrackSource interface. r?roc
Attachment #8676168 - Flags: review?(roc)
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 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.
Attachment #8676169 - Attachment description: MozReview Request: Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc → MozReview Request: Bug 1208371 - Add MediaStreamTrackSourceGetter interface. r?roc
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()`).
Attachment #8676170 - Attachment description: MozReview Request: Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc → MozReview Request: Bug 1208371 - Let MediaStreamTracks know their TrackID at the source. r?roc
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.
Attachment #8676171 - Attachment description: MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc → MozReview Request: Bug 1208371 - Let FindOwnedDOMTrack operate on input stream. r?roc
Attachment #8676172 - Attachment description: MozReview Request: Bug 1208371 - Introduce an unbound state for MediaStreamTracks r?roc,jib → MozReview Request: Bug 1208371 - Add some MediaStreamTrack helper methods. r?roc
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 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
Attachment #8676173 - Attachment description: MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc → MozReview Request: Bug 1208371 - Count the users of a MediaStream to ease Destroy() responsibility. r?roc
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
Attachment #8676174 - Attachment description: MozReview Request: Bug 1208371 - Allow MediaInputPorts to be created before the source's TrackID is known. r?roc → MozReview Request: Bug 1208371 - Add convenience method for checking if TrackID is explicit. r?roc
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.
Attachment #8676175 - Attachment description: MozReview Request: Bug 1208371 - Allow allocating input ports based on not-yet-initialized input ports. r?roc → MozReview Request: Bug 1208371 - Allow MediaInputPorts mapped to a destination TrackID. r?roc
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
Attachment #8676176 - Attachment description: MozReview Request: Bug 1208371 - Pass MediaStreamTrack to ApplyConstraintsToTrack instead of TrackID. r?jib → MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc
Attachment #8676176 - Flags: review?(roc)
Attachment #8676177 - Attachment description: MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc → MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib
Attachment #8676177 - Flags: review+
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 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.
Attachment #8676178 - Attachment description: MozReview Request: Bug 1208371 - Implement MediaStream.clone() r?smaug,jib,roc → MozReview Request: Bug 1208371 - Add a PrincipalChangeObserver interface to MediaStreamTrack. r?roc
Attachment #8676178 - Flags: review+ → review?(roc)
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.
Attachment #8676179 - Attachment description: MozReview Request: Bug 1208371 - Forward applyConstraints() to original track for clones. r?jib → MozReview Request: Bug 1208371 - Add an MSGListener interface to MediaStreamTrack. r?roc
Attachment #8676179 - Flags: review?(roc)
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.
Attachment #8676180 - Attachment description: MozReview Request: Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib → MozReview Request: Bug 1208371 - Make it possible to look up stream id by track in PeerConnectionImpl. r?jib
Comment on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc
Attachment #8676181 - Attachment description: MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback and wrap methods in getters. r?jib → MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc
Attachment #8676181 - Flags: review?(roc)
Comment on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

Bug 1208371 - Remove MediaStreamTrack::GetStream/GetParentObject. r?jib
Attachment #8676182 - Attachment description: MozReview Request: Bug 1208371 - Test MediaStream.clone(). r?jib → MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream/GetParentObject. r?jib
Bug 1208371 - Route ApplyConstraints through MediaStreamTrackSource. r?jib
Attachment #8681202 - Flags: review?(jib)
Bug 1208371 - Make it possible to block tracks in a MediaInputPort initally. r?roc
Attachment #8681203 - Flags: review?(roc)
Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc
Attachment #8681204 - Flags: review?(roc)
Attachment #8681204 - Flags: review?(jib)
Attachment #8681204 - Flags: review?(bugs)
Bug 1208371 - Implement MediaStream.clone() r?smaug,jib,roc
Attachment #8681205 - Flags: review?(roc)
Attachment #8681205 - Flags: review?(jib)
Attachment #8681205 - Flags: review?(bugs)
Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib
Attachment #8681206 - Flags: review?(jib)
Bug 1208371 - Forward input stream and track id on regular track changes for union streams. r?roc
Attachment #8681207 - Flags: review?(roc)
Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback and wrap methods in getters. r?jib
Attachment #8681208 - Flags: review?(jib)
Bug 1208371 - Test MediaStream.clone(). r?jib
Attachment #8681209 - Flags: review?(jib)
Attachment #8681204 - Flags: review?(bugs) → review+
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.)
Attachment #8681205 - Flags: review?(bugs) → review+
Comment on attachment 8676163 [details]
MozReview Request: Bug 1208371 - Track original track in MediaStreamTrack clones. r?jib

https://reviewboard.mozilla.org/r/22577/#review21293
Attachment #8676163 - Flags: review?(jib) → review+
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 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
Attachment #8676182 - Flags: review+
Attachment #8681202 - Flags: review?(jib) → review+
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)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #76)
> Or CreateDOMTrack()/CloneDOMTrack() for
> CreateOwnDOMTrack()/CreateClonedDOMTrack()?

+1
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 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
Attachment #8681205 - Flags: review?(jib)
Attachment #8681206 - Flags: review?(jib) → review+
Comment on attachment 8681206 [details]
MozReview Request: Bug 1208371 - Various cleanups in DOMMediaStream/MediaStreamTrack. r?jib

https://reviewboard.mozilla.org/r/23779/#review21447
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?
Attachment #8681208 - Flags: review?(jib)
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?
Attachment #8681209 - Flags: review?(jib) → review+
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 on attachment 8676164 [details]
MozReview Request: Bug 1208371 - Un-nest MediaEngineSource::PhotoCallback. r?roc

https://reviewboard.mozilla.org/r/22579/#review21475
Attachment #8676164 - Flags: review?(roc) → review+
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.
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.
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 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 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
Attachment #8676174 - Flags: review?(roc) → review+
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"
Attachment #8676175 - Flags: review?(roc) → review+
Comment on attachment 8676176 [details]
MozReview Request: Bug 1208371 - Remove obsolete SetTrackEnabled() from DOMMediaStream r?roc

https://reviewboard.mozilla.org/r/22599/#review21601
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
Attachment #8676178 - Flags: review?(roc)
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 on attachment 8676181 [details]
MozReview Request: Bug 1208371 - Fix DOMMediaStream::OwnsTrack. r?roc

https://reviewboard.mozilla.org/r/22609/#review21609
Attachment #8676181 - Flags: review?(roc) → review+
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
Attachment #8681203 - Flags: review?(roc) → review+
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.
Attachment #8681204 - Flags: review?(roc) → review+
Comment on attachment 8681205 [details]
MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23777/#review21615
Attachment #8681205 - Flags: review?(roc) → review+
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
Attachment #8681207 - Flags: review?(roc) → review+
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.
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.
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.
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 on attachment 8676182 [details]
MozReview Request: Bug 1208371 - Remove MediaStreamTrack::GetStream. r?jib

https://reviewboard.mozilla.org/r/22611/#review21793
Attachment #8676182 - Flags: review+
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.
(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.
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.
(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 ?
Sorry, I meant:

    var clone = new MediaStream(stream.getTracks().map(track => track.clone());
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.
```
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.
(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?
Flags: needinfo?(jib)
https://reviewboard.mozilla.org/r/23775/#review21613

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

Done!
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.
See Also: → 1221890
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 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 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 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 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/
Attachment #8676162 - Attachment description: MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc → MozReview Request: Bug 1208371 - Introduce MediaStreamTrack logs. r?roc,jib
Attachment #8676162 - Flags: review?(roc)
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 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 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/
Attachment #8676168 - Flags: review?(roc)
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 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 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 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 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/
Attachment #8676172 - Flags: review?(roc)
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 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 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 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 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/
Attachment #8676178 - Flags: review?(roc)
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/
Attachment #8676179 - Attachment description: MozReview Request: Bug 1208371 - Add an MSGListener interface to MediaStreamTrack. r?roc → MozReview Request: Bug 1208371 - Add an MSGTrackListener interface to MediaStreamTrack. r?roc
Attachment #8676179 - Flags: review?(roc)
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 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 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 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 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 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 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 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/
Attachment #8681204 - Flags: review+ → review?(jib)
Attachment #8681205 - Attachment description: MozReview Request: Bug 1208371 - Implement MediaStream.clone() r?smaug,jib,roc → MozReview Request: Bug 1208371 - Implement DOMMediaStream::Clone() r?smaug,jib,roc
Attachment #8681205 - Flags: review+ → review?(jib)
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 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 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/
Attachment #8681208 - Attachment description: MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback and wrap methods in getters. r?jib → MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback. r?jib
Attachment #8681208 - Flags: review?(jib)
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 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/
Bug 1208371 - Rename CreateOwnDOMTrack/CreateClonedDOMTrack to CreateDOMTrack/CloneDOMTrack. r?jib
Attachment #8683551 - Flags: review?(jib)
(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.
Flags: needinfo?(jib)
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 on attachment 8681208 [details]
MozReview Request: Bug 1208371 - Move track.stop() helpers to MediaStreamPlayback. r?jib

https://reviewboard.mozilla.org/r/23783/#review21933
Attachment #8681208 - Flags: review?(jib) → review+
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?
Attachment #8681205 - Flags: review?(jib)
Comment on attachment 8683551 [details]
MozReview Request: Bug 1208371 - Rename CreateOwnDOMTrack/CreateClonedDOMTrack to CreateDOMTrack/CloneDOMTrack. r?jib

https://reviewboard.mozilla.org/r/24365/#review21939
Attachment #8683551 - Flags: review?(jib) → review+
Comment on attachment 8676178 [details]
MozReview Request: Bug 1208371 - Add DOMMediaStream::GetTrackById/GetOwnedTrackById. r?jib

https://reviewboard.mozilla.org/r/22603/#review21975
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.
Flags: needinfo?(martin.thomson)
Flags: needinfo?(jib)
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?
Flags: needinfo?(padenot)
Blocks: 1083354
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
(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.
Flags: needinfo?(padenot)
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
Flags: needinfo?(jib)
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.
Attachment #8681205 - Flags: review+
Attachment #8681204 - Flags: review?(jib) → review+
Comment on attachment 8681204 [details]
MozReview Request: Bug 1208371 - Implement MediaStreamTrack::Clone(). r?smaug,jib,roc

https://reviewboard.mozilla.org/r/23775/#review22037
(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.
Flags: needinfo?(martin.thomson)
Martin, please also see comment 200.
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
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?
Flags: needinfo?(martin.thomson)
Flags: needinfo?(jib)
(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.
Flags: needinfo?(jib)
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.
Flags: needinfo?(martin.thomson)
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
(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.
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.
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.
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
(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.
backlog: --- → webrtc/webaudio+
Rank: 19
Priority: -- → P1
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 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 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 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 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 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 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 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 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 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 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/
Attachment #8676165 - Attachment description: MozReview Request: Bug 1208371 - Make DOMMediaStream support WeakPtrs. r?roc → MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib
Attachment #8676178 - Attachment description: MozReview Request: Bug 1208371 - Add a PrincipalChangeObserver interface to MediaStreamTrack. r?roc → MozReview Request: dummy
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 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/
Attachment #8676177 - Attachment description: MozReview Request: Bug 1208371 - Add MediaStreamTrack::Graph(). r?jib → MozReview Request: dummy
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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/
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/
Attachment #8700521 - Attachment is obsolete: true
Attachment #8700522 - Attachment is obsolete: true
Attachment #8700523 - Attachment is obsolete: true
Attachment #8700524 - Attachment is obsolete: true
Attachment #8700525 - Attachment is obsolete: true
Depends on: 1234162
Brace for impact.