changing tracks, multistreaming and renegotiation #250

t-mullen · 2018-03-03T09:55:32Z

The WebRTC API around tracks and multistreaming is finally stable enough to implement this.

Stream methods are convenience wrappers for the track methods. Native addStream, onAddStream usage has been removed since all major browsers support tracks now.

Exposed API:

addStream(MediaStream)
addTrack(MediaStreamTrack)
removeStream(RTCRtpSender[])
removeTrack(RTCRtpSender)
peer.on('stream')
peer.on('track')
peer.on('removestream')
peer.on('removetrack')
peer.on('negotiate')

Also added the streams option for passing an array of MediaStreams into the constructor.

Some issues that had to be worked around:

Firefox only being able to call pc.removeTrack when signalingState is stable.
Neither Firefox or Safari emitting the removetrack event on MediaStreams as per spec. Had to parse remote descriptions to figure out if tracks were being removed.
Non-initators attempting to renegotiate can lead to all sorts of race conditions. A new signal object is used to request negotiation from the initiator. This is a breaking change.
onnegotiationneeded does not fire more than once in Safari, fires multiple times in Chrome and doesn't fire on removeTrack for Firefox. Removing automatic detection is actually nicer since it allows the user to make multiple asynchronous changes to the connection with a single round of negotiation. Not using the event doesn't break anything.

Thanks to @cusspvz for the initial work on this.

nazar-pc · 2018-03-03T10:29:45Z

index.js

@@ -98,19 +105,14 @@ function Peer (opts) {
  self._pc.onicecandidate = function (event) {
    self._onIceCandidate(event)
  }
+  self._pc.onnegotiationneeded = null // wrtc doesn't fire this event, Chromium fires it too often, and Firefox doesn't fire it enough


If onnegotiationneeded is not used, why still assigning null in 2 places?

nazar-pc · 2018-03-03T10:32:07Z

index.js

-        self._pc.addTrack(track, self.stream)
+    if (self.streams) {
+      self.streams.forEach(function (stream) {
+        self._negotationsToIgnore += stream.getTracks().length


_negotationsToIgnore doesn't seem to be used (yet/anymore?)

nazar-pc · 2018-03-03T10:37:16Z

index.js

+    }
+  } else {
+    self._debug('requesting negotiation from initiator')
+    self.emit('signal', JSON.stringify({ // request initiator to renegotiate


Can't we open a dedicated RTCDataChannel for internal purposes instead of explicitly requiring user to send this data?
Can be related to #19, though negotiated only seems to be supported in Chrome for now: https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/negotiated

Sending signaling data over a datachannel would be pretty cool... I think we should look at this when we decide on an API for multiplexed datachannels.

I think this is actually ideal, if we do add this via a datachannel it should be a separate api.

@feross - This new signaling event will cause an error on older versions of simple-peer (which destroys them.) It's required since only the initiator can safely start renegotiation.

nazar-pc · 2018-03-03T10:43:25Z

index.js

+    self.removeSender(sender)
+  })
+}
+Peer.prototype.removeStream = Peer.prototype.removeSenders // for symmetry in the API


I feel like this method should be a separate method that accepts MediaStream instance and simple-peer can use WeakMap to keep internal mapping from MediaStream instance to RTCRtpSender[].
This would hide more of internal implementation from the user and will potentially make things easier to work with.

If we do that, we can avoid returning RTCRtpSender[] from addSteam(). This way user either add stream and remove the same stream or add individual tracks and removes individual tracks (using WeakMap again and without returning RTCRtpSender). This would be perfectly symmetric implementation from user's perspective.

Great idea, I didn't like exposing RTCRtpSenders either.

nazar-pc · 2018-03-03T10:46:09Z

index.js

+ * @param {RTCRtpSender} sender
+ * @returns {RTCRtpSender}
+ */
+Peer.prototype.removeSender = function (sender) {


Why not call it remoteTrack and remove removeSender? Seems like remaining code from earlier prototype.

nazar-pc · 2018-03-03T10:46:38Z

index.js

+ * @param {MediaStream} stream
+ * @returns {RTCRtpSender[]}
+ */
+Peer.prototype.addStream = function (stream) {


Duplicated function, already present above.

nazar-pc · 2018-03-03T10:51:38Z

index.js

+/**
+ * Renegotiate the peer connection.
+ */
+Peer.prototype.renegotiate = function () {


What are the use-cases when this needs to be exposed and can't be handled by the library automatically?
I mean, when multiple streams/tracks need to be added, setTimeout(callback, 0) can be used to batch them into single _negotiate() call.

I exposed it to allow batching of asynchronous changes to the connection. Typically MediaStreams come from several async sources in parallel (ie one from getUserMedia, one from screenCapture, a synchronous WebAudio input).

The API benefits of abstracting negotiation is something to consider though.

If user expects all of these to be added with one negotiation, it is still possible to do Promise.all() and then add necessary streams/tracks synchronously so that only one renegotiation is made.

nazar-pc · 2018-03-03T10:59:50Z

index.js

@@ -573,7 +680,7 @@ Peer.prototype._maybeReady = function () {

      items.forEach(function (item) {
        // Spec-compliant
-        if (item.type === 'transport') {
+        if (item.type === 'transport' && item.selectedCandidatePairId) {


selectedCandidatePairId seems to be required by specification and Chrome supports it. Is there a browser which doesn't, maybe add a comment?

I was getting rare candidates that did not have that property (Chrome). I'll remove this and open a separate bug to look into it.

I've noticed this error only happens in the new tests, so we might want to figure it out before merging:

Here's a sample candidate that causes the failure (they are somewhat rare):

{ bytesReceived:0, bytesSent:0, dtlsState:"new", id:"RTCTransport_audio_1", localCertificateId:"RTCCertificate_F7:92:F4:5E:99:2C:F6:66:92:6C:DA:21:D7:30:AB:D7:61:E5:E2:A2:82:AF:58:2D:1D:1D:BD:99:D4:7E:D8:92", timestamp:1520117393473.471, type:"transport" }

That's the whole object. Note the absence of selectedCandidatePairId

Interesting, basically another aspect of #215. Then workaround makes sense.

I'm just wondering why Chrome even adds type=transport without selectedCandidatePairId property, since it clearly has this property on later stage and not much useful without it.
Might be worth to report as Chrome bug and add a comment with link to bug report (haven't found existing bug report for this).

It only happens when dtlsState is "new", which might suggest the selectedCandidatePairId isn't known at that point?

I've just figured that out, dictionary only contains about half of the properties from the spec. Looks like it is necessary to check dtlsState property.

Here: https://w3c.github.io/webrtc-pc/#rtcdtlstransportstate-enum

nazar-pc · 2018-03-03T11:02:52Z

index.js

+
+  if (self._pc.signalingState === 'stable') {
+    self._isNegotiating = false
+    self._initialNegotiation = false


_initialNegotiation is set, but never used. Any plans for this property?

It was needed for automatic renegotiation, which was removed.

nazar-pc · 2018-03-03T11:08:51Z

index.js

+  if (self._remoteStreams.some(function (stream) {
+    return stream.id === event.streams[0].id // Only fire one 'stream' event, even though there may be multiple tracks per stream
+  })) return
+  self._remoteStreams.push(event.streams[0])


Spec says one track might be a part of more than 1 stream. Why do we only care about one of them?

So you're saying event.streams[1] could exist?

The comment there is referring to only firing one stream event for multiple track events that reference the same stream. We did that before, I've just refactored it.

The spec says there might be event.streams[1], otherwise it wouldn't be called that way. I'm just curious what would happen if we have multiple tracks, some of which are used in multiple streams. I don't think it will happen with high probability, but it is still a valid edge case.

…es, refactor

nazar-pc · 2018-03-03T13:57:45Z

index.js

+  self._debug('addTrack()')
+
+  var sender = self._pc.addTrack(track, stream)
+  self._senderMap.set(track, sender)


_senderMap should be a WeakMap of WeakMaps and track should be added/removed to/from specific stream, since the same track theoretically might be also used for another stream. Otherwise it will override previous sender with new sender, which corresponds to different stream and previous one will be lost.

nazar-pc · 2018-03-03T13:58:06Z

index.js

+ * Remove a MediaStreamTrack from the connection.
+ * @param {MediaStreamTrack} sender
+ */
+Peer.prototype.removeTrack = function (track) {


And stream argument should be added here accordingly.

nazar-pc · 2018-03-03T23:08:25Z

index.js

-  if (self._previousStreams.indexOf(id) !== -1) return // Only fire one 'stream' event, even though there may be multiple tracks per stream
-  self._previousStreams.push(id)
-  self.emit('stream', event.streams[0])
+  self.emit('track', event.track)


I'm not sure if ontrack is can be called multiple times (haven't found this explicitly in specification yet), but intuition indicates that it can. Hence track event and potentially removetrack might need to expose streams argument on them to clearly understand from which streams (if not all) track was removed.

nazar-pc

Looks good to me 👍

t-mullen · 2018-03-05T23:30:56Z

Thanks for the great review @nazar-pc. I'd like to do some more manual testing with real MediaStreams then we can finally get this merged. 🎉

t-mullen · 2018-03-07T08:19:50Z

After manual testing, everything seems good. Some limitations to note:

Safari doesn't use the original ID for MediaStreamTracks, instead it generates it's own, so using ids to correlate tracks won't work for it (all other browsers keep the same ID)
If you remove a track, you cannot add it again. You CAN clone the track and add it though. If you intend to readd a track frequently, you should probably mute it instead.

t-mullen · 2018-03-28T04:33:10Z

Merged. This is a breaking change for interop between peers, although the API changes are not breaking.

feross · 2018-04-12T23:45:20Z

@rationalcoding Can you explain how this breaks interop between peers?

EDIT: This PR does not break interop: #286 (comment)

changing tracks, multistreaming and renegotiation

t-mullen added 8 commits March 2, 2018 23:35

addStream, multistream and renegotiation

747ed8f

removeStream, removeTrack, and remove events

cb55401

formatting

92ddfc0

document removeTrack, removeStream and RTCRtpSenders

7138368

remove tracks in constructors (need a MediaStream argument)

5708431

renegotiate documentation

5d4dfea

renegotiate documentation

1aaced5

README

49222be

t-mullen requested review from feross and nazar-pc March 3, 2018 09:58

nazar-pc requested changes Mar 3, 2018

View reviewed changes

t-mullen added 3 commits March 3, 2018 07:31

hide negotiation, use WeakMap to hide RTCRtpSenders, batch sync chang…

1d25322

…es, refactor

standard style

ca950fe

standard style

3bc327a

nazar-pc reviewed Mar 3, 2018

View reviewed changes

This was referenced Mar 3, 2018

[WIP] Implementation of .renegotiate and .negotiate API #72

Closed

Create renegotiation method #68

Closed

expose addStream on prototype #50

Closed

map senders with stream AND tracks

dc6a933

nazar-pc reviewed Mar 3, 2018

View reviewed changes

expose stream for track events, refactor, track/stream id test

644d5a0

nazar-pc approved these changes Mar 4, 2018

View reviewed changes

t-mullen changed the title ~~changing tracks, multistreaming and renegotiate API~~ changing tracks, multistreaming and renegotiation Mar 4, 2018

t-mullen merged commit 461ed97 into feross:master Mar 28, 2018

nazar-pc mentioned this pull request Mar 30, 2018

Upgrade wrtc to latest release and drop its detection as unnecessary #266

Merged

folkvir mentioned this pull request Apr 11, 2018

(Info) addStream API not available #281

Closed

feross mentioned this pull request Apr 13, 2018

Publish new minor version #286

Closed

aral mentioned this pull request Feb 3, 2019

Add stream to existing swarm? mafintosh/webrtc-swarm#23

Open

FredZeng pushed a commit to FredZeng/simple-peer that referenced this pull request Oct 14, 2023

Merge pull request feross#250 from RationalCoding/addStream

def09af

changing tracks, multistreaming and renegotiation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changing tracks, multistreaming and renegotiation #250

changing tracks, multistreaming and renegotiation #250

t-mullen commented Mar 3, 2018 •

edited

Loading

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

t-mullen Mar 3, 2018

jeregrine Mar 5, 2018

t-mullen Apr 12, 2018

nazar-pc Mar 3, 2018 •

edited

Loading

t-mullen Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

t-mullen Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018 •

edited

Loading

t-mullen Mar 3, 2018 •

edited

Loading

t-mullen Mar 3, 2018 •

edited

Loading

nazar-pc Mar 3, 2018

t-mullen Mar 3, 2018 •

edited

Loading

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

t-mullen Mar 3, 2018

nazar-pc Mar 3, 2018

t-mullen Mar 3, 2018

t-mullen Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc Mar 3, 2018

nazar-pc left a comment

t-mullen commented Mar 5, 2018

t-mullen commented Mar 7, 2018 •

edited

Loading

t-mullen commented Mar 28, 2018

feross commented Apr 12, 2018 •

edited

Loading

changing tracks, multistreaming and renegotiation #250

changing tracks, multistreaming and renegotiation #250

Conversation

t-mullen commented Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

t-mullen Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

t-mullen Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-mullen Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

t-mullen commented Mar 5, 2018

t-mullen commented Mar 7, 2018 • edited Loading

t-mullen commented Mar 28, 2018

feross commented Apr 12, 2018 • edited Loading

t-mullen commented Mar 3, 2018 •

edited

Loading

nazar-pc Mar 3, 2018 •

edited

Loading

nazar-pc Mar 3, 2018 •

edited

Loading

t-mullen Mar 3, 2018 •

edited

Loading

t-mullen Mar 3, 2018 •

edited

Loading

t-mullen Mar 3, 2018 •

edited

Loading

t-mullen commented Mar 7, 2018 •

edited

Loading

feross commented Apr 12, 2018 •

edited

Loading