Skip to content

Commit

Permalink
Fix screenshare failing after several attempts (#2771)
Browse files Browse the repository at this point in the history 
* 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 element-hq/element-call#625

* Fix tests

* Unused import

* Use a map instead of an array

* Add comment

* more comment

* Remove commented code

* Remove unintentional debugging

* Add test for screenshare transceiver re-use

* Type alias for transceiver map
  • Loading branch information
@dbkr
dbkr authored Oct 19, 2022
1 parent dfe535b commit c57c897
Show file tree
Hide file tree
Showing 4 changed files with 235 additions and 108 deletions.
44 changes: 35 additions & 9 deletions spec/test-utils/webrtc.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,12 @@ 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<void>;
private onReadyToNegotiate: () => void;
localDescription: RTCSessionDescription;
signalingState: RTCSignalingState = "stable";
public senders: MockRTCRtpSender[] = [];
public transceivers: MockRTCRtpTransceiver[] = [];

public static triggerAllNegotiations(): void {
for (const inst of this.instances) {
Expand Down Expand Up @@ -169,22 +169,32 @@ 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(this);
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() {
this.needsNegotiation = true;
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) {
Expand All @@ -198,7 +208,23 @@ 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 {
constructor(private peerConn: MockRTCPeerConnection) {}

public sender: RTCRtpSender;
public receiver: RTCRtpReceiver;

public set direction(_: string) {
this.peerConn.needsNegotiation = true;
}

setCodecPreferences = jest.fn<void, RTCRtpCodecCapability[]>();
}

export class MockMediaStreamTrack {
Expand Down
122 changes: 83 additions & 39 deletions spec/unit/webrtc/call.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -370,17 +369,15 @@ describe('Call', function() {
).typed(),
);

const usermediaSenders: Array<RTCRtpSender> = (call as any).usermediaSenders;
// XXX: Lots of inspecting the prvate state of the call object here
const transceivers: Map<string, RTCRtpTransceiver> = (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");
// 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 () => {
Expand All @@ -400,16 +397,13 @@ describe('Call', function() {
// setLocalVideoMuted probably?
await (call as any).upgradeCall(false, true);

const usermediaSenders: Array<RTCRtpSender> = (call as any).usermediaSenders;
// XXX: More inspecting private state of the call object
const transceivers: Map<string, RTCRtpTransceiver> = (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.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 () => {
Expand Down Expand Up @@ -479,6 +473,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" });

Expand Down Expand Up @@ -1057,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<void>(waitNegotiateFunc);

MockRTCPeerConnection.triggerAllNegotiations();
await sendNegotiatePromise;

await call.onAnswerReceived(makeMockEvent("@test:foo", {
"version": 1,
"call_id": call.callId,
Expand Down Expand Up @@ -1090,12 +1116,7 @@ describe('Call', function() {
).toHaveLength(1);

mockSendEvent.mockReset();
const sendNegotiatePromise = new Promise<void>(resolve => {
mockSendEvent.mockImplementationOnce(() => {
resolve();
return Promise.resolve({ event_id: "foo" });
});
});
const sendNegotiatePromise = new Promise<void>(waitNegotiateFunc);

MockRTCPeerConnection.triggerAllNegotiations();
await sendNegotiatePromise;
Expand Down Expand Up @@ -1130,29 +1151,52 @@ describe('Call', function() {
headerExtensions: [],
});

const prom = new Promise<void>(resolve => {
const mockPeerConn = call.peerConn as unknown as MockRTCPeerConnection;
mockPeerConn.addTrack = 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();
},
}]);
mockSendEvent.mockReset();
const sendNegotiatePromise = new Promise<void>(waitNegotiateFunc);

return mockSender;
});
});
await call.setScreensharingEnabled(true);
MockRTCPeerConnection.triggerAllNegotiations();

await sendNegotiatePromise;

const mockPeerConn = call.peerConn as unknown as MockRTCPeerConnection;
expect(
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<void>(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<void>(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<void>(waitNegotiateFunc);
await call.setScreensharingEnabled(true);
MockRTCPeerConnection.triggerAllNegotiations();
await screenshareOnProm2;

await prom;
// should still be two, ie. another one should not have been created
// when re-enabling the screen share.
expect(mockPeerConn.transceivers.length).toEqual(2);
});
});

Expand Down
Loading

0 comments on commit c57c897

Please sign in to comment.