From d4ec023afb1449ed0e0c6a198025e53d09055219 Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Fri, 19 Mar 2021 16:48:16 +0800 Subject: [PATCH 1/8] Add session ID for P2P. --- src/sdk/p2p/p2pclient.js | 60 ++++++++++++++++++++------- src/sdk/p2p/peerconnection-channel.js | 10 +++-- test/unit/resources/scripts/p2p.js | 9 +++- 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/src/sdk/p2p/p2pclient.js b/src/sdk/p2p/p2pclient.js index bbc486a6..d4b89c5d 100644 --- a/src/sdk/p2p/p2pclient.js +++ b/src/sdk/p2p/p2pclient.js @@ -92,15 +92,16 @@ const P2PClient = function(configuration, signalingChannel) { signaling.onMessage = function(origin, message) { Logger.debug('Received signaling message from ' + origin + ': ' + message); const data = JSON.parse(message); + const remoteSessionId = data.sessionId; if (data.type === 'chat-closed') { if (channels.has(origin)) { - getOrCreateChannel(origin, false).onMessage(data); + getOrCreateChannel(origin, remoteSessionId).onMessage(data); channels.delete(origin); } return; } if (self.allowedRemoteIds.indexOf(origin) >= 0) { - getOrCreateChannel(origin, false).onMessage(data); + getOrCreateChannel(origin, remoteSessionId).onMessage(data); } else { sendSignalingMessage(origin, 'chat-closed', ErrorModule.errors.P2P_CLIENT_DENIED); @@ -158,8 +159,10 @@ const P2PClient = function(configuration, signalingChannel) { if (state == ConnectionState.READY) { return; } - channels.forEach((channel)=>{ - channel.stop(); + channels.forEach((sessions) => { + sessions.forEach((channel) => { + channel.stop(); + }); }); channels.clear(); signaling.disconnect(); @@ -184,7 +187,7 @@ const P2PClient = function(configuration, signalingChannel) { return Promise.reject(new ErrorModule.P2PError( ErrorModule.errors.P2P_CLIENT_NOT_ALLOWED)); } - return Promise.resolve(getOrCreateChannel(remoteId, true).publish(stream)); + return Promise.resolve(getOrCreateChannel(remoteId).publish(stream)); }; /** @@ -206,7 +209,7 @@ const P2PClient = function(configuration, signalingChannel) { return Promise.reject(new ErrorModule.P2PError( ErrorModule.errors.P2P_CLIENT_NOT_ALLOWED)); } - return Promise.resolve(getOrCreateChannel(remoteId, true).send(message)); + return Promise.resolve(getOrCreateChannel(remoteId).send(message)); }; /** @@ -247,9 +250,13 @@ const P2PClient = function(configuration, signalingChannel) { return channels.get(remoteId).getStats(); }; - const sendSignalingMessage = function(remoteId, type, message) { + const sendSignalingMessage = function( + remoteId, localSessionId, type, message) { const msg = { - type: type, + type, + // Sender side session ID. Sender and receiver have different identity for + // the same session. + sessionId: localSessionId, }; if (message) { msg.data = message; @@ -261,25 +268,46 @@ const P2PClient = function(configuration, signalingChannel) { }); }; - const getOrCreateChannel = function(remoteId, isInitializer) { - if (!channels.has(remoteId)) { + // If `remoteSessionId` is undefined, a channel for `remoteId` will be + // returned. + const getOrCreateChannel = function(remoteId, remoteSessionId) { + if (!channels.has(remoteId) || + (!remoteSessionId && channels.get(remoteId).size() == 0) || + (remoteSessionId && !channels.get(remoteId).has(remoteSessionId))) { + if (!channels.has(remoteId)) { + channels.set(new Map()); + } + // We expect only a single PeerConnection between two endpoints. But we + // still check duplication to avoid potential issues. + let localSessionId = Math.round(Math.random() * 8); + while (channels.get(remoteId).has(remoteSessionId)) { + localSessionId = Math.round(Math.random() * 8); + } // Construct an signaling sender/receiver for P2PPeerConnection. const signalingForChannel = Object.create(EventDispatcher); signalingForChannel.sendSignalingMessage = sendSignalingMessage; - const pcc = new P2PPeerConnectionChannel(config, myId, remoteId, - signalingForChannel, isInitializer); + const pcc = new P2PPeerConnectionChannel( + config, myId, remoteId, localSessionId, remoteSessionId, + signalingForChannel); pcc.addEventListener('streamadded', (streamEvent)=>{ self.dispatchEvent(streamEvent); }); pcc.addEventListener('messagereceived', (messageEvent)=>{ self.dispatchEvent(messageEvent); }); - pcc.addEventListener('ended', ()=>{ - channels.delete(remoteId); + pcc.addEventListener('ended', () => { + channels.get(remoteId).delete(localSessionId); + if (channels.get(remoteId).size() == 0) { + channels.delete(remoteId); + } }); - channels.set(remoteId, pcc); + channels.get(remoteId).get(localSessionId).set(pcc); + } + if (remoteSessionId) { + return channels.get(remoteId).get(remoteSessionId); + } else { + return channels.get(remoteId).values().next().value; } - return channels.get(remoteId); }; }; diff --git a/src/sdk/p2p/peerconnection-channel.js b/src/sdk/p2p/peerconnection-channel.js index 63fac835..4b8e5714 100644 --- a/src/sdk/p2p/peerconnection-channel.js +++ b/src/sdk/p2p/peerconnection-channel.js @@ -61,11 +61,14 @@ const sysInfo = Utils.sysInfo(); class P2PPeerConnectionChannel extends EventDispatcher { // |signaling| is an object has a method |sendSignalingMessage|. /* eslint-disable-next-line require-jsdoc */ - constructor(config, localId, remoteId, signaling, isInitializer) { + constructor( + config, localId, remoteId, localSessionId, remoteSessionId, signaling) { super(); this._config = config; this._localId = localId; this._remoteId = remoteId; + this._localSessionId = localSessionId; + this._remoteSessionId = remoteSessionId; this._signaling = signaling; this._pc = null; this._publishedStreams = new Map(); // Key is streams published, value is its publication. @@ -227,11 +230,12 @@ class P2PPeerConnectionChannel extends EventDispatcher { _sendSdp(sdp) { return this._signaling.sendSignalingMessage( - this._remoteId, SignalingType.SDP, sdp); + this._remoteId, this._localSessionId, SignalingType.SDP, sdp); } _sendSignalingMessage(type, message) { - return this._signaling.sendSignalingMessage(this._remoteId, type, message); + return this._signaling.sendSignalingMessage( + this._remoteId, this._localSessionId, type, message); } _SignalingMesssageHandler(message) { diff --git a/test/unit/resources/scripts/p2p.js b/test/unit/resources/scripts/p2p.js index fe4aa8f7..f276ee84 100644 --- a/test/unit/resources/scripts/p2p.js +++ b/test/unit/resources/scripts/p2p.js @@ -66,7 +66,7 @@ describe('Unit tests for P2PClient', function() { done(); }); }); - describe('Interop with remote endpoints', function(){ + describe('Interop with remote endpoints (includes end to end tests)', function(){ const sourceInfo = new StreamModule.StreamSourceInfo('mic'); let signaling1, signaling2, signaling3; let p2pclient1, p2pclient2, p2pclient3; @@ -150,5 +150,12 @@ describe('Unit tests for P2PClient', function() { expect(p2pclient3.send('user1', 'message')).to.be.rejected.and.notify( done); }); + it('Signaling collisions should be resolved.', done => { + p2pclient1.send('user2', 'message'); + p2pclient2.send('user1', 'message'); + done(); + //expect(Promise.all([p2pclient1.send('user2', 'message'), p2pclient2.send('user1', 'message')])).to.be.fulfilled.and.notify(done); + // TODO: Check messages are received. + }); }); }); From b77a5a61d1fa76ffc12da98bca387799a3601b1b Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Mon, 22 Mar 2021 16:48:00 +0800 Subject: [PATCH 2/8] Add doc for perfect negotiation. --- docs/design/perfect_negotiation.md | 31 ++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 docs/design/perfect_negotiation.md diff --git a/docs/design/perfect_negotiation.md b/docs/design/perfect_negotiation.md new file mode 100644 index 00000000..84f1508b --- /dev/null +++ b/docs/design/perfect_negotiation.md @@ -0,0 +1,31 @@ +# Perfect Negotiation + +This document describes how perfect negotiation is implemented in OWT P2P SDK to avoid collision. + +**Perfect negotiation**, **polite peer**, **impolite peer** are defined in [Perfect negotiation example of WebRTC 1.0](https://w3c.github.io/webrtc-pc/#perfect-negotiation-example). + +## Determining polite peer and impolite peer + +OWT client SDKs determines polite peer and impolite peer by comparing client IDs. Sorting these two client IDs alphabetically in increasing order, the first one is polite order, the second one is impolite peer. + +Signaling server is required to return a client ID assigned for the client connected after authentication. OWT doesn't define how authentication works and how client IDs are assigned. They depend on application's design. But every client gets a unique client ID after connecting to the signaling server. + +## Connections + +We expect only one `PeerConnection` between two endpoints. A random ID is generated when creating a new `PeerConnection`. All messages (offers, answers, ICE candidates) for this `PeerConnection` have a `connectionId` property with the connection ID as its value. + +## Behaviors + +When WebRTC collision occurred, it basically follows the perfect negotiation example in WebRTC 1.0. This section only describes the implementation for signaling collision. + +A connection typically ends by calling `stop` at local side or receiving a `chat-closed` message from the remote side. Client SDKs stores the most recent connection IDs with all remote endpoints, and clean one of them when a connection ends. If the connection ID of a signaling message received is different from the one stored locally, collision happens. + +An example is both sides call `send` to create a data channel and send a message to the remote endpoint at the same time. Each side creates a new `PeerConnection`, and a connection ID. These two connection IDs are different. Then each of them will receive signaling messages with connection ID differ from the local one. + +### Polite peer + +The polite peer is the controlled side. When a signaling message with a new connection ID is received, it stops current PeerConnection， create a new one, and associate it with the remote connection ID. + +### Impolite peer + +The polite peer is the controlling side. It ignores remote messages conflict with its own state, and continue its process. \ No newline at end of file From 7b814c06f4afae1ae407d8547266d24c2cde5ed5 Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Mon, 22 Mar 2021 16:48:17 +0800 Subject: [PATCH 3/8] Store only one PeerConnection for each remote endpoint. --- src/sdk/p2p/p2pclient.js | 60 ++++++++++++++------------- src/sdk/p2p/peerconnection-channel.js | 12 ++---- 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/sdk/p2p/p2pclient.js b/src/sdk/p2p/p2pclient.js index d4b89c5d..a8ef9497 100644 --- a/src/sdk/p2p/p2pclient.js +++ b/src/sdk/p2p/p2pclient.js @@ -85,6 +85,7 @@ const P2PClient = function(configuration, signalingChannel) { const config = configuration; const signaling = signalingChannel; const channels = new Map(); // Map of PeerConnectionChannels. + const sessionIds = new Map(); // Key is remote user ID, value is current session ID. const self=this; let state = ConnectionState.READY; let myId; @@ -92,16 +93,20 @@ const P2PClient = function(configuration, signalingChannel) { signaling.onMessage = function(origin, message) { Logger.debug('Received signaling message from ' + origin + ': ' + message); const data = JSON.parse(message); - const remoteSessionId = data.sessionId; + const sessionId = data.sessionId; + if (sessionIds.has(origin) && sessionIds.get(origin) != sessionId && + isControlling(origin)) { + // Ignore. + } if (data.type === 'chat-closed') { if (channels.has(origin)) { - getOrCreateChannel(origin, remoteSessionId).onMessage(data); + getOrCreateChannel(origin, sessionId).onMessage(data); channels.delete(origin); } return; } if (self.allowedRemoteIds.indexOf(origin) >= 0) { - getOrCreateChannel(origin, remoteSessionId).onMessage(data); + getOrCreateChannel(origin, sessionId).onMessage(data); } else { sendSignalingMessage(origin, 'chat-closed', ErrorModule.errors.P2P_CLIENT_DENIED); @@ -251,12 +256,10 @@ const P2PClient = function(configuration, signalingChannel) { }; const sendSignalingMessage = function( - remoteId, localSessionId, type, message) { + remoteId, sessionId, type, message) { const msg = { type, - // Sender side session ID. Sender and receiver have different identity for - // the same session. - sessionId: localSessionId, + sessionId, }; if (message) { msg.data = message; @@ -268,27 +271,32 @@ const P2PClient = function(configuration, signalingChannel) { }); }; - // If `remoteSessionId` is undefined, a channel for `remoteId` will be - // returned. - const getOrCreateChannel = function(remoteId, remoteSessionId) { - if (!channels.has(remoteId) || - (!remoteSessionId && channels.get(remoteId).size() == 0) || - (remoteSessionId && !channels.get(remoteId).has(remoteSessionId))) { - if (!channels.has(remoteId)) { - channels.set(new Map()); - } + // Return true if current endpoint is an impolite peer, which controls the + // session. + const isControlling = function(remoteId) { + return myId > remoteId; + }; + + // If `session` is undefined, a channel for `remoteId` will be returned. + const getOrCreateChannel = function(remoteId, sessionId) { + if (!channels.has(remoteId)) { + channels.set(remoteId, new Map()); + } + if (!sessionId) { // We expect only a single PeerConnection between two endpoints. But we // still check duplication to avoid potential issues. - let localSessionId = Math.round(Math.random() * 8); - while (channels.get(remoteId).has(remoteSessionId)) { - localSessionId = Math.round(Math.random() * 8); + const sessionIdLimit = 100000; + sessionId = Math.round(Math.random() * sessionIdLimit); + while (channels.get(remoteId).has(sessionId)) { + sessionId = Math.round(Math.random() * sessionIdLimit); } + } + if (!channels.get(remoteId).has(sessionId)) { // Construct an signaling sender/receiver for P2PPeerConnection. const signalingForChannel = Object.create(EventDispatcher); signalingForChannel.sendSignalingMessage = sendSignalingMessage; const pcc = new P2PPeerConnectionChannel( - config, myId, remoteId, localSessionId, remoteSessionId, - signalingForChannel); + config, myId, remoteId, sessionId, signalingForChannel); pcc.addEventListener('streamadded', (streamEvent)=>{ self.dispatchEvent(streamEvent); }); @@ -296,18 +304,14 @@ const P2PClient = function(configuration, signalingChannel) { self.dispatchEvent(messageEvent); }); pcc.addEventListener('ended', () => { - channels.get(remoteId).delete(localSessionId); + channels.get(remoteId).delete(sessionId); if (channels.get(remoteId).size() == 0) { channels.delete(remoteId); } }); - channels.get(remoteId).get(localSessionId).set(pcc); - } - if (remoteSessionId) { - return channels.get(remoteId).get(remoteSessionId); - } else { - return channels.get(remoteId).values().next().value; + channels.get(remoteId).set(sessionId, pcc); } + return channels.get(remoteId).get(sessionId); }; }; diff --git a/src/sdk/p2p/peerconnection-channel.js b/src/sdk/p2p/peerconnection-channel.js index 4b8e5714..788f9cf2 100644 --- a/src/sdk/p2p/peerconnection-channel.js +++ b/src/sdk/p2p/peerconnection-channel.js @@ -62,13 +62,12 @@ class P2PPeerConnectionChannel extends EventDispatcher { // |signaling| is an object has a method |sendSignalingMessage|. /* eslint-disable-next-line require-jsdoc */ constructor( - config, localId, remoteId, localSessionId, remoteSessionId, signaling) { + config, localId, remoteId, sessionId, signaling) { super(); this._config = config; this._localId = localId; this._remoteId = remoteId; - this._localSessionId = localSessionId; - this._remoteSessionId = remoteSessionId; + this._sessionId = sessionId; this._signaling = signaling; this._pc = null; this._publishedStreams = new Map(); // Key is streams published, value is its publication. @@ -98,9 +97,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { this._infoSent = false; this._disposed = false; this._createPeerConnection(); - if (isInitializer) { - this._sendSignalingMessage(SignalingType.CLOSED); - } this._sendSignalingMessage(SignalingType.UA, sysInfo); } @@ -230,12 +226,12 @@ class P2PPeerConnectionChannel extends EventDispatcher { _sendSdp(sdp) { return this._signaling.sendSignalingMessage( - this._remoteId, this._localSessionId, SignalingType.SDP, sdp); + this._remoteId, this._sessionId, SignalingType.SDP, sdp); } _sendSignalingMessage(type, message) { return this._signaling.sendSignalingMessage( - this._remoteId, this._localSessionId, type, message); + this._remoteId, this._sessionId, type, message); } _SignalingMesssageHandler(message) { From f7d19bfa42c263dd54618e7fbcfad0387fa21f94 Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Tue, 23 Mar 2021 11:35:44 +0800 Subject: [PATCH 4/8] Implement polite peer and impolite peer for signaling. --- src/sdk/p2p/p2pclient.js | 84 ++++++++++++++------------- src/sdk/p2p/peerconnection-channel.js | 8 +-- 2 files changed, 49 insertions(+), 43 deletions(-) diff --git a/src/sdk/p2p/p2pclient.js b/src/sdk/p2p/p2pclient.js index a8ef9497..cf8b7648 100644 --- a/src/sdk/p2p/p2pclient.js +++ b/src/sdk/p2p/p2pclient.js @@ -85,32 +85,36 @@ const P2PClient = function(configuration, signalingChannel) { const config = configuration; const signaling = signalingChannel; const channels = new Map(); // Map of PeerConnectionChannels. - const sessionIds = new Map(); // Key is remote user ID, value is current session ID. - const self=this; + const connectionIds = new Map(); // Key is remote user ID, value is current session ID. + const self = this; let state = ConnectionState.READY; let myId; signaling.onMessage = function(origin, message) { Logger.debug('Received signaling message from ' + origin + ': ' + message); const data = JSON.parse(message); - const sessionId = data.sessionId; - if (sessionIds.has(origin) && sessionIds.get(origin) != sessionId && - isControlling(origin)) { - // Ignore. + const connectionId = data.connectionId; + if (self.allowedRemoteIds.indexOf(origin) < 0) { + sendSignalingMessage( + origin, data.connectionId, 'chat-closed', + ErrorModule.errors.P2P_CLIENT_DENIED); + return; + } + if (connectionIds.has(origin) && + connectionIds.get(origin) != connectionId && !isPolitePeer(origin)) { + Logger.warning( + // eslint-disable-next-line max-len + 'Collision detected, ignore this message because current endpoint is impolite peer.'); + return; } if (data.type === 'chat-closed') { if (channels.has(origin)) { - getOrCreateChannel(origin, sessionId).onMessage(data); + getOrCreateChannel(origin, connectionId).onMessage(data); channels.delete(origin); } return; } - if (self.allowedRemoteIds.indexOf(origin) >= 0) { - getOrCreateChannel(origin, sessionId).onMessage(data); - } else { - sendSignalingMessage(origin, 'chat-closed', - ErrorModule.errors.P2P_CLIENT_DENIED); - } + getOrCreateChannel(origin, connectionId).onMessage(data); }; signaling.onServerDisconnected = function() { @@ -164,10 +168,8 @@ const P2PClient = function(configuration, signalingChannel) { if (state == ConnectionState.READY) { return; } - channels.forEach((sessions) => { - sessions.forEach((channel) => { - channel.stop(); - }); + channels.forEach((channel) => { + channel.stop(); }); channels.clear(); signaling.disconnect(); @@ -256,10 +258,10 @@ const P2PClient = function(configuration, signalingChannel) { }; const sendSignalingMessage = function( - remoteId, sessionId, type, message) { + remoteId, connectionId, type, message) { const msg = { type, - sessionId, + connectionId, }; if (message) { msg.data = message; @@ -273,30 +275,34 @@ const P2PClient = function(configuration, signalingChannel) { // Return true if current endpoint is an impolite peer, which controls the // session. - const isControlling = function(remoteId) { - return myId > remoteId; + const isPolitePeer = function(remoteId) { + return myId < remoteId; }; - // If `session` is undefined, a channel for `remoteId` will be returned. - const getOrCreateChannel = function(remoteId, sessionId) { - if (!channels.has(remoteId)) { - channels.set(remoteId, new Map()); + // If a connection with remoteId with a different session ID exists, it will + // be stopped and a new connection will be created. + const getOrCreateChannel = function(remoteId, connectionId) { + // If `connectionId` is not defined, use the latest one or generate a new + // one. + if (!connectionId && connectionIds.has(remoteId)) { + connectionId = connectionIds.get(remoteId); } - if (!sessionId) { - // We expect only a single PeerConnection between two endpoints. But we - // still check duplication to avoid potential issues. - const sessionIdLimit = 100000; - sessionId = Math.round(Math.random() * sessionIdLimit); - while (channels.get(remoteId).has(sessionId)) { - sessionId = Math.round(Math.random() * sessionIdLimit); - } + // Delete old channel if connection doesn't match. + if (connectionIds.has(remoteId) && + connectionIds.get(remoteId) != connectionId) { + self.stop(remoteId); + } + if (!connectionId) { + const connectionIdLimit = 100000; + connectionId = Math.round(Math.random() * connectionIdLimit); + connectionIds.set(remoteId, connectionId); } - if (!channels.get(remoteId).has(sessionId)) { + if (!channels.has(remoteId)) { // Construct an signaling sender/receiver for P2PPeerConnection. const signalingForChannel = Object.create(EventDispatcher); signalingForChannel.sendSignalingMessage = sendSignalingMessage; const pcc = new P2PPeerConnectionChannel( - config, myId, remoteId, sessionId, signalingForChannel); + config, myId, remoteId, connectionId, signalingForChannel); pcc.addEventListener('streamadded', (streamEvent)=>{ self.dispatchEvent(streamEvent); }); @@ -304,14 +310,14 @@ const P2PClient = function(configuration, signalingChannel) { self.dispatchEvent(messageEvent); }); pcc.addEventListener('ended', () => { - channels.get(remoteId).delete(sessionId); - if (channels.get(remoteId).size() == 0) { + if (channels.has(remoteId)) { channels.delete(remoteId); } + connectionIds.delete(remoteId); }); - channels.get(remoteId).set(sessionId, pcc); + channels.set(remoteId, pcc); } - return channels.get(remoteId).get(sessionId); + return channels.get(remoteId); }; }; diff --git a/src/sdk/p2p/peerconnection-channel.js b/src/sdk/p2p/peerconnection-channel.js index 788f9cf2..3072c94f 100644 --- a/src/sdk/p2p/peerconnection-channel.js +++ b/src/sdk/p2p/peerconnection-channel.js @@ -62,12 +62,12 @@ class P2PPeerConnectionChannel extends EventDispatcher { // |signaling| is an object has a method |sendSignalingMessage|. /* eslint-disable-next-line require-jsdoc */ constructor( - config, localId, remoteId, sessionId, signaling) { + config, localId, remoteId, connectionId, signaling) { super(); this._config = config; this._localId = localId; this._remoteId = remoteId; - this._sessionId = sessionId; + this._connectionId = connectionId; this._signaling = signaling; this._pc = null; this._publishedStreams = new Map(); // Key is streams published, value is its publication. @@ -226,12 +226,12 @@ class P2PPeerConnectionChannel extends EventDispatcher { _sendSdp(sdp) { return this._signaling.sendSignalingMessage( - this._remoteId, this._sessionId, SignalingType.SDP, sdp); + this._remoteId, this._connectionId, SignalingType.SDP, sdp); } _sendSignalingMessage(type, message) { return this._signaling.sendSignalingMessage( - this._remoteId, this._sessionId, type, message); + this._remoteId, this._connectionId, type, message); } _SignalingMesssageHandler(message) { From 9f4b683b340a98873d25fec0024613dca4b74c1e Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Wed, 24 Mar 2021 18:00:15 +0800 Subject: [PATCH 5/8] Implement perfect negotiation for WebRTC collision. --- src/sdk/p2p/p2pclient.js | 4 ++-- src/sdk/p2p/peerconnection-channel.js | 11 +++++++++- .../resources/scripts/fake-p2p-signaling.js | 1 - test/unit/resources/scripts/p2p.js | 22 +++++++++++++++++++ 4 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/sdk/p2p/p2pclient.js b/src/sdk/p2p/p2pclient.js index cf8b7648..b86fa09d 100644 --- a/src/sdk/p2p/p2pclient.js +++ b/src/sdk/p2p/p2pclient.js @@ -101,7 +101,7 @@ const P2PClient = function(configuration, signalingChannel) { return; } if (connectionIds.has(origin) && - connectionIds.get(origin) != connectionId && !isPolitePeer(origin)) { + connectionIds.get(origin) !== connectionId && !isPolitePeer(origin)) { Logger.warning( // eslint-disable-next-line max-len 'Collision detected, ignore this message because current endpoint is impolite peer.'); @@ -295,8 +295,8 @@ const P2PClient = function(configuration, signalingChannel) { if (!connectionId) { const connectionIdLimit = 100000; connectionId = Math.round(Math.random() * connectionIdLimit); - connectionIds.set(remoteId, connectionId); } + connectionIds.set(remoteId, connectionId); if (!channels.has(remoteId)) { // Construct an signaling sender/receiver for P2PPeerConnection. const signalingForChannel = Object.create(EventDispatcher); diff --git a/src/sdk/p2p/peerconnection-channel.js b/src/sdk/p2p/peerconnection-channel.js index 3072c94f..da7ae183 100644 --- a/src/sdk/p2p/peerconnection-channel.js +++ b/src/sdk/p2p/peerconnection-channel.js @@ -93,7 +93,7 @@ class P2PPeerConnectionChannel extends EventDispatcher { this._dataSeq = 1; // Sequence number for data channel messages. this._sendDataPromises = new Map(); // Key is data sequence number, value is an object has |resolve| and |reject|. this._addedTrackIds = []; // Tracks that have been added after receiving remote SDP but before connection is established. Draining these messages when ICE connection state is connected. - this._isCaller = true; + this._isPolitePeer = localId < remoteId; this._infoSent = false; this._disposed = false; this._createPeerConnection(); @@ -416,6 +416,15 @@ class P2PPeerConnectionChannel extends EventDispatcher { _onOffer(sdp) { Logger.debug('About to set remote description. Signaling state: ' + this._pc.signalingState); + if (this._pc.signalingState !== 'stable') { + if (this._isPolitePeer) { + // Rollback. + this._pc.setLocalDescription(); + } else { + // Ignore this offer. + return; + } + } sdp.sdp = this._setRtpSenderOptions(sdp.sdp, this._config); // Firefox only has one codec in answer, which does not truly reflect its // decoding capability. So we set codec preference to remote offer, and let diff --git a/test/unit/resources/scripts/fake-p2p-signaling.js b/test/unit/resources/scripts/fake-p2p-signaling.js index 9a1711d9..38140dc1 100644 --- a/test/unit/resources/scripts/fake-p2p-signaling.js +++ b/test/unit/resources/scripts/fake-p2p-signaling.js @@ -39,7 +39,6 @@ export default class FakeP2PSignalingChannel { } send(targetId, message) { - Logger.debug(this.userId + ' -> ' + targetId + ': ' + message); return new Promise((resolve, reject) => { messageQueue.push({ target: targetId, message, resolve, reject, sender: this.userId }); setTimeout(() => { diff --git a/test/unit/resources/scripts/p2p.js b/test/unit/resources/scripts/p2p.js index f276ee84..aa48b5bc 100644 --- a/test/unit/resources/scripts/p2p.js +++ b/test/unit/resources/scripts/p2p.js @@ -157,5 +157,27 @@ describe('Unit tests for P2PClient', function() { //expect(Promise.all([p2pclient1.send('user2', 'message'), p2pclient2.send('user1', 'message')])).to.be.fulfilled.and.notify(done); // TODO: Check messages are received. }); + it('WebRTC collision should be resolved.', async () => { + const c1Spy = new sinon.spy(); + const c2Spy = new sinon.spy(); + p2pclient1.addEventListener('messagereceived',c1Spy); + p2pclient2.addEventListener('messagereceived',c2Spy); + await p2pclient1.publish('user2', localStream); + // Both sides create PeerConnection. It cannot 100% sure to trigger WebRTC + // collision. But it has a high chance that `setRemoteDescription` get + // failed. However, even `setRemoteDescription` is failed, the SDK stops + // `PeerConnection` silently without firing an event. So this test case + // cannot detect failures like this. + await Promise.all([ + p2pclient1.send('user2', 'message'), p2pclient2.send('user1', 'message') + ]); + await new Promise(resolve => { + setTimeout(() => { + expect(c1Spy.callCount).to.equal(1); + expect(c2Spy.callCount).to.equal(1); + resolve(); + }, 100); + }); + }); }); }); From 12d33ddf4cc0d22459986b78b849c560c4ed22f9 Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Tue, 6 Apr 2021 16:00:11 +0800 Subject: [PATCH 6/8] Code cleanup. --- src/sdk/p2p/peerconnection-channel.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/sdk/p2p/peerconnection-channel.js b/src/sdk/p2p/peerconnection-channel.js index da7ae183..0a1d4be7 100644 --- a/src/sdk/p2p/peerconnection-channel.js +++ b/src/sdk/p2p/peerconnection-channel.js @@ -54,7 +54,10 @@ const sysInfo = Utils.sysInfo(); /** * @class P2PPeerConnectionChannel - * @desc A P2PPeerConnectionChannel handles all interactions between this endpoint and a remote endpoint. + * @desc A P2PPeerConnectionChannel manages a PeerConnection object, handles all + * interactions between this endpoint (local) and a remote endpoint. Only one + * PeerConnectionChannel is alive for a local - remote endpoint pair at any + * given time. * @memberOf Owt.P2P * @private */ @@ -65,7 +68,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { config, localId, remoteId, connectionId, signaling) { super(); this._config = config; - this._localId = localId; this._remoteId = remoteId; this._connectionId = connectionId; this._signaling = signaling; @@ -94,7 +96,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { this._sendDataPromises = new Map(); // Key is data sequence number, value is an object has |resolve| and |reject|. this._addedTrackIds = []; // Tracks that have been added after receiving remote SDP but before connection is established. Draining these messages when ICE connection state is connected. this._isPolitePeer = localId < remoteId; - this._infoSent = false; this._disposed = false; this._createPeerConnection(); this._sendSignalingMessage(SignalingType.UA, sysInfo); From ba31c147c26f0f50b1a4565bc4ef30bddf9257d5 Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Wed, 7 Apr 2021 14:12:03 +0800 Subject: [PATCH 7/8] Listen to negotiation needed event. Negotiation needed works good now, the SDK doesn't need to handle negotiation needed flag manually. --- src/sdk/p2p/peerconnection-channel.js | 41 +++------------------------ 1 file changed, 4 insertions(+), 37 deletions(-) diff --git a/src/sdk/p2p/peerconnection-channel.js b/src/sdk/p2p/peerconnection-channel.js index 0a1d4be7..4b88d7b4 100644 --- a/src/sdk/p2p/peerconnection-channel.js +++ b/src/sdk/p2p/peerconnection-channel.js @@ -126,7 +126,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { for (const track of stream.mediaStream.getTracks()) { this._pc.addTrack(track, stream.mediaStream); } - this._onNegotiationneeded(); this._publishingStreams.push(stream); const trackIds = Array.from(stream.mediaStream.getTracks(), (track) => track.id); @@ -420,6 +419,7 @@ class P2PPeerConnectionChannel extends EventDispatcher { if (this._pc.signalingState !== 'stable') { if (this._isPolitePeer) { // Rollback. + Logger.debug('Rollback.'); this._pc.setLocalDescription(); } else { // Ignore this offer. @@ -549,12 +549,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { } _onNegotiationneeded() { - // This is intented to be executed when onnegotiationneeded event is fired. - // However, onnegotiationneeded may fire mutiple times when more than one - // track is added/removed. So we manually execute this function after - // adding/removing track and creating data channel. - Logger.debug('On negotiation needed.'); - if (this._pc.signalingState === 'stable') { this._doNegotiate(); } else { @@ -699,43 +693,23 @@ class P2PPeerConnectionChannel extends EventDispatcher { this._pc.oniceconnectionstatechange = (event) => { this._onIceConnectionStateChange.apply(this, [event]); }; - /* - this._pc.oniceChannelStatechange = function(event) { - _onIceChannelStateChange(peer, event); - }; - = function() { - onNegotiationneeded(peers[peer.id]); + this._pc.onnegotiationneeded = () => { + this._onNegotiationneeded(); }; - - //DataChannel - this._pc.ondatachannel = function(event) { - Logger.debug(myId + ': On data channel'); - // Save remote created data channel. - if (!peer.dataChannels[event.channel.label]) { - peer.dataChannels[event.channel.label] = event.channel; - Logger.debug('Save remote created data channel.'); - } - bindEventsToDataChannel(event.channel, peer); - };*/ } _drainPendingStreams() { - let negotiationNeeded = false; Logger.debug('Draining pending streams.'); if (this._pc && this._pc.signalingState === 'stable') { Logger.debug('Peer connection is ready for draining pending streams.'); for (let i = 0; i < this._pendingStreams.length; i++) { const stream = this._pendingStreams[i]; - // OnNegotiationNeeded event will be triggered immediately after adding stream to PeerConnection in Firefox. - // And OnNegotiationNeeded handler will execute drainPendingStreams. To avoid add the same stream multiple times, - // shift it from pending stream list before adding it to PeerConnection. this._pendingStreams.shift(); if (!stream.mediaStream) { continue; } for (const track of stream.mediaStream.getTracks()) { this._pc.addTrack(track, stream.mediaStream); - negotiationNeeded = true; } Logger.debug('Added stream to peer connection.'); this._publishingStreams.push(stream); @@ -746,7 +720,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { continue; } this._pc.removeStream(this._pendingUnpublishStreams[j].mediaStream); - negotiationNeeded = true; this._unpublishPromises.get( this._pendingUnpublishStreams[j].mediaStream.id).resolve(); this._publishedStreams.delete(this._pendingUnpublishStreams[j]); @@ -754,9 +727,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { } this._pendingUnpublishStreams.length = 0; } - if (negotiationNeeded) { - this._onNegotiationneeded(); - } } _drainPendingRemoteIceCandidates() { @@ -877,7 +847,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { return; } this._isNegotiationNeeded = false; - this._isCaller = true; let localDesc; this._pc.createOffer().then((desc) => { desc.sdp = this._setRtpReceiverOptions(desc.sdp); @@ -888,7 +857,7 @@ class P2PPeerConnectionChannel extends EventDispatcher { }); } }).catch((e) => { - Logger.error(e.message + ' Please check your codec settings.'); + Logger.error(e.message); const error = new ErrorModule.P2PError(ErrorModule.errors.P2P_WEBRTC_SDP, e.message); this._stop(error, true); @@ -898,7 +867,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { _createAndSendAnswer() { this._drainPendingStreams(); this._isNegotiationNeeded = false; - this._isCaller = false; let localDesc; this._pc.createAnswer().then((desc) => { desc.sdp = this._setRtpReceiverOptions(desc.sdp); @@ -973,7 +941,6 @@ class P2PPeerConnectionChannel extends EventDispatcher { const dc = this._pc.createDataChannel(label); this._bindEventsToDataChannel(dc); this._dataChannels.set(DataChannelLabel.MESSAGE, dc); - this._onNegotiationneeded(); } _bindEventsToDataChannel(dc) { From ffe50ee2ff8257ad77b3fa11b3835cf142b5ee8c Mon Sep 17 00:00:00 2001 From: Jianjun Zhu Date: Fri, 9 Apr 2021 10:56:28 +0800 Subject: [PATCH 8/8] Address comments. --- docs/design/perfect_negotiation.md | 10 +++++----- src/sdk/p2p/peerconnection-channel.js | 3 +-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/design/perfect_negotiation.md b/docs/design/perfect_negotiation.md index 84f1508b..8c1e9dc7 100644 --- a/docs/design/perfect_negotiation.md +++ b/docs/design/perfect_negotiation.md @@ -8,15 +8,15 @@ This document describes how perfect negotiation is implemented in OWT P2P SDK to OWT client SDKs determines polite peer and impolite peer by comparing client IDs. Sorting these two client IDs alphabetically in increasing order, the first one is polite order, the second one is impolite peer. -Signaling server is required to return a client ID assigned for the client connected after authentication. OWT doesn't define how authentication works and how client IDs are assigned. They depend on application's design. But every client gets a unique client ID after connecting to the signaling server. +Signaling server is required to return a client ID assigned to the client connected after authentication. OWT doesn't define how authentication works and how client IDs are assigned. They depend on application's design. But every client gets a unique client ID after connecting to the signaling server. ## Connections -We expect only one `PeerConnection` between two endpoints. A random ID is generated when creating a new `PeerConnection`. All messages (offers, answers, ICE candidates) for this `PeerConnection` have a `connectionId` property with the connection ID as its value. +We expect only one `PeerConnection` between two endpoints. A random ID is generated when creating a new `PeerConnection`. All messages (offers, answers, ICE candidates) for this `PeerConnection` have a `connectionId` property with the connection ID as its value. The connection ID is shared by both clients. -## Behaviors +## Collision -When WebRTC collision occurred, it basically follows the perfect negotiation example in WebRTC 1.0. This section only describes the implementation for signaling collision. +When WebRTC collision occurs, it basically follows the perfect negotiation example in WebRTC 1.0. This section only describes the implementation for signaling collision. A connection typically ends by calling `stop` at local side or receiving a `chat-closed` message from the remote side. Client SDKs stores the most recent connection IDs with all remote endpoints, and clean one of them when a connection ends. If the connection ID of a signaling message received is different from the one stored locally, collision happens. @@ -28,4 +28,4 @@ The polite peer is the controlled side. When a signaling message with a new conn ### Impolite peer -The polite peer is the controlling side. It ignores remote messages conflict with its own state, and continue its process. \ No newline at end of file +The polite peer is the controlling side. It ignores remote messages conflicting with its own state, and continues its own process. \ No newline at end of file diff --git a/src/sdk/p2p/peerconnection-channel.js b/src/sdk/p2p/peerconnection-channel.js index 4b88d7b4..28f9deb2 100644 --- a/src/sdk/p2p/peerconnection-channel.js +++ b/src/sdk/p2p/peerconnection-channel.js @@ -418,11 +418,10 @@ class P2PPeerConnectionChannel extends EventDispatcher { this._pc.signalingState); if (this._pc.signalingState !== 'stable') { if (this._isPolitePeer) { - // Rollback. Logger.debug('Rollback.'); this._pc.setLocalDescription(); } else { - // Ignore this offer. + Logger.debug('Collision detected. Ignore this offer.'); return; } }