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

feat: Implement electron.DesktopCapturer.SetSkipCursor #25103

Closed
wants to merge 169 commits into from

Conversation

CapOM
Copy link
Contributor

@CapOM CapOM commented Aug 24, 2020

Description of Change

Checklist

Release Notes

Notes: Can disable or re-enable cursor capturer while the stream is running

electron-bot and others added 30 commits May 21, 2020 13:33
Co-authored-by: Cheng Zhao <zcbenz@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: Cheng Zhao <zcbenz@gmail.com>
Co-authored-by: John Kleinschmidt <jkleinsc@github.com>
…lectron#23771)

When application is activated thru macOS app switcher (cmd+tab) the
App's activate event is note emitted. The reason is that
`applicationShouldHandleReopen:hasVisibleWindows:` is sent only when app
is activated via Dock. Using `applicationDidBecomeActive:` is handling
all cases properly.

Co-authored-by: Lukas Weber <luweber@microsoft.com>
Co-authored-by: Cheng Zhao <zcbenz@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: Aleksei Kuzmin <alkuzmin@microsoft.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* refactor: use typeutils for nativeImage serialization (electron#23693)

* fix: ensure nativeImage serialization main->renderer
The devtools profiler is not attached at the point we run out init scripts (or our apps preload scripts), we do not really want to change when we run these init scripts but for when a dev is doing performance work it makes sense to give them an option to make the devtools profiler actually work on both our init scripts and their preload script.  This PR adds that logic behind an environment variable ELECTRON_PROFILE_INIT_SCRIPTS.

Co-authored-by: Samuel Attard <samuel.r.attard@gmail.com>
Co-authored-by: Cheng Zhao <zcbenz@gmail.com>
…d preloads (electron#23893)

Co-authored-by: Samuel Attard <samuel.r.attard@gmail.com>
…enabled (electron#23896)

* fix: add patch to prevent crash during frame swap with ctx isolation enabled

* Update .patches

* chore: update patches

Co-authored-by: Samuel Attard <samuel.r.attard@gmail.com>
Co-authored-by: Samuel Attard <sattard@slack-corp.com>
* docs: move protocol-ns to protocol.md

* chore: fix up tests and implement missing pieces required for tests

Co-authored-by: Samuel Attard <samuel.r.attard@gmail.com>
Co-authored-by: Samuel Attard <sattard@slack-corp.com>
…lectron#23899)

This regressed once again in Electron 8 due to Chromium changes.

Test Plan:

- Confirm that test case from electron#15898 (comment) now works

Notes: Fix disabling color correct rendering with `--disable-color-correct-rendering`

Co-authored-by: Biru Mohanathas <birunthan@mohanathas.com>
Useful to dynamically stop cursor capture in the stream.
By default the cursor is captured.
First argument is a deviceId so one can stop capturing
the cursor for a specific streamed source.

electron#23923

Notes: Can enable or disable capturing cursor while the stream is
running
@CapOM CapOM requested review from a team as code owners August 24, 2020 06:58
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 24, 2020
@CapOM CapOM changed the base branch from master to 10-x-y August 24, 2020 06:59
@CapOM CapOM changed the title Set skip cursor feat: Implement electron.DesktopCapturer.SetSkipCursor Aug 24, 2020
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Please target master instead of 10-x-y. We do not add features directly to release branches but we will backport them.

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

These are fairly chonky patches, can you attempt to upstream them so we don't have to take that burden?

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 25, 2020
@CapOM
Copy link
Contributor Author

CapOM commented Sep 12, 2020

These are fairly chonky patches, can you attempt to upstream them so we don't have to take that burden?

It would not be possible to upstream it in the current state. I think the only way would be to start implementing MediaTrackSupportedConstraints.cursor (values always, never, motion) , see the w3c spec https://www.w3.org/TR/screen-capture/#cursorcaptureconstraint . But that would be to support getDisplayMedia which is not currently supported by electron: #16513 (comment)
So that would reduce this PR but not sure how much.

There are some technical details here but essentially skipping the cursor on a particular source would affect all calls to getUserMedia or getDisplayMedia on that particular source (i.e. a particular app window A)

This is because the compositing of the cursor and the frame is done in the webrtc capturer. Some interfaces would need to be changed in chromium/media first to allow passing a frame and the cursor as a meta overlay. So that consumers of the capturer could decide or not to composite the frame with the cursor independently of each others for the same source.

That said currently this PR allows to show and hide the cursor on the fly. And skipping the cursor on a source app window A does not affect capturing the cursor on another source B. It only affects if there are multiple captures of the same source app window A at the same time.
As mentioned above, it would be very difficult to avoid that corner case when implementing MediaTrackSupportedConstraints.cursor. Also the use case were we would want to skip and show the cursor on app window A , should be rare.

Note that even if this PR touches lot of files but only a few lines per files and the changes are quite redundant.

@pavlobu
Copy link

pavlobu commented Oct 7, 2020

Great work!
really waiting for it to appear in new version

@@ -97,6 +97,13 @@ which can detected by [`systemPreferences.getMediaAccessStatus`].
[`navigator.mediaDevices.getUserMedia`]: https://developer.mozilla.org/en/docs/Web/API/MediaDevices/getUserMedia
[`systemPreferences.getMediaAccessStatus`]: system-preferences.md#systempreferencesgetmediaaccessstatusmediatype-macos

### `desktopCapturer.SetSkipCursor(sourceId, skip)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### `desktopCapturer.SetSkipCursor(sourceId, skip)`
### `desktopCapturer.setSkipCursor(sourceId, skip)`

@miniak miniak changed the base branch from 10-x-y to master October 13, 2020 23:03
@miniak miniak requested a review from nornagon as a code owner October 13, 2020 23:03
@apodolny
Copy link

Would love to see this merged — @nornagon @miniak let me know if there's anything I can do to help.

@nornagon
Copy link
Member

Looks like this PR somehow got messed up. I can't review it as is.

@apodolny
Copy link

apodolny commented Nov 2, 2020

@miniak do you have time to resolve the issues here?

@ns-jisorce
Copy link

Hi, I will try to rebase my PR against master later this year

@apodolny
Copy link

@CapOM, are you able to rebase against master? Let me know if there's anything I can do to help.

@adityak368
Copy link

Any update on this? Let me know if I can help

@CapOM
Copy link
Contributor Author

CapOM commented Feb 18, 2021

Yes please go ahead with the rebase if you have some time, I do not have the bandwidth for now. Thanks

@zcbenz zcbenz added the wip ⚒ label Feb 24, 2021
@tristaaan
Copy link

I tried rebasing on master and I had a rather difficult time with it and ended up cherry-picking the commit.
https://github.com/tristaaan/electron/tree/set_skip_cursor_rebased

@apodolny
Copy link

Seems like something has happened to this PR again :(

@nornagon
Copy link
Member

I'm closing this PR as it's unreviewable in its current state, please open a new PR if you're interested in continuing development!

@nornagon nornagon closed this Mar 11, 2021
@apodolny
Copy link

apodolny commented Mar 18, 2021

@tristaaan

I tried rebasing on master and I had a rather difficult time with it and ended up cherry-picking the commit.
https://github.com/tristaaan/electron/tree/set_skip_cursor_rebased

Do you want to make a new PR based on your change?

@adityak368
Copy link

adityak368 commented Mar 19, 2021

@CapOM Created a new PR instead of rebase, which seemed to be the easier way. I think the patches needs to be updated as it failed to apply for me.

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

Successfully merging this pull request may close these issues.

None yet