Send ICCProfile for output display to renderer
Two motivations for this:
1. On Mac, this ensures that the ICCProfile sent to IOSurfaces
(in the renderer) will match the ICCProfile of the display
exactly, ensuring that the WindowServer will not need to do
any color conversion.
2. On all platforms, this fixes a bug wherein GetRasterColorSpace
would fail to find the ICCProfile that it is approximating,
and so it would inappropriately return sRGB instead of a
wide color gamut space.
Fix some tests, and add more tests:
Remove the hard-coded ICC profile IDs that were used for test
color spaces outside of layout tests since they aren't needed,
and were masking bugs in ID tracking.
Add unit tests for the ICC profile cache. The ICC profile
cache is messy and should be removed entirely, but these
tests will at least alert us to changes in behavior, until
we remove it entirely (see crbug.com/766736).
R=dcheng, hubbe
TBR=avi (content/), ccameron@chromium.org
(cherry picked from commit 64a55ad8cf4f8601c5424de21e6900becd890640)
Bug: 764352
Change-Id: Id41c8d8dc2c262a0ecc3f7aa25dcc70f334e7667
Reviewed-on: https://chromium-review.googlesource.com/672154
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503362}
Reviewed-on: https://chromium-review.googlesource.com/683355
Reviewed-by: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#452}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}diff --git a/content/browser/web_contents/web_contents_view_aura.cc b/content/browser/web_contents/web_contents_view_aura.cc
index f407726..53ad9c8 100644
--- a/content/browser/web_contents/web_contents_view_aura.cc
+++ b/content/browser/web_contents/web_contents_view_aura.cc
@@ -702,6 +702,7 @@
results->is_monochrome = display.is_monochrome();
results->device_scale_factor = display.device_scale_factor();
results->color_space = display.color_space();
+ results->color_space.GetICCProfile(&results->icc_profile);
// The Display rotation and the ScreenInfo orientation are not the same
// angle. The former is the physical display rotation while the later is the
diff --git a/content/browser/web_contents/web_contents_view_mac.mm b/content/browser/web_contents/web_contents_view_mac.mm
index a9111f9..92b48b71 100644
--- a/content/browser/web_contents/web_contents_view_mac.mm
+++ b/content/browser/web_contents/web_contents_view_mac.mm
@@ -90,6 +90,7 @@
content::ScreenInfo results;
results.device_scale_factor = static_cast<int>(display.device_scale_factor());
results.color_space = display.color_space();
+ results.color_space.GetICCProfile(&results.icc_profile);
results.depth = display.color_depth();
results.depth_per_component = display.depth_per_component();
results.is_monochrome = display.is_monochrome();
diff --git a/content/common/view_messages.h b/content/common/view_messages.h
index 7562844..ff00cec 100644
--- a/content/common/view_messages.h
+++ b/content/common/view_messages.h
@@ -162,6 +162,7 @@
IPC_STRUCT_TRAITS_BEGIN(content::ScreenInfo)
IPC_STRUCT_TRAITS_MEMBER(device_scale_factor)
IPC_STRUCT_TRAITS_MEMBER(color_space)
+ IPC_STRUCT_TRAITS_MEMBER(icc_profile)
IPC_STRUCT_TRAITS_MEMBER(depth)
IPC_STRUCT_TRAITS_MEMBER(depth_per_component)
IPC_STRUCT_TRAITS_MEMBER(is_monochrome)
diff --git a/content/public/common/screen_info.h b/content/public/common/screen_info.h
index 0a2afbb..a6ff7c7 100644
--- a/content/public/common/screen_info.h
+++ b/content/public/common/screen_info.h
@@ -9,6 +9,7 @@
#include "content/public/common/screen_orientation_values.h"
#include "ui/gfx/color_space.h"
#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/icc_profile.h"
namespace content {
@@ -26,6 +27,9 @@
// The color space of the output display.
gfx::ColorSpace color_space = gfx::ColorSpace::CreateSRGB();
+ // The ICC profile from which |color_space| was derived, if any.
+ gfx::ICCProfile icc_profile;
+
// The screen depth in bits per pixel
uint32_t depth = 0;
diff --git a/third_party/WebKit/Source/platform/graphics/ColorSpaceGamut.cpp b/third_party/WebKit/Source/platform/graphics/ColorSpaceGamut.cpp
index d83dabf..bf6386d9 100644
--- a/third_party/WebKit/Source/platform/graphics/ColorSpaceGamut.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ColorSpaceGamut.cpp
@@ -15,9 +15,10 @@
const gfx::ColorSpace& color_space = screen_info.color_space;
if (!color_space.IsValid())
return ColorSpaceGamut::kUnknown;
-
+ // Return the gamut of the color space used for raster (this will return a
+ // wide gamut for HDR profiles).
return ColorSpaceUtilities::GetColorSpaceGamut(
- color_space.ToSkColorSpace().get());
+ color_space.GetRasterColorSpace().ToSkColorSpace().get());
}
ColorSpaceGamut GetColorSpaceGamut(SkColorSpace* color_space) {
diff --git a/ui/gfx/color_space.cc b/ui/gfx/color_space.cc
index 6044aae..b77a6f6 100644
--- a/ui/gfx/color_space.cc
+++ b/ui/gfx/color_space.cc
@@ -167,14 +167,23 @@
if (primaries_ != other.primaries_ || transfer_ != other.transfer_ ||
matrix_ != other.matrix_ || range_ != other.range_)
return false;
- if (primaries_ == PrimaryID::CUSTOM &&
- memcmp(custom_primary_matrix_, other.custom_primary_matrix_,
- sizeof(custom_primary_matrix_)))
- return false;
- if (transfer_ == TransferID::CUSTOM &&
- memcmp(custom_transfer_params_, other.custom_transfer_params_,
- sizeof(custom_transfer_params_)))
- return false;
+ if (primaries_ == PrimaryID::CUSTOM) {
+ if (memcmp(custom_primary_matrix_, other.custom_primary_matrix_,
+ sizeof(custom_primary_matrix_))) {
+ return false;
+ }
+ }
+ if (transfer_ == TransferID::CUSTOM) {
+ if (memcmp(custom_transfer_params_, other.custom_transfer_params_,
+ sizeof(custom_transfer_params_))) {
+ return false;
+ }
+ }
+ if (primaries_ == PrimaryID::ICC_BASED ||
+ transfer_ == TransferID::ICC_BASED) {
+ if (icc_profile_id_ != other.icc_profile_id_)
+ return false;
+ }
return true;
}
@@ -204,8 +213,12 @@
// Query the ICC profile, if available, for the parametric approximation.
ICCProfile icc_profile;
- if (GetICCProfile(&icc_profile))
+ if (GetICCProfile(&icc_profile)) {
return icc_profile.GetParametricColorSpace();
+ } else {
+ DLOG(ERROR)
+ << "Unable to acquire ICC profile for parametric approximation.";
+ }
// Fall back to sRGB if the ICC profile is no longer cached.
return CreateSRGB();
diff --git a/ui/gfx/color_space.h b/ui/gfx/color_space.h
index 0fdc44f..ba8d5b5 100644
--- a/ui/gfx/color_space.h
+++ b/ui/gfx/color_space.h
@@ -22,6 +22,7 @@
namespace gfx {
class ICCProfile;
+class ICCProfileCache;
// Used to represet a color space for the purpose of color conversion.
// This is designed to be safe and compact enough to send over IPC
@@ -230,6 +231,7 @@
sk_sp<SkColorSpace> icc_profile_sk_color_space_;
friend class ICCProfile;
+ friend class ICCProfileCache;
friend class ColorTransform;
friend class ColorTransformInternal;
friend class ColorSpaceWin;
diff --git a/ui/gfx/icc_profile.cc b/ui/gfx/icc_profile.cc
index d69b746a..0a9517ac 100644
--- a/ui/gfx/icc_profile.cc
+++ b/ui/gfx/icc_profile.cc
@@ -23,9 +23,6 @@
const uint64_t ICCProfile::test_id_color_spin_ = 2;
const uint64_t ICCProfile::test_id_generic_rgb_ = 3;
const uint64_t ICCProfile::test_id_srgb_ = 4;
-const uint64_t ICCProfile::test_id_no_analytic_tr_fn_ = 5;
-const uint64_t ICCProfile::test_id_a2b_only_ = 6;
-const uint64_t ICCProfile::test_id_overshoot_ = 7;
// A MRU cache of ICC profiles. The cache key is a uin64_t which a
// gfx::ColorSpace may use to refer back to an ICC profile in the cache. The
@@ -58,6 +55,17 @@
if (!icc_profile->id_)
icc_profile->id_ = next_unused_id_++;
+ // Ensure that GetColorSpace() point back to this ICCProfile.
+ gfx::ColorSpace& color_space = icc_profile->color_space_;
+ color_space.icc_profile_id_ = icc_profile->id_;
+
+ // Ensure that the GetParametricColorSpace() point back to this ICCProfile
+ // only if the parametric version is accurate.
+ if (color_space.primaries_ != ColorSpace::PrimaryID::ICC_BASED &&
+ color_space.transfer_ != ColorSpace::TransferID::ICC_BASED) {
+ icc_profile->parametric_color_space_.icc_profile_id_ = icc_profile->id_;
+ }
+
Entry entry;
entry.icc_profile = *icc_profile;
id_to_icc_profile_mru_.Put(icc_profile->id_, entry);
@@ -102,6 +110,13 @@
if (found != id_to_icc_profile_mru_.end())
return;
+ // Look up the profile by its data. If there is a new entry for the same
+ // data, don't add a duplicate.
+ if (FindByDataUnderLock(icc_profile.data_.data(), icc_profile.data_.size(),
+ nullptr)) {
+ return;
+ }
+
// If the entry was not found, insert it.
Entry entry;
entry.icc_profile = icc_profile;
@@ -162,8 +177,10 @@
if (iter_data.size() != size || memcmp(data, iter_data.data(), size))
continue;
- *icc_profile = cached_profile;
- id_to_icc_profile_mru_.Get(cached_profile.id_);
+ if (icc_profile) {
+ *icc_profile = cached_profile;
+ id_to_icc_profile_mru_.Get(cached_profile.id_);
+ }
return true;
}
return false;
@@ -377,7 +394,6 @@
case kICCExtractedMatrixAndAnalyticTrFn:
case kICCExtractedMatrixAndApproximatedTrFn:
// Successfully and accurately extracted color space.
- parametric_color_space_.icc_profile_id_ = id_;
color_space_ = parametric_color_space_;
break;
case kICCFailedToExtractRawTrFn:
@@ -387,7 +403,6 @@
// Successfully but extracted a color space, but it isn't accurate enough.
color_space_ = ColorSpace(ColorSpace::PrimaryID::ICC_BASED,
ColorSpace::TransferID::ICC_BASED);
- color_space_.icc_profile_id_ = id_;
color_space_.icc_profile_sk_color_space_ = useable_sk_color_space;
break;
case kICCFailedToParse:
diff --git a/ui/gfx/icc_profile.h b/ui/gfx/icc_profile.h
index 4049df8..257b536 100644
--- a/ui/gfx/icc_profile.h
+++ b/ui/gfx/icc_profile.h
@@ -51,11 +51,6 @@
static ICCProfile FromCGColorSpace(CGColorSpaceRef cg_color_space);
#endif
- // This will recover a ICCProfile from a compact ColorSpace representation.
- // Internally, this will make an effort to create an identical ICCProfile
- // to the one that created |color_space|, but this is not guaranteed.
- static ICCProfile FromColorSpace(const gfx::ColorSpace& color_space);
-
// Create directly from profile data.
static ICCProfile FromData(const void* icc_profile, size_t size);
@@ -102,25 +97,19 @@
friend ICCProfile ICCProfileForTestingColorSpin();
friend ICCProfile ICCProfileForTestingGenericRGB();
friend ICCProfile ICCProfileForTestingSRGB();
- friend ICCProfile ICCProfileForTestingNoAnalyticTrFn();
- friend ICCProfile ICCProfileForTestingA2BOnly();
- friend ICCProfile ICCProfileForTestingOvershoot();
friend ICCProfile ICCProfileForLayoutTests();
static const uint64_t test_id_adobe_rgb_;
static const uint64_t test_id_color_spin_;
static const uint64_t test_id_generic_rgb_;
static const uint64_t test_id_srgb_;
- static const uint64_t test_id_no_analytic_tr_fn_;
- static const uint64_t test_id_a2b_only_;
- static const uint64_t test_id_overshoot_;
// Populate |icc_profile| with the ICCProfile corresponding to id |id|. Return
// false if |id| is not in the cache.
static bool FromId(uint64_t id, ICCProfile* icc_profile);
// This method is used to hard-code the |id_| to a specific value, and is
- // used by test methods to ensure that they don't conflict with the values
- // generated in the browser.
+ // used by layout test methods to ensure that they don't conflict with the
+ // values generated in the browser.
static ICCProfile FromDataWithId(const void* icc_profile,
size_t size,
uint64_t id);
diff --git a/ui/gfx/icc_profile_unittest.cc b/ui/gfx/icc_profile_unittest.cc
index e1222d5a..d9a008d 100644
--- a/ui/gfx/icc_profile_unittest.cc
+++ b/ui/gfx/icc_profile_unittest.cc
@@ -122,6 +122,9 @@
// as invalid.
ICCProfile a2b = ICCProfileForTestingA2BOnly();
EXPECT_FALSE(a2b.GetColorSpace().IsValid());
+
+ // Even though it is invalid, it should not be equal to the empty constructor.
+ EXPECT_NE(a2b, gfx::ICCProfile());
}
TEST(ICCProfile, GarbageData) {
@@ -158,4 +161,97 @@
EXPECT_TRUE(SkMatrixIsApproximatelyIdentity(eye));
}
+// This tests the ICCProfile MRU cache. This cache is sloppy and should be
+// rewritten, now that some of the original design constraints have been lifted.
+// This test exists only to ensure that we are aware of behavior changes, not to
+// enforce that behavior does not change.
+// https://crbug.com/766736
+TEST(ICCProfile, ExhaustCache) {
+ // Get an ICCProfile that can't be parametrically approximated.
+ ICCProfile original = ICCProfileForTestingNoAnalyticTrFn();
+ ColorSpace original_color_space_0 = original.GetColorSpace();
+
+ // Recover the ICCProfile from its GetColorSpace. Recovery should succeed, and
+ // the ICCProfiles should be equal.
+ ICCProfile recovered_0;
+ EXPECT_TRUE(original_color_space_0.GetICCProfile(&recovered_0));
+ EXPECT_EQ(original, recovered_0);
+
+ // The GetColorSpace of the recovered version should match the original.
+ ColorSpace recovered_0_color_space = recovered_0.GetColorSpace();
+ EXPECT_EQ(recovered_0_color_space, original_color_space_0);
+
+ // Create an identical ICCProfile to the original. It should equal the
+ // original, and its GetColorSpace should equal the original.
+ ICCProfile identical_0 = ICCProfileForTestingNoAnalyticTrFn();
+ EXPECT_EQ(original, identical_0);
+ ColorSpace identical_color_space_0 = identical_0.GetColorSpace();
+ EXPECT_EQ(identical_color_space_0, original_color_space_0);
+
+ // Create 128 distinct ICC profiles. This will destroy the cached
+ // ICCProfile<->ColorSpace mapping.
+ for (size_t i = 0; i < 128; ++i) {
+ SkMatrix44 toXYZD50;
+ ColorSpace::CreateSRGB().GetPrimaryMatrix(&toXYZD50);
+ SkColorSpaceTransferFn fn;
+ fn.fA = 1;
+ fn.fB = 0;
+ fn.fC = 1;
+ fn.fD = 0;
+ fn.fE = 0;
+ fn.fF = 0;
+ fn.fG = 1.5f + i / 128.f;
+ ColorSpace color_space = ColorSpace::CreateCustom(toXYZD50, fn);
+ ICCProfile icc_profile;
+ color_space.GetICCProfile(&icc_profile);
+ }
+
+ // Recover the original ICCProfile from its GetColorSpace. Recovery should
+ // fail, because it has been pushed out of the cache.
+ ICCProfile recovered_1;
+ EXPECT_FALSE(original_color_space_0.GetICCProfile(&recovered_1));
+ EXPECT_NE(original, recovered_1);
+
+ // Create an identical ICCProfile to the original. It should equal the
+ // original, because the comparison is based on data.
+ ICCProfile identical_1 = ICCProfileForTestingNoAnalyticTrFn();
+ EXPECT_EQ(original, identical_1);
+
+ // The identical ICCProfile's GetColorSpace will not match, because the
+ // original points to the now-uncached version.
+ ColorSpace identical_1_color_space = identical_1.GetColorSpace();
+ EXPECT_NE(identical_1_color_space, original_color_space_0);
+
+ // The original ICCProfile is now orphaned because there exists a new entry
+ // with the same data.
+ ColorSpace original_color_space_2 = original.GetColorSpace();
+ ICCProfile recovered_2;
+ EXPECT_FALSE(original_color_space_2.GetICCProfile(&recovered_2));
+ EXPECT_NE(original, recovered_2);
+
+ // Blow away the cache one more time.
+ for (size_t i = 0; i < 128; ++i) {
+ SkMatrix44 toXYZD50;
+ ColorSpace::CreateSRGB().GetPrimaryMatrix(&toXYZD50);
+ SkColorSpaceTransferFn fn;
+ fn.fA = 1;
+ fn.fB = 0;
+ fn.fC = 1;
+ fn.fD = 0;
+ fn.fE = 0;
+ fn.fF = 0;
+ fn.fG = 1.5f + i / 128.f;
+ ColorSpace color_space = ColorSpace::CreateCustom(toXYZD50, fn);
+ ICCProfile icc_profile;
+ color_space.GetICCProfile(&icc_profile);
+ }
+
+ // Now the original ICCProfile can re-insert itself into the cache, with its
+ // original id.
+ ColorSpace original_color_space_3 = original.GetColorSpace();
+ ICCProfile recovered_3;
+ EXPECT_TRUE(original_color_space_3.GetICCProfile(&recovered_3));
+ EXPECT_EQ(original, recovered_3);
+}
+
} // namespace gfx
diff --git a/ui/gfx/test/icc_profiles.cc b/ui/gfx/test/icc_profiles.cc
index 3b6ebb0c..cd3c888b 100644
--- a/ui/gfx/test/icc_profiles.cc
+++ b/ui/gfx/test/icc_profiles.cc
@@ -1917,22 +1917,21 @@
}
ICCProfile ICCProfileForTestingNoAnalyticTrFn() {
- return ICCProfile::FromDataWithId(
+ return ICCProfile::FromData(
reinterpret_cast<const char*>(no_analytic_tr_fn_profile_data),
- arraysize(no_analytic_tr_fn_profile_data),
- ICCProfile::test_id_no_analytic_tr_fn_);
+ arraysize(no_analytic_tr_fn_profile_data));
}
ICCProfile ICCProfileForTestingA2BOnly() {
- return ICCProfile::FromDataWithId(
+ return ICCProfile::FromData(
reinterpret_cast<const char*>(a2b_only_profile_data),
- arraysize(a2b_only_profile_data), ICCProfile::test_id_a2b_only_);
+ arraysize(a2b_only_profile_data));
}
ICCProfile ICCProfileForTestingOvershoot() {
- return ICCProfile::FromDataWithId(
+ return ICCProfile::FromData(
reinterpret_cast<const char*>(overshoot_profile_data),
- arraysize(overshoot_profile_data), ICCProfile::test_id_overshoot_);
+ arraysize(overshoot_profile_data));
}
} // namespace gfx
