Swizzler changes Index8 and 565

tests/SwizzlerTest.cpp

+/*
+ * Copyright 2015 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkSwizzler.h"
+#include "Test.h"
+
+static void check_fill(skiatest::Reporter* r,
+                       SkColorType colorType,
+                       uint32_t width,
+                       uint32_t height,
+                       uint32_t row,

[scroggo, 2015/04/08 17:21:27]

nit: startRow?

[msarett, 2015/04/08 19:35:40]

Done.

+                       size_t rowBytes,
+                       size_t totalBytes,
+                       uint32_t input,

[scroggo, 2015/04/08 17:21:27]

Maybe rename this to colorOrIndex? (Same for SkSwizzler::Fill). It provides a
little more context than just "input".

[msarett, 2015/04/08 19:35:40]

Done.

+                       SkPMColor* colorTable) {
+
+    // Create fake image data where every bytes has a value of 0xFF

[scroggo, 2015/04/08 17:21:27]

nit: byte*

[msarett, 2015/04/08 19:35:40]

Done.

+    SkAutoTDelete<uint8_t> imageData((uint8_t*) sk_malloc_throw(totalBytes));
+    memset(imageData.get(), 0xFF, totalBytes);
+
+    // Create fake image info (alpha type does not matter)
+    SkImageInfo imageInfo = SkImageInfo::Make(width, height, colorType, kUnknown_SkAlphaType);
+
+    // Fill image with zeros starting at the indicated row
+    SkSwizzler::Fill(imageData.get(), imageInfo, rowBytes, row, input, colorTable);
+
+    // Ensure that the filled pixels are zero
+    // The bots should catch any memory corruption
+    for (uint32_t y = row; y < height; y++) {
+        uint8_t* indexPtr = imageData.get() + y * rowBytes;
+        uint32_t* colorPtr = (uint32_t*) indexPtr;
+        for (uint32_t x = 0; x < width; x++) {
+            if (kIndex_8_SkColorType == colorType) {
+                REPORTER_ASSERT(r, 0 == indexPtr[x]);
+            } else {
+                REPORTER_ASSERT(r, 0 == colorPtr[x]);
+            }
+        }
+    }
+} 
+
+// Each of these tests provide valid, but sometimes strange, values of width, height, and rowBytes.
+// We ensure that Fill() works properly and does not corrupt memory for all of these inputs.
+DEF_TEST(SwizzlerFill, r) {
+    
+    // Fill using an index into a color table
+    SkPMColor colorTable[256];
+    colorTable[0] = 0xFFFFFFFF;

[scroggo, 2015/04/08 17:21:27]

Why did you choose these colors? It seems like since you memset every byte to
0xFF, Filling with this color afterwards will not tell us whether the Fill did
the right thing?

I think an interesting test might be to use a color where all four bytes are
different. That way if we accidentally Fill off by a byte, the test can catch
that bug.

[msarett, 2015/04/08 19:35:40]

The tests for "an index to a color" always use 1 as the input index, so we
always fill with zeros, which we can detect.  The other entry to the color table
was created so that if we fill with the wrong entry, the test fails.

However, I would agree that filling with four different bytes would make for a
more interesting test.  I will implement this.

+    colorTable[1] = 0;

[scroggo, 2015/04/08 17:21:27]

Do we ever use values outside of the first two entries? If not, why did we
create colorTable with 256 entries?

[msarett, 2015/04/08 19:35:40]

I thought it made sense because this is the most likely size of the color table
when we use Fill() in real code.  But as you noted, the size does not affect the
test and we might as well save space.

[scroggo, 2015/04/08 20:37:37]

In this case, my concern is more about clarity than saving space. Since we are
using a non-intuitive value, it makes me wonder why. e.g. Do we have a
dependency on having 256 colors?

[msarett, 2015/04/09 13:02:47]

Makes sense, I'm in agreement.

+    // Zero dimensional cases
+    check_fill(r, kN32_SkColorType, 0, 1, 0, 0, 0, 1, colorTable);

[scroggo, 2015/04/08 17:21:27]

I'm having trouble following all these cases with so many parameters.

One thing I could imagine doing is using for loops. This would reduce the code
size, and potentially make it look a little clearer:

for....
  for....
    check_fill(r, colorType, w, h, row, rb, totalBytes, colorOrIndex,
colorTable);

(Maybe we want different loops, since it looks like not all the values get
crossed with all other values.)

[msarett, 2015/04/08 19:35:40]

I agree that this benefits from better organization and being easier to follow.

+    check_fill(r, kN32_SkColorType, 0, 1, 0, 10, 0, 1, colorTable);

[scroggo, 2015/04/08 17:21:27]

Do we ever have a case where the dst is not four byte aligned? Maybe that is
covered by using a row other than zero with an odd rowBytes? It would be nice to
call out that case with a comment.

[msarett, 2015/04/08 19:35:40]

This is implicitly covered by odd numbers of row bytes since the next row will
not be four byte aligned.  Regardless of the start row, Fill() will see this
issue because it iterates over rows.

However, I agree it is better to specifically design test cases for this (since
the implementation of Fill() may change) and to also not depend on a non-zero
start row.

+    // Cases with different row padding
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 40, 400, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 41, 410, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 42, 420, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 43, 430, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 44, 440, 1, colorTable);
+    // Cases that neglect to pad the bottom row
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 41, 409, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 42, 418, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 43, 427, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 44, 436, 1, colorTable);
+    // Cases where we do not fill all of the rows
+    check_fill(r, kN32_SkColorType, 10, 10, 1, 40, 400, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 2, 41, 410, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 4, 42, 420, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 6, 43, 430, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 7, 44, 440, 1, colorTable);
+    check_fill(r, kN32_SkColorType, 10, 10, 9, 41, 409, 1, colorTable);
+
+    // Fill using an index
+    // Zero dimensional cases
+    check_fill(r, kIndex_8_SkColorType, 0, 1, 0, 0, 0, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 0, 1, 0, 10, 0, 0, NULL);
+    // Cases with different row padding
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 10, 100, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 11, 110, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 12, 120, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 13, 130, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 14, 140, 0, NULL);
+    // Cases that neglect to pad the bottom row
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 11, 109, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 12, 118, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 13, 127, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 0, 14, 136, 0, NULL);
+    // Cases where we do not fill all of the rows
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 1, 10, 100, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 2, 11, 110, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 4, 12, 120, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 6, 13, 130, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 7, 14, 140, 0, NULL);
+    check_fill(r, kIndex_8_SkColorType, 10, 10, 9, 11, 109, 0, NULL);
+
+    // Fill using a color
+    // Zero dimensional cases
+    check_fill(r, kN32_SkColorType, 0, 1, 0, 0, 0, 0, NULL);
+    check_fill(r, kN32_SkColorType, 0, 1, 0, 10, 0, 0, NULL);
+    // Cases with different row padding
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 40, 400, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 41, 410, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 42, 420, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 43, 430, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 44, 440, 0, NULL);
+    // Cases that neglect to pad the bottom row
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 41, 409, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 42, 418, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 43, 427, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 0, 44, 436, 0, NULL);
+    // Cases where we do not fill all of the rows
+    check_fill(r, kN32_SkColorType, 10, 10, 1, 40, 400, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 2, 41, 410, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 4, 42, 420, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 6, 43, 430, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 7, 44, 440, 0, NULL);
+    check_fill(r, kN32_SkColorType, 10, 10, 9, 41, 409, 0, NULL);
+}