onGetScanlines and onSkipScanlines for interlaced pngs

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 "SamplePipeControllers.h"
 #include "SkCodec.h"
@@ skipping 122 matching lines @@
         decodeInfo = decodeInfo.makeAlphaType(kPremul_SkAlphaType);
     }
 
     SkBitmap bitmap;
     if (!bitmap.tryAllocPixels(decodeInfo, NULL, colorTable.get())) {
         return SkStringPrintf("Image(%s) is too large (%d x %d)\n", fPath.c_str(),
                               decodeInfo.width(), decodeInfo.height());
     }
 
     switch (fMode) {
-        case kNormal_Mode:
+        case kNormal_Mode: {
             switch (codec->getPixels(decodeInfo, bitmap.getPixels(), bitmap.rowBytes(), NULL,
                     colorPtr, colorCountPtr)) {
                 case SkImageGenerator::kSuccess:
                     // We consider incomplete to be valid, since we should still decode what is
                     // available.
                 case SkImageGenerator::kIncompleteInput:
                     break;
                 case SkImageGenerator::kInvalidConversion:
                     return Error::Nonfatal("Incompatible colortype conversion");
                 default:
                     // Everything else is considered a failure.
                     return SkStringPrintf("Couldn't getPixels %s.", fPath.c_str());
             }
             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;
         }
         case kScanline_Subset_Mode: {
             //this mode decodes the image in divisor*divisor subsets, using a scanline decoder
             const int divisor = 2;
             const int w = decodeInfo.width();
             const int h = decodeInfo.height();
             if (w*h == 1) {
@@ skipping 17 matching lines @@
             * subsetBm's size is determined based on the current subset and may be larger for end
             * subsets.
             */
             SkImageInfo largestSubsetDecodeInfo =
                     decodeInfo.makeWH(subsetWidth + extraX, subsetHeight + extraY);
             SkBitmap largestSubsetBm;
             if (!largestSubsetBm.tryAllocPixels(largestSubsetDecodeInfo, NULL, colorTable.get())) {
                 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) ?
                         subsetWidth + extraX : subsetWidth;
                 const int x = col * subsetWidth;
                 for (int row = 0; row < divisor; row++) {
                     //currentSubsetHeight may be larger than subsetHeight for bottom subsets
                     const int currentSubsetHeight = (row + 1 == divisor) ?
                             subsetHeight + extraY : subsetHeight;
                     const int y = row * subsetHeight;
@@ skipping 18 matching lines @@
                         default:
                             return SkStringPrintf("%s failed after attempting to skip %d scanlines"
                                     "with error message %d", fPath.c_str(), y, (int) skipResult);
                     }
                     //create and set size of subsetBm
                     SkBitmap subsetBm;
                     SkIRect bounds = SkIRect::MakeWH(subsetWidth, subsetHeight);
                     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));
                 }
             }
             break;
         }
         case kStripe_Mode: {
             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.
             const int stripeHeight = 37;
@@ skipping 59 matching lines @@
                     switch (result) {
                         case SkImageGenerator::kSuccess:
                         case SkImageGenerator::kIncompleteInput:
                             break;
                         default:
                             return SkStringPrintf("Cannot skip scanlines for %s.", fPath.c_str());
                     }
                 }
             }
             canvas->drawBitmap(bitmap, 0, 0);
+            break;
         }
     }
     return "";
 }
 
 SkISize CodecSrc::size() const {
     SkAutoTUnref<SkData> encoded(SkData::NewFromFileName(fPath.c_str()));
     SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(encoded));
     if (NULL != codec) {
         SkISize size = codec->getScaledDimensions(fScale);
@@ skipping 613 matching lines @@
             skr.visit<void>(i, drawsAsSingletonPictures);
         }
         SkAutoTUnref<SkPicture> macroPic(macroRec.endRecordingAsPicture());
 
         canvas->drawPicture(macroPic);
         return "";
     });
 }
 
 }  // namespace DM