Edid: make a class out of the edid parsing functions

This CL refactors the multiple individual functions of edid_parser.*
into a single EdidParser class that parses an EDID blob passed on
ctor, populating the necessary/available fields.  This simplifies
extending this parser (which has happened and is expected to happen
again), and simplifies the client APIs and the unittests.

The client contract is changed a bit, since EdidParser won't tell
if the parsing failed, but this is not really impactful since the
current users ignore this booleans anyhow. Erroneous parsing is
signalled via error values.

Besides that, a notable change is that now we're parsing the whole
EDID whereas on ToT this was somewhat on demand. But anyway
drm_util always wanted all those parsings, and EDIDParserX11,
which only needed one on ToT, is expected to work on Linux, where
walking a 128B array should not be very demanding.

Also EDIDParserX11 had two unused methods, removed.

Change-Id: Id38a02dad1bc4ba55a3f7041034098dac5153020
Bug: 821393
Reviewed-on: https://chromium-review.googlesource.com/970085Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548858}

ui/display/manager/chromeos/display_change_observer.cc

@@ -317,10 +317,11 @@ ManagedDisplayInfo DisplayChangeObserver::CreateManagedDisplayInfo(
   if (snapshot->product_code() != DisplaySnapshot::kInvalidProductCode) {
     uint16_t manufacturer_id = 0;
     uint16_t product_id = 0;
-    SplitProductCodeInManufacturerIdAndProductId(snapshot->product_code(),
-                                                 &manufacturer_id, &product_id);
-    new_info.set_manufacturer_id(ManufacturerIdToString(manufacturer_id));
-    new_info.set_product_id(ProductIdToString(product_id));
+    EdidParser::SplitProductCodeInManufacturerIdAndProductId(
+        snapshot->product_code(), &manufacturer_id, &product_id);
+    new_info.set_manufacturer_id(
+        EdidParser::ManufacturerIdToString(manufacturer_id));
+    new_info.set_product_id(EdidParser::ProductIdToString(product_id));
   }
   new_info.set_year_of_manufacture(snapshot->year_of_manufacture());
 

ui/display/util/edid_parser.h

@@ -11,77 +11,68 @@
 #include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/optional.h"
+#include "third_party/skia/include/core/SkColorSpace.h"
 #include "ui/display/util/display_util_export.h"
-
-namespace gfx {
-class Size;
-}
-
-struct SkColorSpacePrimaries;
-
-// EDID (Extended Display Identification Data) is a format for monitor
-// metadata. This provides a parser for the data.
+#include "ui/gfx/geometry/size.h"
 
 namespace display {
 
-// Generates the display id and product id for the pair of |edid| and |index|,
-// and store in |display_id_out| and |product_code_out|. Returns true if the
-// display id is successfully generated, or false otherwise.
-DISPLAY_UTIL_EXPORT bool GetDisplayIdFromEDID(const std::vector<uint8_t>& edid,
-                                              uint8_t index,
-                                              int64_t* display_id_out,
-                                              int64_t* product_code_out);
-
-// Parses |edid| as EDID data and stores extracted data into |manufacturer_id|,
-// |product_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.
-DISPLAY_UTIL_EXPORT bool ParseOutputDeviceData(
-    const std::vector<uint8_t>& edid,
-    uint16_t* manufacturer_id,
-    uint16_t* product_id,
-    std::string* human_readable_name,
-    gfx::Size* active_pixel_out,
-    gfx::Size* physical_display_size_out);
-
-// Splits the |product_code| (as returned by GetDisplayIdFromEDID()) into its
-// constituents |manufacturer_id| and |product_id|.
-DISPLAY_UTIL_EXPORT void SplitProductCodeInManufacturerIdAndProductId(
-    int64_t product_code,
-    uint16_t* manufacturer_id,
-    uint16_t* product_id);
-
-// Extracts the three letter Manufacturer ID out of |manufacturer_id|.
-DISPLAY_UTIL_EXPORT std::string ManufacturerIdToString(
-    uint16_t manufacturer_id);
-
-// Extracts the 2 Byte Product ID as hex out of |product_id|.
-DISPLAY_UTIL_EXPORT std::string ProductIdToString(uint16_t product_id);
-
-DISPLAY_UTIL_EXPORT bool ParseOutputOverscanFlag(
-    const std::vector<uint8_t>& edid,
-    bool* flag);
-
-DISPLAY_UTIL_EXPORT bool ParseYearOfManufacture(
-    const std::vector<uint8_t>& edid,
-    int32_t* year);
-
-// Extracts from |edid| the |primaries| chromaticity coordinates (CIE xy
-// coordinates for Red, Green and Blue channels and for the White Point).
-DISPLAY_UTIL_EXPORT bool ParseChromaticityCoordinates(
-    const std::vector<uint8_t>& edid,
-    SkColorSpacePrimaries* primaries) WARN_UNUSED_RESULT;
-
-// Extracts the gamma value from |edid| and returns it, or returns 0.0.
-DISPLAY_UTIL_EXPORT bool ParseGammaValue(const std::vector<uint8_t>& edid,
-                                         double* gamma) WARN_UNUSED_RESULT;
-
-// Extracts the bits per channel from |edid| and returns it, or returns 0.
-DISPLAY_UTIL_EXPORT bool ParseBitsPerChannel(const std::vector<uint8_t>& edid,
-                                             int* bits_per_channel)
-    WARN_UNUSED_RESULT;
+// This class parses a EDID (Extended Display Identification Data) binary blob
+// passed on constructor, and provides access to the parsed information, plus
+// a few utility postprocessings.
+class DISPLAY_UTIL_EXPORT EdidParser {
+ public:
+  explicit EdidParser(const std::vector<uint8_t>& edid_blob);
+  ~EdidParser();
+
+  uint16_t manufacturer_id() const { return manufacturer_id_; }
+  uint16_t product_id() const { return product_id_; }
+  const std::string& display_name() const { return display_name_; }
+  const gfx::Size& active_pixel_size() const { return active_pixel_size_; }
+  int32_t year_of_manufacture() const { return year_of_manufacture_; }
+  bool has_overscan_flag() const { return overscan_flag_.has_value(); }
+  bool overscan_flag() const { return overscan_flag_.value(); }
+  double gamma() const { return gamma_; }
+  int32_t bits_per_channel() const { return bits_per_channel_; }
+  const SkColorSpacePrimaries& primaries() const { return primaries_; }
+
+  // Returns a 32-bit identifier for this display |manufacturer_id_| and
+  // |product_id_|.
+  uint32_t GetProductCode() const;
+
+  // Generates a unique display id out of a mix of |manufacturer_id_|, hashed
+  // |display_name_| if available, and |output_index|.
+  int64_t GetDisplayId(uint8_t output_index) const;
+
+  // Splits the |product_code| (as returned by GetDisplayId()) into its
+  // constituents |manufacturer_id| and |product_id|.
+  static void SplitProductCodeInManufacturerIdAndProductId(
+      int64_t product_code,
+      uint16_t* manufacturer_id,
+      uint16_t* product_id);
+  // Extracts the three letter Manufacturer ID out of |manufacturer_id|.
+  static std::string ManufacturerIdToString(uint16_t manufacturer_id);
+  // Extracts the 2 Byte Product ID as hex out of |product_id|.
+  static std::string ProductIdToString(uint16_t product_id);
+
+ private:
+  // Parses |edid_blob|, filling up as many as possible fields below.
+  void ParseEdid(const std::vector<uint8_t>& edid);
+
+  uint16_t manufacturer_id_;
+  uint16_t product_id_;
+  std::string display_name_;
+  // Active pixel size from the first detailed timing descriptor in the EDID.
+  gfx::Size active_pixel_size_;
+  int32_t year_of_manufacture_;
+  base::Optional<bool> overscan_flag_;
+  double gamma_;
+  int bits_per_channel_;
+  SkColorSpacePrimaries primaries_;
+
+  DISALLOW_COPY_AND_ASSIGN(EdidParser);
+};
 
 }  // namespace display
 

ui/display/util/edid_parser_fuzzer.cc

@@ -23,17 +23,7 @@ Environment* env = new Environment();
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   std::vector<uint8_t> edid;
   edid.assign(data, data + size);
-  uint16_t manufacturer_id, product_code;
-  std::string human_readable_name;
-  gfx::Size active_pixel_size, physical_display_size;
-  bool overscan;
-  int32_t year_of_manufacture;
-
-  display::ParseOutputDeviceData(edid, &manufacturer_id, &product_code,
-                                 &human_readable_name, &active_pixel_size,
-                                 &physical_display_size);
-
-  display::ParseOutputOverscanFlag(edid, &overscan);
-  display::ParseYearOfManufacture(edid, &year_of_manufacture);
+  // Ctor already parses |edid|, which is what we want here.
+  display::EdidParser edid_parser(edid);
   return 0;
 }

ui/display/util/x11/edid_parser_x11.cc

@@ -73,33 +73,18 @@ bool GetEDIDProperty(XID output, std::vector<uint8_t>* edid) {
 
 }  // namespace
 
-EDIDParserX11::EDIDParserX11(XID output_id)
-    : output_id_(output_id) {
+EDIDParserX11::EDIDParserX11(XID output_id) : output_id_(output_id) {
   GetEDIDProperty(output_id_, &edid_);
 }
 
-EDIDParserX11::~EDIDParserX11() {
-}
+EDIDParserX11::~EDIDParserX11() {}
 
 bool EDIDParserX11::GetDisplayId(uint8_t index, int64_t* out_display_id) const {
   if (edid_.empty())
     return false;
 
-  return GetDisplayIdFromEDID(edid_, index, out_display_id, nullptr);
-}
-
-std::string EDIDParserX11::GetDisplayName() const {
-  std::string display_name;
-  ParseOutputDeviceData(edid_, nullptr, nullptr, &display_name, nullptr,
-                        nullptr);
-  return display_name;
-}
-
-bool EDIDParserX11::GetOutputOverscanFlag(bool* out_flag) const {
-  if (edid_.empty())
-    return false;
-
-  return ParseOutputOverscanFlag(edid_, out_flag);
+  *out_display_id = EdidParser(edid_).GetDisplayId(output_id_);
+  return true;
 }
 
 }  // namespace display

ui/display/util/x11/edid_parser_x11.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "ui/display/types/display_constants.h"
 #include "ui/display/util/display_util_export.h"
+#include "ui/display/util/edid_parser.h"
 
 typedef unsigned long XID;
 typedef XID RROutput;
@@ -31,22 +32,11 @@ class DISPLAY_UTIL_EXPORT EDIDParserX11 {
   // Returns true if successful, false otherwise.
   bool GetDisplayId(uint8_t index, int64_t* out_display_id) const;
 
-  // Generate the human readable string from EDID obtained from |output|.
-  // Returns an empty string upon error.
-  std::string GetDisplayName() const;
-
-  // Gets the overscan flag and stores to |out_flag|. Returns true if the flag
-  // is found. Otherwise returns false and doesn't touch |out_flag|. The output
-  // will produce overscan if |out_flag| is set to true, but the output may
-  // still produce overscan even though it returns true and |out_flag| is set to
-  // false.
-  bool GetOutputOverscanFlag(bool* out_flag) const;
-
   XID output_id() const { return output_id_; }
   const std::vector<uint8_t>& edid() const { return edid_; }
 
  private:
-  XID output_id_;
+  const XID output_id_;
 
   // This will be an empty vector upon failure to get the EDID from the
   // |output_id_|.

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

@@ -382,7 +382,7 @@ std::unique_ptr<display::DisplaySnapshot> CreateDisplaySnapshot(
     const base::FilePath& sys_path,
     size_t device_index,
     const gfx::Point& origin) {
-  int64_t display_id = ConnectorIndex(device_index, info->index());
+  const uint8_t display_index = ConnectorIndex(device_index, info->index());
   const gfx::Size physical_size =
       gfx::Size(info->connector()->mmWidth, info->connector()->mmHeight);
   const display::DisplayConnectionType type = GetDisplayType(info->connector());
@@ -392,33 +392,31 @@ std::unique_ptr<display::DisplaySnapshot> CreateDisplaySnapshot(
       HasColorCorrectionMatrix(fd, info->crtc());
   const gfx::Size maximum_cursor_size = GetMaximumCursorSize(fd);
 
-  std::vector<uint8_t> edid;
   std::string display_name;
+  int64_t display_id = -1;
   int64_t product_code = display::DisplaySnapshot::kInvalidProductCode;
   int32_t year_of_manufacture = display::kInvalidYearOfManufacture;
   bool has_overscan = false;
   gfx::ColorSpace display_color_space;
-
-  // This is the size of the active pixels from the first detailed timing
-  // descriptor in the EDID.
+  // Active pixels size from the first detailed timing descriptor in the EDID.
   gfx::Size active_pixel_size;
 
   ScopedDrmPropertyBlobPtr edid_blob(
       GetDrmPropertyBlob(fd, info->connector(), "EDID"));
+  std::vector<uint8_t> edid;
   if (edid_blob) {
     edid.assign(static_cast<uint8_t*>(edid_blob->data),
                 static_cast<uint8_t*>(edid_blob->data) + edid_blob->length);
 
-    // TODO(mcasas): GetDisplayIdFromEDID() calls ParseOutputDeviceData(), clean
-    // up the code and add UMA for EDID errors, https://crbug.com/821393. Also
-    // handle correctly the parsing failures of the following functions.
-    display::GetDisplayIdFromEDID(edid, display_id, &display_id, &product_code);
-    display::ParseOutputDeviceData(edid, nullptr, nullptr, &display_name,
-                                   &active_pixel_size, nullptr);
-    display::ParseYearOfManufacture(edid, &year_of_manufacture);
-    display::ParseOutputOverscanFlag(edid, &has_overscan);
-
-    display_color_space = GetColorSpaceFromEdid(edid);
+    display::EdidParser edid_parser(edid);
+    display_name = edid_parser.display_name();
+    active_pixel_size = edid_parser.active_pixel_size();
+    product_code = edid_parser.GetProductCode();
+    display_id = edid_parser.GetDisplayId(display_index);
+    year_of_manufacture = edid_parser.year_of_manufacture();
+    has_overscan =
+        edid_parser.has_overscan_flag() && edid_parser.overscan_flag();
+    display_color_space = GetColorSpaceFromEdid(edid_parser);
   } else {
     VLOG(1) << "Failed to get EDID blob for connector "
             << info->connector()->connector_id;
@@ -638,10 +636,8 @@ std::vector<OverlayCheckReturn_Params> CreateParamsFromOverlayStatusList(
   return params;
 }
 
-gfx::ColorSpace GetColorSpaceFromEdid(const std::vector<uint8_t>& edid) {
-  SkColorSpacePrimaries primaries = {0};
-  if (!display::ParseChromaticityCoordinates(edid, &primaries))
-    return gfx::ColorSpace();
+gfx::ColorSpace GetColorSpaceFromEdid(const display::EdidParser& edid_parser) {
+  const SkColorSpacePrimaries primaries = edid_parser.primaries();
 
   // Sanity check: primaries should verify By <= Ry <= Gy, Bx <= Rx and Gx <=
   // Rx, to guarantee that the R, G and B colors are each in the correct region.
@@ -677,8 +673,8 @@ gfx::ColorSpace GetColorSpaceFromEdid(const std::vector<uint8_t>& edid) {
   if (!primaries.toXYZD50(&color_space_as_matrix))
     return gfx::ColorSpace();
 
-  double gamma = 0.0;
-  if (!display::ParseGammaValue(edid, &gamma))
+  const double gamma = edid_parser.gamma();
+  if (gamma < 1.0)
     return gfx::ColorSpace();
 
   SkColorSpaceTransferFn transfer = {gamma, 1.f, 0.f, 0.f, 0.f, 0.f, 0.f};

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

@@ -21,6 +21,7 @@ typedef struct _drmModeModeInfo drmModeModeInfo;
 
 namespace display {
 class DisplayMode;
+class EdidParser;
 }  // namespace display
 
 namespace gfx {
@@ -119,9 +120,9 @@ OverlayStatusList CreateOverlayStatusListFrom(
 std::vector<OverlayCheckReturn_Params> CreateParamsFromOverlayStatusList(
     const OverlayStatusList& returns);
 
-// Parses |edid| to extract a gfx::ColorSpace which will be IsValid() if both
-// gamma and the color primaries were correctly found.
-gfx::ColorSpace GetColorSpaceFromEdid(const std::vector<uint8_t>& edid);
+// Uses |edid_parser| to extract a gfx::ColorSpace which will be IsValid() if
+// both gamma and the color primaries were correctly found.
+gfx::ColorSpace GetColorSpaceFromEdid(const display::EdidParser& edid_parser);
 
 }  // namespace ui
 

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

@@ -11,6 +11,7 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/display/types/display_snapshot.h"
+#include "ui/display/util/edid_parser.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/ozone/common/gpu/ozone_gpu_message_params.h"
 
@@ -314,8 +315,8 @@ TEST_F(DrmUtilTest, GetColorSpaceFromEdid) {
   hpz32x_toXYZ50_matrix.setRowMajord(hpz32x_toXYZ50_coeffs);
   const gfx::ColorSpace hpz32x_color_space = gfx::ColorSpace::CreateCustom(
       hpz32x_toXYZ50_matrix, SkColorSpaceTransferFn({2.2, 1, 0, 0, 0, 0, 0}));
-  EXPECT_STREQ(hpz32x_color_space.ToString().c_str(),
-               GetColorSpaceFromEdid(hpz32x_edid).ToString().c_str());
+  EXPECT_EQ(hpz32x_color_space.ToString(),
+            GetColorSpaceFromEdid(display::EdidParser(hpz32x_edid)).ToString());
 
   const std::vector<uint8_t> samus_edid(kSamus, kSamus + arraysize(kSamus) - 1);
   const double samus_toXYZ50_coeffs[] = {0.41211,    0.39743,  0.15468,  0.,
@@ -325,8 +326,8 @@ TEST_F(DrmUtilTest, GetColorSpaceFromEdid) {
   samus_toXYZ50_matrix.setRowMajord(samus_toXYZ50_coeffs);
   const gfx::ColorSpace samus_color_space = gfx::ColorSpace::CreateCustom(
       samus_toXYZ50_matrix, SkColorSpaceTransferFn({2.5, 1, 0, 0, 0, 0, 0}));
-  EXPECT_STREQ(samus_color_space.ToString().c_str(),
-               GetColorSpaceFromEdid(samus_edid).ToString().c_str());
+  EXPECT_EQ(samus_color_space.ToString(),
+            GetColorSpaceFromEdid(display::EdidParser(samus_edid)).ToString());
 
   const std::vector<uint8_t> eve_edid(kEve, kEve + arraysize(kEve) - 1);
   const double eve_toXYZ50_coeffs[] = {0.444601,  0.377972,  0.141646,  0.,
@@ -336,35 +337,37 @@ TEST_F(DrmUtilTest, GetColorSpaceFromEdid) {
   eve_toXYZ50_matrix.setRowMajord(eve_toXYZ50_coeffs);
   const gfx::ColorSpace eve_color_space = gfx::ColorSpace::CreateCustom(
       eve_toXYZ50_matrix, SkColorSpaceTransferFn({2.2, 1, 0, 0, 0, 0, 0}));
-  EXPECT_STREQ(eve_color_space.ToString().c_str(),
-               GetColorSpaceFromEdid(eve_edid).ToString().c_str());
+  EXPECT_EQ(eve_color_space.ToString(),
+            GetColorSpaceFromEdid(display::EdidParser(eve_edid)).ToString());
 
   const std::vector<uint8_t> no_gamma_edid(
       kEdidWithNoGamma, kEdidWithNoGamma + arraysize(kEdidWithNoGamma) - 1);
   const gfx::ColorSpace no_gamma_color_space =
-      GetColorSpaceFromEdid(no_gamma_edid);
+      GetColorSpaceFromEdid(display::EdidParser(no_gamma_edid));
   EXPECT_FALSE(no_gamma_color_space.IsValid());
 }
 
 TEST_F(DrmUtilTest, GetInvalidColorSpaceFromEdid) {
   const std::vector<uint8_t> empty_edid;
-  EXPECT_EQ(gfx::ColorSpace(), GetColorSpaceFromEdid(empty_edid));
+  EXPECT_EQ(gfx::ColorSpace(),
+            GetColorSpaceFromEdid(display::EdidParser(empty_edid)));
 
   const std::vector<uint8_t> invalid_edid(
       kInvalidEdid, kInvalidEdid + arraysize(kInvalidEdid) - 1);
   const gfx::ColorSpace invalid_color_space =
-      GetColorSpaceFromEdid(invalid_edid);
+      GetColorSpaceFromEdid(display::EdidParser(invalid_edid));
   EXPECT_FALSE(invalid_color_space.IsValid());
 
   const std::vector<uint8_t> sst210_edid(kSST210,
                                          kSST210 + arraysize(kSST210) - 1);
-  const gfx::ColorSpace sst210_color_space = GetColorSpaceFromEdid(sst210_edid);
+  const gfx::ColorSpace sst210_color_space =
+      GetColorSpaceFromEdid(display::EdidParser(sst210_edid));
   EXPECT_FALSE(sst210_color_space.IsValid()) << sst210_color_space.ToString();
 
   const std::vector<uint8_t> sst210_edid_2(
       kSST210Corrected, kSST210Corrected + arraysize(kSST210Corrected) - 1);
   const gfx::ColorSpace sst210_color_space_2 =
-      GetColorSpaceFromEdid(sst210_edid_2);
+      GetColorSpaceFromEdid(display::EdidParser(sst210_edid_2));
   EXPECT_FALSE(sst210_color_space_2.IsValid())
       << sst210_color_space_2.ToString();
 
@@ -372,7 +375,7 @@ TEST_F(DrmUtilTest, GetInvalidColorSpaceFromEdid) {
       kBrokenBluePrimaries,
       kBrokenBluePrimaries + arraysize(kBrokenBluePrimaries) - 1);
   const gfx::ColorSpace broken_blue_color_space =
-      GetColorSpaceFromEdid(broken_blue_edid);
+      GetColorSpaceFromEdid(display::EdidParser(broken_blue_edid));
   EXPECT_FALSE(broken_blue_color_space.IsValid())
       << broken_blue_color_space.ToString();
 }