From b4f8b0fe4fed799276fd9595e3b747164ccf1015 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 24 Jan 2023 18:19:19 +0000 Subject: [PATCH 1/8] Remove video tracks on video mute without renegotiating --- src/webrtc/call.ts | 71 +++++++++++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 19 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 8f88e34ba04..f6e91f789d6 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -387,6 +387,10 @@ export class MatrixCall extends TypedEventEmitter; + /** * Construct a new Matrix Call. * @param opts - Config options. @@ -480,7 +484,9 @@ export class MatrixCall extends TypedEventEmitter feed.purpose === SDPStreamMetadataPurpose.Usermedia); } @@ -1292,18 +1306,7 @@ export class MatrixCall extends TypedEventEmitter t.kind === kind)) { - this.peerConn?.removeTrack(sender); - } - } - // Thirdly, we replace the old tracks, if possible. + // Then replace the old tracks, if possible. for (const track of stream.getTracks()) { const tKey = getTransceiverKey(SDPStreamMetadataPurpose.Usermedia, track.kind); @@ -1361,22 +1364,51 @@ export class MatrixCall extends TypedEventEmitter { logger.log(`call ${this.callId} setLocalVideoMuted ${muted}`); + + // if we were still thinking about stopping and removing the video + // track: don't, because we want it back. + if (!muted && this.stopVideoTrackTimer !== undefined) { + clearTimeout(this.stopVideoTrackTimer); + this.stopVideoTrackTimer = undefined; + } + if (!(await this.client.getMediaHandler().hasVideoDevice())) { return this.isLocalVideoMuted(); } - if (!this.hasLocalUserMediaVideoTrack && !muted) { + if (!this.hasUserMediaVideoSender && !muted) { await this.upgradeCall(false, true); return this.isLocalVideoMuted(); } - if (this.opponentSupportsSDPStreamMetadata()) { - const stream = await this.client.getMediaHandler().getUserMediaStream(true, !muted); + + // we may not have a video track - if not, re-request usermedia + if (!muted && this.localUsermediaStream!.getVideoTracks().length === 0) { + const stream = await this.client.getMediaHandler().getUserMediaStream(true, true); await this.updateLocalUsermediaStream(stream); - } else { - this.localUsermediaFeed?.setAudioVideoMuted(null, muted); } + + this.localUsermediaFeed?.setAudioVideoMuted(null, muted); + this.updateMuteStatus(); await this.sendMetadataUpdate(); + + // if we're muting video, set a timeout to stop & remove the video track so we release + // the camera. We wait a short time to do this because when we disable a track, WebRTC + // will send black video for it. If we just stop and remove it straight away, the video + // will just freeze which means that when we unmute video, the other side will briefly + // get a static frame of us from before we muted. This way, the still frame is just black. + // A very small delay is not always enough so the theory here is that it needs to be long + // enough for WebRTC to encode a frame: 120ms should be long enough even if we're only + // doing 10fps. + if (muted) { + this.stopVideoTrackTimer = setTimeout(() => { + for (const t of this.localUsermediaStream!.getVideoTracks()) { + t.stop(); + this.localUsermediaStream!.removeTrack(t); + } + }, 120); + } + return this.isLocalVideoMuted(); } @@ -1404,7 +1436,7 @@ export class MatrixCall extends TypedEventEmitter Date: Tue, 24 Jan 2023 20:30:10 +0000 Subject: [PATCH 2/8] Remove timer on call terminate --- src/webrtc/call.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index f6e91f789d6..ff656777473 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -2498,6 +2498,8 @@ export class MatrixCall extends TypedEventEmitter { if (this.callHasEnded()) return; + if (this.stopVideoTrackTimer !== undefined) clearTimeout(this.stopVideoTrackTimer); + this.hangupParty = hangupParty; this.hangupReason = hangupReason; this.state = CallState.Ended; From ce2a9d70362cd31d9fd2f8f88a5928ece5298a71 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 25 Jan 2023 10:59:03 +0000 Subject: [PATCH 3/8] Fix test --- spec/unit/webrtc/call.spec.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index ce966fbb79f..838809691e9 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -40,6 +40,7 @@ import { MockMediaStreamTrack, installWebRTCMocks, MockRTCPeerConnection, + MockRTCRtpTransceiver, SCREENSHARE_STREAM_ID, } from "../../test-utils/webrtc"; import { CallFeed } from "../../../src/webrtc/callFeed"; @@ -536,6 +537,13 @@ describe("Call", function () { it("if local video", async () => { call.getOpponentMember = jest.fn().mockReturnValue({ userId: "@bob:bar.uk" }); + // since this is testing for the presence of a local sender, we need to add a transciever + // rather than just a source track + (call as any).transceivers.set( + "m.usermedia:video", + new MockRTCRtpTransceiver(call.peerConn as unknown as MockRTCPeerConnection), + ); + (call as any).pushNewLocalFeed( new MockMediaStream("remote_stream1", [new MockMediaStreamTrack("track_id", "video")]), SDPStreamMetadataPurpose.Usermedia, From b328b72cd587c6151cc9ae6e82a28be0197d585f Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 25 Jan 2023 11:07:14 +0000 Subject: [PATCH 4/8] Move timeout clear to be with its friends --- src/webrtc/call.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index ff656777473..f601c90f61e 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -2498,8 +2498,6 @@ export class MatrixCall extends TypedEventEmitter { if (this.callHasEnded()) return; - if (this.stopVideoTrackTimer !== undefined) clearTimeout(this.stopVideoTrackTimer); - this.hangupParty = hangupParty; this.hangupReason = hangupReason; this.state = CallState.Ended; @@ -2512,6 +2510,10 @@ export class MatrixCall extends TypedEventEmitter Date: Wed, 25 Jan 2023 11:16:19 +0000 Subject: [PATCH 5/8] Actually check we have a sender, not just a transceiver --- spec/unit/webrtc/call.spec.ts | 11 ++++++----- src/webrtc/call.ts | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 838809691e9..8bedb34f809 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -42,6 +42,7 @@ import { MockRTCPeerConnection, MockRTCRtpTransceiver, SCREENSHARE_STREAM_ID, + MockRTCRtpSender, } from "../../test-utils/webrtc"; import { CallFeed } from "../../../src/webrtc/callFeed"; import { EventType, IContent, ISendEventResponse, MatrixEvent, Room } from "../../../src"; @@ -539,13 +540,13 @@ describe("Call", function () { // since this is testing for the presence of a local sender, we need to add a transciever // rather than just a source track - (call as any).transceivers.set( - "m.usermedia:video", - new MockRTCRtpTransceiver(call.peerConn as unknown as MockRTCPeerConnection), - ); + const mockTrack = new MockMediaStreamTrack("track_id", "video"); + const mockTransceiver = new MockRTCRtpTransceiver(call.peerConn as unknown as MockRTCPeerConnection); + mockTransceiver.sender = new MockRTCRtpSender(mockTrack) as unknown as RTCRtpSender; + (call as any).transceivers.set("m.usermedia:video", mockTransceiver); (call as any).pushNewLocalFeed( - new MockMediaStream("remote_stream1", [new MockMediaStreamTrack("track_id", "video")]), + new MockMediaStream("remote_stream1", [mockTrack]), SDPStreamMetadataPurpose.Usermedia, false, ); diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index f601c90f61e..98c8e027bf4 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -514,7 +514,7 @@ export class MatrixCall extends TypedEventEmitter Date: Wed, 25 Jan 2023 11:20:25 +0000 Subject: [PATCH 6/8] Check we have both a sender and a track to send --- src/webrtc/call.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 98c8e027bf4..3665b9d18a4 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -1436,7 +1436,7 @@ export class MatrixCall extends TypedEventEmitter Date: Wed, 25 Jan 2023 15:06:36 +0000 Subject: [PATCH 7/8] Add tests --- spec/unit/webrtc/call.spec.ts | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 8bedb34f809..d782c61a903 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -838,6 +838,55 @@ describe("Call", function () { await startVideoCall(client, call); }); + afterEach(() => { + jest.useRealTimers(); + }); + + it("should not remove video sender on video mute", async () => { + await call.setLocalVideoMuted(true); + expect((call as any).hasUserMediaVideoSender).toBe(true); + }); + + it("should release camera after short delay on video mute", async () => { + jest.useFakeTimers(); + + await call.setLocalVideoMuted(true); + + jest.advanceTimersByTime(500); + + expect(call.hasLocalUserMediaVideoTrack).toBe(false); + }); + + it("should re-request video feed on video unmute if it doesn't have one", async () => { + jest.useFakeTimers(); + + const mockGetUserMediaStream = jest + .fn() + .mockReturnValue(client.client.getMediaHandler().getUserMediaStream(true, true)); + + client.client.getMediaHandler().getUserMediaStream = mockGetUserMediaStream; + + await call.setLocalVideoMuted(true); + + jest.advanceTimersByTime(500); + + await call.setLocalVideoMuted(false); + + expect(mockGetUserMediaStream).toHaveBeenCalled(); + }); + + it("should not release camera on fast mute and unmute", async () => { + const mockGetUserMediaStream = jest.fn(); + + client.client.getMediaHandler().getUserMediaStream = mockGetUserMediaStream; + + await call.setLocalVideoMuted(true); + await call.setLocalVideoMuted(false); + + expect(mockGetUserMediaStream).not.toHaveBeenCalled(); + expect(call.hasLocalUserMediaVideoTrack).toBe(true); + }); + describe("sending sdp_stream_metadata_changed events", () => { it("should send sdp_stream_metadata_changed when muting audio", async () => { await call.setMicrophoneMuted(true); From a18d4e226ebdd0dbfea26bf77da7e7ba50eea0c4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 25 Jan 2023 15:07:51 +0000 Subject: [PATCH 8/8] Actually check audio sender too --- src/webrtc/call.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 3665b9d18a4..e0c8310a338 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -510,7 +510,7 @@ export class MatrixCall extends TypedEventEmitter