onGetScanlines and onSkipScanlines for interlaced pngs

dm/DMSrcSink.cpp

Index: dm/DMSrcSink.cpp
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 4598a4894b7956ed681d6b199b569ee58572bf12..71c8b2f9270c6d83639f9b86d56cd3fc29bae622 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -140,7 +140,7 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
     }
 
     switch (fMode) {
-        case kNormal_Mode:
+        case kNormal_Mode: {
             switch (codec->getPixels(decodeInfo, bitmap.getPixels(), bitmap.rowBytes(), NULL,
                     colorPtr, colorCountPtr)) {
                 case SkImageGenerator::kSuccess:
@@ -156,23 +156,22 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
             }
             canvas->drawBitmap(bitmap, 0, 0);
             break;
+        }
         case kScanline_Mode: {
             SkScanlineDecoder* scanlineDecoder = codec->getScanlineDecoder(decodeInfo, NULL,
                     colorPtr, colorCountPtr);
             if (NULL == scanlineDecoder) {
                 return Error::Nonfatal("Cannot use scanline decoder for all images");
             }
-            for (int y = 0; y < decodeInfo.height(); ++y) {
-                const SkImageGenerator::Result result = scanlineDecoder->getScanlines(
-                        bitmap.getAddr(0, y), 1, 0);
-                switch (result) {
-                    case SkImageGenerator::kSuccess:
-                    case SkImageGenerator::kIncompleteInput:
-                        break;
-                    default:
-                        return SkStringPrintf("%s failed after %d scanlines with error message %d",
-                                              fPath.c_str(), y-1, (int) result);
-                }
+            const SkImageGenerator::Result result = scanlineDecoder->getScanlines(
+                    bitmap.getAddr(0, 0), decodeInfo.height(), decodeInfo.minRowBytes());

[scroggo, 2015/06/19 15:45:52]

This is a little tricky: in this case it happens to be correct to use
decodeInfo.minRowBytes(), but it's not guaranteed.

As the client of getScanlines, you're providing the pixel memory, so you know
what you used for the rowbytes. In this case, you're using memory allocated by
SkBitmap::tryAllocPixels (line 137), which initialized the bitmap with a
particular rowBytes, so you can ask the bitmap for its rowbytes:
bitmap.rowBytes().

[emmaleer, 2015/06/19 22:12:19]

Done.

+            switch (result) {
+                case SkImageGenerator::kSuccess:
+                case SkImageGenerator::kIncompleteInput:
+                    break;
+                default:
+                    return SkStringPrintf("%s failed with error message %d",
+                                          fPath.c_str(), (int) result);
             }
             canvas->drawBitmap(bitmap, 0, 0);
             break;
@@ -210,8 +209,9 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                 return SkStringPrintf("Image(%s) is too large (%d x %d)\n", fPath.c_str(),
                         largestSubsetDecodeInfo.width(), largestSubsetDecodeInfo.height());
             }
-            char* line = SkNEW_ARRAY(char, decodeInfo.minRowBytes());
-            SkAutoTDeleteArray<char> lineDeleter(line);
+            const int rowBytes = decodeInfo.minRowBytes();
+            char* buffer = SkNEW_ARRAY(char, h * rowBytes);

[msarett, 2015/06/19 13:35:54]

Can we allocate this buffer with some kind of maxSubsetHeight rather than the
height of the full image?

+            SkAutoTDeleteArray<char> lineDeleter(buffer);
             for (int col = 0; col < divisor; col++) {
                 //currentSubsetWidth may be larger than subsetWidth for rightmost subsets
                 const int currentSubsetWidth = (col + 1 == divisor) ?
@@ -250,21 +250,24 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                     bounds.setXYWH(0, 0, currentSubsetWidth, currentSubsetHeight);
                     SkAssertResult(largestSubsetBm.extractSubset(&subsetBm, bounds));
                     SkAutoLockPixels autlockSubsetBm(subsetBm, true);
+                    const SkImageGenerator::Result subsetResult =
+                            subsetScanlineDecoder->getScanlines(buffer, currentSubsetHeight, rowBytes);

[scroggo, 2015/06/19 15:45:52]

nit: I believe this exceeds 100 characters. We try to keep each line under 100.

[emmaleer, 2015/06/19 22:12:20]

Acknowledged.

+                    switch (subsetResult) {
+                        case SkImageGenerator::kSuccess:
+                        case SkImageGenerator::kIncompleteInput:
+                            break;
+                        default:
+                            return SkStringPrintf("%s failed after %d scanlines with error" 
+                                    "message %d", fPath.c_str(), y-1, (int) subsetResult);

[scroggo, 2015/06/19 15:45:52]

It probably is not useful to print the y value, since we do all the requested
rows at once.

[emmaleer, 2015/06/19 22:12:20]

Acknowledged.

+                    }
+                    const size_t bpp = decodeInfo.bytesPerPixel();
+                    //copy section of lines based on x value
+                    char* bufferRow = buffer; 
                     for (int subsetY = 0; subsetY < currentSubsetHeight; ++subsetY) {
-                        const SkImageGenerator::Result subsetResult =
-                                subsetScanlineDecoder->getScanlines(line, 1, 0);
-                        const size_t bpp = decodeInfo.bytesPerPixel();
-                        //copy section of line based on x value
-                        memcpy(subsetBm.getAddr(0, subsetY), line + x*bpp, currentSubsetWidth*bpp);
-                        switch (subsetResult) {
-                            case SkImageGenerator::kSuccess:
-                            case SkImageGenerator::kIncompleteInput:
-                                break;
-                            default:
-                                return SkStringPrintf("%s failed after %d scanlines with error" 
-                                        "message %d", fPath.c_str(), y-1, (int) subsetResult);
-                        }
+                        memcpy(subsetBm.getAddr(0, subsetY), bufferRow + x*bpp, currentSubsetWidth*bpp);

[scroggo, 2015/06/19 15:45:52]

nit: over 100 characters.

+                        bufferRow += rowBytes;

[msarett, 2015/06/19 13:35:54]

I have a theory that it is faster to copy from a row buffer than from an image
sized buffer (and it's also more memory efficient).  Obviously, this is not an
option got interlaced pngs and this is a correctness test, so what you have here
is perfect.

But this is something to keep in mind/test when we implement subset decodes in
BitmapRegionDecoder.

[scroggo, 2015/06/19 15:45:52]

Agreed. We should confirm that theory in order to decide the best way to do it.
If it in fact is faster for non-interlaced (it's definitely slower for
interlaced), we'll need to have an API that tells you whether the image is
interlaced. (Maybe it should not specify interlacing per se, but give some kind
of hint about the best way to use it.)

Could you add a comment explaining why we're doing all the lines at once, and
acknowledging that it may not always be the fastest way to do it (but this is a
correctness test so that's okay)?

[emmaleer, 2015/06/19 22:12:20]

I added a comment explaining why we decode this way.
I agree that if it's faster to decode non-interlaced line by line, then we will
need some way to determine which method to use, based on if the image is
interlaced or not.
Should we add a flag which specifies the best decoding strategy, either
line-by-line, or all lines at once?
Even if non-interlaced images aren't faster line by line, it might be good to
specify that interlaced images are much faster when decoding all lines at once.

                     }
+                    
                     canvas->drawBitmap(subsetBm, SkIntToScalar(x), SkIntToScalar(y));
                 }
             }
@@ -344,6 +347,7 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                 }
             }
             canvas->drawBitmap(bitmap, 0, 0);
+            break;
         }
     }
     return "";