Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496413002:
mac: Fix screen capture on secondary displays. (Closed)
Descriptionmac: Fix screen capture on secondary displays.
The old API CGScreenRegisterMoveCallback returned update rects in desktop
coordinates [secondary display has an origin != 0,0]. The new CGDisplayStream
API returns update rects in display coordinates [origin == 0,0]. Translating the
update rect based on the display's position on the desktop is now incorrect.
BUG=webrtc:6702
Committed: https://crrev.com/2a3eb9f367a70e2f5060d9dea6edd5ccd326de61
Cr-Commit-Position: refs/heads/master@{#15092}
Patch Set 1 #
Messages
Total messages: 16 (6 generated)
erikchen@chromium.org changed reviewers: + kjellander@webrtc.org
kjellander: Please review.
Description was changed from ========== mac: Fix screen capture on secondary displays. The old API CGScreenRegisterMoveCallback returned update rects in desktop coordinates [secondary display has an origin != 0,0]. The new CGDisplayStream API returns update rects in display coordinates [origin == 0,0]. Translating the update rect based on the display's position on the desktop is now incorrect. BUG=6702 ========== to ========== mac: Fix screen capture on secondary displays. The old API CGScreenRegisterMoveCallback returned update rects in desktop coordinates [secondary display has an origin != 0,0]. The new CGDisplayStream API returns update rects in display coordinates [origin == 0,0]. Translating the update rect based on the display's position on the desktop is now incorrect. BUG=webrtc:6702 ==========
kjellander@webrtc.org changed reviewers: + sergeyu@chromium.org
I'm not an OWNER here, so I'm deferring this review to Sergey.
lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== mac: Fix screen capture on secondary displays. The old API CGScreenRegisterMoveCallback returned update rects in desktop coordinates [secondary display has an origin != 0,0]. The new CGDisplayStream API returns update rects in display coordinates [origin == 0,0]. Translating the update rect based on the display's position on the desktop is now incorrect. BUG=webrtc:6702 ========== to ========== mac: Fix screen capture on secondary displays. The old API CGScreenRegisterMoveCallback returned update rects in desktop coordinates [secondary display has an origin != 0,0]. The new CGDisplayStream API returns update rects in display coordinates [origin == 0,0]. Translating the update rect based on the display's position on the desktop is now incorrect. BUG=webrtc:6702 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== mac: Fix screen capture on secondary displays. The old API CGScreenRegisterMoveCallback returned update rects in desktop coordinates [secondary display has an origin != 0,0]. The new CGDisplayStream API returns update rects in display coordinates [origin == 0,0]. Translating the update rect based on the display's position on the desktop is now incorrect. BUG=webrtc:6702 ========== to ========== mac: Fix screen capture on secondary displays. The old API CGScreenRegisterMoveCallback returned update rects in desktop coordinates [secondary display has an origin != 0,0]. The new CGDisplayStream API returns update rects in display coordinates [origin == 0,0]. Translating the update rect based on the display's position on the desktop is now incorrect. BUG=webrtc:6702 Committed: https://crrev.com/2a3eb9f367a70e2f5060d9dea6edd5ccd326de61 Cr-Commit-Position: refs/heads/master@{#15092} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2a3eb9f367a70e2f5060d9dea6edd5ccd326de61 Cr-Commit-Position: refs/heads/master@{#15092}
Message was sent while issue was closed.
This patch introduced a breaking bug for Chrome Remote Desktop when remoting to a host with two monitors. The mouse location is off by the size of the left most screen when interacting with the right most screen. Please revert.
Message was sent while issue was closed.
On 2017/03/07 16:40:52, nicholss wrote: > This patch introduced a breaking bug for Chrome Remote Desktop when remoting to > a host with two monitors. The mouse location is off by the size of the left most > screen when interacting with the right most screen. Please revert.
Message was sent while issue was closed.
On 2017/03/08 00:10:56, nicholss wrote: > On 2017/03/07 16:40:52, nicholss wrote: > > This patch introduced a breaking bug for Chrome Remote Desktop when remoting > to > > a host with two monitors. The mouse location is off by the size of the left > most > > screen when interacting with the right most screen. Please revert. That is surprising because this CL was introduced to fix a very similar sounding issue. Perhaps there's a configuration difference that's causing this? Do you have more specific details about your macOS version, Chrome version, minimal repro, etc?
Message was sent while issue was closed.
On 2017/03/08 00:14:00, erikchen wrote: > On 2017/03/08 00:10:56, nicholss wrote: > > On 2017/03/07 16:40:52, nicholss wrote: > > > This patch introduced a breaking bug for Chrome Remote Desktop when remoting > > to > > > a host with two monitors. The mouse location is off by the size of the left > > most > > > screen when interacting with the right most screen. Please revert. > > That is surprising because this CL was introduced to fix a very similar sounding > issue. Perhaps there's a configuration difference that's causing this? Do you > have more specific details about your macOS version, Chrome version, minimal > repro, etc? If I understand correctly before this CL the capturer worked correctly only with kFullDesktopScreenId, but not with individual displays. After this change it works correctly for individual displays, but not for kFullDesktopScreenId. CRD always uses kFullDesktopScreenId, while Desktop Capture API never uses it. Before https://codereview.webrtc.org/2391743004 we were always getting update notifications from the OS in screen coordinates. ScreenCapturerMac::ScreenRefresh() accounted for it by shifting rect coordinates according to screen position relative to desktop origin. After ScreenCapturerMac::ScreenRefresh() gets updates in display coordinates. In the current version (with this change) it works for individual display, but not for kFullDesktopScreenId. ScreenRefresh() gets updates in display coordinates, but doesn't translate them to desktop coordinates (and it has no idea which display they belong to). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
