From e5011eb74c718f1ff254c934d502f9e978426ade Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Sun, 4 Aug 2024 20:57:04 -0700 Subject: [PATCH 1/8] use native buffer. this depedency is not necessary for node 18+ --- packages/rtp/package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/rtp/package.json b/packages/rtp/package.json index 80b4b4d3..ee395984 100644 --- a/packages/rtp/package.json +++ b/packages/rtp/package.json @@ -43,7 +43,6 @@ "@minhducsun2002/leb128": "^1.0.0", "aes-js": "^3.1.2", "@shinyoshiaki/binary-data": "^0.6.1", - "buffer": "^6.0.3", "debug": "^4.3.4", "@shinyoshiaki/jspack": "^0.0.6", "mp4box": "^0.5.2", From 6b47fb38dec5c6a10563c9d94543b351ead1b7d7 Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Mon, 5 Aug 2024 11:37:20 -0700 Subject: [PATCH 2/8] fix race condition in local certificate creation --- packages/webrtc/src/transport/dtls.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/webrtc/src/transport/dtls.ts b/packages/webrtc/src/transport/dtls.ts index 3964b550..7454e4c3 100644 --- a/packages/webrtc/src/transport/dtls.ts +++ b/packages/webrtc/src/transport/dtls.ts @@ -52,6 +52,7 @@ export class RTCDtlsTransport { readonly onStateChange = new Event<[DtlsState]>(); localCertificate?: RTCCertificate; + localCertificatePromise?: Promise; private remoteParameters?: RTCDtlsParameters; constructor( @@ -72,7 +73,15 @@ export class RTCDtlsTransport { } async setupCertificate() { - if (!this.localCertificate) { + if (this.localCertificate) { + return this.localCertificate; + } + + if (this.localCertificatePromise) { + return this.localCertificatePromise; + } + + this.localCertificatePromise = (async () => { const { certPem, keyPem, signatureHash } = await CipherContext.createSelfSignedCertificateWithKey( { @@ -86,8 +95,10 @@ export class RTCDtlsTransport { certPem, signatureHash, ); - } - return this.localCertificate; + return this.localCertificate; + })(); + + return this.localCertificatePromise; } setRemoteParams(remoteParameters: RTCDtlsParameters) { From 788ec0a5fd4505f2f868dab027a7dda684a0fc87 Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Mon, 5 Aug 2024 14:53:10 -0700 Subject: [PATCH 3/8] fix event emitter warning if there are many ice candidates --- packages/ice/src/dns/lookup.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/ice/src/dns/lookup.ts b/packages/ice/src/dns/lookup.ts index 6b3613f7..7d2c38dd 100644 --- a/packages/ice/src/dns/lookup.ts +++ b/packages/ice/src/dns/lookup.ts @@ -15,6 +15,10 @@ export class MdnsLookup { cache = new Map>(); mdnsInstance = mdns(); + constructor() { + this.mdnsInstance.setMaxListeners(50); + } + lookup(host: string): Promise { return new Promise((r, f) => { const cleanup = () => { From 4625bc01f0481fa2a883e2fa97655355a12f501e Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Mon, 5 Aug 2024 15:24:12 -0700 Subject: [PATCH 4/8] fix ice candidates gathered during setRemoteDescription offer and dropped on the floor --- packages/webrtc/src/peerConnection.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/webrtc/src/peerConnection.ts b/packages/webrtc/src/peerConnection.ts index 6f881358..2bf56262 100644 --- a/packages/webrtc/src/peerConnection.ts +++ b/packages/webrtc/src/peerConnection.ts @@ -508,8 +508,6 @@ export class RTCPeerConnection extends EventTarget { }); iceTransport.iceGather.onIceCandidate = (candidate) => { - if (!this.localDescription) return; - if (this.config.bundlePolicy === "max-bundle" || this.remoteIsBundled) { candidate.sdpMLineIndex = 0; const media = this._localDescription?.media[0]; From 44ff6a68ec1b5da13a6ece907eea9de16fbabe9c Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Mon, 5 Aug 2024 18:17:25 -0700 Subject: [PATCH 5/8] proper fix that only connects ice when local description is set --- packages/webrtc/src/peerConnection.ts | 32 ++++++++++----------------- packages/webrtc/tests/utils.ts | 15 ++++++++++--- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/packages/webrtc/src/peerConnection.ts b/packages/webrtc/src/peerConnection.ts index 2bf56262..cc32f2ae 100644 --- a/packages/webrtc/src/peerConnection.ts +++ b/packages/webrtc/src/peerConnection.ts @@ -508,6 +508,10 @@ export class RTCPeerConnection extends EventTarget { }); iceTransport.iceGather.onIceCandidate = (candidate) => { + if (!this.localDescription) { + console.warn("localDescription not found when ice candidate was gathered"); + return; + } if (this.config.bundlePolicy === "max-bundle" || this.remoteIsBundled) { candidate.sdpMLineIndex = 0; const media = this._localDescription?.media[0]; @@ -651,14 +655,6 @@ export class RTCPeerConnection extends EventTarget { // for trickle ice this.setLocal(description); - // connect transports - if (description.type === "answer") { - this.connect().catch((err) => { - log("connect failed", err); - this.setConnectionState("failed"); - }); - } - // # gather candidates const connected = this.iceTransports.find( (transport) => transport.state === "connected", @@ -674,6 +670,14 @@ export class RTCPeerConnection extends EventTarget { ); } + // connect transports + if (description.type === "answer") { + this.connect().catch((err) => { + console.log("connect failed", err); + this.setConnectionState("failed"); + }); + } + description.media .filter((m) => ["audio", "video"].includes(m.kind)) .forEach((m, i) => { @@ -996,18 +1000,6 @@ export class RTCPeerConnection extends EventTarget { }); } - const connected = this.iceTransports.find( - (transport) => transport.state === "connected", - ); - if (this.remoteIsBundled && connected) { - // no need to gather ice candidates on an existing bundled connection - await connected.iceGather.gather(); - } else { - await Promise.allSettled( - transports.map((iceTransport) => iceTransport.iceGather.gather()), - ); - } - this.negotiationneeded = false; if (this.shouldNegotiationneeded) { this.needNegotiation(); diff --git a/packages/webrtc/tests/utils.ts b/packages/webrtc/tests/utils.ts index 00661f24..937267fc 100644 --- a/packages/webrtc/tests/utils.ts +++ b/packages/webrtc/tests/utils.ts @@ -100,12 +100,21 @@ async function exchangeAnswer( caller: RTCPeerConnection, callee: RTCPeerConnection, ) { + // @koush: This note below does not seem correct. The answer should be + // applied to setLocalDescription first. + // Only after setLocalDescription is applied should ice candidates + // be generated. This should happen on next tick. + // I have reversed the order + const answer = await callee.createAnswer(); + await callee.setLocalDescription(answer); + await caller.setRemoteDescription(answer); + // Note that caller's remote description must be set first; if not, // there's a chance that candidates from callee arrive at caller before // it has a remote description to apply them to. - const answer = await callee.createAnswer(); - await caller.setRemoteDescription(answer); - await callee.setLocalDescription(answer); + // const answer = await callee.createAnswer(); + // await caller.setRemoteDescription(answer); + // await callee.setLocalDescription(answer); } export function awaitMessage(channel: RTCDataChannel) { From 560cb32cfbaeea712f1c8c76115f6ac84d0ee992 Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Tue, 6 Aug 2024 10:39:44 -0700 Subject: [PATCH 6/8] Revert "use native buffer. this depedency is not necessary for node 18+" This reverts commit e5011eb74c718f1ff254c934d502f9e978426ade. --- packages/rtp/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/rtp/package.json b/packages/rtp/package.json index ee395984..80b4b4d3 100644 --- a/packages/rtp/package.json +++ b/packages/rtp/package.json @@ -43,6 +43,7 @@ "@minhducsun2002/leb128": "^1.0.0", "aes-js": "^3.1.2", "@shinyoshiaki/binary-data": "^0.6.1", + "buffer": "^6.0.3", "debug": "^4.3.4", "@shinyoshiaki/jspack": "^0.0.6", "mp4box": "^0.5.2", From be489889fa6dac566c2ca585d93d64b29e6802f5 Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Tue, 6 Aug 2024 10:47:00 -0700 Subject: [PATCH 7/8] use log --- packages/webrtc/src/peerConnection.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/webrtc/src/peerConnection.ts b/packages/webrtc/src/peerConnection.ts index cc32f2ae..c65f4593 100644 --- a/packages/webrtc/src/peerConnection.ts +++ b/packages/webrtc/src/peerConnection.ts @@ -509,7 +509,7 @@ export class RTCPeerConnection extends EventTarget { iceTransport.iceGather.onIceCandidate = (candidate) => { if (!this.localDescription) { - console.warn("localDescription not found when ice candidate was gathered"); + log("localDescription not found when ice candidate was gathered"); return; } if (this.config.bundlePolicy === "max-bundle" || this.remoteIsBundled) { @@ -673,7 +673,7 @@ export class RTCPeerConnection extends EventTarget { // connect transports if (description.type === "answer") { this.connect().catch((err) => { - console.log("connect failed", err); + log("connect failed", err); this.setConnectionState("failed"); }); } From e379126007641311b456e3219685fd0325e743c5 Mon Sep 17 00:00:00 2001 From: Koushik Dutta Date: Tue, 6 Aug 2024 19:03:05 -0700 Subject: [PATCH 8/8] revert test change, fix race condition in candidate pairing --- packages/ice/src/ice.ts | 37 ++++++++++++++++++++++------------ packages/webrtc/tests/utils.ts | 15 +++----------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/packages/ice/src/ice.ts b/packages/ice/src/ice.ts index 93d2cc81..eb71412c 100644 --- a/packages/ice/src/ice.ts +++ b/packages/ice/src/ice.ts @@ -157,6 +157,7 @@ export class Connection { "host", ); + this.pairLocalProtocol(protocol) candidates.push(protocol.localCandidate); if (cb) { cb(protocol.localCandidate); @@ -911,22 +912,32 @@ export class Connection { } } - private pairRemoteCandidate = (remoteCandidate: Candidate) => { - for (const protocol of this.protocols) { + private tryPair(protocol: Protocol, remoteCandidate: Candidate) { + if ( + protocol.localCandidate?.canPairWith(remoteCandidate) && + !this.findPair(protocol, remoteCandidate) + ) { + const pair = new CandidatePair(protocol, remoteCandidate); if ( - protocol.localCandidate?.canPairWith(remoteCandidate) && - !this.findPair(protocol, remoteCandidate) + this.options.filterCandidatePair && + !this.options.filterCandidatePair(pair) ) { - const pair = new CandidatePair(protocol, remoteCandidate); - if ( - this.options.filterCandidatePair && - !this.options.filterCandidatePair(pair) - ) { - continue; - } - this.checkList.push(pair); - this.setPairState(pair, CandidatePairState.WAITING); + return; } + this.checkList.push(pair); + this.setPairState(pair, CandidatePairState.WAITING); + } + } + + private pairLocalProtocol(protocol: Protocol) { + for (const remoteCandidate of this.remoteCandidates) { + this.tryPair(protocol, remoteCandidate); + } + } + + private pairRemoteCandidate = (remoteCandidate: Candidate) => { + for (const protocol of this.protocols) { + this.tryPair(protocol, remoteCandidate); } }; diff --git a/packages/webrtc/tests/utils.ts b/packages/webrtc/tests/utils.ts index 937267fc..00661f24 100644 --- a/packages/webrtc/tests/utils.ts +++ b/packages/webrtc/tests/utils.ts @@ -100,21 +100,12 @@ async function exchangeAnswer( caller: RTCPeerConnection, callee: RTCPeerConnection, ) { - // @koush: This note below does not seem correct. The answer should be - // applied to setLocalDescription first. - // Only after setLocalDescription is applied should ice candidates - // be generated. This should happen on next tick. - // I have reversed the order - const answer = await callee.createAnswer(); - await callee.setLocalDescription(answer); - await caller.setRemoteDescription(answer); - // Note that caller's remote description must be set first; if not, // there's a chance that candidates from callee arrive at caller before // it has a remote description to apply them to. - // const answer = await callee.createAnswer(); - // await caller.setRemoteDescription(answer); - // await callee.setLocalDescription(answer); + const answer = await callee.createAnswer(); + await caller.setRemoteDescription(answer); + await callee.setLocalDescription(answer); } export function awaitMessage(channel: RTCDataChannel) {