color: Always use parametric color spaces for raster

ui/gfx/color_transform.cc

Index: ui/gfx/color_transform.cc
diff --git a/ui/gfx/color_transform.cc b/ui/gfx/color_transform.cc
index c87652b0c7f81669df4c4f6c05079196b881394c..5ce1abdac4b128a55c8c99d282d48aeb6e74c043 100644
--- a/ui/gfx/color_transform.cc
+++ b/ui/gfx/color_transform.cc
@@ -788,9 +788,20 @@ class QCMSColorTransform : public ColorTransformStep {
 
 ScopedQcmsProfile ColorTransformInternal::GetQCMSProfileIfNecessary(
     const ColorSpace& color_space) {
-  ICCProfile icc_profile;
-  if (!ICCProfile::FromId(color_space.icc_profile_id_, true, &icc_profile))
+  if (color_space.primaries_ != ColorSpace::PrimaryID::ICC_PROFILE_REQUIRED ||
+      color_space.transfer_ != ColorSpace::TransferID::ICC_PROFILE_REQUIRED) {
     return nullptr;
+  }
+  // TODO(ccameron): Use SkColorSpaceXform here to avoid looking up the
+  // ICCProfile.
+  ICCProfile icc_profile;
+  if (!ICCProfile::FromId(color_space.icc_profile_id_, &icc_profile)) {
+    // We needed the original ICC profile to construct this transform, but it

[hubbe, 2017/03/13 17:59:09]

This seems unacceptable to me.
In general, I don't understand what ICC_PROFILE_REQUIRED is for.

[ccameron, 2017/03/13 20:34:18]

It denotes that the color space cannot be represented parametrically (see
previous comments).

We store ICC profiles that we will use for rendering. We should not store an
arbitrarily large number of ICC profiles (that's effectively a memory leak), so,
we do need to have some caching strategy that will at some point drop them. It
is theoretically possible for a frame to start rendering using a given ICC
profile, and then for the display to change enough times that the ICC profile
that we started with has been evicted. We need to draw *something* in that case,
and this is my suggestion.

I reiterate that this is highly unlikely -- it could probably only happen if one
were drawing complex content while running a script to cycle through display
profiles (cause you can't plug and unplug monitors or manually change settings
fast enough). But, it is an edge case that needs to be handled.

Another option would be to make ICCProfile be ref-ptr-ed. That may be something
to evaluate, but is a very large change (and needs consideration about how that
will integrate with cross-process stuff).

[hubbe, 2017/03/13 20:38:56]

I think we should have some sort of approximation of the ICC profile in the
color space class so that we can have a fallback that's bad rather than broken.

[ccameron, 2017/03/14 10:19:54]

This is to ensure well-defined behavior in a vanishingly-unlikely scenario.
"Broken" would mean giving something very wrong -- falling back to sRGB is
closer to "bad".

Also, I implemented this, and the complexity wasn't worth it (we'd have to have
a CUSTOM and CUSTOM_BUT_USE_ICC_IF_POSSIBLE), which share a bunch of state).

Also also, when I replace QCMS with SkColorSpaceXform here, we'll be able to use
|icc_profile_sk_color_space_| directly, so the complexity would be deleted soon
thereafter.

[hubbe, 2017/03/14 18:26:46]

I suppose we're never going to have non-RGB ICC profiles (because in those
cases, substituting with sRGB would definitely be broken.)
So I guess you're right that this isn't really Br0ken, just bad.
If it's going away anyways soon, it's not really a problem I suppose. (Maybe
drop a TODO here so we know it's going to change?)

[ccameron, 2017/03/14 21:41:02]

Done.

+    // has been flushed from our cache. Just make something up. This will be
+    // inaccurate, so spam the console.
+    LOG(ERROR) << "Failed to retrieve original ICC profile, using sRGB";
+    return ScopedQcmsProfile(qcms_profile_sRGB());
+  }
   return ScopedQcmsProfile(qcms_profile_from_memory(
       icc_profile.GetData().data(), icc_profile.GetData().size()));
 }