From 92efd30cbcabe49a5db65459d18706ee9d0b828c Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 18 Oct 2022 21:04:12 +0100 Subject: [PATCH 01/10] Fix screenshare failing after several attempts Re-use any existing transceivers when screen sharing. This prevents transceivers accumulating and making the SDP too big: see linked bug. This also switches from `addTrack()` to `addTransceiver ()` which is not that large of a change, other than having to explicitly find the transceivers after an offer has arrived rather than just adding tracks and letting WebRTC take care of it. Fixes https://github.com/vector-im/element-call/issues/625 --- src/webrtc/call.ts | 151 ++++++++++++++++++++++++++++------------ src/webrtc/groupCall.ts | 4 +- 2 files changed, 108 insertions(+), 47 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index ce1cae8cd08..2358d0f2313 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -308,6 +308,31 @@ export type CallEventHandlerMap = { [CallEvent.SendVoipEvent]: (event: Record) => void; }; +enum TransceiverIndex { + UserMediaAudio = 0, + UserMediaVideo = 1, + ScreenshareAudio = 2, + ScreenshareVideo = 3, +} + +function getTransceiverIndex(purpose: SDPStreamMetadataPurpose, kind: string): TransceiverIndex { + if (purpose == SDPStreamMetadataPurpose.Usermedia) { + if (kind == MediaType.AUDIO) { + return TransceiverIndex.UserMediaAudio; + } else if (kind == MediaType.VIDEO) { + return TransceiverIndex.UserMediaVideo; + } + } else { + if (kind == MediaType.AUDIO) { + return TransceiverIndex.ScreenshareAudio; + } else if (kind == MediaType.VIDEO) { + return TransceiverIndex.ScreenshareVideo; + } + } + + throw new Error(`Unknown media purpose / kind: ${purpose} / ${kind}`); +} + /** * Construct a new Matrix Call. * @constructor @@ -345,8 +370,10 @@ export class MatrixCall extends TypedEventEmitter = []; - private usermediaSenders: Array = []; - private screensharingSenders: Array = []; + + // our transceivers for each purpose and type of media, according to SenderIndex + private transceivers: RTCRtpTransceiver[] = [null, null, null, null]; + private inviteOrAnswerSent = false; private waitForLocalAVStream: boolean; private successor: MatrixCall; @@ -634,6 +661,18 @@ export class MatrixCall extends TypedEventEmitter t.receiver.track == track); + this.transceivers[getTransceiverIndex(purpose, track.kind)] = transceiver; + } + } + this.emit(CallEvent.FeedsChanged, this.feeds); logger.info( @@ -675,6 +714,12 @@ export class MatrixCall extends TypedEventEmitter t.receiver.track == track); + this.transceivers[getTransceiverIndex(purpose, track.kind)] = transceiver; + } + this.emit(CallEvent.FeedsChanged, this.feeds); logger.info(`Call ${this.callId} pushed remote stream (id="${stream.id}", active="${stream.active}")`); @@ -722,11 +767,6 @@ export class MatrixCall extends TypedEventEmitter { return track.kind === "video"; }); - const sender = this.usermediaSenders.find((sender) => { - return sender.track?.kind === "video"; - }); + + const sender = this.transceivers[getTransceiverIndex( + SDPStreamMetadataPurpose.Usermedia, "video", + )].sender; + sender.replaceTrack(track); this.pushNewLocalFeed(stream, SDPStreamMetadataPurpose.Screenshare, false); @@ -1183,9 +1252,9 @@ export class MatrixCall extends TypedEventEmitter { return track.kind === "video"; }); - const sender = this.usermediaSenders.find((sender) => { - return sender.track?.kind === "video"; - }); + const sender = this.transceivers[getTransceiverIndex( + SDPStreamMetadataPurpose.Usermedia, "video", + )].sender; sender.replaceTrack(track); this.client.getMediaHandler().stopScreensharingStream(this.localScreensharingStream); @@ -1219,14 +1288,10 @@ export class MatrixCall extends TypedEventEmitter { - return sender.track?.kind === track.kind; - }); + const tIdx = getTransceiverIndex(SDPStreamMetadataPurpose.Usermedia, track.kind); - let newSender: RTCRtpSender; + const oldSender = this.transceivers[tIdx].sender; try { logger.info( @@ -1239,7 +1304,6 @@ export class MatrixCall extends TypedEventEmitter => { diff --git a/src/webrtc/groupCall.ts b/src/webrtc/groupCall.ts index 350215b9a0f..c20592f52f9 100644 --- a/src/webrtc/groupCall.ts +++ b/src/webrtc/groupCall.ts @@ -607,7 +607,9 @@ export class GroupCall extends TypedEventEmitter< return false; } } else { - await Promise.all(this.calls.map(call => call.removeLocalFeed(call.localScreensharingFeed))); + await Promise.all(this.calls.map(call => { + if (call.localScreensharingFeed) call.removeLocalFeed(call.localScreensharingFeed); + })); this.client.getMediaHandler().stopScreensharingStream(this.localScreenshareFeed.stream); this.removeScreenshareFeed(this.localScreenshareFeed); this.localScreenshareFeed = undefined; From e864d2f9cd9fb5fec4c9595477104370a8d977f2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 18 Oct 2022 22:16:46 +0100 Subject: [PATCH 02/10] Fix tests --- spec/test-utils/webrtc.ts | 36 +++++++++++++++----- spec/unit/webrtc/call.spec.ts | 64 +++++++++++++++++++++++++---------- src/webrtc/call.ts | 33 +++++++++++------- 3 files changed, 94 insertions(+), 39 deletions(-) diff --git a/spec/test-utils/webrtc.ts b/spec/test-utils/webrtc.ts index 09849d4f32b..fda0c09e92d 100644 --- a/spec/test-utils/webrtc.ts +++ b/spec/test-utils/webrtc.ts @@ -109,7 +109,7 @@ export class MockRTCPeerConnection { private onReadyToNegotiate: () => void; localDescription: RTCSessionDescription; signalingState: RTCSignalingState = "stable"; - public senders: MockRTCRtpSender[] = []; + public transceivers: MockRTCRtpTransceiver[] = []; public static triggerAllNegotiations(): void { for (const inst of this.instances) { @@ -169,12 +169,23 @@ export class MockRTCPeerConnection { } close() { } getStats() { return []; } - addTrack(track: MockMediaStreamTrack): MockRTCRtpSender { + addTransceiver(track: MockMediaStreamTrack): MockRTCRtpTransceiver { this.needsNegotiation = true; this.onReadyToNegotiate(); + const newSender = new MockRTCRtpSender(track); - this.senders.push(newSender); - return newSender; + const newReceiver = new MockRTCRtpReceiver(track); + + const newTransceiver = new MockRTCRtpTransceiver(); + newTransceiver.sender = newSender as unknown as RTCRtpSender; + newTransceiver.receiver = newReceiver as unknown as RTCRtpReceiver; + + this.transceivers.push(newTransceiver); + + return newTransceiver; + } + addTrack(track: MockMediaStreamTrack): MockRTCRtpSender { + return this.addTransceiver(track).sender as unknown as MockRTCRtpSender; } removeTrack() { @@ -182,9 +193,8 @@ export class MockRTCPeerConnection { this.onReadyToNegotiate(); } - getSenders(): MockRTCRtpSender[] { return this.senders; } - - getTransceivers = jest.fn().mockReturnValue([]); + getTransceivers(): MockRTCRtpTransceiver[] { return this.transceivers; } + getSenders(): MockRTCRtpSender[] { return this.transceivers.map(t => t.sender as unknown as MockRTCRtpSender); } doNegotiation() { if (this.needsNegotiation && this.negotiationNeededListener) { @@ -198,7 +208,17 @@ export class MockRTCRtpSender { constructor(public track: MockMediaStreamTrack) { } replaceTrack(track: MockMediaStreamTrack) { this.track = track; } - setCodecPreferences(prefs: RTCRtpCodecCapability[]): void {} +} + +export class MockRTCRtpReceiver { + constructor(public track: MockMediaStreamTrack) { } +} + +export class MockRTCRtpTransceiver { + public sender: RTCRtpSender; + public receiver: RTCRtpReceiver; + + setCodecPreferences = jest.fn(); } export class MockMediaStreamTrack { diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index e592cba9b67..37a1e878941 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -110,6 +110,10 @@ describe('Call', function() { const errorListener = () => {}; + // XXX redefining private enum values from call + const userMediaAudioTcvrIdx = 0; + const userMediaVideoTcvrIdx = 1; + beforeEach(function() { prevNavigator = global.navigator; prevDocument = global.document; @@ -370,17 +374,14 @@ describe('Call', function() { ).typed(), ); - const usermediaSenders: Array = (call as any).usermediaSenders; + // XXX: Lots of inspecting the prvate state of the call object here + const transceivers: Array = (call as any).transceivers; expect(call.localUsermediaStream.id).toBe("stream"); expect(call.localUsermediaStream.getAudioTracks()[0].id).toBe("new_audio_track"); expect(call.localUsermediaStream.getVideoTracks()[0].id).toBe("video_track"); - expect(usermediaSenders.find((sender) => { - return sender?.track?.kind === "audio"; - }).track.id).toBe("new_audio_track"); - expect(usermediaSenders.find((sender) => { - return sender?.track?.kind === "video"; - }).track.id).toBe("video_track"); + expect(transceivers[userMediaAudioTcvrIdx].sender.track.id).toBe("new_audio_track"); + expect(transceivers[userMediaVideoTcvrIdx].sender.track.id).toBe("video_track"); }); it("should handle upgrade to video call", async () => { @@ -400,16 +401,13 @@ describe('Call', function() { // setLocalVideoMuted probably? await (call as any).upgradeCall(false, true); - const usermediaSenders: Array = (call as any).usermediaSenders; + // XXX: More inspecting private state of the call object + const transceivers: Array = (call as any).transceivers; expect(call.localUsermediaStream.getAudioTracks()[0].id).toBe("usermedia_audio_track"); expect(call.localUsermediaStream.getVideoTracks()[0].id).toBe("usermedia_video_track"); - expect(usermediaSenders.find((sender) => { - return sender?.track?.kind === "audio"; - }).track.id).toBe("usermedia_audio_track"); - expect(usermediaSenders.find((sender) => { - return sender?.track?.kind === "video"; - }).track.id).toBe("usermedia_video_track"); + expect(transceivers[userMediaAudioTcvrIdx].sender.track.id).toBe("usermedia_audio_track"); + expect(transceivers[userMediaVideoTcvrIdx].sender.track.id).toBe("usermedia_video_track"); }); it("should handle SDPStreamMetadata changes", async () => { @@ -479,6 +477,23 @@ describe('Call', function() { }); describe("should deduce the call type correctly", () => { + beforeEach(async () => { + // start an incoming call, but add no feeds + await call.initWithInvite({ + getContent: jest.fn().mockReturnValue({ + version: "1", + call_id: "call_id", + party_id: "remote_party_id", + lifetime: CALL_LIFETIME, + offer: { + sdp: DUMMY_SDP, + }, + }), + getSender: () => "@test:foo", + getLocalAge: () => 1, + } as unknown as MatrixEvent); + }); + it("if no video", async () => { call.getOpponentMember = jest.fn().mockReturnValue({ userId: "@bob:bar.uk" }); @@ -1130,9 +1145,17 @@ describe('Call', function() { headerExtensions: [], }); - const prom = new Promise(resolve => { + mockSendEvent.mockReset(); + const sendNegotiatePromise = new Promise(resolve => { + mockSendEvent.mockImplementationOnce(() => { + resolve(); + return Promise.resolve({ event_id: "foo" }); + }); + }); + + /*const prom = new Promise(resolve => { const mockPeerConn = call.peerConn as unknown as MockRTCPeerConnection; - mockPeerConn.addTrack = jest.fn().mockImplementation((track: MockMediaStreamTrack) => { + mockPeerConn.addTransceiver = jest.fn().mockImplementation((track: MockMediaStreamTrack) => { const mockSender = new MockRTCRtpSender(track); mockPeerConn.getTransceivers.mockReturnValue([{ sender: mockSender, @@ -1147,12 +1170,17 @@ describe('Call', function() { return mockSender; }); - }); + });*/ await call.setScreensharingEnabled(true); MockRTCPeerConnection.triggerAllNegotiations(); - await prom; + await sendNegotiatePromise; + + const mockPeerConn = call.peerConn as unknown as MockRTCPeerConnection; + expect( + mockPeerConn.transceivers[mockPeerConn.transceivers.length - 1].setCodecPreferences, + ).toHaveBeenCalledWith([expect.objectContaining({ mimeType: "video/somethingelse" })]); }); }); diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 2358d0f2313..dea9aa93ff3 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -1291,20 +1291,27 @@ export class MatrixCall extends TypedEventEmitter Date: Tue, 18 Oct 2022 22:20:17 +0100 Subject: [PATCH 03/10] Unused import --- spec/unit/webrtc/call.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 37a1e878941..ee752153779 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -41,7 +41,6 @@ import { installWebRTCMocks, MockRTCPeerConnection, SCREENSHARE_STREAM_ID, - MockRTCRtpSender, } from "../../test-utils/webrtc"; import { CallFeed } from "../../../src/webrtc/callFeed"; import { EventType, IContent, ISendEventResponse, MatrixEvent, Room } from "../../../src"; From 748bd8cad358baca57bdf401b6f62d3eb7a65285 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Oct 2022 10:58:13 +0100 Subject: [PATCH 04/10] Use a map instead of an array --- spec/unit/webrtc/call.spec.ts | 17 +++---- src/webrtc/call.ts | 89 +++++++++++++++-------------------- 2 files changed, 44 insertions(+), 62 deletions(-) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index ee752153779..7783f145f2a 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -109,10 +109,6 @@ describe('Call', function() { const errorListener = () => {}; - // XXX redefining private enum values from call - const userMediaAudioTcvrIdx = 0; - const userMediaVideoTcvrIdx = 1; - beforeEach(function() { prevNavigator = global.navigator; prevDocument = global.document; @@ -374,13 +370,14 @@ describe('Call', function() { ); // XXX: Lots of inspecting the prvate state of the call object here - const transceivers: Array = (call as any).transceivers; + const transceivers: Map = (call as any).transceivers; expect(call.localUsermediaStream.id).toBe("stream"); expect(call.localUsermediaStream.getAudioTracks()[0].id).toBe("new_audio_track"); expect(call.localUsermediaStream.getVideoTracks()[0].id).toBe("video_track"); - expect(transceivers[userMediaAudioTcvrIdx].sender.track.id).toBe("new_audio_track"); - expect(transceivers[userMediaVideoTcvrIdx].sender.track.id).toBe("video_track"); + // call has a function for generating these but we hardcode here to avoid exporting it + expect(transceivers.get("m.usermedia:audio").sender.track.id).toBe("new_audio_track"); + expect(transceivers.get("m.usermedia:video").sender.track.id).toBe("video_track"); }); it("should handle upgrade to video call", async () => { @@ -401,12 +398,12 @@ describe('Call', function() { await (call as any).upgradeCall(false, true); // XXX: More inspecting private state of the call object - const transceivers: Array = (call as any).transceivers; + const transceivers: Map = (call as any).transceivers; expect(call.localUsermediaStream.getAudioTracks()[0].id).toBe("usermedia_audio_track"); expect(call.localUsermediaStream.getVideoTracks()[0].id).toBe("usermedia_video_track"); - expect(transceivers[userMediaAudioTcvrIdx].sender.track.id).toBe("usermedia_audio_track"); - expect(transceivers[userMediaVideoTcvrIdx].sender.track.id).toBe("usermedia_video_track"); + expect(transceivers.get("m.usermedia:audio").sender.track.id).toBe("usermedia_audio_track"); + expect(transceivers.get("m.usermedia:video").sender.track.id).toBe("usermedia_video_track"); }); it("should handle SDPStreamMetadata changes", async () => { diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index dea9aa93ff3..735f7fec4dd 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -308,29 +308,9 @@ export type CallEventHandlerMap = { [CallEvent.SendVoipEvent]: (event: Record) => void; }; -enum TransceiverIndex { - UserMediaAudio = 0, - UserMediaVideo = 1, - ScreenshareAudio = 2, - ScreenshareVideo = 3, -} - -function getTransceiverIndex(purpose: SDPStreamMetadataPurpose, kind: string): TransceiverIndex { - if (purpose == SDPStreamMetadataPurpose.Usermedia) { - if (kind == MediaType.AUDIO) { - return TransceiverIndex.UserMediaAudio; - } else if (kind == MediaType.VIDEO) { - return TransceiverIndex.UserMediaVideo; - } - } else { - if (kind == MediaType.AUDIO) { - return TransceiverIndex.ScreenshareAudio; - } else if (kind == MediaType.VIDEO) { - return TransceiverIndex.ScreenshareVideo; - } - } - - throw new Error(`Unknown media purpose / kind: ${purpose} / ${kind}`); +// generates keys for the map of transceivers +function getTransceiverKey(purpose: SDPStreamMetadataPurpose, kind: string): string { + return purpose + ':' + kind; } /** @@ -371,8 +351,8 @@ export class MatrixCall extends TypedEventEmitter = []; - // our transceivers for each purpose and type of media, according to SenderIndex - private transceivers: RTCRtpTransceiver[] = [null, null, null, null]; + // our transceivers for each purpose and type of media + private transceivers = new Map(); private inviteOrAnswerSent = false; private waitForLocalAVStream: boolean; @@ -669,7 +649,7 @@ export class MatrixCall extends TypedEventEmitter t.receiver.track == track); - this.transceivers[getTransceiverIndex(purpose, track.kind)] = transceiver; + this.transceivers.set(getTransceiverKey(purpose, track.kind), transceiver); } } @@ -717,7 +697,7 @@ export class MatrixCall extends TypedEventEmitter t.receiver.track == track); - this.transceivers[getTransceiverIndex(purpose, track.kind)] = transceiver; + this.transceivers.set(getTransceiverKey(purpose, track.kind), transceiver); } this.emit(CallEvent.FeedsChanged, this.feeds); @@ -779,22 +759,24 @@ export class MatrixCall extends TypedEventEmitter { return track.kind === "video"; }); - const sender = this.transceivers[getTransceiverIndex( + const sender = this.transceivers.get(getTransceiverKey( SDPStreamMetadataPurpose.Usermedia, "video", - )].sender; + )).sender; sender.replaceTrack(track); this.client.getMediaHandler().stopScreensharingStream(this.localScreensharingStream); @@ -1289,9 +1274,9 @@ export class MatrixCall extends TypedEventEmitter Date: Wed, 19 Oct 2022 11:01:16 +0100 Subject: [PATCH 05/10] Add comment --- src/webrtc/call.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 735f7fec4dd..e7f0c6dac2b 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -309,6 +309,8 @@ export type CallEventHandlerMap = { }; // generates keys for the map of transceivers +// kind is unfortunately a string rather than MediaType as this is the type of +// track.kind function getTransceiverKey(purpose: SDPStreamMetadataPurpose, kind: string): string { return purpose + ':' + kind; } From d2b1bc1a8e047f07e44c9d87b07aa988bf7e0da6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Oct 2022 11:05:56 +0100 Subject: [PATCH 06/10] more comment --- src/webrtc/call.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index e7f0c6dac2b..6d101b8a6df 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -771,8 +771,9 @@ export class MatrixCall extends TypedEventEmitter Date: Wed, 19 Oct 2022 11:26:34 +0100 Subject: [PATCH 07/10] Remove commented code --- spec/unit/webrtc/call.spec.ts | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 7783f145f2a..607fdcbc18d 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -1149,25 +1149,6 @@ describe('Call', function() { }); }); - /*const prom = new Promise(resolve => { - const mockPeerConn = call.peerConn as unknown as MockRTCPeerConnection; - mockPeerConn.addTransceiver = jest.fn().mockImplementation((track: MockMediaStreamTrack) => { - const mockSender = new MockRTCRtpSender(track); - mockPeerConn.getTransceivers.mockReturnValue([{ - sender: mockSender, - setCodecPreferences: (prefs: RTCRtpCodecCapability[]) => { - expect(prefs).toEqual([ - expect.objectContaining({ mimeType: "video/somethingelse" }), - ]); - - resolve(); - }, - }]); - - return mockSender; - }); - });*/ - await call.setScreensharingEnabled(true); MockRTCPeerConnection.triggerAllNegotiations(); From 91cee49dd36319d48b9e26e130c971f397d4bd74 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Oct 2022 11:48:52 +0100 Subject: [PATCH 08/10] Remove unintentional debugging --- src/webrtc/call.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 6d101b8a6df..3ab629dd895 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -1975,10 +1975,8 @@ export class MatrixCall extends TypedEventEmitter Date: Wed, 19 Oct 2022 14:52:20 +0100 Subject: [PATCH 09/10] Add test for screenshare transceiver re-use --- spec/test-utils/webrtc.ts | 10 ++++-- spec/unit/webrtc/call.spec.ts | 63 ++++++++++++++++++++++++++++------- 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/spec/test-utils/webrtc.ts b/spec/test-utils/webrtc.ts index fda0c09e92d..ac9148212b9 100644 --- a/spec/test-utils/webrtc.ts +++ b/spec/test-utils/webrtc.ts @@ -104,7 +104,7 @@ export class MockRTCPeerConnection { private negotiationNeededListener: () => void; public iceCandidateListener?: (e: RTCPeerConnectionIceEvent) => void; public onTrackListener?: (e: RTCTrackEvent) => void; - private needsNegotiation = false; + public needsNegotiation = false; public readyToNegotiate: Promise; private onReadyToNegotiate: () => void; localDescription: RTCSessionDescription; @@ -176,7 +176,7 @@ export class MockRTCPeerConnection { const newSender = new MockRTCRtpSender(track); const newReceiver = new MockRTCRtpReceiver(track); - const newTransceiver = new MockRTCRtpTransceiver(); + const newTransceiver = new MockRTCRtpTransceiver(this); newTransceiver.sender = newSender as unknown as RTCRtpSender; newTransceiver.receiver = newReceiver as unknown as RTCRtpReceiver; @@ -215,9 +215,15 @@ export class MockRTCRtpReceiver { } export class MockRTCRtpTransceiver { + constructor(private peerConn: MockRTCPeerConnection) {} + public sender: RTCRtpSender; public receiver: RTCRtpReceiver; + public set direction(_: string) { + this.peerConn.needsNegotiation = true; + } + setCodecPreferences = jest.fn(); } diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 607fdcbc18d..df9c2aee09b 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -1068,9 +1068,24 @@ describe('Call', function() { }); describe("Screen sharing", () => { + const waitNegotiateFunc = resolve => { + mockSendEvent.mockImplementationOnce(() => { + // Note that the peer connection here is a dummy one and always returns + // dummy SDP, so there's not much point returning the content: the SDP will + // always be the same. + resolve(); + return Promise.resolve({ event_id: "foo" }); + }); + }; + beforeEach(async () => { await startVoiceCall(client, call); + const sendNegotiatePromise = new Promise(waitNegotiateFunc); + + MockRTCPeerConnection.triggerAllNegotiations(); + await sendNegotiatePromise; + await call.onAnswerReceived(makeMockEvent("@test:foo", { "version": 1, "call_id": call.callId, @@ -1101,12 +1116,7 @@ describe('Call', function() { ).toHaveLength(1); mockSendEvent.mockReset(); - const sendNegotiatePromise = new Promise(resolve => { - mockSendEvent.mockImplementationOnce(() => { - resolve(); - return Promise.resolve({ event_id: "foo" }); - }); - }); + const sendNegotiatePromise = new Promise(waitNegotiateFunc); MockRTCPeerConnection.triggerAllNegotiations(); await sendNegotiatePromise; @@ -1142,12 +1152,7 @@ describe('Call', function() { }); mockSendEvent.mockReset(); - const sendNegotiatePromise = new Promise(resolve => { - mockSendEvent.mockImplementationOnce(() => { - resolve(); - return Promise.resolve({ event_id: "foo" }); - }); - }); + const sendNegotiatePromise = new Promise(waitNegotiateFunc); await call.setScreensharingEnabled(true); MockRTCPeerConnection.triggerAllNegotiations(); @@ -1159,6 +1164,40 @@ describe('Call', function() { mockPeerConn.transceivers[mockPeerConn.transceivers.length - 1].setCodecPreferences, ).toHaveBeenCalledWith([expect.objectContaining({ mimeType: "video/somethingelse" })]); }); + + it("re-uses transceiver when screen sharing is re-enabled", async () => { + const mockPeerConn = call.peerConn as unknown as MockRTCPeerConnection; + + // sanity check: we should start with one transciever (user media audio) + expect(mockPeerConn.transceivers.length).toEqual(1); + + const screenshareOnProm1 = new Promise(waitNegotiateFunc); + + await call.setScreensharingEnabled(true); + MockRTCPeerConnection.triggerAllNegotiations(); + + await screenshareOnProm1; + + // we should now have another transciever for the screenshare + expect(mockPeerConn.transceivers.length).toEqual(2); + + const screenshareOffProm = new Promise(waitNegotiateFunc); + await call.setScreensharingEnabled(false); + MockRTCPeerConnection.triggerAllNegotiations(); + await screenshareOffProm; + + // both transceivers should still be there + expect(mockPeerConn.transceivers.length).toEqual(2); + + const screenshareOnProm2 = new Promise(waitNegotiateFunc); + await call.setScreensharingEnabled(true); + MockRTCPeerConnection.triggerAllNegotiations(); + await screenshareOnProm2; + + // should still be two, ie. another one should not have been created + // when re-enabling the screen share. + expect(mockPeerConn.transceivers.length).toEqual(2); + }); }); it("falls back to replaceTrack for opponents that don't support stream metadata", async () => { From 0e740d48918233d9ad3533376585fb351069fc54 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Oct 2022 15:47:13 +0100 Subject: [PATCH 10/10] Type alias for transceiver map --- src/webrtc/call.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 3ab629dd895..1fb6a4c2293 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -308,10 +308,13 @@ export type CallEventHandlerMap = { [CallEvent.SendVoipEvent]: (event: Record) => void; }; +// The key of the transceiver map (purpose + media type, separated by ':') +type TransceiverKey = string; + // generates keys for the map of transceivers // kind is unfortunately a string rather than MediaType as this is the type of // track.kind -function getTransceiverKey(purpose: SDPStreamMetadataPurpose, kind: string): string { +function getTransceiverKey(purpose: SDPStreamMetadataPurpose, kind: TransceiverKey): string { return purpose + ':' + kind; } @@ -354,7 +357,7 @@ export class MatrixCall extends TypedEventEmitter = []; // our transceivers for each purpose and type of media - private transceivers = new Map(); + private transceivers = new Map(); private inviteOrAnswerSent = false; private waitForLocalAVStream: boolean;