unroll srcover_1 for blending a single color

src/core/SkXfermode4f.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkPM4fPriv.h"
 #include "SkUtils.h"
 #include "SkXfermode.h"
@@ skipping 17 matching lines @@
 }
 
 template <DstType D> Sk4f load_dst(SkPMColor dstC) {
     return (D == kSRGB_Dst) ? Sk4f_fromS32(dstC) : Sk4f_fromL32(dstC);
 }
 
 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) {
     return Sk4f::Load(pm4.fVec) * Sk4f(255) + Sk4f(0.5f);
 }
 
 static void pm4f_to_linear_32(SkPMColor dst[], const SkPM4f src[], int count) {
     while (count >= 4) {
         src[0].assertIsUnit();
         src[1].assertIsUnit();
@@ skipping 221 matching lines @@
             Sk4f r4;
             if (a != 0xFF) {
                 s4 = scale_by_coverage(s4, a);
                 r4 = s4 + d4 * Sk4f(1 - get_alpha(s4));
             } else {
                 r4 = s4 + d4 * scale;
             }
             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);
+            }
         }
     }
 }
 
 const XferProcPair gProcs_SrcOver[] = {
     { srcover_1<kLinear_Dst>,   srcover_n<kLinear_Dst> },   // linear   alpha
     { src_1<kLinear_Dst>,       src_n<kLinear_Dst>     },   // linear   opaque [ we are src-mode ]
     { srcover_1<kSRGB_Dst>,     srcover_n<kSRGB_Dst>   },   // srgb     alpha
     { src_1<kSRGB_Dst>,         src_n<kSRGB_Dst>       },   // srgb     opaque [ we are src-mode ]
 };
@@ skipping 25 matching lines @@
 
 SkXfermode::PM4fProc1 SkXfermode::getPM4fProc1(uint32_t flags) const {
     Mode mode;
     return this->asMode(&mode) ? GetPM4fProc1(mode, flags) : xfer_pm4_proc_1;
 }
 
 SkXfermode::PM4fProcN SkXfermode::getPM4fProcN(uint32_t flags) const {
     Mode mode;
     return this->asMode(&mode) ? GetPM4fProcN(mode, flags) : xfer_pm4_proc_n;
 }