Skip to content

Commit

Permalink
Fix #176. Update to the latest IceServer definition
Browse files Browse the repository at this point in the history
Old:

dictionary RTCIceServer {
    DOMString  url;
    DOMString? credential;
};

New:

dictionary RTCIceServer {
    (DOMString or sequence<DOMString>) urls;
    DOMString?                         username = null;
    DOMString?                         credential;
};

Note: Current Firefox Nightly "28.0a1 (2013-11-18)" Does not support multiple 'urls'
  • Loading branch information
jmillan committed Nov 19, 2013
1 parent ed643f7 commit 62e8323
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 15 deletions.
13 changes: 4 additions & 9 deletions src/RTCSession/RTCMediaHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,24 +105,19 @@ RTCMediaHandler.prototype = {
* @param {Function} onSuccess Fired when there are no more ICE candidates
*/
init: function(constraints) {
var idx, length, server, scheme, url,
var idx, length, server,
self = this,
servers = [],
config = this.session.ua.configuration;

length = config.stun_servers.length;
for (idx = 0; idx < length; idx++) {
server = config.stun_servers[idx];
servers.push({'url': server});
}
servers.push({'url': config.stun_servers});

length = config.turn_servers.length;
for (idx = 0; idx < length; idx++) {
server = config.turn_servers[idx];
url = server.server;
scheme = url.substr(0, url.indexOf(':'));
servers.push({
'url': scheme + ':' + server.username + '@' + url.substr(scheme.length+1),
'url': server.urls,
'username': server.username,
'credential': server.password
});
}
Expand Down
30 changes: 24 additions & 6 deletions src/UA.js
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,7 @@ UA.configuration_check = {
},

turn_servers: function(turn_servers) {
var idx, length, turn_server;
var idx, length, turn_server, url;

if (turn_servers instanceof Array) {
// Do nothing
Expand All @@ -1099,14 +1099,32 @@ UA.configuration_check = {
length = turn_servers.length;
for (idx = 0; idx < length; idx++) {
turn_server = turn_servers[idx];
if (!turn_server.server || !turn_server.username || !turn_server.password) {

// Backward compatibility:
//Allow defining the turn_server url with the 'server' property.
if (turn_server.server) {
turn_server.urls = [turn_server.server];
}

if (!turn_server.urls || !turn_server.username || !turn_server.password) {
return;
} else if (!(/^turns?:/.test(turn_server.server))) {
turn_server.server = 'turn:' + turn_server.server;
}

if (!turn_server.urls instanceof Array) {
turn_server.urls = [turn_server.urls];
}

length = turn_server.urls.length;
for (idx = 0; idx < length; idx++) {
url = turn_server.urls[idx];

if (!(/^turns?:/.test(url))) {
url = 'turn:' + url;
}

if(JsSIP.Grammar.parse(turn_server.server, 'turn_URI') === -1) {
return;
if(JsSIP.Grammar.parse(url, 'turn_URI') === -1) {
return;
}
}
}
return turn_servers;
Expand Down

6 comments on commit 62e8323

@rscreene
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there no stun_servers specified then the following line:
servers.push({'url': config.stun_servers});
add an empty url element which causes Chrome and Firefox to complain.

Reverting to the previous code works better!

@jmillan
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A default 'stun_servers' value is set in case you don't define it, so it is never empty.

Please, paste here the error you are getting, and the full JsSIP log.

@rscreene
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the problem only appears if I set "stun_servers" as [] in the configuration object. If I omit it entirely or set a string within the array it works fine.

I can live with that! Thanks for looking.

@jmillan
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a commit in the 'develop' branch that addresses the empty array as an empty configuration option. 0a5bccd

Thanks

@mEkblom
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix broke our implementation. An empty array should be allowed (with the meaning: do not use any STUN servers). Using WebRTC without STUN servers works fine (under some network conditions) in current browsers and should be possible. Please revert to previous state, with for loop.

@ibc
Copy link
Member

@ibc ibc commented on 62e8323 Mar 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's address this in #206.

Please sign in to comment.