Fix screenshare failing after several attempts #2771

dbkr · 2022-10-18T20:09:31Z

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

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

Fix screenshare failing after several attempts (#2771). Fixes Clicking the screen-sharing button rapidly and repeatedly can result in a stream getting stuck element-hq/element-call#625.

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

SimonBrandner

Generally, looks good

Could we get a test for the bug this is trying to fix? Also, it's not immediately clear how this fixes the bug but have been reviewing on an iPad...

src/webrtc/call.ts

spec/unit/webrtc/call.spec.ts

SimonBrandner · 2022-10-19T07:03:32Z

I.e. what exactly was the issue here?

SimonBrandner · 2022-10-19T07:06:56Z

We also ought to properly test this doesn't break anything since it's a large change

SimonBrandner · 2022-10-19T13:19:28Z

src/webrtc/call.ts

-    // 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<string, RTCRtpTransceiver>();


I think I would still prefer either string being replaced by a TransceiverType enum or an object like

{ [SDPStreamMetadata.Usermedia]: { audio: RTCRtpTransceiver, video: RTCRtpTransceiver, }, [SDPStreamMetatada.Screenshare]: { audio: RTCRtpTransceiver, video: RTCRtpTransceiver, }, }

Have made it a type alias, although just for a string since the 'kind' on track is intended to be other thing than just audio & video if necessary.

SimonBrandner

LGTM otherwise

@MrAnno

* Make calls go back to 'connecting' state when media lost ([\matrix-org#2880](matrix-org#2880)). * Add ability to send unthreaded receipt ([\matrix-org#2878](matrix-org#2878)). * Add way to abort search requests ([\matrix-org#2877](matrix-org#2877)). * sliding sync: add custom room subscriptions support ([\matrix-org#2834](matrix-org#2834)). * webrtc: add advanced audio settings ([\matrix-org#2434](matrix-org#2434)). Contributed by @MrAnno. * Add support for group calls using MSC3401 ([\matrix-org#2553](matrix-org#2553)). * Make the js-sdk conform to tsc --strict ([\matrix-org#2835](matrix-org#2835)). Fixes matrix-org#2112 matrix-org#2116 and matrix-org#2124. * Let leave requests outlive the window ([\matrix-org#2815](matrix-org#2815)). Fixes element-hq/element-call#639. * Add event and message capabilities to RoomWidgetClient ([\matrix-org#2797](matrix-org#2797)). * Misc fixes for group call widgets ([\matrix-org#2657](matrix-org#2657)). * Support nested Matrix clients via the widget API ([\matrix-org#2473](matrix-org#2473)). * Set max average bitrate on PTT calls ([\matrix-org#2499](matrix-org#2499)). Fixes element-hq/element-call#440. * Add config option for e2e group call signalling ([\matrix-org#2492](matrix-org#2492)). * Enable DTX on audio tracks in calls ([\matrix-org#2482](matrix-org#2482)). * Don't ignore call member events with a distant future expiration date ([\matrix-org#2466](matrix-org#2466)). * Expire call member state events after 1 hour ([\matrix-org#2446](matrix-org#2446)). * Emit unknown device errors for group call participants without e2e ([\matrix-org#2447](matrix-org#2447)). * Mute disconnected peers in PTT mode ([\matrix-org#2421](matrix-org#2421)). * Add support for sending encrypted to-device events with OLM ([\matrix-org#2322](matrix-org#2322)). Contributed by @robertlong. * Support for PTT group call mode ([\matrix-org#2338](matrix-org#2338)). * Fix registration add phone number not working ([\matrix-org#2876](matrix-org#2876)). Contributed by @bagvand. * Use an underride rule for Element Call notifications ([\matrix-org#2873](matrix-org#2873)). Fixes element-hq/element-web#23691. * Fixes unwanted highlight notifications with encrypted threads ([\matrix-org#2862](matrix-org#2862)). * Extra insurance that we don't mix events in the wrong timelines - v2 ([\matrix-org#2856](matrix-org#2856)). Contributed by @MadLittleMods. * Hide pending events in thread timelines ([\matrix-org#2843](matrix-org#2843)). Fixes element-hq/element-web#23684. * Fix pagination token tracking for mixed room timelines ([\matrix-org#2855](matrix-org#2855)). Fixes element-hq/element-web#23695. * Extra insurance that we don't mix events in the wrong timelines ([\matrix-org#2848](matrix-org#2848)). Contributed by @MadLittleMods. * Do not freeze state in `initialiseState()` ([\matrix-org#2846](matrix-org#2846)). * Don't remove our own member for a split second when entering a call ([\matrix-org#2844](matrix-org#2844)). * Resolve races between `initLocalCallFeed` and `leave` ([\matrix-org#2826](matrix-org#2826)). * Add throwOnFail to groupCall.setScreensharingEnabled ([\matrix-org#2787](matrix-org#2787)). * Fix connectivity regressions ([\matrix-org#2780](matrix-org#2780)). * Fix screenshare failing after several attempts ([\matrix-org#2771](matrix-org#2771)). Fixes element-hq/element-call#625. * Don't block muting/unmuting on network requests ([\matrix-org#2754](matrix-org#2754)). Fixes element-hq/element-call#592. * Fix ICE restarts ([\matrix-org#2702](matrix-org#2702)). * Target widget actions at a specific room ([\matrix-org#2670](matrix-org#2670)). * Add tests for ice candidate sending ([\matrix-org#2674](matrix-org#2674)). * Prevent exception when muting ([\matrix-org#2667](matrix-org#2667)). Fixes element-hq/element-call#578. * Fix race in creating calls ([\matrix-org#2662](matrix-org#2662)). * Add client.waitUntilRoomReadyForGroupCalls() ([\matrix-org#2641](matrix-org#2641)). * Wait for client to start syncing before making group calls ([\matrix-org#2632](matrix-org#2632)). Fixes matrix-org#2589. * Add GroupCallEventHandlerEvent.Room ([\matrix-org#2631](matrix-org#2631)). * Add missing events from reemitter to GroupCall ([\matrix-org#2527](matrix-org#2527)). Contributed by @toger5. * Prevent double mute status changed events ([\matrix-org#2502](matrix-org#2502)). * Don't mute the remote side immediately in PTT calls ([\matrix-org#2487](matrix-org#2487)). Fixes element-hq/element-call#425. * Fix some MatrixCall leaks and use a shared AudioContext ([\matrix-org#2484](matrix-org#2484)). Fixes element-hq/element-call#412. * Don't block muting on determining whether the device exists ([\matrix-org#2461](matrix-org#2461)). * Only clone streams on Safari ([\matrix-org#2450](matrix-org#2450)). Fixes element-hq/element-call#267. * Set PTT mode on call correctly ([\matrix-org#2445](matrix-org#2445)). Fixes element-hq/element-call#382. * Wait for mute event to send in PTT mode ([\matrix-org#2401](matrix-org#2401)). * Handle other members having no e2e keys ([\matrix-org#2383](matrix-org#2383)). Fixes element-hq/element-call#338. * Fix races when muting/unmuting ([\matrix-org#2370](matrix-org#2370)).

dbkr added the T-Defect label Oct 18, 2022

dbkr requested a review from a team as a code owner October 18, 2022 20:09

dbkr added 2 commits October 18, 2022 22:16

Fix tests

e864d2f

Unused import

cfc70b6

SimonBrandner reviewed Oct 19, 2022

View reviewed changes

src/webrtc/call.ts Outdated Show resolved Hide resolved

src/webrtc/call.ts Outdated Show resolved Hide resolved

src/webrtc/call.ts Show resolved Hide resolved

spec/unit/webrtc/call.spec.ts Outdated Show resolved Hide resolved

dbkr added 5 commits October 19, 2022 10:58

Use a map instead of an array

748bd8c

Add comment

a122849

more comment

d2b1bc1

Remove commented code

05a863a

Remove unintentional debugging

91cee49

SimonBrandner reviewed Oct 19, 2022

View reviewed changes

Add test for screenshare transceiver re-use

30739c7

SimonBrandner approved these changes Oct 19, 2022

View reviewed changes

Type alias for transceiver map

0e740d4

dbkr merged commit c57c897 into robertlong/group-call Oct 19, 2022

dbkr deleted the dbkr/reuse_screenshare_transceivers branch October 19, 2022 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix screenshare failing after several attempts #2771

Fix screenshare failing after several attempts #2771

dbkr commented Oct 18, 2022 •

edited by github-actions bot

Loading

SimonBrandner left a comment

SimonBrandner commented Oct 19, 2022

SimonBrandner commented Oct 19, 2022

SimonBrandner Oct 19, 2022

dbkr Oct 19, 2022

SimonBrandner left a comment

Fix screenshare failing after several attempts #2771

Fix screenshare failing after several attempts #2771

Conversation

dbkr commented Oct 18, 2022 • edited by github-actions bot Loading

Checklist

🐛 Bug Fixes

SimonBrandner left a comment

Choose a reason for hiding this comment

SimonBrandner commented Oct 19, 2022

SimonBrandner commented Oct 19, 2022

SimonBrandner Oct 19, 2022

Choose a reason for hiding this comment

dbkr Oct 19, 2022

Choose a reason for hiding this comment

SimonBrandner left a comment

Choose a reason for hiding this comment

dbkr commented Oct 18, 2022 •

edited by github-actions bot

Loading