From a714bc39294f19500269c8ec536139f75c4b1f25 Mon Sep 17 00:00:00 2001 From: msarett Date: Fri, 29 Jul 2016 08:58:33 -0700 Subject: Fix various SkColorSpace bugs (1) Fixes serialization/deserialization of wacky SkColorSpaces (2) Fix gamma equals checking BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2194903002 Review-Url: https://codereview.chromium.org/2194903002 --- bench/ColorCodecBench.cpp | 2 +- dm/DMSrcSink.cpp | 2 +- resources/icc_profiles/HP_Z32x.icc | Bin 0 -> 1856 bytes resources/icc_profiles/HP_ZR30w.icc | Bin 0 -> 1856 bytes resources/icc_profiles/upperLeft.icc | Bin 0 -> 7460 bytes resources/icc_profiles/upperRight.icc | Bin 0 -> 4056 bytes resources/monitor_profiles/HP_ZR30w.icc | Bin 1856 -> 0 bytes src/core/SkColorSpace.cpp | 66 +++++++++++++++++--------------- src/core/SkColorSpaceXform.cpp | 2 +- src/core/SkColorSpace_Base.h | 43 +++++++++++++++------ tests/ColorSpaceTest.cpp | 10 ++++- 11 files changed, 77 insertions(+), 48 deletions(-) create mode 100644 resources/icc_profiles/HP_Z32x.icc create mode 100644 resources/icc_profiles/HP_ZR30w.icc create mode 100644 resources/icc_profiles/upperLeft.icc create mode 100644 resources/icc_profiles/upperRight.icc delete mode 100644 resources/monitor_profiles/HP_ZR30w.icc diff --git a/bench/ColorCodecBench.cpp b/bench/ColorCodecBench.cpp index 5079b482c7..84814226f1 100644 --- a/bench/ColorCodecBench.cpp +++ b/bench/ColorCodecBench.cpp @@ -140,7 +140,7 @@ void ColorCodecBench::onDelayedSetup() { SkAutoTDelete codec(SkCodec::NewFromData(fEncoded.get())); fSrcData = codec->getICCData(); sk_sp dstData = SkData::MakeFromFileName( - GetResourcePath("monitor_profiles/HP_ZR30w.icc").c_str()); + GetResourcePath("icc_profiles/HP_ZR30w.icc").c_str()); SkASSERT(dstData); fDstSpace = nullptr; diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp index c03a8c4c98..f4c8956b25 100644 --- a/dm/DMSrcSink.cpp +++ b/dm/DMSrcSink.cpp @@ -869,7 +869,7 @@ Error ColorCodecSrc::draw(SkCanvas* canvas) const { // Load the dst ICC profile. This particular dst is fairly similar to Adobe RGB. sk_sp dstData = SkData::MakeFromFileName( - GetResourcePath("monitor_profiles/HP_ZR30w.icc").c_str()); + GetResourcePath("icc_profiles/HP_ZR30w.icc").c_str()); if (!dstData) { return "Cannot read monitor profile. Is the resource path set correctly?"; } diff --git a/resources/icc_profiles/HP_Z32x.icc b/resources/icc_profiles/HP_Z32x.icc new file mode 100644 index 0000000000..3d532b89ed Binary files /dev/null and b/resources/icc_profiles/HP_Z32x.icc differ diff --git a/resources/icc_profiles/HP_ZR30w.icc b/resources/icc_profiles/HP_ZR30w.icc new file mode 100644 index 0000000000..b3f860714c Binary files /dev/null and b/resources/icc_profiles/HP_ZR30w.icc differ diff --git a/resources/icc_profiles/upperLeft.icc b/resources/icc_profiles/upperLeft.icc new file mode 100644 index 0000000000..8c521a5c25 Binary files /dev/null and b/resources/icc_profiles/upperLeft.icc differ diff --git a/resources/icc_profiles/upperRight.icc b/resources/icc_profiles/upperRight.icc new file mode 100644 index 0000000000..44ffa87ec8 Binary files /dev/null and b/resources/icc_profiles/upperRight.icc differ diff --git a/resources/monitor_profiles/HP_ZR30w.icc b/resources/monitor_profiles/HP_ZR30w.icc deleted file mode 100644 index b3f860714c..0000000000 Binary files a/resources/monitor_profiles/HP_ZR30w.icc and /dev/null differ diff --git a/src/core/SkColorSpace.cpp b/src/core/SkColorSpace.cpp index 6c34277b31..3874f0e7d0 100644 --- a/src/core/SkColorSpace.cpp +++ b/src/core/SkColorSpace.cpp @@ -201,49 +201,52 @@ struct ColorSpaceHeader { }; size_t SkColorSpace::writeToMemory(void* memory) const { - // If we have a named profile, only write the enum. - switch (fNamed) { - case kSRGB_Named: - case kAdobeRGB_Named: { - if (memory) { - *((ColorSpaceHeader*) memory) = - ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, 0); + // 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) { + // If we have a named profile, only write the enum. + switch (fNamed) { + case kSRGB_Named: + case kAdobeRGB_Named: { + if (memory) { + *((ColorSpaceHeader*) memory) = + ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, 0); + } + return sizeof(ColorSpaceHeader); } - return sizeof(ColorSpaceHeader); + default: + break; } - default: - break; - } - // If we have a named gamma, write the enum and the matrix. - switch (fGammaNamed) { - case kSRGB_GammaNamed: - case k2Dot2Curve_GammaNamed: - case kLinear_GammaNamed: { - if (memory) { - *((ColorSpaceHeader*) memory) = - ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, - ColorSpaceHeader::kMatrix_Flag); - memory = SkTAddOffset(memory, sizeof(ColorSpaceHeader)); - fToXYZD50.as4x3ColMajorf((float*) memory); + // If we have a named gamma, write the enum and the matrix. + switch (fGammaNamed) { + case kSRGB_GammaNamed: + case k2Dot2Curve_GammaNamed: + case kLinear_GammaNamed: { + if (memory) { + *((ColorSpaceHeader*) memory) = + ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, + ColorSpaceHeader::kMatrix_Flag); + memory = SkTAddOffset(memory, sizeof(ColorSpaceHeader)); + fToXYZD50.as4x3ColMajorf((float*) memory); + } + return sizeof(ColorSpaceHeader) + 12 * sizeof(float); } - return sizeof(ColorSpaceHeader) + 12 * sizeof(float); + default: + SkASSERT(false); + return 0; } - default: - break; } - // If we do not have a named gamma, this must have been created from an ICC profile. - // Since we were unable to recognize the gamma, we will have saved the ICC data. - SkASSERT(as_CSB(this)->fProfileData); - + // Otherwise, serialize the ICC data. size_t profileSize = as_CSB(this)->fProfileData->size(); if (SkAlign4(profileSize) != (uint32_t) SkAlign4(profileSize)) { return 0; } if (memory) { - *((ColorSpaceHeader*) memory) = ColorSpaceHeader::Pack(k0_Version, fNamed, fGammaNamed, + *((ColorSpaceHeader*) memory) = ColorSpaceHeader::Pack(k0_Version, kUnknown_Named, + kNonStandard_GammaNamed, ColorSpaceHeader::kICC_Flag); memory = SkTAddOffset(memory, sizeof(ColorSpaceHeader)); @@ -316,7 +319,8 @@ sk_sp SkColorSpace::Deserialize(const void* data, size_t length) { bool SkColorSpace::gammasAreMatching() const { const SkGammas* gammas = as_CSB(this)->gammas(); SkASSERT(gammas); - return gammas->fRedData == gammas->fGreenData && gammas->fGreenData == gammas->fBlueData; + return gammas->fRedType == gammas->fGreenType && gammas->fGreenType == gammas->fBlueType && + gammas->fRedData == gammas->fGreenData && gammas->fGreenData == gammas->fBlueData; } bool SkColorSpace::gammasAreNamed() const { diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp index 4fbfa6cd9e..4a7f175082 100644 --- a/src/core/SkColorSpaceXform.cpp +++ b/src/core/SkColorSpaceXform.cpp @@ -404,7 +404,7 @@ static void build_gamma_tables(const T* outGammaTables[3], T* gammaTableStorage, // share the same table pointer. This should almost always be true. // I've never seen a profile where all three gamma curves didn't match. // But it is possible that they won't. - if (gammas->data(0) == gammas->data(i)) { + if (gammas->type(0) == gammas->type(i) && gammas->data(0) == gammas->data(i)) { outGammaTables[i] = outGammaTables[0]; continue; } diff --git a/src/core/SkColorSpace_Base.h b/src/core/SkColorSpace_Base.h index 6cfbb8c837..f4507732b5 100644 --- a/src/core/SkColorSpace_Base.h +++ b/src/core/SkColorSpace_Base.h @@ -70,38 +70,57 @@ struct SkGammas : SkRefCnt { }; bool isNamed(int i) const { - SkASSERT(0 <= i && i < 3); - return (&fRedType)[i] == Type::kNamed_Type; + return Type::kNamed_Type == this->type(i); } bool isValue(int i) const { - SkASSERT(0 <= i && i < 3); - return (&fRedType)[i] == Type::kValue_Type; + return Type::kValue_Type == this->type(i); } bool isTable(int i) const { - SkASSERT(0 <= i && i < 3); - return (&fRedType)[i] == Type::kTable_Type; + return Type::kTable_Type == this->type(i); } bool isParametric(int i) const { - SkASSERT(0 <= i && i < 3); - return (&fRedType)[i] == Type::kParam_Type; + return Type::kParam_Type == this->type(i); } const Data& data(int i) const { - SkASSERT(0 <= i && i < 3); - return (&fRedData)[i]; + switch (i) { + case 0: + return fRedData; + case 1: + return fGreenData; + case 2: + return fBlueData; + default: + SkASSERT(false); + return fRedData; + } } const float* table(int i) const { SkASSERT(isTable(i)); - return (&fRedData)[i].fTable.table(this); + return this->data(i).fTable.table(this); } const Params& params(int i) const { SkASSERT(isParametric(i)); - return (&fRedData)[i].params(this); + return this->data(i).params(this); + } + + Type type(int i) const { + switch (i) { + case 0: + return fRedType; + case 1: + return fGreenType; + case 2: + return fBlueType; + default: + SkASSERT(false); + return fRedType; + } } SkGammas() diff --git a/tests/ColorSpaceTest.cpp b/tests/ColorSpaceTest.cpp index f9593d91f9..62a898c2f7 100644 --- a/tests/ColorSpaceTest.cpp +++ b/tests/ColorSpaceTest.cpp @@ -125,7 +125,7 @@ DEF_TEST(ColorSpaceWriteICC, r) { // Test saving the original ICC data sk_sp monitorData = SkData::MakeFromFileName( - GetResourcePath("monitor_profiles/HP_ZR30w.icc").c_str()); + GetResourcePath("icc_profiles/HP_ZR30w.icc").c_str()); REPORTER_ASSERT(r, monitorData); if (!monitorData) { return; @@ -211,7 +211,13 @@ DEF_TEST(ColorSpace_Serialize, r) { test_serialize(r, SkColorSpace::NewNamed(SkColorSpace::kAdobeRGB_Named).get(), true); sk_sp monitorData = SkData::MakeFromFileName( - GetResourcePath("monitor_profiles/HP_ZR30w.icc").c_str()); + GetResourcePath("icc_profiles/HP_ZR30w.icc").c_str()); + test_serialize(r, SkColorSpace::NewICC(monitorData->data(), monitorData->size()).get(), false); + monitorData = SkData::MakeFromFileName( GetResourcePath("icc_profiles/HP_Z32x.icc").c_str()); + test_serialize(r, SkColorSpace::NewICC(monitorData->data(), monitorData->size()).get(), false); + monitorData = SkData::MakeFromFileName(GetResourcePath("icc_profiles/upperLeft.icc").c_str()); + test_serialize(r, SkColorSpace::NewICC(monitorData->data(), monitorData->size()).get(), false); + monitorData = SkData::MakeFromFileName(GetResourcePath("icc_profiles/upperRight.icc").c_str()); test_serialize(r, SkColorSpace::NewICC(monitorData->data(), monitorData->size()).get(), false); } -- cgit v1.2.3