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 12530142d29f013eff4f65162721aff7e0437dcc..9a42f8b405ba2d91449c208998c2f11f66103095 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -64,6 +64,118 @@ void GMSrc::modifyGrContextOptions(GrContextOptions* options) const {
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
+BRDSrc::BRDSrc(Path path, SkBitmapRegionDecoder::Strategy strategy, Mode mode,
+        SkColorType dstColorType, uint32_t sampleSize)
+    : fPath(path)
+    , fStrategy(strategy)
+    , fMode(mode)
+    , fColorType(dstColorType)
+    , fSampleSize(sampleSize)
+{}
+
+bool BRDSrc::veto(SinkFlags flags) const {
+    // No need to test to non-raster or indirect 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.");

[scroggo, 2015/09/01 22:05:28]

More importantly, I think we never run this for 565, right? (Or did I misread an
earlier comment?) So we would be decoding an 8888 bitmap and trying to draw it
to 8888.

[msarett, 2015/09/02 18:02:23]

Sorry, forgot about this.  Otherwise I might have proposed a similar change to
CodecSrc.

I don't really like how CodecSrc decodes to 8888 (and maybe other color types)
when the backend is 8888 and decodes to 565 (and has to check for and disable
other color types) when the backend is 565.

For BRDSrc, I decided to only allow one backend (8888).  This is tested to all
of the color types that we want to decode to (8888, 565, Index8 etc).  Maybe
this is just as confusing as what we do in CodecSrc?

Or maybe it's confusing because it's different than what we do in CodecSrc?

[scroggo, 2015/09/02 21:32:24]

I agree that it is a little awkward. I assume your dislike of it is our
combination of two different ideas:
1) decode to the color type of the canvas
This allows us to take advantage of the fact that DM is already set up to choose
a color type (even though it's treated a little differently by the image tests).
It allows us to separate the images into different folders (565, 8888). This
also allows us to compare the images to each other.
2) for index8 and gray, decode to the 8888 canvas
This is because we *cannot* draw to an index8/gray backend. This is sort of a
workaround. It means that these images all go into the 8888 folder, but are
separated by other folders

We could switch everything to (2). I think the disadvantage is that all our
images go into the 8888 folder, and we need to mark their subfolders with their
types (as we already do for index8 and gray).

Or maybe we just need to think harder about how we do this? It occurs to me that
"image" is the only type that uses TaggedSrc::options; maybe options should be
more than one level deep?

Right now, if I look in my dm results folder, I see the following structure:

565
  codec codec_subset decode scanline scanline_subset stripe subset
8888
  <all the same ones> codec_kGray8 codec_kIndex8 <etc...>

Maybe instead it should be something like:

8888
  codec
    8888 565 kIndex8 kGray8
  scanline
    <same config list>
  <etc>

It would be a little bit weird, because we have to drill down into the 8888
folder to find 565, although we already have to do so to get to kIndex8 and
kGray8, so maybe it's not so bad after all?

The other thing to consider is how do we make Gold separate these? The other
nice thing about relying on the canvas config is that Gold correctly shows 565
images as having the config 565 (but obviously it's wrong for kIndex8 and
kGray8...). This would be relegated to options, which may not be so bad. (We
would need to update Gold to handle our new options structure.)
It's probably confusing because it's different. I think we should stick with the
same decision for both.

[msarett, 2015/09/03 19:20:51]

I like this idea.
I'm going to change this to act like CodecSrc for two reasons:
(1) It's simple.
(2) I think you make a good point about Gold.  I like the separation of 8888 and
565 and don't want to lose that.

+    }
+
+    SkAutoTDelete<SkBitmapRegionDecoder> brd(
+            SkBitmapRegionDecoder::CreateBitmapRegionDecoder(
+                    SkNEW_ARGS(SkMemoryStream, (SkData::NewFromFileName(fPath.c_str()))),

[scroggo, 2015/09/01 22:05:28]

nit: no need for SkNEW anymore

[msarett, 2015/09/02 18:02:23]

Done.

+                    fStrategy));
+    if (nullptr == brd.get()) {
+        return Error::Nonfatal(SkStringPrintf("Could not create brd for %s.", fPath.c_str()));
+    }
+
+    switch (fMode) {
+        case kFullImage_Mode: {
+            SkAutoTDelete<SkBitmap> bitmap(brd->decodeRegion(0, 0, brd->width(),
+                    brd->height(), fSampleSize, fColorType));
+            if (nullptr == bitmap.get() || fColorType != bitmap->colorType()) {
+                return Error::Nonfatal("Cannot convert to color type.\n");
+            }
+            canvas->drawBitmap(*bitmap, 0, 0);
+            return "";
+        }
+        case kDivisor_Mode: {
+            uint32_t divisor = 2;
+            // Use a border to test subsets that extend outside the image.
+            // We will not allow the border to be larger than the image dimensions.  Allowing
+            // these large borders causes off by one errors that indicate a problem with the
+            // test suite, not a problem with the implementation.
+            uint32_t maxBorder = SkTMin(brd->width(), brd->height()) / (fSampleSize * divisor);
+            uint32_t scaledBorder = SkTMin(5u, maxBorder);
+            uint32_t unscaledBorder = scaledBorder * fSampleSize;
+
+            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 (nullptr == 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),
+                            nullptr);
+                }
+            }
+            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 (codec) {
+        return SkISize::Make(SkTMax(1, codec->getInfo().width() / (int) fSampleSize),

[scroggo, 2015/09/01 22:05:28]

It seems a little weird that we calculate this using a codec divided by sample
size, but this class does not necessarily use a codec, right? Maybe this should
create a BRD?

[msarett, 2015/09/02 18:02:23]

Agreed.  Fixed to use a BRD.

+                SkTMax(1, codec->getInfo().height() / (int) fSampleSize));
+    }
+    return SkISize::Make(0, 0);
+}
+
+Name BRDSrc::name() const {
+    // We will replicate the names used by CodecSrc so that images can
+    // be compared in Gold.
+    if (1.0f == fSampleSize) {
+        return SkOSPath::Basename(fPath.c_str());
+    }
+    float scale = 1.0f / fSampleSize;
+    return SkStringPrintf("%s_%.3f", SkOSPath::Basename(fPath.c_str()).c_str(), scale);

[scroggo, 2015/09/01 22:05:28]

Maybe this should be in a helper function?

[msarett, 2015/09/02 18:02:23]

Yeah I think so.

+}
+
+/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
+
 CodecSrc::CodecSrc(Path path, Mode mode, DstColorType dstColorType, float scale)
     : fPath(path)
     , fMode(mode)
@@ -471,9 +583,8 @@ SkISize CodecSrc::size() const {
 Name CodecSrc::name() const {
     if (1.0f == fScale) {
         return SkOSPath::Basename(fPath.c_str());
-    } else {
-        return SkStringPrintf("%s_%.3f", SkOSPath::Basename(fPath.c_str()).c_str(), fScale);
     }
+    return SkStringPrintf("%s_%.3f", SkOSPath::Basename(fPath.c_str()).c_str(), fScale);
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/