Cross-platform plumbing for resize-to-client

remoting/host/desktop_resizer_linux.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>

[Lambros, 2012/09/13 22:00:35]

nit: #include <string.h> for strcmp

[Jamie, 2012/09/14 21:07:33]

Done.

+#include <X11/extensions/Xrandr.h>

[Lambros, 2012/09/13 22:00:35]

nit: alphabetize

[Jamie, 2012/09/14 21:07:33]

Done.

+
+#include "base/logging.h"
+
+// On Linux, we use the xrandr extension to change the desktop size. This has
+// a number of quirks that make the code more complex:
+//
+//   1. It's not possible to change the size 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 to create a new mode with the correct size, switch to
+// it, then delete the old one. Since the new mode must have a different name,
+// and we want the current mode name to be consistent, we then recreate the old
+// mode at the new size, switch to it and delete the temporary. The reason for
+// name consistency is to allow a future CL to disable resize-to-client if the
+// user has changed the mode to something other than "Chrome Remote Desktop
+// client size"). It doesn't make the code significantly more complex.
+
+namespace remoting {
+
+namespace {
+// Temporarily install an alternative handler for X errors. The default handler
+// exits the process, which is not what we want.
+// TODO(jamiewalch): This doesn't really belong here, and should provide some
+// means of checking whether or not anything went wrong.
+class ScopedXErrorHandler {
+ public:
+  typedef int (*Handler)(Display*, XErrorEvent*);
+  ScopedXErrorHandler(Handler handler) {
+    previous_handler_ = XSetErrorHandler(handler);
+  }
+  ~ScopedXErrorHandler() {
+    XSetErrorHandler(previous_handler_);
+  }
+
+  static int Ignore(Display* display, XErrorEvent* event) {
+    LOG(INFO) << "Ignoring error.";
+    return 0;
+  }
+
+ private:
+  Handler previous_handler_;

[Lambros, 2012/09/13 22:00:35]

nit: DISALLOW_COPY_AND_ASSIGN

[Jamie, 2012/09/14 21:07:33]

Done.

+};
+
+class ScopedXGrabServer {
+ public:
+  ScopedXGrabServer(Display* display)
+      : display_(display) {
+    XGrabServer(display_);
+  }
+  ~ScopedXGrabServer() {
+    XUngrabServer(display_);
+    XFlush(display_);
+  }
+ private:
+  Display* display_;

[Lambros, 2012/09/13 22:00:35]

nit: DISALLOW_COPY_AND_ASSIGN

[Jamie, 2012/09/14 21:07:33]

Done.

+};
+
+// Wrapper class for the XRRScreenResources struct.
+class ScreenResources {
+ public:
+  ScreenResources() : resources_(NULL) {
+  }
+
+  ~ScreenResources() {
+    Release();
+  }
+
+  bool Refresh(Display* display, Window window) {
+    Release();
+    resources_ = XRRGetScreenResources(display, window);
+    return resources_ != NULL;
+  }
+
+  void Release() {
+    if (resources_) {
+      XRRFreeScreenResources(resources_);
+      resources_ = NULL;
+    }
+  }
+
+  RRMode GetIdForMode(const char* name) {
+    CHECK(resources_);
+    for (int i = 0; i < resources_->nmode; ++i) {
+      const XRRModeInfo& mode = resources_->modes[i];
+      if (strcmp(mode.name, name) == 0) {
+        return mode.id;
+      }
+    }
+    return 0;
+  }
+
+  // For now, assume we're only ever interested in the first output.
+  RROutput GetOutput() {
+    CHECK(resources_);
+    return resources_->outputs[0];
+  }
+
+  // For now, assume we're only ever interested in the first crtc.
+  RRCrtc GetCrtc() {
+    CHECK(resources_);
+    return resources_->crtcs[0];
+  }
+
+  XRROutputInfo* GetOutputInfo(Display* display, RROutput output_id) {
+    CHECK(resources_);
+    return XRRGetOutputInfo(display, resources_, output_id);
+  }
+
+  XRRScreenResources* get() { return resources_; }
+
+ private:
+  XRRScreenResources* resources_;
+};
+
+
+class DesktopResizerLinux : public DesktopResizer {
+ public:
+  DesktopResizerLinux();
+  virtual ~DesktopResizerLinux();
+
+  // DesktopResizer interface
+  virtual SkISize GetSize() OVERRIDE;
+  virtual void SetSize(const SkISize& size) OVERRIDE;
+
+ private:
+  // 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.

[Lambros, 2012/09/13 22:00:35]

Please could you explain what this does if |name| is NULL?  I don't understand
what happens in this case, or why you do it, or what the ramifications are.

[Jamie, 2012/09/14 21:07:33]

TBH, neither do I, but xrandr does it, and you get Xlib errors if you don't.
There's a comment below where I call this with NULL, but I'll add something here
as well.

+  void SwitchToMode(const char* name);
+
+  Display* display_;
+  Window root_;
+  ScreenResources resources_;
+
+  DISALLOW_COPY_AND_ASSIGN(DesktopResizerLinux);
+};
+}  // namespace

[Lambros, 2012/09/13 22:00:35]

nit: Blank line before }  // namespace ?

[Jamie, 2012/09/14 21:07:33]

Done.

+
+DesktopResizerLinux::DesktopResizerLinux()
+    : display_(XOpenDisplay(NULL)),
+      root_(RootWindow(display_, DefaultScreen(display_))) {
+}
+
+DesktopResizerLinux::~DesktopResizerLinux() {
+  XCloseDisplay(display_);
+}
+
+SkISize DesktopResizerLinux::GetSize() {
+  ScopedXErrorHandler handler(ScopedXErrorHandler::Ignore);
+  SkISize result = SkISize::Make(0, 0);
+  int num_sizes;
+  XRRScreenSize *sizes = XRRSizes(display_, 0, &num_sizes);
+  // If xrandr is not supported, num_sizes is set to zero.
+  if (num_sizes) {
+    XRRScreenConfiguration *config = XRRGetScreenInfo(display_, root_);
+    Rotation rotation;
+    SizeID size_id = XRRConfigCurrentConfiguration(config, &rotation);
+    CHECK(size_id < num_sizes);
+    result = SkISize::Make(sizes[size_id].width, sizes[size_id].height);
+  }
+  return result;
+}
+
+void DesktopResizerLinux::SetSize(const SkISize& size) {
+  // 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 size" 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 size. This ensures
+  // that the display configuration doesn't change under our feet.
+  ScopedXGrabServer grabber(display_);

[Lambros, 2012/09/13 22:00:35]

nit: I think it's slightly better to put the scoped error handler inside the
scoped GrabServer.  That way, we change and restore the error-handler before
anyone else sees anything.

[Jamie, 2012/09/14 21:07:33]

I did it this way so that errors from XGrabServer are trapped as well. I don't
expect to see any, but the failure mode is severe enough that I wanted to guard
against it.

+
+  // Clamp the specified size to something valid for the X server.
+  int width = size.width(), height = size.height();
+  int min_width = 0, min_height = 0, max_width = 0, max_height = 0;
+  XRRGetScreenSizeRange(display_, root_,
+                        &min_width, &min_height,
+                        &max_width, &max_height);
+  if (size.width() < min_width) {

[Lambros, 2012/09/13 22:00:35]

nit: How about
  width = std::max(width, min_width);
  width = std::min(width, max_width);
?
In any case, s/size.width()/width/ for readability.

[Jamie, 2012/09/14 21:07:33]

Done.

+    width = min_width;
+  } else if (width > max_width) {
+    width = max_width;
+  }
+  if (height < min_height) {
+    height = min_height;
+  } else if (height > max_height) {
+    height = max_height;
+  }
+  LOG(INFO) << "Changing desktop size to " << width << "x" << height;
+
+  // The name of the mode representing the current client view size 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 size";
+  const char* kTempModeName = "Chrome Remote Desktop temporary mode";
+
+  // Actually do the resize operation, preserving the current mode name. Note
+  // that we have to detatch the output from any mode in order to resize it

[Lambros, 2012/09/13 22:00:35]

nit: detatch -> detach

[Jamie, 2012/09/14 21:07:33]

Done.

+  // (strictly speaking, this is only required when reducing the size, but it
+  // seems safe to do it regardless).

[Lambros, 2012/09/13 22:00:35]

Could you explain why we need both SwitchToMode and XRRSetScreenSize?  Maybe
SwitchToMode could be renamed SetCrtc if it only sets a new Crtc without
actually switching to the new resolution?

[Jamie, 2012/09/14 21:07:33]

I don't understand why both are required, and there's no documentation. This is
how xrandr does it. If you don't call XRRSetScreenSize, then the status bars all
resize, but the desktop itself does not. If you call XRRSetScreenSize without
setting the mode, nothing appears to happen.

+  CreateMode(kTempModeName, width, height);
+  SwitchToMode(NULL);
+  XRRSetScreenSize(display_, root_, width, height, -1, -1);
+  SwitchToMode(kTempModeName);
+  DeleteMode(kModeName);
+  CreateMode(kModeName, width, height);
+  SwitchToMode(kModeName);

[Lambros, 2012/09/13 22:00:35]

Does this actually work if kModeName is the same resolution as kTempModeName?  I
thought the X Server would stay on kTempModeName if the sizes matched, and then
we wouldn't be able to delete the temporary mode.  Am I missing something?

[Jamie, 2012/09/14 21:07:33]

Yes, this works because you set the mode by explicit index.

+  DeleteMode(kTempModeName);
+}
+
+void DesktopResizerLinux::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);
+
+  if (!resources_.Refresh(display_, root_)) {
+    return;
+  }
+  RRMode mode_id = resources_.GetIdForMode(name);
+  if (!mode_id) {
+    return;
+  }
+  XRRAddOutputMode(display_, resources_.GetOutput(), mode_id);
+}
+
+void DesktopResizerLinux::DeleteMode(const char* name) {
+  RRMode mode_id = resources_.GetIdForMode(name);
+  if (mode_id) {
+    XRRDeleteOutputMode(display_, resources_.GetOutput(), mode_id);
+    XRRDestroyMode(display_, mode_id);
+    resources_.Refresh(display_, root_);
+  }
+}
+
+void DesktopResizerLinux::SwitchToMode(const char* name) {
+  RRMode mode_id = None;
+  RROutput* outputs = NULL;
+  int number_of_outputs = 0;

[Lambros, 2012/09/13 22:00:35]

Not that it matters, but I think num_outputs would be fine.  I know we're not
supposed to abbreviate, but Chrome uses num_ prefix all over the place :)

[Jamie, 2012/09/14 21:07:33]

It doesn't cause any unnecessary word-wrap, so I'll leave it for now.

+  if (name) {
+    mode_id = resources_.GetIdForMode(name);
+    CHECK(mode_id);
+    outputs = resources_.get()->outputs;
+    number_of_outputs = resources_.get()->noutput;
+  }
+  XRRSetCrtcConfig(display_, resources_.get(), resources_.GetCrtc(),
+                   CurrentTime, 0, 0, mode_id, 1, outputs, number_of_outputs);
+}
+
+scoped_ptr<DesktopResizer> DesktopResizer::Create() {
+  return scoped_ptr<DesktopResizer>(new DesktopResizerLinux);
+}
+
+}  // namespace remoting