Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only filter 'turns:' and turn/stun with ipv6 #963

Closed
wants to merge 1 commit into from

Conversation

shagkur
Copy link

@shagkur shagkur commented May 15, 2019

Description
This PR should fix the filter function properly. It was tested against Edge 15/16/17/18. In all 4 version only the turns: protocol turned out to be not supported by edge (throwing an exception).
Also the stun protocol seems to be supported, at least no exception and everything works as expected.

Purpose
Should make the filterfunction not so restrictive.

@shagkur
Copy link
Author

shagkur commented May 23, 2019

Any comments on this?

@fippo
Copy link
Member

fippo commented Jun 6, 2019

Sorry for the delay! Getting definite information about the behaviour of all this is a bit complicated (see also #911 and to make things worse there is another copy of the filter in https://github.com/otalk/rtcpeerconnection-shim which may have hidden issues depending on how you tested.

I'll see if I can make time to do testing on browserstack (which hopefully still offers free Edge testing)

@shagkur
Copy link
Author

shagkur commented Jun 7, 2019

Well, first of all, i even didn't know there's a copy/separate shim implementation (based on adapter.js). But all my tests (which were done with the help of browserstack) have been done against the current adapter.js release. To figure that simply filtering 'turns' is enough i wrote up a small js demo for edge purely on the ORTC api.
I hope this background information helps a bit to understand my motivation to this.

@lgrahl
Copy link
Collaborator

lgrahl commented Jun 16, 2019

I think it would be useful if adapter would log a warning in the console in case it strips a URL.

@kekkokk
Copy link
Contributor

kekkokk commented Jul 2, 2019

this is so fucked up.
I really can't understand what actually edge is capable of.
There is any place where we can get informations?
for example, is stun supported?

Version: 44.17763.1.0

Because if I pass :
iceServers: [ { urls: 'stun:stunaddress:3478' } ] without adapter it seems working
if I pass:
iceServers: [ { urls: 'stun:stunaddress:3478?transport=udp' } ] without adapter it breaks

If I pass both with adapter 7.2.4 they get filtered out

If I pass

 {  username: 'fakeusername',
    urls: 'turn:turnaddress:3478?transport=udp',
    credential: 'fakecredential' }

with both adapter and without it works

If i pass:

  { username: 'fakeusername',
    urls: 'turn:turnaddress:3478?transport=tcp',
    credential: 'fakecredential' }

without adapter: works
with adapter: it gets filtered out

Sorry, @fippo I know this question was asked 1000 times

@fippo
Copy link
Member

fippo commented Jul 2, 2019

There is any place where we can get informations?

There is https://github.com/aboba/edgertc but this part is not documented. The situation is pretty complicated... stun urls are supposed to be ignored but that only works in some versions and depending on how exactly they look (e.g. include port and transport)

Because if I pass :
iceServers: [ { urls: 'stun:stunaddress:3478' } ] without adapter it seems working
if I pass:
iceServers: [ { urls: 'stun:stunaddress:3478?transport=udp' } ] without adapter it breaks

Passing the first to the raw ice gatherer (along with gatherPolicy: 'all') only gives me host candidates in the latest insider builds. So filtering doesn't do any harm in this case.

If I pass

 {  username: 'fakeusername',
    urls: 'turn:turnaddress:3478?transport=udp',
    credential: 'fakecredential' }

with both adapter and without it works

Right, that is supposed to work and you get a relay candidate and a srflx candidate. And a tcp srflx which is... not actually working.

If i pass:

  { username: 'fakeusername',
    urls: 'turn:turnaddress:3478?transport=tcp',
    credential: 'fakecredential' }

without adapter: works
with adapter: it gets filtered out

but do you get relay candidates? I don't so filtering does not do any harm.

Now in addition to that some versions of Edge require ?transport=udp and others do not, see #911
(and some still throw on any stun:

Its somewhat hard to do the right thing that works in any version.

@kekkokk
Copy link
Contributor

kekkokk commented Jul 2, 2019

this makes me so sad...
Next question then:
do you know any ETA of the new chromium based edge to the public? this will be fresh air for everyone I suppose

@shagkur
Copy link
Author

shagkur commented Jul 9, 2019

Just as a thought to all of this (MS caused) mess: Do you really need to support ancient versions of Edge?
I tested this filter change against 15, 16, 17 and 18. And i believe those are enough versions to support.
Well, saying that filtering, as of now, does not harm is sort of wrong. Because when the order of the TURN/STUN servers in the list is wrong (yes in the end you'll have it run on different browsers) then you're screwed.
And nothing is, currently, telling you why.

Without any doubt older version of Edge might behave strangely on this. But hey, that's MS and we all know them well ;)

@fippo
Copy link
Member

fippo commented May 6, 2021

overtaken by events.

@fippo fippo closed this May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants