jitsi / lib-jitsi-meet

A low-level JS video API that allows adding a completely custom video experience to web apps.
Apache License 2.0
1.34k stars 1.11k forks source link

Electron ScreenSharing Audio Echo #2597

Open DanielMcAssey opened 4 hours ago

DanielMcAssey commented 4 hours ago

Description


Related to: https://github.com/jitsi/lib-jitsi-meet/blob/bc446e999e4128031f265276e60f9a3808150760/modules/RTC/ScreenObtainer.js#L95

This is more of a discussion, but Electron now has support for getDisplayMedia (Bye bye programmatic screen selection 🥹 ), and it seems getUserMedia is no longer recommended for Screen Capture.

One downside of getUserMedia is that echo cancellation doesn't get applied (See: https://github.com/electron/electron/issues/27337), however its quite nice for programmatic selection of screen sharing, getDisplayMedia on Electron requires setting setDisplayMediaRequestHandler (See: https://www.electronjs.org/docs/latest/api/desktop-capturer), but it means we can make use of the new constraints syntax (min, max, ideal, exact).

However because of potential impact on 3rd party clients that use Screen Sharing, the way you interact and share your screen will differ (No programmatic request, as it requires user interaction).

So is it worth supporting getDisplayMedia in Electron, using some kind of flag?

Current behavior


Audio echo when presenting a screen with audio in Electron

Expected Behavior


No audio echo when presenting a screen with audio in Electron

Possible Solution


Use getDisplayMedia instead of getUserMedia

Steps to reproduce


Start a meeting in Electron, share desktop with audio, participants can hear themselves.

Environment details


Electron v31, latest lib-jitsi-meet, custom client.

saghul commented 4 hours ago

Hey there! Yes we need to be migrating to it.

Note that you still need to do the UI and use a session handler to indicate the user choice. I think Wayland + PipeWire is the exception but that's a different mess :-P

If you want to take a crack at it and send a PR we'd gladly review it!

I'd say let's add a config and keep both for a while, then remove the gUM path after a reasonable amount of time.

DanielMcAssey commented 4 hours ago

Great! I will have a think over the weekend and ill send a PR next week. Have a great weekend

saghul commented 3 hours ago

Excellent! Happy hacking!