Use a table-based implementation of SkDefaultXform

src/core/SkColorSpaceXform.cpp

Index: src/core/SkColorSpaceXform.cpp
diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp
index f42811a549ddaa682ae028872c25b1768c2b9044..f8377db2c9f56c769fd01ab60eebccdc7f0d4bca 100644
--- a/src/core/SkColorSpaceXform.cpp
+++ b/src/core/SkColorSpaceXform.cpp
@@ -8,6 +8,7 @@
 #include "SkColorPriv.h"
 #include "SkColorSpace_Base.h"
 #include "SkColorSpaceXform.h"
+#include "SkColorXform_opts.h"
 #include "SkOpts.h"
 
 static inline bool compute_gamut_xform(SkMatrix44* srcToDst, const SkMatrix44& srcToXYZ,
@@ -70,7 +71,7 @@ std::unique_ptr<SkColorSpaceXform> SkColorSpaceXform::New(const sk_sp<SkColorSpa
     }
 
     return std::unique_ptr<SkColorSpaceXform>(
-            new SkDefaultXform(as_CSB(srcSpace)->gammas(), srcToDst, as_CSB(dstSpace)->gammas()));
+            new SkDefaultXform(srcSpace, srcToDst, dstSpace));
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
@@ -146,12 +147,12 @@ void SkFastXform<SkColorSpace::k2Dot2Curve_GammaNamed, SkColorSpace::k2Dot2Curve
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
-static inline float byte_to_float(uint8_t v) {
+static float byte_to_float(uint8_t v) {
     return ((float) v) * (1.0f / 255.0f);
 }
 
 // Expand range from 0-1 to 0-255, then convert.
-static inline uint8_t clamp_normalized_float_to_byte(float v) {
+static uint8_t clamp_normalized_float_to_byte(float v) {
     // The ordering of the logic is a little strange here in order
     // to make sure we convert NaNs to 0.
     v = v * 255.0f;
@@ -164,23 +165,178 @@ static inline uint8_t clamp_normalized_float_to_byte(float v) {
     }
 }
 
+static void build_table_linear_from_gamma(float* outTable, float exponent) {
+    for (int i = 0; i < 256; i++) {
+        outTable[i] = powf(byte_to_float(i), exponent);

[mtklein_C, 2016/06/21 20:32:52]

Might be clearer and/or faster to just walk the float?

for (float f = 0.0f; f <= 1.0f; f += (1/255.0f) {
    *outTable++ = powf(f, exponent);
}

Same sort of thing below too.

[Brian Osman, 2016/06/21 20:38:17]

But 1/255 isn't exact in float, so you could be setting yourself up for some
(slight) additional error.

[msarett, 2016/06/21 21:24:47]

Interesting thoughts.

I would guess not "faster" enough to matter since powf() is so slow...

I would also guess not "less accurate" enough to matter given that the backward
transform with LUT is so inaccurate...

I lean towards walking the floats (if it works), just seems like nicer code... 
Ahhh but I'm indifferent.

+    }
+}
+
 // Interpolating lookup in a variably sized table.
-static inline float interp_lut(uint8_t byte, float* table, size_t tableSize) {
-    float index = byte_to_float(byte) * (tableSize - 1);
+static float interp_lut(float input, float* table, uint32_t tableSize) {

[mtklein_C, 2016/06/21 20:32:52]

const float* table?

[msarett, 2016/06/21 21:24:47]

Done.

+    float index = input * (tableSize - 1);
     float diff = index - sk_float_floor2int(index);
     return table[(int) sk_float_floor2int(index)] * (1.0f - diff) +
             table[(int) sk_float_ceil2int(index)] * diff;
 }
 
+static void build_table_linear_from_gamma(float* outTable, float* inTable, uint32_t inTableSize) {

[mtklein_C, 2016/06/21 20:32:52]

// outTable is always 256 entries.  inTable can be larger or smaller.

That right?

const float* inTable?

[msarett, 2016/06/21 21:24:47]

Done.

+    if (256 == inTableSize) {
+        memcpy(outTable, inTable, sizeof(float) * 256);
+        return;
+    }
+
+    for (int i = 0; i < 256; i++) {
+        outTable[i] = interp_lut(byte_to_float(i), inTable, inTableSize);
+    }
+}
+
+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
+    for (int i = 0; i < 256; i++) {
+        float x = byte_to_float(i);
+        if (x >= d) {
+            outTable[i] = powf(a * x + b, g) + c;
+        } else {
+            outTable[i] = e * x + f;
+        }
+    }
+}
+
+static constexpr uint8_t linear_to_srgb[1024] = {
+          0,   3,   6,  10,  13,  15,  18,  20,  22,  23,  25,  27,  28,  30,  31,  32,  34,  35,
+         36,  37,  38,  39,  40,  41,  42,  43,  44,  45,  46,  47,  48,  49,  49,  50,  51,  52,
+         53,  53,  54,  55,  56,  56,  57,  58,  58,  59,  60,  61,  61,  62,  62,  63,  64,  64,
+         65,  66,  66,  67,  67,  68,  68,  69,  70,  70,  71,  71,  72,  72,  73,  73,  74,  74,
+         75,  76,  76,  77,  77,  78,  78,  79,  79,  79,  80,  80,  81,  81,  82,  82,  83,  83,
+         84,  84,  85,  85,  85,  86,  86,  87,  87,  88,  88,  88,  89,  89,  90,  90,  91,  91,
+         91,  92,  92,  93,  93,  93,  94,  94,  95,  95,  95,  96,  96,  97,  97,  97,  98,  98,
+         98,  99,  99,  99, 100, 100, 101, 101, 101, 102, 102, 102, 103, 103, 103, 104, 104, 104,
+        105, 105, 106, 106, 106, 107, 107, 107, 108, 108, 108, 109, 109, 109, 110, 110, 110, 110,
+        111, 111, 111, 112, 112, 112, 113, 113, 113, 114, 114, 114, 115, 115, 115, 115, 116, 116,
+        116, 117, 117, 117, 118, 118, 118, 118, 119, 119, 119, 120, 120, 120, 121, 121, 121, 121,
+        122, 122, 122, 123, 123, 123, 123, 124, 124, 124, 125, 125, 125, 125, 126, 126, 126, 126,
+        127, 127, 127, 128, 128, 128, 128, 129, 129, 129, 129, 130, 130, 130, 130, 131, 131, 131,
+        131, 132, 132, 132, 133, 133, 133, 133, 134, 134, 134, 134, 135, 135, 135, 135, 136, 136,
+        136, 136, 137, 137, 137, 137, 138, 138, 138, 138, 138, 139, 139, 139, 139, 140, 140, 140,
+        140, 141, 141, 141, 141, 142, 142, 142, 142, 143, 143, 143, 143, 143, 144, 144, 144, 144,
+        145, 145, 145, 145, 146, 146, 146, 146, 146, 147, 147, 147, 147, 148, 148, 148, 148, 148,
+        149, 149, 149, 149, 150, 150, 150, 150, 150, 151, 151, 151, 151, 152, 152, 152, 152, 152,
+        153, 153, 153, 153, 153, 154, 154, 154, 154, 155, 155, 155, 155, 155, 156, 156, 156, 156,
+        156, 157, 157, 157, 157, 157, 158, 158, 158, 158, 158, 159, 159, 159, 159, 159, 160, 160,
+        160, 160, 160, 161, 161, 161, 161, 161, 162, 162, 162, 162, 162, 163, 163, 163, 163, 163,
+        164, 164, 164, 164, 164, 165, 165, 165, 165, 165, 166, 166, 166, 166, 166, 167, 167, 167,
+        167, 167, 168, 168, 168, 168, 168, 168, 169, 169, 169, 169, 169, 170, 170, 170, 170, 170,
+        171, 171, 171, 171, 171, 171, 172, 172, 172, 172, 172, 173, 173, 173, 173, 173, 173, 174,
+        174, 174, 174, 174, 175, 175, 175, 175, 175, 175, 176, 176, 176, 176, 176, 177, 177, 177,
+        177, 177, 177, 178, 178, 178, 178, 178, 178, 179, 179, 179, 179, 179, 179, 180, 180, 180,
+        180, 180, 181, 181, 181, 181, 181, 181, 182, 182, 182, 182, 182, 182, 183, 183, 183, 183,
+        183, 183, 184, 184, 184, 184, 184, 184, 185, 185, 185, 185, 185, 185, 186, 186, 186, 186,
+        186, 186, 187, 187, 187, 187, 187, 187, 188, 188, 188, 188, 188, 188, 189, 189, 189, 189,
+        189, 189, 190, 190, 190, 190, 190, 190, 191, 191, 191, 191, 191, 191, 191, 192, 192, 192,
+        192, 192, 192, 193, 193, 193, 193, 193, 193, 194, 194, 194, 194, 194, 194, 194, 195, 195,
+        195, 195, 195, 195, 196, 196, 196, 196, 196, 196, 197, 197, 197, 197, 197, 197, 197, 198,
+        198, 198, 198, 198, 198, 199, 199, 199, 199, 199, 199, 199, 200, 200, 200, 200, 200, 200,
+        200, 201, 201, 201, 201, 201, 201, 202, 202, 202, 202, 202, 202, 202, 203, 203, 203, 203,
+        203, 203, 203, 204, 204, 204, 204, 204, 204, 204, 205, 205, 205, 205, 205, 205, 206, 206,
+        206, 206, 206, 206, 206, 207, 207, 207, 207, 207, 207, 207, 208, 208, 208, 208, 208, 208,
+        208, 209, 209, 209, 209, 209, 209, 209, 210, 210, 210, 210, 210, 210, 210, 211, 211, 211,
+        211, 211, 211, 211, 212, 212, 212, 212, 212, 212, 212, 212, 213, 213, 213, 213, 213, 213,
+        213, 214, 214, 214, 214, 214, 214, 214, 215, 215, 215, 215, 215, 215, 215, 216, 216, 216,
+        216, 216, 216, 216, 216, 217, 217, 217, 217, 217, 217, 217, 218, 218, 218, 218, 218, 218,
+        218, 219, 219, 219, 219, 219, 219, 219, 219, 220, 220, 220, 220, 220, 220, 220, 221, 221,
+        221, 221, 221, 221, 221, 221, 222, 222, 222, 222, 222, 222, 222, 222, 223, 223, 223, 223,
+        223, 223, 223, 224, 224, 224, 224, 224, 224, 224, 224, 225, 225, 225, 225, 225, 225, 225,
+        225, 226, 226, 226, 226, 226, 226, 226, 227, 227, 227, 227, 227, 227, 227, 227, 228, 228,
+        228, 228, 228, 228, 228, 228, 229, 229, 229, 229, 229, 229, 229, 229, 230, 230, 230, 230,
+        230, 230, 230, 230, 231, 231, 231, 231, 231, 231, 231, 231, 232, 232, 232, 232, 232, 232,
+        232, 232, 233, 233, 233, 233, 233, 233, 233, 233, 234, 234, 234, 234, 234, 234, 234, 234,
+        235, 235, 235, 235, 235, 235, 235, 235, 236, 236, 236, 236, 236, 236, 236, 236, 236, 237,
+        237, 237, 237, 237, 237, 237, 237, 238, 238, 238, 238, 238, 238, 238, 238, 239, 239, 239,
+        239, 239, 239, 239, 239, 239, 240, 240, 240, 240, 240, 240, 240, 240, 241, 241, 241, 241,
+        241, 241, 241, 241, 241, 242, 242, 242, 242, 242, 242, 242, 242, 243, 243, 243, 243, 243,
+        243, 243, 243, 243, 244, 244, 244, 244, 244, 244, 244, 244, 245, 245, 245, 245, 245, 245,
+        245, 245, 245, 246, 246, 246, 246, 246, 246, 246, 246, 246, 247, 247, 247, 247, 247, 247,
+        247, 247, 248, 248, 248, 248, 248, 248, 248, 248, 248, 249, 249, 249, 249, 249, 249, 249,
+        249, 249, 250, 250, 250, 250, 250, 250, 250, 250, 250, 251, 251, 251, 251, 251, 251, 251,
+        251, 251, 252, 252, 252, 252, 252, 252, 252, 252, 252, 253, 253, 253, 253, 253, 253, 253,
+        253, 253, 254, 254, 254, 254, 254, 254, 254, 254, 254, 255, 255, 255, 255, 255
+};
+
+static constexpr uint8_t linear_to_2dot2[1024] = {
+          0,  11,  15,  18,  21,  23,  25,  26,  28,  30,  31,  32,  34,  35,  36,  37,  39,  40,
+         41,  42,  43,  44,  45,  45,  46,  47,  48,  49,  50,  50,  51,  52,  53,  54,  54,  55,
+         56,  56,  57,  58,  58,  59,  60,  60,  61,  62,  62,  63,  63,  64,  65,  65,  66,  66,
+         67,  68,  68,  69,  69,  70,  70,  71,  71,  72,  72,  73,  73,  74,  74,  75,  75,  76,
+         76,  77,  77,  78,  78,  79,  79,  80,  80,  81,  81,  81,  82,  82,  83,  83,  84,  84,
+         84,  85,  85,  86,  86,  87,  87,  87,  88,  88,  89,  89,  89,  90,  90,  91,  91,  91,
+         92,  92,  93,  93,  93,  94,  94,  94,  95,  95,  96,  96,  96,  97,  97,  97,  98,  98,
+         98,  99,  99,  99, 100, 100, 101, 101, 101, 102, 102, 102, 103, 103, 103, 104, 104, 104,
+        105, 105, 105, 106, 106, 106, 107, 107, 107, 108, 108, 108, 108, 109, 109, 109, 110, 110,
+        110, 111, 111, 111, 112, 112, 112, 112, 113, 113, 113, 114, 114, 114, 115, 115, 115, 115,
+        116, 116, 116, 117, 117, 117, 117, 118, 118, 118, 119, 119, 119, 119, 120, 120, 120, 121,
+        121, 121, 121, 122, 122, 122, 123, 123, 123, 123, 124, 124, 124, 124, 125, 125, 125, 125,
+        126, 126, 126, 127, 127, 127, 127, 128, 128, 128, 128, 129, 129, 129, 129, 130, 130, 130,
+        130, 131, 131, 131, 131, 132, 132, 132, 132, 133, 133, 133, 133, 134, 134, 134, 134, 135,
+        135, 135, 135, 136, 136, 136, 136, 137, 137, 137, 137, 138, 138, 138, 138, 138, 139, 139,
+        139, 139, 140, 140, 140, 140, 141, 141, 141, 141, 142, 142, 142, 142, 142, 143, 143, 143,
+        143, 144, 144, 144, 144, 144, 145, 145, 145, 145, 146, 146, 146, 146, 146, 147, 147, 147,
+        147, 148, 148, 148, 148, 148, 149, 149, 149, 149, 149, 150, 150, 150, 150, 151, 151, 151,
+        151, 151, 152, 152, 152, 152, 152, 153, 153, 153, 153, 154, 154, 154, 154, 154, 155, 155,
+        155, 155, 155, 156, 156, 156, 156, 156, 157, 157, 157, 157, 157, 158, 158, 158, 158, 158,
+        159, 159, 159, 159, 159, 160, 160, 160, 160, 160, 161, 161, 161, 161, 161, 162, 162, 162,
+        162, 162, 163, 163, 163, 163, 163, 164, 164, 164, 164, 164, 165, 165, 165, 165, 165, 165,
+        166, 166, 166, 166, 166, 167, 167, 167, 167, 167, 168, 168, 168, 168, 168, 168, 169, 169,
+        169, 169, 169, 170, 170, 170, 170, 170, 171, 171, 171, 171, 171, 171, 172, 172, 172, 172,
+        172, 173, 173, 173, 173, 173, 173, 174, 174, 174, 174, 174, 174, 175, 175, 175, 175, 175,
+        176, 176, 176, 176, 176, 176, 177, 177, 177, 177, 177, 177, 178, 178, 178, 178, 178, 179,
+        179, 179, 179, 179, 179, 180, 180, 180, 180, 180, 180, 181, 181, 181, 181, 181, 181, 182,
+        182, 182, 182, 182, 182, 183, 183, 183, 183, 183, 183, 184, 184, 184, 184, 184, 185, 185,
+        185, 185, 185, 185, 186, 186, 186, 186, 186, 186, 186, 187, 187, 187, 187, 187, 187, 188,
+        188, 188, 188, 188, 188, 189, 189, 189, 189, 189, 189, 190, 190, 190, 190, 190, 190, 191,
+        191, 191, 191, 191, 191, 192, 192, 192, 192, 192, 192, 192, 193, 193, 193, 193, 193, 193,
+        194, 194, 194, 194, 194, 194, 195, 195, 195, 195, 195, 195, 195, 196, 196, 196, 196, 196,
+        196, 197, 197, 197, 197, 197, 197, 197, 198, 198, 198, 198, 198, 198, 199, 199, 199, 199,
+        199, 199, 199, 200, 200, 200, 200, 200, 200, 201, 201, 201, 201, 201, 201, 201, 202, 202,
+        202, 202, 202, 202, 202, 203, 203, 203, 203, 203, 203, 204, 204, 204, 204, 204, 204, 204,
+        205, 205, 205, 205, 205, 205, 205, 206, 206, 206, 206, 206, 206, 206, 207, 207, 207, 207,
+        207, 207, 207, 208, 208, 208, 208, 208, 208, 209, 209, 209, 209, 209, 209, 209, 210, 210,
+        210, 210, 210, 210, 210, 211, 211, 211, 211, 211, 211, 211, 212, 212, 212, 212, 212, 212,
+        212, 213, 213, 213, 213, 213, 213, 213, 213, 214, 214, 214, 214, 214, 214, 214, 215, 215,
+        215, 215, 215, 215, 215, 216, 216, 216, 216, 216, 216, 216, 217, 217, 217, 217, 217, 217,
+        217, 218, 218, 218, 218, 218, 218, 218, 218, 219, 219, 219, 219, 219, 219, 219, 220, 220,
+        220, 220, 220, 220, 220, 221, 221, 221, 221, 221, 221, 221, 221, 222, 222, 222, 222, 222,
+        222, 222, 223, 223, 223, 223, 223, 223, 223, 223, 224, 224, 224, 224, 224, 224, 224, 225,
+        225, 225, 225, 225, 225, 225, 225, 226, 226, 226, 226, 226, 226, 226, 226, 227, 227, 227,
+        227, 227, 227, 227, 228, 228, 228, 228, 228, 228, 228, 228, 229, 229, 229, 229, 229, 229,
+        229, 229, 230, 230, 230, 230, 230, 230, 230, 230, 231, 231, 231, 231, 231, 231, 231, 232,
+        232, 232, 232, 232, 232, 232, 232, 233, 233, 233, 233, 233, 233, 233, 233, 234, 234, 234,
+        234, 234, 234, 234, 234, 235, 235, 235, 235, 235, 235, 235, 235, 236, 236, 236, 236, 236,
+        236, 236, 236, 237, 237, 237, 237, 237, 237, 237, 237, 238, 238, 238, 238, 238, 238, 238,
+        238, 238, 239, 239, 239, 239, 239, 239, 239, 239, 240, 240, 240, 240, 240, 240, 240, 240,
+        241, 241, 241, 241, 241, 241, 241, 241, 242, 242, 242, 242, 242, 242, 242, 242, 243, 243,
+        243, 243, 243, 243, 243, 243, 243, 244, 244, 244, 244, 244, 244, 244, 244, 245, 245, 245,
+        245, 245, 245, 245, 245, 245, 246, 246, 246, 246, 246, 246, 246, 246, 247, 247, 247, 247,
+        247, 247, 247, 247, 248, 248, 248, 248, 248, 248, 248, 248, 248, 249, 249, 249, 249, 249,
+        249, 249, 249, 249, 250, 250, 250, 250, 250, 250, 250, 250, 251, 251, 251, 251, 251, 251,
+        251, 251, 251, 252, 252, 252, 252, 252, 252, 252, 252, 252, 253, 253, 253, 253, 253, 253,
+        253, 253, 254, 254, 254, 254, 254, 254, 254, 254, 254, 255, 255, 255, 255, 255,
+};
+
+static void build_table_linear_to_gamma(uint8_t* outTable, uint32_t outTableSize, float exponent) {
+    float toGammaExp = 1.0f / exponent;
+
+    for (uint32_t i = 0; i < outTableSize; i++) {
+        float x = ((float) i) * (1.0f / ((float) (outTableSize - 1)));
+        outTable[i] = clamp_normalized_float_to_byte(powf(x, toGammaExp));
+    }
+}
+
 // Inverse table lookup.  Ex: what index corresponds to the input value?  This will
 // have strange results when the table is non-increasing.  But any sane gamma
 // function will be increasing.
-// FIXME (msarett):
-// This is a placeholder implementation for inverting table gammas.  First, I need to
-// verify if there are actually destination profiles that require this functionality.
-// Next, there are certainly faster and more robust approaches to solving this problem.
-// The LUT based approach in QCMS would be a good place to start.
-static inline float interp_lut_inv(float input, float* table, size_t tableSize) {
+static float inverse_interp_lut(float input, float* table, uint32_t tableSize) {
     if (input <= table[0]) {
         return table[0];
     } else if (input >= table[tableSize - 1]) {
@@ -203,46 +359,228 @@ static inline float interp_lut_inv(float input, float* table, size_t tableSize)
     return 0.0f;
 }
 
-SkDefaultXform::SkDefaultXform(const sk_sp<SkGammas>& srcGammas, const SkMatrix44& srcToDst,
-                               const sk_sp<SkGammas>& dstGammas)
-    : fSrcGammas(srcGammas)
-    , fSrcToDst(srcToDst)
-    , fDstGammas(dstGammas)
-{}
+static void build_table_linear_to_gamma(uint8_t* outTable, uint32_t outTableSize, float* inTable,
+                                        uint32_t inTableSize) {
+    for (uint32_t i = 0; i < outTableSize; i++) {
+        float x = ((float) i) * (1.0f / ((float) (outTableSize - 1)));
+        float y = inverse_interp_lut(x, inTable, inTableSize);
+        outTable[i] = clamp_normalized_float_to_byte(y);
+    }
+}
 
-void SkDefaultXform::xform_RGB1_8888(uint32_t* dst, const uint32_t* src, uint32_t len) const {
-    while (len-- > 0) {
-        // Convert to linear.
-        // FIXME (msarett):
-        // Rather than support three different strategies of transforming gamma, QCMS
-        // builds a 256 entry float lookup table from the gamma info.  This handles
-        // the gamma transform and the conversion from bytes to floats.  This may
-        // be simpler and faster than our current approach.
-        float srcFloats[3];
-        for (int i = 0; i < 3; i++) {
-            uint8_t byte = (*src >> (8 * i)) & 0xFF;
-            if (fSrcGammas) {
-                const SkGammaCurve& gamma = (*fSrcGammas)[i];
-                if (gamma.isValue()) {
-                    srcFloats[i] = powf(byte_to_float(byte), gamma.fValue);
-                } else if (gamma.isTable()) {
-                    srcFloats[i] = interp_lut(byte, gamma.fTable.get(), gamma.fTableSize);
+static float inverse_parametric(float x, float g, float a, float b, float c, float d, float e,
+                                float f) {
+    // We need to take the inverse of the following piecewise function.
+    // Y = (aX + b)^g + c  for X >= d
+    // Y = eX + f          otherwise
+
+    // 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;
+    if (x < interval) {
+        // X = (Y - F) / E
+        if (0.0f == e) {
+            // 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;
+    }
+
+    // X = ((Y - C)^(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;
+}
+
+static void build_table_linear_to_gamma(uint8_t* outTable, uint32_t outTableSize, float g, float a,
+                                        float b, float c, float d, float e, float f) {
+    for (uint32_t i = 0; i < outTableSize; i++) {
+        float x = ((float) i) * (1.0f / ((float) (outTableSize - 1)));
+        float y = inverse_parametric(x, g, a, b, c, d, e, f);
+        outTable[i] = clamp_normalized_float_to_byte(y);
+    }
+}
+
+SkDefaultXform::SkDefaultXform(const sk_sp<SkColorSpace>& srcSpace, const SkMatrix44& srcToDst,
+                               const sk_sp<SkColorSpace>& dstSpace)
+    : fSrcToDst(srcToDst)
+{
+    // Build tables to transform src gamma to linear.
+    switch (srcSpace->gammaNamed()) {
+        case SkColorSpace::kSRGB_GammaNamed:
+            fSrcGammaTables[0] = fSrcGammaTables[1] = fSrcGammaTables[2] =
+                    (float*) SK_OPTS_NS::linear_from_srgb;

[mtklein_C, 2016/06/21 20:32:52]

This is weird to refer to symbols in SK_OPTS_NS.  Let's put these tables in here
and just declare them in SkColorXform_opts.h?

(This will also reduce duplication... there are probably identical
sk_default::linear_from_srgb and an sk_sse41::linear_from_srgb tables right
now.)

[msarett, 2016/06/21 21:24:47]

SGTM.  Done.

+            break;
+        case SkColorSpace::k2Dot2Curve_GammaNamed:
+            fSrcGammaTables[0] = fSrcGammaTables[1] = fSrcGammaTables[2] =
+                    (float*) SK_OPTS_NS::linear_from_2dot2;
+            break;
+        case SkColorSpace::kLinear_GammaNamed:
+            build_table_linear_from_gamma(fSrcGammaTableStorage, 1.0f);
+            fSrcGammaTables[0] = fSrcGammaTables[1] = fSrcGammaTables[2] = fSrcGammaTableStorage;
+            break;
+        default: {
+            SkGammas* gammas = as_CSB(srcSpace)->gammas();
+            SkASSERT(gammas);
+
+            for (int i = 0; i < 3; i++) {
+                const SkGammaCurve& curve = (*gammas)[i];
+
+                if (i > 0) {
+                    // Check if this curve matches the first curve.  In this case, we can
+                    // share the same table pointer.  Logically, this should almost always
+                    // be true.  I've never seen a profile where all three gamma curves
+                    // didn't match.  But it is possible that they won't.
+                    // TODO (msarett):
+                    // This comparison won't catch the case where each gamma curve has a
+                    // pointer to its own look-up table, but the tables actually match.
+                    // Should we perform a deep compare of gamma tables here?  Or should
+                    // we catch this when parsing the profile?  Or should we not worry
+                    // about a bit of redundant work?
+                    const SkGammaCurve& firstCurve = (*gammas)[0];
+                    if (curve == firstCurve) {
+                        fSrcGammaTables[i] = fSrcGammaTables[0];
+                        continue;
+                    }
+                }
+
+                if (curve.isNamed()) {
+                    switch (curve.fNamed) {
+                        case SkColorSpace::kSRGB_GammaNamed:
+                            fSrcGammaTables[i] = (float*) SK_OPTS_NS::linear_from_srgb;
+                            break;
+                        case SkColorSpace::k2Dot2Curve_GammaNamed:
+                            fSrcGammaTables[i] = (float*) SK_OPTS_NS::linear_from_2dot2;
+                            break;
+                        case SkColorSpace::kLinear_GammaNamed:
+                            build_table_linear_from_gamma(&fSrcGammaTableStorage[i * 256], 1.0f);
+                            fSrcGammaTables[i] = &fSrcGammaTableStorage[i * 256];
+                            break;
+                        default:
+                            SkASSERT(false);
+                            break;
+                    }
+                } else if (curve.isValue()) {
+                    build_table_linear_from_gamma(&fSrcGammaTableStorage[i * 256], curve.fValue);
+                    fSrcGammaTables[i] = &fSrcGammaTableStorage[i * 256];
+                } else if (curve.isTable()) {
+                    build_table_linear_from_gamma(&fSrcGammaTableStorage[i * 256],
+                                                  curve.fTable.get(), curve.fTableSize);
+                    fSrcGammaTables[i] = &fSrcGammaTableStorage[i * 256];
                 } else {
-                    SkASSERT(gamma.isParametric());
-                    float component = byte_to_float(byte);
-                    if (component < gamma.fD) {
-                        // Y = E * X + F
-                        srcFloats[i] = gamma.fE * component + gamma.fF;
-                    } else {
-                        // Y = (A * X + B)^G + C
-                        srcFloats[i] = powf(gamma.fA * component + gamma.fB, gamma.fG) + gamma.fC;
+                    SkASSERT(curve.isParametric());
+                    build_table_linear_from_gamma(&fSrcGammaTableStorage[i * 256], curve.fG,
+                                                  curve.fA, curve.fB, curve.fC, curve.fD, curve.fE,
+                                                  curve.fF);
+                    fSrcGammaTables[i] = &fSrcGammaTableStorage[i * 256];
+                }
+            }
+        }
+    }
+
+    // Build tables to transform linear to dst gamma.
+    switch (dstSpace->gammaNamed()) {
+        case SkColorSpace::kSRGB_GammaNamed:
+            fDstGammaTables[0] = fDstGammaTables[1] = fDstGammaTables[2] =
+                    (uint8_t*) linear_to_srgb;
+            break;
+        case SkColorSpace::k2Dot2Curve_GammaNamed:
+            fDstGammaTables[0] = fDstGammaTables[1] = fDstGammaTables[2] =
+                    (uint8_t*) linear_to_2dot2;
+            break;
+        case SkColorSpace::kLinear_GammaNamed:
+            build_table_linear_to_gamma(fDstGammaTableStorage, kDstGammaTableSize, 1.0f);
+            fDstGammaTables[0] = fDstGammaTables[1] = fDstGammaTables[2] = fDstGammaTableStorage;
+            break;
+        default: {
+            SkGammas* gammas = as_CSB(dstSpace)->gammas();
+            SkASSERT(gammas);
+
+            for (int i = 0; i < 3; i++) {
+                const SkGammaCurve& curve = (*gammas)[i];
+
+                if (i > 0) {
+                    // Check if this curve matches the first curve.  In this case, we can
+                    // share the same table pointer.  Logically, this should almost always
+                    // be true.  I've never seen a profile where all three gamma curves
+                    // didn't match.  But it is possible that they won't.
+                    // TODO (msarett):
+                    // This comparison won't catch the case where each gamma curve has a
+                    // pointer to its own look-up table (but the tables actually match).
+                    // Should we perform a deep compare of gamma tables here?  Or should
+                    // we catch this when parsing the profile?  Or should we not worry
+                    // about a bit of redundant work?
+                    const SkGammaCurve& firstCurve = (*gammas)[0];
+                    if (curve == firstCurve) {
+                        fDstGammaTables[i] = fDstGammaTables[0];
+                        continue;
                     }
                 }
-            } else {
-                // FIXME: Handle named gammas.
-                srcFloats[i] = powf(byte_to_float(byte), 2.2f);
+
+                if (curve.isNamed()) {
+                    switch (curve.fNamed) {
+                        case SkColorSpace::kSRGB_GammaNamed:
+                            fDstGammaTables[i] = (uint8_t*) linear_to_srgb;
+                            break;
+                        case SkColorSpace::k2Dot2Curve_GammaNamed:
+                            fDstGammaTables[i] = (uint8_t*) linear_to_2dot2;
+                            break;
+                        case SkColorSpace::kLinear_GammaNamed:
+                            build_table_linear_to_gamma(
+                                    &fDstGammaTableStorage[i * kDstGammaTableSize],
+                                    kDstGammaTableSize, 1.0f);
+                            fDstGammaTables[i] = &fDstGammaTableStorage[i * kDstGammaTableSize];
+                            break;
+                        default:
+                            SkASSERT(false);
+                            break;
+                    }
+                } else if (curve.isValue()) {
+                    build_table_linear_to_gamma(&fDstGammaTableStorage[i * kDstGammaTableSize],
+                                                kDstGammaTableSize, curve.fValue);
+                    fDstGammaTables[i] = &fDstGammaTableStorage[i * kDstGammaTableSize];
+                } else if (curve.isTable()) {
+                    build_table_linear_to_gamma(&fDstGammaTableStorage[i * kDstGammaTableSize],
+                                                kDstGammaTableSize, curve.fTable.get(),
+                                                curve.fTableSize);
+                    fDstGammaTables[i] = &fDstGammaTableStorage[i * kDstGammaTableSize];
+                } else {
+                    SkASSERT(curve.isParametric());
+                    build_table_linear_to_gamma(&fDstGammaTableStorage[i * kDstGammaTableSize],
+                                                kDstGammaTableSize, curve.fG, curve.fA, curve.fB,
+                                                curve.fC, curve.fD, curve.fE, curve.fF);
+                    fDstGammaTables[i] = &fDstGammaTableStorage[i * kDstGammaTableSize];
+                }
             }
         }
+    }
+}
+
+// Clamp to the 0-1 range.
+static float clamp_normalized_float(float v) {
+    if (v > 1.0f) {
+        return 1.0f;
+    } else if ((v < 0.0f) || (v != v)) {
+        return 0.0f;
+    } else {
+        return v;
+    }
+}
+
+void SkDefaultXform::xform_RGB1_8888(uint32_t* dst, const uint32_t* src, uint32_t len) const {

[msarett, 2016/06/20 19:18:53]

This is a functions that we can actually optimize.

+    while (len-- > 0) {
+        // Convert to linear.
+        float srcFloats[3];
+        srcFloats[0] = fSrcGammaTables[0][(*src >>  0) & 0xFF];
+        srcFloats[1] = fSrcGammaTables[1][(*src >>  8) & 0xFF];
+        srcFloats[2] = fSrcGammaTables[2][(*src >> 16) & 0xFF];
 
         // Convert to dst gamut.
         float dstFloats[3];
@@ -256,67 +594,17 @@ void SkDefaultXform::xform_RGB1_8888(uint32_t* dst, const uint32_t* src, uint32_
                        srcFloats[1] * fSrcToDst.getFloat(1, 2) +
                        srcFloats[2] * fSrcToDst.getFloat(2, 2) + fSrcToDst.getFloat(3, 2);
 
+        // Clamp to 0-1.
+        dstFloats[0] = clamp_normalized_float(dstFloats[0]);
+        dstFloats[1] = clamp_normalized_float(dstFloats[1]);
+        dstFloats[2] = clamp_normalized_float(dstFloats[2]);
+
         // Convert to dst gamma.
-        // FIXME (msarett):
-        // Rather than support three different strategies of transforming inverse gamma,
-        // QCMS builds a large float lookup table from the gamma info.  Is this faster or
-        // better than our approach?
-        for (int i = 0; i < 3; i++) {
-            if (fDstGammas) {
-                const SkGammaCurve& gamma = (*fDstGammas)[i];
-                if (gamma.isValue()) {
-                    dstFloats[i] = powf(dstFloats[i], 1.0f / gamma.fValue);
-                } else if (gamma.isTable()) {
-                    // FIXME (msarett):
-                    // An inverse table lookup is particularly strange and non-optimal.
-                    dstFloats[i] = interp_lut_inv(dstFloats[i], gamma.fTable.get(),
-                                                  gamma.fTableSize);
-                } else {
-                    SkASSERT(gamma.isParametric());
-                    // FIXME (msarett):
-                    // This is a placeholder implementation for inverting parametric gammas.
-                    // First, I need to verify if there are actually destination profiles that
-                    // require this functionality. Next, I need to explore other possibilities
-                    // for this implementation.  The LUT based approach in QCMS would be a good
-                    // place to start.
-
-                    // We need to take the inverse of a piecewise function.  Assume that
-                    // the gamma function is continuous, or this won't make much sense
-                    // anyway.
-                    // Plug in |fD| to the first equation to calculate the new piecewise
-                    // interval.  Then simply use the inverse of the original functions.
-                    float interval = gamma.fE * gamma.fD + gamma.fF;
-                    if (dstFloats[i] < interval) {
-                        // X = (Y - F) / E
-                        if (0.0f == gamma.fE) {
-                            // The gamma curve for this segment is constant, so the inverse
-                            // is undefined.
-                            dstFloats[i] = 0.0f;
-                        } else {
-                            dstFloats[i] = (dstFloats[i] - gamma.fF) / gamma.fE;
-                        }
-                    } else {
-                        // X = ((Y - C)^(1 / G) - B) / A
-                        if (0.0f == gamma.fA || 0.0f == gamma.fG) {
-                            // The gamma curve for this segment is constant, so the inverse
-                            // is undefined.
-                            dstFloats[i] = 0.0f;
-                        } else {
-                            dstFloats[i] = (powf(dstFloats[i] - gamma.fC, 1.0f / gamma.fG) -
-                                            gamma.fB) / gamma.fA;
-                        }
-                    }
-                }
-            } else {
-                // FIXME: Handle named gammas.
-                dstFloats[i] = powf(dstFloats[i], 1.0f / 2.2f);
-            }
-        }
+        uint8_t r = fDstGammaTables[0][sk_float_round2int((kDstGammaTableSize - 1) * dstFloats[0])];
+        uint8_t g = fDstGammaTables[1][sk_float_round2int((kDstGammaTableSize - 1) * dstFloats[1])];
+        uint8_t b = fDstGammaTables[2][sk_float_round2int((kDstGammaTableSize - 1) * dstFloats[2])];
 
-        *dst = SkPackARGB32NoCheck(((*src >> 24) & 0xFF),
-                                   clamp_normalized_float_to_byte(dstFloats[0]),
-                                   clamp_normalized_float_to_byte(dstFloats[1]),
-                                   clamp_normalized_float_to_byte(dstFloats[2]));
+        *dst = SkPackARGB32NoCheck(0xFF, r, g, b);
 
         dst++;
         src++;