[Chromoting] Fix mouse coordinates 2nd display with mixed-dpi environment.

This removes the code from input_injector_mac.cc that tried to
calculate the correct offset from the desktop configuration and passes
the correct (adjusted) coordinates.

Bug: 946001
Change-Id: Iedea08a9d019c0fcdfa615b1e83a88ed4474f2c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2029448Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738659}

remoting/host/client_session.cc

@@ -552,20 +552,23 @@ void ClientSession::SetMouseClampingFilter(const DisplaySize& size) {
       break;
 
     case protocol::SessionConfig::Protocol::WEBRTC: {
+#if defined(OS_MACOSX)
+      mouse_clamping_filter_.set_input_size(size.WidthAsPixels(),
+                                            size.HeightAsPixels());
+#else
       // When using the WebRTC protocol the client sends mouse coordinates in
       // DIPs, while InputInjector expects them in physical pixels.
       // TODO(sergeyu): Fix InputInjector implementations to use DIPs as well.
       mouse_clamping_filter_.set_input_size(size.WidthAsDips(),
                                             size.HeightAsDips());
+#endif  // defined(OS_MACOSX)
     }
   }
 }
 
 void ClientSession::UpdateMouseClampingFilterOffset() {
   webrtc::DesktopVector origin;
-  if (show_display_id_ != webrtc::kFullDesktopScreenId) {
-    origin = desktop_display_info_.CalcDisplayOffset(show_display_id_);
-  }
+  origin = desktop_display_info_.CalcDisplayOffset(show_display_id_);
   mouse_clamping_filter_.set_output_offset(origin);
 }
 
@@ -704,6 +707,11 @@ void ClientSession::OnDesktopDisplayChanged(
               << display.y_dpi() << "]";
   }
 
+  // We need to update the input filters whenever the displays change.
+  DisplaySize display_size =
+      DisplaySize::FromPixels(size.width(), size.height(), default_x_dpi_);
+  SetMouseClampingFilter(display_size);
+
   connection_->client_stub()->SetVideoLayout(layout);
 }
 

remoting/host/desktop_display_info.cc

@@ -72,8 +72,13 @@ const DisplayGeometry* DesktopDisplayInfo::GetDisplayInfo(unsigned int id) {
   return &displays_[id];
 }
 
-// Calculate the offset from the upper-left of the desktop to the origin of
-// the specified display.
+// Calculate the offset from the origin of the desktop to the origin of the
+// specified display.
+//
+// For Mac, the origin of the desktop is the origin of the default display.
+//
+// For Windows/Linux, the origin of the desktop is the upper-left of the
+// entire desktop region.
 //
 // x         b-----------+            ---
 //           |           |             |  y-offset to c
@@ -89,13 +94,31 @@ const DisplayGeometry* DesktopDisplayInfo::GetDisplayInfo(unsigned int id) {
 // x = upper left of desktop
 // a,b,c = origin of display A,B,C
 webrtc::DesktopVector DesktopDisplayInfo::CalcDisplayOffset(
-    unsigned int disp_id) {
-  if (disp_id >= displays_.size()) {
-    LOG(INFO) << "Invalid display id for CalcDisplayOffset: " << disp_id;
+    webrtc::ScreenId disp_id) {
+  bool full_desktop = (disp_id == webrtc::kFullDesktopScreenId);
+  unsigned int disp_index = disp_id;
+
+  if (full_desktop) {
+#if defined(OS_MACOSX)
+    // For Mac, we need to calculate the offset relative to the default
+    // display.
+    disp_index = 0;
+#else
+    // For other platforms, the origin for full desktop is 0,0.
     return webrtc::DesktopVector();
+#endif  // !defined(OS_MACOSX)
   }
 
-  DisplayGeometry disp_info = displays_[disp_id];
+  if (displays_.size() == 0) {
+    LOG(INFO) << "No display info available";
+    return webrtc::DesktopVector();
+  }
+  if (disp_index >= displays_.size()) {
+    LOG(INFO) << "Invalid display id for CalcDisplayOffset: " << disp_index;
+    return webrtc::DesktopVector();
+  }
+
+  DisplayGeometry disp_info = displays_[disp_index];
   webrtc::DesktopVector origin(disp_info.x, disp_info.y);
 
   // Find topleft-most display coordinate. This is the topleft of the desktop.
@@ -109,7 +132,21 @@ webrtc::DesktopVector DesktopDisplayInfo::CalcDisplayOffset(
       dy = disp.y;
   }
   webrtc::DesktopVector topleft(dx, dy);
+
+#if defined(OS_MACOSX)
+  // Mac display offsets need to be relative to the main display's origin.
+  if (full_desktop) {
+    // For full desktop, this is the offset to the topleft display coord.
+    return topleft;
+  } else {
+    // For single displays, this offset is stored in the DisplayGeometry
+    // x,y values.
+    return origin;
+  }
+#else
+  // Return offset to this screen, relative to topleft.
   return origin.subtract(topleft);
+#endif  // defined(OS_MACOSX)
 }
 
 void DesktopDisplayInfo::AddDisplay(DisplayGeometry* display) {

remoting/host/desktop_display_info.h

@@ -9,6 +9,7 @@
 
 #include "base/logging.h"
 #include "remoting/proto/control.pb.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
 
 namespace remoting {
@@ -36,7 +37,8 @@ class DesktopDisplayInfo {
   int NumDisplays();
   const DisplayGeometry* GetDisplayInfo(unsigned int id);
 
-  webrtc::DesktopVector CalcDisplayOffset(unsigned int id);
+  // Calculate the offset to the origin (upper left) of the specific display.
+  webrtc::DesktopVector CalcDisplayOffset(webrtc::ScreenId id);
 
   // Add a new display with the given info to the display list.
   void AddDisplay(DisplayGeometry* display);

remoting/host/desktop_display_info_unittest.cc

@@ -104,8 +104,13 @@ TEST_F(DesktopDisplayInfoTest, MAYBE_DualDisplayLeft) {
   AddDisplay(0, 0, 500, 400);
   AddDisplay(-300, 0, 300, 200);
 
+#if defined(OS_MACOSX)
+  VerifyDisplayOffset(FROM_HERE, 0, 0, 0);
+  VerifyDisplayOffset(FROM_HERE, 1, -300, 0);
+#else
   VerifyDisplayOffset(FROM_HERE, 0, 300, 0);
   VerifyDisplayOffset(FROM_HERE, 1, 0, 0);
+#endif  // defined(OS_MACOSX)
 }
 
 // +---------o------------+
@@ -117,8 +122,13 @@ TEST_F(DesktopDisplayInfoTest, MAYBE_DualDisplayLeft_ReverseOrder) {
   AddDisplay(-300, 0, 300, 200);
   AddDisplay(0, 0, 500, 400);
 
+#if defined(OS_MACOSX)
+  VerifyDisplayOffset(FROM_HERE, 0, -300, 0);
+  VerifyDisplayOffset(FROM_HERE, 1, 0, 0);
+#else
   VerifyDisplayOffset(FROM_HERE, 0, 0, 0);
   VerifyDisplayOffset(FROM_HERE, 1, 300, 0);
+#endif  // defined(OS_MACOSX)
 }
 
 // +---------o------------+
@@ -131,9 +141,15 @@ TEST_F(DesktopDisplayInfoTest, MAYBE_TripleDisplayMiddle) {
   AddDisplay(0, 0, 500, 400);  // Default display.
   AddDisplay(500, 50, 400, 350);
 
+#if defined(OS_MACOSX)
+  VerifyDisplayOffset(FROM_HERE, 0, -300, 0);
+  VerifyDisplayOffset(FROM_HERE, 1, 0, 0);
+  VerifyDisplayOffset(FROM_HERE, 2, 500, 50);
+#else
   VerifyDisplayOffset(FROM_HERE, 0, 0, 0);
   VerifyDisplayOffset(FROM_HERE, 1, 300, 0);
   VerifyDisplayOffset(FROM_HERE, 2, 800, 50);
+#endif  // defined(OS_MACOSX)
 }
 
 //  x         o-----------+            - 0
@@ -150,9 +166,15 @@ TEST_F(DesktopDisplayInfoTest, MAYBE_Multimon3) {
   AddDisplay(300, 400, 600, 450);
   AddDisplay(-300, 350, 300, 200);
 
+#if defined(OS_MACOSX)
+  VerifyDisplayOffset(FROM_HERE, 0, 0, 0);
+  VerifyDisplayOffset(FROM_HERE, 1, 300, 400);
+  VerifyDisplayOffset(FROM_HERE, 2, -300, 350);
+#else
   VerifyDisplayOffset(FROM_HERE, 0, 300, 0);
   VerifyDisplayOffset(FROM_HERE, 1, 600, 400);
   VerifyDisplayOffset(FROM_HERE, 2, 0, 350);
+#endif  // defined(OS_MACOSX)
 }
 
 //  x                     +-------+               -- -50
@@ -181,6 +203,16 @@ TEST_F(DesktopDisplayInfoTest, MAYBE_Multimon7) {
   AddDisplay(70, 100, 65, 20);
   AddDisplay(0, 0, 80, 55);  // Default display.
 
+#if defined(OS_MACOSX)
+  // Relative to display 6.
+  VerifyDisplayOffset(FROM_HERE, 0, 80, -10);
+  VerifyDisplayOffset(FROM_HERE, 1, 60, -50);
+  VerifyDisplayOffset(FROM_HERE, 2, -70, 40);
+  VerifyDisplayOffset(FROM_HERE, 3, 135, 50);
+  VerifyDisplayOffset(FROM_HERE, 4, -40, -20);
+  VerifyDisplayOffset(FROM_HERE, 5, 70, 100);
+  VerifyDisplayOffset(FROM_HERE, 6, 0, 0);
+#else
   VerifyDisplayOffset(FROM_HERE, 0, 150, 40);
   VerifyDisplayOffset(FROM_HERE, 1, 130, 0);
   VerifyDisplayOffset(FROM_HERE, 2, 0, 90);
@@ -188,6 +220,7 @@ TEST_F(DesktopDisplayInfoTest, MAYBE_Multimon7) {
   VerifyDisplayOffset(FROM_HERE, 4, 30, 30);
   VerifyDisplayOffset(FROM_HERE, 5, 140, 150);
   VerifyDisplayOffset(FROM_HERE, 6, 70, 50);
+#endif  // defined(OS_MACOSX)
 }
 
 }  // namespace remoting

remoting/host/input_injector_mac.cc

@@ -259,39 +259,7 @@ void InputInjectorMac::Core::InjectMouseEvent(const MouseEvent& event) {
   WakeUpDisplay();
 
   if (event.has_x() && event.has_y()) {
-    // On multi-monitor systems (0,0) refers to the top-left of the "main"
-    // display, whereas our coordinate scheme places (0,0) at the top-left of
-    // the bounding rectangle around all the displays, so we need to translate
-    // accordingly.
-
-    // Set the mouse position assuming single-monitor.
     mouse_pos_.set(event.x(), event.y());
-
-    // Fetch the desktop configuration.
-    // TODO(wez): Optimize this out, or at least only enumerate displays in
-    // response to display-changed events. VideoFrameCapturer's VideoFrames
-    // could be augmented to include native cursor coordinates for use by
-    // MouseClampingFilter, removing the need for translation here.
-    webrtc::MacDesktopConfiguration desktop_config =
-        webrtc::MacDesktopConfiguration::GetCurrent(
-            webrtc::MacDesktopConfiguration::TopLeftOrigin);
-
-    // Translate the mouse position into desktop coordinates.
-    mouse_pos_ = mouse_pos_.add(
-        webrtc::DesktopVector(desktop_config.pixel_bounds.left(),
-                              desktop_config.pixel_bounds.top()));
-
-    // Constrain the mouse position to the desktop coordinates.
-    mouse_pos_.set(
-       std::max(desktop_config.pixel_bounds.left(),
-           std::min(desktop_config.pixel_bounds.right(), mouse_pos_.x())),
-       std::max(desktop_config.pixel_bounds.top(),
-           std::min(desktop_config.pixel_bounds.bottom(), mouse_pos_.y())));
-
-    // Convert from pixel to Density Independent Pixel coordinates.
-    mouse_pos_.set(mouse_pos_.x() / desktop_config.dip_to_pixel_scale,
-                   mouse_pos_.y() / desktop_config.dip_to_pixel_scale);
-
     VLOG(3) << "Moving mouse to " << mouse_pos_.x() << "," << mouse_pos_.y();
   }
   if (event.has_button() && event.has_button_down()) {

remoting/protocol/mouse_input_filter.cc

@@ -30,35 +30,42 @@ void MouseInputFilter::InjectMouseEvent(const MouseEvent& event) {
     return;
   }
 
-  // We scale based on the maximum input & output coordinates, rather than the
-  // input and output sizes, so that it's possible to reach the edge of the
-  // output when up-scaling.  We also take care to round up or down correctly,
-  // which is important when down-scaling.
-  // After scaling, we offset by the output rect origin. This is normally (0,0),
-  // but will be non-zero when we are showing a single display.
+  // We scale based on the max input and output coordinates (which are equal
+  // to size-1), rather than the input and output sizes, so that it's possible
+  // to reach the edge of the output when up-scaling.  We also take care to
+  // round up or down correctly, which is important when down-scaling.
+  // After scaling, we offset by the output rect origin. This is normally
+  // (0,0), but may be non-zero when there are multiple displays and we are
+  // showing a single display.
+
   MouseEvent out_event(event);
   if (out_event.has_x()) {
-    int x = out_event.x() * x_output_;
-    x = (x + x_input_ / 2) / x_input_;
+    int x = out_event.x();
+    if (x_output_ != x_input_)
+      x = ((x * x_output_) + (x_input_ / 2)) / x_input_;
     out_event.set_x(output_offset_.x() + base::ClampToRange(x, 0, x_output_));
   }
   if (out_event.has_y()) {
-    int y = out_event.y() * y_output_;
-    y = (y + y_input_ / 2) / y_input_;
+    int y = out_event.y();
+    if (y_output_ != y_input_)
+      y = ((y * y_output_) + (y_input_ / 2)) / y_input_;
     out_event.set_y(output_offset_.y() + base::ClampToRange(y, 0, y_output_));
   }
-
   InputFilter::InjectMouseEvent(out_event);
 }
 
 void MouseInputFilter::set_input_size(const int32_t x, const int32_t y) {
   x_input_ = x - 1;
   y_input_ = y - 1;
+  LOG(INFO) << "Setting MouseInputFilter input_size to " << x_input_ << ","
+            << y_input_;
 }
 
 void MouseInputFilter::set_output_size(const int32_t x, const int32_t y) {
   x_output_ = x - 1;
   y_output_ = y - 1;
+  LOG(INFO) << "Setting MouseInputFilter output_size to " << x_output_ << ","
+            << y_output_;
 }
 
 void MouseInputFilter::set_output_offset(const webrtc::DesktopVector& v) {

remoting/protocol/mouse_input_filter.h

@@ -23,15 +23,19 @@ class MouseInputFilter : public InputFilter {
   explicit MouseInputFilter(InputStub* input_stub);
   ~MouseInputFilter() override;
 
-  // Specify the input dimensions (unitless) for mouse events.
-  // The input size can be in either pixels (for ICE protocol) or dips (for
-  // webrtc), depending on the units used by the protocol.
+  // Specify the input dimensions (DIPs or pixels) for mouse events.
+  // Deoending on the protocol, the input size can be in either pixels (for
+  // ICE protocol) or DIPs (for webrtc - except for Mac, which is pixels).
   void set_input_size(const int32_t x, const int32_t y);
 
-  // Specify the output dimensions (unitless).
+  // Specify the output dimensions (DIPs or pixels, depending on platform).
   // The output size is the scale that we should apply to the incoming mouse
-  // events before injecting them.
+  // events before injecting them using the platform native APIs.
   void set_output_size(const int32_t x, const int32_t y);
+
+  // Offset to display origin, in DIPs or pixels depending on the native
+  // platform. This will typically be (0,0), but may take on other values
+  // when there are multiple displays.
   void set_output_offset(const webrtc::DesktopVector& v);
 
   // InputStub overrides.