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 kSubset_Mode: {
+            //this mode decodes the image in 4 subsets, using a scanline decoder  
+            int fDivisor = 2;

[scroggo, 2015/05/18 14:03:58]

style nit: Skia adds an 'f' to the beginning of variables to show that they are
member variables (or 'fields'). For local variables, we would just name them
something like "divisor".

Also, this variable could be const. (Not absolutely necessary, but it lets a
reader and the compiler know that you won't be changing it.)

+            SkScanlineDecoder* subsetScanlineDecoder = codec->getScanlineDecoder(decodeInfo, NULL,
+                    colorPtr, colorCountPtr);
+            if (NULL == subsetScanlineDecoder) {
+                return Error::Nonfatal("Cannot use scanline decoder for all images");
+            }
+            SkImageInfo subsetDecodeInfo = decodeInfo.makeWH(decodeInfo.width()/fDivisor, 
+                    decodeInfo.height()/fDivisor);
+            SkBitmap subsetBm;
+            if (!subsetBm.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 = new char[decodeInfo.width()*decodeInfo.bytesPerPixel()];

[scroggo, 2015/05/18 14:03:58]

This will leak. You can solve this a few different ways. The most
straightforward way is to call

delete[] line;

when you are done with it. The problem with this approach is that if you had
some error case that resulted in a return statement before you get to the line
that calls delete[], it would still leak the memory.

We have some ways to make it automatically get deleted when it goes out of
scope, similar to smart pointers, which we have discussed. The Skia objects that
do this are in SkTemplates. For example, you can use SkAutoTDeleteArray. After
the line that declares the variable "line", you would write the following:

SkAutoTDeleteArray lineDeleter(line);

[scroggo, 2015/05/18 20:43:18]

Yes.

+            for (int subset = 0; subset < fDivisor*2; subset++) {

[scroggo, 2015/05/18 14:03:58]

subset < fDivisor*2

Shouldn't this be subset < fDivisor * fDivisor? If you changed fDivisor to 3,
won't you have 9 subsets?

+                //reset scanline decoder for right half of image
+                if (subset == 2) {

[scroggo, 2015/05/18 14:03:59]

Again, this supposes that fDivisor is 2. The nice thing about using the variable
fDivisor is that we can change one line of code and test a different thing. 

+                    subsetScanlineDecoder = codec->getScanlineDecoder(decodeInfo, NULL,
+                            colorPtr, colorCountPtr);
+                    if (NULL == subsetScanlineDecoder) {
+                        return Error::Nonfatal("Cannot use scanline decoder for all images");

[scroggo, 2015/05/18 14:03:59]

I think in this case we have an actual error. If the scanline decoder cannot be
created the first time, it may mean the image was a progressive image, so we
cannot create one. If we can create it once, we should be able to create it
again for the same image. (Assuming we could rewind the stream, but all of our
streams we use here should be rewindable.)

[scroggo, 2015/05/18 20:43:18]

Oh, no, the check is necessary; the behavior if subsetScanlineDecoder is null is
incorrect. Place the check back, and if it is NULL return an error.

DM is a little sneaky: draw() returns an Error, which is a glorified SkString.
You can return an error just by saying:

return "Error message!";

or

return SkStringPrintf("Error %s", "message!");

[scroggo, 2015/05/18 23:02:17]

Yes. Both the message itself and "Error::Nonfatal", which tells us that it is
not a real error.

+                    }
+                }
+                for (int y = 0; y < subsetDecodeInfo.height(); ++y) {
+                    const SkImageGenerator::Result subsetResult = 
+                            subsetScanlineDecoder->getScanlines(line, 1, 0); 
+                    if (subset == 0 || subset == 1) {
+                        //if subset is on left copy left half of line
+                        memcpy(subsetBm.getAddr(0, y), line, 
+                                subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel());  
+                    } else {
+                        //if subset if on right copy right half of line

[scroggo, 2015/05/18 14:03:58]

is*

[emmaleeroach, 2015/05/18 20:04:27]

Acknowledged.

+                        memcpy(subsetBm.getAddr(0,y), 

[scroggo, 2015/05/18 14:03:58]

Won't this copy to the beginning of this row?

[scroggo, 2015/05/18 20:43:18]

Oh, of course!

+                                line + subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel(), 
+                                subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel());

[scroggo, 2015/05/18 14:03:58]

Won't this copy too much?

[scroggo, 2015/05/18 20:43:17]

Got it. My eyes must have glazed over when I read subsetDecodeInfo.width(), so I
thought it was the width of the whole image instead of the subset, as it clearly
states.

+                    }
+                    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);
+                    }
+                }
+                int w;
+                int h; 

[scroggo, 2015/05/18 14:03:58]

nit: These variables look like width and height, but they are x and y, right?

+                //set drawing position based on subset number
+                switch (subset) {

[scroggo, 2015/05/18 14:03:58]

nit: This works with a divisor of 2, but I think it will be harder if we try to
make it more general purpose. It might be easier to follow the pattern of
ImageSrc when it has an fDivisor != 0.

[scroggo, 2015/05/18 20:43:18]

https://skia.googlesource.com/skia/+/7be0ce0ab123bbad8ec22b67b8593f1bdd5179e0...,
lines 232-253. I don't know that it's significantly better; it is just a little
more succinct.

+                    case 0:
+                        w = 0;
+                        h = 0;
+                        break;
+                    case 1:
+                        w = 0;
+                        h = subsetDecodeInfo.height();
+                        break;
+                    case 2:
+                        w = subsetDecodeInfo.width();
+                        h = 0;
+                        break;
+                    case 3:
+                        w = subsetDecodeInfo.width();
+                        h = subsetDecodeInfo.height();
+                        break;
+                }
+                canvas->drawBitmap(subsetBm, w, h);
+            }  
             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