Refactored SkColorSpace and added in a Lab PCS GM

src/core/SkColorSpace.cpp

Index: src/core/SkColorSpace.cpp
diff --git a/src/core/SkColorSpace.cpp b/src/core/SkColorSpace.cpp
index c6bf4b9431300460b4fbe4dd2b6f276de9dda4a2..0cf8d0d88dfb22a54067bf105871b52286e8467f 100644
--- a/src/core/SkColorSpace.cpp
+++ b/src/core/SkColorSpace.cpp
@@ -7,26 +7,13 @@
 
 #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[] {
@@ -90,8 +77,8 @@ sk_sp<SkColorSpace> SkColorSpace_Base::NewRGB(const float values[3], const SkMat
         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);
@@ -121,7 +108,7 @@ sk_sp<SkColorSpace> SkColorSpace_Base::NewRGB(SkGammaNamed gammaNamed, const SkM
             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) {
@@ -152,7 +139,7 @@ sk_sp<SkColorSpace> SkColorSpace::NewNamed(Named named) {
 
                 // 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);
         }
@@ -163,7 +150,7 @@ sk_sp<SkColorSpace> SkColorSpace::NewNamed(Named named) {
 
                 // 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);
         }
@@ -174,7 +161,7 @@ sk_sp<SkColorSpace> SkColorSpace::NewNamed(Named named) {
 
                 // 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);
         }
@@ -188,32 +175,50 @@ sk_sp<SkColorSpace> SkColorSpace::makeLinearGamma() {
     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;
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
@@ -273,41 +278,42 @@ 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);
             }
@@ -315,11 +321,11 @@ size_t SkColorSpace::writeToMemory(void* memory) const {
                 // 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 &&
@@ -329,7 +335,7 @@ size_t SkColorSpace::writeToMemory(void* memory) const {
                     *(((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);
         }
@@ -438,6 +444,10 @@ bool SkColorSpace::Equals(const SkColorSpace* src, const SkColorSpace* dst) {
     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();
@@ -451,22 +461,53 @@ bool SkColorSpace::Equals(const SkColorSpace* src, const SkColorSpace* dst) {
     }
 
     // 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;
             }
-
-            // 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());
+        }
+        // a/b/m-curve tables (if applicable) and CLUT still needs checking
     }
+
+    // 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());
 }