Add Xorg+dummy as alternative for Xvfb

remoting/host/linux/linux_me2me_host.py

Index: remoting/host/linux/linux_me2me_host.py
diff --git a/remoting/host/linux/linux_me2me_host.py b/remoting/host/linux/linux_me2me_host.py
index 8e9f753ce5a6bafb15d97a4c6408664928a5d454..5cc0a2ff7522e1a89ddc4e2ddccf5989d164b0d2 100755
--- a/remoting/host/linux/linux_me2me_host.py
+++ b/remoting/host/linux/linux_me2me_host.py
@@ -42,6 +42,11 @@ LOG_FILE_ENV_VAR = "CHROME_REMOTE_DESKTOP_LOG_FILE"
 # list of sizes in this environment variable.
 DEFAULT_SIZES_ENV_VAR = "CHROME_REMOTE_DESKTOP_DEFAULT_DESKTOP_SIZES"
 
+# By default, this script launches Xvfb as the virtual X display. When this
+# environment variable is set, the script will instead launch an instance of
+# Xorg using the dummy display driver and void input device.

[Jamie, 2016/05/07 00:05:47]

Add instructions here on how to install dummy and void (you can remove them from
the CL description if you like).

[rkjnsn, 2016/05/09 16:58:34]

It's obvious now that you mention it.

+USE_XORG_ENV_VAR = "CHROME_REMOTE_DESKTOP_USE_XORG"
+
 # By default, provide a maximum size that is large enough to support clients
 # with large or multiple monitors. This is a comma-separated list of
 # resolutions that will be made available if the X server supports RANDR. These
@@ -102,9 +107,9 @@ def is_supported_platform():
   return (distribution[0]).lower() == 'ubuntu'
 
 
-def get_randr_supporting_x_server():
-  """Returns a path to an X server that supports the RANDR extension, if this
-  is found on the system. Otherwise returns None."""
+def locate_xvfb_randr():
+  """Returns a path to our RANDR-supporting Xvfb server, if it is found on the
+  system. Otherwise returns None."""
 
   xvfb = "/usr/bin/Xvfb-randr"
   if os.path.exists(xvfb):
@@ -238,6 +243,7 @@ class Desktop:
     self.host_proc = None
     self.child_env = None
     self.sizes = sizes
+    self.xorg_conf = None
     self.pulseaudio_pipe = None
     self.server_supports_exact_resize = False
     self.host_ready = False
@@ -356,6 +362,114 @@ class Desktop:
     self.ssh_auth_sockname = ("/tmp/chromoting.%s.ssh_auth_sock" %
                               os.environ["USER"])
 
+  def _launch_xvfb(self, display, x_auth_file, extra_x_args):
+    max_width = max([width for width, height in self.sizes])
+    max_height = max([height for width, height in self.sizes])
+
+    xvfb = locate_xvfb_randr()
+    if xvfb:
+      self.server_supports_exact_resize = True
+    else:
+      xvfb = "Xvfb"
+      self.server_supports_exact_resize = False
+
+    logging.info("Starting %s on display :%d" % (xvfb, display))
+    screen_option = "%dx%dx24" % (max_width, max_height)
+    self.x_proc = subprocess.Popen(
+        [xvfb, ":%d" % display,
+         "-auth", x_auth_file,
+         "-nolisten", "tcp",
+         "-noreset",
+         "-screen", "0", screen_option
+        ] + extra_x_args)
+    if not self.x_proc.pid:
+      raise Exception("Could not start Xvfb.")
+
+  def _launch_xorg(self, display, x_auth_file, extra_x_args):
+    config_file = tempfile.NamedTemporaryFile(
+        prefix="chrome_remote_desktop_", suffix=".conf", delete=False)
+    config_file.write(
+        # This causes X to load the default GLX module, even if a proprietary
+        # one is installed in a different directory.
+        'Section "Files"\n'
+        '  ModulePath "/usr/lib/xorg/modules"\n'
+        'EndSection\n'
+        '\n'
+        # Suppress device probing, which happens by default.
+        'Section "ServerFlags"\n'
+        '  Option "AutoAddDevices" "false"\n'
+        '  Option "AutoEnableDevices" "false"\n'
+        '  Option "DontVTSwitch" "true"\n'
+        '  Option "PciForceNone" "true"\n'
+        'EndSection\n'
+        '\n'
+        'Section "InputDevice"\n'
+        '  Identifier "Chrome Remote Desktop Input"\n'
+        '  Option "CoreKeyboard" "true"\n'
+        '  Option "CorePointer" "true"\n'
+        '  Driver "void"\n'
+        'EndSection\n'
+        '\n'
+        'Section "Device"\n'
+        '  Identifier "Chrome Remote Desktop Videocard"\n'
+        '  Driver "dummy"\n'
+        '  VideoRam 1048576\n'

[Jamie, 2016/05/07 00:05:47]

I think it's worth defining named constants for this (and other) magic values in
the config file, with a brief description of why they were chosen.

[rkjnsn, 2016/05/09 16:58:34]

That requires me to have a good reason for why they were chosen. :) Will do.

+        'EndSection\n'
+        '\n'
+        'Section "Monitor"\n'
+        '  Identifier "Chrome Remote Desktop Monitor"\n'
+        '  HorizSync   3.3\n'
+        '  VertRefresh 0.1\n'
+        '{modelines}'
+        'EndSection\n'
+        '\n'
+        'Section "Screen"\n'
+        '  Identifier "Chrome Remote Desktop Screen"\n'
+        '  Device "Chrome Remote Desktop Videocard"\n'
+        '  Monitor "Chrome Remote Desktop Monitor"\n'
+        '  DefaultDepth 24\n'
+        '  SubSection "Display"\n'
+        '    Viewport 0 0\n'
+        '    Depth 24\n'
+        '    Modes {modes}\n'
+        '  EndSubSection\n'
+        'EndSection\n'
+        '\n'
+        'Section "ServerLayout"\n'
+        '  Identifier   "Chrome Remote Desktop Layout"\n'
+        '  Screen       "Chrome Remote Desktop Screen"\n'
+        '  InputDevice  "Chrome Remote Desktop Input"\n'
+        'EndSection\n'.format(
+            modelines="".join(
+                '  Modeline "{0}x{1}" 108.9 {0} 32998 32999 33000 '
+                '{1} 32998 32999 33000\n'.format(w, h) for w, h in self.sizes),
+            modes=" ".join('"{0},{1}"'.format(w, h) for w, h in self.sizes)))
+    config_file.close()

[Jamie, 2016/05/07 00:05:47]

Would using "with" be more Pythonic than explicitly closing the file here?

[rkjnsn, 2016/05/09 16:58:33]

Probably. Will do.

+
+    # For now, we don't support exact resize with Xorg+dummy
+    self.server_supports_exact_resize = False
+    self.xorg_conf = config_file.name
+
+    logging.info("Starting Xorg on display :%d" % display)
+    # We use the child environment so the Xorg server picks up the Mesa libGL
+    # instead of any proprietary versions that may be installed, thanks to
+    # LD_LIBRARY_PATH.
+    # TODO(rkjnsn): Perhaps we want to pass the rest of our environment through
+    # instead of using the clean one we create for the session so that the user
+    # can pass environment variables to Xorg like they could for Xvfb.

[Jamie, 2016/05/07 00:05:47]

I don't think we need this TODO. It seems unlikely we'll ever do it unless it
proves to be a problem for end-users.

[rkjnsn, 2016/05/09 16:58:33]

Acknowledged.

+    self.x_proc = subprocess.Popen(
+        ["Xorg", ":%d" % display,
+         "-auth", x_auth_file,
+         "-nolisten", "tcp",
+         "-noreset",
+         # Log to stderr instead of a file so the output ends up in our main
+         # log file

[Jamie, 2016/05/07 00:05:47]

How does specifying the logfile as /dev/null log to stderr? I'd have thought it
effectively disabled logging.

[rkjnsn, 2016/05/09 16:58:33]

Ah, yes. I was worried this might not be clear. By default, Xorg logs to both a
file at level 3 and stderr at level 0. This is disabling logging to the file,
and bumping the stderr logging (which is already enabled) to level 3 so it
mirrors what would be written to the log file. I'll try to expand a bit more in
the comment.

[Jamie, 2016/05/10 00:37:09]

I completely missed the -verbose flag. Please move it to a separate line so that
you've got one option per line as per the other options.

+         "-logfile", "/dev/null", "-verbose", "3",
+         "-config", config_file.name
+        ] + extra_x_args, env=self.child_env)
+    if not self.x_proc.pid:
+      raise Exception("Could not start Xorg.")
+
   def _launch_x_server(self, extra_x_args):
     x_auth_file = os.path.expanduser("~/.Xauthority")
     self.child_env["XAUTHORITY"] = x_auth_file
@@ -369,16 +483,6 @@ class Desktop:
     if ret_code != 0:
       raise Exception("xauth failed with code %d" % ret_code)
 
-    max_width = max([width for width, height in self.sizes])
-    max_height = max([height for width, height in self.sizes])
-
-    xvfb = get_randr_supporting_x_server()
-    if xvfb:
-      self.server_supports_exact_resize = True
-    else:
-      xvfb = "Xvfb"
-      self.server_supports_exact_resize = False
-
     # Disable the Composite extension iff the X session is the default
     # Unity-2D, since it uses Metacity which fails to generate DAMAGE
     # notifications correctly. See crbug.com/166468.
@@ -387,17 +491,10 @@ class Desktop:
         x_session[1] == "/usr/bin/gnome-session --session=ubuntu-2d"):
       extra_x_args.extend(["-extension", "Composite"])
 
-    logging.info("Starting %s on display :%d" % (xvfb, display))
-    screen_option = "%dx%dx24" % (max_width, max_height)
-    self.x_proc = subprocess.Popen(
-        [xvfb, ":%d" % display,
-         "-auth", x_auth_file,
-         "-nolisten", "tcp",
-         "-noreset",
-         "-screen", "0", screen_option
-        ] + extra_x_args)
-    if not self.x_proc.pid:
-      raise Exception("Could not start Xvfb.")
+    if USE_XORG_ENV_VAR in os.environ:
+      self._launch_xorg(display, x_auth_file, extra_x_args)
+    else:
+      self._launch_xvfb(display, x_auth_file, extra_x_args)
 
     self.child_env["DISPLAY"] = ":%d" % display
     self.child_env["CHROME_REMOTE_DESKTOP_SESSION"] = "1"
@@ -419,9 +516,9 @@ class Desktop:
         break
       time.sleep(0.5)
     if retcode != 0:
-      raise Exception("Could not connect to Xvfb.")
+      raise Exception("Could not connect to X server.")
     else:
-      logging.info("Xvfb is active.")
+      logging.info("X server is active.")
 
     # The remoting host expects the server to use "evdev" keycodes, but Xvfb
     # starts configured to use the "base" ruleset, resulting in XKB configuring
@@ -831,7 +928,7 @@ def cleanup():
 
   global g_desktops
   for desktop in g_desktops:
-    for proc, name in [(desktop.x_proc, "Xvfb"),
+    for proc, name in [(desktop.x_proc, "X server"),
                        (desktop.session_proc, "session"),
                        (desktop.host_proc, "host")]:
       if proc is not None:
@@ -848,6 +945,8 @@ def cleanup():
           psutil_proc.kill()
         except psutil.Error:
           logging.error("Error terminating process")
+    if desktop.xorg_conf is not None:
+      os.remove(desktop.xorg_conf)
 
   g_desktops = []
   if ParentProcessLogger.instance():
@@ -1048,7 +1147,7 @@ Web Store: https://chrome.google.com/remotedesktop"""
   parser.add_option("-s", "--size", dest="size", action="append",
                     help="Dimensions of virtual desktop. This can be specified "
                     "multiple times to make multiple screen resolutions "
-                    "available (if the Xvfb server supports this).")
+                    "available (if the X server supports this).")
   parser.add_option("-f", "--foreground", dest="foreground", default=False,
                     action="store_true",
                     help="Don't run as a background daemon.")
@@ -1182,7 +1281,7 @@ Web Store: https://chrome.google.com/remotedesktop"""
 
   # If a RANDR-supporting Xvfb is not available, limit the default size to
   # something more sensible.
-  if get_randr_supporting_x_server():
+  if USE_XORG_ENV_VAR not in os.environ and locate_xvfb_randr():
     default_sizes = DEFAULT_SIZES
   else:
     default_sizes = DEFAULT_SIZE_NO_RANDR