feross / simple-peer

📡 Simple WebRTC video, voice, and data channels
MIT License
7.44k stars 974 forks source link

Duplicate candidates when iceCandidatePoolSize and trickle are enabled. #469

Open guanzo opened 5 years ago

guanzo commented 5 years ago

Using simple-peer@9.3.0 Demo: https://jsfiddle.net/guanzo/gupdrm1a/5/

Setting the iceCandidatePoolSize option enables candidate prefetching to occur before setLocalDescription is called.

When creating an initiating peer like so:

const p = new SimplePeer({ 
    initiator: true, 
    trickle: true, 
    config: { 
        iceCandidatePoolSize: 5 
    } 
})

The sdp offer in the first signal event contains a prefetched host candidate. Problem is, the next signal event which contains a trickled candidate, is the same candidate as the prefetched one. Is this supposed to happen? It seems redundant to send the same candidate twice to the other peer.

Example:

1st signal

v=0
o=- 965062851818018781 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0
a=msid-semantic: WMS
m=application 39452 DTLS/SCTP 5000
c=IN IP4 192.168.1.92
/// Here's the prefetched candidate ///
a=candidate:1769160098 1 udp 2113937152 192.168.1.92 39452 typ host generation 0 network-cost 999
a=ice-ufrag:4xkg
a=ice-pwd:c2DfHfJZ4J4+K4tuqHVv//bq
a=ice-options:trickle
a=fingerprint:sha-256 32:AE:E1:ED:83:9E:9A:CC:FE:9B:32:D9:7F:6F:C6:61:71:13:3E:84:8C:B2:D6:64:E3:44:D3:1C:4C:B3:DD:85
a=setup:actpass
a=mid:0
a=sctpmap:5000 webrtc-datachannel 1024

2nd signal. It's the same as the prefetched candidate!? candidate:1769160098 1 udp 2113937152 192.168.1.92 39452 typ host generation 0 ufrag 4xkg network-cost 999

3rd signal candidate:842163049 1 udp 1677729535 73.189.204.6 39452 typ srflx raddr 192.168.1.92 rport 39452 generation 0 ufrag 4xkg network-cost 999

Is it safe to not send the 2nd signal to the signaling server, seeing how it's already been sent in the initial offer? Trickling candidates puts a lot more load on my server than non-trickle, so I'll take any opportunity to reduce the amount of messaging required.

t-mullen commented 5 years ago

Yes, it's safe to drop ice candidates that have been sent another way (either by not sending the 2nd message or by filtering out the relevant lines from the offer).

However, based on the candidate type, you may want to consider removing the duplicate line from the SDP instead. reflex and relay candidates should be trickled.

https://tools.ietf.org/html/draft-ietf-mmusic-trickle-ice-02#section-6.1:

   Trickle ICE agents MAY include any set of candidates in an offer.
   This includes the possibility of generating one with no candidates,
   or one that contains all the candidates that the agent is planning on
   using in the following session.

   For optimal performance, it is RECOMMENDED that an initial offer
   contains host candidates only.  This would allow both agents to start
   gathering server reflexive, relayed and other non-host candidates
   simultaneously, and it would also enable them to begin connectivity
   checks.
fippo commented 5 years ago

at first I thought this was a chrome bug as the spec is pretty clear that the candidates should only be surfaced via the candidate event. As looking at the fiddle in chrome://webrtc-internals shows, the SDP from createOffer and setLocalDescription doesn't contain those candidates however. What simple-peer does is accessing peerconnection.localDescription which has already been updated with some pre-gathered candidates (before they are emitted even; known issue) instead of signaling the offer that was generated by createOffer.

btw, its unlikely you need a iceCandidatePoolSize of 5. Note that this is not the number of candidates to pre-gather but the number of components (one for sctp, one for audio, one for video, etc; and with bundle you rarely need more than 1 even)