From 03737546fef23fda96384e893056f99f434adeff Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 4 Dec 2020 20:03:11 +0000 Subject: [PATCH] Remove the media operation queues As per the comment on playRemoteVideo() --- src/webrtc/call.ts | 188 +++++++++++++++++++++------------------------ 1 file changed, 87 insertions(+), 101 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 4e378096f..89be9ef62 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -101,12 +101,6 @@ export enum CallEvent { HoldUnhold = 'hold_unhold', } -enum MediaQueueId { - RemoteVideo = 'remote_video', - RemoteAudio = 'remote_audio', - LocalVideo = 'local_video', -} - export enum CallErrorCode { /** The user chose to end the call */ UserHangup = 'user_hangup', @@ -226,7 +220,6 @@ export class MatrixCall extends EventEmitter { private turnServers: Array; private candidateSendQueue: Array; private candidateSendTries: number; - private mediaPromises: { [queueId: string]: Promise; }; private sentEndOfCandidates: boolean; private peerConn: RTCPeerConnection; private localVideoElement: HTMLVideoElement; @@ -291,12 +284,6 @@ export class MatrixCall extends EventEmitter { this.candidateSendQueue = []; this.candidateSendTries = 0; - // Lookup from opaque queue ID to a promise for media element operations that - // need to be serialised into a given queue. Store this per-MatrixCall on the - // assumption that multiple matrix calls will never compete for control of the - // same DOM elements. - this.mediaPromises = Object.create(null); - this.sentEndOfCandidates = false; this.inviteOrAnswerSent = false; this.makingOffer = false; @@ -371,14 +358,6 @@ export class MatrixCall extends EventEmitter { return this.opponentMember; } - private queueMediaOperation(queueId: MediaQueueId, operation: () => any) { - if (this.mediaPromises[queueId] !== undefined) { - this.mediaPromises[queueId] = this.mediaPromises[queueId].then(operation, operation); - } else { - this.mediaPromises[queueId] = Promise.resolve(operation()); - } - } - /** * Retrieve the local <video> DOM element. * @return {Element} The dom element @@ -410,17 +389,19 @@ export class MatrixCall extends EventEmitter { * video will be rendered to it immediately. * @param {Element} element The <video> DOM element. */ - setLocalVideoElement(element : HTMLVideoElement) { + async setLocalVideoElement(element : HTMLVideoElement) { this.localVideoElement = element; if (element && this.localAVStream && this.type === CallType.Video) { element.autoplay = true; - this.queueMediaOperation(MediaQueueId.LocalVideo, () => { - element.srcObject = this.localAVStream; - element.muted = true; - return element.play(); - }); + element.srcObject = this.localAVStream; + element.muted = true; + try { + await element.play(); + } catch (e) { + logger.info("Failed to play local video element", e); + } } } @@ -441,10 +422,7 @@ export class MatrixCall extends EventEmitter { this.remoteVideoElement = element; if (this.remoteStream) { - this.queueMediaOperation(MediaQueueId.RemoteVideo, () => { - element.srcObject = this.remoteStream; - return element.play(); - }); + this.playRemoteVideo(); } } @@ -748,19 +726,21 @@ export class MatrixCall extends EventEmitter { const videoEl = this.getLocalVideoElement(); if (videoEl && this.type === CallType.Video) { - this.queueMediaOperation(MediaQueueId.LocalVideo, () => { - videoEl.autoplay = true; - if (this.screenSharingStream) { - logger.debug( - "Setting screen sharing stream to the local video element", - ); - videoEl.srcObject = this.screenSharingStream; - } else { - videoEl.srcObject = stream; - } - videoEl.muted = true; - return videoEl.play(); - }); + videoEl.autoplay = true; + if (this.screenSharingStream) { + logger.debug( + "Setting screen sharing stream to the local video element", + ); + videoEl.srcObject = this.screenSharingStream; + } else { + videoEl.srcObject = stream; + } + videoEl.muted = true; + try { + await videoEl.play(); + } catch (e) { + logger.info("Failed to play local video element", e); + } } this.localAVStream = stream; @@ -825,13 +805,15 @@ export class MatrixCall extends EventEmitter { const localVidEl = this.getLocalVideoElement(); if (localVidEl && this.type === CallType.Video) { - this.queueMediaOperation(MediaQueueId.LocalVideo, () => { - localVidEl.autoplay = true; - localVidEl.srcObject = stream; + localVidEl.autoplay = true; + localVidEl.srcObject = stream; - localVidEl.muted = true; - return localVidEl.play(); - }); + localVidEl.muted = true; + try { + await localVidEl.play(); + } catch (e) { + logger.info("Failed to play local video element", e); + } } this.localAVStream = stream; @@ -1231,14 +1213,7 @@ export class MatrixCall extends EventEmitter { if (ev.track.kind === 'video') { if (this.remoteVideoElement) { - this.queueMediaOperation(MediaQueueId.RemoteVideo, async () => { - this.remoteVideoElement.srcObject = this.remoteStream; - try { - await this.remoteVideoElement.play(); - } catch (e) { - logger.error("Failed to play remote video element", e); - } - }); + this.playRemoteVideo(); } } else { if (this.remoteAudioElement) this.playRemoteAudio(); @@ -1265,33 +1240,31 @@ export class MatrixCall extends EventEmitter { } }; - playRemoteAudio() { - this.queueMediaOperation(MediaQueueId.RemoteAudio, async () => { - if (this.remoteVideoElement) this.remoteVideoElement.muted = true; - this.remoteAudioElement.muted = false; + async playRemoteAudio() { + if (this.remoteVideoElement) this.remoteVideoElement.muted = true; + this.remoteAudioElement.muted = false; - this.remoteAudioElement.srcObject = this.remoteStream; + this.remoteAudioElement.srcObject = this.remoteStream; - // if audioOutput is non-default: - try { - if (audioOutput) { - // This seems quite unreliable in Chrome, although I haven't yet managed to make a jsfiddle where - // it fails. - // It seems reliable if you set the sink ID after setting the srcObject and then set the sink ID - // back to the default after the call is over - logger.info("Setting audio sink to " + audioOutput + ", was " + this.remoteAudioElement.sinkId); - await this.remoteAudioElement.setSinkId(audioOutput); - } - } catch (e) { - logger.warn("Couldn't set requested audio output device: using default", e); + // if audioOutput is non-default: + try { + if (audioOutput) { + // This seems quite unreliable in Chrome, although I haven't yet managed to make a jsfiddle where + // it fails. + // It seems reliable if you set the sink ID after setting the srcObject and then set the sink ID + // back to the default after the call is over + logger.info("Setting audio sink to " + audioOutput + ", was " + this.remoteAudioElement.sinkId); + await this.remoteAudioElement.setSinkId(audioOutput); } + } catch (e) { + logger.warn("Couldn't set requested audio output device: using default", e); + } - try { - await this.remoteAudioElement.play(); - } catch (e) { - logger.error("Failed to play remote audio element", e); - } - }); + try { + await this.remoteAudioElement.play(); + } catch (e) { + logger.error("Failed to play remote audio element", e); + } } onHangupReceived = (msg) => { @@ -1370,7 +1343,7 @@ export class MatrixCall extends EventEmitter { } } - private terminate(hangupParty: CallParty, hangupReason: CallErrorCode, shouldEmit: boolean) { + private async terminate(hangupParty: CallParty, hangupReason: CallErrorCode, shouldEmit: boolean) { if (this.callHasEnded()) return; if (this.inviteTimeout) { @@ -1383,29 +1356,23 @@ export class MatrixCall extends EventEmitter { const localVid = this.getLocalVideoElement(); if (remoteVid) { - this.queueMediaOperation(MediaQueueId.RemoteVideo, () => { - remoteVid.pause(); - remoteVid.srcObject = null; - }); + remoteVid.pause(); + remoteVid.srcObject = null; } if (remoteAud) { - this.queueMediaOperation(MediaQueueId.RemoteAudio, async () => { - remoteAud.pause(); - remoteAud.srcObject = null; - try { - // As per comment in playRemoteAudio, setting the sink ID back to the default - // once the call is over makes setSinkId work reliably. - await this.remoteAudioElement.setSinkId('') - } catch (e) { - logger.warn("Failed to set sink ID back to default"); - } - }); + remoteAud.pause(); + remoteAud.srcObject = null; + try { + // As per comment in playRemoteAudio, setting the sink ID back to the default + // once the call is over makes setSinkId work reliably. + await this.remoteAudioElement.setSinkId('') + } catch (e) { + logger.warn("Failed to set sink ID back to default"); + } } if (localVid) { - this.queueMediaOperation(MediaQueueId.LocalVideo, () => { - localVid.pause(); - localVid.srcObject = null; - }); + localVid.pause(); + localVid.srcObject = null; } this.hangupParty = hangupParty; this.hangupReason = hangupReason; @@ -1528,6 +1495,25 @@ export class MatrixCall extends EventEmitter { const msgPartyId = msg.party_id || null; return msgPartyId === this.opponentPartyId; } + + private async playRemoteVideo() { + // A note on calling methods on media elements: + // We used to have queues per media element to serialise all calls on those elements. + // The reason given for this was that load() and play() were racing. However, we now + // never call load() explicitly so this seems unnecessary. However, serialising every + // operation was causing bugs where video would not resume because some play command + // had got stuck and all media operations were queued up behind it. If necessary, we + // should serialise the ones that need to be serialised but then be able to interrupt + // them with another load() which will cancel the pending one, but since we don't call + // load() explicitly, it shouldn't be a problem. + this.remoteVideoElement.srcObject = this.remoteStream; + logger.info("playing remote video. stream active? " + this.remoteStream.active); + try { + await this.remoteVideoElement.play(); + } catch (e) { + logger.info("Failed to play remote video element", e); + } + } } function setTracksEnabled(tracks: Array, enabled: boolean) {