Fix replacestream #12

y-i · 2017-09-25T07:52:33Z

In Firefox, even if a client who is only sending audio uses replaceStream to add video, the client can not send video. This PR fixes it.
Multi track will be implemented after Chrome uses Unified Plan SDP.

iwashi

As I commented, can you confirm these case?

both video and audio -> only audio
audio -> both video and audio
both video and audio -> both video and audio // As regression

iwashi · 2017-09-26T05:17:07Z

src/peer/negotiator.js

@@ -114,6 +114,23 @@ class Negotiator extends EventEmitter {
        }
      });

+      if (this._pc.getSenders().every(sender => {


About L:117-130, what happens in the use case described below?

The user has both an audio and a video track; which means, _pc.getSenders() should return the array including 2 rtpSender.

Then user wants to stop sending the video track to reduce bandwidth, so s/he calls replaceStream(newStream) where newStream contains only the audio track.

Does the user can successfully create an offer SDP and communicate with the other party after negotiation as the user expects?

iwashi · 2017-09-26T05:36:21Z

src/peer/negotiator.js

@@ -114,6 +114,23 @@ class Negotiator extends EventEmitter {
        }
      });

+      if (this._pc.getSenders().every(sender => {
+        return sender.track.kind !== 'audio';


Personally I prefer extracting this condition to another method like,

Before

if (this._pc.getSenders().every(sender => { return sender.track.kind !== 'audio'; }))

After

if (_onlyVideoTracksExist(this._pc)) { ...}

This explains what we're doing in the condition.

leaysgur · 2017-09-29T02:31:09Z

src/peer/negotiator.js

@@ -407,6 +366,7 @@ class Negotiator extends EventEmitter {
        return this._pc.setLocalDescription(answer)
          .then(() => {
            logger.log('Set localDescription: answer');
+            logger.log(`Setting local description ${JSON.stringify(answer.sdp)}`);


Even if user set log level to 0, JSON.stringify() is called.
Is it ok?

JSON.stringify() is called at _setRemoteDescription too. So I think it is ok.

leaysgur · 2017-09-29T02:31:12Z

src/peer/negotiator.js

+          this._pc.addTrack(track, newStream);
+        } else {
+          // need to check id before replace..?
+          vSenders[idx].replaceTrack(track);


As I commented here, what will happen when trying to replace track A with track A?

leaysgur · 2017-09-29T02:31:23Z

src/peer/negotiator.js

+      this._pc.removeStream(localStreams[0]);
+    }
+
+    this._replaceStreamCalled = true;


This line should be placed after this._pc.addStream(newStream); for naming..?

_replaceStreamCalled is true after replaceStream and is false after onnegotiationneeded.
_replaceStreamCalled is used to check if onnegotiationneeded event has finished or not.
If it is placed after addStream, _replaceStreamCalled becomes true after onnegotiationneeded.

alanmshelly · 2017-10-04T05:13:40Z

src/peer/negotiator.js

@@ -477,6 +438,77 @@ class Negotiator extends EventEmitter {
  }

  /**
+   * Replace the stream being sent with a new one.


Can you add comments to differentiate _replacePerTrack() and _replacePerStream()?

alanmshelly · 2017-10-04T06:56:38Z

NOTE: more discussion here: https://gist.github.com/leader22/fb538265d7f24217dada1d6b8a8d2380

alanmshelly · 2017-10-05T07:41:57Z

tests/peer/negotiator.js

+        it('should not call addTrack for audio sender', () => {
+          negotiator.replaceStream(newStream);
+
+          console.log(negotiator._pc.getSenders());


can you get rid of this?
I'm assuming it's for debugging

alanmshelly · 2017-10-05T07:44:42Z

src/peer/negotiator.js

+        return;
+      }
+      // if track was not replaced, do nothing
+      if (sender.track.id === track.id) {


The tests are failing

I'm not sure but I think this line might be the problem. Could you see what setting the track.id in the tests does?

In this test, both stubs of senders and stubs of tracks do not have id key. So I think this statement is always true because it is undefined === undefined.

alanmshelly · 2017-10-05T07:54:53Z

The direction of the code looks good. Thanks! 👍

alanmshelly · 2017-10-06T04:30:47Z

LGTM! 👍
Could you please rebase and clean up the commit log? (you can force push to this branch)

iwashi · 2017-10-10T03:49:57Z

LGTM!
Could @y-i make a last check on browsers?

y-i · 2017-10-10T07:17:23Z

skyway.js:420 SkyWay:  DOMException: Failed to set local answer sdp: Called in wrong state: STATE_INPROGRESS
skyway.js:420 SkyWay:  DOMException: Failed to set local answer sdp: Called in wrong state: STATE_INPROGRESS
skyway.js:439 SkyWay:  signalingState is stable
index.html:1 Uncaught (in promise) DOMException: Failed to set local answer sdp: Called in wrong state: STATE_INPROGRESS

y-i · 2017-10-12T06:31:31Z

I checked on Chrome and Firefox.

…replaceStream If `getSenders` is not defined, `addTrack` will not be defined either. If broweser dosen't support `getSenders`, stream is replaced by stream. Otherwise stream is replaced per stream. We assume that there are at most 1 audio and at most 1 video in local stream.

alanmshelly · 2017-10-12T06:40:48Z

LGTM!

iwashi · 2017-10-12T06:41:55Z

y-i · 2017-10-12T06:43:02Z

src/peer/negotiator.js

+    this._replaceStreamCalled = true;
+
+    // a chance to trigger (and do nothing) on removeStream.
+    setTimeout(() => {


add setTimeout before addStream to fix state error on chrome

iwashi reviewed Sep 26, 2017

View reviewed changes

iwashi requested a review from leaysgur September 26, 2017 05:38

y-i changed the base branch from v1.0.2 to release/1.0.2 September 27, 2017 06:49

y-i force-pushed the fix_replacestream branch 2 times, most recently from 0c12526 to 44b7f99 Compare September 28, 2017 05:37

leaysgur reviewed Sep 29, 2017

View reviewed changes

alanmshelly reviewed Oct 4, 2017

View reviewed changes

alanmshelly reviewed Oct 5, 2017

View reviewed changes

add local description logger

2887319

y-i force-pushed the fix_replacestream branch from a254d87 to 371e893 Compare October 6, 2017 05:41

y-i force-pushed the fix_replacestream branch from 371e893 to d8b5be6 Compare October 12, 2017 06:27

y-i added 2 commits October 12, 2017 15:33

add some variables and fix tests which correspond with replaceStream

e9d7bce

y-i force-pushed the fix_replacestream branch from d8b5be6 to e9d7bce Compare October 12, 2017 06:35

iwashi merged commit 51045b3 into release/1.0.2 Oct 12, 2017

iwashi deleted the fix_replacestream branch October 12, 2017 06:42

y-i commented Oct 12, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix replacestream #12

Fix replacestream #12

y-i commented Sep 25, 2017 •

edited

iwashi left a comment

iwashi Sep 26, 2017

iwashi Sep 26, 2017

leaysgur Sep 29, 2017

y-i Oct 3, 2017

leaysgur Sep 29, 2017

leaysgur Sep 29, 2017

y-i Oct 3, 2017

alanmshelly Oct 4, 2017

alanmshelly commented Oct 4, 2017 •

edited

alanmshelly Oct 5, 2017

alanmshelly Oct 5, 2017

y-i Oct 5, 2017

alanmshelly commented Oct 5, 2017

alanmshelly commented Oct 6, 2017

iwashi commented Oct 10, 2017

y-i commented Oct 10, 2017

y-i commented Oct 12, 2017

alanmshelly commented Oct 12, 2017

iwashi commented Oct 12, 2017

y-i Oct 12, 2017

Fix replacestream #12

Fix replacestream #12

Conversation

y-i commented Sep 25, 2017 • edited

iwashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

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

alanmshelly commented Oct 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanmshelly commented Oct 5, 2017

alanmshelly commented Oct 6, 2017

iwashi commented Oct 10, 2017

y-i commented Oct 10, 2017

y-i commented Oct 12, 2017

alanmshelly commented Oct 12, 2017

iwashi commented Oct 12, 2017

Choose a reason for hiding this comment

y-i commented Sep 25, 2017 •

edited

alanmshelly commented Oct 4, 2017 •

edited