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;
