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 (Chrome 59/1-8-x) #11677

Merged
merged 1 commit into from Mar 1, 2018

Conversation

Projects
None yet
4 participants
@loc
Contributor

loc commented Jan 20, 2018

Same fix as #11664, but for Chrome 59 (where content/public/browser/desktop_capture.h doesn't exist). This relies on a ref counted DXGI controller as patched and explained here: electron/libchromiumcontent#437. Will update libcc in this PR when it merges.

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

@loc loc changed the title from Use same screen capturer settings for thumbnails as getUserMedia (Chrome 59/1_8_x) to Use same screen capturer settings for thumbnails as getUserMedia (Chrome 59/1-8-x) Jan 20, 2018

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Jan 29, 2018

Member

@loc can you update the libcc ref, thanks!

Member

deepak1556 commented Jan 29, 2018

@loc can you update the libcc ref, thanks!

@loc

This comment has been minimized.

Show comment
Hide comment
@loc

loc Feb 2, 2018

Contributor

updated! @deepak1556

Contributor

loc commented Feb 2, 2018

updated! @deepak1556

@loc

This comment has been minimized.

Show comment
Hide comment
@loc

loc Feb 2, 2018

Contributor

actually let's hold off. getting a seg fault on subsequent screen shares in the release target. looking into it.

Contributor

loc commented Feb 2, 2018

actually let's hold off. getting a seg fault on subsequent screen shares in the release target. looking into it.

@loc

This comment has been minimized.

Show comment
Hide comment
@loc

loc Feb 7, 2018

Contributor

@deepak1556: okay. Found the trouble. I think this is okay to merge.

First, there's a bug in the DirectX capturer when using it in a debug Electron because static variables can't cross shared library bounds. So you end up with two different DXGIDuplicatorControllers, even though Windows strictly prohibits multiple output duplicators (per application). In the static lib version, things look good, except there's a crash in certain cases with 3 monitors when capturing both thumbnails and a capture stream from getUserMedia at the same time. I don't think that should block this merge. I plan to investigate further, but the easy workaround for now is to stop capture streams before grabbing thumbnails again.

Contributor

loc commented Feb 7, 2018

@deepak1556: okay. Found the trouble. I think this is okay to merge.

First, there's a bug in the DirectX capturer when using it in a debug Electron because static variables can't cross shared library bounds. So you end up with two different DXGIDuplicatorControllers, even though Windows strictly prohibits multiple output duplicators (per application). In the static lib version, things look good, except there's a crash in certain cases with 3 monitors when capturing both thumbnails and a capture stream from getUserMedia at the same time. I don't think that should block this merge. I plan to investigate further, but the easy workaround for now is to stop capture streams before grabbing thumbnails again.

@ckerr ckerr requested a review from deepak1556 Feb 7, 2018

@deepak1556

LGTM, Thanks!

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Feb 10, 2018

Member

except there's a crash in certain cases with 3 monitors when capturing both thumbnails and a capture stream from getUserMedia at the same time

Did this case not crash before this change?

Member

MarshallOfSound commented Feb 10, 2018

except there's a crash in certain cases with 3 monitors when capturing both thumbnails and a capture stream from getUserMedia at the same time

Did this case not crash before this change?

@loc

This comment has been minimized.

Show comment
Hide comment
@loc

loc Feb 13, 2018

Contributor

@MarshallOfSound correct, unfortunately. It seems to be a very narrow case, but I haven't explored it completely. If that's unacceptable, I may be able to carve out some time in the next week or two to try and figure it out. It's quite a bit harder to debug because the shared library build hides the issue with its own bug. The good news is that there is no crash in master, so there's probably a relevant WebRTC commit that I just haven't found yet.

Contributor

loc commented Feb 13, 2018

@MarshallOfSound correct, unfortunately. It seems to be a very narrow case, but I haven't explored it completely. If that's unacceptable, I may be able to carve out some time in the next week or two to try and figure it out. It's quite a bit harder to debug because the shared library build hides the issue with its own bug. The good news is that there is no crash in master, so there's probably a relevant WebRTC commit that I just haven't found yet.

@loc

This comment has been minimized.

Show comment
Hide comment
@loc

loc Feb 22, 2018

Contributor

@deepak1556 @MarshallOfSound when can I expect this to merge? If merge is contingent on fixing the crash I disclosed, I'd love to know.

Contributor

loc commented Feb 22, 2018

@deepak1556 @MarshallOfSound when can I expect this to merge? If merge is contingent on fixing the crash I disclosed, I'd love to know.

@zcbenz zcbenz merged commit d1f1210 into electron:1-8-x Mar 1, 2018

7 of 8 checks passed

jenkins: macOS/pr-head This commit cannot be built
Details
WIP ready for review
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-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-mips64el Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment