From d7b75e4b9e25463b355f922b21fa3e7cd89ad4fc Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 24 Mar 2023 14:09:22 +0000 Subject: [PATCH 1/7] Add the call object to Call events As explained in the comment. I've added it to the end so this should be completely backwards compatible (although it would be much nicer if it were the first arg, probably). --- spec/unit/webrtc/call.spec.ts | 4 +- spec/unit/webrtc/callFeed.spec.ts | 2 +- spec/unit/webrtc/groupCall.spec.ts | 6 +- src/webrtc/call.ts | 110 ++++++++++++++++++----------- 4 files changed, 75 insertions(+), 47 deletions(-) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 51798d441af..646a2c7eec2 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -737,7 +737,7 @@ describe("Call", function () { const dataChannel = call.createDataChannel("data_channel_label", { id: 123 }); - expect(dataChannelCallback).toHaveBeenCalledWith(dataChannel); + expect(dataChannelCallback).toHaveBeenCalledWith(dataChannel, call); expect(dataChannel.label).toBe("data_channel_label"); expect(dataChannel.id).toBe(123); }); @@ -1604,7 +1604,7 @@ describe("Call", function () { hasAdvancedBy += advanceBy; expect(lengthChangedListener).toHaveBeenCalledTimes(hasAdvancedBy); - expect(lengthChangedListener).toHaveBeenCalledWith(hasAdvancedBy); + expect(lengthChangedListener).toHaveBeenCalledWith(hasAdvancedBy, call); } }); diff --git a/spec/unit/webrtc/callFeed.spec.ts b/spec/unit/webrtc/callFeed.spec.ts index 803e4648ed6..ea80d5267f2 100644 --- a/spec/unit/webrtc/callFeed.spec.ts +++ b/spec/unit/webrtc/callFeed.spec.ts @@ -102,7 +102,7 @@ describe("CallFeed", () => { [CallState.Connected, true], [CallState.Connecting, false], ])("should react to call state, when !isLocal()", (state: CallState, expected: Boolean) => { - call.emit(CallEvent.State, state); + call.emit(CallEvent.State, state, CallState.InviteSent, call.typed()); expect(feed.connected).toBe(expected); }); diff --git a/spec/unit/webrtc/groupCall.spec.ts b/spec/unit/webrtc/groupCall.spec.ts index 9743b332e14..c1cd0d3cb2e 100644 --- a/spec/unit/webrtc/groupCall.spec.ts +++ b/spec/unit/webrtc/groupCall.spec.ts @@ -792,7 +792,7 @@ describe("Group Call", function () { call.isLocalVideoMuted = jest.fn().mockReturnValue(true); call.setLocalVideoMuted = jest.fn(); - call.emit(CallEvent.State, CallState.Connected); + call.emit(CallEvent.State, CallState.Connected, CallState.InviteSent, call); expect(call.setMicrophoneMuted).toHaveBeenCalledWith(false); expect(call.setLocalVideoMuted).toHaveBeenCalledWith(false); @@ -1080,7 +1080,7 @@ describe("Group Call", function () { }); it("handles regular case", () => { - oldMockCall.emit(CallEvent.Replaced, newMockCall.typed()); + oldMockCall.emit(CallEvent.Replaced, newMockCall.typed(), oldMockCall.typed()); expect(oldMockCall.hangup).toHaveBeenCalled(); expect(callChangedListener).toHaveBeenCalledWith(newCallsMap); @@ -1091,7 +1091,7 @@ describe("Group Call", function () { it("handles case where call is missing from the calls map", () => { // @ts-ignore groupCall.calls = new Map(); - oldMockCall.emit(CallEvent.Replaced, newMockCall.typed()); + oldMockCall.emit(CallEvent.Replaced, newMockCall.typed(), oldMockCall.typed()); expect(oldMockCall.hangup).toHaveBeenCalled(); expect(callChangedListener).toHaveBeenCalledWith(newCallsMap); diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index d74b755eb5e..85f4fb88503 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -296,20 +296,34 @@ export interface VoipEvent { content: Record; } +/** + * These now all have the call object as an argument. Why? Well, to know which call a given event is + * about you have three options: + * 1. Use a closure as the callback that remembers what call it's listening to. This can be + * a pain because you need to pass the listener function again when you remove the listener, + * which might be somewhere else. + * 2. Use not-very-well-known fact that EventEmitter sets 'this' to the emitter object in the + * callback. This doesn't really play well with modern Typescript and eslint and doesn't work + * with our pattern of re-emitting events. + * 3. Pass the object in question as an argument to the callback. + * + * Now that we have group calls which have to deal with multiple call objects, this will + * become more important, and I think methods 1 and 2 are just going to cause issues. + */ export type CallEventHandlerMap = { - [CallEvent.DataChannel]: (channel: RTCDataChannel) => void; - [CallEvent.FeedsChanged]: (feeds: CallFeed[]) => void; - [CallEvent.Replaced]: (newCall: MatrixCall) => void; - [CallEvent.Error]: (error: CallError) => void; - [CallEvent.RemoteHoldUnhold]: (onHold: boolean) => void; - [CallEvent.LocalHoldUnhold]: (onHold: boolean) => void; - [CallEvent.LengthChanged]: (length: number) => void; - [CallEvent.State]: (state: CallState, oldState?: CallState) => void; + [CallEvent.DataChannel]: (channel: RTCDataChannel, call: MatrixCall) => void; + [CallEvent.FeedsChanged]: (feeds: CallFeed[], call: MatrixCall) => void; + [CallEvent.Replaced]: (newCall: MatrixCall, oldCall: MatrixCall) => void; + [CallEvent.Error]: (error: CallError, call: MatrixCall) => void; + [CallEvent.RemoteHoldUnhold]: (onHold: boolean, call: MatrixCall) => void; + [CallEvent.LocalHoldUnhold]: (onHold: boolean, call: MatrixCall) => void; + [CallEvent.LengthChanged]: (length: number, call: MatrixCall) => void; + [CallEvent.State]: (state: CallState, oldState: CallState, call: MatrixCall) => void; [CallEvent.Hangup]: (call: MatrixCall) => void; - [CallEvent.AssertedIdentityChanged]: () => void; + [CallEvent.AssertedIdentityChanged]: (call: MatrixCall) => void; /* @deprecated */ [CallEvent.HoldUnhold]: (onHold: boolean) => void; - [CallEvent.SendVoipEvent]: (event: VoipEvent) => void; + [CallEvent.SendVoipEvent]: (event: VoipEvent, call: MatrixCall) => void; }; // The key of the transceiver map (purpose + media type, separated by ':') @@ -459,7 +473,7 @@ export class MatrixCall extends TypedEventEmittererror), + this, ); } } @@ -1513,7 +1528,7 @@ export class MatrixCall extends TypedEventEmittererror)); + this.emit(CallEvent.Error, new CallError(code, message, error), this); throw error; } @@ -1987,7 +2002,7 @@ export class MatrixCall extends TypedEventEmittererror)); + this.emit(CallEvent.Error, new CallError(code, message, error), this); this.terminate(CallParty.Local, code, false); // no need to carry on & send the candidate queue, but we also @@ -2158,7 +2173,11 @@ export class MatrixCall extends TypedEventEmitter { logger.error(`Call ${this.callId} getLocalOfferFailed() running`, err); - this.emit(CallEvent.Error, new CallError(CallErrorCode.LocalOfferFailed, "Failed to get local offer!", err)); + this.emit( + CallEvent.Error, + new CallError(CallErrorCode.LocalOfferFailed, "Failed to get local offer!", err), + this, + ); this.terminate(CallParty.Local, CallErrorCode.LocalOfferFailed, false); }; @@ -2177,6 +2196,7 @@ export class MatrixCall extends TypedEventEmitter { - this.emit(CallEvent.LengthChanged, Math.round((Date.now() - this.callStartTime!) / 1000)); + this.emit(CallEvent.LengthChanged, Math.round((Date.now() - this.callStartTime!) / 1000), this); }, CALL_LENGTH_INTERVAL); } } else if (this.peerConn?.iceConnectionState == "failed") { @@ -2267,7 +2287,7 @@ export class MatrixCall extends TypedEventEmitter { - this.emit(CallEvent.DataChannel, ev.channel); + this.emit(CallEvent.DataChannel, ev.channel, this); }; /** @@ -2380,13 +2400,17 @@ export class MatrixCall extends TypedEventEmittererror)); + this.emit(CallEvent.Error, new CallError(code, message, error), this); this.hangup(code, false); return; From 0349411e6d31a1106de0a4216dc9db0d93788c90 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 24 Mar 2023 14:26:03 +0000 Subject: [PATCH 2/7] Rename group call error event So it doesn't clash with the call error event --- src/webrtc/groupCall.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/webrtc/groupCall.ts b/src/webrtc/groupCall.ts index 0504066aa85..b0c26acd5d5 100644 --- a/src/webrtc/groupCall.ts +++ b/src/webrtc/groupCall.ts @@ -40,6 +40,11 @@ export enum GroupCallTerminationReason { CallEnded = "call_ended", } +/** + * Because event names are just strings, they do need + * to be unique over all event types of event emitter. + * Some objects could emit more then one set of events. + */ export enum GroupCallEvent { GroupCallStateChanged = "group_call_state_changed", ActiveSpeakerChanged = "active_speaker_changed", @@ -49,7 +54,7 @@ export enum GroupCallEvent { LocalScreenshareStateChanged = "local_screenshare_state_changed", LocalMuteStateChanged = "local_mute_state_changed", ParticipantsChanged = "participants_changed", - Error = "error", + Error = "group_call_error", } export type GroupCallEventHandlerMap = { From 037cbdd2140e805d5d11eb44a4f60c44892590de Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 27 Mar 2023 14:33:58 +0100 Subject: [PATCH 3/7] Basic test for call replace --- spec/unit/webrtc/call.spec.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 646a2c7eec2..2cb8f572f90 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -1634,4 +1634,24 @@ describe("Call", function () { expect(call.hangup).not.toHaveBeenCalled(); }); }); + + describe("Call replace", () => { + it("Fires event when call replaced", async () => { + const onReplace = jest.fn(); + call.on(CallEvent.Replaced, onReplace); + + await call.placeVoiceCall(); + + const call2 = new MatrixCall({ + client: client.client, + roomId: FAKE_ROOM_ID, + }); + call2.on(CallEvent.Error, errorListener); + await fakeIncomingCall(client, call2); + + call.replacedBy(call2); + + expect(onReplace).toHaveBeenCalled(); + }); + }); }); From 5b5a3d8b5e042aeea4b66f9a4e3483c6f0a16aa4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 27 Mar 2023 15:44:56 +0100 Subject: [PATCH 4/7] Test failed call upgrade --- spec/unit/webrtc/call.spec.ts | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 2cb8f572f90..8a5eee30319 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -431,6 +431,33 @@ describe("Call", function () { expect(transceivers.get("m.usermedia:video")!.sender.track!.id).toBe("usermedia_video_track"); }); + it("should handle error on call upgrade", async () => { + const mockGetUserMediaStream = jest.fn().mockRejectedValue(new Error("Test error")); + client.client.getMediaHandler().getUserMediaStream = mockGetUserMediaStream; + + const onError = jest.fn(); + call.on(CallEvent.Error, onError); + + await startVoiceCall(client, call); + + await call.onAnswerReceived( + makeMockEvent("@test:foo", { + version: 1, + call_id: call.callId, + party_id: "party_id", + answer: { + sdp: DUMMY_SDP, + }, + [SDPStreamMetadataKey]: {}, + }), + ); + + // then unmute which should cause an upgrade + await call.setLocalVideoMuted(false); + + expect(onError).toHaveBeenCalled(); + }); + it("should unmute video after upgrading to video call", async () => { // Regression test for https://github.com/vector-im/element-call/issues/925 await startVoiceCall(client, call); From f9a222eceac5144be2634824854a0bc248918d01 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 27 Mar 2023 16:18:00 +0100 Subject: [PATCH 5/7] Mock out media getter after setting up the vocie call Otherwise we won't get far enough to test the right part --- spec/unit/webrtc/call.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index 8a5eee30319..e6c69ae823f 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -432,9 +432,6 @@ describe("Call", function () { }); it("should handle error on call upgrade", async () => { - const mockGetUserMediaStream = jest.fn().mockRejectedValue(new Error("Test error")); - client.client.getMediaHandler().getUserMediaStream = mockGetUserMediaStream; - const onError = jest.fn(); call.on(CallEvent.Error, onError); @@ -452,6 +449,9 @@ describe("Call", function () { }), ); + const mockGetUserMediaStream = jest.fn().mockRejectedValue(new Error("Test error")); + client.client.getMediaHandler().getUserMediaStream = mockGetUserMediaStream; + // then unmute which should cause an upgrade await call.setLocalVideoMuted(false); From 8b5098690655756d4a5ab39ddeb05119329ea70b Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 28 Mar 2023 09:30:17 +0100 Subject: [PATCH 6/7] Add test for incoming data channel --- spec/test-utils/webrtc.ts | 7 +++++++ spec/unit/webrtc/call.spec.ts | 11 +++++++++++ 2 files changed, 18 insertions(+) diff --git a/spec/test-utils/webrtc.ts b/spec/test-utils/webrtc.ts index ba93a6c1c97..5b65557e07c 100644 --- a/spec/test-utils/webrtc.ts +++ b/spec/test-utils/webrtc.ts @@ -122,6 +122,7 @@ export class MockRTCPeerConnection { public iceCandidateListener?: (e: RTCPeerConnectionIceEvent) => void; public iceConnectionStateChangeListener?: () => void; public onTrackListener?: (e: RTCTrackEvent) => void; + public onDataChannelListener?: (ev: RTCDataChannelEvent) => void; public needsNegotiation = false; public readyToNegotiate: Promise; private onReadyToNegotiate?: () => void; @@ -167,6 +168,8 @@ export class MockRTCPeerConnection { this.iceConnectionStateChangeListener = listener; } else if (type == "track") { this.onTrackListener = listener; + } else if (type == "datachannel") { + this.onDataChannelListener = listener; } } public createDataChannel(label: string, opts: RTCDataChannelInit) { @@ -231,6 +234,10 @@ export class MockRTCPeerConnection { this.negotiationNeededListener(); } } + + public triggerIncomingDataChannel(): void { + this.onDataChannelListener?.({ channel: {} } as RTCDataChannelEvent); + } } export class MockRTCRtpSender { diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index e6c69ae823f..db84ee54004 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -769,6 +769,17 @@ describe("Call", function () { expect(dataChannel.id).toBe(123); }); + it("should emit a data channel event when the other side adds a data channel", async () => { + await startVoiceCall(client, call); + + const dataChannelCallback = jest.fn(); + call.on(CallEvent.DataChannel, dataChannelCallback); + + (call.peerConn as unknown as MockRTCPeerConnection).triggerIncomingDataChannel(); + + expect(dataChannelCallback).toHaveBeenCalled(); + }); + describe("supportsMatrixCall", () => { it("should return true when the environment is right", () => { expect(supportsMatrixCall()).toBe(true); From 2ab3566f951a5962010518c18410143c80d50d9d Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 28 Mar 2023 13:47:22 +0100 Subject: [PATCH 7/7] Add comment on confusing exception handler I've failed to add a test for --- src/webrtc/call.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 85f4fb88503..b74afc0f7c0 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -2055,6 +2055,12 @@ export class MatrixCall extends TypedEventEmitter { this.makingOffer = true; try { + // XXX: in what situations do we believe gotLocalOffer actually throws? It appears + // to handle most of its exceptions itself and terminate the call. I'm not entirely + // sure it would ever throw, so I can't add a test for these lines. + // Also the tense is different between "gotLocalOffer" and "getLocalOfferFailed" so + // it's not entirely clear whether getLocalOfferFailed is just misnamed or whether + // they've been cross-polinated somehow at some point. await this.gotLocalOffer(); } catch (e) { this.getLocalOfferFailed(e as Error);