DRAFT: Don't immediately renegotiate for initial streams #671
Conversation
feross
force-pushed
the
no-immediate-renegotiate
branch
from
3ed22bb to
1aaa0bd
Compare
| @@ -146,7 +146,7 @@ class Peer extends stream.Duplex { | |||
|
|
|||
| if (this.streams) { | |||
| this.streams.forEach(stream => { | |||
| this.addStream(stream) | |||
| this.addStream(stream, false) | |||
There was a problem hiding this comment.
This parameter tells addTrack that it should not trigger a renegotiation.
| @@ -270,11 +270,11 @@ class Peer extends stream.Duplex { | |||
| * Add a MediaStream to the connection. | |||
| * @param {MediaStream} stream | |||
| */ | |||
| addStream (stream) { | |||
| addStream (stream, triggerNegotiate = true) { | |||
There was a problem hiding this comment.
I don't actually want to add this to the public API, but this PR is just to demonstrate a fix to the problem
| this._debug('negotiate') | ||
| this.emit('negotiate') |
There was a problem hiding this comment.
Unrelated fix here: I moved this debug log and the undocumented 'negotiate' event to fire when negotiate happens. Before, it was firing when the connection becomes stable, which seemed like a bug
There was a problem hiding this comment.
It's meant to fire when negotiation completes, which coincides with stable without a queued renegotiation.
It's only used in some tests.
There was a problem hiding this comment.
I think it'd be better to name the event something other than 'negotiate' since the convention is to emit debug logs and events named after methods when they're first called. Maybe it should be called 'negotiated' then?
| @@ -882,6 +885,7 @@ class Peer extends stream.Duplex { | |||
| if (this.destroyed) return | |||
|
|
|||
| if (this._pc.signalingState === 'stable' && !this._firstStable) { | |||
| this._firstStable = false | |||
There was a problem hiding this comment.
This boolean seemed to be set in the wrong place. Before it was being set after any signalingStateChange instead of after the first stable state. I moved it inside the if statement so it is only set after first stable.
|
Just tested this, it's working great! Looking forward to a release with this in. |
|
Superceded by #722 |
Fixes #670
This is a draft because I don't want to change the public API in order to fix this bug.