Update Subset benches to support interlacing and fix bugs

bench/subset/SubsetTranslateBench.cpp

Index: bench/subset/SubsetTranslateBench.cpp
diff --git a/bench/subset/SubsetTranslateBench.cpp b/bench/subset/SubsetTranslateBench.cpp
index 8f29ba8fa011f25ef74eee4eb09d2ebb09fcb73a..057916feba37b182570ecf8e647cb253fa623712 100644
--- a/bench/subset/SubsetTranslateBench.cpp
+++ b/bench/subset/SubsetTranslateBench.cpp
@@ -62,8 +62,10 @@ void SubsetTranslateBench::onDraw(int n, SkCanvas* canvas) {
         for (int count = 0; count < n; count++) {
             SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(fStream->duplicate()));
             const SkImageInfo info = codec->getInfo().makeColorType(fColorType);
-            SkAutoTDeleteArray<uint8_t> row(new uint8_t[info.minRowBytes()]);
-            codec->startScanlineDecode(info, nullptr, colors, &colorCount);
+            SkAutoTDeleteArray<uint8_t> row(nullptr);
+            if (codec->getScanlineOrder() == SkCodec::kTopDown_SkScanlineOrder) {
+                row.reset(new uint8_t[info.minRowBytes()]);
+            }
 
             SkBitmap bitmap;
             // Note that we use the same bitmap for all of the subsets.
@@ -71,20 +73,47 @@ void SubsetTranslateBench::onDraw(int n, SkCanvas* canvas) {
             SkImageInfo subsetInfo = info.makeWH(fSubsetWidth, fSubsetHeight);
             alloc_pixels(&bitmap, subsetInfo, colors, colorCount);
 
+            const uint32_t bpp = info.bytesPerPixel();
+
             for (int x = 0; x < info.width(); x += fSubsetWidth) {
                 for (int y = 0; y < info.height(); y += fSubsetHeight) {
+                    codec->startScanlineDecode(info, nullptr, colors, &colorCount);

[scroggo, 2015/10/06 20:36:04]

Here's the bug (unless I'm missing something?): without calling startScanlines
again, we're just attempting to read off the end of the image, which will return
some failure case (which we never read).

On that note, perhaps we should read the Results and assert them. (I also wonder
if we should make it so kNonrendering tests in nanobench still get a canvas, at
least when you use the flag -w to write the output images, so we can verify the
results somehow.)

Taking it a step further, maybe we ultimately don't need these tests - instead,
we should be using some version of SkBitmapRegionDecoderInterface.

[msarett, 2015/10/06 21:58:52]

Ugh this is an embarrassing bug.

Theoretically, we test correctness in dm, so we can test performance here
without worrying about checking correctness.  But I guess the code got
complicated enough that I made a mistake.  I'm not sure if that is a bad
oversight on my part or an indication that we need more ways of error checking
here.

Certainly asserting the results as a first step can't hurt.

I do think that the SkBitmapRegionDecoderInterface tests are more important at
this point...these will be tests we look at when replacing BRD.  But I think
these still have value, especially now that we need to redo them to make valid
conclusions.

[scroggo, 2015/10/07 19:50:58]

Done.
That is probably ultimately true, although we cannot look at those until we
settle on how to do subsets in SkScaledCodec (and of course, the hard part is
sampling combined with subsets).

In the meantime, I think this gives us something interesting to look at. (Also,
if we find the straightforward code here is better, but not so in SkScaledCodec,
we can know that we've probably done something wrong in SkScaledCodec...)

                     codec->skipScanlines(y);
+
                     const uint32_t currSubsetWidth =
                             x + (int) fSubsetWidth > info.width() ?
                             info.width() - x : fSubsetWidth;
                     const uint32_t currSubsetHeight =
                             y + (int) fSubsetHeight > info.height() ?
                             info.height() - y : fSubsetHeight;
-                    const uint32_t bpp = info.bytesPerPixel();
-                    for (uint32_t y = 0; y < currSubsetHeight; y++) {
-                        codec->getScanlines(row.get(), 1, 0);
-                        memcpy(bitmap.getAddr(0, y), row.get() + x * bpp,
-                                currSubsetWidth * bpp);
+
+                    switch (codec->getScanlineOrder()) {
+                        case SkCodec::kTopDown_SkScanlineOrder:
+                            for (uint32_t y = 0; y < currSubsetHeight; y++) {

[scroggo, 2015/10/06 20:36:04]

Again, I think this loop is unchanged.

[msarett, 2015/10/06 21:58:52]

Acknowledged.

+                                codec->getScanlines(row.get(), 1, 0);
+                                memcpy(bitmap.getAddr(0, y), row.get() + x * bpp,
+                                        currSubsetWidth * bpp);
+                            }
+                            break;
+                        case SkCodec::kNone_SkScanlineOrder: {
+                            // decode all scanlines that intersect the subset, and copy the subset
+                            // into the output.
+                            SkImageInfo stripeInfo = info.makeWH(info.width(), currSubsetHeight);
+                            SkBitmap stripeBm;
+                            alloc_pixels(&stripeBm, stripeInfo, colors, colorCount);
+
+                            codec->getScanlines(stripeBm.getPixels(), currSubsetHeight,
+                                    stripeBm.rowBytes());
+                            for (uint32_t subsetY = 0; subsetY < currSubsetHeight; subsetY++) {
+                                memcpy(bitmap.getAddr(0, subsetY), stripeBm.getAddr(x, subsetY),
+                                        currSubsetWidth * bpp);
+                            }
+                            break;
+                        }
+                        default:
+                            // We currently are only testing kTopDown and kNone, which are the only
+                            // two used by the subsets we care about. skbug.com/4428
+                            SkASSERT(false);
                     }
                 }
             }