CURSORINFO.flags should be checked before capturing its bitmap

webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/desktop_capture/mouse_cursor_monitor.h"
 
 #include <assert.h>
+#include <string.h>
 
 #include <memory>
 
 #include "webrtc/modules/desktop_capture/desktop_frame.h"
+#include "webrtc/modules/desktop_capture/desktop_geometry.h"
 #include "webrtc/modules/desktop_capture/mouse_cursor.h"
 #include "webrtc/modules/desktop_capture/win/cursor.h"
 #include "webrtc/modules/desktop_capture/win/window_capture_utils.h"
 #include "webrtc/system_wrappers/include/logging.h"
 
 namespace webrtc {
 
+namespace {
+
+bool Equal(const CURSORINFO& left, const CURSORINFO& right) {

[Sergey Ulanov, 2016/05/12 23:23:33]

Rename this to IsSameCursorShape(). This function verifies that the shape is the
same, not that the CURSORINFO are equal (it doesn't check position)

[Hzj_jie, 2016/05/13 01:05:28]

Done.

+  if (left.flags == right.flags) {

[Sergey Ulanov, 2016/05/12 23:23:33]

You can express the whole function with a single return statement:
 return left.flags == right.flags && 
        (left.flags != CURSOR_SHOWING ||
         left.hCursor == right.hCursor);

[Hzj_jie, 2016/05/13 01:05:28]

Done.

+    // If the cursors are not showing, we do not care the hCursor handle.
+    return left.flags != CURSOR_SHOWING ||
+           left.hCursor == right.hCursor;
+  }
+  return false;
+}
+
+}  // namespace
+
 class MouseCursorMonitorWin : public MouseCursorMonitor {
  public:
   explicit MouseCursorMonitorWin(HWND window);
   explicit MouseCursorMonitorWin(ScreenId screen);
   virtual ~MouseCursorMonitorWin();
 
   void Init(Callback* callback, Mode mode) override;
   void Capture() override;
 
  private:
   // Get the rect of the currently selected screen, relative to the primary
   // display's top-left. If the screen is disabled or disconnected, or any error
   // happens, an empty rect is returned.
   DesktopRect GetScreenRect();
 
   HWND window_;
   ScreenId screen_;
 
   Callback* callback_;
   Mode mode_;
 
   HDC desktop_dc_;
 
-  HCURSOR last_cursor_;
+  // The last CURSORINFO (converted to MouseCursor) we have sent to the client.
+  CURSORINFO last_cursor_;
 };
 
 MouseCursorMonitorWin::MouseCursorMonitorWin(HWND window)
     : window_(window),
       screen_(kInvalidScreenId),
       callback_(NULL),
       mode_(SHAPE_AND_POSITION),
-      desktop_dc_(NULL),
-      last_cursor_(NULL) {
+      desktop_dc_(NULL) {
+  memset(&last_cursor_, 0, sizeof(CURSORINFO));
 }
 
 MouseCursorMonitorWin::MouseCursorMonitorWin(ScreenId screen)
     : window_(NULL),
       screen_(screen),
       callback_(NULL),
       mode_(SHAPE_AND_POSITION),
-      desktop_dc_(NULL),
-      last_cursor_(NULL) {
+      desktop_dc_(NULL) {
   assert(screen >= kFullDesktopScreenId);
+  memset(&last_cursor_, 0, sizeof(CURSORINFO));
 }
 
 MouseCursorMonitorWin::~MouseCursorMonitorWin() {
   if (desktop_dc_)
     ReleaseDC(NULL, desktop_dc_);
 }
 
 void MouseCursorMonitorWin::Init(Callback* callback, Mode mode) {
   assert(!callback_);
   assert(callback);
 
   callback_ = callback;
   mode_ = mode;
 
   desktop_dc_ = GetDC(NULL);
 }
 
 void MouseCursorMonitorWin::Capture() {
   assert(callback_);
 
   CURSORINFO cursor_info;
   cursor_info.cbSize = sizeof(CURSORINFO);
   if (!GetCursorInfo(&cursor_info)) {
     LOG_F(LS_ERROR) << "Unable to get cursor info. Error = " << GetLastError();
     return;
   }
 
-  if (last_cursor_ != cursor_info.hCursor) {
-    last_cursor_ = cursor_info.hCursor;
-    // Note that |cursor_info.hCursor| does not need to be freed.
-    std::unique_ptr<MouseCursor> cursor(
-        CreateMouseCursorFromHCursor(desktop_dc_, cursor_info.hCursor));
-    if (cursor.get())
-      callback_->OnMouseCursor(cursor.release());
+  if (!Equal(cursor_info, last_cursor_)) {
+    if (cursor_info.flags == CURSOR_SUPPRESSED) {
+      // The cursor is intentionally hidden now, send an empty bitmap.
+      last_cursor_ = cursor_info;
+      callback_->OnMouseCursor(new MouseCursor(
+          new BasicDesktopFrame(DesktopSize()), DesktopVector()));
+    } else {
+      // Note that |cursor_info.hCursor| does not need to be freed.

[Sergey Ulanov, 2016/05/12 23:23:33]

move this comment below, where we call CreateMouseCursorFromHCursor()

[Hzj_jie, 2016/05/13 01:05:28]

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.

[Sergey Ulanov, 2016/05/13 21:32:40]

Please don't use shortn links in code reviews - these links don't work
externally.
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.

+      if (cursor_info.flags == 0) {
+        // Host machine does not have a hardware mouse attached, we will send a
+        // default one instead.
+        // Note, Windows automatically caches cursor resource, so we do not need
+        // to cache the result of LoadCursor.
+        cursor_info.hCursor = LoadCursor(nullptr, IDC_ARROW);

[Sergey Ulanov, 2016/05/12 23:23:33]

I think you also need to CloseHandle() for the handle returned by LoadCursor()

[Hzj_jie, 2016/05/13 01:05:28]

Done.

+      }
+      std::unique_ptr<MouseCursor> cursor(
+          CreateMouseCursorFromHCursor(desktop_dc_, cursor_info.hCursor));
+      if (cursor) {
+        last_cursor_ = cursor_info;
+        callback_->OnMouseCursor(cursor.release());
+      }
+    }
   }
 
   if (mode_ != SHAPE_AND_POSITION)
     return;
 
   DesktopVector position(cursor_info.ptScreenPos.x, cursor_info.ptScreenPos.y);
   bool inside = cursor_info.flags == CURSOR_SHOWING;
 
   if (window_) {
     DesktopRect original_rect;
@@ skipping 54 matching lines @@
   return new MouseCursorMonitorWin(reinterpret_cast<HWND>(window));
 }
 
 MouseCursorMonitor* MouseCursorMonitor::CreateForScreen(
     const DesktopCaptureOptions& options,
     ScreenId screen) {
   return new MouseCursorMonitorWin(screen);
 }
 
 }  // namespace webrtc