[Desktop Capture] Inaccurate mouse cursor position when sharing window on Mac with retina screen |
||||||||||||||||||||
Issue descriptionOS: 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.
,
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
,
Oct 26 2017
This seems not an urgent problem for a merging request. Let me know if you have a different opinion.
,
Oct 26 2017
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?
,
Oct 27 2017
I am OK with merging. Change in #c2 needs to be merged to M63/WebRTC.
,
Oct 27 2017
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
,
Oct 28 2017
,
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.
,
Oct 30 2017
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.!
,
Oct 30 2017
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?
,
Oct 30 2017
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
,
Oct 31 2017
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.!
,
Oct 31 2017
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.
,
Oct 31 2017
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.
,
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
,
Oct 31 2017
This change does not need to be merged to M63 anymore.
,
Nov 1 2017
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
,
Nov 1 2017
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
,
Nov 1 2017
,
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
,
Nov 7 2017
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
,
Nov 7 2017
It's expected. The new logic has not been enabled, so the behavior won't change by last CL.
,
Dec 1 2017
Based on the discussion in another thread, change in #c2 needs to be merged to M63/WebRTC.
,
Dec 1 2017
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
,
Dec 1 2017
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?
,
Dec 1 2017
,
Dec 1 2017
PTAL comment #25. Please plan to have this merge in before 4:00 PM PT tomorrow, Friday. Thank you.
,
Dec 1 2017
The cl in #20 doesn't need the merge, which doesn't take effect because of the merge in #18.
,
Dec 1 2017
Ok, thanks for confirmation. Please plan to have CL listed #2 merge in before 4:00 PM PT tomorrow, Friday. Thank you.
,
Dec 1 2017
Based on the Buganizer discussion, I think the only needed fix is the one from c#2 (https://webrtc-review.googlesource.com/15782).
,
Dec 1 2017
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
,
Dec 1 2017
Per comment #31, this is already merged to M63. So removing "Merge-Approved-63" label.
,
Dec 11 2017
Issue 788613 has been merged into this issue.
,
Jan 22 2018
Issue 804228 has been merged into this issue. |
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by braveyao@chromium.org
, Oct 25 2017