Simple-peer: Fix Travis Tests

[feross]

I thought we already fixed the tests in #264, but they're not passing again. We should fix this ASAP. What is the cause?

[feross]

Tests are passing on this commit: https://github.com/feross/simple-peer/commit/7001cc72e0dba8d3abb9c9661d4d162aa33decc9 (Results: https://travis-ci.org/feross/simple-peer/builds/357622511)

They start failing after this commit: https://github.com/feross/simple-peer/commit/461ed97509e4b2ec10944510802703e47101126b (Results: https://travis-ci.org/feross/simple-peer/builds/359204617)

cc @RationalCoding

[feross]

@RationalCoding After your PR #284 the tests get further along, but they're still failing once the sauce labs tests actually run. It looks like Firefox is the issue: https://travis-ci.org/feross/simple-peer/builds/365902348

# track and stream ids are correct
not ok 81 track id is correct
  ---
    operator: equal
    expected: |-
      '{34b6f430-ba21-4215-bf62-c3c1b03a70f4}'
    actual: |-
      '{c43d5a10-2315-4377-bb32-22b948dc1c9d}'
    stack: |-
      assert@http://airtap.local:35609/__zuul/test-bundle.js:11546:54
      bound@http://airtap.local:35609/__zuul/test-bundle.js:11398:28
      ["/home/travis/build/feross/[secure]/node_modules/tape/lib/test.js"]</</Test.prototype.strictEquals@http://airtap.local:35609/__zuul/test-bundle.js:11709:5
      bound@http://airtap.local:35609/__zuul/test-bundle.js:11398:28
      ["/home/travis/build/feross/[secure]/test/multistream.js"]</</<@http://airtap.local:35609/__zuul/test-bundle.js:13525:5
      ["/home/travis/build/feross/[secure]/node_modules/events/events.js"]</EventEmitter.prototype.emit@http://airtap.local:35609/__zuul/test-bundle.js:4911:9
      ["/home/travis/build/feross/[secure]/index.js"]</</Peer.prototype._onTrack/<@http://airtap.local:35609/__zuul/test-bundle.js:858:5
      ["/home/travis/build/feross/[secure]/index.js"]</</Peer.prototype._onTrack@http://airtap.local:35609/__zuul/test-bundle.js:856:3
      Peer/self._pc.ontrack@http://airtap.local:35609/__zuul/test-bundle.js:135:7

[t-mullen]

It looks like latest Firefox has updated WebRTC to no longer pass local MSIDs through to the media description (like Safari). I wasn't able to determine whether this is on-spec or not, and skipped the test for Safari. There is an open, but ignored bug for Safari: https://bugs.webkit.org/show_bug.cgi?id=174519

[t-mullen]

Matching MSIDs isn't important to any feature of simple-peer but might be useful for correlating tracks.

I'll silence the test on Firefox for now, but I'd like to understand why this behaviour exists and why Chrome doesn't do it.

[feross]

Up to you -- you could open an issue on Firefox and try to get a response if this is intended behavior. If they're not meant to match, let's remove the test for all browsers.

[t-mullen]

Let's just remove the test for now. It's existence suggests msids are reliable for correlating tracks, which they clearly aren't in at least two browsers now.

[feross]

Fixed by https://github.com/feross/simple-peer/pull/285

[feross]

Test still seem to be stalling out... any ideas @RationalCoding?

https://travis-ci.org/feross/simple-peer/builds/365986308

[t-mullen]

Ah, this is due to #95. When the non-initiator adds a stream, it needs to specify constraints or the track event won't fire. However, it can't do this in the constructor like before, because the stream is being added on renegotiation. I didn't implement renegotiation of constraints, so it's not currently possible to upgrade a datachannel-only connection to a mediastream connection without adding a stream to the initiator side.

[t-mullen]

I'll put this feature on a TODO and document the limitation.

[feross]

Ah, good find. In the meantime, can we adjust that test so it passes? I don't want to release a new version until tests on master are passing.

[t-mullen]

Firefox msids no longer correspond to the track IDs in the emitted objects, so my method of detecting track/stream removal won't work. I'm working on a fix.

[feross]

@RationalCoding Thanks for the update. It may be worth asking other WebRTC users/library authors what they do to handle this.

@fippo - do you have any thoughts on the previous comment by @RationalCoding?

[t-mullen]

Here's an overview of the removetrack implementations I've tried:

The track.onmute listener: only works on Chrome.
Parsing the SDP: This worked in Firefox 58, but it seems this isn't supposed to work at all with the new transceiver msids.
Checking for track changes in the stream: This seems to work but fails when the non-initiator removes streams. Might be related to that constraints issue.

It's exams currently and I don't have very much time to diagnose this. It's possible to use the removeStream API without this event, since the video/audio will still update accordingly. It's just not possible to listen in to this removal, so you'll end up playing empty streams...

I can get a PR ready to remove the unreliable events if you would like to release the working features.

[t-mullen]

I haven't found any other libraries that implement multistreaming. The WebRTC experiments demos are the most I could find, and much of that is no longer working.

[feross]

I can get a PR ready to remove the unreliable events if you would like to release the working features.

I don't want to take you away from your exams! If it's easy for you to send a quick PR that make the tests pass, that'd be great. Then I can do the release.

Otherwise, we can wait until you have more time to fix the issue properly. Thanks for being awesome, @RationalCoding!

[t-mullen]

No problem. My mistake for merging a large PR before exams...

[feross]

@RationalCoding No worries, you can do it after exams. No rush.

[t-mullen]

🎉

[fippo]

track ids are no longer something you can rely on, see Jan-Ivars blog post at https://blog.mozilla.org/webrtc/rtcrtptransceiver-explored/ which also explains how things are supposed to work now. ofc they don't work consistently among browsers :-/