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

Issue 778049 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

[Desktop Capture] Inaccurate mouse cursor position when sharing window on Mac with retina screen

Project Member Reported by braveyao@chromium.org, Oct 25 2017

Issue description

OS: Mac OSX 10.12

What steps will reproduce the problem?
(1) Launch Canary
(2) Start desktopCapture and share any application window
(3) Move cursor within app window

What is the expected result?
Cursor position is accurate in the captured image.

What happens instead?
Cursor is missing in the captured image.

Please use labels and text to provide additional information.
This issue firstly emerges in chromium after revision #500761, https://chromium-review.googlesource.com/c/chromium/src/+/651147. But the real culprit should be in other previous changes.

 
This one may be related to  issue 778035 . Since it's on another platform with different symptom, so this issue is filed separately.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 26 2017

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/c2a0eb2699f7871c876a487105070f84c38a3dd0

commit c2a0eb2699f7871c876a487105070f84c38a3dd0
Author: Zijie He <zijiehe@google.com>
Date: Thu Oct 26 21:14:57 2017

[Window Capture] Mouse cursor missing during window sharing on Mac OSX

CGWindowID is 32-bit, WindowId is 64-bit, using WindowId to receive int value
from CFNumberGetValue() causes the top 32 bits to be random. WindowFinderMac is
impacted by this issue and returns a random number. WindowCapturerMac cannot
match the window_id_ with the the random number.

Meanwhile MouseCursorMonitorMac uses window title to match "Dock" window. See,
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm?rcl=a194e58e799ccab6c999998e5d0f75725aa3f748&l=174

This logic should not be necessary on 10.12 or upper, the name of dock window
is not "Dock" anymore. But to ensure the consistency on old platforms, I have
also added this logic back into GetWindowList() function.

Bug:  chromium:778049 
Change-Id: Ie827bcd5d31f2ca69ff24c24cf640cb7cc50d419
Reviewed-on: https://webrtc-review.googlesource.com/15782
Commit-Queue: Zijie He <zijiehe@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#20451}
[modify] https://crrev.com/c2a0eb2699f7871c876a487105070f84c38a3dd0/modules/desktop_capture/mac/window_list_utils.cc

Owner: braveyao@chromium.org
Status: Fixed (was: Assigned)
This seems not an urgent problem for a merging request.
Let me know if you have a different opinion.
We do have issue reports against missing cursor or wrong cursor position before. So I think having accurate cursor during desktop sharing is kind of important. M63 should be impacted by this issue and also  issue778035 . I suppose it's better to merge these two back to M63. WDYT?
Labels: Merge-Request-63
I am OK with merging.

Change in #c2 needs to be merged to M63/WebRTC.
Project Member

Comment 6 by sheriffbot@chromium.org, Oct 27 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: zijiehe@chromium.org

Comment 8 by gov...@chromium.org, Oct 28 2017

Before we approve merge to M63, please provide following details:

Is this M63 regression and critical to merge?
Is the change well baked/verified in Canary and safe to merge?
Please provide any other details to justify the merge. Thank you.

Please note M63 is already promoted to Beta so merge bar is very high. Thank you.
Cc: ranjitkan@chromium.org
Rechecked this issue on Mac 10.12.6 using chrome Canary version 64.0.3253.0 by using the sample extension downloaded from https://developer.chrome.com/extensions/samples#desktop-capture-example and observed that still some times cursor is missing when sharing an application.

Attached a Video recording for the same. On the left side is the Browser window where the cursor is moved on a shared application and on to the right is the extension where the captured screen is observed.

Thanks.!
Cursor Missing.mov
9.1 MB Download
This bug should be fixed on 64.0.3253.0 (Official Build) canary (64-bit). See the screenshot. Would you please share your OS as well as screenshot for further investigation?
778049.png
805 KB View Download
Still can reproduce it with latest Canary and ToT chromium building. And today I found out that the cursor is not missing, but deviates too much. See the screenshot for more details.

MBP: OSX10.12.6, Retina 15-inch, Mid2015
screenshot.png
952 KB View Download
Rechecked this issue on Mac 10.12.6 using chrome Canary version 64.0.3254.0 by using the sample extension downloaded from https://developer.chrome.com/extensions/samples#desktop-capture-example and observed that still some times cursor is missing when sharing an application. When taking the screen shot, missing the cursor. hence recorded a video to show the cursor missing.

Request you to please take a look into it.

Thanks.!
Cursor Missing_1.mov
3.0 MB Download
Status: Assigned (was: Fixed)
Figure out what's going on here now.
The patch in #c2 can fix the problem on Mac machines with non-Retina screen, and on Mac machines with Retina screen, there are other issues to be fixed.

Simply to say, the problem on Retina screen is because of mix-using Retina coordination and non-Retina coordination.
- now the cursor position is in Retina coordination.
- as to the capture window frame, its size is in Retina coordination, and its top-left is in non-Retina coordination which is from invoking "GetWindowBounds()", https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_capture/window_capturer_mac.mm?sq=package:chromium&dr&l=207. So most probably it will get wrong result when it checks if the window contains cursor position.
- when we check if the window is occluded, https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_capture/window_capturer_mac.mm?rcl=1d4c152a3836df3b687268561018ccb7172cf177&l=138, it finally will go to https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_capture/window_finder_mac.mm?type=cs&q=GetWindowUnderPoint&sq=package:chromium&l=23, where "GetWindowBounds()" is called. Then the window bounds is in non-retina coordinates while the cursor position is in Retina coordinates. So most probably the IsOcculude() will return wrong result.

I suppose the solution would be to make all items in Rects in same coordinates as cursor position.
Talked offline with Brave, GetWindowBounds() returns both top-left and size in non-retina coordinates. So we can simply return non-retina coordinates from MouseCursorMonitorMac.
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a511b3d8f416498d105a3a42e7c95b16ba2e4d5d

commit a511b3d8f416498d105a3a42e7c95b16ba2e4d5d
Author: Zijie He <zijiehe@chromium.org>
Date: Tue Oct 31 22:20:34 2017

Revert "[Desktop Capturer] Use new DesktopAndCursorComposer constructor"

This reverts commit cf3aebae2044c573b4b23a2fa490ff0cc3d2bfe0.

Reason for revert: The new API breaks on X11 and Mac OSX.

Original change's description:
> [Desktop Capturer] Use new DesktopAndCursorComposer constructor
> 
> After several changes, we finally removed the dependency of SourceId in
> MouseCursorMonitor. Now DesktopAndCursorComposer provides a clearer
> constructor for its clients.
> 
> Bug:  webrtc:7950 
> Change-Id: I8716d736897637412e712fcc2b2d17ef0acf3eab
> Reviewed-on: https://chromium-review.googlesource.com/651147
> Commit-Queue: Zijie He <zijiehe@chromium.org>
> Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
> Reviewed-by: Weiyong Yao <braveyao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#500761}

TBR=jamiewalch@chromium.org,braveyao@chromium.org,zijiehe@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  webrtc:7950 ,  chromium:778035 ,  chromium:778049 
Change-Id: I45fbc68ae52194164828bc34207fe849850a98bf
Reviewed-on: https://chromium-review.googlesource.com/746947
Reviewed-by: Weiyong Yao <braveyao@chromium.org>
Reviewed-by: Zijie He <zijiehe@chromium.org>
Commit-Queue: Zijie He <zijiehe@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512976}
[modify] https://crrev.com/a511b3d8f416498d105a3a42e7c95b16ba2e4d5d/content/browser/media/capture/desktop_capture_device.cc

Labels: -Hotlist-Merge-Review -Merge-Review-63
This change does not need to be merged to M63 anymore.
Labels: TE-Verified-M64 TE-Verified-64.0.3255.0
Rechecked this issue on Mac 10.12.6 Retina using chrome version 64.0.3255.0 and fix is working as intended. Able to view the cursor and is not hiding after some movements.

Adding TE-Verified labels for M64
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 1 2017

Labels: merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f9fc3111e7035dc8e133f1f736e014df9f399b7

commit 9f9fc3111e7035dc8e133f1f736e014df9f399b7
Author: Zijie He <zijiehe@chromium.org>
Date: Wed Nov 01 17:40:52 2017

Revert "[Desktop Capturer] Use new DesktopAndCursorComposer constructor"

This reverts commit cf3aebae2044c573b4b23a2fa490ff0cc3d2bfe0.

Reason for revert: The new API breaks on X11 and Mac OSX.

Original change's description:
> [Desktop Capturer] Use new DesktopAndCursorComposer constructor
> 
> After several changes, we finally removed the dependency of SourceId in
> MouseCursorMonitor. Now DesktopAndCursorComposer provides a clearer
> constructor for its clients.
> 
> Bug:  webrtc:7950 
> Change-Id: I8716d736897637412e712fcc2b2d17ef0acf3eab
> Reviewed-on: https://chromium-review.googlesource.com/651147
> Commit-Queue: Zijie He <zijiehe@chromium.org>
> Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
> Reviewed-by: Weiyong Yao <braveyao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#500761}

TBR=jamiewalch@chromium.org,braveyao@chromium.org,zijiehe@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  webrtc:7950 ,  chromium:778035 ,  chromium:778049 
Change-Id: I45fbc68ae52194164828bc34207fe849850a98bf
Reviewed-on: https://chromium-review.googlesource.com/746947
Reviewed-by: Weiyong Yao <braveyao@chromium.org>
Reviewed-by: Zijie He <zijiehe@chromium.org>
Commit-Queue: Zijie He <zijiehe@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#512976}(cherry picked from commit a511b3d8f416498d105a3a42e7c95b16ba2e4d5d)
Reviewed-on: https://chromium-review.googlesource.com/749104
Cr-Commit-Position: refs/branch-heads/3239@{#334}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/9f9fc3111e7035dc8e133f1f736e014df9f399b7/content/browser/media/capture/desktop_capture_device.cc

Summary: [Desktop Capture] Inaccurate mouse cursor position when sharing window on Mac with retina screen (was: [Desktop Capture] cursor missing during Window sharing on Mac OSX)
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 7 2017

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/411582b13a08ca85c19fc5dbba66bcf9ffc157e1

commit 411582b13a08ca85c19fc5dbba66bcf9ffc157e1
Author: Zijie He <zijiehe@google.com>
Date: Tue Nov 07 01:49:35 2017

[Window Capturer] Implement scaling in GetWindowBounds()

On Mac OSX system, if retina screen is used, the GetWindowBounds() returns
pre-scaled values instead of system coordinates. So this fix considers
per-monitor scale-factor, and stretchs the DesktopRect.

Bug:  chromium:778049 
Change-Id: I9dc51e08235eba9b3ef6378eaa15737aa444b0c8
Reviewed-on: https://webrtc-review.googlesource.com/17600
Commit-Queue: Zijie He <zijiehe@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#20578}
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/BUILD.gn
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/mac/window_list_utils.cc
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/mac/window_list_utils.h
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/mouse_cursor_monitor_mac.mm
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/window_capturer_mac.mm
[add] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/window_finder.cc
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/window_finder.h
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/window_finder_mac.h
[modify] https://crrev.com/411582b13a08ca85c19fc5dbba66bcf9ffc157e1/modules/desktop_capture/window_finder_mac.mm

Labels: TE-Verified-M63 TE-Verified-63.0.3239.39
Rechecked this issue on Mac 10.12.6 Retina using chrome version 63.0.3239.39 and fix is working as intended. Able to view the cursor and is not hiding after some movements.

Adding TE-Verified labels for M63
Status: Fixed (was: Assigned)
It's expected. The new logic has not been enabled, so the behavior won't change by last CL.
Labels: Merge-Request-63
Based on the discussion in another thread, change in #c2 needs to be merged to M63/WebRTC.
Project Member

Comment 24 by sheriffbot@chromium.org, Dec 1 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: We are only 3 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-63 Merge-Approved-63
Approving merge to M63 branch 3239 for CL listed #2 based on internal buganizer bug 68698848.

Just checking, Is cl listed at #20 ( https://webrtc.googlesource.com/src.git/+/411582b13a08ca85c19fc5dbba66bcf9ffc157e1) need to be merged as well?
Cc: braveyao@chromium.org
Cc: jamiewa...@chromium.org
PTAL comment #25.

Please plan to have this merge in before 4:00 PM PT tomorrow, Friday. Thank you.
The cl in #20 doesn't need the merge, which doesn't take effect because of the merge in #18.
Ok, thanks for confirmation.

Please plan to have CL listed #2 merge in before 4:00 PM PT tomorrow, Friday. Thank you.



Based on the Buganizer discussion, I think the only needed fix is the one from c#2 (https://webrtc-review.googlesource.com/15782).
Project Member

Comment 31 by bugdroid1@chromium.org, Dec 1 2017

Labels: merge-merged-63
The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/b3fd9700eb1d5e79e7eed37db0bca2fae059fefe

commit b3fd9700eb1d5e79e7eed37db0bca2fae059fefe
Author: Zijie He <zijiehe@google.com>
Date: Fri Dec 01 01:51:08 2017

[Window Capture] Mouse cursor missing during window sharing on Mac OSX

CGWindowID is 32-bit, WindowId is 64-bit, using WindowId to receive int value
from CFNumberGetValue() causes the top 32 bits to be random. WindowFinderMac is
impacted by this issue and returns a random number. WindowCapturerMac cannot
match the window_id_ with the the random number.

Meanwhile MouseCursorMonitorMac uses window title to match "Dock" window. See,
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm?rcl=a194e58e799ccab6c999998e5d0f75725aa3f748&l=174

This logic should not be necessary on 10.12 or upper, the name of dock window
is not "Dock" anymore. But to ensure the consistency on old platforms, I have
also added this logic back into GetWindowList() function.

Bug:  chromium:778049 
Change-Id: Ie827bcd5d31f2ca69ff24c24cf640cb7cc50d419
Reviewed-on: https://webrtc-review.googlesource.com/15782
Commit-Queue: Zijie He <zijiehe@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#20451}(cherry picked from commit c2a0eb2699f7871c876a487105070f84c38a3dd0)
Reviewed-on: https://webrtc-review.googlesource.com/27822
Reviewed-by: Zijie He <zijiehe@chromium.org>
Cr-Commit-Position: refs/branch-heads/63@{#13}
Cr-Branched-From: bef8a5d2ca5413c680995584b8c0976852ba5f25-refs/heads/master@{#20237}
[modify] https://crrev.com/b3fd9700eb1d5e79e7eed37db0bca2fae059fefe/modules/desktop_capture/mac/window_list_utils.cc

Labels: -Merge-Approved-63
Per comment #31, this is already merged to M63. So removing "Merge-Approved-63" label.
Issue 788613 has been merged into this issue.
 Issue 804228  has been merged into this issue.

Sign in to add a comment