unroll srcover_1 for blending a single color

src/core/SkXfermode4f.cpp

Index: src/core/SkXfermode4f.cpp
diff --git a/src/core/SkXfermode4f.cpp b/src/core/SkXfermode4f.cpp
index 0485a5e6edd4c0c65f884eae88b27beb8c9cd637..98b73b527aae5952c7e52ee8b6e472a139b2eeee 100644
--- a/src/core/SkXfermode4f.cpp
+++ b/src/core/SkXfermode4f.cpp
@@ -35,6 +35,16 @@ template <DstType D> uint32_t store_dst(const Sk4f& x4) {
     return (D == kSRGB_Dst) ? Sk4f_toS32(x4) : Sk4f_toL32(x4);
 }
 
+template <DstType D> Sk4f linear_to_dst_255(const Sk4f& l4) {

[mtklein, 2016/02/01 21:55:32]

Seems odd to template this on DstType if we're only calling it for D ==
kSRGB_Dst.

[reed1, 2016/02/02 14:04:39]

I had a bool, but it made reading profiles harder. This way the profile reads
(e.g.) srcover<kSRGB> instead of srcover<true>

[mtklein, 2016/02/02 14:26:06]

No no, I mean, it's weird to template at all.  Only the D == kSRGB_Dst path is
being called.

[reed1, 2016/02/02 14:45:58]

Ah. It *was* being used more generically, until I did more unrolling and
specializing. I think I kept it as a preemptive attempt to (eventually) create a
std vocabulary of transformers for this space:
- loading from either linear or srgb bytes into linear floats, w/ or w/o
normalizing to 0...1
- storing linear floats to either linear or srgb bytes, w/ or w/o
scaling+rounding

If it is hard to read, I can remove and re-add later if it comes up again.

+    Sk4f x4;
+    if (D == kSRGB_Dst) {
+        x4 = linear_to_srgb(l4);
+    } else {
+        x4 = l4;
+    }
+    return x4 * Sk4f(255) + Sk4f(0.5f);
+}
+
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
 static Sk4f scale_255_round(const SkPM4f& pm4) {
@@ -276,10 +286,35 @@ template <DstType D> void srcover_1(const SkXfermode::PM4fState& state, uint32_t
             dst[i] = store_dst<D>(r4);
         }
     } else {
+        if (D == kLinear_Dst) {
+            s4 = s4 * Sk4f(255) + Sk4f(0.5f);

[mtklein, 2016/02/01 21:55:32]

I find it really confusing to mutate values conditionally, especially when you
need to remember that these changes happened in two places below, but not in two
other places.

It'd be easier to follow this if we give the new values new names, then use them
only in the kLinear case:

Sk4f src4_255 = s4 * Sk4f(255) + Sk4f(0.5f);
Sk4f scale_255 = scale * Sk4f(255);

...
if (D == kLinear_Dst) {
    Sk4f_ToBytes(..., src4_255 + d0 * scale_255, ... );
} else {
    Sk4f_ToBytes(..., linear_to_sRGB_4f(s4 + d0 * scale), ...);
}

The compiler will eliminate src4_255 and scale_255 completely in the sRGB case,
so there's no need to be conditional around their computation unless it's so
smart it starts whining that they're unused.

[reed1, 2016/02/02 14:04:39]

Agreed. I will give them meaningful names

+            scale = scale * Sk4f(255);
+        }

[mtklein, 2016/02/01 21:58:44]

I'm also curious to see if this hoisting can be done by the compiler if we just
give it some parenthesis hints:

Sk4f_ToBytes(...,
    (s4 * Sk4f(255) + Sk4f(0.5f)) + d0 * (scale * Sk4f(255)),
    (s4 * Sk4f(255) + Sk4f(0.5f)) + d1 * (scale * Sk4f(255)),
    (s4 * Sk4f(255) + Sk4f(0.5f)) + d2 * (scale * Sk4f(255)),
    (s4 * Sk4f(255) + Sk4f(0.5f)) + d3 * (scale * Sk4f(255)));
    

+        while (count >= 4) {
+            Sk4f d0 = load_dst<D>(dst[0]);
+            Sk4f d1 = load_dst<D>(dst[1]);
+            Sk4f d2 = load_dst<D>(dst[2]);
+            Sk4f d3 = load_dst<D>(dst[3]);
+            if (D == kLinear_Dst) {
+                Sk4f_ToBytes((uint8_t*)dst,
+                             s4 + d0 * scale, s4 + d1 * scale, s4 + d2 * scale, s4 + d3 * scale);

[mtklein, 2016/02/01 21:55:32]

Might be nice to line-break after each comma here too so it's also 5 lines like
the one below.

[reed1, 2016/02/02 14:04:39]

Agreed.

+            } else {
+                Sk4f_ToBytes((uint8_t*)dst,
+                             linear_to_dst_255<D>(s4 + d0 * scale),
+                             linear_to_dst_255<D>(s4 + d1 * scale),
+                             linear_to_dst_255<D>(s4 + d2 * scale),
+                             linear_to_dst_255<D>(s4 + d3 * scale));
+            }
+            dst += 4;
+            count -= 4;
+        }
         for (int i = 0; i < count; ++i) {
-            Sk4f d4 = load_dst<D>(dst[i]);
-            Sk4f r4 = s4 + d4 * scale;
-            dst[i] = store_dst<D>(r4);
+            Sk4f r4 = s4 + load_dst<D>(dst[i]) * scale;
+            if (D == kLinear_Dst) {
+                dst[i] = to_4b(r4);
+            } else {
+                dst[i] = store_dst<D>(r4);
+            }
         }
     }
 }