dm changes

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 123 matching lines @@
                     // 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);
                 }
             }
+            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 "";
 }
 
 SkISize CodecSrc::size() const {
     SkAutoTUnref<SkData> encoded(SkData::NewFromFileName(fPath.c_str()));
     SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(encoded));
     if (NULL != codec) {
         return codec->getInfo().dimensions();
     } else {
         return SkISize::Make(0, 0);
@@ skipping 603 matching lines @@
             skr.visit<void>(i, drawsAsSingletonPictures);
         }
         SkAutoTUnref<SkPicture> macroPic(macroRec.endRecordingAsPicture());
 
         canvas->drawPicture(macroPic);
         return "";
     });
 }
 
 }  // namespace DM