Refactor monitor_reconfigure to stop using system("xrandr").

monitor_reconfigure_main.cc

 // Copyright (c) 2010 The Chromium OS 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 "monitor_reconfig/monitor_reconfigure_main.h"
 
 #include <algorithm>
 #include <cstdio>
 #include <cstdlib>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 
 using std::map;
 using std::sort;
 using std::string;
 using std::vector;
 
 namespace monitor_reconfig {
 
 static const char kLcdOutputName[] = "LVDS1";
+static const float kScreenDpi = 96.0;
+static const float kInchInMm = 25.4;
 
 MonitorReconfigureMain::MonitorReconfigureMain(Display* display,
+                                               Window window,
                                                XRRScreenResources* screen_info)
     : display_(display),
+      window_(window),
       screen_info_(screen_info),
-      lcd_output_(NULL),
-      external_output_(NULL) {
+      lcd_output_(0),
+      lcd_output_info_(NULL),
+      external_output_(0),
+      external_output_info_(NULL) {
   for (int i = 0; i < screen_info_->nmode; ++i) {
     XRRModeInfo* current_mode = &screen_info_->modes[i];
     mode_map_[current_mode->id] = current_mode;
   }
   DetermineOutputs();
 }
 
 void MonitorReconfigureMain::Run() {
   vector<ResolutionSelector::Mode> lcd_modes;
-  SortModesByResolution(*lcd_output_, &lcd_modes);
+  SortModesByResolution(lcd_output_, &lcd_modes);
   DCHECK(!lcd_modes.empty());
 
   vector<ResolutionSelector::Mode> external_modes;
   if (IsExternalMonitorConnected()) {
-    SortModesByResolution(*external_output_, &external_modes);
+    SortModesByResolution(external_output_, &external_modes);
     DCHECK(!external_modes.empty());
   }
 
   ResolutionSelector selector;
-  string lcd_resolution, external_resolution, screen_resolution;
+  ResolutionSelector::Mode lcd_resolution(0, 0, "", 0);

[Daniel Erat, 2011/03/25 03:39:02]

i think that it's okay to make an additional c'tor that takes no arguments and
initializes the object to these default values.

[marcheu, 2011/03/25 17:39:42]

Done.

+  ResolutionSelector::Mode external_resolution(0, 0, "", 0);
+  ResolutionSelector::Mode screen_resolution(0, 0, "", 0);
+
   CHECK(selector.FindBestResolutions(lcd_modes,
                                      external_modes,
                                      &lcd_resolution,
                                      &external_resolution,
                                      &screen_resolution));
-  CHECK(!lcd_resolution.empty() || !external_resolution.empty());
+  CHECK(!lcd_resolution.name.empty() || !external_resolution.name.empty());
+
+  // Grab the server during mode changes.
+  XGrabServer(display_);
 
   // Disable the LCD if we were told to do so (because we're using a higher
   // resolution that'd be clipped on the LCD).
-  if (lcd_resolution.empty())
-    DisableDevice(lcd_output_->name);
+  // Otherwise enable the LCD if appropriate.
+  if (lcd_resolution.name.empty())
+    DisableDevice(lcd_output_, lcd_output_info_);
+  else
+    SetDeviceResolution(lcd_output_, lcd_output_info_, lcd_resolution);
 
   // If there's no external output connected, disable the device before we try
   // to set the screen resolution; otherwise xrandr will complain if we're
   // trying to set the screen to a smaller size than what the now-unplugged
   // device was using.
-  if (external_resolution.empty())
-    DisableDevice(external_output_->name);
+  // Otherwise enable the external device if appropriate.
+  if (external_resolution.name.empty())
+    DisableDevice(external_output_, external_output_info_);
+  else
+    SetDeviceResolution(external_output_, external_output_info_,
+                        external_resolution);
 
-  // Set the fb to try to avoid the driver's "crtc has no fb" message.
-  // Doing this before enabling the display reduces the likelihood of a
-  // visible "snap" when returning to the panel.
+  // Set the fb resolution.
   SetScreenResolution(screen_resolution);
 
-  // Enable the LCD if appropriate.
-  if (!lcd_resolution.empty())
-    SetDeviceResolution(lcd_output_->name, lcd_resolution);
+  if (!lcd_resolution.name.empty())

[Daniel Erat, 2011/03/25 03:39:02]

i think that something went horribly wrong here and in the next if statement :-)

[marcheu, 2011/03/25 17:39:42]

Indeed. Oops.

 
-  // Enable the external device if appropriate.
-  if (!external_resolution.empty())
-    SetDeviceResolution(external_output_->name, external_resolution);
+  if (!external_resolution.name.empty())
+
+  // Now let the server go and sync all changes.
+  XUngrabServer(display_);
+  XSync(display_, False);
 }
 
 void MonitorReconfigureMain::DetermineOutputs() {
   CHECK(screen_info_->noutput > 1) << "Expected at least two outputs";
-  XRROutputInfo* first_output =
-      XRRGetOutputInfo(display_, screen_info_, screen_info_->outputs[0]);
-  XRROutputInfo* second_output =
-      XRRGetOutputInfo(display_, screen_info_, screen_info_->outputs[1]);
 
-  if (strcmp(first_output->name, kLcdOutputName) == 0) {
+  RROutput first_output = screen_info_->outputs[0];
+  RROutput second_output = screen_info_->outputs[1];
+
+  XRROutputInfo* first_output_info =
+      XRRGetOutputInfo(display_, screen_info_, first_output);
+  XRROutputInfo* second_output_info =
+      XRRGetOutputInfo(display_, screen_info_, second_output);
+
+  if (strcmp(first_output_info->name, kLcdOutputName) == 0) {
     lcd_output_ = first_output;
+    lcd_output_info_ = first_output_info;
     external_output_ = second_output;
+    external_output_info_ = second_output_info;

[Daniel Erat, 2011/03/25 03:39:02]

can you add a check earlier in this method to make sure that it's only called
once (e.g. CHECK(!lcd_output_info_);)?  we'll leak memory otherwise if someone
screws up down the line and calls it twice.

[marcheu, 2011/03/25 17:39:42]

Done.

   } else {
     lcd_output_ = second_output;
+    lcd_output_info_ = second_output_info;
     external_output_ = first_output;
+    external_output_info_ = first_output_info;
   }
 
-  LOG(INFO) << "LCD name: " << lcd_output_->name;
-  for (int i = 0; i < lcd_output_->nmode; ++i) {
-    XRRModeInfo* mode = mode_map_[lcd_output_->modes[i]];
-    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height;
+  LOG(INFO) << "LCD name: " << lcd_output_info_->name << " (xid "
+            << lcd_output_ << ")";
+  for (int i = 0; i < lcd_output_info_->nmode; ++i) {
+    XRRModeInfo* mode = mode_map_[lcd_output_info_->modes[i]];
+    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height
+              << " (xid " << mode->id << ")";
   }
 
-  LOG(INFO) << "External name: " << external_output_->name;
-  for (int i = 0; i < external_output_->nmode; ++i) {
-    XRRModeInfo* mode = mode_map_[external_output_->modes[i]];
-    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height;
+  LOG(INFO) << "External name: " << external_output_info_->name
+            << " (xid " << external_output_ << ")";
+  for (int i = 0; i < external_output_info_->nmode; ++i) {
+    XRRModeInfo* mode = mode_map_[external_output_info_->modes[i]];
+    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height
+              << " (xid " << mode->id << ")";
   }
 }
 
 bool MonitorReconfigureMain::IsExternalMonitorConnected() {
-  return (external_output_->connection == RR_Connected);
+  return (external_output_info_->connection == RR_Connected);
 }
 
 void MonitorReconfigureMain::SortModesByResolution(
-    const XRROutputInfo& output_info,
+    RROutput output,
     vector<ResolutionSelector::Mode>* modes_out) {
   modes_out->clear();
 
-  for (int i = 0; i < output_info.nmode; ++i) {
-    const XRRModeInfo* mode = mode_map_[output_info.modes[i]];
+  XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_info_, output);
+  for (int i = 0; i < output_info->nmode; ++i) {
+    const XRRModeInfo* mode = mode_map_[output_info->modes[i]];
     DCHECK(mode);
+    LOG(INFO) << "Adding mode " << mode->width << " " << mode->height
+              << " (xid " << mode->id << ")";
     modes_out->push_back(
-        ResolutionSelector::Mode(mode->width, mode->height, mode->name));
+        ResolutionSelector::Mode(mode->width, mode->height, mode->name,
+                                 mode->id));
   }
 
   sort(modes_out->begin(), modes_out->end(),
        ResolutionSelector::ModeResolutionComparator());
+
+  XRRFreeOutputInfo(output_info);
 }
 
 bool MonitorReconfigureMain::SetDeviceResolution(
-    const std::string& device_name, const std::string& resolution) {
-  string command = StringPrintf("xrandr --output %s --mode %s",
-                                device_name.c_str(), resolution.c_str());
-  LOG(INFO) << "Running " << command.c_str();
-  return system(command.c_str()) == 0;
+    RROutput output, const XRROutputInfo* output_info,
+    const ResolutionSelector::Mode& resolution) {
+  Status s = XRRSetCrtcConfig(display_, screen_info_, output_info->crtcs[0],
+                              CurrentTime, 0, 0, resolution.id, RR_Rotate_0,
+                              &output, 1);
+  return (s == RRSetConfigSuccess);
 }
 
 bool MonitorReconfigureMain::SetScreenResolution(
-    const std::string& resolution) {
-  string command = StringPrintf("xrandr --fb %s", resolution.c_str());
-  LOG(INFO) << "Running " << command.c_str();
-  return system(command.c_str()) == 0;
+    const ResolutionSelector::Mode& resolution) {
+  int screen = DefaultScreen(display_);

[Daniel Erat, 2011/03/25 03:39:02]

this variable looks like it isn't being used

[marcheu, 2011/03/25 17:39:42]

Done.

+  LOG(INFO) << "Setting resolution " << resolution.name.c_str() << " "
+            << resolution.width << "x" << resolution.height;
+  XRRSetScreenSize(display_, window_,
+                   resolution.width,
+                   resolution.height,
+                   resolution.width * kInchInMm / kScreenDpi,
+                   resolution.height * kInchInMm / kScreenDpi);
+
+  return true;
 }
 
-bool MonitorReconfigureMain::DisableDevice(const std::string& device_name) {
-  string command = StringPrintf("xrandr --output %s --off",
-                                device_name.c_str());
-  LOG(INFO) << "Running " << command.c_str();
-  return system(command.c_str()) == 0;
+bool MonitorReconfigureMain::DisableDevice(RROutput output,
+                                           const XRROutputInfo* output_info) {
+  if (!output_info->crtc)
+    return true;
+  LOG(INFO) << "Disabling output " << output_info->name;
+  Status s = XRRSetCrtcConfig(display_, screen_info_, output_info->crtc,
+    CurrentTime, 0, 0, None, RR_Rotate_0, NULL, 0);
+  return (s == RRSetConfigSuccess);
 }
 
 }  // end namespace monitor_reconfig
 
 int main(int argc, char** argv) {
   CommandLine::Init(argc, argv);
   logging::InitLogging(NULL,
                        logging::LOG_ONLY_TO_SYSTEM_DEBUG_LOG,
                        logging::DONT_LOCK_LOG_FILE,
                        logging::APPEND_TO_OLD_LOG_FILE);
   Display* display = XOpenDisplay(NULL);
   CHECK(display) << "Could not open display";
 
   Window window = RootWindow(display, DefaultScreen(display));
   XRRScreenResources* screen_info = XRRGetScreenResources(display, window);
-  monitor_reconfig::MonitorReconfigureMain main_app(display, screen_info);
+  monitor_reconfig::MonitorReconfigureMain main_app(display, window,
+                                                    screen_info);
   main_app.Run();
+  XRRFreeScreenResources(screen_info);
   return 0;
 }