EricssonResearch / openwebrtc

A cross-platform WebRTC client framework based on GStreamer
http://www.openwebrtc.org
BSD 2-Clause "Simplified" License
1.8k stars 537 forks source link

OpenWebRTC returns the same priority number multiple times #522

Open NCommander opened 8 years ago

NCommander commented 8 years ago

When getting ICE candidates on a data session (not sure if its specific to data sessions), candidates with the same IP vs. different methods use the same priority number. This is a violation of RFC 5245, section 4.1.2.1.

To quote: More generally, if there are multiple candidates for a particular component for a particular media stream that have the same type, the local preference MUST be unique for each one.

Firefox catches this and throws an ICE error, and only accepts the first candidate it got with a given priority (according to about:webrtc). As for Chrome; it appears to not care.

NCommander commented 8 years ago

Here's an offer/answer reply from Firefox to a OpenWebRTC client. Rereading the specification and the code, I'm not sure anymore who's erring here, us, libnice (where the priority number comes from), or Firefox. I'm willing to write a patch, but I'd like to patch the right thing :)

Received : {"type":"offer","data":{"sdp":{"version":0,"originator":{"username":"mozilla...THIS_IS_SDPARTA-42.0","sessionId":"1401626202004890786","sessionVersion":0,"netType":"IN","addressType":"IP4","address":"0.0.0.0"},"sessionName":"-","startTime":0,"stopTime":0,"mediaDescriptions":[{"type":"application","port":9,"protocol":"DTLS/SCTP","netType":"IN","addressType":"IP4","address":"0.0.0.0","mode":"sendrecv","ssrcs":[1020872779],"cname":"{975deacb-b134-4f9e-8fab-e28083e3155a}","ice":{"ufrag":"d3fafa13","password":"df067a1c07ea982898362d2993330855","candidates":[{"foundation":"0","componentId":1,"transport":"UDP","priority":2122121471,"address":"192.168.1.110","port":52857,"type":"host"},{"foundation":"1","componentId":1,"transport":"UDP","priority":2122187007,"address":"2001:470:1f07:10f5:f087:a139:aad0:9d43","port":42169,"type":"host"},{"foundation":"2","componentId":1,"transport":"UDP","priority":2122252543,"address":"2001:470:1f07:10f5:3285:a9ff:fe3c:9d99","port":56061,"type":"host"}]},"dtls":{"fingerprintHashFunction":"sha-256","fingerprint":"77:DA:AF:36:E7:4C:7A:21:A7:EB:D4:A9:62:26:11:F5:52:27:6A:66:40:FF:C1:C5:03:C8:A1:53:37:C7:55:45","setup":"actpass"},"sctp":{"port":5000,"app":"webrtc-datachannel","streams":256}}]}}} 1E:FA:8E:9B:92:C6:42:D4:D8:0F:73:1C:35:F2:D1:34:B5:DE:9D:37:02:3C:63:49:E4:72:B7:F8:78:F7:EE:8C 1E:FA:8E:9B:92:C6:42:D4:D8:0F:73:1C:35:F2:D1:34:B5:DE:9D:37:02:3C:63:49:E4:72:B7:F8:78:F7:EE:8C Answer: {"type":"answer","data":{"sdp":{"mediaDescriptions":[{"ice":{"ufrag":"/W/f","password":"hIx9cBNpfm7OaZ2ai/XhX7"},"type":"application","protocol":"DTLS/SCTP","addressType":"IP4","address":"0.0.0.0","candidates":[{"foundation":"2","componentId":1,"priority":2013266431,"address":"2001:470:1f07:10f5:3285:a9ff:fe3c:9d99","port":5033,"type":"host"},{"foundation":"3","componentId":1,"priority":1019216383,"address":"2001:470:1f07:10f5:3285:a9ff:fe3c:9d99","port":0,"type":"host","tcpType":"active"},{"foundation":"4","componentId":1,"priority":1015022079,"address":"2001:470:1f07:10f5:3285:a9ff:fe3c:9d99","port":5034,"type":"host","tcpType":"passive"},{"foundation":"5","componentId":1,"priority":2013266431,"address":"2001:470:1f07:10f5:f087:a139:aad0:9d43","port":5026,"type":"host"},{"foundation":"6","componentId":1,"priority":1019216383,"address":"2001:470:1f07:10f5:f087:a139:aad0:9d43","port":0,"type":"host","tcpType":"active"},{"foundation":"7","componentId":1,"priority":1015022079,"address":"2001:470:1f07:10f5:f087:a139:aad0:9d43","port":5075,"type":"host","tcpType":"passive"},{"foundation":"8","componentId":1,"priority":2013266431,"address":"192.168.1.110","port":5032,"type":"host"},{"foundation":"9","componentId":1,"priority":1019216383,"address":"192.168.1.110","port":0,"type":"host","tcpType":"active"},{"foundation":"10","componentId":1,"priority":1015022079,"address":"192.168.1.110","port":5031,"type":"host","tcpType":"passive"}],"sctp":{"port":5000,"app":"webrtc-datachannel","streams":1024},"dtls":{"fingerprintHashFunction":"sha-256","fingerprint":"1E:FA:8E:9B:92:C6:42:D4:D8:0F:73:1C:35:F2:D1:34:B5:DE:9D:37:02:3C:63:49:E4:72:B7:F8:78:F7:EE:8C","setup":"active"}}]}}}

stefhak commented 8 years ago

My understanding is that the priorities must be unique, and that libnice (if they aren't) doesn't do the right thing. That Chrome accepts that doesn't mean it's right.

stefanalund commented 8 years ago

Perhaps @ocrete knows.

ocrete commented 8 years ago

Ooops, this is definitely wrong in libnice. The "lazy" solution is to just add a local counter in nice_candidate_ice_local_preference() and set it in the lower bits of the preference... Patches welcome. I guess the "better" solution includes trying to get more information from the OS in nice_interfaces and passing it around...

superdump commented 8 years ago

@sdroege - can someone patch this up please?

superdump commented 8 years ago

@alessandrod : perhaps this issue is also quick to fix?

ocrete commented 8 years ago

This should be fixed upstream in libnice. Although I have a slightly more correct patch in my dev branch that I will commit when I have time to untangle the whole thing.

superdump commented 8 years ago

@ocrete - of course, all the issues should be fixed upstream in libnice and everything will be upstreamed as long as it's not too hacky to be. :) I'll take a look at the changes since what we're using and master and see if it's a good idea to bump.

ocrete commented 8 years ago

This is fixed upstream now.