Remove video track when muting video (#3028)

SimonBrandner · web-flow · commit fdb80ad2594b · 2023-01-06T12:09:01.000-05:00
diff --git a/spec/unit/webrtc/groupCall.spec.ts b/spec/unit/webrtc/groupCall.spec.ts
@@ -810,7 +810,10 @@ describe("Group Call", function () {
             it("should mute local video when calling setLocalVideoMuted()", async () => {
                 const groupCall = await createAndEnterGroupCall(mockClient, room);
 
-                groupCall.localCallFeed!.setAudioVideoMuted = jest.fn();
+                jest.spyOn(mockClient.getMediaHandler(), "getUserMediaStream");
+                jest.spyOn(groupCall, "updateLocalUsermediaStream");
+                jest.spyOn(groupCall.localCallFeed!, "setAudioVideoMuted");
+
                 const setAVMutedArray: ((audioMuted: boolean | null, videoMuted: boolean | null) => void)[] = [];
                 const tracksArray: MediaStreamTrack[] = [];
                 const sendMetadataUpdateArray: (() => Promise<void>)[] = [];
@@ -824,7 +827,8 @@ describe("Group Call", function () {
                 await groupCall.setLocalVideoMuted(true);
 
                 groupCall.localCallFeed!.stream.getVideoTracks().forEach((track) => expect(track.enabled).toBe(false));
-                expect(groupCall.localCallFeed!.setAudioVideoMuted).toHaveBeenCalledWith(null, true);
+                expect(mockClient.getMediaHandler().getUserMediaStream).toHaveBeenCalledWith(true, false);
+                expect(groupCall.updateLocalUsermediaStream).toHaveBeenCalled();
                 setAVMutedArray.forEach((f) => expect(f).toHaveBeenCalledWith(null, true));
                 tracksArray.forEach((track) => expect(track.enabled).toBe(false));
                 sendMetadataUpdateArray.forEach((f) => expect(f).toHaveBeenCalled());
diff --git a/spec/unit/webrtc/mediaHandler.spec.ts b/spec/unit/webrtc/mediaHandler.spec.ts
@@ -308,20 +308,18 @@ describe("Media Handler", function () {
             expect(stream2.isCloneOf(stream1)).toEqual(false);
         });
 
-        it("strips unwanted audio tracks from re-used stream", async () => {
-            const stream1 = await mediaHandler.getUserMediaStream(true, true);
-            const stream2 = (await mediaHandler.getUserMediaStream(false, true)) as unknown as MockMediaStream;
+        it("creates new stream when we no longer want audio", async () => {
+            await mediaHandler.getUserMediaStream(true, true);
+            const stream = await mediaHandler.getUserMediaStream(false, true);
 
-            expect(stream2.isCloneOf(stream1)).toEqual(true);
-            expect(stream2.getAudioTracks().length).toEqual(0);
+            expect(stream.getAudioTracks().length).toEqual(0);
         });
 
-        it("strips unwanted video tracks from re-used stream", async () => {
-            const stream1 = await mediaHandler.getUserMediaStream(true, true);
-            const stream2 = (await mediaHandler.getUserMediaStream(true, false)) as unknown as MockMediaStream;
+        it("creates new stream when we no longer want video", async () => {
+            await mediaHandler.getUserMediaStream(true, true);
+            const stream = await mediaHandler.getUserMediaStream(true, false);
 
-            expect(stream2.isCloneOf(stream1)).toEqual(true);
-            expect(stream2.getVideoTracks().length).toEqual(0);
+            expect(stream.getVideoTracks().length).toEqual(0);
         });
     });
 
diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts
@@ -1280,7 +1280,10 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
         setTracksEnabled(stream.getAudioTracks(), audioEnabled);
         setTracksEnabled(stream.getVideoTracks(), videoEnabled);
 
-        // We want to keep the same stream id, so we replace the tracks rather than the whole stream
+        // We want to keep the same stream id, so we replace the tracks rather
+        // than the whole stream.
+
+        // Firstly, we replace the tracks in our localUsermediaStream.
         for (const track of this.localUsermediaStream!.getTracks()) {
             this.localUsermediaStream!.removeTrack(track);
             track.stop();
@@ -1289,10 +1292,23 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
             this.localUsermediaStream!.addTrack(track);
         }
 
+        // Secondly, we remove tracks that we no longer need from the peer
+        // connection, if any. This only happens when we mute the video atm.
+        // This will change the transceiver direction to "inactive" and
+        // therefore cause re-negotiation.
+        for (const kind of ["audio", "video"]) {
+            const sender = this.transceivers.get(getTransceiverKey(SDPStreamMetadataPurpose.Usermedia, kind))?.sender;
+            // Only remove the track if we aren't going to immediately replace it
+            if (sender && !stream.getTracks().find((t) => t.kind === kind)) {
+                this.peerConn?.removeTrack(sender);
+            }
+        }
+        // Thirdly, we replace the old tracks, if possible.
         for (const track of stream.getTracks()) {
             const tKey = getTransceiverKey(SDPStreamMetadataPurpose.Usermedia, track.kind);
 
-            const oldSender = this.transceivers.get(tKey)?.sender;
+            const transceiver = this.transceivers.get(tKey);
+            const oldSender = transceiver?.sender;
             let added = false;
             if (oldSender) {
                 try {
@@ -1306,6 +1322,10 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
                             `) to peer connection`,
                     );
                     await oldSender.replaceTrack(track);
+                    // Set the direction to indicate we're going to be sending.
+                    // This is only necessary in the cases where we're upgrading
+                    // the call to video after downgrading it.
+                    transceiver.direction = transceiver.direction === "inactive" ? "sendonly" : "sendrecv";
                     added = true;
                 } catch (error) {
                     logger.warn(`replaceTrack failed: adding new transceiver instead`, error);
@@ -1349,7 +1369,12 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
             await this.upgradeCall(false, true);
             return this.isLocalVideoMuted();
         }
-        this.localUsermediaFeed?.setAudioVideoMuted(null, muted);
+        if (this.opponentSupportsSDPStreamMetadata()) {
+            const stream = await this.client.getMediaHandler().getUserMediaStream(true, !muted);
+            await this.updateLocalUsermediaStream(stream);
+        } else {
+            this.localUsermediaFeed?.setAudioVideoMuted(null, muted);
+        }
         this.updateMuteStatus();
         await this.sendMetadataUpdate();
         return this.isLocalVideoMuted();
diff --git a/src/webrtc/groupCall.ts b/src/webrtc/groupCall.ts
@@ -609,6 +609,9 @@ export class GroupCall extends TypedEventEmitter<
             logger.log(
                 `groupCall ${this.groupCallId} setLocalVideoMuted stream ${this.localCallFeed.stream.id} muted ${muted}`,
             );
+
+            const stream = await this.client.getMediaHandler().getUserMediaStream(true, !muted);
+            await this.updateLocalUsermediaStream(stream);
             this.localCallFeed.setAudioVideoMuted(null, muted);
             setTracksEnabled(this.localCallFeed.stream.getVideoTracks(), !muted);
         } else {
diff --git a/src/webrtc/mediaHandler.ts b/src/webrtc/mediaHandler.ts
@@ -203,24 +203,30 @@ export class MediaHandler extends TypedEventEmitter<
 
         let canReuseStream = true;
         if (this.localUserMediaStream) {
+            // This figures out if we can reuse the current localUsermediaStream
+            // based on whether or not the "mute state" (presence of tracks of a
+            // given kind) matches what is being requested
+            if (shouldRequestAudio !== this.localUserMediaStream.getAudioTracks().length > 0) {
+                canReuseStream = false;
+            }
+            if (shouldRequestVideo !== this.localUserMediaStream.getVideoTracks().length > 0) {
+                canReuseStream = false;
+            }
+
             // This code checks that the device ID is the same as the localUserMediaStream stream, but we update
             // the localUserMediaStream whenever the device ID changes (apart from when restoring) so it's not
             // clear why this would ever be different, unless there's a race.
-            if (shouldRequestAudio) {
-                if (
-                    this.localUserMediaStream.getAudioTracks().length === 0 ||
-                    this.localUserMediaStream.getAudioTracks()[0]?.getSettings()?.deviceId !== this.audioInput
-                ) {
-                    canReuseStream = false;
-                }
+            if (
+                shouldRequestAudio &&
+                this.localUserMediaStream.getAudioTracks()[0]?.getSettings()?.deviceId !== this.audioInput
+            ) {
+                canReuseStream = false;
             }
-            if (shouldRequestVideo) {
-                if (
-                    this.localUserMediaStream.getVideoTracks().length === 0 ||
-                    this.localUserMediaStream.getVideoTracks()[0]?.getSettings()?.deviceId !== this.videoInput
-                ) {
-                    canReuseStream = false;
-                }
+            if (
+                shouldRequestVideo &&
+                this.localUserMediaStream.getVideoTracks()[0]?.getSettings()?.deviceId !== this.videoInput
+            ) {
+                canReuseStream = false;
             }
         } else {
             canReuseStream = false;
