Fix storage of gamut transform matrices in SkColorSpace
We were effectively storing the transpose, which made all of our
operations on individual colors, and our concatenation of matrices
awkward and backwards.
I'm planning to push this further into Ganesh, where I had incorrectly
adjusted to the previous layout, treating colors as row vectors in the
shaders.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2324843003
Review-Url: https://codereview.chromium.org/2324843003
diff --git a/gm/color4f.cpp b/gm/color4f.cpp
index 2f86842..8414659 100644
--- a/gm/color4f.cpp
+++ b/gm/color4f.cpp
@@ -96,7 +96,7 @@
SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
// red -> blue, green -> red, blue -> green
- mat.set3x3(0, 1, 0, 0, 0, 1, 1, 0, 0);
+ mat.set3x3(0, 0, 1, 1, 0, 0, 0, 1, 0);
const SkColor4f colors[] {
{ 1, 0, 0, 1 },
diff --git a/gm/gamut.cpp b/gm/gamut.cpp
index a1bea3c..595e36b 100644
--- a/gm/gamut.cpp
+++ b/gm/gamut.cpp
@@ -106,9 +106,9 @@
// We want our colors in our wide gamut to be obviously visibly distorted from sRGB, so we use
// Wide Gamut RGB (with sRGB gamma, for HW acceleration) as the working space for this test:
const float gWideGamutRGB_toXYZD50[]{
- 0.7161046f, 0.2581874f, 0.0000000f, // * R
- 0.1009296f, 0.7249378f, 0.0517813f, // * G
- 0.1471858f, 0.0168748f, 0.7734287f, // * B
+ 0.7161046f, 0.1009296f, 0.1471858f, // -> X
+ 0.2581874f, 0.7249378f, 0.0168748f, // -> Y
+ 0.0000000f, 0.0517813f, 0.7734287f, // -> Z
};
SkMatrix44 wideGamutRGB_toXYZD50(SkMatrix44::kUninitialized_Constructor);
diff --git a/include/core/SkMatrix44.h b/include/core/SkMatrix44.h
index 6b5e65d..9820ee5 100644
--- a/include/core/SkMatrix44.h
+++ b/include/core/SkMatrix44.h
@@ -457,8 +457,8 @@
kAllPublic_Masks = 0xF
};
- void as4x3ColMajorf(float[]) const;
- void set4x3ColMajorf(const float[]);
+ void as3x4RowMajorf(float[]) const;
+ void set3x4RowMajorf(const float[]);
SkMScalar transX() const { return fMat[3][0]; }
SkMScalar transY() const { return fMat[3][1]; }
diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index 36ec21f..46da29e 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -195,9 +195,9 @@
}
static constexpr float gSRGB_toXYZD50[] {
- 0.4358f, 0.2224f, 0.0139f, // * R
- 0.3853f, 0.7170f, 0.0971f, // * G
- 0.1430f, 0.0606f, 0.7139f, // * B
+ 0.4358f, 0.3853f, 0.1430f, // Rx, Gx, Bx
+ 0.2224f, 0.7170f, 0.0606f, // Ry, Gy, Gz
+ 0.0139f, 0.0971f, 0.7139f, // Rz, Gz, Bz
};
static bool convert_to_D50(SkMatrix44* toXYZD50, float toXYZ[9], float whitePoint[2]) {
@@ -251,8 +251,9 @@
toXYZ[8]);
toXYZ3x3.postConcat(DXToD50);
- toXYZD50->set3x3(toXYZ3x3[0], toXYZ3x3[1], toXYZ3x3[2], toXYZ3x3[3], toXYZ3x3[4], toXYZ3x3[5],
- toXYZ3x3[6], toXYZ3x3[7], toXYZ3x3[8]);
+ toXYZD50->set3x3(toXYZ3x3[0], toXYZ3x3[3], toXYZ3x3[6],
+ toXYZ3x3[1], toXYZ3x3[4], toXYZ3x3[7],
+ toXYZ3x3[2], toXYZ3x3[5], toXYZ3x3[8]);
return true;
}
diff --git a/src/core/SkColorSpace.cpp b/src/core/SkColorSpace.cpp
index 9f04de0..02f03ce 100644
--- a/src/core/SkColorSpace.cpp
+++ b/src/core/SkColorSpace.cpp
@@ -34,15 +34,15 @@
{}
static constexpr float gSRGB_toXYZD50[] {
- 0.4358f, 0.2224f, 0.0139f, // * R
- 0.3853f, 0.7170f, 0.0971f, // * G
- 0.1430f, 0.0606f, 0.7139f, // * B
+ 0.4358f, 0.3853f, 0.1430f, // Rx, Gx, Bx
+ 0.2224f, 0.7170f, 0.0606f, // Ry, Gy, Gz
+ 0.0139f, 0.0971f, 0.7139f, // Rz, Gz, Bz
};
static constexpr float gAdobeRGB_toXYZD50[] {
- 0.6098f, 0.3111f, 0.0195f, // * R
- 0.2052f, 0.6257f, 0.0609f, // * G
- 0.1492f, 0.0632f, 0.7448f, // * B
+ 0.6098f, 0.2052f, 0.1492f, // Rx, Gx, Bx
+ 0.3111f, 0.6257f, 0.0632f, // Ry, Gy, By
+ 0.0195f, 0.0609f, 0.7448f, // Rz, Gz, Bz
};
/**
@@ -281,7 +281,7 @@
ColorSpaceHeader::Pack(k0_Version, 0, as_CSB(this)->fGammaNamed,
ColorSpaceHeader::kMatrix_Flag);
memory = SkTAddOffset<void>(memory, sizeof(ColorSpaceHeader));
- fToXYZD50.as4x3ColMajorf((float*) memory);
+ fToXYZD50.as3x4RowMajorf((float*) memory);
}
return sizeof(ColorSpaceHeader) + 12 * sizeof(float);
}
@@ -303,7 +303,7 @@
*(((float*) memory) + 2) = gammas->fBlueData.fValue;
memory = SkTAddOffset<void>(memory, 3 * sizeof(float));
- fToXYZD50.as4x3ColMajorf((float*) memory);
+ fToXYZD50.as3x4RowMajorf((float*) memory);
}
return sizeof(ColorSpaceHeader) + 15 * sizeof(float);
}
@@ -362,7 +362,7 @@
}
SkMatrix44 toXYZ(SkMatrix44::kUninitialized_Constructor);
- toXYZ.set4x3ColMajorf((const float*) data);
+ toXYZ.set3x4RowMajorf((const float*) data);
return SkColorSpace_Base::NewRGB((SkGammaNamed) header.fGammaNamed, toXYZ);
}
default:
@@ -396,7 +396,7 @@
data = SkTAddOffset<const void>(data, 3 * sizeof(float));
SkMatrix44 toXYZ(SkMatrix44::kUninitialized_Constructor);
- toXYZ.set4x3ColMajorf((const float*) data);
+ toXYZ.set3x4RowMajorf((const float*) data);
return SkColorSpace_Base::NewRGB(gammas, toXYZ);
}
default:
diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp
index d43cb56..697169f 100644
--- a/src/core/SkColorSpaceXform.cpp
+++ b/src/core/SkColorSpaceXform.cpp
@@ -328,12 +328,6 @@
///////////////////////////////////////////////////////////////////////////////////////////////////
-static inline void compute_gamut_xform(SkMatrix44* srcToDst, const SkColorSpace* src,
- const SkColorSpace* dst) {
- *srcToDst = as_CSB(dst)->fromXYZD50();
- srcToDst->postConcat(src->toXYZD50());
-}
-
static inline bool is_almost_identity(const SkMatrix44& srcToDst) {
for (int i = 0; i < 4; i++) {
for (int j = 0; j < 4; j++) {
@@ -361,7 +355,7 @@
srcToDst.setIdentity();
csm = kFull_ColorSpaceMatch;
} else {
- compute_gamut_xform(&srcToDst, srcSpace.get(), dstSpace.get());
+ srcToDst.setConcat(as_CSB(dstSpace)->fromXYZD50(), srcSpace->toXYZD50());
if (is_almost_identity(srcToDst)) {
srcToDst.setIdentity();
@@ -984,7 +978,7 @@
const sk_sp<SkColorSpace>& dstSpace)
: fColorLUT(sk_ref_sp((SkColorLookUpTable*) as_CSB(srcSpace)->colorLUT()))
{
- srcToDst.asRowMajorf(fSrcToDst);
+ srcToDst.asColMajorf(fSrcToDst);
build_gamma_tables(fSrcGammaTables, fSrcGammaTableStorage, 256, srcSpace, kToLinear);
build_gamma_tables(fDstGammaTables, fDstGammaTableStorage, kDstGammaTableSize, dstSpace,
kFromLinear);
diff --git a/src/core/SkColorSpace_ICC.cpp b/src/core/SkColorSpace_ICC.cpp
index 27d947b..4ef9f2b 100755
--- a/src/core/SkColorSpace_ICC.cpp
+++ b/src/core/SkColorSpace_ICC.cpp
@@ -714,7 +714,7 @@
array[13] = 0.0f;
array[14] = 0.0f;
array[15] = 1.0f;
- toXYZ->setColMajorf(array);
+ toXYZ->setRowMajorf(array);
return true;
}
@@ -962,7 +962,9 @@
return_null("Need valid rgb tags for XYZ space");
}
SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
- mat.set3x3RowMajorf(toXYZ);
+ mat.set3x3(toXYZ[0], toXYZ[1], toXYZ[2],
+ toXYZ[3], toXYZ[4], toXYZ[5],
+ toXYZ[6], toXYZ[7], toXYZ[8]);
r = ICCTag::Find(tags.get(), tagCount, kTAG_rTRC);
g = ICCTag::Find(tags.get(), tagCount, kTAG_gTRC);
@@ -1213,12 +1215,12 @@
0, // Zero records
};
-static void write_xyz_tag(uint32_t* ptr, const SkMatrix44& toXYZ, int row) {
+static void write_xyz_tag(uint32_t* ptr, const SkMatrix44& toXYZ, int col) {
ptr[0] = SkEndian_SwapBE32(kXYZ_PCSSpace);
ptr[1] = 0;
- ptr[2] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(row, 0)));
- ptr[3] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(row, 1)));
- ptr[4] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(row, 2)));
+ ptr[2] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(0, col)));
+ ptr[3] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(1, col)));
+ ptr[4] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(2, col)));
}
static void write_trc_tag(uint32_t* ptr, float value) {
diff --git a/src/core/SkMatrix44.cpp b/src/core/SkMatrix44.cpp
index 83c1adc..818f23a 100644
--- a/src/core/SkMatrix44.cpp
+++ b/src/core/SkMatrix44.cpp
@@ -85,15 +85,10 @@
#endif
}
-void SkMatrix44::as4x3ColMajorf(float dst[]) const {
- const SkMScalar* src = &fMat[0][0];
-#ifdef SK_MSCALAR_IS_DOUBLE
- for (int i = 0; i < 12; ++i) {
- dst[i] = SkMScalarToFloat(src[i]);
- }
-#elif defined SK_MSCALAR_IS_FLOAT
- memcpy(dst, src, 12 * sizeof(float));
-#endif
+void SkMatrix44::as3x4RowMajorf(float dst[]) const {
+ dst[0] = fMat[0][0]; dst[1] = fMat[1][0]; dst[2] = fMat[2][0]; dst[3] = fMat[3][0];
+ dst[4] = fMat[0][1]; dst[5] = fMat[1][1]; dst[6] = fMat[2][1]; dst[7] = fMat[3][1];
+ dst[8] = fMat[0][2]; dst[9] = fMat[1][2]; dst[10] = fMat[2][2]; dst[11] = fMat[3][2];
}
void SkMatrix44::asColMajord(double dst[]) const {
@@ -228,11 +223,11 @@
this->dirtyTypeMask();
}
-void SkMatrix44::set4x3ColMajorf(const float src[]) {
- fMat[0][0] = src[0]; fMat[0][1] = src[1]; fMat[0][2] = src[2]; fMat[0][3] = src[3];
- fMat[1][0] = src[4]; fMat[1][1] = src[5]; fMat[1][2] = src[6]; fMat[1][3] = src[7];
- fMat[2][0] = src[8]; fMat[2][1] = src[9]; fMat[2][2] = src[10]; fMat[2][3] = src[11];
- fMat[3][0] = 0; fMat[3][1] = 0; fMat[3][2] = 0; fMat[3][3] = 1;
+void SkMatrix44::set3x4RowMajorf(const float src[]) {
+ fMat[0][0] = src[0]; fMat[1][0] = src[1]; fMat[2][0] = src[2]; fMat[3][0] = src[3];
+ fMat[0][1] = src[4]; fMat[1][1] = src[5]; fMat[2][1] = src[6]; fMat[3][1] = src[7];
+ fMat[0][2] = src[8]; fMat[1][2] = src[9]; fMat[2][2] = src[10]; fMat[3][2] = src[11];
+ fMat[0][3] = 0; fMat[1][3] = 0; fMat[2][3] = 0; fMat[3][3] = 1;
this->dirtyTypeMask();
}
diff --git a/src/gpu/GrColorSpaceXform.cpp b/src/gpu/GrColorSpaceXform.cpp
index 8ff6fda..1eb7328 100644
--- a/src/gpu/GrColorSpaceXform.cpp
+++ b/src/gpu/GrColorSpaceXform.cpp
@@ -37,7 +37,8 @@
GrColorSpaceXform::GrColorSpaceXform(const SkMatrix44& srcToDst, SkAlphaType srcAlphaType)
: fSrcAlphaType(srcAlphaType) {
- srcToDst.asColMajorf(fSrcToDst);
+ // TODO: Fix this, and store things as column major!
+ srcToDst.asRowMajorf(fSrcToDst);
}
sk_sp<GrColorSpaceXform> GrColorSpaceXform::Make(SkColorSpace* src, SkColorSpace* dst,
@@ -52,8 +53,8 @@
return nullptr;
}
- SkMatrix44 srcToDst = as_CSB(dst)->fromXYZD50();
- srcToDst.postConcat(src->toXYZD50());
+ SkMatrix44 srcToDst(SkMatrix44::kUninitialized_Constructor);
+ srcToDst.setConcat(as_CSB(dst)->fromXYZD50(), src->toXYZD50());
if (matrix_is_almost_identity(srcToDst)) {
return nullptr;
diff --git a/tests/ColorSpaceTest.cpp b/tests/ColorSpaceTest.cpp
index 969e092..4a64460 100644
--- a/tests/ColorSpaceTest.cpp
+++ b/tests/ColorSpaceTest.cpp
@@ -30,12 +30,13 @@
0, 1, 0, 1,
0, 0, 1, 1,
};
+ const float* ref[3] = { red, green, blue };
float dst[4];
for (int i = 0; i < 3; ++i) {
mat.mapScalars(&src[i*4], dst);
- REPORTER_ASSERT(r, almost_equal(red[i], dst[0]));
- REPORTER_ASSERT(r, almost_equal(green[i], dst[1]));
- REPORTER_ASSERT(r, almost_equal(blue[i], dst[2]));
+ REPORTER_ASSERT(r, almost_equal(ref[i][0], dst[0]));
+ REPORTER_ASSERT(r, almost_equal(ref[i][1], dst[1]));
+ REPORTER_ASSERT(r, almost_equal(ref[i][2], dst[2]));
}
}
@@ -63,20 +64,26 @@
test_space(r, colorSpace, red, green, blue, expectedGamma);
}
-const float g_sRGB_XYZ[] = { 0.4358f, 0.2224f, 0.0139f, // R
- 0.3853f, 0.7170f, 0.0971f, // G
- 0.1430f, 0.0606f, 0.7139f }; // B
+static constexpr float g_sRGB_XYZ[]{
+ 0.4358f, 0.3853f, 0.1430f, // Rx, Gx, Bx
+ 0.2224f, 0.7170f, 0.0606f, // Ry, Gy, Gz
+ 0.0139f, 0.0971f, 0.7139f, // Rz, Gz, Bz
+};
+
+static constexpr float g_sRGB_R[]{ 0.4358f, 0.2224f, 0.0139f };
+static constexpr float g_sRGB_G[]{ 0.3853f, 0.7170f, 0.0971f };
+static constexpr float g_sRGB_B[]{ 0.1430f, 0.0606f, 0.7139f };
DEF_TEST(ColorSpace_sRGB, r) {
test_space(r, SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named).get(),
- g_sRGB_XYZ, &g_sRGB_XYZ[3], &g_sRGB_XYZ[6], kSRGB_SkGammaNamed);
+ g_sRGB_R, g_sRGB_G, g_sRGB_B, kSRGB_SkGammaNamed);
}
DEF_TEST(ColorSpaceParseICCProfiles, r) {
#if (PNG_LIBPNG_VER_MAJOR > 1) || (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 6)
- test_path(r, "color_wheel_with_profile.png", &g_sRGB_XYZ[0], &g_sRGB_XYZ[3], &g_sRGB_XYZ[6],
+ test_path(r, "color_wheel_with_profile.png", g_sRGB_R, g_sRGB_G, g_sRGB_B,
kSRGB_SkGammaNamed);
#endif
@@ -125,8 +132,7 @@
sk_sp<SkColorSpace> namedColorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
sk_sp<SkData> namedData = ColorSpaceTest::WriteToICC(namedColorSpace.get());
sk_sp<SkColorSpace> iccColorSpace = SkColorSpace::NewICC(namedData->data(), namedData->size());
- test_space(r, iccColorSpace.get(), g_sRGB_XYZ, &g_sRGB_XYZ[3], &g_sRGB_XYZ[6],
- k2Dot2Curve_SkGammaNamed);
+ test_space(r, iccColorSpace.get(), g_sRGB_R, g_sRGB_G, g_sRGB_B, k2Dot2Curve_SkGammaNamed);
// FIXME (msarett): Test disabled. sRGB profiles are written approximately as 2.2f curves.
// REPORTER_ASSERT(r, iccColorSpace == namedColorSpace);
diff --git a/tools/visualize_color_gamut.cpp b/tools/visualize_color_gamut.cpp
index a0b8b82..cc15601 100644
--- a/tools/visualize_color_gamut.cpp
+++ b/tools/visualize_color_gamut.cpp
@@ -32,15 +32,15 @@
// rx = rX / (rX + rY + rZ)
// ry = rX / (rX + rY + rZ)
// gx, gy, bx, and gy are calulcated similarly.
- float rSum = xyz.get(0, 0) + xyz.get(0, 1) + xyz.get(0, 2);
- float gSum = xyz.get(1, 0) + xyz.get(1, 1) + xyz.get(1, 2);
- float bSum = xyz.get(2, 0) + xyz.get(2, 1) + xyz.get(2, 2);
+ float rSum = xyz.get(0, 0) + xyz.get(1, 0) + xyz.get(2, 0);
+ float gSum = xyz.get(0, 1) + xyz.get(1, 1) + xyz.get(2, 1);
+ float bSum = xyz.get(0, 2) + xyz.get(1, 2) + xyz.get(2, 2);
rgb[0].fX = xyz.get(0, 0) / rSum;
- rgb[0].fY = xyz.get(0, 1) / rSum;
- rgb[1].fX = xyz.get(1, 0) / gSum;
+ rgb[0].fY = xyz.get(1, 0) / rSum;
+ rgb[1].fX = xyz.get(0, 1) / gSum;
rgb[1].fY = xyz.get(1, 1) / gSum;
- rgb[2].fX = xyz.get(2, 0) / bSum;
- rgb[2].fY = xyz.get(2, 1) / bSum;
+ rgb[2].fX = xyz.get(0, 2) / bSum;
+ rgb[2].fY = xyz.get(1, 2) / bSum;
}
/**
@@ -57,10 +57,10 @@
bool label) {
// Report the XYZ values.
SkDebugf("%s\n", name);
- SkDebugf(" X Y Z\n");
- SkDebugf("Red %.3f %.3f %.3f\n", xyz.get(0, 0), xyz.get(0, 1), xyz.get(0, 2));
- SkDebugf("Green %.3f %.3f %.3f\n", xyz.get(1, 0), xyz.get(1, 1), xyz.get(1, 2));
- SkDebugf("Blue %.3f %.3f %.3f\n", xyz.get(2, 0), xyz.get(2, 1), xyz.get(2, 2));
+ SkDebugf(" R G B\n");
+ SkDebugf("X %.3f %.3f %.3f\n", xyz.get(0, 0), xyz.get(0, 1), xyz.get(0, 2));
+ SkDebugf("Y %.3f %.3f %.3f\n", xyz.get(1, 0), xyz.get(1, 1), xyz.get(1, 2));
+ SkDebugf("Z %.3f %.3f %.3f\n", xyz.get(2, 0), xyz.get(2, 1), xyz.get(2, 2));
// Calculate the points in the gamut from the XYZ values.
SkPoint rgb[4];