SkColorSpaceXform bug fixes

(1) Clamp properly!

Finally came to this realization: clamping in the
store functions (after gamma encoding) is ridiculous.
It is impossible to know how to clamp premul values
to alpha when they are already gamma encoded.

I've moved the clamp out of the store function.
Whew, this actually makes the code look simpler.

And I expect this to fix some buggy images on Gold!

(2) Correctly handle the memcpy() case.

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:

Change-Id: I4870048105efcbecc70b4bd5f77c39537006363e
Reviewed-on: https://skia-review.googlesource.com/5389
Commit-Queue: Mike Klein <mtklein@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>

diff --git a/src/core/SkColorSpaceXform.cpp b/src/core/SkColorSpaceXform.cpp
index 450a643cfc6f8f017d39aa4142a2acfff3ecaceb..af119134841e47a78053590b6ff13a9b3b364d23 100644 (file)
--- a/src/core/SkColorSpaceXform.cpp
+++ b/src/core/SkColorSpaceXform.cpp
@@ -645,10 +645,6 @@ static AI void store_srgb(void* dst, const uint32_t* src, Sk4f& dr, Sk4f& dg, Sk
     dg = sk_linear_to_srgb_needs_trunc(dg);
     db = sk_linear_to_srgb_needs_trunc(db);
 
-    dr = sk_clamp_0_255(dr);
-    dg = sk_clamp_0_255(dg);
-    db = sk_clamp_0_255(db);
-
     Sk4i da = Sk4i::Load(src) & 0xFF000000;
 
     Sk4i rgba = (SkNx_cast<int>(dr) << kRShift)
@@ -662,7 +658,7 @@ template <Order kOrder>
 static AI void store_srgb_1(void* dst, const uint32_t* src,
                             Sk4f& rgba, const Sk4f&,
                             const uint8_t* const[3]) {
-    rgba = sk_clamp_0_255(sk_linear_to_srgb_needs_trunc(rgba));
+    rgba = sk_linear_to_srgb_needs_trunc(rgba);
 
     uint32_t tmp;
     SkNx_cast<uint8_t>(SkNx_cast<int32_t>(rgba)).store(&tmp);
@@ -693,10 +689,6 @@ static AI void store_2dot2(void* dst, const uint32_t* src, Sk4f& dr, Sk4f& dg, S
     dg = linear_to_2dot2(dg);
     db = linear_to_2dot2(db);
 
-    dr = sk_clamp_0_255(dr);
-    dg = sk_clamp_0_255(dg);
-    db = sk_clamp_0_255(db);
-
     Sk4i da = Sk4i::Load(src) & 0xFF000000;
 
     Sk4i rgba = (Sk4f_round(dr) << kRShift)
@@ -710,7 +702,7 @@ template <Order kOrder>
 static AI void store_2dot2_1(void* dst, const uint32_t* src,
                              Sk4f& rgba, const Sk4f&,
                              const uint8_t* const[3]) {
-    rgba = sk_clamp_0_255(linear_to_2dot2(rgba));
+    rgba = linear_to_2dot2(rgba);
 
     uint32_t tmp;
     SkNx_cast<uint8_t>(Sk4f_round(rgba)).store(&tmp);
@@ -727,9 +719,9 @@ static AI void store_linear(void* dst, const uint32_t* src, Sk4f& dr, Sk4f& dg,
                             const uint8_t* const[3]) {
     int kRShift, kGShift = 8, kBShift;
     set_rb_shifts(kOrder, &kRShift, &kBShift);
-    dr = sk_clamp_0_255(255.0f * dr);
-    dg = sk_clamp_0_255(255.0f * dg);
-    db = sk_clamp_0_255(255.0f * db);
+    dr = 255.0f * dr;
+    dg = 255.0f * dg;
+    db = 255.0f * db;
 
     Sk4i da = Sk4i::Load(src) & 0xFF000000;
 
@@ -744,7 +736,7 @@ template <Order kOrder>
 static AI void store_linear_1(void* dst, const uint32_t* src,
                               Sk4f& rgba, const Sk4f&,
                               const uint8_t* const[3]) {
-    rgba = sk_clamp_0_255(255.0f * rgba);
+    rgba = 255.0f * rgba;
 
     uint32_t tmp;
     SkNx_cast<uint8_t>(Sk4f_round(rgba)).store(&tmp);
@@ -811,9 +803,9 @@ static AI void store_generic(void* dst, const uint32_t* src, Sk4f& dr, Sk4f& dg,
                              const uint8_t* const dstTables[3]) {
     int kRShift, kGShift = 8, kBShift;
     set_rb_shifts(kOrder, &kRShift, &kBShift);
-    dr = Sk4f::Min(Sk4f::Max(1023.0f * dr, 0.0f), 1023.0f);
-    dg = Sk4f::Min(Sk4f::Max(1023.0f * dg, 0.0f), 1023.0f);
-    db = Sk4f::Min(Sk4f::Max(1023.0f * db, 0.0f), 1023.0f);
+    dr = 1023.0f * dr;
+    dg = 1023.0f * dg;
+    db = 1023.0f * db;
 
     Sk4i ir = Sk4f_round(dr);
     Sk4i ig = Sk4f_round(dg);
@@ -846,7 +838,7 @@ static AI void store_generic_1(void* dst, const uint32_t* src,
                                const uint8_t* const dstTables[3]) {
     int kRShift, kGShift = 8, kBShift;
     set_rb_shifts(kOrder, &kRShift, &kBShift);
-    rgba = Sk4f::Min(Sk4f::Max(1023.0f * rgba, 0.0f), 1023.0f);
+    rgba = 1023.0f * rgba;
 
     Sk4i indices = Sk4f_round(rgba);
 
@@ -990,6 +982,9 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
             break;
     }
 
+    static constexpr bool dstIs8888 = kF16_Linear_DstFormat != kDst &&
+                                      kF32_Linear_DstFormat != kDst;
+
     const uint32_t* src = (const uint32_t*) vsrc;
     Sk4f rXgXbX, rYgYbY, rZgZbZ, rTgTbT;
     load_matrix(matrix, rXgXbX, rYgYbY, rZgZbZ, rTgTbT);
@@ -1007,6 +1002,12 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
             if (kNone_ColorSpaceMatch == kCSM) {
                 transform_gamut(r, g, b, a, rXgXbX, rYgYbY, rZgZbZ, dr, dg, db, da);
                 translate_gamut(rTgTbT, dr, dg, db);
+
+                if (dstIs8888) {
+                    dr = sk_clamp_0_1(dr);
+                    dg = sk_clamp_0_1(dg);
+                    db = sk_clamp_0_1(db);
+                }
             } else {
                 dr = r;
                 dg = g;
@@ -1029,6 +1030,12 @@ static void color_xform_RGBA(void* dst, const void* vsrc, int len,
         if (kNone_ColorSpaceMatch == kCSM) {
             transform_gamut(r, g, b, a, rXgXbX, rYgYbY, rZgZbZ, dr, dg, db, da);
             translate_gamut(rTgTbT, dr, dg, db);
+
+            if (dstIs8888) {
+                dr = sk_clamp_0_1(dr);
+                dg = sk_clamp_0_1(dg);
+                db = sk_clamp_0_1(db);
+            }
         } else {
             dr = r;
             dg = g;
@@ -1171,26 +1178,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/core/SkSRGB.h b/src/core/SkSRGB.h
index c182f329de1e31a65128f3322987e4aa2e713cc1..cf41654149bd68892fcbdca59193fffc1165726b 100644 (file)
--- a/src/core/SkSRGB.h
+++ b/src/core/SkSRGB.h
@@ -29,6 +29,13 @@ static inline V sk_clamp_0_255(const V& x) {
     return V::Min(V::Max(x, 0.0f), 255.0f);
 }
 
+template <typename V>
+static inline V sk_clamp_0_1(const V& x) {
+    // The order of the arguments is important here.  We want to make sure that NaN
+    // clamps to zero.  Note that max(NaN, 0) = 0, while max(0, NaN) = NaN.
+    return V::Min(V::Max(x, 0.0f), 1.0f);
+}
+
 // [0.0f, 1.0f] -> [0.0f, 255.xf], for small x.  Correct after truncation.
 template <typename V>
 static inline V sk_linear_to_srgb_needs_trunc(const V& x) {