Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1959863002:
CURSORINFO.flags should be checked before capturing its bitmap (Closed)
DescriptionCURSORINFO.flags should be checked before capturing its bitmap
BUG=566178, 428886
Committed: https://crrev.com/99f8cd0ae2f05c24410ddd6b2d9ea3d56f1fec5f
Cr-Commit-Position: refs/heads/master@{#12739}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Only ignore cursor bitmap when host hides it #
Total comments: 4
Patch Set 3 : Send an empty bitmap if cursor is suppressed, send a default arrow if cursor is hidden. #
Total comments: 9
Patch Set 4 : Resolve review comments #Patch Set 5 : More comments about why we do not CloseHandle on an HCURSOR. #Messages
Total messages: 26 (8 generated)
Description was changed from ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178 ========== to ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178 ==========
zijiehe@chromium.org changed reviewers: + joedow@chromium.org
https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags != CURSOR_SHOWING) { Do any clients rely on receiving the bitmap when the cursor is hidden/suppressed to determine whether to show/hide the cursor? Perhaps the WebApp on Windows or ChromeOS.
https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags != CURSOR_SHOWING) { On 2016/05/09 17:14:05, joedow wrote: > Do any clients rely on receiving the bitmap when the cursor is hidden/suppressed > to determine whether to show/hide the cursor? Perhaps the WebApp on Windows or > ChromeOS. I believe other platform won't be impacted, they do not actively draw the cursor themselves.
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey, would you please have a look at this change?
https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:96: return; I see two issues with this fix: 1. This code is used for Desktop Capture API. For that API we want to capture the screen and mouse exactly as the user sees it (DesktopAndCursorComposer is used to render mouse cursor on captured frames). It means that if the cursor is hidden we want to hide it from captured frames as well. 2. If the cursor was shown and then then becomes hidden then the client will keep showing the last cursor it received. The right behavior here is to default to an arrow. I suggest adding a flag in webrtc::MouseCursor to indicate that the cursor is hidden and then handling that case in Chromoting by sending default arrow cursor.
https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags != CURSOR_SHOWING) { On 2016/05/09 17:14:05, joedow wrote: > Do any clients rely on receiving the bitmap when the cursor is hidden/suppressed > to determine whether to show/hide the cursor? Perhaps the WebApp on Windows or > ChromeOS. Done. https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags != CURSOR_SHOWING) { On 2016/05/10 00:40:58, Hzj_jie wrote: > On 2016/05/09 17:14:05, joedow wrote: > > Do any clients rely on receiving the bitmap when the cursor is > hidden/suppressed > > to determine whether to show/hide the cursor? Perhaps the WebApp on Windows > or > > ChromeOS. > > I believe other platform won't be impacted, they do not actively draw the cursor > themselves. Done. https://codereview.chromium.org/1959863002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:96: return; On 2016/05/10 17:17:07, Sergey Ulanov wrote: > I see two issues with this fix: > 1. This code is used for Desktop Capture API. For that API we want to capture > the screen and mouse exactly as the user sees it (DesktopAndCursorComposer is > used to render mouse cursor on captured frames). It means that if the cursor is > hidden we want to hide it from captured frames as well. > 2. If the cursor was shown and then then becomes hidden then the client will > keep showing the last cursor it received. The right behavior here is to default > to an arrow. > > I suggest adding a flag in webrtc::MouseCursor to indicate that the cursor is > hidden and then handling that case in Chromoting by sending default arrow > cursor. > The following logic (after if (mode_ != SHAPE_AND_POSITION)) will eventually draw the cursor out of the screen, if host hides it. But yes, I should not return directly here. I have updated the code.
https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags == CURSOR_SHOWING && I still think that wont do the right thing. Particularly in Desktop Capture API, if the cursor hidden we don't want it to be rendered on the screen by DesktopAndCursorComposer. With this change DesktopAndCursorComposer will not be notified that the cursor is hidden and it will keep drawing it. Also in CRD we want to default to arrow in that case instead of using the last non-hidden cursor.
https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags == CURSOR_SHOWING && On 2016/05/10 20:51:15, Sergey Ulanov wrote: > I still think that wont do the right thing. Particularly in Desktop Capture API, > if the cursor hidden we don't want it to be rendered on the screen by > DesktopAndCursorComposer. With this change DesktopAndCursorComposer will not be > notified that the cursor is hidden and it will keep drawing it. > Also in CRD we want to default to arrow in that case instead of using the last > non-hidden cursor. Actually looking at this again I see your point - the Desktop Capture API should work correctly as OnMouseCursorPosition() will be called with state=OUTSIDE. But I still think the behavior in CRD (when mode==SHAPE_ONLY) is incorrect.
https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags == CURSOR_SHOWING && On 2016/05/10 20:51:15, Sergey Ulanov wrote: > I still think that wont do the right thing. Particularly in Desktop Capture API, > if the cursor hidden we don't want it to be rendered on the screen by > DesktopAndCursorComposer. With this change DesktopAndCursorComposer will not be > notified that the cursor is hidden and it will keep drawing it. > Also in CRD we want to default to arrow in that case instead of using the last > non-hidden cursor. Done. https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if (cursor_info.flags == CURSOR_SHOWING && On 2016/05/10 20:58:12, Sergey Ulanov wrote: > On 2016/05/10 20:51:15, Sergey Ulanov wrote: > > I still think that wont do the right thing. Particularly in Desktop Capture > API, > > if the cursor hidden we don't want it to be rendered on the screen by > > DesktopAndCursorComposer. With this change DesktopAndCursorComposer will not > be > > notified that the cursor is hidden and it will keep drawing it. > > Also in CRD we want to default to arrow in that case instead of using the last > > non-hidden cursor. > > Actually looking at this again I see your point - the Desktop Capture API should > work correctly as OnMouseCursorPosition() will be called with state=OUTSIDE. But > I still think the behavior in CRD (when mode==SHAPE_ONLY) is incorrect. Based on our discussion, we will do this, 1. Always return a cursor info to the client. If Windows does not return a valid cursor, we will return a default one. 2. Add a field in CursorShapeInfo to indicate whether currently the cursor is shown or not.
On 2016/05/11 00:55:49, Hzj_jie wrote: > https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... > File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): > > https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... > webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if > (cursor_info.flags == CURSOR_SHOWING && > On 2016/05/10 20:51:15, Sergey Ulanov wrote: > > I still think that wont do the right thing. Particularly in Desktop Capture > API, > > if the cursor hidden we don't want it to be rendered on the screen by > > DesktopAndCursorComposer. With this change DesktopAndCursorComposer will not > be > > notified that the cursor is hidden and it will keep drawing it. > > Also in CRD we want to default to arrow in that case instead of using the last > > non-hidden cursor. > > Done. > > https://codereview.chromium.org/1959863002/diff/20001/webrtc/modules/desktop_... > webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:95: if > (cursor_info.flags == CURSOR_SHOWING && > On 2016/05/10 20:58:12, Sergey Ulanov wrote: > > On 2016/05/10 20:51:15, Sergey Ulanov wrote: > > > I still think that wont do the right thing. Particularly in Desktop Capture > > API, > > > if the cursor hidden we don't want it to be rendered on the screen by > > > DesktopAndCursorComposer. With this change DesktopAndCursorComposer will not > > be > > > notified that the cursor is hidden and it will keep drawing it. > > > Also in CRD we want to default to arrow in that case instead of using the > last > > > non-hidden cursor. > > > > Actually looking at this again I see your point - the Desktop Capture API > should > > work correctly as OnMouseCursorPosition() will be called with state=OUTSIDE. > But > > I still think the behavior in CRD (when mode==SHAPE_ONLY) is incorrect. > > Based on our discussion, we will do this, > 1. Always return a cursor info to the client. If Windows does not return a valid > cursor, we will return a default one. > 2. Add a field in CursorShapeInfo to indicate whether currently the cursor is > shown or not. Besides bug 566178, this change also intents to fix bug 428886. We will send a default arrow if Windows returns hidden cursor, and send an empty bitmap if Windows returns suppressed cursor.
Description was changed from ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178 ========== to ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178, 428886 ==========
https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:29: bool Equal(const CURSORINFO& left, const CURSORINFO& right) { Rename this to IsSameCursorShape(). This function verifies that the shape is the same, not that the CURSORINFO are equal (it doesn't check position) https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:30: if (left.flags == right.flags) { You can express the whole function with a single return statement: return left.flags == right.flags && (left.flags != CURSOR_SHOWING || left.hCursor == right.hCursor); https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:118: // Note that |cursor_info.hCursor| does not need to be freed. move this comment below, where we call CreateMouseCursorFromHCursor() https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:124: cursor_info.hCursor = LoadCursor(nullptr, IDC_ARROW); I think you also need to CloseHandle() for the handle returned by LoadCursor()
https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:29: bool Equal(const CURSORINFO& left, const CURSORINFO& right) { On 2016/05/12 23:23:33, Sergey Ulanov wrote: > Rename this to IsSameCursorShape(). This function verifies that the shape is the > same, not that the CURSORINFO are equal (it doesn't check position) Done. https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:30: if (left.flags == right.flags) { On 2016/05/12 23:23:33, Sergey Ulanov wrote: > You can express the whole function with a single return statement: > return left.flags == right.flags && > (left.flags != CURSOR_SHOWING || > left.hCursor == right.hCursor); Done. https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:118: // Note that |cursor_info.hCursor| does not need to be freed. On 2016/05/12 23:23:33, Sergey Ulanov wrote: > move this comment below, where we call CreateMouseCursorFromHCursor() According to MSDN http://shortn/_FXmsigcgUC (You may find "Before closing, you must use the DestroyCursor function to destroy any cursors you created with CreateCursor. It is not necessary to destroy cursors created by other functions."), we do not need to free HCURSOR created by other functions. So this comment explains both scenarios (use existing hCursor or use LoadCursor). I will update the comment to make it more clear. https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:124: cursor_info.hCursor = LoadCursor(nullptr, IDC_ARROW); On 2016/05/12 23:23:33, Sergey Ulanov wrote: > I think you also need to CloseHandle() for the handle returned by LoadCursor() Done.
lgtm https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:118: // Note that |cursor_info.hCursor| does not need to be freed. On 2016/05/13 01:05:28, Hzj_jie wrote: > On 2016/05/12 23:23:33, Sergey Ulanov wrote: > > move this comment below, where we call CreateMouseCursorFromHCursor() > > According to MSDN http://shortn/_FXmsigcgUC Please don't use shortn links in code reviews - these links don't work externally. > (You may find "Before closing, you > must use the DestroyCursor function to destroy any cursors you created with > CreateCursor. It is not necessary to destroy cursors created by other > functions."), we do not need to free HCURSOR created by other functions. So this > comment explains both scenarios (use existing hCursor or use LoadCursor). I will > update the comment to make it more clear. That comment is about cursors created using CreateCursor(). I don't think it applies here. Also if I understand this comment correctly it's about destroying cursor which is different from closing cursor _handle_. The "Before closing" part actually suggests that HCURSOR instances do need to be closed. Anyway, MSDN is confusion about it. From LoadCursor() documentation it looks like we should expect the same handle to be returned every time, so it doesn't need to be closed.
On 2016/05/13 21:32:40, Sergey Ulanov wrote: > lgtm > > https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... > File webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc (right): > > https://codereview.chromium.org/1959863002/diff/40001/webrtc/modules/desktop_... > webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc:118: // Note that > |cursor_info.hCursor| does not need to be freed. > On 2016/05/13 01:05:28, Hzj_jie wrote: > > On 2016/05/12 23:23:33, Sergey Ulanov wrote: > > > move this comment below, where we call CreateMouseCursorFromHCursor() > > > > According to MSDN http://shortn/_FXmsigcgUC > > Please don't use shortn links in code reviews - these links don't work > externally. > > > (You may find "Before closing, you > > must use the DestroyCursor function to destroy any cursors you created with > > CreateCursor. It is not necessary to destroy cursors created by other > > functions."), we do not need to free HCURSOR created by other functions. So > this > > comment explains both scenarios (use existing hCursor or use LoadCursor). I > will > > update the comment to make it more clear. > > That comment is about cursors created using CreateCursor(). I don't think it > applies here. Also if I understand this comment correctly it's about destroying > cursor which is different from closing cursor _handle_. The "Before closing" > part actually suggests that HCURSOR instances do need to be closed. > Anyway, MSDN is confusion about it. From LoadCursor() documentation it looks > like we should expect the same handle to be returned every time, so it doesn't > need to be closed. I agree, there is no information on MSDN (at least I have not found one) about the type of HCURSOR, and CloseHandle function does not cover a cursor (https://goo.gl/ja5ycW).
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1959863002/#ps80001 (title: "More comments about why we do not CloseHandle on an HCURSOR.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1959863002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1959863002/80001
Message was sent while issue was closed.
Description was changed from ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178, 428886 ========== to ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178, 428886 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178, 428886 ========== to ========== CURSORINFO.flags should be checked before capturing its bitmap BUG=566178, 428886 Committed: https://crrev.com/99f8cd0ae2f05c24410ddd6b2d9ea3d56f1fec5f Cr-Commit-Position: refs/heads/master@{#12739} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/99f8cd0ae2f05c24410ddd6b2d9ea3d56f1fec5f Cr-Commit-Position: refs/heads/master@{#12739} |
