libp2p / specs

Technical specifications for the libp2p networking stack
https://libp2p.io
1.58k stars 275 forks source link

webrtc: exchanging ICE candidates #584

Closed achingbrain closed 1 year ago

achingbrain commented 1 year ago

There are only a finite amount of ICE candidates a peer will send during connection negotiation.

The js implementation detects that the remote has sent all of their candidates by receiving an message with type ICE_CANDIDATE with the data field set as an empty string.

Once a candidate message with an empty string data field is received it stops reading from the signalling stream.

This is not in the spec, I think it's just a side-effect of the RTCPeerConnectionIceEvent.candidate property being null when there are no more candidates for this negotiation session.

Do we care about this or would it be better to specify this properly?

E.g. to add an additional explicit ICE_CANDIDATES_END message that means "no more candidates will be received, you should wait for either the peer connection to become connected, the peer connection to fail or for an implementation-specific timeout to elapse"?

cc @sukunrt @mxinden @marten-seemann

marten-seemann commented 1 year ago

Do we need to specify anything? How does ICE handle this?

achingbrain commented 1 year ago

Do we need to specify anything?

An explicit flag would make things easier to debug in the future. Inferring the end of ICE candidates by receiving an empty string feels like it was introduced by a quirk of the browser API.

How does ICE handle this?

Trickle ICE has an "end of candidates indication" so this would be consistent with that:

Once all candidate gathering is completed or expires for an ICE session associated with a specific data stream, the agent will generate an "end-of-candidates" indication for that session and convey it to the remote agent via the signaling channel.

https://datatracker.ietf.org/doc/html/rfc8838#section-13

thomaseizinger commented 1 year ago

How does ICE handle this?

Trickle ICE has an "end of candidates indication" so this would be consistent with that:

Once all candidate gathering is completed or expires for an ICE session associated with a specific data stream, the agent will generate an "end-of-candidates" indication for that session and convey it to the remote agent via the signaling channel.

datatracker.ietf.org/doc/html/rfc8838#section-13

FWIW, we use this in webrtc-direct: https://github.com/libp2p/rust-libp2p/blob/7d1d67cad3847a845ad50d9e56b3b68ca53f5e22/misc/webrtc-utils/src/sdp.rs#L95

achingbrain commented 1 year ago

Just came across this in the w3 spec:

NOTE The icecandidate event is used for three different types of indications: A candidate has been gathered. The candidate member of the event will be populated normally. It should be signaled to the remote peer and passed into addIceCandidate.

An RTCIceTransport has finished gathering a generation of candidates, and is providing an end-of-candidates indication as defined by Section 8.2 of [RFC8838]. This is indicated by candidate.candidate being set to an empty string. The candidate object should be signaled to the remote peer and passed into addIceCandidate like a typical ICE candidate, in order to provide the end-of-candidates indication to the remote peer.

All RTCIceTransports have finished gathering candidates, and the RTCPeerConnection's RTCIceGatheringState has transitioned to "complete". This is indicated by the candidate member of the event being set to null. This only exists for backwards compatibility, and this event does not need to be signaled to the remote peer. It's equivalent to an icegatheringstatechange event with the "complete" state.

https://www.w3.org/TR/webrtc/#rtcpeerconnectioniceevent

Since the data field in the signalling protobuf is optional it can encompass all three uses - a stringified JSON representation of an object for a candidate, an empty string for "end-of-candidates for this generation" or omit the value (or pass "null" since JSON.parse will return null?) for "end-of-candidates full stop".

Though this still feels more like by accident than on purpose. For the sake of debugability I would prefer this to be specified properly and to not have to rely on magic values but I guess there's nothing to do here since we can signal "end-of-candidates" in a way that mirrors with the w3 spec.