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

[Lambros, 2016/05/21 00:57:00]

Update this comment.

[rkjnsn, 2016/05/24 00:52:34]

Done.

 // the client resolution exactly. To support best-resolution matching, it would
 // be necessary to implement |GetSupportedResolutions|, but it's not considered
 // a priority now.
 //
 // Xrandr has a number of restrictions that make this code 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
@@ skipping 100 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 SetResolutionExact(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 SetResolutionRandr(const ScreenResolution& resolution);

[rkjnsn, 2016/05/20 22:08:40]

Are these decent names for these functions?

[Jamie, 2016/05/20 23:56:19]

Since they both use RANDR, maybe SetResolutionNewMode and
SetResolutionExistingMode might be better choices.

[rkjnsn, 2016/05/24 00:52:34]

Done.

+
   // 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_;

[rkjnsn, 2016/05/20 22:08:39]

We don't currently use either of these, so it might make sense to discard the
values returned by XRRQueryExtension until we need to do something that actually
makes use of them.

[Jamie, 2016/05/20 23:56:19]

SGTM.

[rkjnsn, 2016/05/24 00:52:34]

Done.

+  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")) {
+                    HasSwitch("server-supports-exact-resize")),
+      // TODO(rkjnsn): Do we need to handle the case where the display failed to
+      // open? It doesn't look like we do, elsewhere.

[rkjnsn, 2016/05/20 22:08:40]

It appears we generally assume that XOpenDisplay succeeds, and thus display_,
screen_, and root_ are all always valid, so I do the same here, for now.

[Jamie, 2016/05/20 23:56:19]

If this is standard elsewhere, then it's okay to do it here (and you don't need
a TODO).

[rkjnsn, 2016/05/24 00:52:34]

Done.

+      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.
+  // TODO(rkjnsn): Don't DisplayWidth and DisplayHeight work even without RandR?
+  // Would it make sense to return the size here, even when RandR isn't present?
+  // (Obviously we'd want to skip the XRRUpdateConfiguration loop.)

[rkjnsn, 2016/05/20 22:08:39]

I think we could return something meaningful here even when XRandR isn't
supported, as I believe DisplayWidth and DisplayHeight are base Xlib features,
and not RandR specific. Of course, the XRRUpdateConfiguration loop in RandR
specific, but that should be fine since if RandR isn't supported, there won't be
RandR events that need to be processed to update DisplayWidth and DisplayHeight.

Would returning something here when we don't support resize confuse logic
elsewhere?

[Jamie, 2016/05/20 23:56:19]

I'll defer to Lambros here, given his TODO

[Lambros, 2016/05/21 00:57:00]

I can't remember why we return an invalid resolution. Makes sense to fix this to
return the actual resolution in all cases.

[rkjnsn, 2016/05/24 00:52:34]

Done.

+  if (!has_randr_) {
     return ScreenResolution();
   }
 
+  // TODO(rkjnsn): What's the todo, here? To investigate another approach? To
+  // revisit if we ever get a central X event loop?
   // 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.

[rkjnsn, 2016/05/20 22:08:40]

I wasn't sure what the actual TODO item is, here, and at what point it would
make sense to do it.

[Lambros, 2016/05/21 00:57:00]

I don't know what the TODO is, either :)
The problem seems to be that we're trying to use multiple X connections on the
same thread, which is why we have to manually pump the message queue.
The proper fixes are to either:
* Pass in an existing Display*, instead of opening a new connection.
* Open a new X connection on a new thread, and make it a UI thread that pumps X
messages.

I'm not exactly sure what blocks us from doing either of these. In any case,
we'd have to hook into the message-loop to spy on X messages and pass the
"relevant" ones into XRRUpdateConfiguration().

[Sergey Ulanov, 2016/05/22 20:00:17]

I don't think there are any other X connections used on the main thread. Input
injector and screen capturer both use their own X display on their own threads
(input injector actually uses two x connections: one for input injection, and
one for clipboard)
Potentially we could share this X connection with the input injector, but that
won't do anything about this TODO as we will still need to pump the events
somewhere.

[rkjnsn, 2016/05/24 00:52:34]

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?

   //
   // 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.
   while (XEventsQueued(display_, QueuedAlready)) {
     XEvent event;
     XNextEvent(display_, &event);
     XRRUpdateConfiguration(&event);
   }
 
   ScreenResolution result(
       webrtc::DesktopSize(
           DisplayWidth(display_, DefaultScreen(display_)),
           DisplayHeight(display_, DefaultScreen(display_))),
+      // TODO(rkjnsn): Do we want to calculate this from the screen dimensions?

[rkjnsn, 2016/05/20 22:08:39]

We seem to assume 96 DPI everywhere. Would it make sense to calculate the
actually DPI when we're not in curtain mode?

[Jamie, 2016/05/20 23:56:19]

I wouldn't worry about it for now, unless it's a really trivial change. AFAIK,
no-one is using this for non-virtual sessions, since we don't make it easy to do
so and it's not supported.

[Sergey Ulanov, 2016/05/22 20:00:17]

This code is used only with Me2Me where we only support standard DPI at the
moment. The DPI that's reported to the client comes from ScreenCapturer.
ScreenCapturerLinux currently doesn't return DPI for captured frames, so the
client gets 96x96. Ideally we should fix it (in ScreenCapturerLinux and here).
But it would be wrong to use physical screen dimensions to calculate DPI.
Applications don't scale their content according to the screen dimensions and
it's the only thing that matters here. I think we will want something like this:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[rkjnsn, 2016/05/24 00:52:34]

Acknowledged.

       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

[Lambros, 2016/05/21 00:57:00]

Maybe add a blank line before this comment, though it's not part of this CL?

     // 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),
+            // TODO(rkjnsn): Do we want to calculate this from the screen

[Lambros, 2016/05/21 00:57:01]

nit: Blank line before comment.

+            // dimensions?
+            webrtc::DesktopVector(kDefaultDPI, kDefaultDPI)));
+      }
+
+      XRRFreeScreenConfigInfo(config);

[Jamie, 2016/05/20 23:56:19]

Use the ScreenResources class to manage the lifetime of these.

[Lambros, 2016/05/21 00:57:00]

I guess you'd need a separate scoper, with a dtor that calls this specific
XRRFree...() function? I think Chromium supports some way of writing custom
scopers like this, though I'm not sure it's worth the effort in this case?

[rkjnsn, 2016/05/24 00:52:34]

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.

+    }
   }
   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_) {
+    SetResolutionExact(resolution);
+  } else {
+    SetResolutionRandr(resolution);
+  }
+}
+
+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.
+
+  // TODO(rkjnsn): Previously we only changed the resolution while in curtain
+  // mode with exact resize support Since we now pick the best resolution
+  // wherever RandR is supported, presumably we want to actually do the restore
+  // if and only if RandR is supported and we are not in curtain mode.

[Jamie, 2016/05/20 23:56:19]

Agreed; that makes sense.

[rkjnsn, 2016/05/24 00:52:34]

Per our discussion, this now unconditionally calls SetResolution, and it will be
up to the cross-platform logic to avoid calling RestoreResolution when we are in
curtain mode. I'll probably put that in another CL, but we may want to wait
until it's ready before committing this one, as otherwise it would be a behavior
change on curtained Linux.

+}
+
+void DesktopResizerX11::SetResolutionExact(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::SetResolutionRandr(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);

[Lambros, 2016/05/21 00:57:00]

If you do a custom scoper for XRRFreeScreenConfigInfo() above, you might as well
do another one here?

+  }
 }
 
 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