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 startRow,
+                       size_t rowBytes,
+                       size_t totalBytes,
+                       uint32_t offset,
+                       uint32_t colorOrIndex,
+                       SkPMColor* colorTable) {
+
+    // Create fake image data where every byte has a value of 0
+    SkAutoTDelete<uint8_t> storage((uint8_t*) sk_malloc_throw(totalBytes));
+    memset(storage.get(), 0, totalBytes);
+    uint8_t* imageData = storage.get() + offset;
+
+    // Create fake image info (alpha type does not matter)
+    SkImageInfo imageInfo = SkImageInfo::Make(width, height, colorType, kUnknown_SkAlphaType);
+
+    // Fill image with the fill value starting at the indicated row
+    SkSwizzler::Fill(imageData, imageInfo, rowBytes, startRow, colorOrIndex, colorTable);
+
+    // Ensure that the pixels are filled properly
+    // The bots should catch any memory corruption
+    uint8_t* indexPtr = imageData + startRow * rowBytes;
+    uint32_t* colorPtr = (uint32_t*) indexPtr;
+    for (uint32_t y = startRow; y < height; y++) {
+        for (uint32_t x = 0; x < width; x++) {
+            if (kIndex_8_SkColorType == colorType) {
+                REPORTER_ASSERT(r, 0x1 == indexPtr[x]);

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

nit: make these constants?

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

Done.

+            } else {
+                REPORTER_ASSERT(r, 0x22334455 == colorPtr[x]);
+            }
+        }
+        indexPtr += rowBytes;
+        colorPtr = (uint32_t*) indexPtr;
+    }
+} 
+
+// Test Fill() with different combinations of dimensions, alignment, and padding
+DEF_TEST(SwizzlerFill, r) {
+    // These are the values we will look for to indicate that the fill was successful
+    uint8_t fillIndex = 0x1;
+    uint32_t fillColor = 0x22334455;
+    
+    // Set up a color table
+    SkPMColor colorTable[fillIndex + 1];
+    memset(colorTable, 0, sizeof(SkPMColor) * (fillIndex + 1));

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

This seems extra complicated? Why not:

colorTable[0] = 0?

Is this in case we decide to use a different value for fillIndex?

I also see that we never use colorTable[0]. Are we testing to make sure we
didn't accidentally get it?

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

Yeah, essentially it seemed weird to me to test with a color table that only has
one color.  And also, I didn't want to test with the fillIndex being 0 since we
initialize the memory to 0.

I would agree that it is overcomplicated.  I think maybe its best to leave the
color table uninitialized except for the fill index?  The bots should catch it
if we fill with uninitialized memory anyway?

[scroggo, 2015/04/09 14:53:48]

Sgtm. Please add a comment that explains why you're leaving it uninitialized.

+    colorTable[fillIndex] = fillColor;
+
+    // Test on an invalid width and representative widths
+    const uint32_t widths[] = { 0, 10, 50 };
+
+    // In order to call Fill(), there must be at least one row to fill
+    // Test on the smallest possible height and representative heights
+    const uint32_t heights[] = { 1, 5, 10 };
+
+    // Test on interesting possibilities for row padding
+    const uint32_t paddings[] = { 0, 1, 2, 3, 4 };
+
+    // Iterate over test dimensions
+    for (uint32_t w = 0; w < SK_ARRAY_COUNT(widths); w++) {

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

I know I led you astray when I suggested this before, but I think now that we
only support C++11 builds, we can use the range syntax:

for (uint32_t w : widths)

etc.

Which I think will make this code a lot clearer.

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

Excellent!

+        for (uint32_t h = 0; h < SK_ARRAY_COUNT(heights); h++) {
+            for (uint32_t p = 0; p < SK_ARRAY_COUNT(paddings); p++) {
+
+                // Calculate row bytes
+                uint32_t colorRowBytes = sizeof(SkPMColor) * widths[w] + paddings[p];

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

Same as:

 SkColorTypeBytesPerPixel(kN32) + paddings[p]

Not sure if that's clearer.

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

I think it is a little clearer!

+                uint32_t indexRowBytes = widths[w] + paddings[p];
+
+                // If there is padding, we can invent an offset to change the memory alignment
+                for (uint32_t offset = p; offset <= p; offset++) {

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

Wait, isn't this just a loop that only executes once?

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

Yes this is a bug.  I intended offset to go from 0 to padding.

+
+                    // Calculate total bytes
+                    // The last row may not be padded.  We will always assume that it is in fact
+                    // not padded and test on the smallest possible value for total bytes.
+                    uint32_t colorTotalBytes = colorRowBytes * heights[h] - paddings[p] + offset;

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

It seems like this is

SkImageInfo colorInfo = SkImageInfo::MakeN32(w, h);

colorTotalBytes = colorInfo.getSafeSize(colorRowBytes) + offset;

Not sure if that's better, but it puts things into Skia terms.

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

I think it is better.  I need to be better at being aware of Skia calls and
using them.

+                    uint32_t indexTotalBytes = indexRowBytes * heights[h] - paddings[p] + offset;
+
+                    // Test all possible start rows
+                    for (uint32_t startRow = 0; startRow < heights[h]; startRow++) {
+
+                        // Fill with an index that we use to look up a color
+                        check_fill(r, kN32_SkColorType, widths[w], heights[h], startRow,
+                                colorRowBytes, colorTotalBytes, offset, fillIndex,
+                                colorTable);
+
+                        // Fill with a color
+                        check_fill(r, kN32_SkColorType, widths[w], heights[h], startRow,
+                                colorRowBytes, colorTotalBytes, offset, fillColor,
+                                NULL);
+
+                        // Fill with an index
+                        check_fill(r, kIndex_8_SkColorType, widths[w], heights[h], startRow,
+                                indexRowBytes, indexTotalBytes, offset, fillIndex,
+                                NULL);
+                    }
+                }
+            }
+        }
+    }
+}