Refactored SkColorSpace and added in a Lab PCS GM

src/core/SkColorSpace.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkColorSpace.h"
 #include "SkColorSpace_Base.h"
+#include "SkColorSpace_A2B0.h"
+#include "SkColorSpace_XYZTRC.h"
 #include "SkColorSpacePriv.h"
 #include "SkOnce.h"
 
-SkColorSpace_Base::SkColorSpace_Base(SkGammaNamed gammaNamed, const SkMatrix44& toXYZD50)
-    : fGammaNamed(gammaNamed)
-    , fGammas(nullptr)
-    , fProfileData(nullptr)
-    , fToXYZD50(toXYZD50)
-    , fFromXYZD50(SkMatrix44::kUninitialized_Constructor)
-{}
-
-SkColorSpace_Base::SkColorSpace_Base(sk_sp<SkColorLookUpTable> colorLUT, SkGammaNamed gammaNamed,
-                                     sk_sp<SkGammas> gammas, const SkMatrix44& toXYZD50,
-                                     sk_sp<SkData> profileData)
-    : fColorLUT(std::move(colorLUT))
-    , fGammaNamed(gammaNamed)
-    , fGammas(std::move(gammas))
-    , fProfileData(std::move(profileData))
-    , fToXYZD50(toXYZD50)
-    , fFromXYZD50(SkMatrix44::kUninitialized_Constructor)
+SkColorSpace_Base::SkColorSpace_Base(sk_sp<SkData> profileData)
+    : fProfileData(std::move(profileData))
 {}
 
 static constexpr float gSRGB_toXYZD50[] {
     0.4358f, 0.3853f, 0.1430f,    // Rx, Gx, Bx
     0.2224f, 0.7170f, 0.0606f,    // Ry, Gy, Gz
     0.0139f, 0.0971f, 0.7139f,    // Rz, Gz, Bz
 };
 
 static constexpr float gAdobeRGB_toXYZD50[] {
     0.6098f, 0.2052f, 0.1492f,    // Rx, Gx, Bx
@@ skipping 43 matching lines @@
     }
 
     if (kNonStandard_SkGammaNamed == gammaNamed) {
         sk_sp<SkGammas> gammas = sk_sp<SkGammas>(new SkGammas());
         gammas->fRedType = SkGammas::Type::kValue_Type;
         gammas->fGreenType = SkGammas::Type::kValue_Type;
         gammas->fBlueType = SkGammas::Type::kValue_Type;
         gammas->fRedData.fValue = values[0];
         gammas->fGreenData.fValue = values[1];
         gammas->fBlueData.fValue = values[2];
-        return sk_sp<SkColorSpace>(new SkColorSpace_Base(nullptr, kNonStandard_SkGammaNamed, gammas,
-                                                         toXYZD50, nullptr));
+        return sk_sp<SkColorSpace>(new SkColorSpace_XYZTRC(kNonStandard_SkGammaNamed,

[msarett, 2016/10/07 01:48:35]

This line makes me feel good about this change: just seems more logical now.

+                                                           gammas, toXYZD50, nullptr));
     }
 
     return SkColorSpace_Base::NewRGB(gammaNamed, toXYZD50);
 }
 
 sk_sp<SkColorSpace> SkColorSpace_Base::NewRGB(SkGammaNamed gammaNamed, const SkMatrix44& toXYZD50) {
     switch (gammaNamed) {
         case kSRGB_SkGammaNamed:
             if (xyz_almost_equal(toXYZD50, gSRGB_toXYZD50)) {
                 return SkColorSpace::NewNamed(kSRGB_Named);
             }
             break;
         case k2Dot2Curve_SkGammaNamed:
             if (xyz_almost_equal(toXYZD50, gAdobeRGB_toXYZD50)) {
                 return SkColorSpace::NewNamed(kAdobeRGB_Named);
             }
             break;
         case kLinear_SkGammaNamed:
             if (xyz_almost_equal(toXYZD50, gSRGB_toXYZD50)) {
                 return SkColorSpace::NewNamed(kSRGBLinear_Named);
             }
             break;
         case kNonStandard_SkGammaNamed:
             // This is not allowed.
             return nullptr;
         default:
             break;
     }
 
-    return sk_sp<SkColorSpace>(new SkColorSpace_Base(gammaNamed, toXYZD50));
+    return sk_sp<SkColorSpace>(new SkColorSpace_XYZTRC(gammaNamed, toXYZD50));
 }
 
 sk_sp<SkColorSpace> SkColorSpace::NewRGB(RenderTargetGamma gamma, const SkMatrix44& toXYZD50) {
     switch (gamma) {
         case kLinear_RenderTargetGamma:
             return SkColorSpace_Base::NewRGB(kLinear_SkGammaNamed, toXYZD50);
         case kSRGB_RenderTargetGamma:
             return SkColorSpace_Base::NewRGB(kSRGB_SkGammaNamed, toXYZD50);
         default:
             return nullptr;
@@ skipping 10 matching lines @@
     static SkOnce sRGBLinearOnce;
 
     switch (named) {
         case kSRGB_Named: {
             sRGBOnce([] {
                 SkMatrix44 srgbToxyzD50(SkMatrix44::kUninitialized_Constructor);
                 srgbToxyzD50.set3x3RowMajorf(gSRGB_toXYZD50);
 
                 // Force the mutable type mask to be computed.  This avoids races.
                 (void)srgbToxyzD50.getType();
-                gSRGB = new SkColorSpace_Base(kSRGB_SkGammaNamed, srgbToxyzD50);
+                gSRGB = new SkColorSpace_XYZTRC(kSRGB_SkGammaNamed, srgbToxyzD50);
             });
             return sk_ref_sp<SkColorSpace>(gSRGB);
         }
         case kAdobeRGB_Named: {
             adobeRGBOnce([] {
                 SkMatrix44 adobergbToxyzD50(SkMatrix44::kUninitialized_Constructor);
                 adobergbToxyzD50.set3x3RowMajorf(gAdobeRGB_toXYZD50);
 
                 // Force the mutable type mask to be computed.  This avoids races.
                 (void)adobergbToxyzD50.getType();
-                gAdobeRGB = new SkColorSpace_Base(k2Dot2Curve_SkGammaNamed, adobergbToxyzD50);
+                gAdobeRGB = new SkColorSpace_XYZTRC(k2Dot2Curve_SkGammaNamed, adobergbToxyzD50);
             });
             return sk_ref_sp<SkColorSpace>(gAdobeRGB);
         }
         case kSRGBLinear_Named: {
             sRGBLinearOnce([] {
                 SkMatrix44 srgbToxyzD50(SkMatrix44::kUninitialized_Constructor);
                 srgbToxyzD50.set3x3RowMajorf(gSRGB_toXYZD50);
 
                 // Force the mutable type mask to be computed.  This avoids races.
                 (void)srgbToxyzD50.getType();
-                gSRGBLinear = new SkColorSpace_Base(kLinear_SkGammaNamed, srgbToxyzD50);
+                gSRGBLinear = new SkColorSpace_XYZTRC(kLinear_SkGammaNamed, srgbToxyzD50);
             });
             return sk_ref_sp<SkColorSpace>(gSRGBLinear);
         }
         default:
             break;
     }
     return nullptr;
 }
 
 sk_sp<SkColorSpace> SkColorSpace::makeLinearGamma() {

[msarett, 2016/10/07 01:48:35]

Let's make this virutal on SkColorSpace_Base.

[raftias, 2016/10/10 20:37:32]

Done.

     if (this->gammaIsLinear()) {
         return sk_ref_sp(this);
     }
-    return SkColorSpace_Base::NewRGB(kLinear_SkGammaNamed, as_CSB(this)->fToXYZD50);
+        // A2B0 Color Spaces do not have a single Gamma, so this method should not be

[msarett, 2016/10/07 01:48:35]

nit: Convert tabs to spaces

[raftias, 2016/10/10 20:37:32]

Done. I had forgot to change my difftool to spaces.

+        // called on them.
+        SkASSERT(as_CSB(this)->type() == SkColorSpace_Base::Type::kXYZTRC);

[msarett, 2016/10/07 01:48:35]

This is really tricky...  I don't know what we should do here uggh.

Any thoughts reed@, brianosman@?

I'm leaning towards returning nullptr...
Or we could just throw them linear sRGB? (that's likely what they'll want when
they realize this doesn't work)

We could also try to retract this from the public API, but I think it's useful
and I know that Chrome is using it.

[raftias, 2016/10/10 20:37:32]

If linear sRGB is what they want, they can get that other ways though, can't
they? If I was a user I wouldn't like it if it gave me linear sRGB when it's not
what I wanted, without any warning or anything.

+        const SkColorSpace_XYZTRC* thisXYZ = static_cast<const SkColorSpace_XYZTRC*>(this);
+    return SkColorSpace_Base::NewRGB(kLinear_SkGammaNamed, thisXYZ->toXYZD50());
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
 bool SkColorSpace::gammaCloseToSRGB() const {
-    return kSRGB_SkGammaNamed == as_CSB(this)->fGammaNamed ||
-           k2Dot2Curve_SkGammaNamed == as_CSB(this)->fGammaNamed;
+    switch (as_CSB(this)->type())

[msarett, 2016/10/07 01:48:35]

Can we make all of these virtual on SkColorSpace_Base?

[raftias, 2016/10/10 20:37:32]

Done.

+    {
+        case SkColorSpace_Base::Type::kXYZTRC: {
+            const SkColorSpace_XYZTRC* thisXYZ = static_cast<const SkColorSpace_XYZTRC*>(this);
+            return kSRGB_SkGammaNamed == thisXYZ->fGammaNamed ||
+                   k2Dot2Curve_SkGammaNamed == thisXYZ->fGammaNamed;
+        }
+        case SkColorSpace_Base::Type::kA2B0:

[msarett, 2016/10/07 01:48:35]

Returning false in this case is just fine.  No need to assert or anything.

[raftias, 2016/10/10 20:37:32]

Acknowledged.

+            // Uncomment this assert later on
+            //SkASSERT(false);
+            return false;
+        default:
+            break;
+    }
+    SkASSERT(false);
+    return false;
 }
 
 bool SkColorSpace::gammaIsLinear() const {
-    return kLinear_SkGammaNamed == as_CSB(this)->fGammaNamed;
-}
-
-const SkMatrix44& SkColorSpace_Base::fromXYZD50() const {
-    fFromXYZOnce([this] {
-        if (!fToXYZD50.invert(&fFromXYZD50)) {
-            // If a client gives us a dst gamut with a transform that we can't invert, we will
-            // simply give them back a transform to sRGB gamut.
-            SkDEBUGFAIL("Non-invertible XYZ matrix, defaulting to sRGB");
-            SkMatrix44 srgbToxyzD50(SkMatrix44::kUninitialized_Constructor);
-            srgbToxyzD50.set3x3RowMajorf(gSRGB_toXYZD50);
-            srgbToxyzD50.invert(&fFromXYZD50);
+    switch (as_CSB(this)->type())
+    {
+        case SkColorSpace_Base::Type::kXYZTRC: {
+            const SkColorSpace_XYZTRC* thisXYZ = static_cast<const SkColorSpace_XYZTRC*>(this);
+            return kLinear_SkGammaNamed == thisXYZ->fGammaNamed;
         }
-    });
-    return fFromXYZD50;
+        case SkColorSpace_Base::Type::kA2B0:
+            // Uncomment this assert later on
+            //SkASSERT(false);
+            return false;
+        default:
+            break;
+    }
+    SkASSERT(false);
+    return false;
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
 enum Version {
     k0_Version, // Initial version, header + flags for matrix and profile
 };
 
 struct ColorSpaceHeader {
     /**
@@ skipping 39 matching lines @@
     uint8_t fVersion;    // Always zero
     uint8_t fNamed;      // Must be a SkColorSpace::Named
     uint8_t fGammaNamed; // Must be a SkGammaNamed
     uint8_t fFlags;      // Some combination of the flags listed above
 };
 
 size_t SkColorSpace::writeToMemory(void* memory) const {
     // Start by trying the serialization fast path.  If we haven't saved ICC profile data,
     // we must have a profile that we can serialize easily.
     if (!as_CSB(this)->fProfileData) {
+        // Profile data is mandatory for A2B0 color spaces.
         // If we have a named profile, only write the enum.
+        SkASSERT(as_CSB(this)->type() == SkColorSpace_Base::Type::kXYZTRC);
+        const SkColorSpace_XYZTRC* thisXYZ = static_cast<const SkColorSpace_XYZTRC*>(this);
+        const SkGammaNamed gammaNamed = thisXYZ->gammaNamed();
         if (this == gSRGB) {
             if (memory) {
                 *((ColorSpaceHeader*) memory) =
-                        ColorSpaceHeader::Pack(k0_Version, kSRGB_Named,
-                                               as_CSB(this)->fGammaNamed, 0);
+                        ColorSpaceHeader::Pack(k0_Version, kSRGB_Named, gammaNamed, 0);
             }
             return sizeof(ColorSpaceHeader);
         } else if (this == gAdobeRGB) {
             if (memory) {
                 *((ColorSpaceHeader*) memory) =
-                        ColorSpaceHeader::Pack(k0_Version, kAdobeRGB_Named,
-                                               as_CSB(this)->fGammaNamed, 0);
+                        ColorSpaceHeader::Pack(k0_Version, kAdobeRGB_Named, gammaNamed, 0);
             }
             return sizeof(ColorSpaceHeader);
         } else if (this == gSRGBLinear) {
             if (memory) {
                 *((ColorSpaceHeader*)memory) =
-                        ColorSpaceHeader::Pack(k0_Version, kSRGBLinear_Named,
-                                               as_CSB(this)->fGammaNamed, 0);
+                        ColorSpaceHeader::Pack(k0_Version, kSRGBLinear_Named, gammaNamed, 0);
             }
             return sizeof(ColorSpaceHeader);
         }
 
         // If we have a named gamma, write the enum and the matrix.
-        switch (as_CSB(this)->fGammaNamed) {
+        switch (gammaNamed) {
             case kSRGB_SkGammaNamed:
             case k2Dot2Curve_SkGammaNamed:
             case kLinear_SkGammaNamed: {
                 if (memory) {
                     *((ColorSpaceHeader*) memory) =
-                            ColorSpaceHeader::Pack(k0_Version, 0, as_CSB(this)->fGammaNamed,
+                            ColorSpaceHeader::Pack(k0_Version, 0, gammaNamed,
                                                    ColorSpaceHeader::kMatrix_Flag);
                     memory = SkTAddOffset<void>(memory, sizeof(ColorSpaceHeader));
-                    as_CSB(this)->fToXYZD50.as3x4RowMajorf((float*) memory);
+                    thisXYZ->toXYZD50().as3x4RowMajorf((float*) memory);
                 }
                 return sizeof(ColorSpaceHeader) + 12 * sizeof(float);
             }
             default:
                 // Otherwise, write the gamma values and the matrix.
                 if (memory) {
                     *((ColorSpaceHeader*) memory) =
-                            ColorSpaceHeader::Pack(k0_Version, 0, as_CSB(this)->fGammaNamed,
+                            ColorSpaceHeader::Pack(k0_Version, 0, gammaNamed,
                                                    ColorSpaceHeader::kFloatGamma_Flag);
                     memory = SkTAddOffset<void>(memory, sizeof(ColorSpaceHeader));
 
-                    const SkGammas* gammas = as_CSB(this)->gammas();
+                    const SkGammas* gammas = thisXYZ->gammas();
                     SkASSERT(gammas);
                     SkASSERT(SkGammas::Type::kValue_Type == gammas->fRedType &&
                              SkGammas::Type::kValue_Type == gammas->fGreenType &&
                              SkGammas::Type::kValue_Type == gammas->fBlueType);
                     *(((float*) memory) + 0) = gammas->fRedData.fValue;
                     *(((float*) memory) + 1) = gammas->fGreenData.fValue;
                     *(((float*) memory) + 2) = gammas->fBlueData.fValue;
                     memory = SkTAddOffset<void>(memory, 3 * sizeof(float));
 
-                    as_CSB(this)->fToXYZD50.as3x4RowMajorf((float*) memory);
+                    thisXYZ->toXYZD50().as3x4RowMajorf((float*) memory);
                 }
                 return sizeof(ColorSpaceHeader) + 15 * sizeof(float);
         }
     }
 
     // Otherwise, serialize the ICC data.
     size_t profileSize = as_CSB(this)->fProfileData->size();
     if (SkAlign4(profileSize) != (uint32_t) SkAlign4(profileSize)) {
         return 0;
     }
@@ skipping 88 matching lines @@
 }
 
 bool SkColorSpace::Equals(const SkColorSpace* src, const SkColorSpace* dst) {
     if (src == dst) {
         return true;
     }
 
     if (!src || !dst) {
         return false;
     }
+    
+    if (as_CSB(src)->type() != as_CSB(dst)->type()) {
+        return false;
+    }
 
     SkData* srcData = as_CSB(src)->fProfileData.get();
     SkData* dstData = as_CSB(dst)->fProfileData.get();
     if (srcData || dstData) {
         if (srcData && dstData) {
             return srcData->size() == dstData->size() &&
                    0 == memcmp(srcData->data(), dstData->data(), srcData->size());
         }
 
         return false;
     }
 
     // It's important to check fProfileData before named gammas.  Some profiles may have named
-    // gammas, but also include other wacky features that cause us to save the data.
-    switch (as_CSB(src)->fGammaNamed) {
-        case kSRGB_SkGammaNamed:
-        case k2Dot2Curve_SkGammaNamed:
-        case kLinear_SkGammaNamed:
-            return (as_CSB(src)->fGammaNamed == as_CSB(dst)->fGammaNamed) &&
-                   (as_CSB(src)->fToXYZD50 == as_CSB(dst)->fToXYZD50);
-        default:
-            if (as_CSB(src)->fGammaNamed != as_CSB(dst)->fGammaNamed) {
+    // gammas, but also include other wacky features that cause us to save the data.    
+    if (as_CSB(src)->type() == SkColorSpace_Base::Type::kXYZTRC) {
+        const SkColorSpace_XYZTRC& srcXYZ = *static_cast<const SkColorSpace_XYZTRC*>(src);
+        const SkColorSpace_XYZTRC& dstXYZ = *static_cast<const SkColorSpace_XYZTRC*>(dst);
+
+        switch (srcXYZ.gammaNamed()) {
+            case kSRGB_SkGammaNamed:
+            case k2Dot2Curve_SkGammaNamed:
+            case kLinear_SkGammaNamed:
+                return (srcXYZ.gammaNamed() == dstXYZ.gammaNamed()) &&
+                       (srcXYZ.toXYZD50() == dstXYZ.toXYZD50());
+            default:
+                if (srcXYZ.gammaNamed() != dstXYZ.gammaNamed()) {
+                    return false;
+                }
+        }
+        // TRC tables still need checking
+    } else {
+        const SkColorSpace_A2B0& srcA2B = *static_cast<const SkColorSpace_A2B0*>(src);
+        const SkColorSpace_A2B0& dstA2B = *static_cast<const SkColorSpace_A2B0*>(dst);
+
+        const SkGammaNamed aNamed = srcA2B.aCurveNamed();
+        const SkGammaNamed bNamed = srcA2B.bCurveNamed();
+        const SkGammaNamed mNamed = srcA2B.mCurveNamed();
+        if ((aNamed != dstA2B.aCurveNamed()) ||
+            (bNamed != dstA2B.bCurveNamed()) ||
+            (mNamed != dstA2B.mCurveNamed())) {
+            return false;
+        }
+        if (aNamed != kNonStandard_SkGammaNamed &&
+            bNamed != kNonStandard_SkGammaNamed &&
+            mNamed != kNonStandard_SkGammaNamed) {
+                        // All gammas are the same type and are not tables, so see if
+                        // the matrices differ
+            if ((srcA2B.toPCS() != dstA2B.toPCS()) ||
+                (srcA2B.toPCS() != dstA2B.toPCS()) ||
+                (srcA2B.toPCS() != dstA2B.toPCS())) {
                 return false;
             }
+        }
+        // a/b/m-curve tables (if applicable) and CLUT still needs checking
+    }
 
-            // It is unlikely that we will reach this case.
-            sk_sp<SkData> srcData = src->serialize();
-            sk_sp<SkData> dstData = dst->serialize();
-            return srcData->size() == dstData->size() &&
-                   0 == memcmp(srcData->data(), dstData->data(), srcData->size());
-    }
+    // It is unlikely that we will reach this case.
+    sk_sp<SkData> serializedSrcData = src->serialize();
+    sk_sp<SkData> serializedDstData = dst->serialize();
+    return serializedSrcData->size() == serializedDstData->size() &&
+           0 == memcmp(serializedSrcData->data(), serializedDstData->data(),
+                       serializedSrcData->size());
 }