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

desktopCapturer Electron Client Screenshare with Audio Causes Participants to Hear Echo #27337

Closed
info-ankit opened this issue Jan 16, 2021 · 63 comments
Labels

Comments

@info-ankit
Copy link

info-ankit commented Jan 16, 2021

I am using the desktop sharing feature of the Electron desktopCapturer on our demos. Some of our demos require to share audio of our application. When we share the desktop with audio on desktopCapturer electron client participants start to hear their own voice.

const constraints = {
  audio: {
    mandatory: {
      chromeMediaSource: 'desktop'
    }
  },
  video: {
    mandatory: {
      chromeMediaSource: 'desktop'
    }
  }
}

Current behavior:

When screen share enabled with audio participant sounds echoing back to themselves.

Expected Behavior:

Only system sounds should be broadcasted to the conference.

Expected Solution:

Behaviour should be converted as same as chrome web client.

Steps:

1- Using desktopCapturer Electron
2- Share screen with audio
3- Another participant starts to talk. (As a result, hears themself)

Environment:

Windows 10
Electron ^9.2.1
desktopCapturer Electron

@info-ankit info-ankit changed the title desktopCapturer Electron Client Screenshare with Audio Causes Participants to hear echo desktopCapturer Electron Client Screenshare with Audio Causes Participants to Hear Echo Jan 16, 2021
@asineth0
Copy link

Same issue here. Also trying to capture the audio for one application/window results in getting the entire system audio instead of the audio for that specific application.

@nornagon nornagon added the blocked/need-repro Needs a test case to reproduce the bug label Mar 30, 2021
@nornagon
Copy link
Member

Thanks for reporting this and helping to make Electron better!

Would it be possible for you to make a standalone testcase with only the code necessary to reproduce the issue? For example, Electron Fiddle is a great tool for making small test cases and makes it easy to publish your test case to a gist that Electron maintainers can use.

Stand-alone test cases make fixing issues go more smoothly: it ensure everyone's looking at the same issue, it removes all unnecessary variables from the equation, and it can also provide the basis for automated regression tests.

I'm adding the blocked/need-repro label for this reason. After you make a test case, please link to it in a followup comment.

Thanks in advance! Your help is appreciated.

@nornagon
Copy link
Member

Thanks for reporting this and helping to make Electron better!

Because of time constraints, triaging code with third-party dependencies is usually not feasible for a small team like Electron's.

Would it be possible for you to make a standalone testcase with only the code necessary to reproduce the issue? For example, Electron Fiddle is a great tool for making small test cases and makes it easy to publish your test case to a gist that Electron maintainers can use.

Stand-alone test cases make fixing issues go more smoothly: it ensure everyone's looking at the same issue, it removes all unnecessary variables from the equation, and it can also provide the basis for automated regression tests.

I'm adding the blocked/need-repro label for this reason. After you make a test case, please link to it in a followup comment.

Thanks in advance! Your help is appreciated.

@nornagon
Copy link
Member

nornagon commented Jul 6, 2021

I'm closing this as there's no minimal repro available. I'm happy to reopen if you're able to provide a more directed example that shows the issue.

@nornagon nornagon closed this as completed Jul 6, 2021
@Apak00
Copy link

Apak00 commented Jul 7, 2021

@nornagon
Hello there I created this repository https://github.com/Apak00/electron-ss-echo-problem starting from electron-quick-starter.
As it requires a peerconnection to be established, there is a signaling server involved but it should have nothing to do with echo problem because all it does is really trading the sdp between peers.
Also added reproduce steps, but if you have any problem with reproducing, I am more than happy to help.

I hope this helps you

@nornagon nornagon reopened this Jul 7, 2021
@nornagon nornagon added 13-x-y has-repro-repo Issue can be reproduced by cloning a git repo and removed blocked/need-repro Needs a test case to reproduce the bug labels Jul 7, 2021
@dx200010
Copy link

Here’re some findings after struggling with this issue for months.

  1. Using getDisplayMedia() in chrome doesn’t have echo issue, but it will have it too if echo cancellation was turned off.
  2. Debug log shows getUserMedia() have no audio processing applied, but getDisplayMedia() does.
  3. Audio processing was skipped since chrome want to create a direct-path and skipped it for screen capture device.
  4. getUserMedia() was caught in above by its device type, getDisplayMedia() have different device type and escaped
  5. Tried to made a mod to disable checking of screen catpure device type, it do works and echo was removed. But there're still lot of differences comparing to getDisplayMedia()

What I modified:
third_party/blink/renderer/modules/mediastream/user_media_processor.cc:CreateAudioSource()
Commented out blink::IsScreenCaptureMediaType(device.type)

@asineth0
Copy link

Seems to work, should make a PR.

@as0577

This comment has been minimized.

@Durgaprasad-Budhwani

This comment has been minimized.

@Durgaprasad-Budhwani
Copy link

@dx200010 If possible, can you please provide your code snippet or repo with working example.

@dx200010
Copy link

@dx200010 If possible, can you please provide your code snippet or repo with working example.

Firstly, please make sure you have downloaded full source codes of electron by following official build instruction.

Find the UserMediaProcessor::CreateAudioSource() in third_party/blink/renderer/modules/mediastream/user_media_processor.cc
and change below if-block

  if (blink::IsScreenCaptureMediaType(device.type) ||
      !blink::MediaStreamAudioProcessor::WouldModifyAudio(
          audio_processing_properties)) {

to

  if (//blink::IsScreenCaptureMediaType(device.type) ||
      !blink::MediaStreamAudioProcessor::WouldModifyAudio(
          audio_processing_properties)) {

Then build a modified electron from source, and use this modified electron to build your own application.

Now, you can enable echo cancellation by inserting the constraint like below.

navigator.mediaDevices.getUserMedia({
    audio : {
        mandatory: {
            echoCancellation: true,
            chromeMediaSource: 'desktop'
          }
    },
    video :
    {
        mandatory :
        {
            chromeMediaSource : 'desktop'
        }
    }
})	

@Apak00
Copy link

Apak00 commented Aug 4, 2021

@dx200010 Thanks a lot for the details, But unfortunately this did not work for me.

Could it be because of a certain electron version I need to checkout? the version I tried is : 12.0.1

@dx200010
Copy link

dx200010 commented Aug 5, 2021

@dx200010 Thanks a lot for the details, But unfortunately this did not work for me.

Could it be because of a certain electron version I need to checkout? the version I tried is : 12.0.1

Maybe...
I have only tried this on 13.1.4, don't know if it's version specific or not.

@Apak00
Copy link

Apak00 commented Aug 5, 2021

@dx200010 Have you had a chance to try your custom electron build on this repo?
https://github.com/Apak00/electron-ss-echo-problem

@Apak00
Copy link

Apak00 commented Aug 5, 2021

I point my package.json electron dependency to custom electron build like this
devDependencies: { ... electron: "file:../electron/src/electron" }

Even though I am able to build the app with this, could that be wrong?

What is the right way of adding custom electron build to a project?

@dx200010
Copy link

dx200010 commented Aug 6, 2021

@Apak00 Maybe your app was still linked to an unmodified electron.

Try launching your app directly from your own built electron for testing,
<Path to your electron source>\out\Release\electron.exe .

If it does works, then it's just packaging issue. Or, this workaround may just not always works.

@asineth0
Copy link

asineth0 commented Aug 6, 2021

Either way, would love to fix this upstream instead of having to build Electron which is quite time consuming for many.

@berkon
Copy link

berkon commented Jun 14, 2022

I've just built Electron by myself with the proposed patch and I can confirm that it works like a charm with the demo app from @Apak00! Thanks for your great effort to discover this @Apak00 !

Guys, it would really help so much if you could implement this in the officially build. I'd suggest to simply evaluate the (in this case unused) echoCancellation: true constraint.

@codebytere, I could try to build a pull request, if you want me to, but I guess you guys will be 100 times faster than me. I have no experience in contributing to the Electron code base.

@berkon
Copy link

berkon commented Jun 16, 2022

@Apak00, I guess you probably solved the issue with packaging your app with a custom Electron build meanwhile. I had the same problem. My impression is that electron-builder is not using the Electron binaries installed in node_modules folder. Instead it seems to just lookup the Electron version number in package.json, but then downloads that Electron version by itself from the server, thus making it impossible to use a private Electron build. I found a way to workaround this by using the win-unpacked folder, by replacing the Electron files by my self-built ones manually. Its not nice, but it works. So in case others ran into the same issue: https://stackoverflow.com/questions/72633870/how-to-use-self-built-electron-binaries-with-electron-builder If someone knows how to properly use a self-built Electron in a an app please let me know/post a better answer on Stackoverflow. ;-) Thanks!

@Apak00
Copy link

Apak00 commented Jun 16, 2022

I guess you are looking for 'electronDist' property on electron-builder configs, here;
https://www.electron.build/configuration/configuration.html

@asineth0
Copy link

asineth0 commented Jun 16, 2022

I just ended up writing an entire separate media engine for my app in native C++. Unfortunately, Electron almost never fixes or adds anything unless some major company needs it.

@bzmaxat
Copy link

bzmaxat commented Aug 1, 2022

@Apak00 @berkon I built my own electron with the suggested changes and ran my application from it. Although the stream.getAudioTracks()[0].getSettings() shows that echoCancellation: true, the participants of the stream still hear their own voice. Any thoughts on how to fix this? I am using Electron v13.6.9

@berkon
Copy link

berkon commented Aug 1, 2022

@bzmaxat, did you see Apak00's post regarding the electronDist attribute? In my case that worked. If it doesn't, then I'd guess that something went wrong with the compilation. To be honest, I wouldn't trust the content of the echoCancellation attribute in this specific case.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@github-actions github-actions bot added the stale label Oct 31, 2022
@bukharin
Copy link

Bump

@nornagon
Copy link
Member

FYI getDisplayMedia works in Electron now: #30702

@github-actions github-actions bot removed the stale label Nov 1, 2022
@berkon
Copy link

berkon commented Nov 2, 2022

@nornagon, I tried to use getDisplayMedia() with Electron 22.0.0-beta.1. But I'm getting Uncaught (in promise) DOMException: Not supported

@nornagon
Copy link
Member

nornagon commented Nov 2, 2022

@berkon please open a new issue with an example gist. Note that you'll have to install a handler in the main process using setDisplayMediaHandler.

@bukharin
Copy link

bukharin commented Nov 3, 2022

Support for getDisplayMedia apu does not related to this issue

@berkon
Copy link

berkon commented Nov 3, 2022

You were absolutely right @nornagon! I adapted @Apak00 's demo and it works fine now. As you said, it is important to add the handler in Main. So I guess this issue can be closed once Electron 22 is released.

@bukharin
Copy link

bukharin commented Nov 3, 2022

@berkon Could you provide an adapted demo?

@MustafaEminn
Copy link

Please could someone open pr for this issue? The issue opened 1 year ago but still not solved on Electron codebase.

@saghul
Copy link
Contributor

saghul commented Dec 2, 2022

Have you re-tested now what getDisplayMedia is available?

@MustafaEminn
Copy link

I did not test getDisplayMedia because has not good browser compatibility

@saghul
Copy link
Contributor

saghul commented Dec 2, 2022

That's absolutely not true. Also this is Electron we're talking about so even if you were right, that wouldn't apply here.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

@github-actions github-actions bot added the stale label Mar 3, 2023
@kycnexynpuwejl
Copy link

bump

@kycnexynpuwejl
Copy link

can anyone share custom electron build with updated(commented or removed) line blink::IsScreenCaptureMediaType(device.type)?
it's kind of magic for me to build custom electron with these depot_tool and gclient.

@github-actions github-actions bot removed the stale label Apr 3, 2023
@codebytere
Copy link
Member

This should be closed by #37315

@simo-an
Copy link

simo-an commented Aug 17, 2023

set enableLocalEcho to true at ses.setDisplayMediaRequestHandler(handler) will cancel all the voice ?

@ProdigyView
Copy link

bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests