From 008fbd1b8e8339797d0f43164acde6357edb9dc6 Mon Sep 17 00:00:00 2001 From: mgiuca Date: Mon, 5 Sep 2016 04:58:55 -0700 Subject: [PATCH] Revert of Delete SkColorSpace::kUnknown_Named, remove fNamed field (patchset #1 id:20001 of https://codereview.chromium.org/2302413002/ ) Reason for revert: This CL introduced two static initializers (gAdobeRGB and gSRGB) which are causing a sizes regression on Chromium builders: https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/24981 Original issue's description: > Delete SkColorSpace::kUnknown_Named, remove fNamed field > > BUG=skia: > GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2302413002 > > Committed: https://skia.googlesource.com/skia/+/54682e856cb66c653bc7e253981a421a2618398e TBR=reed@google.com,brianosman@google.com,msarett@google.com # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=skia:5724 Review-Url: https://codereview.chromium.org/2306313002 --- include/core/SkColorSpace.h | 17 +++------ src/core/SkColorSpace.cpp | 74 ++++++++++++++++++++++-------------- src/core/SkColorSpace_Base.h | 2 +- tests/ColorSpaceTest.cpp | 8 ++-- 4 files changed, 56 insertions(+), 45 deletions(-) diff --git a/include/core/SkColorSpace.h b/include/core/SkColorSpace.h index 257893f495..58095775e4 100644 --- a/include/core/SkColorSpace.h +++ b/include/core/SkColorSpace.h @@ -19,21 +19,13 @@ public: /** * Common, named profiles that we can recognize. */ - enum Named : uint8_t { - /** - * By far the most common color space. - * This is the default space for images, unmarked content, and monitors. - */ + enum Named { + kUnknown_Named, kSRGB_Named, - - /** - * Very common wide gamut color space. - * Often used by images and monitors. - */ kAdobeRGB_Named, }; - enum GammaNamed : uint8_t { + enum GammaNamed { kLinear_GammaNamed, /** @@ -124,10 +116,11 @@ public: static bool Equals(const SkColorSpace* src, const SkColorSpace* dst); protected: - SkColorSpace(GammaNamed gammaNamed, const SkMatrix44& toXYZD50); + SkColorSpace(GammaNamed gammaNamed, const SkMatrix44& toXYZD50, Named named); const GammaNamed fGammaNamed; const SkMatrix44 fToXYZD50; + const Named fNamed; }; #endif diff --git a/src/core/SkColorSpace.cpp b/src/core/SkColorSpace.cpp index 682c980def..abdd647a25 100644 --- a/src/core/SkColorSpace.cpp +++ b/src/core/SkColorSpace.cpp @@ -10,13 +10,14 @@ #include "SkColorSpacePriv.h" #include "SkOnce.h" -SkColorSpace::SkColorSpace(GammaNamed gammaNamed, const SkMatrix44& toXYZD50) +SkColorSpace::SkColorSpace(GammaNamed gammaNamed, const SkMatrix44& toXYZD50, Named named) : fGammaNamed(gammaNamed) , fToXYZD50(toXYZD50) + , fNamed(named) {} -SkColorSpace_Base::SkColorSpace_Base(GammaNamed gammaNamed, const SkMatrix44& toXYZD50) - : INHERITED(gammaNamed, toXYZD50) +SkColorSpace_Base::SkColorSpace_Base(GammaNamed gammaNamed, const SkMatrix44& toXYZD50, Named named) + : INHERITED(gammaNamed, toXYZD50, named) , fGammas(nullptr) , fProfileData(nullptr) {} @@ -24,7 +25,7 @@ SkColorSpace_Base::SkColorSpace_Base(GammaNamed gammaNamed, const SkMatrix44& to SkColorSpace_Base::SkColorSpace_Base(sk_sp colorLUT, GammaNamed gammaNamed, sk_sp gammas, const SkMatrix44& toXYZD50, sk_sp profileData) - : INHERITED(gammaNamed, toXYZD50) + : INHERITED(gammaNamed, toXYZD50, kUnknown_Named) , fColorLUT(std::move(colorLUT)) , fGammas(std::move(gammas)) , fProfileData(std::move(profileData)) @@ -117,19 +118,18 @@ sk_sp SkColorSpace_Base::NewRGB(GammaNamed gammaNamed, const SkMat break; } - return sk_sp(new SkColorSpace_Base(gammaNamed, toXYZD50)); + return sk_sp(new SkColorSpace_Base(gammaNamed, toXYZD50, kUnknown_Named)); } sk_sp SkColorSpace::NewRGB(GammaNamed gammaNamed, const SkMatrix44& toXYZD50) { return SkColorSpace_Base::NewRGB(gammaNamed, toXYZD50); } -static sk_sp gAdobeRGB; -static sk_sp gSRGB; - sk_sp SkColorSpace::NewNamed(Named named) { static SkOnce sRGBOnce; + static sk_sp sRGB; static SkOnce adobeRGBOnce; + static sk_sp adobeRGB; switch (named) { case kSRGB_Named: { @@ -139,9 +139,9 @@ sk_sp SkColorSpace::NewNamed(Named named) { // Force the mutable type mask to be computed. This avoids races. (void)srgbToxyzD50.getType(); - gSRGB.reset(new SkColorSpace_Base(kSRGB_GammaNamed, srgbToxyzD50)); + sRGB.reset(new SkColorSpace_Base(kSRGB_GammaNamed, srgbToxyzD50, kSRGB_Named)); }); - return gSRGB; + return sRGB; } case kAdobeRGB_Named: { adobeRGBOnce([] { @@ -150,9 +150,10 @@ sk_sp SkColorSpace::NewNamed(Named named) { // Force the mutable type mask to be computed. This avoids races. (void)adobergbToxyzD50.getType(); - gAdobeRGB.reset(new SkColorSpace_Base(k2Dot2Curve_GammaNamed, adobergbToxyzD50)); + adobeRGB.reset(new SkColorSpace_Base(k2Dot2Curve_GammaNamed, adobergbToxyzD50, + kAdobeRGB_Named)); }); - return gAdobeRGB; + return adobeRGB; } default: break; @@ -193,8 +194,8 @@ struct ColorSpaceHeader { */ static constexpr uint8_t kFloatGamma_Flag = 1 << 2; - static ColorSpaceHeader Pack(Version version, uint8_t named, uint8_t gammaNamed, uint8_t flags) - { + static ColorSpaceHeader Pack(Version version, SkColorSpace::Named named, + SkColorSpace::GammaNamed gammaNamed, uint8_t flags) { ColorSpaceHeader header; SkASSERT(k0_Version == version); @@ -222,17 +223,17 @@ size_t SkColorSpace::writeToMemory(void* memory) const { // we must have a profile that we can serialize easily. if (!as_CSB(this)->fProfileData) { // If we have a named profile, only write the enum. - if (this == gSRGB.get()) { - if (memory) { - *((ColorSpaceHeader*) memory) = - ColorSpaceHeader::Pack(k0_Version, kSRGB_Named, fGammaNamed, 0); - } - return sizeof(ColorSpaceHeader); - } else if (this == gAdobeRGB.get()) { - if (memory) { - *((ColorSpaceHeader*) memory) = - ColorSpaceHeader::Pack(k0_Version, kAdobeRGB_Named, fGammaNamed, 0); + switch (fNamed) { + case kSRGB_Named: + case kAdobeRGB_Named: { + if (memory) { + *((ColorSpaceHeader*) memory) = + ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, 0); + } + return sizeof(ColorSpaceHeader); } + default: + break; } // If we have a named gamma, write the enum and the matrix. @@ -242,7 +243,7 @@ size_t SkColorSpace::writeToMemory(void* memory) const { case kLinear_GammaNamed: { if (memory) { *((ColorSpaceHeader*) memory) = - ColorSpaceHeader::Pack(k0_Version, 0, fGammaNamed, + ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, ColorSpaceHeader::kMatrix_Flag); memory = SkTAddOffset(memory, sizeof(ColorSpaceHeader)); fToXYZD50.as4x3ColMajorf((float*) memory); @@ -253,7 +254,7 @@ size_t SkColorSpace::writeToMemory(void* memory) const { // Otherwise, write the gamma values and the matrix. if (memory) { *((ColorSpaceHeader*) memory) = - ColorSpaceHeader::Pack(k0_Version, 0, fGammaNamed, + ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, ColorSpaceHeader::kFloatGamma_Flag); memory = SkTAddOffset(memory, sizeof(ColorSpaceHeader)); @@ -280,7 +281,7 @@ size_t SkColorSpace::writeToMemory(void* memory) const { } if (memory) { - *((ColorSpaceHeader*) memory) = ColorSpaceHeader::Pack(k0_Version, 0, + *((ColorSpaceHeader*) memory) = ColorSpaceHeader::Pack(k0_Version, kUnknown_Named, kNonStandard_GammaNamed, ColorSpaceHeader::kICC_Flag); memory = SkTAddOffset(memory, sizeof(ColorSpaceHeader)); @@ -313,8 +314,12 @@ sk_sp SkColorSpace::Deserialize(const void* data, size_t length) { ColorSpaceHeader header = *((const ColorSpaceHeader*) data); data = SkTAddOffset(data, sizeof(ColorSpaceHeader)); length -= sizeof(ColorSpaceHeader); - if (0 == header.fFlags) { - return NewNamed((Named) header.fNamed); + switch ((Named) header.fNamed) { + case kSRGB_Named: + case kAdobeRGB_Named: + return NewNamed((Named) header.fNamed); + default: + break; } switch ((GammaNamed) header.fGammaNamed) { @@ -377,6 +382,17 @@ bool SkColorSpace::Equals(const SkColorSpace* src, const SkColorSpace* dst) { return false; } + switch (src->fNamed) { + case kSRGB_Named: + case kAdobeRGB_Named: + return src->fNamed == dst->fNamed; + case kUnknown_Named: + if (kUnknown_Named != dst->fNamed) { + return false; + } + break; + } + SkData* srcData = as_CSB(src)->fProfileData.get(); SkData* dstData = as_CSB(dst)->fProfileData.get(); if (srcData || dstData) { diff --git a/src/core/SkColorSpace_Base.h b/src/core/SkColorSpace_Base.h index 5c36e441ea..38e0ba0026 100644 --- a/src/core/SkColorSpace_Base.h +++ b/src/core/SkColorSpace_Base.h @@ -193,7 +193,7 @@ private: static sk_sp NewRGB(GammaNamed gammaNamed, const SkMatrix44& toXYZD50); - SkColorSpace_Base(GammaNamed gammaNamed, const SkMatrix44& toXYZ); + SkColorSpace_Base(GammaNamed gammaNamed, const SkMatrix44& toXYZ, Named named); SkColorSpace_Base(sk_sp colorLUT, GammaNamed gammaNamed, sk_sp gammas, const SkMatrix44& toXYZ, sk_sp profileData); diff --git a/tests/ColorSpaceTest.cpp b/tests/ColorSpaceTest.cpp index b59b6cc0cc..3eb145b2e8 100644 --- a/tests/ColorSpaceTest.cpp +++ b/tests/ColorSpaceTest.cpp @@ -149,15 +149,17 @@ DEF_TEST(ColorSpaceWriteICC, r) { DEF_TEST(ColorSpace_Named, r) { const struct { SkColorSpace::Named fNamed; + bool fExpectedToSucceed; bool fIsSRGB; } recs[] { - { SkColorSpace::kSRGB_Named, true }, - { SkColorSpace::kAdobeRGB_Named, false }, + { SkColorSpace::kUnknown_Named, false, false }, + { SkColorSpace::kSRGB_Named, true, true }, + { SkColorSpace::kAdobeRGB_Named, true, false }, }; for (auto rec : recs) { auto cs = SkColorSpace::NewNamed(rec.fNamed); - REPORTER_ASSERT(r, cs); + REPORTER_ASSERT(r, !cs == !rec.fExpectedToSucceed); if (cs) { if (rec.fIsSRGB) { REPORTER_ASSERT(r, SkColorSpace::kSRGB_GammaNamed == cs->gammaNamed());