search

Temasys / AdapterJS

AdapterJS Javascript Polyfill and Tools for WebRTC - Skylink WebRTC
http://skylink.io/web
Other
428 stars 100 forks source link

Use native RTCPeerConnection API when available for Edge browsers on Windows Creator Update #265

Closed oooookk7 closed 7 years ago

oooookk7 commented 7 years ago
johache commented 7 years ago

Hum, that does't sound good to me.

If Google is shimming RTCPC, I'm guessing they have a reason. Last time I tried, I remember the connection working Egde to Egde. Is that no longer the case ? What is the test scenario/link ?

Also, the adapter.js repo seems to be pretty active regarding Edge support : https://github.com/webrtc/adapter/commits/master Did you check if updating our dependency would fix you issue ?

oooookk7 commented 7 years ago

@johache actually I tested the later version of their release: https://github.com/webrtc/adapter/tree/v3.3.2 and it seems like their ORTC polyfill for WebRTC API didn't work well with the Edge 15.xxxx Windows Creator Update version.

They seem to be overriding the RTCPeerConnection API instead of using the native implemented one which is better as according to here.

Perhaps we can do a PR to their repo if that's possible.

oooookk7 commented 7 years ago

@johache actually, let me just do a PR into their repo instead. Depending on the time constraints as well if we need to get our AdapterJS to use Edge native RTCPeerConnection, I'll leave this PR open for now.

Update: So it made sense since the native RTCPeerConnection that is available is for pre-1.0 legacy support as according to the article here, in which they won't support for the future WebRTC API in which is valid hence the PR is closed.

The problem now is that the current latest version of the RTCPeerConnection ORTC API polyfill doesn't work well with 15.15063.

@johache perhaps I suggest we store the native RTCPeerConnection API as window.legacyRTCPeerConnection which we can use for now in the SDK release for Edge interop supports which is now broken.

fippo commented 7 years ago

if you want Edge's native RTCPeerConnection use webkitRTCPeerConnection (which says a lot about what that implementation does) which will be left untouched by adapter.

@letchoo what seems to be broken? Still works for me (even though getUserMedia is busted in the insider builds)

oooookk7 commented 7 years ago

@fippo it seems like there was some "InvalidAccessError" whilst testing with the latest master branch which had issues with the 15.15063 version and had no issues with the 14.xxx version. The native legacy RTCPeerConnection API seem to work with 15.15063. Will probably open the relevant tickets in any case.

This was done with Chrome <-> Edge connection test.

fippo commented 7 years ago

@letchoo looks like there is some issue in 15063 which wasn't there in 14393. Will take a look, thanks! On a second look appear.in still works for me in 15063 (using adapter 3.3.1) so can't reproduce.

johache commented 7 years ago

@letchoo keeping msRTCPC Sounds fair, although it does not solve our actual issue. I'm happy with merging that first if you want.

@fippo We've mostly tested against our AdapterJS (built from different version of webrtc/adapter). I feel like the best way of moving this forward would be for us to produce a minimal test case to reproduce. I'll try to do that next week.

oooookk7 commented 7 years ago

Hey there @fippo, damn me but the tests for the latest webrtc was not an accurate test since it was basing off an 2.x.x version of the webrtc/adapter after I double-checked again. I was testing with the latest webrtc/adapter and it seems like there are separate issues (I think I opened one #510) with the connections. Thanks a lot for the investigations!

Update: I lied, it seems like connecting ORTC polyfill peer connection from Edge to Chrome is reproducible with empty video stream appearing on Chrome. ICE connection is established but the video packets is not streaming to Chrome from Edge for some reasons. Could be a browser issue instead of a webrtc/adapter issue. "broken" is the wrong word to use, perhaps it's just not streaming.

oooookk7 commented 7 years ago

@johache so now it's cached to msRTCPeerConnection. Is this ok to go?

johache commented 7 years ago

Looks fine to me.