SkPDF: Implement Paeth predictor for images.

src/pdf/SkPngPredictors.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 "SkPngPredictors.h"
+#include "SkTypes.h"
+
+namespace {
+enum EncodeOrDecode {

[mtklein, 2016/02/19 21:53:07]

Please remove the dead kDecode path and all related cruft, and please do not
send me code reviews with dead code.

+    kEncode,
+    kDecode,
+};
+}
+
+// If encoding, prev == out is okay.
+// If decoding, scanline == out is okay.
+// prev can be null.
+template<int COMPONENT_COUNT, EncodeOrDecode ENCODE_OR_DECODE>

[mtklein, 2016/02/19 21:53:07]

ALL_UPPPER_CASE_IS_FOR_MACROS_DUDE

This is generally called bpp in libpng, for good reason... it's not the
component count.  You can have, e.g. bpp = 8 with 4 components.

+static void paeth(int width, const uint8_t* prev, const uint8_t* scanline, uint8_t* out) {
+    static_assert(COMPONENT_COUNT > 0, "");
+    // a = left, b = above, c = upper left
+    if (prev) {
+        uint8_t a[COMPONENT_COUNT]{}, c[COMPONENT_COUNT]{};

[mtklein, 2016/02/19 21:53:07]

I think this would be easier to follow byte-by-byte instead of pixel-by-pixel. 
I'm finding it hard to track the state of a and c.

+        while (width-- > 0) {
+            for (int cmp = 0; cmp < COMPONENT_COUNT; ++cmp) {
+                uint8_t b = *prev++;
+                uint8_t x = *scanline++;
+                int pa = SkTAbs<int>(b - c[cmp]);
+                int pb = SkTAbs<int>(a[cmp] - c[cmp]);
+                int pc = SkTAbs<int>((b - c[cmp]) + (a[cmp] - c[cmp]));
+                int smallest = SkTMin(pc, SkTMin(pa, pb));

[mtklein, 2016/02/19 21:53:07]

For serial code, this is faster and simpler like this:

uint8_t pred = a;
if (pb < pa) { pred = b; pa = pb; }
if (pc < pa) { pred = c;          }

+                uint8_t pred = smallest == pa ? a[cmp] : (smallest == pb ? b : c[cmp]);
+                static_assert(ENCODE_OR_DECODE == kEncode || ENCODE_OR_DECODE == kDecode, "");
+                if (ENCODE_OR_DECODE == kEncode) {
+                    *out++ = x - pred;
+                } else {
+                    *out++ = x + pred;
+                }
+                a[cmp] = x;
+                c[cmp] = b;
+            }
+        }
+    } else {

[mtklein, 2016/02/19 21:53:07]

Let's split this else case off as 
  static void sub(...)
and call it from SkPaethEncode{1,3} instead of having to have paeth() think
about it.

+        uint8_t a[COMPONENT_COUNT]{};
+        while (width-- > 0) {
+            for (int cmp = 0; cmp < COMPONENT_COUNT; ++cmp) {
+                uint8_t x = *scanline++;
+                *out++ = x - a[cmp];

[mtklein, 2016/02/19 21:53:07]

This is why you should not land dead code.

+                a[cmp] = x;
+            }
+        }
+    }
+}
+
+void SkPaethEncode3(int width, const uint8_t* prev, const uint8_t* scanline, uint8_t* out) {
+    return paeth<3, kEncode>(width, prev, scanline, out);
+}
+
+void SkPaethEncode1(int width, const uint8_t* prev, const uint8_t* scanline, uint8_t* out) {
+    return paeth<1, kEncode>(width, prev, scanline, out);
+}