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

Use same screen capturer settings for thumbnails as getUserMedia #11664

Merged
merged 2 commits into from Jan 24, 2018

Conversation

Projects
None yet
4 participants
@loc
Contributor

loc commented Jan 18, 2018

Fixes #9933 (and its duplicates #10513, #11609)

Following along from http://crrev.com/2961193002: ensure the
screen thumbnail capturers initialize with the same settings as the screen capturer
from a getUserMedia call. Otherwise, there is no guarantee that the
sources on Windows will match. As a result, this change switches the default capturer for thumbnails on Windows from GDI to the newer DirectX capturer. It maintains GDI as a fallback in the event of failure.

A caveat: there is still a bug that causes Chrome and atom::api::DesktopCapturer to end up with different screen capture sources. If you try to capture thumbnails while already capturing a screen from getUserMedia, it seems that DXGIDuplicatorController gets double initialized, DXGI output duplication fails (because you're explicitly not allowed to duplicate the same monitor twice), and the thumbnail capturer will fallback to the old GDI capturer (and thus, different source IDs). I'm still looking into why the statically initialized DXGIDuplicatorController doesn't end up being reused, but my guess is that it's happening in a different process. Any thoughts on next steps there would be welcome.

Testing:
Windows 10: Tested with various monitor setups and adapters.
Mac (High Sierra): Quick smoke test that screen sharing still works.

use same settings for screen thumbnails as chrome does webrtc capturing
Following along from http://crrev.com/2961193002: make sure the
thumbnail capturers initialize with the same settings as the capturer
from a getUserMedia request does. Otherwise, there is no guarentee that the
sources on Windows will match.

@loc loc requested a review from electron/reviewers as a code owner Jan 18, 2018

@welcome

This comment has been minimized.

welcome bot commented Jan 18, 2018

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it 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.
@deepak1556

This comment has been minimized.

Member

deepak1556 commented Jan 18, 2018

DxgiDuplicatorController has been turned into a ref counted instance https://codereview.chromium.org/2933893003/ , its possible that the states got uninitialized between captures because we reset the media capturer. We can probably store a reference to DxgiDuplicatorController::Instance() in atom::api::DesktopCapturer to work around this. Can you verify if it fixes the issue ? Also can you verify how does this PR behave on 1-8-x branch of electron where the DxgiDuplicatorController is not ref counted. Thanks!

@loc

This comment has been minimized.

Contributor

loc commented Jan 20, 2018

Ah, good thought @deepak1556. PR for 1-8-x (would love to get this in ASAP), dependent on the ref counted DXGIDuplicatorController, as you suspected: #11677

@zcbenz

zcbenz approved these changes Jan 24, 2018

Nice!

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jan 24, 2018

The failing test is not related to this PR, I'm merging anyway.

@zcbenz zcbenz merged commit 0207aeb into electron:master Jan 24, 2018

5 of 8 checks passed

ci/circleci: electron-linux-arm-test Your tests failed on CircleCI
Details
ci/circleci: electron-linux-ia32 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-x64 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
@welcome

This comment has been minimized.

welcome bot commented Jan 24, 2018

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment