More work to integrate skimage with rebaseline tools.

tools/skimage_main.cpp

Index: tools/skimage_main.cpp
diff --git a/tools/skimage_main.cpp b/tools/skimage_main.cpp
index 5b8f09548535069b5f8c347cb12c1b24992bbd82..0bb81c366f6f4dc5abeed76197b2ae0f54198ae9 100644
--- a/tools/skimage_main.cpp
+++ b/tools/skimage_main.cpp
@@ -31,6 +31,8 @@ DEFINE_bool(reencode, true, "Reencode the images to test encoding.");
 DEFINE_int32(sampleSize, 1, "Set the sampleSize for decoding.");
 DEFINE_bool(skip, false, "Skip writing zeroes.");
 DEFINE_bool(testSubsetDecoding, true, "Test decoding subsets of images.");
+DEFINE_bool(writeChecksumBasedFilenames, false,  "When writing out actual images, use checksum-"
+            "based filenames, as rebaseline.py will use when downloading them from Google Storage");
 DEFINE_string2(writePath, w, "",  "Write rendered images into this directory.");
 
 struct Format {
@@ -85,7 +87,11 @@ static void make_outname(SkString* dst, const char outDir[], const char src[],
         const char* dot = strrchr(cstyleDst, '.');
         if (dot != NULL) {
             int32_t index = SkToS32(dot - cstyleDst);
-            dst->remove(index, dst->size() - index);
+            const size_t amountToRemove = dst->size() - index;
+            // Only remove if this is a 4-5 character suffix (including '.').

[epoger, 2013/10/17 15:54:13]

Why this "magic" constraint on the length of the extension to remove?

If we're going to have this constraint, please make it configurable using a
constant (or even better, a flag)

maybe something like this at the top...
DEFINE_int32(maxExtensionLength, 4);

and then...
if (amountToRemove <= (FLAGS_maxExtensionLength+1)) {
  dst->remove(index, amountToRemove);
}

[scroggo, 2013/10/18 21:28:03]

Here is the motivation: It is used to create the filename of an image which was
sometimes created by taking a subset. The name that gets passed to this function
is something like <original image.png>-[subset dimensions]. In the case of the
subset, I don't want to remove ".png-[subset dimensions]".

That said, you've forced me to rethink this a bit. As currently written,
decoding the file "image.webp" will create a few files. It will create
"image.png", which is the same image decoded to Skia and then reencoded to png.
It will also create various subsets, with names like "image.webp-[L,T,R,B].png".
The second name retains information about the original image (it was a webp
image) and also ensures that if I ran the test on both "image.webp" and
"image.jpg" the output from both images survives. The first name does neither of
these.

So I've updated the code to never remove any suffix, but to instead change the
name of the file to contain the original suffix (after a '-' instead of a '.').
Now "image.webp" begets "image-webp.png" and "image-webp-[L,T,R,B].png". This
also means that "image.jpg" can also create an output without one of them being
overwritten by the other.

It also means that I have removed the magic.

[epoger, 2013/10/21 15:13:39]

Thank you for:
1. removing the magic
2. the cogent explanation
3. using the word "beget"

+            if (amountToRemove == 4 || amountToRemove == 5) {
+                dst->remove(index, amountToRemove);
+            }
         }
         dst->append(suffix);
     }
@@ -129,8 +135,44 @@ static SkBitmap::Config gPrefConfig(SkBitmap::kNo_Config);
 // previously written using createExpectationsPath.
 SkAutoTUnref<skiagm::JsonExpectationsSource> gJsonExpectations;
 
-static bool write_bitmap(const char outName[], const SkBitmap& bm) {
-    if (SkImageEncoder::EncodeFile(outName, bm, SkImageEncoder::kPNG_Type, 100)) {
+/**
+ *  Encode the bitmap to a file, written one of two ways, depending on
+ *  FLAGS_writeChecksumBasedFilenames. If true, the final image will be

[epoger, 2013/10/17 15:54:13]

Maybe make writeChecksumBasedFilenames a parameter, for greater
flexibility/reusability?

[epoger, 2013/10/21 15:13:39]

I still think it would be better for this to be a parameter, but I won't close
the government over it or anything.

+ *  written to:
+ *      outDir/hashType/src/digestValue.png
+ *  If false, the final image will be written out to:
+ *      outDir/src.png
+ *  The function returns whether the file was successfully written.
+ */
+static bool write_bitmap(const char outDir[], const char src[],
+                         const skiagm::BitmapAndDigest& bitmapAndDigest) {
+    SkString filename;
+    if (FLAGS_writeChecksumBasedFilenames) {
+        // First create the directory for the hashtype.
+        const SkString hashType = bitmapAndDigest.fDigest.getHashType();
+        const SkString hashDir = SkOSPath::SkPathJoin(outDir, hashType.c_str());
+        if (!sk_mkdir(hashDir.c_str())) {
+            return false;
+        }
+
+        // Now create the name of the folder specific to this image.
+        SkString basename = SkOSPath::SkBasename(src);
+        const SkString imageDir = SkOSPath::SkPathJoin(hashDir.c_str(), basename.c_str());
+        if (!sk_mkdir(imageDir.c_str())) {
+            return false;
+        }
+
+        // Name the file <digest>.png
+        SkString checksumBasedName = bitmapAndDigest.fDigest.getDigestValue();
+        checksumBasedName.append(".png");
+
+        filename = SkOSPath::SkPathJoin(imageDir.c_str(), checksumBasedName.c_str());
+    } else {
+        make_outname(&filename, outDir, src, ".png");
+    }
+
+    const SkBitmap& bm = bitmapAndDigest.fBitmap;
+    if (SkImageEncoder::EncodeFile(filename.c_str(), bm, SkImageEncoder::kPNG_Type, 100)) {
         return true;
     }
 
@@ -145,7 +187,7 @@ static bool write_bitmap(const char outName[], const SkBitmap& bm) {
     if (!bm.copyTo(&bm8888, SkBitmap::kARGB_8888_Config)) {
         return false;
     }
-    return SkImageEncoder::EncodeFile(outName, bm8888, SkImageEncoder::kPNG_Type, 100);
+    return SkImageEncoder::EncodeFile(filename.c_str(), bm8888, SkImageEncoder::kPNG_Type, 100);
 }
 
 /**
@@ -189,27 +231,30 @@ static SkIRect generate_random_rect(SkRandom* rand, int32_t maxX, int32_t maxY)
     return rect;
 }
 
-// Stored expectations to be written to a file if createExpectationsPath is specified.
-static Json::Value gExpectationsToWrite;
-
 /**
- *  If expectations are to be recorded, record the bitmap expectations into global
- *  expectations array.
+ *  Return a string which includes the name of the file and the pref config,

[epoger, 2013/10/17 15:54:13]

what does "pref" mean?  ("prefix"?  "preference"?  If "preference", a preference
among which choices?)

[scroggo, 2013/10/18 21:28:03]

I've clarified the comment.

+ *  matching the pattern of gm_json.py's IMAGE_FILENAME_PATTERN: "name_config.png"
  */
-static void write_expectations(const SkBitmap& bitmap, const char* filename) {
-    if (!FLAGS_createExpectationsPath.isEmpty()) {
-        // Creates an Expectations object, and add it to the list to write.
-        skiagm::Expectations expectation(bitmap);
-        Json::Value value = expectation.asJsonValue();
-        gExpectationsToWrite[filename] = value;
-    }
+static SkString create_json_key(const char* filename) {
+    SkASSERT(FLAGS_config.count() == 1);
+    return SkStringPrintf("%s_%s.png", filename, FLAGS_config[0]);
 }
 
+// Stored expectations to be written to a file if createExpectationsPath is specified.
+static Json::Value gExpectationsToWrite;
+
 /**
- *  Compare against an expectation for this filename, if there is one.
- *  @param digest GmResultDigest, computed from the decoded bitmap, to compare to the
- *          expectation.
- *  @param filename String used to find the expected value.
+ *  This function performs two purposes:
+ *      1) If --createExpectationsPath is set, record the expectations to the global
+ *         expectations object, for future writing to a file, which will later be

[epoger, 2013/10/17 15:54:13]

I don't think this needs to be fixed within this CL... but it bothers me that we
are using globals.  I think we're better off using member variables in C++
objects.

(Right now, when I see that this method is static, I think "oh goody, it doesn't
depend on any object state".  But it does, really.)

I suggest you add a TODO about this.

[scroggo, 2013/10/18 21:28:03]

I actually like using globals in this case. If I were to remove the dependence
on global variables, decodeFileAndWrite would basically need to take all of the
flags as parameters, which would be very messy. Alternatively, we could create
an object which holds all the flags, but that still seems like a convoluted way
to do what globals do very easily in this case.

I agree in general that we should avoid global variables, but since this a tool
which does not need to be reused by any other code, the convenience of global
variables outweigh the costs.

[epoger, 2013/10/21 15:13:39]

Fair enough.  Personally, I would prefer member variables on an object, but I
suppose it's a matter of taste.

+ *         used for comparisons.
+ *      2) If --readExpectationsPath is set, compare this bitmap to the json expectations
+ *         provided. This portion determines the return value, as described below.
+ *  The two purposes share a function so they can share filenames and expectations.

[epoger, 2013/10/17 15:54:13]

IMHO it would be clearer for this to remain as two separate methods.

It seems to me that, if both --createExpectationsPath and --readExpectationsPath
have been set, the only overlap between the two methods would be calling
create_json_key(filename), which doesn't seem that bad.

[scroggo, 2013/10/18 21:28:03]

Done.

+ *  @param bitmapAndDigest BitmapAndDigest, computed from the decoded bitmap, to both

[epoger, 2013/10/17 15:54:13]

add newline before first param

[scroggo, 2013/10/18 21:28:03]

Done.

+ *         write and compare to the existing expectation.
+ *  @param filename String used to record the new expectation and to find the existing
+ *         expectation.
  *  @param failureArray Array to add a failure message to on failure.
  *  @param missingArray Array to add failure message to when missing image
  *          expectation.
@@ -223,11 +268,26 @@ static void write_expectations(const SkBitmap& bitmap, const char* filename) {
  *                  - there is an expectation for this bitmap, but it did not match.
  *                  - expectation could not be computed from the bitmap.
  */
-static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& digest,
-                                                 const char* filename,
-                                                 SkTArray<SkString, false>* failureArray,
-                                                 SkTArray<SkString, false>* missingArray,
-                                                 SkTArray<SkString, false>* ignoreArray) {
+static bool write_and_compare_expectations(const skiagm::BitmapAndDigest& bitmapAndDigest,
+                                           const char* filename,
+                                           SkTArray<SkString, false>* failureArray,
+                                           SkTArray<SkString, false>* missingArray,
+                                           SkTArray<SkString, false>* ignoreArray) {
+    // For both writing and reading, the key for this entry will include the name
+    // of the file and the pref config, matching the pattern of gm_json.py's
+    // IMAGE_FILENAME_PATTERN: "name_config.png"
+    const SkString name_config = create_json_key(filename);
+
+    // Record the new expectation.
+    if (!FLAGS_createExpectationsPath.isEmpty()) {
+        // Creates an Expectations object, and add it to the list to write.
+        skiagm::Expectations expectation(bitmapAndDigest);
+        Json::Value value = expectation.asJsonValue();
+        gExpectationsToWrite[name_config.c_str()] = value;
+    }
+
+    // Compare to the existing expectation.
+    const skiagm::GmResultDigest& digest = bitmapAndDigest.fDigest;
     if (!digest.isValid()) {
         if (failureArray != NULL) {
             failureArray->push_back().printf("decoded %s, but could not create a GmResultDigest.",
@@ -240,7 +300,7 @@ static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& d
         return false;
     }
 
-    skiagm::Expectations jsExpectation = gJsonExpectations->get(filename);
+    skiagm::Expectations jsExpectation = gJsonExpectations->get(name_config.c_str());
     if (jsExpectation.empty()) {
         if (missingArray != NULL) {
             missingArray->push_back().printf("decoded %s, but could not find expectation.",
@@ -266,72 +326,82 @@ static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& d
 
 /**
  *  Helper function to write a bitmap subset to a file. Only called if subsets were created
- *  and a writePath was provided. Creates a subdirectory called 'subsets' and writes a PNG to
- *  that directory. Also creates a subdirectory called 'extracted' and writes a bitmap created
- *  using extractSubset to a PNG in that directory. Both files will represent the same
- *  subrectangle and have the same name for comparison.
+ *  and a writePath was provided. Behaves differently depending on
+ *  FLAGS_writeChecksumBasedFilenames. If true:

[epoger, 2013/10/17 15:54:13]

Again, rather than a static method depending on global state, I think it would
be preferable to pass in this parameter (or make it a non-static method that
depends on an object variable).

+ *      Writes the image to a PNG file named according to the digest hash, as described in
+ *      write_bitmap.
+ *  If false:
+ *      Creates a subdirectory called 'subsets' and writes a PNG to that directory. Also
+ *      creates a subdirectory called 'extracted' and writes a bitmap created using
+ *      extractSubset to a PNG in that directory. Both files will represent the same
+ *      subrectangle and have the same name for convenient comparison. In this case, the
+ *      digest is ignored.
  *  @param writePath Parent directory to hold the folders for the PNG files to write. Must

[epoger, 2013/10/17 15:54:13]

newline before first param (just to make it easier to read)

[scroggo, 2013/10/18 21:28:03]

Done.

  *      not be NULL.
- *  @param filename Basename of the original file. Used to name the new files. Must not be
- *      NULL.
- *  @param subsetDim String representing the dimensions of the subset. Used to name the new
- *      files. Must not be NULL.
- *  @param bitmapFromDecodeSubset Pointer to SkBitmap created by SkImageDecoder::DecodeSubset,
- *      using rect as the area to decode.
+ *  @param subsetName Basename of the original file, with the dimensions of the subset tacked
+ *      on. Used to name the new file/folder.
+ *  @param bitmapAndDigestFromDecodeSubset SkBitmap (with digest) created by
+ *      SkImageDecoder::DecodeSubset, using rect as the area to decode.
  *  @param rect Rectangle of the area decoded into bitmapFromDecodeSubset. Used to call
  *      extractSubset on originalBitmap to create a bitmap with the same dimensions/pixels as
  *      bitmapFromDecodeSubset (assuming decodeSubset worked properly).
  *  @param originalBitmap SkBitmap decoded from the same stream as bitmapFromDecodeSubset,
  *      using SkImageDecoder::decode to get the entire image. Used to create a PNG file for
- *      comparison to the PNG created by bitmapFromDecodeSubset.
+ *      comparison to the PNG created by bitmapAndDigestFromDecodeSubset's bitmap.
  *  @return bool Whether the function succeeded at drawing the decoded subset and the extracted
  *      subset to files.
  */
-static bool write_subset(const char* writePath, const char* filename, const char* subsetDim,
-                          SkBitmap* bitmapFromDecodeSubset, SkIRect rect,
-                          const SkBitmap& originalBitmap) {
+static bool write_subset(const char* writePath, const SkString& subsetName,
+                          const skiagm::BitmapAndDigest bitmapAndDigestFromDecodeSubset,
+                          SkIRect rect, const SkBitmap& originalBitmap) {
     // All parameters must be valid.
     SkASSERT(writePath != NULL);
-    SkASSERT(filename != NULL);
-    SkASSERT(subsetDim != NULL);
-    SkASSERT(bitmapFromDecodeSubset != NULL);
-
-    // Create a subdirectory to hold the results of decodeSubset.
-    SkString dir = SkOSPath::SkPathJoin(writePath, "subsets");
-    if (!sk_mkdir(dir.c_str())) {
-        gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from %s, but failed to "
-                                                "create a directory to write to.", subsetDim,
-                                                 filename);
-        return false;
-    }
 
-    // Write the subset to a file whose name includes the dimensions.
-    SkString suffix = SkStringPrintf("_%s.png", subsetDim);
-    SkString outPath;
-    make_outname(&outPath, dir.c_str(), filename, suffix.c_str());
-    SkAssertResult(write_bitmap(outPath.c_str(), *bitmapFromDecodeSubset));
-    gSuccessfulSubsetDecodes.push_back().printf("\twrote %s", outPath.c_str());
-
-    // Also use extractSubset from the original for visual comparison.
-    // Write the result to a file in a separate subdirectory.
-    SkBitmap extractedSubset;
-    if (!originalBitmap.extractSubset(&extractedSubset, rect)) {
-        gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from %s, but failed to "
-                                                "extract a similar subset for comparison.",
-                                                subsetDim, filename);
-        return false;
+    SkString subsetPath;
+    if (FLAGS_writeChecksumBasedFilenames) {
+        subsetPath.set(writePath);
+    } else {
+        // Create a subdirectory to hold the results of decodeSubset.
+        subsetPath = SkOSPath::SkPathJoin(writePath, "subsets");
+        if (!sk_mkdir(subsetPath.c_str())) {
+            gFailedSubsetDecodes.push_back().printf("Successfully decoded subset %s, but "
+                                                    "failed to create a directory to write to.",
+                                                     subsetName.c_str());
+            return false;
+        }
     }
+    SkAssertResult(write_bitmap(subsetPath.c_str(), subsetName.c_str(),
+                                bitmapAndDigestFromDecodeSubset));
+    gSuccessfulSubsetDecodes.push_back().printf("\twrote %s", subsetName.c_str());
+
+    if (!FLAGS_writeChecksumBasedFilenames) {
+        // FIXME: The goal of extracting the subset is for visual comparison/using skdiff/skpdiff.
+        // Currently disabling for writeChecksumBasedFilenames since it will be trickier to
+        // determine which files to compare.
+
+        // Also use extractSubset from the original for visual comparison.
+        // Write the result to a file in a separate subdirectory.
+        SkBitmap extractedSubset;
+        if (!originalBitmap.extractSubset(&extractedSubset, rect)) {
+            gFailedSubsetDecodes.push_back().printf("Successfully decoded subset %s, but failed "
+                                                    "to extract a similar subset for comparison.",
+                                                    subsetName.c_str());
+            return false;
+        }
 
-    SkString dirExtracted = SkOSPath::SkPathJoin(writePath, "extracted");
-    if (!sk_mkdir(dirExtracted.c_str())) {
-        gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from %s, but failed to "
-                                                "create a directory for extractSubset comparison.",
-                                                subsetDim, filename);
-        return false;
-    }
+        SkString dirExtracted = SkOSPath::SkPathJoin(writePath, "extracted");
+        if (!sk_mkdir(dirExtracted.c_str())) {
+            gFailedSubsetDecodes.push_back().printf("Successfully decoded subset%s, but failed "
+                                                    "to create a directory for extractSubset "
+                                                    "comparison.",
+                                                    subsetName.c_str());
+            return false;
+        }
 
-    make_outname(&outPath, dirExtracted.c_str(), filename, suffix.c_str());
-    SkAssertResult(write_bitmap(outPath.c_str(), extractedSubset));
+        skiagm::BitmapAndDigest bitmapAndDigestFromExtractSubset(extractedSubset);
+        SkAssertResult(write_bitmap(dirExtracted.c_str(), subsetName.c_str(),
+                                    bitmapAndDigestFromExtractSubset));
+    }
     return true;
 }
 
@@ -397,6 +467,20 @@ static void test_stream_without_length(const char srcPath[], SkImageDecoder* cod
 }
 #endif // defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
 
+/**
+ *  Replace all instances of oldChar with newChar in str.
+ */
+static void replace_char(SkString* str, const char oldChar, const char newChar) {

[epoger, 2013/10/17 15:54:13]

I don't think this needs to happen within this CL, but I think we'd be better
off in the long run if this method lived somewhere (like SkString.h) where it
would be:

1. available for general use
2. unit-tested

I suggest a TODO

[scroggo, 2013/10/18 21:28:03]

Done.

+    if (NULL == str) {
+        return;
+    }
+    for (size_t i = 0; i < str->size(); ++i) {
+        if (oldChar == str->operator[](i)) {
+            str->operator[](i) = newChar;
+        }
+    }
+}
+
 static void decodeFileAndWrite(const char srcPath[], const SkString* writePath) {
     SkBitmap bitmap;
     SkFILEStream stream(srcPath);
@@ -419,12 +503,15 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
 
     // Create a string representing just the filename itself, for use in json expectations.
     SkString basename = SkOSPath::SkBasename(srcPath);
+    // Replace '_' with '-', so that the names can fit gm_json.py's IMAGE_FILENAME_PATTERN
+    replace_char(&basename, '_', '-');
     const char* filename = basename.c_str();
 
     if (!codec->decode(&stream, &bitmap, gPrefConfig,
                        SkImageDecoder::kDecodePixels_Mode)) {
         if (NULL != gJsonExpectations.get()) {
-            skiagm::Expectations jsExpectations = gJsonExpectations->get(filename);
+            const SkString name_config = create_json_key(filename);
+            skiagm::Expectations jsExpectations = gJsonExpectations->get(name_config.c_str());
             if (jsExpectations.ignoreFailure()) {
                 // This is a known failure.
                 gKnownFailures.push_back().appendf(
@@ -462,41 +549,33 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
         }
     }
 
-    skiagm::GmResultDigest digest(bitmap);
-    if (compare_to_expectations_if_necessary(digest, filename,
-                                             &gDecodeFailures,
-                                             &gMissingExpectations,
-                                             &gKnownFailures)) {
+    skiagm::BitmapAndDigest bitmapAndDigest(bitmap);
+    if (write_and_compare_expectations(bitmapAndDigest, filename, &gDecodeFailures,
+                                       &gMissingExpectations, &gKnownFailures)) {
         gSuccessfulDecodes.push_back().printf("%s [%d %d]", srcPath, bitmap.width(),
                                               bitmap.height());
     } else if (!FLAGS_mismatchPath.isEmpty()) {
-        SkString outPath;
-        make_outname(&outPath, FLAGS_mismatchPath[0], srcPath, ".png");
-        if (write_bitmap(outPath.c_str(), bitmap)) {
-            gSuccessfulDecodes.push_back().appendf("\twrote %s", outPath.c_str());
+        if (write_bitmap(FLAGS_mismatchPath[0], filename, bitmapAndDigest)) {
+            gSuccessfulDecodes.push_back().appendf("\twrote %s", filename);
         } else {
-            gEncodeFailures.push_back().set(outPath);
+            gEncodeFailures.push_back().set(filename);
         }
     }
 
 // FIXME: This test could be run on windows/mac once we remove their dependence on
 // getLength. See https://code.google.com/p/skia/issues/detail?id=1570
 #if defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
-    test_stream_without_length(srcPath, codec, digest);
+    test_stream_without_length(srcPath, codec, bitmapAndDigest.fDigest);
 #endif
 
     if (writePath != NULL) {
-        SkString outPath;
-        make_outname(&outPath, writePath->c_str(), srcPath, ".png");
-        if (write_bitmap(outPath.c_str(), bitmap)) {
-            gSuccessfulDecodes.push_back().appendf("\twrote %s", outPath.c_str());
+        if (write_bitmap(writePath->c_str(), filename, bitmapAndDigest)) {
+            gSuccessfulDecodes.push_back().appendf("\twrote %s", filename);
         } else {
-            gEncodeFailures.push_back().set(outPath);
+            gEncodeFailures.push_back().set(filename);
         }
     }
 
-    write_expectations(bitmap, filename);
-
     if (FLAGS_testSubsetDecoding) {
         SkDEBUGCODE(bool couldRewind =) stream.rewind();
         SkASSERT(couldRewind);
@@ -514,24 +593,23 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
                 SkString subsetDim = SkStringPrintf("[%d,%d,%d,%d]", rect.fLeft, rect.fTop,
                                                     rect.fRight, rect.fBottom);
                 if (codec->decodeSubset(&bitmapFromDecodeSubset, rect, gPrefConfig)) {
-                    SkString subsetName = SkStringPrintf("%s_%s", filename, subsetDim.c_str());
-                    skiagm::GmResultDigest subsetDigest(bitmapFromDecodeSubset);
-                    if (compare_to_expectations_if_necessary(subsetDigest,
-                                                             subsetName.c_str(),
-                                                             &gFailedSubsetDecodes,
-                                                             &gMissingSubsetExpectations,
-                                                             &gKnownSubsetFailures)) {
+                    SkString subsetName = SkStringPrintf("%s-%s", filename, subsetDim.c_str());
+                    skiagm::BitmapAndDigest subsetBitmapAndDigest(bitmapFromDecodeSubset);
+                    if (write_and_compare_expectations(subsetBitmapAndDigest,
+                                                       subsetName.c_str(),
+                                                       &gFailedSubsetDecodes,
+                                                       &gMissingSubsetExpectations,
+                                                       &gKnownSubsetFailures)) {
                         gSuccessfulSubsetDecodes.push_back().printf("Decoded subset %s from %s",
                                                               subsetDim.c_str(), srcPath);
                     } else if (!FLAGS_mismatchPath.isEmpty()) {
-                        write_subset(FLAGS_mismatchPath[0], filename, subsetDim.c_str(),
-                                     &bitmapFromDecodeSubset, rect, bitmap);
+                        write_subset(FLAGS_mismatchPath[0], subsetName,
+                                     subsetBitmapAndDigest, rect, bitmap);
                     }
 
-                    write_expectations(bitmapFromDecodeSubset, subsetName.c_str());
                     if (writePath != NULL) {
-                        write_subset(writePath->c_str(), filename, subsetDim.c_str(),
-                                     &bitmapFromDecodeSubset, rect, bitmap);
+                        write_subset(writePath->c_str(), subsetName,
+                                     subsetBitmapAndDigest, rect, bitmap);
                     }
                 } else {
                     gFailedSubsetDecodes.push_back().printf("Failed to decode region %s from %s",