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

rtciceserver: fix represents a turn server #1427

Closed
wants to merge 1 commit into from
Closed

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Jun 26, 2017

fixes #1198

seemed simpler than what I had in mind initially.

@aboba aboba requested a review from taylor-b June 26, 2017 21:42
@taylor-b
Copy link
Contributor

Previously, it was implied that each RTCIceServer represents a single "server" in the abstract sense, where the list of URLs represents a list of ways of connecting to that server.

If we're loosening that definition, and allowing a single RTCIceServer to include STUN/TURN URLs, then the question becomes: "What's the difference between putting multiple URLs in one RTCIceServer instead of multiple?" It looks like step 11 of setConfiguration flattens the structure (or at least, is intended to)? If so, this should be called out in a note. If not, what is the difference?

@fippo
Copy link
Contributor Author

fippo commented Jun 27, 2017

My interpretation is that if the application specifies a group of urls like
[turn:foo?transport=udp, turn:foo?transport=tcp, turns:foo?transport=tcp]
then the ICE engine may interpret this as "just get me the best candidate from any of these servers (which would at least be the same physical entity). So if you know you have a relay candidate over turn/udp that is all you need and you could stop gathering turn/tcp and turn/tls candidates.
This optimization reduces the number of candidates and therefore checking times etc.

But we haven't written that down anywhere what this "represent" means.

@nils-ohlmeier
Copy link

To be honest I never really understood the difference between the list of servers and the list of URLs.
It make to some degree sense if the list of servers means "try to get responses from all of these" and the list of URL means "a response from just of these is enough". But as @fippo pointed out I think that expected behavior needs to be better documented.

@stefhak
Copy link
Contributor

stefhak commented Jul 27, 2017

@pthatcherg could you comment on this one?

@taylor-b
Copy link
Contributor

I asked Peter, and he said he doesn't recall there being any intention to treat a list of servers with one URL differently from one server with multiple URLs. In other words, they all get flattened to the same thing. He says "if there's logic to discard a TCP candidate if a UDP candidate could be allocated, that should happen regardless of how the URLs are grouped".

So I'd propose closing this, and just making the RTCIceServer-flattening more clear, if it isn't already;
after @fippo's PR to change the indentation of some steps, we may be fine.

@stefhak
Copy link
Contributor

stefhak commented Aug 17, 2017

@taylor-b do you think we can close this one now?

@taylor-b
Copy link
Contributor

#1457 was merged, so that's fine by me.

@taylor-b taylor-b closed this Aug 24, 2017
@fippo fippo deleted the fix-1198 branch August 24, 2017 08:11
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.

RTCIceServer "represents a TURN server"
5 participants