Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Electron ScreenSharing Audio Echo #2597

Open
DanielMcAssey opened this issue Nov 15, 2024 · 6 comments · May be fixed by #2605
Open

Electron ScreenSharing Audio Echo #2597

DanielMcAssey opened this issue Nov 15, 2024 · 6 comments · May be fixed by #2605

Comments

@DanielMcAssey
Copy link
Contributor

DanielMcAssey commented Nov 15, 2024

Description


Related to:

if (window.JitsiMeetScreenObtainer && window.JitsiMeetScreenObtainer.openDesktopPicker) {

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: electron/electron#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
Copy link
Member

saghul commented Nov 15, 2024

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
Copy link
Contributor Author

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

@saghul
Copy link
Member

saghul commented Nov 15, 2024

Excellent! Happy hacking!

@jaseemuddinn
Copy link

Hey @saghul, if it is still the case, I can send a PR for this issue; I would love to contribute.

@saghul
Copy link
Member

saghul commented Dec 5, 2024

Ping @DanielMcAssey ?

@DanielMcAssey
Copy link
Contributor Author

Sorry! Work got a little hectic. I'll open a PR tomorrow. Got a working version

@DanielMcAssey DanielMcAssey linked a pull request Dec 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants