dm changes

dm/DMSrcSink.cpp

Index: dm/DMSrcSink.cpp
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 090a24d86a62590f123881274578b0468e59a01d..fa7d7a72418d596758b25aaf9d431de8c9e238cc 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -141,6 +141,7 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                     // 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,
@@ -160,10 +161,93 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                                               fPath.c_str(), y-1, (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();
+            const int subsetWidth = decodeInfo.width()/divisor;

[scroggo, 2015/05/21 19:32:35]

nit: From here on you could use w and h instead of decodeInfo.width() and
decodeInfo.height()

+            const int subsetHeight = decodeInfo.height()/divisor;
+            SkImageInfo subsetDecodeInfo = 

[scroggo, 2015/05/21 19:32:36]

I think these names might be a little misleading. subsetDecodeInfo has a
different width from subsetWidth. Maybe rename this largestSubsetDecodeInfo?
(That does get a little unwieldy though...)

[emmaleeroach, 2015/05/21 22:17:24]

Acknowledged.

+                    decodeInfo.makeWH(subsetWidth + decodeInfo.width()%divisor, 

[scroggo, 2015/05/21 19:32:36]

Please add a comment that explains why we're adding w % divisor.

+                    subsetHeight + decodeInfo.height()%divisor);
+            SkBitmap subsetBm;
+            if (!subsetBm.tryAllocPixels(subsetDecodeInfo, NULL, colorTable.get())) {

[scroggo, 2015/05/21 19:32:35]

I don't think you need this. This will set subsetBm's info and allocate an
SkPixelRef. Down below, you call:

largestSubsetBm.extractSubset(&subsetBM, bounds);

which will set the info again and replace the pixel ref. It doesn't look like
subsetBm is used until then.

[emmaleeroach, 2015/05/21 22:17:24]

Acknowledged.

+                return SkStringPrintf("Image(%s) is too large (%d x %d)\n", fPath.c_str(),
+                        subsetDecodeInfo.width(), subsetDecodeInfo.height());
+            }
+            SkBitmap largestSubsetBm;
+            if (!largestSubsetBm.tryAllocPixels(subsetDecodeInfo, NULL, colorTable.get())) {
+                return SkStringPrintf("Image(%s) is too large (%d x %d)\n", fPath.c_str(),
+                        subsetDecodeInfo.width(), subsetDecodeInfo.height());
+            }
+            char* line = SkNEW_ARRAY(char, decodeInfo.width()*decodeInfo.bytesPerPixel());

[scroggo, 2015/05/21 19:32:35]

FYI: You could instead use decodeInfo.minRowBytes().

+            SkAutoTDeleteArray<char> lineDeleter(line);
+            SkIRect bounds = SkIRect::MakeWH(subsetWidth, subsetHeight);
+            //used if image w and h are not evenly divided by divisor 
+            int extrax = 0;

[scroggo, 2015/05/21 19:32:36]

nit: extraX (camelCase)

+            int extray = 0;
+            if (divisor > w || divisor > h) {

[scroggo, 2015/05/21 19:32:35]

This probably belongs closer to the beginning, right after we've declared w and
h.

+                return SkStringPrintf("divisor %d is too big for %s with dimensions (%d x %d)",
+                        divisor, fPath.c_str(), w, h);
+            }
+            for (int x = 0; x < subsetWidth*divisor; x += subsetWidth) {
+                //consider extrax for rightmost subset 
+                if (x + 2*subsetWidth > w) {
+                    extrax = w - subsetWidth*divisor;
+                }         
+                for (int y = 0; y < subsetHeight*divisor; y += subsetHeight) {
+                    //consider extray for bottom subset
+                    if (y + 2*subsetHeight > h) {
+                        extray = h - subsetHeight*divisor;
+                    }
+                    //create scanline decoder for each subset
+                    SkScanlineDecoder* subsetScanlineDecoder = codec->getScanlineDecoder(decodeInfo, 
+                            NULL, colorPtr, colorCountPtr);
+                    if (NULL == subsetScanlineDecoder) {
+                        return Error::Nonfatal("Cannot use scanline decoder for all images");

[scroggo, 2015/05/21 19:32:36]

This is good for the first time through the loop, but after that, this should be
a fatal error.

[emmaleeroach, 2015/05/21 22:17:24]

Acknowledged.

+                    }
+                    //skip to first line of subset
+                    const SkImageGenerator::Result skipResult = 
+                            subsetScanlineDecoder->skipScanlines(y);
+                    switch (skipResult) {
+                        case SkImageGenerator::kSuccess:
+                        case SkImageGenerator::kIncompleteInput:
+                            break;
+                        default:
+                            return SkStringPrintf("%s failed after skipping %d scanlines with"

[scroggo, 2015/05/21 19:32:36]

Maybe rephrase this: y-1 is not the number of scanlines we successfully skipped;
y is the number we attempted to skip. We do not know how many lines were skipped
before failing. (This is different from pure kScanline_Mode, where we read one
scanline at a time, so we know how many succeeded.)

[emmaleeroach, 2015/05/21 22:17:24]

Acknowledged.

+                                    "error message %d", fPath.c_str(), y-1, (int) skipResult);
+                    }
+                    //set size of subsetBm
+                    bounds.setXYWH(0, 0, subsetWidth + extrax, subsetHeight + extray);

[scroggo, 2015/05/21 19:32:35]

This is always the size of subsetBm, right? Can you move the declaration here?

+                    largestSubsetBm.extractSubset(&subsetBm, bounds);

[scroggo, 2015/05/21 19:32:36]

Try to declare variables just before they're used. You can declare subsetBm
here.

Also, you should check the return value in case it fails. (If it should never
fail, you can SkASSERT(success);)

[emmaleeroach, 2015/05/21 22:17:24]

Acknowledged.

+                    SkAutoLockPixels autlockSubsetBm(subsetBm, true);
+                    for (int subsetY = 0; subsetY < subsetHeight + extray; ++subsetY) {
+                        const SkImageGenerator::Result subsetResult = 
+                                subsetScanlineDecoder->getScanlines(line, 1, 0); 
+                        //copy section of line based on x value   
+                        memcpy(subsetBm.getAddr(0, subsetY), 
+                                line + x*subsetDecodeInfo.bytesPerPixel(), 
+                                (subsetWidth + extrax)*subsetDecodeInfo.bytesPerPixel());

[scroggo, 2015/05/21 19:32:36]

I wonder if it would be cleaner to define new variables for 

subsetWidth + extrax

and

subsetHeight + extray

I was trying to think of the best way to do that, but it might just mean more
names...

+                        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);
+                        }
+                    }
+                    canvas->drawBitmap(subsetBm, x, y);
+                }
+                extray = 0;
+            }
             break;
         }
     }
-    canvas->drawBitmap(bitmap, 0, 0);
     return "";
 }