New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 699672 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , Mac
Pri: 1
Type: Bug



Sign in to add a comment

ScreenCapturerMac doesn't capture whole desktop correctly (kFullDesktopScreenId)

Project Member Reported by sergeyu@chromium.org, Mar 8 2017

Issue description

What steps will reproduce the problem?
(1) Connect to Chrome Remote Desktop host running on Mac with multiple monitors

What is the expected result?
All monitors are shown on the client

What happens instead?
Screen content is not updated correctly

The same issue can be reproduced by commenting SelectSource() call in desktop_capture_device.cc (here https://codesearch.chromium.org/chromium/src/content/browser/media/capture/desktop_capture_device.cc?l=388 ). With that change the desktop capturer should capture whole desktop.

See my comments on https://codereview.webrtc.org/2496413002

I'd like to get this fixed for M-58. Erik, please let me know if you don't have time to work on this fix or if you need help reproducing it.
 
Based on your comments in https://codereview.webrtc.org/2496413002#msg16, it sounds like this might be as simple as some translation logic gated behind a conditional that checks for kFullDesktopScreenId. I can certainly throw up a quick CL to do that.
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/31bbee73a029376ca5a6f74e3d5bf224053a6208

commit 31bbee73a029376ca5a6f74e3d5bf224053a6208
Author: erikchen <erikchen@chromium.org>
Date: Thu Mar 09 21:19:03 2017

mac: Fix screen capture for whole-desktop capture.

DisplayStream refresh rects are in display coordinates. When the whole screen is
being captured, the coordinates passed to the ScreenCapturerHelper need to be in
screen coordinates. This CL translates display coordinates to screen
coordinates for whole screen capture.

BUG= chromium:699672 

Review-Url: https://codereview.webrtc.org/2740823002
Cr-Commit-Position: refs/heads/master@{#17153}

[modify] https://crrev.com/31bbee73a029376ca5a6f74e3d5bf224053a6208/webrtc/modules/desktop_capture/screen_capturer_mac.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/31bbee73a029376ca5a6f74e3d5bf224053a6208

commit 31bbee73a029376ca5a6f74e3d5bf224053a6208
Author: erikchen <erikchen@chromium.org>
Date: Thu Mar 09 21:19:03 2017

mac: Fix screen capture for whole-desktop capture.

DisplayStream refresh rects are in display coordinates. When the whole screen is
being captured, the coordinates passed to the ScreenCapturerHelper need to be in
screen coordinates. This CL translates display coordinates to screen
coordinates for whole screen capture.

BUG= chromium:699672 

Review-Url: https://codereview.webrtc.org/2740823002
Cr-Commit-Position: refs/heads/master@{#17153}

[modify] https://crrev.com/31bbee73a029376ca5a6f74e3d5bf224053a6208/webrtc/modules/desktop_capture/screen_capturer_mac.mm

Labels: Merge-Request-58
I'm not familiar with merging webrtc changes to branches - can someone take care of that part?
Project Member

Comment 5 by sheriffbot@chromium.org, Mar 10 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), bhthompson@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: erikc...@chromium.org
Owner: ----
Status: Untriaged (was: Assigned)

Comment 7 by gov...@chromium.org, Mar 12 2017

Please merge your change to M58 branch 3029 before 5:00 PM PT, Monday (03/13/17) so we can take it in for next week dev release. Thank you!

Project Member

Comment 8 by sheriffbot@chromium.org, Mar 14 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: sergeyu@chromium.org
Status: Started (was: Untriaged)
Your change is approved for M58. Please ensure whether this fix is verified in canary. If yes, please merge ASAP so that it will be picked up for Beta promotion RC cut on 03/15 at 5.00 PM PST.
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 15 2017

Labels: merge-merged-58
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/5810d27a9b13fe581282acc33350525be43e6f95

commit 5810d27a9b13fe581282acc33350525be43e6f95
Author: Sergey Ulanov <sergeyu@chromium.org>
Date: Wed Mar 15 18:40:11 2017

mac: Fix screen capture for whole-desktop capture.

DisplayStream refresh rects are in display coordinates. When the whole screen is
being captured, the coordinates passed to the ScreenCapturerHelper need to be in
screen coordinates. This CL translates display coordinates to screen
coordinates for whole screen capture.

BUG= chromium:699672 

Review-Url: https://codereview.webrtc.org/2740823002
Cr-Commit-Position: refs/heads/master@{#17153}
(cherry picked from commit 31bbee73a029376ca5a6f74e3d5bf224053a6208)

Review-Url: https://codereview.webrtc.org/2755663004 .
Cr-Commit-Position: refs/branch-heads/58@{#4}
Cr-Branched-From: f31969a584bcafe9406c214a9d4c3afb49d19650-refs/heads/master@{#16937}

[modify] https://crrev.com/5810d27a9b13fe581282acc33350525be43e6f95/webrtc/modules/desktop_capture/screen_capturer_mac.mm

Status: Fixed (was: Started)
Labels: -Merge-Approved-58
Per comment #11, this is already merged to M58. Hence, removing "Merge-Approved-58" label. 
When connecting to a Mac mini with two equal sized monitors, this is fixed. The mini is non-corp. When connecting to a corp MBP with a second monitor connected through Displayport with curtain mode on, one screen is fine but the other is black. I do not have access to another corp Mac, so cannot say if this is because the second monitor is not the same resolution, if its because its corp, or if its because curtain mode is on. I'll disable curtain mode and update the bug with the results. 
When connecting to the MBP with curtain mode off, both displays show up, so it is mostly correct. However, there is corruption below the area of the first monitor (the smaller one) which fills in the gap to match the size of the second monitor. 
Status: Untriaged (was: Fixed)
AJ, can you please attach a screenshot?
After connecting from a different client, I can no longer repro what I described above. Instead, the client only shows the primary monitor. I wrote  bug 704781  for this. 
Status: Fixed (was: Untriaged)
Labels: OS-Android
Status: Verified (was: Fixed)
Verified Fixed in 58.0.3029.28

Sign in to add a comment