From 2410717f900c2691db880d84a2e03a6a24905ee2 Mon Sep 17 00:00:00 2001 From: Matt Sarett Date: Mon, 19 Dec 2016 14:33:35 -0500 Subject: Fix swapped interpretation of c and e in SkColorSpace_ICC The ICC errata supports the opposite of what we do. http://www.color.org/icc_specs2.xalter TBR=reed@google.com BUG=skia: CQ_INCLUDE_TRYBOTS=skia.primary:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD Change-Id: I18ace7f312926b264e624c30d8cb983eff5c434b Reviewed-on: https://skia-review.googlesource.com/6277 Commit-Queue: Matt Sarett Reviewed-by: Brian Osman --- include/core/SkColorSpace.h | 4 ++-- src/core/SkColorSpacePriv.h | 28 +++++++++++++------------- src/core/SkColorSpaceXform.cpp | 24 +++++++++++------------ src/core/SkColorSpaceXform_A2B.cpp | 40 +++++++++++++++++++------------------- src/core/SkColorSpace_ICC.cpp | 27 ++++++++++--------------- src/opts/SkRasterPipeline_opts.h | 4 ++-- tests/ColorSpaceTest.cpp | 8 ++++---- tests/ColorSpaceXformTest.cpp | 10 +++++----- 8 files changed, 69 insertions(+), 76 deletions(-) diff --git a/include/core/SkColorSpace.h b/include/core/SkColorSpace.h index 2139cf8451..6c0db85d7b 100644 --- a/include/core/SkColorSpace.h +++ b/include/core/SkColorSpace.h @@ -33,8 +33,8 @@ struct SK_API SkColorSpacePrimaries { * Contains the coefficients for a common transfer function equation, specified as * a transformation from a curved space to linear. * - * LinearVal = E*InputVal + F , for 0.0f <= InputVal < D - * LinearVal = (A*InputVal + B)^G + C, for D <= InputVal <= 1.0f + * LinearVal = C*InputVal + F , for 0.0f <= InputVal < D + * LinearVal = (A*InputVal + B)^G + E, for D <= InputVal <= 1.0f * * Function is undefined if InputVal is not in [ 0.0f, 1.0f ]. * Resulting LinearVals must be in [ 0.0f, 1.0f ]. diff --git a/src/core/SkColorSpacePriv.h b/src/core/SkColorSpacePriv.h index 32981ae875..f1ef8dc6d6 100644 --- a/src/core/SkColorSpacePriv.h +++ b/src/core/SkColorSpacePriv.h @@ -45,7 +45,7 @@ static inline bool is_valid_transfer_fn(const SkColorSpaceTransferFn& coeffs) { } } - if (coeffs.fD == 1.0f) { + if (coeffs.fD >= 1.0f) { // Y = eX + f for always if (0.0f == coeffs.fE) { SkColorSpacePrintf("E is zero, constant transfer function is " @@ -54,13 +54,13 @@ static inline bool is_valid_transfer_fn(const SkColorSpaceTransferFn& coeffs) { } } - if ((0.0f == coeffs.fA || 0.0f == coeffs.fG) && 0.0f == coeffs.fE) { + if ((0.0f == coeffs.fA || 0.0f == coeffs.fG) && 0.0f == coeffs.fC) { SkColorSpacePrintf("A or G, and E are zero, constant transfer function " "is nonsense"); return false; } - if (coeffs.fE < 0.0f) { + if (coeffs.fC < 0.0f) { SkColorSpacePrintf("Transfer function must be increasing"); return false; } @@ -74,13 +74,13 @@ static inline bool is_valid_transfer_fn(const SkColorSpaceTransferFn& coeffs) { } static inline bool is_almost_srgb(const SkColorSpaceTransferFn& coeffs) { - return color_space_almost_equal(0.9479f, coeffs.fA) && - color_space_almost_equal(0.0521f, coeffs.fB) && - color_space_almost_equal(0.0000f, coeffs.fC) && - color_space_almost_equal(0.0405f, coeffs.fD) && - color_space_almost_equal(0.0774f, coeffs.fE) && - color_space_almost_equal(0.0000f, coeffs.fF) && - color_space_almost_equal(2.4000f, coeffs.fG); + return color_space_almost_equal(1.0f / 1.055f, coeffs.fA) && + color_space_almost_equal(0.055f / 1.055f, coeffs.fB) && + color_space_almost_equal(1.0f / 12.92f, coeffs.fC) && + color_space_almost_equal(0.04045f, coeffs.fD) && + color_space_almost_equal(0.00000f, coeffs.fE) && + color_space_almost_equal(0.00000f, coeffs.fF) && + color_space_almost_equal(2.40000f, coeffs.fG); } static inline bool is_almost_2dot2(const SkColorSpaceTransferFn& coeffs) { @@ -109,9 +109,9 @@ static inline bool named_to_parametric(SkColorSpaceTransferFn* coeffs, case kSRGB_SkGammaNamed: coeffs->fA = 1.0f / 1.055f; coeffs->fB = 0.055f / 1.055f; - coeffs->fC = 0.0f; + coeffs->fC = 1.0f / 12.92f; coeffs->fD = 0.04045f; - coeffs->fE = 1.0f / 12.92f; + coeffs->fE = 0.0f; coeffs->fF = 0.0f; coeffs->fG = 2.4f; return true; @@ -121,11 +121,11 @@ static inline bool named_to_parametric(SkColorSpaceTransferFn* coeffs, case kLinear_SkGammaNamed: coeffs->fA = 0.0f; coeffs->fB = 0.0f; - coeffs->fC = 0.0f; + coeffs->fC = 1.0f; // Make sure that we use the linear segment of the transfer function even // when the x-value is 1.0f. coeffs->fD = add_epsilon(1.0f); - coeffs->fE = 1.0f; + coeffs->fE = 0.0f; coeffs->fF = 0.0f; coeffs->fG = 0.0f; return true; diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp index fb603db45e..6eaab832f0 100644 --- a/src/core/SkColorSpaceXform.cpp +++ b/src/core/SkColorSpaceXform.cpp @@ -116,13 +116,13 @@ static void build_table_linear_from_gamma(float* outTable, const float* inTable, static void build_table_linear_from_gamma(float* outTable, float g, float a, float b, float c, float d, float e, float f) { - // Y = (aX + b)^g + c for X >= d - // Y = eX + f otherwise + // Y = (aX + b)^g + e for X >= d + // Y = cX + f otherwise for (float x = 0.0f; x <= 1.0f; x += (1.0f/255.0f)) { if (x >= d) { - *outTable++ = clamp_0_1(powf(a * x + b, g) + c); + *outTable++ = clamp_0_1(powf(a * x + b, g) + e); } else { - *outTable++ = clamp_0_1(e * x + f); + *outTable++ = clamp_0_1(c * x + f); } } } @@ -154,26 +154,26 @@ static float inverse_parametric(float x, float g, float a, float b, float c, flo // Assume that the gamma function is continuous, or this won't make much sense anyway. // Plug in |d| to the first equation to calculate the new piecewise interval. // Then simply use the inverse of the original functions. - float interval = e * d + f; + float interval = c * d + f; if (x < interval) { - // X = (Y - F) / E - if (0.0f == e) { + // X = (Y - F) / C + if (0.0f == c) { // The gamma curve for this segment is constant, so the inverse is undefined. // Since this is the lower segment, guess zero. return 0.0f; } - return (x - f) / e; + return (x - f) / c; } - // X = ((Y - C)^(1 / G) - B) / A + // X = ((Y - E)^(1 / G) - B) / A if (0.0f == a || 0.0f == g) { // The gamma curve for this segment is constant, so the inverse is undefined. // Since this is the upper segment, guess one. return 1.0f; } - return (powf(x - c, 1.0f / g) - b) / a; + return (powf(x - e, 1.0f / g) - b) / a; } static void build_table_linear_to_gamma(uint8_t* outTable, float g, float a, @@ -237,8 +237,8 @@ static void build_gamma_tables(const T* outGammaTables[3], T* gammaTableStorage, switch (gammas->data(i).fNamed) { case kSRGB_SkGammaNamed: (*fns.fBuildFromParam)(&gammaTableStorage[i * gammaTableSize], 2.4f, - (1.0f / 1.055f), (0.055f / 1.055f), 0.0f, - 0.04045f, (1.0f / 12.92f), 0.0f); + (1.0f / 1.055f), (0.055f / 1.055f), + (1.0f / 12.92f), 0.04045f, 0.0f, 0.0f); outGammaTables[i] = &gammaTableStorage[i * gammaTableSize]; break; case k2Dot2Curve_SkGammaNamed: diff --git a/src/core/SkColorSpaceXform_A2B.cpp b/src/core/SkColorSpaceXform_A2B.cpp index cb8dd57f9d..f65df4c409 100644 --- a/src/core/SkColorSpaceXform_A2B.cpp +++ b/src/core/SkColorSpaceXform_A2B.cpp @@ -85,49 +85,49 @@ static inline bool gamma_to_parametric(SkColorSpaceTransferFn* coeffs, const SkG } } static inline SkColorSpaceTransferFn invert_parametric(const SkColorSpaceTransferFn& fn) { - // Original equation is: y = (ax + b)^g + c for x >= d - // y = ex + f otherwise + // Original equation is: y = (ax + b)^g + e for x >= d + // y = cx + f otherwise // - // so 1st inverse is: (y - c)^(1/g) = ax + b - // x = ((y - c)^(1/g) - b) / a + // so 1st inverse is: (y - e)^(1/g) = ax + b + // x = ((y - e)^(1/g) - b) / a // - // which can be re-written as: x = (1/a)(y - c)^(1/g) - b/a - // x = ((1/a)^g)^(1/g) * (y - c)^(1/g) - b/a - // x = ([(1/a)^g]y + [-((1/a)^g)c]) ^ [1/g] + [-b/a] + // which can be re-written as: x = (1/a)(y - e)^(1/g) - b/a + // x = ((1/a)^g)^(1/g) * (y - e)^(1/g) - b/a + // x = ([(1/a)^g]y + [-((1/a)^g)e]) ^ [1/g] + [-b/a] // - // and 2nd inverse is: x = (y - f) / e - // which can be re-written as: x = [1/e]y + [-f/e] + // and 2nd inverse is: x = (y - f) / c + // which can be re-written as: x = [1/c]y + [-f/c] // // and now both can be expressed in terms of the same parametric form as the // original - parameters are enclosed in square brackets. // find inverse for linear segment (if possible) - float e, f; - if (0.f == fn.fE) { + float c, f; + if (0.f == fn.fC) { // otherwise assume it should be 0 as it is the lower segment // as y = f is a constant function - e = 0.f; + c = 0.f; f = 0.f; } else { - e = 1.f / fn.fE; - f = -fn.fF / fn.fE; + c = 1.f / fn.fC; + f = -fn.fF / fn.fC; } // find inverse for the other segment (if possible) - float g, a, b, c; + float g, a, b, e; if (0.f == fn.fA || 0.f == fn.fG) { // otherwise assume it should be 1 as it is the top segment // as you can't invert the constant functions y = b^g + c, or y = 1 + c g = 1.f; a = 0.f; b = 0.f; - c = 1.f; + e = 1.f; } else { g = 1.f / fn.fG; a = powf(1.f / fn.fA, fn.fG); - b = -a * fn.fC; - c = -fn.fB / fn.fA; + b = -a * fn.fE; + e = -fn.fB / fn.fA; } - const float d = fn.fE * fn.fD + fn.fF; + const float d = fn.fC * fn.fD + fn.fF; return {g, a, b, c, d, e, f}; } @@ -152,7 +152,7 @@ SkColorSpaceXform_A2B::SkColorSpaceXform_A2B(SkColorSpace_A2B* srcSpace, // CMYK images from JPEGs (the only format that supports it) are actually // inverted CMYK, so we need to invert every channel. // TransferFn is y = -x + 1 for x < 1.f, otherwise 0x + 0, ie y = 1 - x for x in [0,1] - this->addTransferFns({1.f, 0.f, 0.f, 0.f, 1.f, -1.f, 1.f}, 4); + this->addTransferFns({1.f, 0.f, 0.f, -1.f, 1.f, 0.f, 1.f}, 4); break; case SkColorSpace_Base::InputColorFormat::kGray: currentChannels = 1; diff --git a/src/core/SkColorSpace_ICC.cpp b/src/core/SkColorSpace_ICC.cpp index b76a981743..adb54565d6 100644 --- a/src/core/SkColorSpace_ICC.cpp +++ b/src/core/SkColorSpace_ICC.cpp @@ -407,8 +407,8 @@ static SkGammas::Type parse_gamma(SkGammas::Data* outData, SkColorSpaceTransferF // Here's where the real parametric gammas start. There are many // permutations of the same equations. // - // Y = (aX + b)^g + c for X >= d - // Y = eX + f otherwise + // Y = (aX + b)^g + e for X >= d + // Y = cX + f otherwise // // We will fill in with zeros as necessary to always match the above form. if (len < 24) { @@ -434,11 +434,11 @@ static SkGammas::Type parse_gamma(SkGammas::Data* outData, SkColorSpaceTransferF return SkGammas::Type::kNone_Type; } - // Y = (aX + b)^g + c for X >= -b/a - // Y = c otherwise - c = read_big_endian_16_dot_16(src + 24); + // Y = (aX + b)^g + e for X >= -b/a + // Y = e otherwise + e = read_big_endian_16_dot_16(src + 24); d = -b / a; - f = c; + f = e; break; case kGABDE_ParaCurveType: tagBytes = 12 + 20; @@ -448,14 +448,9 @@ static SkGammas::Type parse_gamma(SkGammas::Data* outData, SkColorSpaceTransferF } // Y = (aX + b)^g for X >= d - // Y = eX otherwise + // Y = cX otherwise + c = read_big_endian_16_dot_16(src + 24); d = read_big_endian_16_dot_16(src + 28); - - // Not a bug! We define |e| to always be the coefficient on X in the - // second equation. The spec calls this |c| in this particular equation. - // We don't follow their convention because then |c| would have a - // different meaning in each of our cases. - e = read_big_endian_16_dot_16(src + 24); break; case kGABCDEF_ParaCurveType: tagBytes = 12 + 28; @@ -464,10 +459,8 @@ static SkGammas::Type parse_gamma(SkGammas::Data* outData, SkColorSpaceTransferF return SkGammas::Type::kNone_Type; } - // Y = (aX + b)^g + c for X >= d - // Y = eX + f otherwise - // NOTE: The ICC spec writes "cX" in place of "eX" but I think - // it's a typo. + // Y = (aX + b)^g + e for X >= d + // Y = cX + f otherwise c = read_big_endian_16_dot_16(src + 24); d = read_big_endian_16_dot_16(src + 28); e = read_big_endian_16_dot_16(src + 32); diff --git a/src/opts/SkRasterPipeline_opts.h b/src/opts/SkRasterPipeline_opts.h index de25b2d0d8..e011541a16 100644 --- a/src/opts/SkRasterPipeline_opts.h +++ b/src/opts/SkRasterPipeline_opts.h @@ -685,8 +685,8 @@ SI SkNf parametric(const SkNf& v, const SkColorSpaceTransferFn& p) { float result[N]; // Unconstrained powf() doesn't vectorize well... for (int i = 0; i < N; i++) { float s = v[i]; - result[i] = (s <= p.fD) ? p.fE * s + p.fF - : powf(s * p.fA + p.fB, p.fG) + p.fC; + result[i] = (s <= p.fD) ? p.fC * s + p.fF + : powf(s * p.fA + p.fB, p.fG) + p.fE; } // Clamp the output to [0, 1]. // Max(NaN, 0) = 0, but Max(0, NaN) = NaN, so we want this exact order to ensure NaN => 0 diff --git a/tests/ColorSpaceTest.cpp b/tests/ColorSpaceTest.cpp index e1ff1609b2..070d50264f 100644 --- a/tests/ColorSpaceTest.cpp +++ b/tests/ColorSpaceTest.cpp @@ -240,9 +240,9 @@ DEF_TEST(ColorSpace_Serialize, r) { SkColorSpaceTransferFn fn; fn.fA = 1.0f; fn.fB = 0.0f; - fn.fC = 0.0f; + fn.fC = 1.0f; fn.fD = 0.5f; - fn.fE = 1.0f; + fn.fE = 0.0f; fn.fF = 0.0f; fn.fG = 1.0f; SkMatrix44 toXYZ(SkMatrix44::kIdentity_Constructor); @@ -265,9 +265,9 @@ DEF_TEST(ColorSpace_Equals, r) { SkColorSpaceTransferFn fn; fn.fA = 1.0f; fn.fB = 0.0f; - fn.fC = 0.0f; + fn.fC = 1.0f; fn.fD = 0.5f; - fn.fE = 1.0f; + fn.fE = 0.0f; fn.fF = 0.0f; fn.fG = 1.0f; SkMatrix44 toXYZ(SkMatrix44::kIdentity_Constructor); diff --git a/tests/ColorSpaceXformTest.cpp b/tests/ColorSpaceXformTest.cpp index d2bd2a3358..413d74ee85 100644 --- a/tests/ColorSpaceXformTest.cpp +++ b/tests/ColorSpaceXformTest.cpp @@ -176,18 +176,18 @@ DEF_TEST(ColorSpaceXform_ParametricGamma, r) { SkColorSpaceTransferFn* params = SkTAddOffset (memory, sizeof(SkGammas)); - // Interval, switch xforms at 0.0031308f + // Interval. params->fD = 0.04045f; // First equation: - params->fE = 1.0f / 12.92f; + params->fC = 1.0f / 12.92f; params->fF = 0.0f; // Second equation: // Note that the function is continuous (it's actually sRGB). params->fA = 1.0f / 1.055f; params->fB = 0.055f / 1.055f; - params->fC = 0.0f; + params->fE = 0.0f; params->fG = 2.4f; test_identity_xform(r, gammas, true); test_identity_xform_A2B(r, kNonStandard_SkGammaNamed, gammas); @@ -239,9 +239,9 @@ DEF_TEST(ColorSpaceXform_NonMatchingGamma, r) { sizeof(SkGammas) + sizeof(float) * tableSize); params->fA = 1.0f / 1.055f; params->fB = 0.055f / 1.055f; - params->fC = 0.0f; + params->fC = 1.0f / 12.92f; params->fD = 0.04045f; - params->fE = 1.0f / 12.92f; + params->fE = 0.0f; params->fF = 0.0f; params->fG = 2.4f; -- cgit v1.2.3