color: Always use parametric color spaces for raster

ui/gfx/color_transform.cc

 // Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/color_transform.h"
 
 #include <algorithm>
 #include <cmath>
 #include <list>
 #include <memory>
@@ skipping 770 matching lines @@
                          reinterpret_cast<float*>(colors), num * 3);
   }
 
  private:
   ScopedQcmsProfile from_;
   ScopedQcmsProfile to_;
 };
 
 ScopedQcmsProfile ColorTransformInternal::GetQCMSProfileIfNecessary(
     const ColorSpace& color_space) {
+  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_, true, &icc_profile))
-    return nullptr;
+  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()));
 }
 
 ScopedQcmsProfile GetXYZD50Profile() {
   // QCMS is trixy, it has a datatype called qcms_CIE_xyY, but what it expects
   // is in fact not xyY color coordinates, it just wants the x/y values of the
   // primaries with Y equal to 1.0.
   qcms_CIE_xyYTRIPLE xyz;
   qcms_CIE_xyY w;
@@ skipping 109 matching lines @@
     const ColorSpace& to,
     Intent intent) {
   return std::unique_ptr<ColorTransform>(
       new ColorTransformInternal(from, to, intent));
 }
 
 ColorTransform::ColorTransform() {}
 ColorTransform::~ColorTransform() {}
 
 }  // namespace gfx