From df61f8a304ef184f03c0c2eafb14ad07677e2fac Mon Sep 17 00:00:00 2001 From: David Rowe Date: Tue, 26 Oct 2021 09:58:05 +1300 Subject: [PATCH 1/8] Add WebRTC debug --- .../networking/webrtc/WebRTCDataChannel.ts | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/domain/networking/webrtc/WebRTCDataChannel.ts b/src/domain/networking/webrtc/WebRTCDataChannel.ts index 2a528b8e..3a002b10 100644 --- a/src/domain/networking/webrtc/WebRTCDataChannel.ts +++ b/src/domain/networking/webrtc/WebRTCDataChannel.ts @@ -107,6 +107,7 @@ class WebRTCDataChannel { #_nodeType = NodeType.Unassigned; + #_nodeTypeName = ""; #_signalingChannel: WebRTCSignalingChannel | null = null; #_peerConnection: RTCPeerConnection | null = null; @@ -119,9 +120,12 @@ class WebRTCDataChannel { #_oncloseCallback: OnCloseCallback | null = null; #_onerrorCallback: OnErrorCallback | null = null; + #_DEBUG = false; + constructor(nodeType: NodeTypeValue, signalingChannel: WebRTCSignalingChannel) { this.#_nodeType = nodeType; + this.#_nodeTypeName = NodeType.getNodeTypeName(nodeType); this.#_signalingChannel = signalingChannel; this.#_readyState = WebRTCDataChannel.CONNECTING; setTimeout(() => { @@ -230,8 +234,14 @@ class WebRTCDataChannel { // Send ICE candidates to the domain server. this.#_peerConnection.onicecandidate = ({ candidate }) => { + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Obtained ICE candidate.`); + } if (candidate // The candidate is sometimes null for unknown reasons; don't send this. && this.#_signalingChannel && this.#_signalingChannel.readyState === WebRTCSignalingChannel.OPEN) { + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Send ICE candidate.`); + } this.#_signalingChannel.send({ to: this.#_nodeType, data: candidate }); } }; @@ -244,10 +254,16 @@ class WebRTCDataChannel { } try { // Create offer. + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Create offer.`); + } const offer = await this.#_peerConnection.createOffer(); await this.#_peerConnection.setLocalDescription(offer); // Send offer to domain server. + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Send offer.`); + } this.#_signalingChannel.send({ to: this.#_nodeType, data: { description: this.#_peerConnection.localDescription } @@ -263,6 +279,10 @@ class WebRTCDataChannel { // Observe connection state changes. this.#_peerConnection.onconnectionstatechange = () => { + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Connection state changed:`, + this.#_peerConnection?.connectionState); + } let errorMessage = ""; switch (this.#_peerConnection ? this.#_peerConnection.connectionState : "") { case "new": @@ -363,6 +383,10 @@ class WebRTCDataChannel { try { if (description) { + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Received description.`); + } + if (!this.#_peerConnection) { const errorMessage = "WebRTCDataChannel: Peer connection is closed!"; console.error(errorMessage); @@ -376,14 +400,23 @@ class WebRTCDataChannel { await this.#_peerConnection.setRemoteDescription(description); // We got an offer; reply with an answer. + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Description is offer.`); + } if (description.type === "offer" && this.#_signalingChannel) { await this.#_peerConnection.setLocalDescription(description); + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Send local description.`); + } this.#_signalingChannel.send({ description: this.#_peerConnection.localDescription }); } } else if (candidate) { // Add ICE candidate to peer connection. + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Received ICE candidate.`); + } if (this.#_peerConnection) { await this.#_peerConnection.addIceCandidate(candidate); } From e4bfa709c26cf067b10b34388a3c9ec5f65d241d Mon Sep 17 00:00:00 2001 From: David Rowe Date: Tue, 26 Oct 2021 10:14:01 +1300 Subject: [PATCH 2/8] Fix socket states --- src/domain/networking/udt/Socket.ts | 2 +- src/domain/networking/webrtc/WebRTCSocket.ts | 3 +-- .../domain/networking/webrtc/WebRTCSocket.integration.test.js | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/domain/networking/udt/Socket.ts b/src/domain/networking/udt/Socket.ts index 1080a652..05b42a55 100644 --- a/src/domain/networking/udt/Socket.ts +++ b/src/domain/networking/udt/Socket.ts @@ -59,7 +59,7 @@ class Socket { static readonly #WEBRTCSOCKET_TO_SOCKET_STATES = [ Socket.UNCONNECTED, - Socket.UNCONNECTED, + Socket.CONNECTING, Socket.CONNECTING, Socket.CONNECTED ]; diff --git a/src/domain/networking/webrtc/WebRTCSocket.ts b/src/domain/networking/webrtc/WebRTCSocket.ts index 6003ed6f..c668e0f1 100644 --- a/src/domain/networking/webrtc/WebRTCSocket.ts +++ b/src/domain/networking/webrtc/WebRTCSocket.ts @@ -120,10 +120,9 @@ class WebRTCSocket { // Fall through. } } - if (this.#_webrtcSignalingChannel !== null) { + if (nodeType === NodeTypeValue.DomainServer && this.#_webrtcSignalingChannel !== null) { switch (this.#_webrtcSignalingChannel.readyState) { case WebRTCSignalingChannel.OPEN: - return WebRTCSocket.SIGNALING; case WebRTCSignalingChannel.CONNECTING: return WebRTCSocket.SIGNALING; default: diff --git a/tests/domain/networking/webrtc/WebRTCSocket.integration.test.js b/tests/domain/networking/webrtc/WebRTCSocket.integration.test.js index 39e67740..fd2c923a 100644 --- a/tests/domain/networking/webrtc/WebRTCSocket.integration.test.js +++ b/tests/domain/networking/webrtc/WebRTCSocket.integration.test.js @@ -32,7 +32,7 @@ describe("WebRTCSocket - integration tests", () => { expect(webrtcSocket.state(TestConfig.SERVER_SIGNALING_SOCKET_URL, NodeType.DomainServer)) .toBe(WebRTCSocket.CONNECTED); expect(webrtcSocket.state(TestConfig.SERVER_SIGNALING_SOCKET_URL, NodeType.AudioMixer)) - .toBe(WebRTCSocket.SIGNALING); + .toBe(WebRTCSocket.UNCONNECTED); expect(webrtcSocket.state(TestConfig.SERVER_SIGNALING_SOCKET_URL + "1", NodeType.DomainServer)) .toBe(WebRTCSocket.UNCONNECTED); webrtcSocket.abort(); From 1ac41320838afa9568d893587c9388fb5d700940 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Tue, 26 Oct 2021 10:20:04 +1300 Subject: [PATCH 3/8] Don't use WebRTC audio or video --- src/domain/networking/webrtc/WebRTCDataChannel.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/domain/networking/webrtc/WebRTCDataChannel.ts b/src/domain/networking/webrtc/WebRTCDataChannel.ts index 3a002b10..5f716866 100644 --- a/src/domain/networking/webrtc/WebRTCDataChannel.ts +++ b/src/domain/networking/webrtc/WebRTCDataChannel.ts @@ -257,7 +257,11 @@ class WebRTCDataChannel { if (this.#_DEBUG) { console.debug(`[webrtc] [${this.#_nodeTypeName}] Create offer.`); } - const offer = await this.#_peerConnection.createOffer(); + const rtcOfferOptions = { + offerToReceiveAudio: false, + offerToReceiveVideo: false + }; + const offer = await this.#_peerConnection.createOffer(rtcOfferOptions); await this.#_peerConnection.setLocalDescription(offer); // Send offer to domain server. From 875acf19800213a954cf04c7ffed7e8f17aa4312 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Tue, 26 Oct 2021 11:22:42 +1300 Subject: [PATCH 4/8] Fix ICE servers used for STUN --- src/domain/networking/webrtc/WebRTCDataChannel.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/domain/networking/webrtc/WebRTCDataChannel.ts b/src/domain/networking/webrtc/WebRTCDataChannel.ts index 5f716866..a6a6bf0b 100644 --- a/src/domain/networking/webrtc/WebRTCDataChannel.ts +++ b/src/domain/networking/webrtc/WebRTCDataChannel.ts @@ -102,7 +102,16 @@ class WebRTCDataChannel { static readonly #CONFIGURATION = { // WEBRTC TODO: Make configurable in the API. - iceServers: [{ urls: "stun:ice.vircadia.com:7337" }] + // FIXME: stun:ice.vircadia.com:7337 doesn't work for WebRTC. + iceServers: [ + { + urls: [ + "stun:stun1.l.google.com:19302", + "stun:stun4.l.google.com:19302", + "stun:stun.schlund.de" + ] + } + ] }; From 287bdbd5f8a85da60b9f8e83ede58fd3a0bcca27 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Tue, 26 Oct 2021 12:14:17 +1300 Subject: [PATCH 5/8] Simplify WebRTC code --- .../networking/webrtc/WebRTCDataChannel.ts | 71 +++++++------------ 1 file changed, 27 insertions(+), 44 deletions(-) diff --git a/src/domain/networking/webrtc/WebRTCDataChannel.ts b/src/domain/networking/webrtc/WebRTCDataChannel.ts index a6a6bf0b..7ec1c550 100644 --- a/src/domain/networking/webrtc/WebRTCDataChannel.ts +++ b/src/domain/networking/webrtc/WebRTCDataChannel.ts @@ -10,6 +10,7 @@ import NodeType, { NodeTypeValue } from "../NodeType"; import WebRTCSignalingChannel, { SignalingMessage } from "./WebRTCSignalingChannel"; +import assert from "../../shared/assert"; type OnOpenCallback = () => void; @@ -236,7 +237,9 @@ class WebRTCDataChannel { // Starts making a WebRTC connection. - #start(): void { + async #start(): Promise { + + assert(this.#_signalingChannel !== null); // Create new peer connection object. this.#_peerConnection = new RTCPeerConnection(WebRTCDataChannel.#CONFIGURATION); @@ -246,7 +249,7 @@ class WebRTCDataChannel { if (this.#_DEBUG) { console.debug(`[webrtc] [${this.#_nodeTypeName}] Obtained ICE candidate.`); } - if (candidate // The candidate is sometimes null for unknown reasons; don't send this. + if (candidate // The candidate is sometimes null for unknown reasons; don't send this but do send empty string. && this.#_signalingChannel && this.#_signalingChannel.readyState === WebRTCSignalingChannel.OPEN) { if (this.#_DEBUG) { console.debug(`[webrtc] [${this.#_nodeTypeName}] Send ICE candidate.`); @@ -255,41 +258,6 @@ class WebRTCDataChannel { } }; - // Generate an offer. - this.#_peerConnection.onnegotiationneeded = async () => { - if (!this.#_peerConnection || !this.#_signalingChannel - || this.#_signalingChannel.readyState !== WebRTCSignalingChannel.OPEN) { - return; - } - try { - // Create offer. - if (this.#_DEBUG) { - console.debug(`[webrtc] [${this.#_nodeTypeName}] Create offer.`); - } - const rtcOfferOptions = { - offerToReceiveAudio: false, - offerToReceiveVideo: false - }; - const offer = await this.#_peerConnection.createOffer(rtcOfferOptions); - await this.#_peerConnection.setLocalDescription(offer); - - // Send offer to domain server. - if (this.#_DEBUG) { - console.debug(`[webrtc] [${this.#_nodeTypeName}] Send offer.`); - } - this.#_signalingChannel.send({ - to: this.#_nodeType, - data: { description: this.#_peerConnection.localDescription } - }); - } catch (err) { - const errorMessage = "WebRTCDataChannel: Error during offer negotiation: " + err; - console.error(errorMessage); - if (this.#_onerrorCallback) { - this.#_onerrorCallback(errorMessage); - } - } - }; - // Observe connection state changes. this.#_peerConnection.onconnectionstatechange = () => { if (this.#_DEBUG) { @@ -305,7 +273,7 @@ class WebRTCDataChannel { break; case "connected": // The connection has become fully connected. - // However, _readyState isn't set to OPEN until the data channel has been connected. + // However, #_readyState isn't set to OPEN until the data channel has been connected. break; case "disconnected": case "failed": @@ -362,6 +330,26 @@ class WebRTCDataChannel { } }; + // Create offer. + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Create offer.`); + } + const rtcOfferOptions = { + offerToReceiveAudio: false, + offerToReceiveVideo: false + }; + const offer = await this.#_peerConnection.createOffer(rtcOfferOptions); + await this.#_peerConnection.setLocalDescription(offer); + + // Send offer to domain server. + if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Send offer.`); + } + this.#_signalingChannel.send({ + to: this.#_nodeType, + data: { description: offer } + }); + } // start // Instigates the WebRTC connection process. @@ -389,11 +377,6 @@ class WebRTCDataChannel { return; } - // Start a new peer connection if necessary. - if (!this.#_peerConnection && (description || candidate)) { - this.#start(); - } - try { if (description) { if (this.#_DEBUG) { @@ -455,7 +438,7 @@ class WebRTCDataChannel { }); // Start the WebRTC connection process. - this.#start(); + void this.#start(); } // #connect From 959b2589af1faa98f44ba8e564a4cb00e8195a94 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Tue, 26 Oct 2021 16:28:16 +1300 Subject: [PATCH 6/8] Delay setting local description until have answer from domain server --- .../networking/webrtc/WebRTCDataChannel.ts | 50 ++++++++++++------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/domain/networking/webrtc/WebRTCDataChannel.ts b/src/domain/networking/webrtc/WebRTCDataChannel.ts index 7ec1c550..3927f13c 100644 --- a/src/domain/networking/webrtc/WebRTCDataChannel.ts +++ b/src/domain/networking/webrtc/WebRTCDataChannel.ts @@ -121,6 +121,8 @@ class WebRTCDataChannel { #_signalingChannel: WebRTCSignalingChannel | null = null; #_peerConnection: RTCPeerConnection | null = null; + #_offer: RTCSessionDescriptionInit | null = null; + #_haveSetRemoteDescription = false; #_dataChannel: RTCDataChannel | null = null; #_dataChannelID = 0; #_readyState = WebRTCDataChannel.CLOSED; @@ -249,12 +251,12 @@ class WebRTCDataChannel { if (this.#_DEBUG) { console.debug(`[webrtc] [${this.#_nodeTypeName}] Obtained ICE candidate.`); } - if (candidate // The candidate is sometimes null for unknown reasons; don't send this but do send empty string. + if (candidate // The candidate is sometimes null; don't send this but do send empty string. && this.#_signalingChannel && this.#_signalingChannel.readyState === WebRTCSignalingChannel.OPEN) { if (this.#_DEBUG) { console.debug(`[webrtc] [${this.#_nodeTypeName}] Send ICE candidate.`); } - this.#_signalingChannel.send({ to: this.#_nodeType, data: candidate }); + this.#_signalingChannel.send({ to: this.#_nodeType, data: { candidate } }); } }; @@ -338,8 +340,10 @@ class WebRTCDataChannel { offerToReceiveAudio: false, offerToReceiveVideo: false }; - const offer = await this.#_peerConnection.createOffer(rtcOfferOptions); - await this.#_peerConnection.setLocalDescription(offer); + this.#_offer = await this.#_peerConnection.createOffer(rtcOfferOptions); + // Don't set the local description until we have the remote answer because setting the local description triggers ICE + // candidate gathering and the remote isn't ready to handle them yet. + this.#_haveSetRemoteDescription = false; // Send offer to domain server. if (this.#_DEBUG) { @@ -347,7 +351,7 @@ class WebRTCDataChannel { } this.#_signalingChannel.send({ to: this.#_nodeType, - data: { description: offer } + data: { description: this.#_offer } }); } // start @@ -392,29 +396,37 @@ class WebRTCDataChannel { return; } - // Add remote connection information to peer connection. - await this.#_peerConnection.setRemoteDescription(description); - - // We got an offer; reply with an answer. + // We got an answer. if (this.#_DEBUG) { - console.debug(`[webrtc] [${this.#_nodeTypeName}] Description is offer.`); + console.debug(`[webrtc] [${this.#_nodeTypeName}] Description is ${description.type}.`); } - if (description.type === "offer" && this.#_signalingChannel) { - await this.#_peerConnection.setLocalDescription(description); - if (this.#_DEBUG) { - console.debug(`[webrtc] [${this.#_nodeTypeName}] Send local description.`); + if (description.type === "answer" && this.#_signalingChannel) { + assert(this.#_offer !== null); + + // The server is ready to handle ICE candidates so set we can set the local description now. + await this.#_peerConnection.setLocalDescription(this.#_offer); + + await this.#_peerConnection.setRemoteDescription(description); + this.#_haveSetRemoteDescription = true; + } else { + const errorMessage = `WebRTCDataChannel: Unexpected answer! ${description.type}`; + console.error(errorMessage); + if (this.#_onerrorCallback) { + this.#_onerrorCallback(errorMessage); } - this.#_signalingChannel.send({ - description: this.#_peerConnection.localDescription - }); } } else if (candidate) { - // Add ICE candidate to peer connection. + // Add ICE candidate to the peer connection. + // Don't set unless the remote description has been set, otherwise an error is generated. The first ICE + // candidate from the server may arrive before the remote description has been set because of the delay + // introduced by setting the local description just before setting the remote description. if (this.#_DEBUG) { console.debug(`[webrtc] [${this.#_nodeTypeName}] Received ICE candidate.`); } - if (this.#_peerConnection) { + if (this.#_peerConnection && this.#_haveSetRemoteDescription) { await this.#_peerConnection.addIceCandidate(candidate); + } else if (this.#_DEBUG) { + console.debug(`[webrtc] [${this.#_nodeTypeName}] Skipped adding ICE candidate.`); } } else if (echo) { // Ignore signaling channel "echo" messages. From dff3cfab0102715aef1dab9e9deb58a6a723d113 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Tue, 26 Oct 2021 20:05:07 +1300 Subject: [PATCH 7/8] Typo --- src/domain/networking/NodeList.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domain/networking/NodeList.ts b/src/domain/networking/NodeList.ts index 5dd15cba..34fa954e 100644 --- a/src/domain/networking/NodeList.ts +++ b/src/domain/networking/NodeList.ts @@ -423,7 +423,7 @@ class NodeList extends LimitedNodeList { #startNodeHolePunch = (node: Node): void => { // C++ void startNodeHolePunch(const Node* node); // While we don't need to do hole punching per se because WebRTC handles this, we initiate opening the WebRTC data - // channel and adopt the native client's use of pings and replys to coordinate setting up communications with the + // channel and adopt the native client's use of pings and replies to coordinate setting up communications with the // assignment client. // WebRTC: Initiate opening the WebRTC data channel. From 33a92f0664fb10f8d1570bd03bf1dac4d4078239 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Wed, 27 Oct 2021 21:40:16 +1300 Subject: [PATCH 8/8] Reduce the number of ICE servers used --- src/domain/networking/webrtc/WebRTCDataChannel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domain/networking/webrtc/WebRTCDataChannel.ts b/src/domain/networking/webrtc/WebRTCDataChannel.ts index 3927f13c..a3602a43 100644 --- a/src/domain/networking/webrtc/WebRTCDataChannel.ts +++ b/src/domain/networking/webrtc/WebRTCDataChannel.ts @@ -104,11 +104,11 @@ class WebRTCDataChannel { static readonly #CONFIGURATION = { // WEBRTC TODO: Make configurable in the API. // FIXME: stun:ice.vircadia.com:7337 doesn't work for WebRTC. + // Firefox warns: "WebRTC: Using more than two STUN/TURN servers slows down discovery" iceServers: [ { urls: [ "stun:stun1.l.google.com:19302", - "stun:stun4.l.google.com:19302", "stun:stun.schlund.de" ] }