feat: add desktopCapturer.getMediaSourceIdForWebContents() to get stream source id from web contents #22701

Volune · 2020-03-16T08:17:58Z

Description of Change

New feature: add desktopCapturer.getWebContentsStream() to provide a way to get a MediaStream from a WebContents using getUserMedia.

An example code is provided in docs/api/desktop-capturer.md

This should be enough to resolve #4776

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added desktopCapturer.getMediaSourceIdForWebContents(), can be used with getUserMedia to get a stream for a WebContent.

Comments

Feedbacks on the API (especially on functions and arguments naming) are welcomed

These two tests failed locally. Since they don't seem related with the code changes, I assume it's caused by the test environment.

not ok 746 Notification module (dbus) Notification module using org.freedesktop.Notifications "before all" hook
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/mnt/c/dev/github.volune/electron-gn/src/electron/spec-main/api-notification-dbus-spec.ts)
      at listOnTimeout (internal/timers.js:549:17)
      at processTimers (internal/timers.js:492:7)


not ok 324 BrowserWindow module "webPreferences" option "sandbox" option exposes "exit" event to preload script
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/mnt/c/dev/github.volune/electron-gn/src/electron/spec-main/api-browser-window-spec.ts)
      at listOnTimeout (internal/timers.js:549:17)
      at processTimers (internal/timers.js:492:7)

welcome · 2020-03-16T08:18:00Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

docs/api/desktop-capturer.md

zcbenz · 2020-03-19T01:28:57Z

shell/browser/media/media_stream_devices_controller.cc

+    }
+  }
+
+  VLOG(0) << "DEVICES " << devices.size();


This can be removed.

Removed that whole file change, it was actually an experiment to use getDisplayMedia and should not have been committed.

nornagon · 2020-03-19T16:34:55Z

cc @electron/wg-api

docs/api/structures/web-contents-stream-result.md

loc · 2020-03-19T19:36:51Z

docs/api/desktop-capturer.md

@@ -94,6 +138,13 @@ Returns `Promise<DesktopCapturerSource[]>` - Resolves with an array of [`Desktop
 **Note** Capturing the screen contents requires user consent on macOS 10.15 Catalina or higher,
 which can detected by [`systemPreferences.getMediaAccessStatus`].

+### `desktopCapturer.getWebContentsStream(options)`


Maybe getWebContentsStreamInfo? It's not really a stream yet.

Right

At some point I was considering returning a DesktopCapturerSource, but:

the description of DesktopCapturerSource.id would not match this case (not "screen:XX" format, that's the "mediaId")

don't know if it should return the "mediaId" in the display_id field or not, since that's different format

name, thumbnail, appIcon are not provided

At this point, the returned id is expected to be used, otherwise it will leak inside DesktopStreamsRegistry. This behavior is not similar to screen/application sources.

I don't know if getWebContentsSource would be a good name or more confusing.

loc · 2020-03-26T16:41:52Z

@Volune what do you think about moving this to be on WebContents? I think contents.getCaptureMediaId() or something like that would provide better ergonomics. Otherwise I think this is looking pretty good.

jpmcgrath · 2020-04-25T06:30:52Z

@MarshallOfSound just wondering if this is close to getting approved/merged? I'd love to be able to try of this feature. It would be great to know If this is just around the corner as I could abort my n00bish attempt to build electron from this branch :-)

Apologies if this question sounds demanding/entitled, or is not in keeping with electron community expectations/conventions. Thanks everyone for your efforts with this great framework!

Volune · 2020-04-25T07:02:41Z

@Volune what do you think about moving this to be on WebContents? I think contents.getCaptureMediaId() or something like that would provide better ergonomics. Otherwise I think this is looking pretty good.

@loc The issue I have with that is I need to know the requesting WebContents in order to call content::DesktopStreamsRegistry::GetInstance()->RegisterStream. I don't know how to do that from WebContents. Also a benefit of desktopCapturer is it's available in rendering thread, so there always is a "request_web_contents"
One solution could be to split into WebContents.getMediaId() and deskopCapturer.registerWebContentsStream()
Maybe another solution would be to patch Blink to stop verifying that the stream has been registered, which could make sense for Electron.

(I feel like I already replied that, I guess I forgot to submit my response ☹️ )

just wondering if this is close to getting approved/merged?

The API is not stabilized, so probably not. I will try to update the PR (rebase on top of master) during next week.

gamerz1990 · 2020-05-17T17:38:40Z

I am in need for this feature hope it will 🙏 done soon 😊

loc · 2020-05-19T01:06:45Z

Sorry for dropping the ball on this @Volune. I spent some time today thinking about how we could make this better, but couldn't come up with anything that didn't involve significant patching. In the future, I'll try to find an opportunity to improve Chrome's deference to embedders on the issue of screen capture.

One last minor suggestion. I've gone back and forth, but I think I'd prefer to change the signature from:

desktopCapturer.getWebContentsStream({ webContentsId: number }): Promise<{ id: string, mediaId: string}>

to

desktopCapturer.getMediaSourceIdForWebContents(webContentsId: number): Promise<string>

The name is more clear about what it's returning and, though it's reasonable to want to use options objects here to make this more extensible, I think that is pretty unlikely and comes at the cost of a stranger API. Also, since the mediaId is not useful to the consumer, we can safely get rid of it.

Also, we should add a note in the docs that this media source ID is only valid for 10 seconds after it's called.

WDYT?

…eam source id from web contents

Volune · 2020-05-20T03:06:41Z

@loc ok for me

I rebased on top of master, and made the API changes. Tests passed on my Windows, and a small demo app worked too.

loc · 2020-05-20T22:56:13Z

docs/api/structures/web-contents-stream-result.md

@@ -0,0 +1,6 @@
+# WebContentsStreamResult Object


We can get rid of this object now.

loc · 2020-05-20T22:57:36Z

docs/api/desktop-capturer.md

+* `webContentsId` number - Id of the WebContents to get stream of
+
+Returns `Promise<string>` - Resolves with the identifier of a WebContents stream, this identifier can be
+    used with [`navigator.mediaDevices.getUserMedia`].


Can this extra indent be removed?

loc · 2020-05-20T23:00:19Z

lib/renderer/api/desktop-capturer.ts

@@ -37,3 +37,7 @@ export async function getSources (options: Electron.SourcesOptions) {

  return deserialize(sources);
 }
+
+export function getMediaSourceIdForWebContents (webContentsId: number) {
+  return ipcRendererInternal.invoke<Electron.WebContentsStreamResult>('ELECTRON_BROWSER_DESKTOP_CAPTURER_GET_MEDIA_SOURCE_ID_FOR_WEB_CONTENTS', webContentsId, getCurrentStack());


Suggested change

return ipcRendererInternal.invoke<Electron.WebContentsStreamResult>('ELECTRON_BROWSER_DESKTOP_CAPTURER_GET_MEDIA_SOURCE_ID_FOR_WEB_CONTENTS', webContentsId, getCurrentStack());

return ipcRendererInternal.invoke<string>('ELECTRON_BROWSER_DESKTOP_CAPTURER_GET_MEDIA_SOURCE_ID_FOR_WEB_CONTENTS', webContentsId, getCurrentStack());

Volune · 2020-05-21T07:19:49Z

updated PR (sorry should have better reviewed last changes)
CI test issue on mac seems unrelated to this PR (but I can't test on mac)
Do I need to do something about release notes?

jkleinsc · 2020-05-21T15:50:07Z

@Volune the failing mac tests were flakes that disappeared on a re-run. In regards to the release notes, I updated your PR description to have a release note that matches the feature. The release-notes status being marked as pending is an issue unrelated to this PR.

welcome · 2020-05-26T20:34:27Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2020-05-26T20:34:28Z

Release Notes Persisted

Added desktopCapturer.getMediaSourceIdForWebContents(), can be used with getUserMedia to get a stream for a WebContent.

…eam source id from web contents (#22701) * feat: add desktopCapturer.getMediaSourceIdForWebContents() to get stream source id from web contents * Cleanup from #22701 PR comments

yozzh · 2020-06-09T09:31:48Z

Hello, @Volune. I have a question about this feature. Is it possible to add alpha channel (transparency) when capturing tab? It will be very useful for third party widgets (ex. stickers)

… get stream source id from web contents (#22701)" This reverts commit 204f001.

electron-cation bot added new-pr 🌱 PR opened in the last 24 hours and removed new-pr 🌱 PR opened in the last 24 hours labels Mar 16, 2020

zcbenz approved these changes Mar 19, 2020

View reviewed changes

nornagon added the api-review/requested 🗳 label Mar 19, 2020

loc reviewed Mar 19, 2020

View reviewed changes

MarshallOfSound self-requested a review March 23, 2020 23:34

Volune force-pushed the record-web-contents branch from fc60001 to b70cf58 Compare March 26, 2020 08:11

feat: add desktopCapturer.getMediaSourceIdForWebContents() to get str…

b9cfaba

…eam source id from web contents

Volune force-pushed the record-web-contents branch from b70cf58 to b9cfaba Compare May 20, 2020 03:03

loc reviewed May 20, 2020

View reviewed changes

Cleanup from electron#22701 PR comments

ac5e56a

Volune changed the title ~~feat: add desktopCapturer.getWebContentsStream() to get media stream id from web contents~~ feat: add desktopCapturer.getMediaSourceIdForWebContents() to get stream source id from web contents May 21, 2020

jkleinsc requested a review from loc May 21, 2020 14:45

loc approved these changes May 26, 2020

View reviewed changes

jkleinsc added api-review/approved ✅ and removed api-review/requested 🗳 labels May 26, 2020

jkleinsc merged commit 204f001 into electron:master May 26, 2020

nornagon added a commit that referenced this pull request Sep 10, 2020

Revert "feat: add desktopCapturer.getMediaSourceIdForWebContents() to…

3d8ec37

… get stream source id from web contents (#22701)" This reverts commit 204f001.

nornagon mentioned this pull request Sep 10, 2020

feat: remove getMediaSourceIdForWebContents() #25414

Merged

6 tasks

nornagon added a commit that referenced this pull request Sep 11, 2020

Revert "feat: add desktopCapturer.getMediaSourceIdForWebContents() to…

58413e9

… get stream source id from web contents (#22701)" This reverts commit 204f001.

mitchemmc mentioned this pull request Sep 30, 2021

feat: add webContents.getMediaSourceId() method #31204

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add desktopCapturer.getMediaSourceIdForWebContents() to get stream source id from web contents #22701

feat: add desktopCapturer.getMediaSourceIdForWebContents() to get stream source id from web contents #22701

Volune commented Mar 16, 2020 •

edited by jkleinsc

Loading

welcome bot commented Mar 16, 2020

zcbenz Mar 19, 2020

Volune Mar 19, 2020

nornagon commented Mar 19, 2020

loc Mar 19, 2020

Volune Mar 20, 2020

loc commented Mar 26, 2020

jpmcgrath commented Apr 25, 2020

Volune commented Apr 25, 2020

gamerz1990 commented May 17, 2020

loc commented May 19, 2020

Volune commented May 20, 2020

loc May 20, 2020

loc May 20, 2020

loc May 20, 2020

Volune commented May 21, 2020

jkleinsc commented May 21, 2020

welcome bot commented May 26, 2020

release-clerk bot commented May 26, 2020

yozzh commented Jun 9, 2020

	return ipcRendererInternal.invoke<Electron.WebContentsStreamResult>('ELECTRON_BROWSER_DESKTOP_CAPTURER_GET_MEDIA_SOURCE_ID_FOR_WEB_CONTENTS', webContentsId, getCurrentStack());
	return ipcRendererInternal.invoke<string>('ELECTRON_BROWSER_DESKTOP_CAPTURER_GET_MEDIA_SOURCE_ID_FOR_WEB_CONTENTS', webContentsId, getCurrentStack());

feat: add desktopCapturer.getMediaSourceIdForWebContents() to get stream source id from web contents #22701

feat: add desktopCapturer.getMediaSourceIdForWebContents() to get stream source id from web contents #22701

Conversation

Volune commented Mar 16, 2020 • edited by jkleinsc Loading

Description of Change

Checklist

Release Notes

Comments

welcome bot commented Mar 16, 2020

zcbenz Mar 19, 2020

Choose a reason for hiding this comment

Volune Mar 19, 2020

Choose a reason for hiding this comment

nornagon commented Mar 19, 2020

loc Mar 19, 2020

Choose a reason for hiding this comment

Volune Mar 20, 2020

Choose a reason for hiding this comment

loc commented Mar 26, 2020

jpmcgrath commented Apr 25, 2020

Volune commented Apr 25, 2020

gamerz1990 commented May 17, 2020

loc commented May 19, 2020

Volune commented May 20, 2020

loc May 20, 2020

Choose a reason for hiding this comment

loc May 20, 2020

Choose a reason for hiding this comment

loc May 20, 2020

Choose a reason for hiding this comment

Volune commented May 21, 2020

jkleinsc commented May 21, 2020

welcome bot commented May 26, 2020

release-clerk bot commented May 26, 2020

yozzh commented Jun 9, 2020

Volune commented Mar 16, 2020 •

edited by jkleinsc

Loading