Provides multiple implementations of Android's SkBitmapRegionDecoder

dm/DMSrcSink.cpp

Index: dm/DMSrcSink.cpp
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index f555b9fcbdba78f485e47b9baf4be67d2fa1d83c..6d4c3892d422e07c1cfced1db4739e9e16dee7be 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -64,6 +64,121 @@ void GMSrc::modifyGrContextOptions(GrContextOptions* options) const {
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
+BRDSrc::BRDSrc(Path path, SkBitmapRegionDecoder::Strategy strategy, Mode mode, SkColorType dstColorType, uint32_t sampleSize)

[scroggo, 2015/08/13 16:53:06]

nit: too many chars

[msarett, 2015/08/13 18:10:22]

Done.

+    : fPath(path)
+    , fStrategy(strategy)
+    , fMode(mode)
+    , fColorType(dstColorType)
+    , fSampleSize(sampleSize)
+{}
+
+bool BRDSrc::veto(SinkFlags flags) const {
+    // It is interesting to test different dstColorTypes instead of different sinks

[scroggo, 2015/08/13 16:53:06]

Huh?

[msarett, 2015/08/13 18:10:22]

I changed this comment.

The original comment was an attempt to say that we want to test on different
SkColorTypes rather than different backends.

+    return flags.type != SinkFlags::kRaster
+        || flags.approach != SinkFlags::kDirect;
+}
+
+Error BRDSrc::draw(SkCanvas* canvas) const {
+    if (kRGB_565_SkColorType == canvas->imageInfo().colorType()) {
+        return Error::Nonfatal("Testing to multiple backends is uninteresting.");
+    }
+
+    switch (fMode) {
+        case kNormal_Mode: {
+            SkAutoTDelete<SkBitmapRegionDecoder> brd(
+                    SkBitmapRegionDecoder::CreateBitmapRegionDecoder(
+                            SkNEW_ARGS(SkMemoryStream, (SkData::NewFromFileName(fPath.c_str()))),
+                            fStrategy));
+            if (NULL == brd.get()) {
+                return "Error: Could not CreateBRD.\n";

[scroggo, 2015/08/13 16:53:06]

nit: I don't think "Error: " is necessary here. I think if it returns anything
other than "Error::Nonfatal" it is considered an error. (We might want to
include the path though, as in CodecSrc.)

[msarett, 2015/08/13 18:10:22]

Done.

+            }
+            SkAutoTDelete<SkBitmap> bitmap(brd->decodeRegion(0, 0, brd->width(),
+                    brd->height(), fSampleSize, fColorType));
+            if (NULL == bitmap.get() || fColorType != bitmap->colorType()) {
+                return Error::Nonfatal("Cannot convert to color type.\n");
+            }
+            canvas->drawBitmap(*bitmap, 0, 0);
+            return "";
+        }
+        case kSubset_Mode: {
+            SkAutoTDelete<SkBitmapRegionDecoder> brd(

[scroggo, 2015/08/13 16:53:06]

It looks like these two are the same? Why not create this outside of the switch
statement, and save duplicate code?

[msarett, 2015/08/13 18:10:22]

Done.

+                    SkBitmapRegionDecoder::CreateBitmapRegionDecoder(
+                            SkNEW_ARGS(SkMemoryStream, (SkData::NewFromFileName(fPath.c_str()))),
+                            fStrategy));
+            if (NULL == brd.get()) {
+                return "Error: Could not CreateBRD.\n";
+            }
+
+            // Use a border to test what happens when we ask for subsets that extend outside the image

[msarett, 2015/08/13 15:16:34]

This leads to outputs that are a bit strange for 565.  We zero the part of the
bitmap rect that is outside the image.  For 565, this is black.  When we
eventually draw on the canvas, some scaling modes will have averaged some of
these black pixels with pixels on the edge of the image.  For very small scaled
images, ~1-5 pixels, black edges are noticeable.

[scroggo, 2015/08/13 16:53:06]

Wait, so if the subset is bigger than 5 pixels, these edges are not noticeable?
I hope no one is creating 5 pixel subsets!

That said, if we do have some sort of problem, it seems like we could do the
following:
- only draw the subset that intersects with the actual image
- if the requested subset is goes outside the bounds of the image, fill the
destination with black (rather than filling the pre-scaled destination with
black)
- then draw the subset

That is probably better anyway, since it means:
- we filled less
- we drew a smaller temporary bitmap to the final output

[msarett, 2015/08/13 18:10:22]

So I think my comment was completely wrong.

We are already doing what you suggest.

I think that a few of the really small outputs have black pixels on the edges in
565 due to "an implementation bug that I don't understand yet".

I know we don't really care about these subsets (the reason I have the tests is
because I want to make sure we don't crash), but I should probably figure out
what is actually going on.

[scroggo, 2015/08/28 13:49:43]

+1

[msarett, 2015/08/28 18:45:50]

I've fixed this.  I had an off by one error that was caused when the border was
larger than the image.  I think that the issue is with this test, not the
implementation.  So I've set a limit on the border size and added a comment.  If
you're interested in what was actually going on, I'm happy to explain in person
but I think it's a little tricky to explain here.

+            uint32_t scaledBorder = 5;
+            uint32_t unscaledBorder = 5 * fSampleSize;
+            uint32_t divisor = 2;
+            uint32_t width = brd->width();
+            uint32_t height = brd->height();
+            for (uint32_t x = 0; x < divisor; x++) {
+                for (uint32_t y = 0; y < divisor; y++) {
+                    // Calculate the subset dimensions
+                    uint32_t subsetWidth = width / divisor;
+                    uint32_t subsetHeight = height / divisor;
+                    int left = x * subsetWidth;
+                    int top = y * subsetHeight;
+
+                    // Increase the size of the last subset in each row or column, when the
+                    // divisor does not divide evenly into the image dimensions
+                    subsetWidth += (x + 1 == divisor) ? (width % divisor) : 0;
+                    subsetHeight += (y + 1 == divisor) ? (height % divisor) : 0;
+
+                    // Increase the size of the subset in order to have a border on each side
+                    int decodeLeft = left - unscaledBorder;
+                    int decodeTop = top - unscaledBorder;
+                    uint32_t decodeWidth = subsetWidth + unscaledBorder * 2;
+                    uint32_t decodeHeight = subsetHeight + unscaledBorder * 2;
+
+                    SkAutoTDelete<SkBitmap> bitmap(brd->decodeRegion(decodeLeft,
+                            decodeTop, decodeWidth, decodeHeight, fSampleSize, fColorType));
+                    if (NULL == bitmap.get() || fColorType != bitmap->colorType()) {
+                        return Error::Nonfatal("Cannot convert to color type.\n");
+                    }
+                    canvas->drawBitmapRect(*bitmap,
+                            SkRect::MakeXYWH(scaledBorder, scaledBorder,
+                                    subsetWidth / fSampleSize, subsetHeight / fSampleSize),
+                            SkRect::MakeXYWH(left / fSampleSize, top / fSampleSize,
+                                    subsetWidth / fSampleSize, subsetHeight / fSampleSize),
+                            NULL);
+                }
+            }
+            return "";
+        }
+        default:
+            SkASSERT(false);
+            return "Error: Should not be reached.\n";
+    }
+}
+
+SkISize BRDSrc::size() const {
+    SkAutoTUnref<SkData> encoded(SkData::NewFromFileName(fPath.c_str()));
+    SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(encoded));
+    if (NULL != codec) {

[scroggo, 2015/08/13 16:53:06]

nit: checking !condition seems odd to me, if there's going to be an else. i.e.

if (!condition) {
  // condition was false
} else {
  // condition was true
}

Seems less straightforward than

if (condition) {
  // condition was true
} else {
  // condition was false
}

If you want to have the condition where we had a codec first, you could make
that:

if (codec) {
  // ...
} else {
  // ...
}

(I just looked at the style guide and was reminded that if (codec) is the
"slightly preferred" way to check for the existence of codec.)

[msarett, 2015/08/13 18:10:22]

Done.

+        return SkISize::Make(codec->getInfo().width() / fSampleSize,
+                codec->getInfo().height() / fSampleSize);
+    } else {

[scroggo, 2015/08/13 16:53:06]

This else is unnecessary, since the earlier condition returned.

[msarett, 2015/08/13 18:10:23]

Done.

+        return SkISize::Make(0, 0);
+    }
+}
+
+Name BRDSrc::name() const {
+    // We will try to replicate the names used by CodecSrc so that images can

[scroggo, 2015/08/13 16:53:06]

Try to? Do we ever fail?

[msarett, 2015/08/13 18:10:22]

Nope :)

+    // be compared in Gold.
+    if (1.0f == fSampleSize) {
+        return SkOSPath::Basename(fPath.c_str());
+    } else {
+        float scale = 1.0f / fSampleSize;
+        return SkStringPrintf("%s_%.3f", SkOSPath::Basename(fPath.c_str()).c_str(), scale);
+    }
+}
+
+/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
+
 CodecSrc::CodecSrc(Path path, Mode mode, DstColorType dstColorType, float scale)
     : fPath(path)
     , fMode(mode)
@@ -448,8 +563,7 @@ 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);
-        return size;
+        return codec->getScaledDimensions(fScale);
     } else {
         return SkISize::Make(0, 0);
     }