Fix swapped interpretation of c and e in SkColorSpace_ICC [m56] The ICC errata supports the opposite of what we do. http://www.color.org/icc_specs2.xalter BUG:687194 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=8257 NOTREECHECKS=true NOTRY=true NOPRESUBMIT=true CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot Change-Id: Id363969e15ba12832acb2736e61face10eefe01a Reviewed-on: https://skia-review.googlesource.com/8257 Reviewed-by: Matt Sarett <msarett@google.com> Commit-Queue: Matt Sarett <msarett@google.com>
diff --git a/include/core/SkColorSpace.h b/include/core/SkColorSpace.h index 2139cf8..6c0db85 100644 --- a/include/core/SkColorSpace.h +++ b/include/core/SkColorSpace.h
@@ -33,8 +33,8 @@ * 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 7303873..606442d 100644 --- a/src/core/SkColorSpacePriv.h +++ b/src/core/SkColorSpacePriv.h
@@ -37,7 +37,7 @@ } } - 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 " @@ -46,13 +46,13 @@ } } - 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; } @@ -66,13 +66,13 @@ } 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) {
diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp index 450a643..cadf675 100644 --- a/src/core/SkColorSpaceXform.cpp +++ b/src/core/SkColorSpaceXform.cpp
@@ -117,13 +117,13 @@ 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); } } } @@ -173,26 +173,26 @@ // 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, @@ -256,8 +256,8 @@ 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 ead48f3..9db2ba5 100644 --- a/src/core/SkColorSpaceXform_A2B.cpp +++ b/src/core/SkColorSpaceXform_A2B.cpp
@@ -81,7 +81,7 @@ case kLinear_SkGammaNamed: return value_to_parametric(1.f); case kSRGB_SkGammaNamed: - return {2.4f, (1.f / 1.055f), (0.055f / 1.055f), 0.f, 0.04045f, (1.f / 12.92f), 0.f}; + return {2.4f, (1.f / 1.055f), (0.055f / 1.055f), (1.f / 12.92f), 0.04045f, 0.f, 0.f}; case k2Dot2Curve_SkGammaNamed: return value_to_parametric(2.2f); default: @@ -104,49 +104,49 @@ } } 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 barckets. // 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}; }
diff --git a/src/core/SkColorSpace_ICC.cpp b/src/core/SkColorSpace_ICC.cpp index e365e8a..72a20a8 100644 --- a/src/core/SkColorSpace_ICC.cpp +++ b/src/core/SkColorSpace_ICC.cpp
@@ -419,8 +419,8 @@ // 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) { @@ -446,11 +446,11 @@ 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; @@ -460,14 +460,9 @@ } // 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; @@ -476,10 +471,8 @@ 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 f1aa250..2a9200f 100644 --- a/src/opts/SkRasterPipeline_opts.h +++ b/src/opts/SkRasterPipeline_opts.h
@@ -578,8 +578,8 @@ 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; } return SkNf::Load(result); }
diff --git a/tests/ColorSpaceTest.cpp b/tests/ColorSpaceTest.cpp index e1ff160..070d502 100644 --- a/tests/ColorSpaceTest.cpp +++ b/tests/ColorSpaceTest.cpp
@@ -240,9 +240,9 @@ 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 @@ 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 0e67fe4..3b95d06 100644 --- a/tests/ColorSpaceXformTest.cpp +++ b/tests/ColorSpaceXformTest.cpp
@@ -172,18 +172,18 @@ SkColorSpaceTransferFn* params = SkTAddOffset<SkColorSpaceTransferFn> (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, true); @@ -233,9 +233,9 @@ 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;