Generate display product id with EDID manufacturer product code

Formerly the product id for the DisplaySnapshot was generated with a
combination of the manufacturer code combined with a hash of the product
display name. However as the product display name can vary based on
display connector it is more appropriate to use the product code.

TEST=display_unittests passes.
BUG=471749

Review URL: https://codereview.chromium.org/1151583002

Cr-Commit-Position: refs/heads/master@{#330929}

ui/display/util/edid_parser.cc

@@ -29,11 +29,11 @@ int64_t GetID(uint16_t manufacturer_id,
           (static_cast<int64_t>(product_code_hash) << 8) | output_index);
 }
 
-// Returns a 64-bit identifier for this model of display, using
-// |manufacturer_id| and |product_code_hash|.
-int64_t GetProductID(uint16_t manufacturer_id, uint32_t product_code_hash) {
-  return ((static_cast<int64_t>(manufacturer_id) << 32) |
-          (static_cast<int64_t>(product_code_hash)));
+// Returns a 32-bit identifier for this model of display, using
+// |manufacturer_id| and |product_code|.
+uint32_t GetProductID(uint16_t manufacturer_id, uint16_t product_code) {
+  return ((static_cast<uint32_t>(manufacturer_id) << 16) |
+          (static_cast<uint32_t>(product_code)));
 }
 
 }  // namespace
@@ -43,23 +43,27 @@ bool GetDisplayIdFromEDID(const std::vector<uint8_t>& edid,
                           int64_t* display_id_out,
                           int64_t* product_id_out) {
   uint16_t manufacturer_id = 0;
+  uint16_t product_code = 0;
   std::string product_name;
 
   // ParseOutputDeviceData fails if it doesn't have product_name.
-  ParseOutputDeviceData(edid, &manufacturer_id, &product_name, nullptr,
-                        nullptr);
+  ParseOutputDeviceData(edid, &manufacturer_id, &product_code, &product_name,
+                        nullptr, nullptr);
 
-  // Generates product specific value from product_name instead of product code.
-  // See crbug.com/240341
-  uint32_t product_code_hash = product_name.empty() ?
-      0 : base::Hash(product_name);
   if (manufacturer_id != 0) {
+    // Generates product specific value from product_name instead of product
+    // code.
+    // See crbug.com/240341
+    uint32_t product_code_hash =
+        product_name.empty() ? 0 : base::Hash(product_name);
     // An ID based on display's index will be assigned later if this call
     // fails.
     *display_id_out = GetID(
         manufacturer_id, product_code_hash, output_index);
+    // product_id is 64-bit signed so it can store -1 as kInvalidProductID and
+    // not match a valid product id which will all be in the lowest 32-bits.
     if (product_id_out)
-      *product_id_out = GetProductID(manufacturer_id, product_code_hash);
+      *product_id_out = GetProductID(manufacturer_id, product_code);
     return true;
   }
   return false;
@@ -67,16 +71,20 @@ bool GetDisplayIdFromEDID(const std::vector<uint8_t>& edid,
 
 bool ParseOutputDeviceData(const std::vector<uint8_t>& edid,
                            uint16_t* manufacturer_id,
+                           uint16_t* product_code,
                            std::string* human_readable_name,
                            gfx::Size* active_pixel_out,
                            gfx::Size* physical_display_size_out) {
   // 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: manufacturer 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;
@@ -85,7 +93,7 @@ bool ParseOutputDeviceData(const std::vector<uint8_t>& edid,
 
   if (manufacturer_id) {
     if (edid.size() < kManufacturerOffset + kManufacturerLength) {
-      LOG(ERROR) << "too short EDID data: manifacturer id";
+      LOG(ERROR) << "too short EDID data: manufacturer id";
       return false;
     }
 
@@ -96,6 +104,16 @@ bool ParseOutputDeviceData(const std::vector<uint8_t>& edid,
 #endif
   }
 
+  if (product_code) {
+    if (edid.size() < kProductCodeOffset + kProductCodeLength) {
+      LOG(ERROR) << "too short EDID data: manufacturer product code";
+      return false;
+    }
+
+    *product_code =
+        *reinterpret_cast<const uint16_t*>(&edid[kProductCodeOffset]);
+  }
+
   if (human_readable_name)
     human_readable_name->clear();
 

ui/display/util/edid_parser.h

@@ -30,13 +30,15 @@ DISPLAY_UTIL_EXPORT bool GetDisplayIdFromEDID(const std::vector<uint8_t>& edid,
                                               int64_t* product_id_out);
 
 // Parses |edid| as EDID data and stores extracted data into |manufacturer_id|,
-// |human_readable_name|, |active_pixel_out| and |physical_display_size_out|,
-// then returns true. nullptr can be passed for unwanted output parameters.
-// Some devices (especially internal displays) may not have the field for
-// |human_readable_name|, and it will return true in that case.
+// |product_code|, |human_readable_name|, |active_pixel_out| and
+// |physical_display_size_out|, then returns true. nullptr can be passed for
+// unwanted output parameters.  Some devices (especially internal displays) may
+// not have the field for |human_readable_name|, and it will return true in
+// that case.
 DISPLAY_UTIL_EXPORT bool ParseOutputDeviceData(
     const std::vector<uint8_t>& edid,
     uint16_t* manufacturer_id,
+    uint16_t* product_code,
     std::string* human_readable_name,
     gfx::Size* active_pixel_out,
     gfx::Size* physical_display_size_out);

ui/display/util/edid_parser_unittest.cc

@@ -152,42 +152,48 @@ TEST(EDIDParserTest, ParseBrokenOverscanData) {
 
 TEST(EDIDParserTest, ParseEDID) {
   uint16_t manufacturer_id = 0;
+  uint16_t product_code = 0;
   std::string human_readable_name;
   std::vector<uint8_t> edid(
       kNormalDisplay, kNormalDisplay + charsize(kNormalDisplay));
   gfx::Size pixel;
   gfx::Size size;
-  EXPECT_TRUE(ParseOutputDeviceData(edid, &manufacturer_id,
+  EXPECT_TRUE(ParseOutputDeviceData(edid, &manufacturer_id, &product_code,
                                     &human_readable_name, &pixel, &size));
   EXPECT_EQ(0x22f0u, manufacturer_id);
+  EXPECT_EQ(0x286cu, product_code);
   EXPECT_EQ("HP ZR30w", human_readable_name);
   EXPECT_EQ("2560x1600", pixel.ToString());
   EXPECT_EQ("641x400", size.ToString());
 
   manufacturer_id = 0;
+  product_code = 0;
   human_readable_name.clear();
   Reset(&pixel, &size);
   edid.assign(kInternalDisplay, kInternalDisplay + charsize(kInternalDisplay));
 
-  EXPECT_TRUE(
-      ParseOutputDeviceData(edid, &manufacturer_id, nullptr, &pixel, &size));
+  EXPECT_TRUE(ParseOutputDeviceData(edid, &manufacturer_id, &product_code,
+                                    nullptr, &pixel, &size));
   EXPECT_EQ(0x4ca3u, manufacturer_id);
+  EXPECT_EQ(0x3142u, product_code);
   EXPECT_EQ("", human_readable_name);
   EXPECT_EQ("1280x800", pixel.ToString());
   EXPECT_EQ("261x163", size.ToString());
 
   // Internal display doesn't have name.
-  EXPECT_TRUE(ParseOutputDeviceData(edid, nullptr, &human_readable_name, &pixel,
-                                    &size));
+  EXPECT_TRUE(ParseOutputDeviceData(edid, nullptr, nullptr,
+                                    &human_readable_name, &pixel, &size));
   EXPECT_TRUE(human_readable_name.empty());
 
   manufacturer_id = 0;
+  product_code = 0;
   human_readable_name.clear();
   Reset(&pixel, &size);
   edid.assign(kOverscanDisplay, kOverscanDisplay + charsize(kOverscanDisplay));
-  EXPECT_TRUE(ParseOutputDeviceData(edid, &manufacturer_id,
+  EXPECT_TRUE(ParseOutputDeviceData(edid, &manufacturer_id, &product_code,
                                     &human_readable_name, &pixel, &size));
   EXPECT_EQ(0x4c2du, manufacturer_id);
+  EXPECT_EQ(0x08feu, product_code);
   EXPECT_EQ("SAMSUNG", human_readable_name);
   EXPECT_EQ("1920x1080", pixel.ToString());
   EXPECT_EQ("160x90", size.ToString());
@@ -195,13 +201,14 @@ TEST(EDIDParserTest, ParseEDID) {
 
 TEST(EDIDParserTest, ParseBrokenEDID) {
   uint16_t manufacturer_id = 0;
+  uint16_t product_code = 0;
   std::string human_readable_name;
   std::vector<uint8_t> edid;
 
   gfx::Size dummy;
 
   // length == 0
-  EXPECT_FALSE(ParseOutputDeviceData(edid, &manufacturer_id,
+  EXPECT_FALSE(ParseOutputDeviceData(edid, &manufacturer_id, &product_code,
                                      &human_readable_name, &dummy, &dummy));
 
   // name is broken. Copying kNormalDisplay and substitute its name data by
@@ -211,14 +218,16 @@ TEST(EDIDParserTest, ParseBrokenEDID) {
   // display's name data is embedded in byte 95-107 in this specific example.
   // Fix here too when the contents of kNormalDisplay is altered.
   edid[97] = '\x1b';
-  EXPECT_FALSE(ParseOutputDeviceData(edid, &manufacturer_id,
+  EXPECT_FALSE(ParseOutputDeviceData(edid, &manufacturer_id, nullptr,
                                      &human_readable_name, &dummy, &dummy));
 
   // If |human_readable_name| isn't specified, it skips parsing the name.
   manufacturer_id = 0;
-  EXPECT_TRUE(
-      ParseOutputDeviceData(edid, &manufacturer_id, nullptr, &dummy, &dummy));
+  product_code = 0;
+  EXPECT_TRUE(ParseOutputDeviceData(edid, &manufacturer_id, &product_code,
+                                    nullptr, &dummy, &dummy));
   EXPECT_EQ(0x22f0u, manufacturer_id);
+  EXPECT_EQ(0x286cu, product_code);
 }
 
 TEST(EDIDParserTest, GetDisplayId) {
@@ -232,7 +241,10 @@ TEST(EDIDParserTest, GetDisplayId) {
   edid.assign(kLP2565B, kLP2565B + charsize(kLP2565B));
   EXPECT_TRUE(GetDisplayIdFromEDID(edid, 0, &id2, &product_id2));
   EXPECT_EQ(id1, id2);
-  EXPECT_EQ(product_id1, product_id2);
+  // The product code in the two EDIDs varies.
+  EXPECT_NE(product_id1, product_id2);
+  EXPECT_EQ(0x22f02676, product_id1);
+  EXPECT_EQ(0x22f02675, product_id2);
   EXPECT_NE(-1, id1);
   EXPECT_NE(-1, product_id1);
 }

ui/display/util/x11/edid_parser_x11.cc

@@ -86,8 +86,8 @@ bool GetOutputDeviceData(XID output,
   if (!GetEDIDProperty(output, &edid))
     return false;
 
-  return ParseOutputDeviceData(edid, manufacturer_id, human_readable_name,
-                               nullptr, nullptr);
+  return ParseOutputDeviceData(edid, manufacturer_id, nullptr,
+                               human_readable_name, nullptr, nullptr);
 }
 
 }  // namespace

ui/ozone/platform/drm/common/drm_util.cc

@@ -231,7 +231,7 @@ DisplaySnapshot_Params CreateDisplaySnapshotParams(
                               &params.product_id))
       params.display_id = display_index;
 
-    ParseOutputDeviceData(edid, nullptr, &params.display_name, nullptr,
+    ParseOutputDeviceData(edid, nullptr, nullptr, &params.display_name, nullptr,
                           nullptr);
     ParseOutputOverscanFlag(edid, &params.has_overscan);
   } else {