SkColorSpaceXform bug fixes attempt 2

(1) Clamping

If we're going to clamp (8888 outputs), we need to clamp properly
to alpha (not 1) when we premultiply.  This fix is made in
SkColorSpaceXform_XYZ.

An alternative fix would move all clamping out of the store
functions, to before the gamma encoding.  This generally makes sense,
but the "to 2.2 conversion" may introduce NaNs and always needs a
clamp.  So another fix is to just have an extra clamp in the store 2.2
function.  Since we have two pipelines, let's try this one in
SkColorSpaceXform_Pipeline :).

(2) Correctly handle the memcpy() case.

This is not changed from a previous (reverted) CL.

Looks like this only ever worked for RGBA inputs,
never got updated when we added BGRA inputs.

This probably flew under the radar because the
clients are smart enough to avoid performing a
color xform altogether when the color spaces
match.

BUG=skia:

CQ_INCLUDE_TRYBOTS=skia.primary:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD

Change-Id: I0b59239d2488ce9fdbe11efbd96567e420bb9813
Reviewed-on: https://skia-review.googlesource.com/5464
Commit-Queue: Matt Sarett <msarett@google.com>
Reviewed-by: Mike Klein <mtklein@chromium.org>

diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp
index 009b79e186dad613c0301babaa3e341074df08dc..a6d69caf592a53e95eac61115fc67a0e90db1b13 100644 (file)
--- a/src/core/SkColorSpaceXform.cpp
+++ b/src/core/SkColorSpaceXform.cpp
@@ -628,13 +628,23 @@ static AI void translate_gamut_1(const Sk4f& rTgTbT, Sk4f& rgba) {
     rgba = rgba + rTgTbT;
 }
 
-static AI void premultiply(Sk4f& dr, Sk4f& dg, Sk4f& db, const Sk4f& da) {
+static AI void premultiply(Sk4f& dr, Sk4f& dg, Sk4f& db, const Sk4f& da, bool kClamp) {
+    if (kClamp) {
+        dr = Sk4f::Max(dr, 1.0f);
+        dg = Sk4f::Max(dg, 1.0f);
+        db = Sk4f::Max(db, 1.0f);
+    }
+
     dr = da * dr;
     dg = da * dg;
     db = da * db;
 }
 
-static AI void premultiply_1(const Sk4f& a, Sk4f& rgba) {
+static AI void premultiply_1(const Sk4f& a, Sk4f& rgba, bool kClamp) {
+    if (kClamp) {
+        rgba = Sk4f::Max(rgba, 1.0f);
+    }
+
     rgba = a * rgba;
 }
 
@@ -892,12 +902,12 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
                              const uint8_t* const dstTables[3]) {
     LoadFn load;
     Load1Fn load_1;
-    static constexpr bool loadAlpha = (kPremul_SkAlphaType == kAlphaType) ||
-                                      (kF16_Linear_DstFormat == kDst) ||
-                                      (kF32_Linear_DstFormat == kDst);
+    const bool kLoadAlpha = (kPremul_SkAlphaType == kAlphaType) ||
+                            (kF16_Linear_DstFormat == kDst) ||
+                            (kF32_Linear_DstFormat == kDst);
     switch (kSrc) {
         case kRGBA_8888_Linear_SrcFormat:
-            if (loadAlpha) {
+            if (kLoadAlpha) {
                 load = load_rgba_linear<kRGBA_Order>;
                 load_1 = load_rgba_linear_1<kRGBA_Order>;
             } else {
@@ -906,7 +916,7 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
             }
             break;
         case kRGBA_8888_Table_SrcFormat:
-            if (loadAlpha) {
+            if (kLoadAlpha) {
                 load = load_rgba_from_tables<kRGBA_Order>;
                 load_1 = load_rgba_from_tables_1<kRGBA_Order>;
             } else {
@@ -915,7 +925,7 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
             }
             break;
         case kBGRA_8888_Linear_SrcFormat:
-            if (loadAlpha) {
+            if (kLoadAlpha) {
                 load = load_rgba_linear<kBGRA_Order>;
                 load_1 = load_rgba_linear_1<kBGRA_Order>;
             } else {
@@ -924,7 +934,7 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
             }
             break;
         case kBGRA_8888_Table_SrcFormat:
-            if (loadAlpha) {
+            if (kLoadAlpha) {
                 load = load_rgba_from_tables<kBGRA_Order>;
                 load_1 = load_rgba_from_tables_1<kBGRA_Order>;
             } else {
@@ -992,6 +1002,14 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
             break;
     }
 
+    // We always clamp before converting to 8888 outputs (because we have to).
+    // In these cases, we also need a clamp before the premul step to make sure
+    // R, G, B are not logically greater than A.
+    const bool kClamp = kRGBA_8888_Linear_DstFormat == kDst ||
+                        kRGBA_8888_SRGB_DstFormat == kDst ||
+                        kRGBA_8888_2Dot2_DstFormat == kDst ||
+                        kRGBA_8888_Table_DstFormat == kDst;
+
     const uint32_t* src = (const uint32_t*) vsrc;
     Sk4f rXgXbX, rYgYbY, rZgZbZ, rTgTbT;
     load_matrix(matrix, rXgXbX, rYgYbY, rZgZbZ, rTgTbT);
@@ -1017,7 +1035,7 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
             }
 
             if (kPremul_SkAlphaType == kAlphaType) {
-                premultiply(dr, dg, db, da);
+                premultiply(dr, dg, db, da, kClamp);
             }
 
             load(src, r, g, b, a, srcTables);
@@ -1039,7 +1057,7 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
         }
 
         if (kPremul_SkAlphaType == kAlphaType) {
-            premultiply(dr, dg, db, da);
+            premultiply(dr, dg, db, da, kClamp);
         }
 
         store(dst, src - 4, dr, dg, db, da, dstTables);
@@ -1059,7 +1077,7 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
         }
 
         if (kPremul_SkAlphaType == kAlphaType) {
-            premultiply_1(a, rgba);
+            premultiply_1(a, rgba, kClamp);
         }
 
         store_1(dst, src, rgba, a, dstTables);
@@ -1173,26 +1191,23 @@ bool SkColorSpaceXform_XYZ<kSrc, kDst, kCSM>
           int len, SkAlphaType alphaType) const
 {
     if (kFull_ColorSpaceMatch == kCSM) {
-        switch (alphaType) {
-            case kPremul_SkAlphaType:
-                // We can't skip the xform since we need to perform a premultiply in the
-                // linear space.
-                break;
-            default:
-                switch (dstColorFormat) {
-                    case kRGBA_8888_ColorFormat:
-                        memcpy(dst, src, len * sizeof(uint32_t));
-                        return true;
-                    case kBGRA_8888_ColorFormat:
-                        SkOpts::RGBA_to_BGRA((uint32_t*) dst, src, len);
-                        return true;
-                    case kRGBA_F16_ColorFormat:
-                    case kRGBA_F32_ColorFormat:
-                        // There's still work to do to xform to linear floats.
-                        break;
-                    default:
-                        return false;
-                }
+        if (kPremul_SkAlphaType != alphaType) {
+            if ((kRGBA_8888_ColorFormat == dstColorFormat &&
+                 kRGBA_8888_ColorFormat == srcColorFormat) ||
+                (kBGRA_8888_ColorFormat == dstColorFormat &&
+                 kBGRA_8888_ColorFormat == srcColorFormat))
+            {
+                memcpy(dst, src, len * sizeof(uint32_t));
+                return true;
+            }
+            if ((kRGBA_8888_ColorFormat == dstColorFormat &&
+                 kBGRA_8888_ColorFormat == srcColorFormat) ||
+                (kBGRA_8888_ColorFormat == dstColorFormat &&
+                 kRGBA_8888_ColorFormat == srcColorFormat))
+            {
+                SkOpts::RGBA_to_BGRA((uint32_t*) dst, src, len);
+                return true;
+            }
         }
     }
 

diff --git a/src/opts/SkRasterPipeline_opts.h b/src/opts/SkRasterPipeline_opts.h
index d7f05a92d99115342a62e27f07bd018c97d3d692..962ef1568f5dfee99eb309b14c4d3ad385d23c01 100644 (file)
--- a/src/opts/SkRasterPipeline_opts.h
+++ b/src/opts/SkRasterPipeline_opts.h
@@ -349,7 +349,7 @@ STAGE(to_2dot2) {
              x64 = x32.rsqrt();                          // x^(+1/64)
 
         // 29 = 32 - 2 - 1
-        return x2.invert() * x32 * x64.invert();
+        return SkNf::Max(x2.invert() * x32 * x64.invert(), 0.0f); // Watch out for NaN.
     };
 
     r = to_2dot2(r);