Add support for inexact resize on X11

remoting/host/desktop_resizer_x11.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/desktop_resizer.h"
 
 #include <X11/Xlib.h>
 #include <X11/extensions/Xrandr.h>
 #include <string.h>
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "remoting/base/logging.h"
 #include "remoting/host/linux/x11_util.h"
 
-// On Linux, we use the xrandr extension to change the desktop resolution. For
-// now, we only support resize-to-client for Xvfb-based servers that can match
-// the client resolution exactly. To support best-resolution matching, it would
-// be necessary to implement |GetSupportedResolutions|, but it's not considered
-// a priority now.
+// On Linux, we use the xrandr extension to change the desktop resolution. In
+// curtain mode, we do exact resize where supported (currently only using our

[Lambros, 2016/06/08 00:12:39]

"our" is a bit weird. It's not obvious (or desirable?) that Google is speaking
here?
Maybe use more neutral language, such as "... if the version of Xvfb is patched
to support it".

[rkjnsn, 2016/06/08 01:32:42]

I'll change it to read "(currently only using a patched Xvfb server)"

+// custom Xvfb server). Otherwise, we try to pick the best resolution from the
+// existing modes.
 //
-// Xrandr has a number of restrictions that make this code more complex:
+// Xrandr has a number of restrictions that make exact resize more complex:
 //
 //   1. It's not possible to change the resolution of an existing mode. Instead,
 //      the mode must be deleted and recreated.
 //   2. It's not possible to delete a mode that's in use.
 //   3. Errors are communicated via Xlib's spectacularly unhelpful mechanism
 //      of terminating the process unless you install an error handler.
 //
 // The basic approach is as follows:
 //
 //   1. Create a new mode with the correct resolution;
@@ skipping 95 matching lines @@
   ~DesktopResizerX11() override;
 
   // DesktopResizer interface
   ScreenResolution GetCurrentResolution() override;
   std::list<ScreenResolution> GetSupportedResolutions(
       const ScreenResolution& preferred) override;
   void SetResolution(const ScreenResolution& resolution) override;
   void RestoreResolution(const ScreenResolution& original) override;
 
  private:
+  // Add a mode matching the specified resolution and switch to it.
+  void SetResolutionNewMode(const ScreenResolution& resolution);
+
+  // Attempt to switch to an existing mode matching the specified resolution
+  // using RandR, if such a resolution exists. Otherwise, do nothing.
+  void SetResolutionExistingMode(const ScreenResolution& resolution);
+
   // Create a mode, and attach it to the primary output. If the mode already
   // exists, it is left unchanged.
   void CreateMode(const char* name, int width, int height);
 
   // Remove the specified mode from the primary output, and delete it. If the
   // mode is in use, it is not deleted.
   void DeleteMode(const char* name);
 
   // Switch the primary output to the specified mode. If name is nullptr, the
   // primary output is disabled instead, which is required before changing
   // its resolution.
   void SwitchToMode(const char* name);
 
   Display* display_;
   int screen_;
   Window root_;
   ScreenResources resources_;
   bool exact_resize_;
+  int rr_event_base_;
+  int rr_error_base_;
+  bool has_randr_;
 
   DISALLOW_COPY_AND_ASSIGN(DesktopResizerX11);
 };
 
 DesktopResizerX11::DesktopResizerX11()
     : display_(XOpenDisplay(nullptr)),
       screen_(DefaultScreen(display_)),
       root_(RootWindow(display_, screen_)),
       exact_resize_(base::CommandLine::ForCurrentProcess()->
                     HasSwitch("server-supports-exact-resize")) {
+  int rr_event_base;

[Lambros, 2016/06/08 00:12:39]

Don't need these, since you already have rr_event_base_ ... members.

[rkjnsn, 2016/06/08 01:32:42]

Oops. The goal was to avoid adding the rr_event_base_ and rr_error_base_
members, since we don't use them anywhere. Nice catch. (I'm actually surprised
this didn't throw an "unused private member" warning.)

+  int rr_error_base;
+
+  has_randr_ = XRRQueryExtension(display_, &rr_event_base, &rr_error_base);
+
   XRRSelectInput(display_, root_, RRScreenChangeNotifyMask);
 }
 
 DesktopResizerX11::~DesktopResizerX11() {
   XCloseDisplay(display_);
 }
 
 ScreenResolution DesktopResizerX11::GetCurrentResolution() {
-  if (!exact_resize_) {
-    // TODO(jamiewalch): Remove this early return if we decide to support
-    // non-Xvfb servers.
-    return ScreenResolution();
-  }
-
   // TODO(lambroslambrou): Xrandr requires that we process RRScreenChangeNotify
   // events, otherwise DisplayWidth and DisplayHeight do not return the current
   // values. Normally, this would be done via a central X event loop, but we
   // don't have one, hence this horrible hack.
   //
   // Note that the WatchFileDescriptor approach taken in XServerClipboard
   // doesn't work here because resize events have already been read from the
   // X server socket by the time the resize function returns, hence the
   // file descriptor is never seen as readable.

[rkjnsn, 2016/06/07 23:02:10]

Given the previous discussion, are there any specific action items that I should
notate in the code for this? Or would it make sense to remove "TODO" and leave
the rest as a normal comment?

[Jamie, 2016/06/07 23:40:11]

I think just removing the TODO part of the comment is fine.

-  while (XEventsQueued(display_, QueuedAlready)) {
-    XEvent event;
-    XNextEvent(display_, &event);
-    XRRUpdateConfiguration(&event);
+  if (has_randr_) {
+    while (XEventsQueued(display_, QueuedAlready)) {
+      XEvent event;
+      XNextEvent(display_, &event);
+      XRRUpdateConfiguration(&event);
+    }
   }
 
   ScreenResolution result(
       webrtc::DesktopSize(
-          DisplayWidth(display_, DefaultScreen(display_)),
-          DisplayHeight(display_, DefaultScreen(display_))),
+          DisplayWidth(display_, screen_),
+          DisplayHeight(display_, screen_)),
       webrtc::DesktopVector(kDefaultDPI, kDefaultDPI));
   return result;
 }
 
 std::list<ScreenResolution> DesktopResizerX11::GetSupportedResolutions(
     const ScreenResolution& preferred) {
   std::list<ScreenResolution> result;
   if (exact_resize_) {
     // Clamp the specified size to something valid for the X server.
     int min_width = 0, min_height = 0, max_width = 0, max_height = 0;
     XRRGetScreenSizeRange(display_, root_,
                           &min_width, &min_height,
                           &max_width, &max_height);
     int width = std::min(std::max(preferred.dimensions().width(), min_width),
                          max_width);
     int height = std::min(std::max(preferred.dimensions().height(), min_height),
                           max_height);
     // Additionally impose a minimum size of 640x480, since anything smaller
     // doesn't seem very useful.
     ScreenResolution actual(
         webrtc::DesktopSize(std::max(640, width), std::max(480, height)),
         webrtc::DesktopVector(kDefaultDPI, kDefaultDPI));
     result.push_back(actual);
-  } else {
-    // TODO(jamiewalch): Return the list of supported resolutions if we can't
-    // support exact-size matching.
+  } else if (has_randr_) {
+    // Retrieve supported resolutions with RandR
+    XRRScreenConfiguration *config = XRRGetScreenInfo(display_, root_);
+    if (config) {
+      int num_sizes = 0;
+      XRRScreenSize *sizes = XRRConfigSizes(config, &num_sizes);
+
+      for (int i = 0; i < num_sizes; ++i) {
+        result.push_back(ScreenResolution(
+            webrtc::DesktopSize(sizes[i].width, sizes[i].height),
+            webrtc::DesktopVector(kDefaultDPI, kDefaultDPI)));
+      }
+
+      XRRFreeScreenConfigInfo(config);

[rkjnsn, 2016/06/07 23:02:10]

As I noted before, I wouldn't be able to reuse ScreenResources for this. I'd
have to make my own ScreenConfig class. How much of an anathema is manually
freeing a resource like this in Chrome? I'll add a class for it if you think
it's worth the effort.

[Jamie, 2016/06/07 23:40:10]

No, I think this is fine. I didn't spot that it was a different cleanup
function.

+    }
   }
   return result;
 }
 
 void DesktopResizerX11::SetResolution(const ScreenResolution& resolution) {
-  if (!exact_resize_) {
-    // TODO(jamiewalch): Remove this early return if we decide to support
-    // non-Xvfb servers.
+  if (!has_randr_) {
     return;
   }
 
   // Ignore X errors encountered while resizing the display. We might hit an
   // error, for example if xrandr has been used to add a mode with the same
   // name as our temporary mode, or to remove the "client resolution" mode. We
   // don't want to terminate the process if this happens.
   ScopedXErrorHandler handler(ScopedXErrorHandler::Ignore());
 
   // Grab the X server while we're changing the display resolution. This ensures
   // that the display configuration doesn't change under our feet.
   ScopedXGrabServer grabber(display_);
 
+  if (exact_resize_) {
+    SetResolutionNewMode(resolution);
+  } else {
+    SetResolutionExistingMode(resolution);
+  }
+}
+
+void DesktopResizerX11::RestoreResolution(const ScreenResolution& original) {
+  SetResolution(original);
+}
+
+void DesktopResizerX11::SetResolutionNewMode(
+    const ScreenResolution& resolution) {
   // The name of the mode representing the current client view resolution and
   // the temporary mode used for the reasons described at the top of this file.
   // The former should be localized if it's user-visible; the latter only
   // exists briefly and does not need to localized.
   const char* kModeName = "Chrome Remote Desktop client resolution";
   const char* kTempModeName = "Chrome Remote Desktop temporary mode";
 
   // Actually do the resize operation, preserving the current mode name. Note
   // that we have to detach the output from any mode in order to resize it
   // (strictly speaking, this is only required when reducing the size, but it
@@ skipping 12 matching lines @@
   XRRSetScreenSize(display_, root_, resolution.dimensions().width(),
                    resolution.dimensions().height(), width_mm, height_mm);
   SwitchToMode(kTempModeName);
   DeleteMode(kModeName);
   CreateMode(kModeName, resolution.dimensions().width(),
              resolution.dimensions().height());
   SwitchToMode(kModeName);
   DeleteMode(kTempModeName);
 }
 
-void DesktopResizerX11::RestoreResolution(const ScreenResolution& original) {
-  // Since the desktop is only visible via a remote connection, the original
-  // resolution of the desktop will never been seen and there's no point
-  // restoring it; if we did, we'd just risk messing up the user's window
-  // layout.
+void DesktopResizerX11::SetResolutionExistingMode(
+    const ScreenResolution& resolution) {
+  XRRScreenConfiguration *config = XRRGetScreenInfo(display_, root_);
+  if (config) {
+    int num_sizes = 0;
+    XRRScreenSize *sizes = XRRConfigSizes(config, &num_sizes);
+    Rotation current_rotation = 0;
+    XRRConfigCurrentConfiguration(config, &current_rotation);
+
+    for (int i = 0; i < num_sizes; ++i) {
+      if (sizes[i].width == resolution.dimensions().width()
+          && sizes[i].height == resolution.dimensions().height()) {
+        XRRSetScreenConfig(display_, config, root_, i, current_rotation,
+                           CurrentTime);
+        break;
+      }
+    }
+
+    XRRFreeScreenConfigInfo(config);
+  }
 }
 
 void DesktopResizerX11::CreateMode(const char* name, int width, int height) {
   XRRModeInfo mode;
   memset(&mode, 0, sizeof(mode));
   mode.width = width;
   mode.height = height;
   mode.name = const_cast<char*>(name);
   mode.nameLength = strlen(name);
   XRRCreateMode(display_, root_, &mode);
@@ skipping 29 matching lines @@
   }
   XRRSetCrtcConfig(display_, resources_.get(), resources_.GetCrtc(),
                    CurrentTime, 0, 0, mode_id, 1, outputs, number_of_outputs);
 }
 
 std::unique_ptr<DesktopResizer> DesktopResizer::Create() {
   return base::WrapUnique(new DesktopResizerX11);
 }
 
 }  // namespace remoting