Fix the byte positions for Gx and Wx in the EDID parser.

This CL corrects two erroneous byte positions during EDID parsing, namely for
Green and White LSByte positions. Affected unit tests are fixed.

Bug: 821393
Change-Id: Ica3c54258e0d58b55bd5c5779e9ef6b6f5e4a610
Reviewed-on: https://chromium-review.googlesource.com/1005534
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551133}

ui/display/util/edid_parser.cc

@@ -182,13 +182,13 @@ void EdidParser::ParseEdid(const std::vector<uint8_t>& edid) {
   constexpr size_t kRedGreenLsbOffset = 25;
   constexpr uint8_t kRedxLsbPosition = 6;
   constexpr uint8_t kRedyLsbPosition = 4;
-  constexpr uint8_t kGreenxLsbPosition = 3;
+  constexpr uint8_t kGreenxLsbPosition = 2;
   constexpr uint8_t kGreenyLsbPosition = 0;
 
   constexpr size_t kBlueWhiteLsbOffset = 26;
   constexpr uint8_t kBluexLsbPosition = 6;
   constexpr uint8_t kBlueyLsbPosition = 4;
-  constexpr uint8_t kWhitexLsbPosition = 3;
+  constexpr uint8_t kWhitexLsbPosition = 2;
   constexpr uint8_t kWhiteyLsbPosition = 0;
 
   // All LSBits parts are 2 bits wide.

ui/display/util/edid_parser_unittest.cc

@@ -152,27 +152,29 @@ constexpr unsigned char kEve[] =
     "\x00\x4c\x51\x31\x32\x33\x50\x31\x4a\x58\x33\x32\x0a\x20\x00\xb6";
 constexpr size_t kEveLength = arraysize(kEve);
 
+// Primaries coordinates ({RX, RY, GX, GY, BX, BY, WX, WY}) calculated by hand
+// and rounded to 4 decimal places.
 constexpr SkColorSpacePrimaries kNormalDisplayPrimaries = {
-    0.6777f, 0.3086f, 0.2080f, 0.6923f, 0.1465f, 0.0546f, 0.3125f, 0.3291f};
+    0.6777f, 0.3086f, 0.2100f, 0.6924f, 0.1465f, 0.0547f, 0.3135f, 0.3291f};
 constexpr SkColorSpacePrimaries kInternalDisplayPrimaries = {
-    0.5849f, 0.3603f, 0.3769f, 0.5654f, 0.1552f, 0.0996f, 0.3125f, 0.3291f};
+    0.5850f, 0.3604f, 0.3750f, 0.5654f, 0.1553f, 0.0996f, 0.3135f, 0.3291f};
 constexpr SkColorSpacePrimaries kOverscanDisplayPrimaries = {
-    0.6396f, 0.3291f, 0.2978f, 0.5996f, 0.1494f, 0.0595f, 0.3144f, 0.3291f};
+    0.6396f, 0.3301f, 0.2998f, 0.5996f, 0.1504f, 0.0596f, 0.3125f, 0.3291f};
 constexpr SkColorSpacePrimaries kMisdetectedDisplayPrimaries = {
-    0.6777f, 0.3086f, 0.2080f, 0.6923f, 0.1465f, 0.0546f, 0.3125f, 0.3291f};
+    0.6777f, 0.3086f, 0.2100f, 0.6924f, 0.1465f, 0.0547f, 0.3135f, 0.3291f};
 constexpr SkColorSpacePrimaries kLP2565APrimaries = {
-    0.6396f, 0.3291f, 0.2978f, 0.6083f, 0.1494f, 0.0595f, 0.3144f, 0.3291f};
+    0.6396f, 0.3301f, 0.2998f, 0.6084f, 0.1504f, 0.0596f, 0.3135f, 0.3291f};
 constexpr SkColorSpacePrimaries kLP2565BPrimaries = {
-    0.6396f, 0.3291f, 0.2978f, 0.6083f, 0.1494f, 0.0595f, 0.3144f, 0.3291f};
+    0.6396f, 0.3301f, 0.2998f, 0.6084f, 0.1504f, 0.0596f, 0.3135f, 0.3291f};
 constexpr SkColorSpacePrimaries kHPz32xPrimaries = {
-    0.6738f, 0.3164f, 0.1962f, 0.7197f, 0.1484f, 0.0439f, 0.3144f, 0.3291f};
+    0.6738f, 0.3164f, 0.1982f, 0.7197f, 0.1484f, 0.0439f, 0.3135f, 0.3291f};
 constexpr SkColorSpacePrimaries kSamusPrimaries = {
-    0.6337f, 0.3476f, 0.3212f, 0.5771f, 0.1513f, 0.0908f, 0.3144f, 0.3291f};
+    0.6338f, 0.3477f, 0.3232f, 0.5771f, 0.1514f, 0.0908f, 0.3135f, 0.3291f};
 constexpr SkColorSpacePrimaries kEvePrimaries = {
-    0.6396f, 0.3291f, 0.2998f, 0.5996f, 0.1494f, 0.0595f, 0.3144f, 0.3281f};
+    0.6396f, 0.3291f, 0.2998f, 0.5996f, 0.1494f, 0.0596f, 0.3125f, 0.3281f};
 
 // Chromaticity primaries in EDID are specified with 10 bits precision.
-constexpr static float kPrimariesPrecision = 0.001f;
+constexpr static float kPrimariesPrecision = 1 / 2048.f;
 
 ::testing::AssertionResult SkColorSpacePrimariesEquals(
     const char* lhs_expr,

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

@@ -8,8 +8,12 @@
 #include <xf86drmMode.h>
 
 #include <map>
+#include <string>
 
+#include "base/numerics/ranges.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkColorSpace.h"
+#include "third_party/skia/include/core/SkMatrix44.h"
 #include "ui/display/types/display_snapshot.h"
 #include "ui/display/util/edid_parser.h"
 #include "ui/gfx/geometry/size.h"
@@ -306,40 +310,66 @@ TEST_F(DrmUtilTest, OverlaySurfaceCandidate) {
 }
 
 TEST_F(DrmUtilTest, GetColorSpaceFromEdid) {
+  // Test with HP z32x monitor.
+  constexpr SkColorSpacePrimaries expected_hpz32x_primaries = {
+      .fRX = 0.673828f,
+      .fRY = 0.316406f,
+      .fGX = 0.198242f,
+      .fGY = 0.719727f,
+      .fBX = 0.148438f,
+      .fBY = 0.043945f,
+      .fWX = 0.313477f,
+      .fWY = 0.329102f};
+  SkMatrix44 expected_hpz32x_toXYZ50_matrix;
+  expected_hpz32x_primaries.toXYZD50(&expected_hpz32x_toXYZ50_matrix);
   const std::vector<uint8_t> hpz32x_edid(kHPz32x,
                                          kHPz32x + arraysize(kHPz32x) - 1);
-  const double hpz32x_toXYZ50_coeffs[] = {0.620465,   0.200003,  0.143752,  0.,
-                                          0.290159,   0.667573,  0.0422682, 0.,
-                                          0.00523514, 0.0673996, 0.752575,  0.};
-  SkMatrix44 hpz32x_toXYZ50_matrix;
-  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_EQ(hpz32x_color_space.ToString(),
+  const gfx::ColorSpace expected_hpz32x_color_space =
+      gfx::ColorSpace::CreateCustom(
+          expected_hpz32x_toXYZ50_matrix,
+          SkColorSpaceTransferFn({2.2, 1, 0, 0, 0, 0, 0}));
+  EXPECT_EQ(expected_hpz32x_color_space.ToString(),
             GetColorSpaceFromEdid(display::EdidParser(hpz32x_edid)).ToString());
 
+  // Test with Chromebook Samus internal display.
+  constexpr SkColorSpacePrimaries expected_samus_primaries = {.fRX = 0.633789f,
+                                                              .fRY = 0.347656f,
+                                                              .fGX = 0.323242f,
+                                                              .fGY = 0.577148f,
+                                                              .fBX = 0.151367f,
+                                                              .fBY = 0.090820f,
+                                                              .fWX = 0.313477f,
+                                                              .fWY = 0.329102f};
+  SkMatrix44 expected_samus_toXYZ50_matrix;
+  expected_samus_primaries.toXYZD50(&expected_samus_toXYZ50_matrix);
   const std::vector<uint8_t> samus_edid(kSamus, kSamus + arraysize(kSamus) - 1);
-  const double samus_toXYZ50_coeffs[] = {0.41211,    0.39743,  0.15468,  0.,
-                                         0.22317,    0.674029, 0.102801, 0.,
-                                         0.00831561, 0.095953, 0.720941, 0.};
-  SkMatrix44 samus_toXYZ50_matrix;
-  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_EQ(samus_color_space.ToString(),
+  const gfx::ColorSpace expected_samus_color_space =
+      gfx::ColorSpace::CreateCustom(
+          expected_samus_toXYZ50_matrix,
+          SkColorSpaceTransferFn({2.5, 1, 0, 0, 0, 0, 0}));
+  EXPECT_EQ(expected_samus_color_space.ToString(),
             GetColorSpaceFromEdid(display::EdidParser(samus_edid)).ToString());
 
+  // Test with Chromebook Eve internal display.
+  constexpr SkColorSpacePrimaries expected_eve_primaries = {.fRX = 0.639648f,
+                                                            .fRY = 0.329102f,
+                                                            .fGX = 0.299805f,
+                                                            .fGY = 0.599609f,
+                                                            .fBX = 0.149414f,
+                                                            .fBY = 0.059570f,
+                                                            .fWX = 0.312500f,
+                                                            .fWY = 0.328125f};
+  SkMatrix44 expected_eve_toXYZ50_matrix;
+  expected_eve_primaries.toXYZD50(&expected_eve_toXYZ50_matrix);
   const std::vector<uint8_t> eve_edid(kEve, kEve + arraysize(kEve) - 1);
-  const double eve_toXYZ50_coeffs[] = {0.444601,  0.377972,  0.141646,  0.,
-                                       0.226825,  0.713295,  0.0598808, 0.,
-                                       0.0149676, 0.0972496, 0.712993,  0.};
-  SkMatrix44 eve_toXYZ50_matrix;
-  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_EQ(eve_color_space.ToString(),
+  const gfx::ColorSpace expected_eve_color_space =
+      gfx::ColorSpace::CreateCustom(
+          expected_eve_toXYZ50_matrix,
+          SkColorSpaceTransferFn({2.2, 1, 0, 0, 0, 0, 0}));
+  EXPECT_EQ(expected_eve_color_space.ToString(),
             GetColorSpaceFromEdid(display::EdidParser(eve_edid)).ToString());
 
+  // Test with gamma marked as non-existent.
   const std::vector<uint8_t> no_gamma_edid(
       kEdidWithNoGamma, kEdidWithNoGamma + arraysize(kEdidWithNoGamma) - 1);
   const gfx::ColorSpace no_gamma_color_space =