dm changes

dm/DMSrcSink.cpp

Index: dm/DMSrcSink.cpp
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 090a24d86a62590f123881274578b0468e59a01d..e208770490623f327d31046259e8238139705a54 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,63 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                                               fPath.c_str(), y-1, (int) result);
                 }
             }
+            canvas->drawBitmap(bitmap, 0, 0);
+            break;
+        }
+        case kSubset_Mode: {
+            //this mode decodes the image in divisor*divisor subsets, using a scanline decoder  
+            const int divisor = 2;
+            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()/divisor, 
+                    decodeInfo.height()/divisor);
+            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()];

[msarett, 2015/05/19 14:01:13]

nit: Let's check with Leon, but I think the preference here is to use
SkNEW_ARRAY.

[scroggo, 2015/05/19 15:21:41]

The short answer is yes.

A little background: we have some macros defined in SkPostConfig.h (various
versions of SkNEW and SkDELETE) which are used to call new/delete. The
motivation (as I understand it) is to allow someone using Skia to track memory
usage of Skia objects. By default, they do what you expect - they just call
new/delete, and this is how it is used by Chrome and Android.

Someone *could* redefine them to keep track of Skia memory usage (I believe
Mozilla used this at one point), or they could even use some special allocator.
I do not know whether anyone defines them to anything else. I believe there was
some talk of removing them recently, and providing another way to do it.

Internally, we try to use them everywhere. In our tests, we seem to have a mix
and match. I believe I've heard it argued that our tests are not Skia proper,
and whoever might care to use a different implementation is really concerned
with Skia itself, so there is no reason to use it in tests.

That said, SkAutoTDeleteArray calls SkDELETE_ARRAY, so yes, we should probably
use SkNEW_ARRAY. Otherwise if we were building with alternate implementations (a
big if...), we would not call the right version of delete[].

[emmaleer, 2015/05/21 17:51:13]

Acknowledged.

+            SkAutoTDeleteArray<char> lineDeleter(line);
+            const int subsetWidth  = subsetDecodeInfo.width();
+            const int subsetHeight = subsetDecodeInfo.height();
+            const int w = subsetWidth*divisor;

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

I think it would be better to use decodeInfo.width().

One problem with the current code shows itself if the width is odd: e.g. 501

subsetWidth will be 250, and w will be 500. So we'll miss out on the last pixel
on the right.

Which maybe means we should round up when finding the subsetWidth? (i.e. we'd
rather have two tiles horizontally, with one at 251 and one at 250 than three,
with two at 250 and one at 1)

[msarett, 2015/05/19 14:01:13]

I think this is the tricky part of this test.  If the divisor does not divide
evenly into width and height, we may need to adjust the width and height of the
end subsets to make sure we cover the entire image.

The automated test system will indicate that decodes are failing if we miss a
row or column of pixels.

[scroggo, 2015/05/19 15:21:41]

Indeed. We need to investigate how exactly BitmapRegionDecoder is used to
determine the right way to do that adjustment.
FYI Emmalee: Matt is referring to gold.skia.org, which Matt or I can show you
how to use.

Although it is automated, we need to make sure we stay on top of baselining
images in order for us to know when we've broken something. Matt, it looks like
you've done most of that (or Derek?) - thanks! There are a few at head that are
untriaged though:

A couple are JPEG, which are slightly different for SkCodec vs SkImageDecoder.
I'm guessing that's the difference between using the visual optimizations? The
others look to be bmp images that we just have never triaged. These can be a
good chance to show Emmalee how to use it.

+            const int h = subsetHeight*divisor;
+            if (divisor > w || divisor > 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 < w; x += subsetWidth) {
+                for (int y = 0; y < h; y += subsetHeight) {
+                    for (int subsetY = 0; subsetY < subsetHeight; ++subsetY) {

[msarett, 2015/05/19 14:01:13]

My preference is to use a different scanline decoder for each of the subsets. 
That is because my personal stake in this test is to test the behavior of
subsetScanlineDecoder->skipScanlines(int numLinesToSkip).

This call is useful when we need to decode a single subset of an image and we
want to skip to that subset without doing extra decoding work on other subsets.

However, there are also cases where we want to decode all of the subsets of an
image, so your choice is completely valid.  Maybe we should be testing both? 
Any thoughts Leon?

[scroggo, 2015/05/19 15:21:41]

Good questions. I think it's reasonable to use a new scanline decoder for each
subset and use skipScanlines. In DM, where we're testing correctness, I don't
think it's necessary to test both, since I don't think using the same scanline
decoder for the entire vertical stripe tests anything different from the pure
scanline mode (i.e. kSubset_Mode) that isn't tested by using skipScanlines.

When we write the performance tests, however, maybe we will want to test more
variations. The real question there will be which use cases are most important?
If someone is only decoding a single subset, the question is somewhat moot (we
always will skip to the line we need, and we never need to reuse the scanline
decoder). But if someone potentially wants the whole image (as is the case in
Gallery), how to do the next subset is an interesting question. Unfortunately,
apps are already using BitmapRegionDecoder, which allows requesting arbitrary
subsets at any time. If they happen to decode a subset below the last one, we
could potentially avoid skipping scanlines, although they likely choose which
subset to decode based on which direction the user scrolls.

+                        const SkImageGenerator::Result subsetResult = 
+                                subsetScanlineDecoder->getScanlines(line, 1, 0); 

[msarett, 2015/05/19 14:01:13]

I'm a little confused about how this works.  It looks like you'll call
getScanlines() (subsetHeight * divisor * divisor) times.  Doesn't it stop giving
you back scanlines after height calls?

If the code works then I'm obviously confused (sorry!), but I can't seem to
figure it out.

[scroggo, 2015/05/19 15:21:41]

I missed this at first, too - at the end of the y loop we call
getScanlineDecoder to get a new one. (API question - it may be cleaner to add a
new API: SkScanlineDecoder::reset().)

(And to answer your question, yes, one scanline decoder would stop giving you
scanlines after height calls.)

+                        //copy section of line based on x value
+                        memcpy(subsetBm.getAddr(0, subsetY), 
+                                line + x*subsetDecodeInfo.bytesPerPixel(), 
+                                subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel());  
+                        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);
+                }
+                subsetScanlineDecoder = codec->getScanlineDecoder(decodeInfo, NULL,
+                        colorPtr, colorCountPtr);
+                if (NULL == subsetScanlineDecoder) {
+                    return SkStringPrintf("Error scanline decoder is NULL");

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

No need for SkStringPrintf. You're returning an Error (defined in DMSrcSink.h),
which has an implicit constructor that takes a char*. So you can just say:

return "Error scanline decoder is NULL";

+                }
+            }
             break;
         }
     }
-    canvas->drawBitmap(bitmap, 0, 0);
     return "";
 }