Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes

dm/DMSrcSink.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 "DMSrcSink.h"
 #include "SkAndroidCodec.h"
 #include "SkCodec.h"
@@ skipping 452 matching lines @@
                         codec->getScanlines(dstPtr, 1, bitmap.rowBytes());
                     }
                     break;
                 }
             }
 
             premultiply_if_necessary(bitmap);
             canvas->drawBitmap(bitmap, 0, 0);
             break;
         }
-        case kStripe_Mode: {
+        case kTile_Mode: {
+            const int width = decodeInfo.width();
             const int height = decodeInfo.height();
             // This value is chosen arbitrarily.  We exercise more cases by choosing a value that
             // does not align with image blocks.

[scroggo, 2016/02/22 15:55:03]

Given that our implementation changes a bit depending on whether we align with
an image block, it seems like it would be interesting to also test when
aligning.

[msarett, 2016/02/22 17:38:15]

Agreed.  Changing this value to one that will test both.

-            const int stripeHeight = 37;
-            const int numStripes = (height + stripeHeight - 1) / stripeHeight;
+            const int tileSize = 51;

[scroggo, 2016/02/22 15:55:03]

Why the change from 37 to 51?

[msarett, 2016/02/22 17:38:15]

The larger the value, the test runs.  however, I've decided to bring back
kStripe_Mode and use a different value for this test.

[scroggo, 2016/02/22 18:46:47]

I'm guessing you left out the word "faster"?

[msarett, 2016/02/22 19:07:43]

Haha yes.

+            const int tileWidth = (width + tileSize - 1) / tileSize;
+            const int tileHeight = (height + tileSize - 1) / tileSize;

[scroggo, 2016/02/22 15:55:03]

Why are we concerned with tiles in Y here? The old test messed with Y in order
to test scanlines out of order. I guess this sort of does this, since we'll
start top down, and then move back up to the top.

[msarett, 2016/02/22 17:38:16]

I guess we just use this to know how many times to loop?  I've changed this to
something that I think is more clear.

EDIT: Oh I get it!  Let's *only* test subsetting in the x-dimension in this
test.  I think this is fine.  I don't foresee any weird interactions between
jpeg_crop_scanlines() and jpeg_skip_scanlines().

Will make this test way faster as well.

 
-            // Decode odd stripes
-            if (SkCodec::kSuccess != codec->startScanlineDecode(decodeInfo, NULL, colorPtr,
-                                                                colorCountPtr)
-                    || SkCodec::kTopDown_SkScanlineOrder != codec->getScanlineOrder()) {
-                // This mode was designed to test the new skip scanlines API in libjpeg-turbo.
-                // Jpegs have kTopDown_SkScanlineOrder, and at this time, it is not interesting
-                // to run this test for image types that do not have this scanline ordering.
-                return Error::Nonfatal("Could not start top-down scanline decoder");
-            }
+            SkCodec::Options opts;
+            SkIRect subset;
+            for (int tileX = 0; tileX < tileWidth; tileX++) {
+                const int startX = tileX * tileSize;
+                subset = SkIRect::MakeXYWH(startX, 0, SkTMin(tileSize, width - startX), height);
+                opts.fSubset = &subset;
+                for (int tileY = 0; tileY < tileHeight; tileY++) {
+                    if (SkCodec::kSuccess != codec->startScanlineDecode(decodeInfo, &opts,
+                            colorPtr, colorCountPtr) ||
+                            SkCodec::kTopDown_SkScanlineOrder != codec->getScanlineOrder()) {

[scroggo, 2016/02/22 15:55:03]

This is now only used for JPEG, so it seems like we should never get a scanline
order that is not top down.

[msarett, 2016/02/22 17:38:16]

Yes, I've fixed this.

+                        if (0 == tileY && 0 == tileX) {
+                            return Error::Nonfatal("Could not start top-down scanline decoder");

[scroggo, 2016/02/22 15:55:03]

Is this not a bug? Why would startScanlineDecode fail that is okay? I could
imagine incomplete images (if we still test those in DM), although those could
fail past the first tile.

[msarett, 2016/02/22 17:38:15]

I believe that we used to need this when kInvalidConversion was an ok return
value.  Now that we've fixed that, I think this should be a fatal error.

I'll fix this elsewhere in a follow-up CL.

[scroggo, 2016/02/22 18:46:47]

sgtm

+                        }
+                        return "Could not restart scanline decoder.";
+                    }
 
-            for (int i = 0; i < numStripes; i += 2) {
-                // Skip a stripe
-                const int linesToSkip = SkTMin(stripeHeight, height - i * stripeHeight);
-                codec->skipScanlines(linesToSkip);
-
-                // Read a stripe
-                const int startY = (i + 1) * stripeHeight;
-                const int linesToRead = SkTMin(stripeHeight, height - startY);
-                if (linesToRead > 0) {
-                    codec->getScanlines(bitmap.getAddr(0, startY), linesToRead, bitmap.rowBytes());
+                    const int startY = tileY * tileSize;
+                    codec->skipScanlines(startY);
+                    const int linesToGet = SkTMin(tileSize, height - startY);
+                    codec->getScanlines(bitmap.getAddr(startX, startY), linesToGet,
+                            bitmap.rowBytes());
                 }
             }
 
-            // Decode even stripes
-            const SkCodec::Result startResult = codec->startScanlineDecode(decodeInfo, nullptr,
-                    colorPtr, colorCountPtr);
-            if (SkCodec::kSuccess != startResult) {
-                return "Failed to restart scanline decoder with same parameters.";
-            }
-            for (int i = 0; i < numStripes; i += 2) {
-                // Read a stripe
-                const int startY = i * stripeHeight;
-                const int linesToRead = SkTMin(stripeHeight, height - startY);
-                codec->getScanlines(bitmap.getAddr(0, startY), linesToRead, bitmap.rowBytes());
-
-                // Skip a stripe
-                const int linesToSkip = SkTMin(stripeHeight, height - (i + 1) * stripeHeight);
-                if (linesToSkip > 0) {
-                    codec->skipScanlines(linesToSkip);
-                }
-            }
             premultiply_if_necessary(bitmap);
             canvas->drawBitmap(bitmap, 0, 0);
             break;
         }
         case kSubset_Mode: {
             // Arbitrarily choose a divisor.
             int divisor = 2;
             // Total width/height of the image.
             const int W = codec->getInfo().width();
             const int H = codec->getInfo().height();
@@ skipping 876 matching lines @@
             skr.visit<void>(i, drawsAsSingletonPictures);
         }
         SkAutoTUnref<SkPicture> macroPic(macroRec.endRecordingAsPicture());
 
         canvas->drawPicture(macroPic);
         return check_against_reference(bitmap, src, fSink);
     });
 }
 
 }  // namespace DM