Add support for inexact resize on X11

remoting/host/desktop_resizer_x11.cc

Index: remoting/host/desktop_resizer_x11.cc
diff --git a/remoting/host/desktop_resizer_x11.cc b/remoting/host/desktop_resizer_x11.cc
index e59fa2c10f104023600bfe51e82fd6097f9fb62f..b22dcd88930a1b6d55147eeecbfa8891d62b570d 100644
--- a/remoting/host/desktop_resizer_x11.cc
+++ b/remoting/host/desktop_resizer_x11.cc
@@ -136,6 +136,13 @@ class DesktopResizerX11 : public DesktopResizer {
   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);
@@ -154,6 +161,9 @@ class DesktopResizerX11 : public DesktopResizer {
   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);
 };
@@ -163,7 +173,11 @@ DesktopResizerX11::DesktopResizerX11()
       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);
 }
 
@@ -172,12 +186,15 @@ DesktopResizerX11::~DesktopResizerX11() {
 }
 
 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
@@ -197,6 +214,7 @@ ScreenResolution DesktopResizerX11::GetCurrentResolution() {
       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;
 }
@@ -220,17 +238,29 @@ std::list<ScreenResolution> DesktopResizerX11::GetSupportedResolutions(
         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;
   }
 
@@ -244,6 +274,26 @@ void DesktopResizerX11::SetResolution(const ScreenResolution& resolution) {
   // 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
@@ -276,11 +326,25 @@ void DesktopResizerX11::SetResolution(const ScreenResolution& resolution) {
   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) {