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.

Sign up for GitHub

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

Jump to bottom

feat: session.setDisplayMediaRequestHandler #30702

Merged
merged 40 commits into from Aug 22, 2022
Merged

feat: session.setDisplayMediaRequestHandler #30702

merged 40 commits into from Aug 22, 2022

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Aug 25, 2021 •
edited

Description of Change

This allows Electron apps to support responding to
navigator.mediaDevices.getDisplayMedia. Closes #16513.

Checklist

Release Notes

Notes: Added support for navigator.mediaDevices.getDisplayMedia via a new session handler, ses.setDisplayMediaRequestHandler.

@nornagon nornagon added no-backport semver/minor backwards-compatible functionality labels Aug 25, 2021
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours api-review/requested 🗳 labels Aug 25, 2021
MarshallOfSound
MarshallOfSound reviewed Aug 25, 2021
View reviewed changes
docs/api/session.md Outdated Show resolved Hide resolved
docs/api/session.md Outdated Show resolved Hide resolved
docs/api/session.md Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/media/media_stream_devices_controller.cc Outdated Show resolved Hide resolved
zcbenz
zcbenz approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API looks good to me.

shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/electron_browser_context.cc Outdated Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 1, 2021
@nornagon
Copy link
Member Author

nornagon commented Sep 2, 2021 •
edited

I have two vague concerns about this API.

  1. I think it might be possible to break (CHECK? UAF? Type confusion?) things by returning the wrong type of media streams here. If so we should add checks/throw/etc. to make it more dev-friendly.
  2. It's unclear to me how difficult it would be to build a "chooser"-type UI using this API, mimicking what Chrome shows. Is this API too intricate/powerful? Could we expose something simpler, perhaps technically a little less powerful but covering 99% of use cases in a much simpler-to-understand and less error-prone way?

For (2) here, by "intricate", I mean things like: how should an app differently handle deviceAccess, deviceUpdate and generateStream requests? What about openDevicePepperOnly? Should it be possible to return the video stream for a different tab for requests for displayVideoCaptureThisTab? etc.

Also, should this same API be responsible for handling navigator.mediaDevices.getUserMedia as well? If so, how is the app supposed to enumerate video/microphone devices?

@nornagon nornagon marked this pull request as draft September 3, 2021 17:14
@nornagon
Copy link
Member Author

nornagon commented Sep 3, 2021

Converting to draft because I think this needs some thought before it's ready to be merged.

@zcbenz zcbenz added the wip ⚒ label Sep 21, 2021
@nornagon nornagon mentioned this pull request Jan 26, 2022
Closed
3 tasks
dsanders11
dsanders11 reviewed Mar 3, 2022
View reviewed changes
docs/api/session.md Outdated
* `request` Object
* `frame` [WebFrameMain](web-frame-main.md) - frame that is requesting access to media
* `type` String - Type of request. Can be 'deviceAccess', 'deviceUpdate', 'generateStream' or 'openDevicePepperOnly'.
* `audioType` String - can be 'deviceAudioCapture', 'displayAudioCapture' or 'noService'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be 'device', 'display', and null? The 'AudioCapture' is redundant since the key is 'audioType'.

@nornagon nornagon mentioned this pull request Mar 3, 2022
Closed
@nornagon
Copy link
Member Author

nornagon commented Mar 3, 2022

NB let's make sure we support applyConstraints and investigate adding cursor constraint support to Chrome per #30231 (comment)

@nornagon @MarshallOfSound
Update docs/api/session.md
8c8a464 
Co-authored-by: Samuel Attard <sam@electronjs.org>
@nornagon nornagon merged commit 221bb51 into main Aug 22, 2022
@nornagon nornagon deleted the media-request-handler branch August 22, 2022 21:15
@release-clerk
Copy link

release-clerk bot commented Aug 22, 2022

Release Notes Persisted

Added support for navigator.mediaDevices.getDisplayMedia via a new session handler, ses.setDisplayMediaRequestHandler.

@saghul saghul mentioned this pull request Aug 23, 2022
Open
@miniak miniak mentioned this pull request Aug 31, 2022
Merged
3 tasks
@nornagon nornagon mentioned this pull request Oct 31, 2022
Closed
@commit-lint commit-lint bot mentioned this pull request Nov 14, 2022
Open
@jeanfbrito
Copy link

Hello!
Any idea of what version will have this released?

@nornagon
Copy link
Member Author

this is in v22.

@dtzxporter
Copy link

Does this support constraints like frameRate, width, height yet?

@nornagon
Copy link
Member Author

@dtzxporter are you having trouble with those?

@dtzxporter
Copy link

dtzxporter commented Nov 29, 2022 •
edited

@nornagon I had just taken a look at the pull, in the tests it seemed to just supply true/false, I am mainly curious if passing constraints directly to getDisplayMedia are supported or if we have to use applyConstraints right now, (Which, is horribly broken in chrome/chromium)

@nornagon
Copy link
Member Author

@dtzxporter there's nothing in this PR that specifically supports or doesn't support those constraints; they should work the same as they do in chrome I believe.

@theogravity theogravity mentioned this pull request Feb 16, 2023
Closed
3 tasks
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@nornagon @khalwa
feat: session.setDisplayMediaRequestHandler (electron#30702)
1362e92
@theogravity theogravity mentioned this pull request Feb 27, 2023
Merged
5 tasks
@stefansundin
Copy link

@nornagon Hello. Sorry for the ping but I have two questions:

  1. What does loopbackWithMute mute exactly? I haven't been able to find information explaining what makes it different from just loopback.
  2. Is callback(undefined) the recommended way to reject the getDisplayMedia promise? Would be cool if there were both a resolve and reject callback, but maybe that's not very common in electron APIs.

Thanks!

@mattxie02
Copy link

@nornagon Does this include cursor constraints per #30702 (comment)?

SimonBrandner added a commit to element-hq/element-desktop that referenced this pull request Jul 14, 2023
@SimonBrandner
Switch to the new session API for screen-sharing
a21e5af 
See electron/electron#30702 - this has the benefit of the js-sdk and LiveKit not having to add custom logic for Electron

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
SimonBrandner added a commit to matrix-org/matrix-react-sdk that referenced this pull request Jul 14, 2023
@SimonBrandner
Switch to the new session API for screen-sharing
a6d5a17 
See electron/electron#30702 - this has the benefit of the js-sdk and LiveKit not having to add custom logic for Electron

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
SimonBrandner added a commit to element-hq/element-web that referenced this pull request Jul 14, 2023
@SimonBrandner
Switch to the new session API for screen-sharing
d9a4149 
See electron/electron#30702 - this has the benefit of the js-sdk and LiveKit not having to add custom logic for Electron

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
This was referenced Jul 14, 2023
Merged
Merged
Merged
github-merge-queue bot pushed a commit to matrix-org/matrix-react-sdk that referenced this pull request Jul 14, 2023
@SimonBrandner
Switch to the new session API for screen-sharing (#11266)
a6d4ce4 
See electron/electron#30702 - this has the benefit of the js-sdk and LiveKit not having to add custom logic for Electron

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
github-merge-queue bot pushed a commit to matrix-org/matrix-react-sdk that referenced this pull request Jul 14, 2023
@SimonBrandner
Switch to the new session API for screen-sharing (#11266)
a4b2365 
See electron/electron#30702 - this has the benefit of the js-sdk and LiveKit not having to add custom logic for Electron

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
github-merge-queue bot pushed a commit to matrix-org/matrix-react-sdk that referenced this pull request Jul 14, 2023
@SimonBrandner
Switch to the new session API for screen-sharing (#11266)
cb03aac 
See electron/electron#30702 - this has the benefit of the js-sdk and LiveKit not having to add custom logic for Electron

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theogravity theogravity theogravity left review comments

@dsanders11 dsanders11 dsanders11 left review comments

@MarshallOfSound MarshallOfSound MarshallOfSound left review comments

@jkleinsc jkleinsc jkleinsc approved these changes

@zcbenz zcbenz zcbenz approved these changes

Assignees
No one assigned
Labels
api-review/approved ✅ no-backport semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

getDisplayMedia with Chrome 72 throwing Not Allowed