From 63789ccc03f6e63caa3e6c74fee9ad18e073cecf Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Thu, 14 Dec 2017 12:10:05 +0000 Subject: [PATCH] Revert "Remove SkColorSpace_Base::MakeNamed, along with most uses of Adobe RGB" This reverts commit 411b8ea74d66d08252d9b617d7e7d458604dbc2e. Reason for revert: a couple layout tests in the roll. :/ Original change's description: > Remove SkColorSpace_Base::MakeNamed, along with most uses of Adobe RGB > > Bug: skia: > Change-Id: If5935eac48184bc8cbe4db21dac4d6033a8704e6 > Reviewed-on: https://skia-review.googlesource.com/84200 > Commit-Queue: Brian Osman > Reviewed-by: Mike Klein TBR=mtklein@chromium.org,brianosman@google.com Change-Id: I6d2eb57b613035ec26da15218182c808bed364ed No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: skia: Reviewed-on: https://skia-review.googlesource.com/84920 Reviewed-by: Mike Klein Commit-Queue: Mike Klein --- src/codec/SkRawCodec.cpp | 4 +- src/core/SkColorSpace.cpp | 55 +++++++++++++++++----------- src/core/SkColorSpace_Base.h | 9 +++++ src/gpu/GrTestUtils.cpp | 14 +++---- tests/ColorSpaceTest.cpp | 69 +++++++++++++++++++++++++++++++++++ tests/ColorSpaceXformTest.cpp | 5 +-- tests/ICCTest.cpp | 18 +++++---- tests/ImageIsOpaqueTest.cpp | 1 + tests/SurfaceTest.cpp | 3 ++ 9 files changed, 136 insertions(+), 42 deletions(-) diff --git a/src/codec/SkRawCodec.cpp b/src/codec/SkRawCodec.cpp index 4fda120bb8..5f3d469b5d 100644 --- a/src/codec/SkRawCodec.cpp +++ b/src/codec/SkRawCodec.cpp @@ -7,7 +7,6 @@ #include "SkCodec.h" #include "SkCodecPriv.h" -#include "SkColorSpacePriv.h" #include "SkColorData.h" #include "SkData.h" #include "SkJpegCodec.h" @@ -655,8 +654,7 @@ std::unique_ptr SkRawCodec::MakeFromStream(std::unique_ptr st colorSpace = SkColorSpace::MakeSRGB(); break; case ::piex::PreviewImageData::kAdobeRgb: - colorSpace = SkColorSpace::MakeRGB(g2Dot2_TransferFn, - SkColorSpace::kAdobeRGB_Gamut); + colorSpace = SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named); break; } diff --git a/src/core/SkColorSpace.cpp b/src/core/SkColorSpace.cpp index 33f8402da3..06ec26d101 100644 --- a/src/core/SkColorSpace.cpp +++ b/src/core/SkColorSpace.cpp @@ -116,12 +116,17 @@ sk_sp SkColorSpace_Base::MakeRGB(SkGammaNamed gammaNamed, const Sk switch (gammaNamed) { case kSRGB_SkGammaNamed: if (xyz_almost_equal(toXYZD50, gSRGB_toXYZD50)) { - return SkColorSpace::MakeSRGB(); + return SkColorSpace_Base::MakeNamed(kSRGB_Named); + } + break; + case k2Dot2Curve_SkGammaNamed: + if (xyz_almost_equal(toXYZD50, gAdobeRGB_toXYZD50)) { + return SkColorSpace_Base::MakeNamed(kAdobeRGB_Named); } break; case kLinear_SkGammaNamed: if (xyz_almost_equal(toXYZD50, gSRGB_toXYZD50)) { - return SkColorSpace::MakeSRGBLinear(); + return SkColorSpace_Base::MakeNamed(kSRGBLinear_Named); } break; case kNonStandard_SkGammaNamed: @@ -196,6 +201,10 @@ static SkColorSpace* singleton_colorspace(SkGammaNamed gamma, const float to_xyz return new SkColorSpace_XYZ(gamma, m44); } +static SkColorSpace* adobe_rgb() { + static SkColorSpace* cs = singleton_colorspace(k2Dot2Curve_SkGammaNamed, gAdobeRGB_toXYZD50); + return cs; +} static SkColorSpace* srgb() { static SkColorSpace* cs = singleton_colorspace(kSRGB_SkGammaNamed, gSRGB_toXYZD50); return cs; @@ -205,12 +214,22 @@ static SkColorSpace* srgb_linear() { return cs; } +sk_sp SkColorSpace_Base::MakeNamed(Named named) { + switch (named) { + case kSRGB_Named: return sk_ref_sp(srgb()); + case kAdobeRGB_Named: return sk_ref_sp(adobe_rgb()); + case kSRGBLinear_Named: return sk_ref_sp(srgb_linear()); + default: break; + } + return nullptr; +} + sk_sp SkColorSpace::MakeSRGB() { - return sk_ref_sp(srgb()); + return SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kSRGB_Named); } sk_sp SkColorSpace::MakeSRGBLinear() { - return sk_ref_sp(srgb_linear()); + return SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kSRGBLinear_Named); } /////////////////////////////////////////////////////////////////////////////////////////////////// @@ -271,13 +290,6 @@ enum Version { k0_Version, // Initial version, header + flags for matrix and profile }; -enum NamedColorSpace { - kSRGB_NamedColorSpace, - // No longer a singleton, preserved to support reading data from branches m65 and older - kAdobeRGB_NamedColorSpace, - kSRGBLinear_NamedColorSpace, -}; - struct ColorSpaceHeader { /** * It is only valid to set zero or one flags. @@ -310,7 +322,7 @@ struct ColorSpaceHeader { SkASSERT(k0_Version == version); header.fVersion = (uint8_t) version; - SkASSERT(named <= kSRGBLinear_NamedColorSpace); + SkASSERT(named <= SkColorSpace_Base::kSRGBLinear_Named); header.fNamed = (uint8_t) named; SkASSERT(gammaNamed <= kNonStandard_SkGammaNamed); @@ -339,13 +351,19 @@ size_t SkColorSpace::writeToMemory(void* memory) const { if (this == srgb()) { if (memory) { *((ColorSpaceHeader*) memory) = ColorSpaceHeader::Pack( - k0_Version, kSRGB_NamedColorSpace, gammaNamed, 0); + k0_Version, SkColorSpace_Base::kSRGB_Named, gammaNamed, 0); + } + return sizeof(ColorSpaceHeader); + } else if (this == adobe_rgb()) { + if (memory) { + *((ColorSpaceHeader*) memory) = ColorSpaceHeader::Pack( + k0_Version, SkColorSpace_Base::kAdobeRGB_Named, gammaNamed, 0); } return sizeof(ColorSpaceHeader); } else if (this == srgb_linear()) { if (memory) { *((ColorSpaceHeader*) memory) = ColorSpaceHeader::Pack( - k0_Version, kSRGBLinear_NamedColorSpace, gammaNamed, 0); + k0_Version, SkColorSpace_Base::kSRGBLinear_Named, gammaNamed, 0); } return sizeof(ColorSpaceHeader); } @@ -437,14 +455,7 @@ sk_sp SkColorSpace::Deserialize(const void* data, size_t length) { data = SkTAddOffset(data, sizeof(ColorSpaceHeader)); length -= sizeof(ColorSpaceHeader); if (0 == header.fFlags) { - switch ((NamedColorSpace)header.fNamed) { - case kSRGB_NamedColorSpace: - return SkColorSpace::MakeSRGB(); - case kSRGBLinear_NamedColorSpace: - return SkColorSpace::MakeSRGBLinear(); - case kAdobeRGB_NamedColorSpace: - return SkColorSpace::MakeRGB(g2Dot2_TransferFn, SkColorSpace::kAdobeRGB_Gamut); - } + return SkColorSpace_Base::MakeNamed((SkColorSpace_Base::Named) header.fNamed); } switch ((SkGammaNamed) header.fGammaNamed) { diff --git a/src/core/SkColorSpace_Base.h b/src/core/SkColorSpace_Base.h index 1b58280ccb..c59a827a96 100644 --- a/src/core/SkColorSpace_Base.h +++ b/src/core/SkColorSpace_Base.h @@ -141,6 +141,15 @@ public: static sk_sp MakeRGB(SkGammaNamed gammaNamed, const SkMatrix44& toXYZD50); + enum Named : uint8_t { + kSRGB_Named, + kAdobeRGB_Named, + kSRGBLinear_Named, + kSRGB_NonLinearBlending_Named, + }; + + static sk_sp MakeNamed(Named); + protected: SkColorSpace_Base(sk_sp profileData); diff --git a/src/gpu/GrTestUtils.cpp b/src/gpu/GrTestUtils.cpp index 4c35264643..bd190a42ee 100644 --- a/src/gpu/GrTestUtils.cpp +++ b/src/gpu/GrTestUtils.cpp @@ -309,9 +309,9 @@ sk_sp TestColorSpace(SkRandom* random) { gOnce = true; // No color space (legacy mode) gColorSpaces[0] = nullptr; - // sRGB or color-spin sRGB + // sRGB or Adobe gColorSpaces[1] = SkColorSpace::MakeSRGB(); - gColorSpaces[2] = SkColorSpace::MakeSRGB()->makeColorSpin(); + gColorSpaces[2] = SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named); } return gColorSpaces[random->nextULessThan(static_cast(SK_ARRAY_COUNT(gColorSpaces)))]; } @@ -322,13 +322,13 @@ sk_sp TestColorXform(SkRandom* random) { if (!gOnce) { gOnce = true; sk_sp srgb = SkColorSpace::MakeSRGB(); - sk_sp spin = SkColorSpace::MakeSRGB()->makeColorSpin(); + sk_sp adobe = SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named); // No gamut change gXforms[0] = nullptr; - // To different gamut (with automatic transfer function) - gXforms[1] = GrColorSpaceXform::Make(srgb.get(), kSRGBA_8888_GrPixelConfig, spin.get()); - // To different gamut (with manual transfer function) - gXforms[2] = GrColorSpaceXform::Make(spin.get(), kRGBA_8888_GrPixelConfig, srgb.get()); + // To larger gamut (with automatic transfer function) + gXforms[1] = GrColorSpaceXform::Make(srgb.get(), kSRGBA_8888_GrPixelConfig, adobe.get()); + // To smaller gamut (with manual transfer function) + gXforms[2] = GrColorSpaceXform::Make(adobe.get(), kRGBA_8888_GrPixelConfig, srgb.get()); } return gXforms[random->nextULessThan(static_cast(SK_ARRAY_COUNT(gXforms)))]; } diff --git a/tests/ColorSpaceTest.cpp b/tests/ColorSpaceTest.cpp index ff784baf0a..5aa9f17e94 100644 --- a/tests/ColorSpaceTest.cpp +++ b/tests/ColorSpaceTest.cpp @@ -174,6 +174,49 @@ DEF_TEST(ColorSpaceSRGBLinearCompare, r) { REPORTER_ASSERT(r, strangeColorSpace != namedColorSpace); } +DEF_TEST(ColorSpaceAdobeCompare, r) { + // Create an sRGB color space by name + sk_sp namedColorSpace = + SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named); + + // Create an sRGB color space by value + SkMatrix44 adobeToxyzD50(SkMatrix44::kUninitialized_Constructor); + adobeToxyzD50.set3x3RowMajorf(gAdobeRGB_toXYZD50); + + SkColorSpaceTransferFn fn; + fn.fA = 1.0f; + fn.fB = 0.0f; + fn.fC = 0.0f; + fn.fD = 0.0f; + fn.fE = 0.0f; + fn.fF = 0.0f; + fn.fG = 2.2f; + sk_sp rgbColorSpace = SkColorSpace::MakeRGB(fn, adobeToxyzD50); + REPORTER_ASSERT(r, rgbColorSpace == namedColorSpace); +} + +DEF_TEST(ColorSpace_Named, r) { + const struct { + SkColorSpace_Base::Named fNamed; + SkGammaNamed fExpectedGamma; + } recs[] { + { SkColorSpace_Base::kSRGB_Named, kSRGB_SkGammaNamed }, + { SkColorSpace_Base::kAdobeRGB_Named, k2Dot2Curve_SkGammaNamed }, + { SkColorSpace_Base::kSRGBLinear_Named, kLinear_SkGammaNamed }, + }; + + for (auto rec : recs) { + auto cs = SkColorSpace_Base::MakeNamed(rec.fNamed); + REPORTER_ASSERT(r, cs); + if (cs) { + REPORTER_ASSERT(r, rec.fExpectedGamma == cs->gammaNamed()); + } + } + + SkImageInfo info = SkImageInfo::MakeS32(10, 10, kPremul_SkAlphaType); + REPORTER_ASSERT(r, info.gammaCloseToSRGB()); +} + static void test_serialize(skiatest::Reporter* r, SkColorSpace* space, bool isNamed) { sk_sp data1 = space->serialize(); @@ -195,6 +238,7 @@ static void test_serialize(skiatest::Reporter* r, SkColorSpace* space, bool isNa DEF_TEST(ColorSpace_Serialize, r) { test_serialize(r, SkColorSpace::MakeSRGB().get(), true); + test_serialize(r, SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named).get(), true); test_serialize(r, SkColorSpace::MakeSRGBLinear().get(), true); sk_sp monitorData = GetResourceAsData("icc_profiles/HP_ZR30w.icc"); @@ -220,6 +264,7 @@ DEF_TEST(ColorSpace_Serialize, r) { DEF_TEST(ColorSpace_Equals, r) { sk_sp srgb = SkColorSpace::MakeSRGB(); + sk_sp adobe = SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named); sk_sp data = GetResourceAsData("icc_profiles/HP_ZR30w.icc"); sk_sp z30 = SkColorSpace::MakeICC(data->data(), data->size()); data = GetResourceAsData("icc_profiles/HP_Z32x.icc"); @@ -242,6 +287,7 @@ DEF_TEST(ColorSpace_Equals, r) { REPORTER_ASSERT(r, SkColorSpace::Equals(nullptr, nullptr)); REPORTER_ASSERT(r, SkColorSpace::Equals(srgb.get(), srgb.get())); + REPORTER_ASSERT(r, SkColorSpace::Equals(adobe.get(), adobe.get())); REPORTER_ASSERT(r, SkColorSpace::Equals(z30.get(), z30.get())); REPORTER_ASSERT(r, SkColorSpace::Equals(z32.get(), z32.get())); REPORTER_ASSERT(r, SkColorSpace::Equals(upperLeft.get(), upperLeft.get())); @@ -250,11 +296,13 @@ DEF_TEST(ColorSpace_Equals, r) { REPORTER_ASSERT(r, !SkColorSpace::Equals(nullptr, srgb.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(srgb.get(), nullptr)); + REPORTER_ASSERT(r, !SkColorSpace::Equals(adobe.get(), srgb.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(z30.get(), srgb.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(z32.get(), z30.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(upperLeft.get(), srgb.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(upperLeft.get(), upperRight.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(z30.get(), upperRight.get())); + REPORTER_ASSERT(r, !SkColorSpace::Equals(upperRight.get(), adobe.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(z30.get(), rgb4.get())); REPORTER_ASSERT(r, !SkColorSpace::Equals(srgb.get(), rgb4.get())); } @@ -305,6 +353,27 @@ DEF_TEST(ColorSpace_Primaries, r) { srgbToXYZ); REPORTER_ASSERT(r, SkColorSpace::MakeSRGB() == space); + // AdobeRGB primaries (D65) + SkColorSpacePrimaries adobe; + adobe.fRX = 0.64f; + adobe.fRY = 0.33f; + adobe.fGX = 0.21f; + adobe.fGY = 0.71f; + adobe.fBX = 0.15f; + adobe.fBY = 0.06f; + adobe.fWX = 0.3127f; + adobe.fWY = 0.3290f; + SkMatrix44 adobeToXYZ(SkMatrix44::kUninitialized_Constructor); + result = adobe.toXYZD50(&adobeToXYZ); + REPORTER_ASSERT(r, result); + + SkColorSpaceTransferFn fn; + fn.fA = 1.0f; + fn.fB = fn.fC = fn.fD = fn.fE = fn.fF = 0.0f; + fn.fG = 2.2f; + space = SkColorSpace::MakeRGB(fn, adobeToXYZ); + REPORTER_ASSERT(r, SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named) == space); + // ProPhoto (D50) SkColorSpacePrimaries proPhoto; proPhoto.fRX = 0.7347f; diff --git a/tests/ColorSpaceXformTest.cpp b/tests/ColorSpaceXformTest.cpp index db4ac44dc7..041796c5c4 100644 --- a/tests/ColorSpaceXformTest.cpp +++ b/tests/ColorSpaceXformTest.cpp @@ -331,10 +331,9 @@ DEF_TEST(SkColorSpaceXform_LoadTail, r) { std::unique_ptr srcPixel(new uint64_t[1]); srcPixel[0] = 0; uint32_t dstPixel; - sk_sp p3 = SkColorSpace::MakeRGB(SkColorSpace::kSRGB_RenderTargetGamma, - SkColorSpace::kDCIP3_D65_Gamut); + sk_sp adobe = SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named); sk_sp srgb = SkColorSpace::MakeSRGB(); - std::unique_ptr xform = SkColorSpaceXform::New(p3.get(), srgb.get()); + std::unique_ptr xform = SkColorSpaceXform::New(adobe.get(), srgb.get()); // ASAN will catch us if we read past the tail. bool success = xform->apply(SkColorSpaceXform::kRGBA_8888_ColorFormat, &dstPixel, diff --git a/tests/ICCTest.cpp b/tests/ICCTest.cpp index cfe4dc04a0..4933155148 100644 --- a/tests/ICCTest.cpp +++ b/tests/ICCTest.cpp @@ -99,7 +99,8 @@ DEF_TEST(ICC_IsNumericalTransferFn, r) { } static inline void test_write_icc(skiatest::Reporter* r, const SkColorSpaceTransferFn& fn, - const SkMatrix44& toXYZD50, bool writeToFile) { + const SkMatrix44& toXYZD50, SkColorSpace* reference, + bool writeToFile) { sk_sp profile = SkICC::WriteToICC(fn, toXYZD50); if (writeToFile) { SkFILEWStream stream("out.icc"); @@ -107,8 +108,7 @@ static inline void test_write_icc(skiatest::Reporter* r, const SkColorSpaceTrans } sk_sp colorSpace = SkColorSpace::MakeICC(profile->data(), profile->size()); - sk_sp reference = SkColorSpace::MakeRGB(fn, toXYZD50); - REPORTER_ASSERT(r, SkColorSpace::Equals(reference.get(), colorSpace.get())); + REPORTER_ASSERT(r, SkColorSpace::Equals(reference, colorSpace.get())); } DEF_TEST(ICC_WriteICC, r) { @@ -122,9 +122,8 @@ DEF_TEST(ICC_WriteICC, r) { adobeFn.fG = 2.2f; SkMatrix44 adobeMatrix(SkMatrix44::kUninitialized_Constructor); adobeMatrix.set3x3RowMajorf(gAdobeRGB_toXYZD50); - // TODO: Restore this test once we fix our Adobe matrix to be based on the decoded ICC - // fixed point values, and once we use a rounding conversion to fixed-point. -// test_write_icc(r, adobeFn, adobeMatrix, false); + test_write_icc(r, adobeFn, adobeMatrix, + SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named).get(), false); SkColorSpaceTransferFn srgbFn; srgbFn.fA = 1.0f / 1.055f; @@ -136,7 +135,8 @@ DEF_TEST(ICC_WriteICC, r) { srgbFn.fG = 2.4f; SkMatrix44 srgbMatrix(SkMatrix44::kUninitialized_Constructor); srgbMatrix.set3x3RowMajorf(gSRGB_toXYZD50); - test_write_icc(r, srgbFn, srgbMatrix, false); + test_write_icc(r, srgbFn, srgbMatrix, SkColorSpace::MakeSRGB().get(), + false); SkString adobeTag = SkICCGetColorProfileTag(adobeFn, adobeMatrix); SkString srgbTag = SkICCGetColorProfileTag(srgbFn, srgbMatrix); @@ -174,6 +174,10 @@ DEF_TEST(ICC_RawTransferFns, r) { sk_sp srgb = ICCTest::MakeICC(SkColorSpace::MakeSRGB()); test_raw_transfer_fn(r, srgb.get()); + sk_sp adobe = + ICCTest::MakeICC(SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named)); + test_raw_transfer_fn(r, adobe.get()); + // Lookup-table based gamma curves constexpr size_t tableSize = 10; void* memory = sk_malloc_throw(sizeof(SkGammas) + sizeof(float) * tableSize); diff --git a/tests/ImageIsOpaqueTest.cpp b/tests/ImageIsOpaqueTest.cpp index 3850b4f112..38f9b6ae90 100644 --- a/tests/ImageIsOpaqueTest.cpp +++ b/tests/ImageIsOpaqueTest.cpp @@ -52,6 +52,7 @@ DEF_TEST(ImageInfo_flattening, reporter) { sk_sp spaces[] = { nullptr, SkColorSpace::MakeSRGB(), + SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named), space0, space1, space2, diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp index 4b5866466b..942cd8f40d 100644 --- a/tests/SurfaceTest.cpp +++ b/tests/SurfaceTest.cpp @@ -811,6 +811,7 @@ static void test_surface_creation_and_snapshot_with_color_space( std::function(const SkImageInfo&)> surfaceMaker) { auto srgbColorSpace = SkColorSpace::MakeSRGB(); + auto adobeColorSpace = SkColorSpace_Base::MakeNamed(SkColorSpace_Base::kAdobeRGB_Named); const SkMatrix44* srgbMatrix = srgbColorSpace->toXYZD50(); SkASSERT(srgbMatrix); SkColorSpaceTransferFn oddGamma; @@ -829,10 +830,12 @@ static void test_surface_creation_and_snapshot_with_color_space( { kN32_SkColorType, nullptr, true, "N32-nullptr" }, { kN32_SkColorType, linearColorSpace, false, "N32-linear" }, { kN32_SkColorType, srgbColorSpace, true, "N32-srgb" }, + { kN32_SkColorType, adobeColorSpace, true, "N32-adobe" }, { kN32_SkColorType, oddColorSpace, false, "N32-odd" }, { kRGBA_F16_SkColorType, nullptr, true, "F16-nullptr" }, { kRGBA_F16_SkColorType, linearColorSpace, true, "F16-linear" }, { kRGBA_F16_SkColorType, srgbColorSpace, false, "F16-srgb" }, + { kRGBA_F16_SkColorType, adobeColorSpace, false, "F16-adobe" }, { kRGBA_F16_SkColorType, oddColorSpace, false, "F16-odd" }, { kRGB_565_SkColorType, srgbColorSpace, false, "565-srgb" }, { kAlpha_8_SkColorType, srgbColorSpace, false, "A8-srgb" },