From d00d07a1c17cbfbef39d8b04594d42c826bd6552 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 21 Oct 2020 18:23:01 +0100 Subject: [PATCH] Improve ICE candidate batching Hopefully send fewer ICE candidate events by obeying the batching guidelines in MSC2476. --- src/webrtc/call.ts | 100 +++++++++++++++++++++++---------- src/webrtc/callEventHandler.ts | 2 + 2 files changed, 72 insertions(+), 30 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 9f75c3b08..3ea746b92 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -225,7 +225,7 @@ export class MatrixCall extends EventEmitter { private screenSharingStream: MediaStream; private remoteStream: MediaStream; private localAVStream: MediaStream; - private answerContent: object; + private inviteOrAnswerSent: boolean; private waitForLocalAVStream: boolean; // XXX: This is either the invite or answer from remote... private msg: any; @@ -272,6 +272,7 @@ export class MatrixCall extends EventEmitter { this.mediaPromises = Object.create(null); this.sentEndOfCandidates = false; + this.inviteOrAnswerSent = false; } /** @@ -490,20 +491,21 @@ export class MatrixCall extends EventEmitter { * Answer a call. */ async answer() { - logger.debug(`Answering call ${this.callId} of type ${this.type}`); - - if (this.answerContent) { - this.sendAnswer(); + if (this.inviteOrAnswerSent) { return; } + logger.debug(`Answering call ${this.callId} of type ${this.type}`); + if (!this.localAVStream && !this.waitForLocalAVStream) { const constraints = getUserMediaVideoContraints(this.type); logger.log("Getting user media with constraints", constraints); this.setState(CallState.WaitLocalMedia); + this.waitForLocalAVStream = true; try { const mediaStream = await navigator.mediaDevices.getUserMedia(constraints); + this.waitForLocalAVStream = false; this.gotUserMediaForAnswer(mediaStream); } catch (e) { this.getUserMediaFailed(e); @@ -692,10 +694,24 @@ export class MatrixCall extends EventEmitter { }; private sendAnswer() { - this.setState(CallState.Connecting); - this.sendVoipEvent(EventType.CallAnswer, this.answerContent).then(() => { + const answerContent = { + answer: { + sdp: this.peerConn.localDescription.sdp, + // type is now deprecated as of Matrix VoIP v1, but + // required to still be sent for backwards compat + type: this.peerConn.localDescription.type, + }, + }; + // We have just taken the local description from the peerconnection which will + // contain all the local candidates added so far, so we can discard any candidates + // we had queued up because they'll be in the answer. + logger.info(`Discarding ${this.candidateSendQueue.length} candidates that will be sent in answer`); + this.candidateSendQueue = []; + + this.sendVoipEvent(EventType.CallAnswer, answerContent).then(() => { // If this isn't the first time we've tried to send the answer, // we may have candidates queued up, so send them now. + this.inviteOrAnswerSent = true; this.sendCandidateQueue(); }).catch((error) => { // We've failed to answer: back to the ringing state @@ -749,15 +765,13 @@ export class MatrixCall extends EventEmitter { try { await this.peerConn.setLocalDescription(myAnswer); + this.setState(CallState.Connecting); + + // Allow a short time for initial candidates to be gathered + await new Promise(resolve => { + setTimeout(resolve, 200); + }); - this.answerContent = { - answer: { - sdp: this.peerConn.localDescription.sdp, - // type is now deprecated as of Matrix VoIP v1, but - // required to still be sent for backwards compat - type: this.peerConn.localDescription.type, - }, - }; this.sendAnswer(); } catch (err) { logger.debug("Error setting local description!", err); @@ -782,7 +796,7 @@ export class MatrixCall extends EventEmitter { // As with the offer, note we need to make a copy of this object, not // pass the original: that broke in Chrome ~m43. if (event.candidate.candidate !== '' || !this.sentEndOfCandidates) { - this.sendCandidate(event.candidate); + this.queueCandidate(event.candidate); if (event.candidate.candidate === '') this.sentEndOfCandidates = true; } @@ -802,7 +816,7 @@ export class MatrixCall extends EventEmitter { const c = { candidate: '', } as RTCIceCandidate; - this.sendCandidate(c); + this.queueCandidate(c); this.sentEndOfCandidates = true; } }; @@ -860,7 +874,19 @@ export class MatrixCall extends EventEmitter { this.opponentVersion = event.getContent().version; this.opponentPartyId = event.getContent().party_id || null; + this.setState(CallState.Connecting); + + try { + await this.peerConn.setRemoteDescription(event.getContent().answer); + } catch (e) { + logger.debug("Failed to set remote description", e); + this.terminate(CallParty.Local, CallErrorCode.SetRemoteDescription, false); + return; + } + // If the answer we selected has a party_id, send a select_answer event + // We do this after setting the remote description since otherwise we'd block + // call setup on it if (this.opponentPartyId !== null) { try { await this.sendVoipEvent(EventType.CallSelectAnswer, { @@ -872,16 +898,6 @@ export class MatrixCall extends EventEmitter { logger.warn("Failed to send select_answer event", err); } } - - try { - await this.peerConn.setRemoteDescription(event.getContent().answer); - } catch (e) { - logger.debug("Failed to set remote description", e); - this.terminate(CallParty.Local, CallErrorCode.SetRemoteDescription, false); - return; - } - - this.setState(CallState.Connecting); } async onSelectAnswerReceived(event: MatrixEvent) { @@ -921,6 +937,11 @@ export class MatrixCall extends EventEmitter { return } + // Allow a short time for initial candidates to be gathered + await new Promise(resolve => { + setTimeout(resolve, 200); + }); + const content = { // OpenWebRTC appears to add extra stuff (like the DTLS fingerprint) // to the description when setting it on the peerconnection. @@ -939,8 +960,16 @@ export class MatrixCall extends EventEmitter { }, lifetime: CALL_TIMEOUT_MS, }; + + // Get rid of any candidates waiting to be sent: they'll be included in the local + // description we just got and will send in the offer. + logger.info(`Discarding ${this.candidateSendQueue.length} candidates that will be sent in offer`); + this.candidateSendQueue = []; + try { await this.sendVoipEvent(EventType.CallInvite, content); + this.sendCandidateQueue(); + this.inviteOrAnswerSent = true; this.setState(CallState.InviteSent); this.inviteTimeout = setTimeout(() => { this.inviteTimeout = null; @@ -1146,7 +1175,7 @@ export class MatrixCall extends EventEmitter { })); } - sendCandidate(content: RTCIceCandidate) { + queueCandidate(content: RTCIceCandidate) { // Sends candidates with are sent in a special way because we try to amalgamate // them into one message this.candidateSendQueue.push(content); @@ -1155,12 +1184,18 @@ export class MatrixCall extends EventEmitter { // means we tried to pick (ie. started generating candidates) and then failed to // send the answer and went back to the ringing state. Queue up the candidates // to send if we sucessfully send the answer. - if (this.state === CallState.Ringing) return; + // Equally don't send if we haven't yet sent the answer because we can send the + // first batch of candidates along with the answer + if (this.state === CallState.Ringing || !this.inviteOrAnswerSent) return; + + // MSC2746 reccomends these values (can be quite long when calling because the + // callee will need a while to answer the call) + const delay = this.direction === CallDirection.Inbound ? 500 : 2000; if (this.candidateSendTries === 0) { setTimeout(() => { this.sendCandidateQueue(); - }, 100); + }, delay); } } @@ -1286,6 +1321,11 @@ export class MatrixCall extends EventEmitter { this.setState(CallState.WaitLocalMedia); this.direction = CallDirection.Outbound; this.config = constraints; + // It would be really nice if we could start gathering candidates at this point + // so the ICE agent could be gathering while we open our media devices: we already + // know the type of the call and therefore what tracks we want to send. + // Perhaps we could do this by making fake tracks now and then using replaceTrack() + // once we have the actual tracks? (Can we make fake tracks?) try { const mediaStream = await navigator.mediaDevices.getUserMedia(constraints); this.gotUserMediaForInvite(mediaStream); diff --git a/src/webrtc/callEventHandler.ts b/src/webrtc/callEventHandler.ts index 41befddb8..af93ca310 100644 --- a/src/webrtc/callEventHandler.ts +++ b/src/webrtc/callEventHandler.ts @@ -252,6 +252,8 @@ export class CallEventHandler { } } } else if (event.getType() === EventType.CallSelectAnswer) { + if (!call) return; + if (event.getContent().party_id === call.ourPartyId) { // Ignore remote echo return;