chromeos: Include mode details in OutputSnapshot.

chromeos: Include mode details in OutputSnapshot.

This updates OutputConfigurator to include modes'
resolutions and interlaced-ness in OutputSnapshot structs,
and to pass OutputSnapshots to observers after the display
mode is changed.

A following change will update ash's
DisplayChangeObserverX11 class to use OutputSnapshots
instead of asking the X server for the same information via
XRandR.

BUG=266113
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218069

[oshima, 2013-08-15 19:03:23]

looks good, but want to double check "a class in a struct" question.

https://codereview.chromium.org/22871010/diff/3001/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/22871010/diff/3001/chromeos/display/output_co...
chromeos/display/output_configurator.h:97: std::map<RRMode, ModeInfo>
mode_infos;
Does style-guide allow a class in a struct? (just double checking)

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/output_co...
chromeos/display/output_configurator.h:50: bool interlaced;
just FYI: i'm going to add preferred flag to this.

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/output_co...
chromeos/display/output_configurator.h:92: uint64 height_mm;
are we using uint64 because that's what X is using?

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
chromeos/display/real_output_configurator_delegate.cc:128:
OutputConfigurator::OutputSnapshot output;
Consider moving the creation to separate method as this method is
relatively big.

[Daniel Erat, 2013-08-15 19:26:23]

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/output_co...
chromeos/display/output_configurator.h:92: uint64 height_mm;
On 2013/08/15 19:03:24, oshima wrote:
> are we using uint64 because that's what X is using?

Xrandr.h and ash::internal::ShouldIgnoreSize() look like they use unsigned longs
(i.e. uint32 in 32-bit builds and uint64 in 64-bit builds).

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
chromeos/display/real_output_configurator_delegate.cc:128:
OutputConfigurator::OutputSnapshot output;
On 2013/08/15 19:03:24, oshima wrote:
> Consider moving the creation to separate method as this method is
> relatively big.

I'm worried that that would make the code more complicated (the method would
take a long list of parameters; when a new field gets added both the method
signature and all of the callers need to be updated; the initialization of the
struct would be distributed across that method and this one; etc.).

[Daniel Erat, 2013-08-15 19:29:26]

https://codereview.chromium.org/22871010/diff/3001/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/22871010/diff/3001/chromeos/display/output_co...
chromeos/display/output_configurator.h:97: std::map<RRMode, ModeInfo>
mode_infos;
On 2013/08/15 19:03:24, oshima wrote:
> Does style-guide allow a class in a struct? (just double checking)

Whoops, missed this question. Yes, I think this is allowed:

"Use a struct only for passive objects that carry data; everything else is a
class.
The struct and class keywords behave almost identically in C++. We add our own
semantic meanings to each keyword, so you should use the appropriate keyword for
the data-type you're defining.

structs should be used for passive objects that carry data, and may have
associated constants, but lack any functionality other than access/setting the
data members. The accessing/setting of fields is done by directly accessing the
fields rather than through method invocations. Methods should not provide
behavior but should only be used to set up the data members, e.g., constructor,
destructor, Initialize(), Reset(), Validate()."

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...

(I'm not worried about copies being expensive since these structs live in a tiny
vector and don't get copied around.)

[oshima, 2013-08-15 19:37:26]

lgtm

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
chromeos/display/real_output_configurator_delegate.cc:128:
OutputConfigurator::OutputSnapshot output;
On 2013/08/15 19:26:23, Daniel Erat wrote:
> On 2013/08/15 19:03:24, oshima wrote:
> > Consider moving the creation to separate method as this method is
> > relatively big.
> 
> I'm worried that that would make the code more complicated (the method would
> take a long list of parameters; when a new field gets added both the method
> signature and all of the callers need to be updated; the initialization of the
> struct would be distributed across that method and this one; etc.).

It looked to me that you can create from three parameters,
output_id, i, and output_info. If that's not the case, then please disregard.

[Daniel Erat, 2013-08-15 21:20:33]

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/22871010/diff/6001/chromeos/display/real_outp...
chromeos/display/real_output_configurator_delegate.cc:128:
OutputConfigurator::OutputSnapshot output;
On 2013/08/15 19:37:26, oshima wrote:
> On 2013/08/15 19:26:23, Daniel Erat wrote:
> > On 2013/08/15 19:03:24, oshima wrote:
> > > Consider moving the creation to separate method as this method is
> > > relatively big.
> > 
> > I'm worried that that would make the code more complicated (the method would
> > take a long list of parameters; when a new field gets added both the method
> > signature and all of the callers need to be updated; the initialization of
the
> > struct would be distributed across that method and this one; etc.).
> 
> It looked to me that you can create from three parameters,
> output_id, i, and output_info. If that's not the case, then please disregard.

It takes few more than that (to initialize all of the fields, apart from the
ones that depend on other outputs), but I've made this change.

[oshima, 2013-08-15 21:35:59]

thanks, lgtm

https://codereview.chromium.org/22871010/diff/16001/chromeos/display/real_out...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/22871010/diff/16001/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:353: output.selected_mode
= output.native_mode;
finding selected_mode could have been done after this
method is returned (in other words, Initialize the snapshot using
output info, then update selected_mode using state_controller).
This is fine too, so I'll leave it to you.

[Daniel Erat, 2013-08-15 21:57:22]

https://codereview.chromium.org/22871010/diff/16001/chromeos/display/real_out...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/22871010/diff/16001/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:353: output.selected_mode
= output.native_mode;
On 2013/08/15 21:35:59, oshima wrote:
> finding selected_mode could have been done after this
> method is returned (in other words, Initialize the snapshot using
> output info, then update selected_mode using state_controller).
> This is fine too, so I'll leave it to you.

Done.

[commit-bot: I haz the power, 2013-08-15 21:59:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/22871010/21004

[commit-bot: I haz the power, 2013-08-16 01:00:27]

Failed to trigger a try job on android_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-16 01:00:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/22871010/29002

[commit-bot: I haz the power, 2013-08-16 02:52:43]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-16 03:13:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/22871010/29002

[commit-bot: I haz the power, 2013-08-16 07:23:21]

Failed to apply patch for chromeos/display/real_output_configurator_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chromeos/display/real_output_configurator_delegate.cc
  Hunk #1 FAILED at 12.
  Hunk #2 succeeded at 116 (offset 1 line).
  Hunk #3 succeeded at 199 (offset 1 line).
  Hunk #4 succeeded at 290 (offset 1 line).
  Hunk #5 succeeded at 387 (offset 1 line).
  Hunk #6 succeeded at 583 (offset 2 lines).
  Hunk #7 succeeded at 597 (offset 2 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
chromeos/display/real_output_configurator_delegate.cc.rej

Patch:       chromeos/display/real_output_configurator_delegate.cc
Index: chromeos/display/real_output_configurator_delegate.cc
diff --git a/chromeos/display/real_output_configurator_delegate.cc
b/chromeos/display/real_output_configurator_delegate.cc
index
47fbd581a6159a346532a4e45191946757493ee0..b2804ba45f30c04dc8e17898abbb2f55edc5ac98
100644
--- a/chromeos/display/real_output_configurator_delegate.cc
+++ b/chromeos/display/real_output_configurator_delegate.cc
@@ -12,6 +12,7 @@
 #include <X11/extensions/Xrandr.h>
 
 #include <cmath>
+#include <utility>
 
 #include "base/logging.h"
 #include "base/message_loop/message_pump_aurax11.h"
@@ -115,68 +116,38 @@ RealOutputConfiguratorDelegate::GetOutputs(
   RRCrtc last_used_crtc = None;
 
   for (int i = 0; i < screen_->noutput && outputs.size() < 2; ++i) {
-    RROutput this_id = screen_->outputs[i];
-    XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_, this_id);
+    RROutput output_id = screen_->outputs[i];
+    XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_,
output_id);
     bool is_connected = (output_info->connection == RR_Connected);
 
-    if (is_connected) {
-      OutputConfigurator::OutputSnapshot to_populate;
-      to_populate.output = this_id;
-      to_populate.has_display_id =
-          GetDisplayId(this_id, i, &to_populate.display_id);
-      to_populate.is_internal = IsInternalOutput(output_info);
-      // Use the index as a valid display id even if the internal
-      // display doesn't have valid EDID because the index
-      // will never change.
-      if (!to_populate.has_display_id && to_populate.is_internal)
-        to_populate.has_display_id = true;
-
-      (outputs.empty() ? one_info : two_info) = output_info;
-
-      // Now, look up the current CRTC and any related info.
-      if (output_info->crtc) {
-        XRRCrtcInfo* crtc_info = XRRGetCrtcInfo(
-            display_, screen_, output_info->crtc);
-        to_populate.current_mode = crtc_info->mode;
-        to_populate.x = crtc_info->x;
-        to_populate.y = crtc_info->y;
-        XRRFreeCrtcInfo(crtc_info);
-      }
+    if (!is_connected) {
+      XRRFreeOutputInfo(output_info);
+      continue;
+    }
 
-      // Assign a CRTC that isn't already in use.
-      for (int j = 0; j < output_info->ncrtc; ++j) {
-        if (output_info->crtcs[j] != last_used_crtc) {
-          to_populate.crtc = output_info->crtcs[j];
-          last_used_crtc = to_populate.crtc;
-          break;
-        }
-      }
-      to_populate.native_mode = GetOutputNativeMode(output_info);
-      if (to_populate.has_display_id) {
-        int width = 0, height = 0;
-        if (state_controller &&
-            state_controller->GetResolutionForDisplayId(
-                to_populate.display_id, &width, &height)) {
-          to_populate.selected_mode =
-              FindOutputModeMatchingSize(screen_, output_info, width, height);
-        }
+    (outputs.empty() ? one_info : two_info) = output_info;
+
+    OutputConfigurator::OutputSnapshot output = InitOutputSnapshot(
+        output_id, output_info, &last_used_crtc, i);
+
+    if (output.has_display_id) {
+      int width = 0, height = 0;
+      if (state_controller &&
+          state_controller->GetResolutionForDisplayId(
+              output.display_id, &width, &height)) {
+        output.selected_mode =
+            FindOutputModeMatchingSize(screen_, output_info, width, height);
       }
-      // Fallback to native mode.
-      if (to_populate.selected_mode == None)
-        to_populate.selected_mode = to_populate.native_mode;
-
-      to_populate.is_aspect_preserving_scaling =
-          IsOutputAspectPreservingScaling(this_id);
-      to_populate.touch_device_id = None;
-
-      VLOG(2) << "Found display " << outputs.size() << ":"
-              << " output=" << to_populate.output
-              << " crtc=" << to_populate.crtc
-              << " current_mode=" << to_populate.current_mode;
-      outputs.push_back(to_populate);
-    } else {
-      XRRFreeOutputInfo(output_info);
     }
+    // Fall back to native mode.
+    if (output.selected_mode == None)
+      output.selected_mode = output.native_mode;
+
+    VLOG(2) << "Found display " << outputs.size() << ":"
+            << " output=" << output.output
+            << " crtc=" << output.crtc
+            << " current_mode=" << output.current_mode;
+    outputs.push_back(output);
   }
 
   if (outputs.size() == 2) {
@@ -228,30 +199,6 @@ RealOutputConfiguratorDelegate::GetOutputs(
   return outputs;
 }
 
-bool RealOutputConfiguratorDelegate::GetModeDetails(RRMode mode,
-                                                    int* width,
-                                                    int* height,
-                                                    bool* interlaced) {
-  CHECK(screen_) << "Server not grabbed";
-  // TODO: Determine if we need to organize modes in a way which provides
-  // better than O(n) lookup time.  In many call sites, for example, the
-  // "next" mode is typically what we are looking for so using this
-  // helper might be too expensive.
-  for (int i = 0; i < screen_->nmode; ++i) {
-    if (mode == screen_->modes[i].id) {
-      const XRRModeInfo& info = screen_->modes[i];
-      if (width)
-        *width = info.width;
-      if (height)
-        *height = info.height;
-      if (interlaced)
-        *interlaced = info.modeFlags & RR_Interlace;
-      return true;
-    }
-  }
-  return false;
-}
-
 bool RealOutputConfiguratorDelegate::ConfigureCrtc(
     RRCrtc crtc,
     RRMode mode,
@@ -343,6 +290,84 @@ void
RealOutputConfiguratorDelegate::SendProjectingStateToPowerManager(
       SetIsProjecting(projecting);
 }
 
+bool RealOutputConfiguratorDelegate::GetModeDetails(RRMode mode,
+                                                    int* width,
+                                                    int* height,
+                                                    bool* interlaced) {
+  CHECK(screen_) << "Server not grabbed";
+  // TODO: Determine if we need to organize modes in a way which provides
+  // better than O(n) lookup time.  In many call sites, for example, the
+  // "next" mode is typically what we are looking for so using this
+  // helper might be too expensive.
+  for (int i = 0; i < screen_->nmode; ++i) {
+    if (mode == screen_->modes[i].id) {
+      const XRRModeInfo& info = screen_->modes[i];
+      if (width)
+        *width = info.width;
+      if (height)
+        *height = info.height;
+      if (interlaced)
+        *interlaced = info.modeFlags & RR_Interlace;
+      return true;
+    }
+  }
+  return false;
+}
+
+OutputConfigurator::OutputSnapshot
+RealOutputConfiguratorDelegate::InitOutputSnapshot(
+    RROutput id,
+    XRROutputInfo* info,
+    RRCrtc* last_used_crtc,
+    int index) {
+  OutputConfigurator::OutputSnapshot output;
+  output.output = id;
+  output.width_mm = info->mm_width;
+  output.height_mm = info->mm_height;
+  output.has_display_id = GetDisplayId(id, index, &output.display_id);
+  output.is_internal = IsInternalOutput(info);
+
+  // Use the index as a valid display ID even if the internal
+  // display doesn't have valid EDID because the index
+  // will never change.
+  if (!output.has_display_id && output.is_internal)
+    output.has_display_id = true;
+
+  if (info->crtc) {
+    XRRCrtcInfo* crtc_info = XRRGetCrtcInfo(display_, screen_, info->crtc);
+    output.current_mode = crtc_info->mode;
+    output.x = crtc_info->x;
+    output.y = crtc_info->y;
+    XRRFreeCrtcInfo(crtc_info);
+  }
+
+  // Assign a CRTC that isn't already in use.
+  for (int i = 0; i < info->ncrtc; ++i) {
+    if (info->crtcs[i] != *last_used_crtc) {
+      output.crtc = info->crtcs[i];
+      *last_used_crtc = output.crtc;
+      break;
+    }
+  }
+
+  output.native_mode = GetOutputNativeMode(info);
+  output.is_aspect_preserving_scaling = IsOutputAspectPreservingScaling(id);
+  output.touch_device_id = None;
+
+  for (int i = 0; i < info->nmode; ++i) {
+    const RRMode mode = info->modes[i];
+    OutputConfigurator::ModeInfo mode_info;
+    if (GetModeDetails(mode, &mode_info.width, &mode_info.height,
+                       &mode_info.interlaced)) {
+      output.mode_infos.insert(std::make_pair(mode, mode_info));
+    } else {
+      LOG(WARNING) << "Unable to find XRRModeInfo for mode " << mode;
+    }
+  }
+
+  return output;
+}
+
 void RealOutputConfiguratorDelegate::DestroyUnusedCrtcs(
     const std::vector<OutputConfigurator::OutputSnapshot>& outputs) {
   CHECK(screen_) << "Server not grabbed";
@@ -362,25 +387,28 @@ void RealOutputConfiguratorDelegate::DestroyUnusedCrtcs(
     RRCrtc crtc = screen_->crtcs[i];
     RRMode mode = None;
     RROutput output = None;
+    const OutputConfigurator::ModeInfo* mode_info = NULL;
     for (std::vector<OutputConfigurator::OutputSnapshot>::const_iterator it =
          outputs.begin(); it != outputs.end(); ++it) {
       if (crtc == it->crtc) {
         mode = it->current_mode;
         output = it->output;
+        if (mode != None)
+          mode_info = OutputConfigurator::GetModeInfo(*it, mode);
         break;
       }
     }
 
-    if (mode != None) {
+    if (mode_info) {
       // In case our CRTC doesn't fit in our current framebuffer, disable it.
       // It'll get reenabled after we resize the framebuffer.
- …
(message too large)

[commit-bot: I haz the power, 2013-08-16 15:28:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/22871010/59001

[commit-bot: I haz the power, 2013-08-16 19:56:50]

Change committed as 218069