Make ICCProfile no longer store a ColorSpace directly

Make ICCProfile store the primaries and transfer function that it
read or computed directly, rather than storing a ColorSpace object.
Construct the ColorSpace object when requested, rather than storing it.

Store an approximate (or guessed) primary matrix and transfer function
for ICC_BASED ColorSpace objects. When computing the parametric
approximation of an ICC_BASED ColorSpace object, use these values
directly, rather than having to look them up from original ICCProfile.

Bug: 766736
Change-Id: I21d6dfaa38706f814f6c9dd54517806e99366113
Reviewed-on: https://chromium-review.googlesource.com/745311Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512958}

ui/gfx/color_space.h

@@ -53,7 +53,8 @@ class COLOR_SPACE_EXPORT ColorSpace {
     CUSTOM,
     // For color spaces defined by an ICC profile which cannot be represented
     // parametrically. Any ColorTransform using this color space will use the
-    // ICC profile directly to compute a transform LUT.
+    // ICC profile directly to compute a transform LUT. A parametric
+    // approximation of these primaries is stored in |custom_primary_matrix_|.
     ICC_BASED,
     LAST = ICC_BASED,
   };
@@ -89,7 +90,8 @@ class COLOR_SPACE_EXPORT ColorSpace {
     LINEAR_HDR,
     // A parametric transfer function defined by |custom_transfer_params_|.
     CUSTOM,
-    // See PrimaryID::ICC_BASED.
+    // See PrimaryID::ICC_BASED. A parametric approximation of the transfer
+    // function is stored in |custom_transfer_params_|.
     ICC_BASED,
     LAST = ICC_BASED,
   };
@@ -171,9 +173,6 @@ class COLOR_SPACE_EXPORT ColorSpace {
   // Returns true if the encoded values can be outside of the 0.0-1.0 range.
   bool FullRangeEncodedValues() const;
 
-  // Returns true if this color space can be represented parametrically.
-  bool IsParametric() const;
-
   // Return a parametric approximation of this color space (if it is not already
   // parametric).
   ColorSpace GetParametricApproximation() const;
@@ -208,6 +207,7 @@ class COLOR_SPACE_EXPORT ColorSpace {
   void GetRangeAdjustMatrix(SkMatrix44* matrix) const;
 
  private:
+  void SetCustomPrimaries(const SkMatrix44& to_XYZD50);
   void SetCustomTransferFunction(const SkColorSpaceTransferFn& fn);
 
   // Returns true if the transfer function is defined by an
@@ -219,12 +219,12 @@ class COLOR_SPACE_EXPORT ColorSpace {
   MatrixID matrix_ = MatrixID::INVALID;
   RangeID range_ = RangeID::INVALID;
 
-  // Only used if primaries_ is PrimaryID::CUSTOM.
+  // Only used if primaries_ is PrimaryID::CUSTOM or PrimaryID::ICC_BASED.
   float custom_primary_matrix_[9] = {0, 0, 0, 0, 0, 0, 0, 0};
 
-  // Only used if transfer_ is TransferID::CUSTOM. This array consists of the A
-  // through G entries of the SkColorSpaceTransferFn structure in alphabetical
-  // order.
+  // Only used if transfer_ is TransferID::CUSTOM or PrimaryID::ICC_BASED.
+  // This array consists of the A through G entries of the
+  // SkColorSpaceTransferFn structure in alphabetical order.
   float custom_transfer_params_[7] = {0, 0, 0, 0, 0, 0, 0};
 
   // This is used to look up the ICCProfile from which this ColorSpace was

ui/gfx/color_transform_unittest.cc

@@ -252,8 +252,9 @@ TEST(SimpleColorSpace, ICCProfileOnlyColorSpin) {
   const float kEpsilon = 2.5f / 255.f;
   ICCProfile icc_profile = ICCProfileForTestingNoAnalyticTrFn();
   ColorSpace icc_space = icc_profile.GetColorSpace();
-  ColorSpace colorspin =
-      ICCProfileForTestingColorSpin().GetParametricColorSpace();
+  ColorSpace colorspin = ICCProfileForTestingColorSpin()
+                             .GetColorSpace()
+                             .GetParametricApproximation();
 
   ColorTransform::TriStim input_value(0.25f, 0.5f, 0.75f);
   ColorTransform::TriStim transformed_value = input_value;

ui/gfx/icc_profile.cc

@@ -54,17 +54,6 @@ class ICCProfileCache {
     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);
@@ -201,76 +190,69 @@ static base::LazyInstance<ICCProfileCache>::DestructorAtExit g_cache =
 
 }  // namespace
 
-// static
-ICCProfile::AnalyzeResult ICCProfile::ExtractColorSpaces(
-    const std::vector<char>& data,
-    gfx::ColorSpace* parametric_color_space,
-    float* parametric_tr_fn_max_error,
-    sk_sp<SkColorSpace>* useable_sk_color_space) {
-  // Initialize the output parameters as invalid.
-  *parametric_color_space = gfx::ColorSpace();
-  *parametric_tr_fn_max_error = 0;
-  *useable_sk_color_space = nullptr;
+ICCProfile::AnalyzeResult ICCProfile::Initialize() {
+  // Start out with no parametric data.
+  primaries_ = gfx::ColorSpace::PrimaryID::INVALID;
+  transfer_ = gfx::ColorSpace::TransferID::INVALID;
 
   // Parse the profile and attempt to create a SkColorSpaceXform out of it.
   sk_sp<SkColorSpace> sk_srgb_color_space = SkColorSpace::MakeSRGB();
-  sk_sp<SkICC> sk_icc = SkICC::Make(data.data(), data.size());
+  sk_sp<SkICC> sk_icc = SkICC::Make(data_.data(), data_.size());
   if (!sk_icc) {
     DLOG(ERROR) << "Failed to parse ICC profile to SkICC.";
     return kICCFailedToParse;
   }
-  sk_sp<SkColorSpace> sk_icc_color_space =
-      SkColorSpace::MakeICC(data.data(), data.size());
-  if (!sk_icc_color_space) {
+  sk_color_space_ = SkColorSpace::MakeICC(data_.data(), data_.size());
+  if (!sk_color_space_) {
     DLOG(ERROR) << "Failed to parse ICC profile to SkColorSpace.";
     return kICCFailedToExtractSkColorSpace;
   }
+
+  // If our SkColorSpace representation is sRGB then return that.
+  if (SkColorSpace::Equals(sk_srgb_color_space.get(), sk_color_space_.get())) {
+    primaries_ = gfx::ColorSpace::PrimaryID::BT709;
+    transfer_ = gfx::ColorSpace::TransferID::IEC61966_2_1;
+    return kICCExtractedSRGBColorSpace;
+  }
+
   std::unique_ptr<SkColorSpaceXform> sk_color_space_xform =
-      SkColorSpaceXform::New(sk_srgb_color_space.get(),
-                             sk_icc_color_space.get());
+      SkColorSpaceXform::New(sk_srgb_color_space.get(), sk_color_space_.get());
   if (!sk_color_space_xform) {
     DLOG(ERROR) << "Parsed ICC profile but can't create SkColorSpaceXform.";
     return kICCFailedToCreateXform;
   }
 
-  // Because this SkColorSpace can be used to construct a transform, mark it
-  // as "useable". Mark the "best approximation" as sRGB to start.
-  *useable_sk_color_space = sk_icc_color_space;
-  *parametric_color_space = ColorSpace::CreateSRGB();
-
-  // If our SkColorSpace representation is sRGB then return that.
-  if (SkColorSpace::Equals(sk_srgb_color_space.get(),
-                           sk_icc_color_space.get())) {
-    return kICCExtractedSRGBColorSpace;
-  }
+  // Because this SkColorSpace can be used to construct a transform, we can use
+  // it to create a LUT based color transform, at the very least. If we fail to
+  // get any better approximation, we'll use sRGB as our approximation.
+  primaries_ = ColorSpace::PrimaryID::ICC_BASED;
+  transfer_ = ColorSpace::TransferID::ICC_BASED;
+  ColorSpace::CreateSRGB().GetPrimaryMatrix(&to_XYZD50_);
+  ColorSpace::CreateSRGB().GetTransferFunction(&transfer_fn_);
 
-  // A primary matrix is required for our parametric approximation.
+  // A primary matrix is required for our parametric representations. Use it if
+  // it exists.
   SkMatrix44 to_XYZD50_matrix;
   if (!sk_icc->toXYZD50(&to_XYZD50_matrix)) {
     DLOG(ERROR) << "Failed to extract ICC profile primary matrix.";
     return kICCFailedToExtractMatrix;
   }
+  primaries_ = ColorSpace::PrimaryID::CUSTOM;
+  to_XYZD50_ = to_XYZD50_matrix;
 
-  // Try to directly extract a numerical transfer function.
+  // Try to directly extract a numerical transfer function. Use it if it
+  // exists.
   SkColorSpaceTransferFn exact_tr_fn;
   if (sk_icc->isNumericalTransferFn(&exact_tr_fn)) {
-    *parametric_color_space =
-        gfx::ColorSpace::CreateCustom(to_XYZD50_matrix, exact_tr_fn);
+    transfer_ = ColorSpace::TransferID::CUSTOM;
+    transfer_fn_ = exact_tr_fn;
     return kICCExtractedMatrixAndAnalyticTrFn;
   }
 
-  // If we fail to get a transfer function, use the sRGB transfer function,
-  // and return false to indicate that the gfx::ColorSpace isn't accurate, but
-  // we can construct accurate LUT transforms using the underlying
-  // SkColorSpace.
-  *parametric_color_space = gfx::ColorSpace::CreateCustom(
-      to_XYZD50_matrix, ColorSpace::TransferID::IEC61966_2_1);
-
   // Attempt to fit a parametric transfer function to the table data in the
   // profile.
   SkColorSpaceTransferFn approx_tr_fn;
-  if (!SkApproximateTransferFn(sk_icc, parametric_tr_fn_max_error,
-                               &approx_tr_fn)) {
+  if (!SkApproximateTransferFn(sk_icc, &transfer_fn_error_, &approx_tr_fn)) {
     DLOG(ERROR) << "Failed approximate transfer function.";
     return kICCFailedToConvergeToApproximateTrFn;
   }
@@ -278,16 +260,16 @@ ICCProfile::AnalyzeResult ICCProfile::ExtractColorSpaces(
   // If this converged, but has too high error, use the sRGB transfer function
   // from above.
   const float kMaxError = 2.f / 256.f;
-  if (*parametric_tr_fn_max_error >= kMaxError) {
+  if (transfer_fn_error_ >= kMaxError) {
     DLOG(ERROR) << "Failed to accurately approximate transfer function, error: "
-                << 256.f * (*parametric_tr_fn_max_error) << "/256";
+                << 256.f * transfer_fn_error_ << "/256";
     return kICCFailedToApproximateTrFnAccurately;
   };
 
   // If the error is sufficiently low, declare that the approximation is
   // accurate.
-  *parametric_color_space =
-      gfx::ColorSpace::CreateCustom(to_XYZD50_matrix, approx_tr_fn);
+  transfer_ = ColorSpace::TransferID::CUSTOM;
+  transfer_fn_ = approx_tr_fn;
   return kICCExtractedMatrixAndApproximatedTrFn;
 }
 
@@ -346,14 +328,28 @@ const std::vector<char>& ICCProfile::GetData() const {
   return data_;
 }
 
-const ColorSpace& ICCProfile::GetColorSpace() const {
+ColorSpace ICCProfile::GetColorSpace() const {
   g_cache.Get().TouchEntry(*this);
-  return color_space_;
-}
 
-const ColorSpace& ICCProfile::GetParametricColorSpace() const {
-  g_cache.Get().TouchEntry(*this);
-  return parametric_color_space_;
+  ColorSpace color_space;
+  if (!IsValid())
+    return color_space;
+  color_space.icc_profile_id_ = id_;
+  color_space.icc_profile_sk_color_space_ = sk_color_space_;
+  DCHECK(color_space.icc_profile_sk_color_space_);
+  color_space.matrix_ = ColorSpace::MatrixID::RGB;
+  color_space.range_ = ColorSpace::RangeID::FULL;
+  if (primaries_ == ColorSpace::PrimaryID::CUSTOM ||
+      primaries_ == ColorSpace::PrimaryID::ICC_BASED) {
+    color_space.SetCustomPrimaries(to_XYZD50_);
+  }
+  color_space.primaries_ = primaries_;
+  if (transfer_ == ColorSpace::TransferID::CUSTOM ||
+      transfer_ == ColorSpace::TransferID::ICC_BASED) {
+    color_space.SetCustomTransferFunction(transfer_fn_);
+  }
+  color_space.transfer_ = transfer_;
+  return color_space;
 }
 
 // static
@@ -376,34 +372,7 @@ void ICCProfile::ComputeColorSpaceAndCache() {
     return;
 
   // Parse the ICC profile
-  sk_sp<SkColorSpace> useable_sk_color_space;
-  analyze_result_ =
-      ExtractColorSpaces(data_, &parametric_color_space_,
-                         &parametric_tr_fn_error_, &useable_sk_color_space);
-  switch (analyze_result_) {
-    case kICCExtractedSRGBColorSpace:
-    case kICCExtractedMatrixAndAnalyticTrFn:
-    case kICCExtractedMatrixAndApproximatedTrFn:
-      // Successfully and accurately extracted color space.
-      color_space_ = parametric_color_space_;
-      break;
-    case kICCFailedToExtractRawTrFn:
-    case kICCFailedToExtractMatrix:
-    case kICCFailedToConvergeToApproximateTrFn:
-    case kICCFailedToApproximateTrFnAccurately:
-      // 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_sk_color_space_ = useable_sk_color_space;
-      break;
-    case kICCFailedToParse:
-    case kICCFailedToExtractSkColorSpace:
-    case kICCFailedToCreateXform:
-      // Can't even use this color space as a LUT.
-      DCHECK(!parametric_color_space_.IsValid());
-      color_space_ = parametric_color_space_;
-      break;
-  }
+  analyze_result_ = Initialize();
 
   // Add to the cache.
   g_cache.Get().InsertAndSetIdIfNeeded(this);
@@ -429,7 +398,7 @@ void ICCProfile::HistogramDisplay(int64_t display_id) const {
   if (nonlinear_fit_converged) {
     UMA_HISTOGRAM_CUSTOM_COUNTS(
         "Blink.ColorSpace.Destination.NonlinearFitError",
-        static_cast<int>(parametric_tr_fn_error_ * 255), 0, 127, 16);
+        static_cast<int>(transfer_fn_error_ * 255), 0, 127, 16);
   }
 }
 

ui/gfx/icc_profile.h

@@ -52,12 +52,7 @@ class COLOR_SPACE_EXPORT ICCProfile {
 
   // Return a ColorSpace that references this ICCProfile. ColorTransforms
   // created using this ColorSpace will match this ICCProfile precisely.
-  const ColorSpace& GetColorSpace() const;
-
-  // Return a ColorSpace that is the best parametric approximation of this
-  // ICCProfile. The resulting ColorSpace will reference this ICCProfile only
-  // if the parametric approximation is almost exact.
-  const ColorSpace& GetParametricColorSpace() const;
+  ColorSpace GetColorSpace() const;
 
   const std::vector<char>& GetData() const;
 
@@ -103,12 +98,7 @@ class COLOR_SPACE_EXPORT ICCProfile {
                                    size_t size,
                                    uint64_t id);
 
-  static AnalyzeResult ExtractColorSpaces(
-      const std::vector<char>& data,
-      gfx::ColorSpace* parametric_color_space,
-      float* parametric_tr_fn_max_error,
-      sk_sp<SkColorSpace>* useable_sk_color_space);
-
+  AnalyzeResult Initialize();
   void ComputeColorSpaceAndCache();
 
   // This globally identifies this ICC profile. It is used to look up this ICC
@@ -120,18 +110,21 @@ class COLOR_SPACE_EXPORT ICCProfile {
   // The result of attepting to extract a color space from the color profile.
   AnalyzeResult analyze_result_ = kICCFailedToParse;
 
-  // |color_space| always links back to this ICC profile, and its SkColorSpace
-  // is always equal to the SkColorSpace created from this ICCProfile.
-  gfx::ColorSpace color_space_;
+  // Results of Skia parsing the ICC profile data.
+  sk_sp<SkColorSpace> sk_color_space_;
+
+  // The best-fit parametric primaries.
+  gfx::ColorSpace::PrimaryID primaries_;
+  SkMatrix44 to_XYZD50_;
 
-  // |parametric_color_space_| will only link back to this ICC profile if it
-  // is accurate, and its SkColorSpace will always be parametrically created.
-  gfx::ColorSpace parametric_color_space_;
+  // The best-fit parametric transfer function.
+  gfx::ColorSpace::TransferID transfer_;
+  SkColorSpaceTransferFn transfer_fn_;
 
   // The L-infinity error of the parametric color space fit. This is undefined
   // unless |analyze_result_| is kICCFailedToApproximateTrFnAccurately or
   // kICCExtractedMatrixAndApproximatedTrFn.
-  float parametric_tr_fn_error_ = -1;
+  float transfer_fn_error_ = 0;
 
   FRIEND_TEST_ALL_PREFIXES(SimpleColorSpace, BT709toSRGBICC);
   FRIEND_TEST_ALL_PREFIXES(SimpleColorSpace, GetColorSpace);

ui/gfx/icc_profile_unittest.cc

@@ -32,8 +32,12 @@ TEST(ICCProfile, SRGB) {
   EXPECT_EQ(icc_profile.GetColorSpace().ToSkColorSpace().get(),
             sk_color_space.get());
   // The parametric generating code should recognize that this is SRGB.
-  EXPECT_EQ(icc_profile.GetParametricColorSpace(), ColorSpace::CreateSRGB());
-  EXPECT_EQ(icc_profile.GetParametricColorSpace().ToSkColorSpace().get(),
+  EXPECT_EQ(icc_profile.GetColorSpace().GetParametricApproximation(),
+            ColorSpace::CreateSRGB());
+  EXPECT_EQ(icc_profile.GetColorSpace()
+                .GetParametricApproximation()
+                .ToSkColorSpace()
+                .get(),
             sk_color_space.get());
   // The generated color space should recognize that this is SRGB.
   EXPECT_EQ(color_space.ToSkColorSpace().get(), sk_color_space.get());
@@ -78,7 +82,8 @@ TEST(ICCProfile, ParametricVersusExact) {
   // This ICC profile has three transfer functions that differ enough that the
   // parametric color space is considered inaccurate.
   ICCProfile multi_tr_fn = ICCProfileForTestingNoAnalyticTrFn();
-  EXPECT_NE(multi_tr_fn.GetColorSpace(), multi_tr_fn.GetParametricColorSpace());
+  EXPECT_NE(multi_tr_fn.GetColorSpace(),
+            multi_tr_fn.GetColorSpace().GetParametricApproximation());
 
   ICCProfile multi_tr_fn_color_space;
   EXPECT_TRUE(
@@ -86,35 +91,40 @@ TEST(ICCProfile, ParametricVersusExact) {
   EXPECT_EQ(multi_tr_fn_color_space, multi_tr_fn);
 
   ICCProfile multi_tr_fn_parametric_color_space;
-  EXPECT_TRUE(multi_tr_fn.GetParametricColorSpace().GetICCProfile(
-      &multi_tr_fn_parametric_color_space));
+  EXPECT_TRUE(
+      multi_tr_fn.GetColorSpace().GetParametricApproximation().GetICCProfile(
+          &multi_tr_fn_parametric_color_space));
   EXPECT_NE(multi_tr_fn_parametric_color_space, multi_tr_fn);
 
   // This ICC profile has a transfer function with T(1) that is greater than 1
   // in the approximation, but is still close enough to be considered accurate.
   ICCProfile overshoot = ICCProfileForTestingOvershoot();
-  EXPECT_EQ(overshoot.GetColorSpace(), overshoot.GetParametricColorSpace());
+  EXPECT_EQ(overshoot.GetColorSpace(),
+            overshoot.GetColorSpace().GetParametricApproximation());
 
   ICCProfile overshoot_color_space;
   EXPECT_TRUE(overshoot.GetColorSpace().GetICCProfile(&overshoot_color_space));
   EXPECT_EQ(overshoot_color_space, overshoot);
 
   ICCProfile overshoot_parametric_color_space;
-  EXPECT_TRUE(overshoot.GetParametricColorSpace().GetICCProfile(
-      &overshoot_parametric_color_space));
+  EXPECT_TRUE(
+      overshoot.GetColorSpace().GetParametricApproximation().GetICCProfile(
+          &overshoot_parametric_color_space));
   EXPECT_EQ(overshoot_parametric_color_space, overshoot);
 
   // This ICC profile is precisely represented by the parametric color space.
   ICCProfile accurate = ICCProfileForTestingAdobeRGB();
-  EXPECT_EQ(accurate.GetColorSpace(), accurate.GetParametricColorSpace());
+  EXPECT_EQ(accurate.GetColorSpace(),
+            accurate.GetColorSpace().GetParametricApproximation());
 
   ICCProfile accurate_color_space;
   EXPECT_TRUE(accurate.GetColorSpace().GetICCProfile(&accurate_color_space));
   EXPECT_EQ(accurate_color_space, accurate);
 
   ICCProfile accurate_parametric_color_space;
-  EXPECT_TRUE(accurate.GetParametricColorSpace().GetICCProfile(
-      &accurate_parametric_color_space));
+  EXPECT_TRUE(
+      accurate.GetColorSpace().GetParametricApproximation().GetICCProfile(
+          &accurate_parametric_color_space));
   EXPECT_EQ(accurate_parametric_color_space, accurate);
 
   // This ICC profile has only an A2B representation. We cannot create an
@@ -136,12 +146,15 @@ TEST(ICCProfile, GarbageData) {
       ICCProfile::FromData(bad_data.data(), bad_data.size());
   EXPECT_FALSE(garbage_profile.IsValid());
   EXPECT_FALSE(garbage_profile.GetColorSpace().IsValid());
-  EXPECT_FALSE(garbage_profile.GetParametricColorSpace().IsValid());
+  EXPECT_FALSE(
+      garbage_profile.GetColorSpace().GetParametricApproximation().IsValid());
 
   ICCProfile default_ctor_profile;
   EXPECT_FALSE(default_ctor_profile.IsValid());
   EXPECT_FALSE(default_ctor_profile.GetColorSpace().IsValid());
-  EXPECT_FALSE(default_ctor_profile.GetParametricColorSpace().IsValid());
+  EXPECT_FALSE(default_ctor_profile.GetColorSpace()
+                   .GetParametricApproximation()
+                   .IsValid());
 }
 
 TEST(ICCProfile, GenericRGB) {