Changes how to compute the id of a display.

Due to the discussion in crbug.com/240341, we prefer to use
the name field rather than the product_code field.

BUG=240341
R=oshima@chromium.org, marcheu@chromium.org
TEST=compilation passed

Review URL: https://chromiumcodereview.appspot.com/15368003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@201756 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 96 insertions, 48 deletions

diff --git a/chromeos/display/output_util.cc b/chromeos/display/output_util.cc
index aa235b0..9995822 100644
--- a/chromeos/display/output_util.cc
+++ b/chromeos/display/output_util.cc
@@ -8,6 +8,7 @@
 #include <X11/extensions/Xrandr.h>
 #include <X11/Xatom.h>
 
+#include "base/hash.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
 #include "base/sys_byteorder.h"
@@ -20,17 +21,17 @@ const char kInternal_LVDS[] = "LVDS";
 const char kInternal_eDP[] = "eDP";
 
 // Returns 64-bit persistent ID for the specified manufacturer's ID and
-// product_code, and the index of the output it is connected to.
+// product_code_hash, and the index of the output it is connected to.
 // |output_index| is used to distinguish the displays of the same type. For
 // example, swapping two identical display between two outputs will not be
 // treated as swap. The 'serial number' field in EDID isn't used here because
 // it is not guaranteed to have unique number and it may have the same fixed
 // value (like 0).
 int64 GetID(uint16 manufacturer_id,
-            uint16 product_code,
+            uint32 product_code_hash,
             uint8 output_index) {
-  return ((static_cast<int64>(manufacturer_id) << 24) |
-          (static_cast<int64>(product_code) << 8) | output_index);
+  return ((static_cast<int64>(manufacturer_id) << 40) |
+          (static_cast<int64>(product_code_hash) << 8) | output_index);
 }
 
 bool IsRandRAvailable() {
@@ -97,7 +98,6 @@ bool GetEDIDProperty(XID output, unsigned long* nitems, unsigned char** prop) {
 // NULL can be passed for unwanted output parameters.
 bool GetOutputDeviceData(XID output,
                          uint16* manufacturer_id,
-                         uint16* product_code,
                          std::string* human_readable_name) {
   unsigned long nitems = 0;
   unsigned char *prop = NULL;
@@ -105,7 +105,7 @@ bool GetOutputDeviceData(XID output,
     return false;
 
   bool result = ParseOutputDeviceData(
-      prop, nitems, manufacturer_id, product_code, human_readable_name);
+      prop, nitems, manufacturer_id, human_readable_name);
   XFree(prop);
   return result;
 }
@@ -114,19 +114,41 @@ bool GetOutputDeviceData(XID output,
 
 std::string GetDisplayName(XID output_id) {
   std::string display_name;
-  GetOutputDeviceData(output_id, NULL, NULL, &display_name);
+  GetOutputDeviceData(output_id, NULL, &display_name);
   return display_name;
 }
 
 bool GetDisplayId(XID output_id, size_t output_index, int64* display_id_out) {
+  unsigned long nitems = 0;
+  unsigned char* prop = NULL;
+  if (!GetEDIDProperty(output_id, &nitems, &prop))
+    return false;
+
+  bool result =
+      GetDisplayIdFromEDID(prop, nitems, output_index, display_id_out);
+  XFree(prop);
+  return result;
+}
+
+bool GetDisplayIdFromEDID(const unsigned char* prop,
+                          unsigned long nitems,
+                          size_t output_index,
+                          int64* display_id_out) {
   uint16 manufacturer_id = 0;
-  uint16 product_code = 0;
-  if (GetOutputDeviceData(
-          output_id, &manufacturer_id, &product_code, NULL) &&
-      manufacturer_id != 0) {
+  std::string product_name;
+
+  // ParseOutputDeviceData fails if it doesn't have product_name.
+  ParseOutputDeviceData(prop, nitems, &manufacturer_id, &product_name);
+
+  // Generates product specific value from product_name instead of product code.
+  // See crbug.com/240341
+  uint32 product_code_hash = product_name.empty() ?
+      0 : base::Hash(product_name);
+  if (manufacturer_id != 0) {
     // An ID based on display's index will be assigned later if this call
     // fails.
-    *display_id_out = GetID(manufacturer_id, product_code, output_index);
+    *display_id_out = GetID(
+        manufacturer_id, product_code_hash, output_index);
     return true;
   }
   return false;
@@ -135,18 +157,14 @@ bool GetDisplayId(XID output_id, size_t output_index, int64* display_id_out) {
 bool ParseOutputDeviceData(const unsigned char* prop,
                            unsigned long nitems,
                            uint16* manufacturer_id,
-                           uint16* product_code,
                            std::string* human_readable_name) {
   // See http://en.wikipedia.org/wiki/Extended_display_identification_data
   // for the details of EDID data format.  We use the following data:
   //   bytes 8-9: manufacturer EISA ID, in big-endian
-  //   bytes 10-11: represents product code, in little-endian
   //   bytes 54-125: four descriptors (18-bytes each) which may contain
   //     the display name.
   const unsigned int kManufacturerOffset = 8;
   const unsigned int kManufacturerLength = 2;
-  const unsigned int kProductCodeOffset = 10;
-  const unsigned int kProductCodeLength = 2;
   const unsigned int kDescriptorOffset = 54;
   const unsigned int kNumDescriptors = 4;
   const unsigned int kDescriptorLength = 18;
@@ -166,16 +184,6 @@ bool ParseOutputDeviceData(const unsigned char* prop,
 #endif
   }
 
-  if (product_code) {
-    if (nitems < kProductCodeOffset + kProductCodeLength) {
-      LOG(ERROR) << "too short EDID data: product code";
-      return false;
-    }
-
-    *product_code = base::ByteSwapToLE16(
-        *reinterpret_cast<const uint16*>(prop + kProductCodeOffset));
-  }
-
   if (!human_readable_name)
     return true;
 
diff --git a/chromeos/display/output_util.h b/chromeos/display/output_util.h
index 5ecda9b..0cbe159 100644
--- a/chromeos/display/output_util.h
+++ b/chromeos/display/output_util.h
@@ -16,12 +16,19 @@ typedef unsigned long XID;
 
 namespace chromeos {
 
-// Generates the display id for the pair of |output| and |index| and store
-// in |display_id_out|. Returns true if the display id is successfully
-// generated, or false otherwise.
+// Gets the EDID data from |output| and generates the display id through
+// |GetDisplayIdFromEDID|.
 CHROMEOS_EXPORT bool GetDisplayId(XID output, size_t index,
                                   int64* display_id_out);
 
+// Generates the display id for the pair of |prop| with |nitems| length and
+// |index|, and store in |display_id_out|. Returns true if the display id is
+// successfully generated, or false otherwise.
+CHROMEOS_EXPORT bool GetDisplayIdFromEDID(const unsigned char* prop,
+                                          unsigned long nitems,
+                                          size_t index,
+                                          int64* display_id_out);
+
 // Generates the human readable string from EDID obtained for |output|.
 CHROMEOS_EXPORT std::string GetDisplayName(XID output);
 
@@ -32,14 +39,13 @@ CHROMEOS_EXPORT std::string GetDisplayName(XID output);
 // false.
 CHROMEOS_EXPORT bool GetOutputOverscanFlag(XID output, bool* flag);
 
-// Parses |prop| as EDID data and stores extracted data into |manufacturer_id|,
-// |product_code|, and |human_readable_name| and returns true. NULL can be
+// Parses |prop| as EDID data and stores extracted data into |manufacturer_id|
+// and |human_readable_name| and returns true. NULL can be
 // passed for unwanted output parameters. This is exported for
 // x11_util_unittest.cc.
 CHROMEOS_EXPORT bool ParseOutputDeviceData(const unsigned char* prop,
                                            unsigned long nitems,
                                            uint16* manufacturer_id,
-                                           uint16* product_code,
                                            std::string* human_readable_name);
 
 // Parses |prop| as EDID data and stores the overscan flag to |flag|. Returns
diff --git a/chromeos/display/output_util_unittest.cc b/chromeos/display/output_util_unittest.cc
index cd724ae..84a0685a 100644
--- a/chromeos/display/output_util_unittest.cc
+++ b/chromeos/display/output_util_unittest.cc
@@ -75,52 +75,65 @@ const unsigned char kMisdetecedDisplay[] =
 
 }
 
+const unsigned char kLP2565A[] =
+    "\x00\xFF\xFF\xFF\xFF\xFF\xFF\x00\x22\xF0\x76\x26\x01\x01\x01\x01"
+    "\x02\x12\x01\x03\x80\x34\x21\x78\xEE\xEF\x95\xA3\x54\x4C\x9B\x26"
+    "\x0F\x50\x54\xA5\x6B\x80\x81\x40\x81\x80\x81\x99\x71\x00\xA9\x00"
+    "\xA9\x40\xB3\x00\xD1\x00\x28\x3C\x80\xA0\x70\xB0\x23\x40\x30\x20"
+    "\x36\x00\x07\x44\x21\x00\x00\x1A\x00\x00\x00\xFD\x00\x30\x55\x1E"
+    "\x5E\x11\x00\x0A\x20\x20\x20\x20\x20\x20\x00\x00\x00\xFC\x00\x48"
+    "\x50\x20\x4C\x50\x32\x34\x36\x35\x0A\x20\x20\x20\x00\x00\x00\xFF"
+    "\x00\x43\x4E\x4B\x38\x30\x32\x30\x34\x48\x4D\x0A\x20\x20\x00\xA4";
+
+const unsigned char kLP2565B[] =
+    "\x00\xFF\xFF\xFF\xFF\xFF\xFF\x00\x22\xF0\x75\x26\x01\x01\x01\x01"
+    "\x02\x12\x01\x03\x6E\x34\x21\x78\xEE\xEF\x95\xA3\x54\x4C\x9B\x26"
+    "\x0F\x50\x54\xA5\x6B\x80\x81\x40\x71\x00\xA9\x00\xA9\x40\xA9\x4F"
+    "\xB3\x00\xD1\xC0\xD1\x00\x28\x3C\x80\xA0\x70\xB0\x23\x40\x30\x20"
+    "\x36\x00\x07\x44\x21\x00\x00\x1A\x00\x00\x00\xFD\x00\x30\x55\x1E"
+    "\x5E\x15\x00\x0A\x20\x20\x20\x20\x20\x20\x00\x00\x00\xFC\x00\x48"
+    "\x50\x20\x4C\x50\x32\x34\x36\x35\x0A\x20\x20\x20\x00\x00\x00\xFF"
+    "\x00\x43\x4E\x4B\x38\x30\x32\x30\x34\x48\x4D\x0A\x20\x20\x00\x45";
+
 TEST(OutputUtilTest, ParseEDID) {
   uint16 manufacturer_id = 0;
-  uint16 product_code = 0;
   std::string human_readable_name;
   EXPECT_TRUE(ParseOutputDeviceData(
       kNormalDisplay, charsize(kNormalDisplay),
-      &manufacturer_id, &product_code, &human_readable_name));
+      &manufacturer_id, &human_readable_name));
   EXPECT_EQ(0x22f0u, manufacturer_id);
-  EXPECT_EQ(0x286cu, product_code);
   EXPECT_EQ("HP ZR30w", human_readable_name);
 
   manufacturer_id = 0;
-  product_code = 0;
   human_readable_name.clear();
   EXPECT_TRUE(ParseOutputDeviceData(
       kInternalDisplay, charsize(kInternalDisplay),
-      &manufacturer_id, &product_code, NULL));
+      &manufacturer_id, NULL));
   EXPECT_EQ(0x4ca3u, manufacturer_id);
-  EXPECT_EQ(0x3142u, product_code);
   EXPECT_EQ("", human_readable_name);
 
   // Internal display doesn't have name.
   EXPECT_FALSE(ParseOutputDeviceData(
       kInternalDisplay, charsize(kInternalDisplay),
-      NULL, NULL, &human_readable_name));
+      NULL, &human_readable_name));
 
   manufacturer_id = 0;
-  product_code = 0;
   human_readable_name.clear();
   EXPECT_TRUE(ParseOutputDeviceData(
       kOverscanDisplay, charsize(kOverscanDisplay),
-      &manufacturer_id, &product_code, &human_readable_name));
+      &manufacturer_id, &human_readable_name));
   EXPECT_EQ(0x4c2du, manufacturer_id);
-  EXPECT_EQ(0x08feu, product_code);
   EXPECT_EQ("SAMSUNG", human_readable_name);
 }
 
 TEST(OutputUtilTest, ParseBrokenEDID) {
   uint16 manufacturer_id = 0;
-  uint16 product_code = 0;
   std::string human_readable_name;
 
   // length == 0
   EXPECT_FALSE(ParseOutputDeviceData(
       kNormalDisplay, 0,
-      &manufacturer_id, &product_code, &human_readable_name));
+      &manufacturer_id, &human_readable_name));
 
   // name is broken. Copying kNormalDisplay and substitute its name data by
   // some control code.
@@ -133,17 +146,15 @@ TEST(OutputUtilTest, ParseBrokenEDID) {
   EXPECT_FALSE(ParseOutputDeviceData(
       reinterpret_cast<const unsigned char*>(display_data.data()),
       display_data.size(),
-      &manufacturer_id, &product_code, &human_readable_name));
+      &manufacturer_id, &human_readable_name));
 
   // If |human_readable_name| isn't specified, it skips parsing the name.
   manufacturer_id = 0;
-  product_code = 0;
   EXPECT_TRUE(ParseOutputDeviceData(
       reinterpret_cast<const unsigned char*>(display_data.data()),
       display_data.size(),
-      &manufacturer_id, &product_code, NULL));
+      &manufacturer_id, NULL));
   EXPECT_EQ(0x22f0u, manufacturer_id);
-  EXPECT_EQ(0x286cu, product_code);
 }
 
 TEST(OutputUtilTest, ParseOverscanFlag) {
@@ -205,4 +216,27 @@ TEST(OutputUtilTest, IsInternalOutputName) {
   EXPECT_FALSE(IsInternalOutputName("eD"));
 }
 
+TEST(OutputUtilTest, GetDisplayId) {
+  // EDID of kLP2565A and B are slightly different but actually the same device.
+  int64 id1 = -1;
+  int64 id2 = -1;
+  EXPECT_TRUE(GetDisplayIdFromEDID(kLP2565A, charsize(kLP2565A), 0, &id1));
+  EXPECT_TRUE(GetDisplayIdFromEDID(kLP2565B, charsize(kLP2565B), 0, &id2));
+  EXPECT_EQ(id1, id2);
+  EXPECT_NE(-1, id1);
+}
+
+TEST(OutputUtilTest, GetDisplayIdFromInternal) {
+  int64 id = -1;
+  EXPECT_TRUE(GetDisplayIdFromEDID(
+      kInternalDisplay, charsize(kInternalDisplay), 0, &id));
+  EXPECT_NE(-1, id);
+}
+
+TEST(OutputUtilTest, GetDisplayIdFailure) {
+  int64 id = -1;
+  EXPECT_FALSE(GetDisplayIdFromEDID(NULL, 0, 0, &id));
+  EXPECT_EQ(-1, id);
+}
+
 }   // namespace chromeos