More work to integrate skimage with rebaseline tools.

tools/skimage_main.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "gm_expectations.h"
 #include "SkBitmap.h"
 #include "SkColorPriv.h"
@@ skipping 13 matching lines @@
 
 DEFINE_string(config, "None", "Preferred config to decode into. [None|8888|565|A8]");
 DEFINE_string(createExpectationsPath, "", "Path to write JSON expectations.");
 DEFINE_string(mismatchPath, "", "Folder to write mismatched images to.");
 DEFINE_string2(readPath, r, "", "Folder(s) and files to decode images. Required.");
 DEFINE_string(readExpectationsPath, "", "Path to read JSON expectations from.");
 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 {
     SkImageEncoder::Type    fType;
     SkImageDecoder::Format  fFormat;
     const char*             fSuffix;
 };
 
 static const Format gFormats[] = {
     { SkImageEncoder::kBMP_Type, SkImageDecoder::kBMP_Format, ".bmp" },
@@ skipping 34 matching lines @@
 
 static void make_outname(SkString* dst, const char outDir[], const char src[],
                          const char suffix[]) {
     SkString basename = SkOSPath::SkBasename(src);
     dst->set(SkOSPath::SkPathJoin(outDir, basename.c_str()));
     if (!dst->endsWith(suffix)) {
         const char* cstyleDst = dst->c_str();
         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);
     }
 }
 
 // Store the names of the filenames to report later which ones failed, succeeded, and were
 // invalid.
 // FIXME: Add more arrays, for more specific types of errors, and make the output simpler.
 // If each array holds one type of error, the output can change from:
 //
@@ skipping 23 matching lines @@
 // For files that are expected to fail.
 static SkTArray<SkString, false> gKnownFailures;
 static SkTArray<SkString, false> gKnownSubsetFailures;
 
 static SkBitmap::Config gPrefConfig(SkBitmap::kNo_Config);
 
 // Expections read from a file specified by readExpectationsPath. The expectations must have been
 // 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;
     }
 
     if (bm.config() == SkBitmap::kARGB_8888_Config) {
         // First attempt at encoding failed, and the bitmap was already 8888. Making
         // a copy is not going to help.
         return false;
     }
 
     // Encoding failed. Copy to 8888 and try again.
     SkBitmap bm8888;
     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);
 }
 
 /**
  *  Return a random SkIRect inside the range specified.
  *  @param rand Random number generator.
  *  @param maxX Exclusive maximum x-coordinate. SkIRect's fLeft and fRight will be
  *      in the range [0, maxX)
  *  @param maxY Exclusive maximum y-coordinate. SkIRect's fTop and fBottom will be
  *      in the range [0, maxY)
  *  @return SkIRect Non-empty, non-degenerate rectangle.
@@ skipping 23 matching lines @@
             rect.fTop--;
         } else {
             rect.fBottom++;
             // Again, this must be in range.
             SkASSERT(rect.fBottom < maxY);
         }
     }
     return rect;
 }
 
+/**
+ *  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 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;
 
 /**
- *  If expectations are to be recorded, record the bitmap expectations into global
- *  expectations array.
- */
-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;
-    }
-}
-
-/**
- *  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.
  *  @param ignoreArray Array to add failure message to when the image does not match
  *          the expectation, but this is a failure we can ignore.
  *  @return bool True in any of these cases:
  *                  - the bitmap matches the expectation.
  *               False in any of these cases:
  *                  - there is no expectations file.
  *                  - there is an expectations file, but no expectation for this bitmap.
  *                  - 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.",
                                              filename);
         }
         return false;
     }
 
     if (NULL == gJsonExpectations.get()) {
         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.",
                                              filename);
         }
         return false;
     }
 
     if (jsExpectation.match(digest)) {
         return true;
     }
 
     if (jsExpectation.ignoreFailure()) {
         ignoreArray->push_back().printf("%s does not match expectation, but this is known.",
                                         filename);
     } else if (failureArray != NULL) {
         failureArray->push_back().printf("decoded %s, but the result does not match "
                                          "expectations.",
                                          filename);
     }
     return false;
 }
 
 /**
  *  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;
+    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());
 
-    // 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());
+    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 %s from %s, but failed to "
-                                                "extract a similar subset for comparison.",
-                                                subsetDim, filename);
-        return false;
+        // 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 subset%s, but failed "
+                                                    "to create a directory for extractSubset "
+                                                    "comparison.",
+                                                    subsetName.c_str());
+            return false;
+        }
+
+        skiagm::BitmapAndDigest bitmapAndDigestFromExtractSubset(extractedSubset);
+        SkAssertResult(write_bitmap(dirExtracted.c_str(), subsetName.c_str(),
+                                    bitmapAndDigestFromExtractSubset));
     }
-
-    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;
-    }
-
-    make_outname(&outPath, dirExtracted.c_str(), filename, suffix.c_str());
-    SkAssertResult(write_bitmap(outPath.c_str(), extractedSubset));
     return true;
 }
 
 // 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)
 
 /**
  *  Dummy class for testing to ensure that a stream without a length decodes the same
  *  as a stream with a length.
@@ skipping 45 matching lines @@
                                             "a digest\n", srcPath);
         return;
     }
     if (!lengthLessDigest.equals(digest)) {
         gDecodeFailures.push_back().appendf("Without using getLength, %s did not match digest "
                                             "that uses getLength\n", srcPath);
     }
 }
 #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);
     if (!stream.isValid()) {
         gInvalidStreams.push_back().set(srcPath);
         return;
     }
 
     SkImageDecoder* codec = SkImageDecoder::Factory(&stream);
     if (NULL == codec) {
         gMissingCodecs.push_back().set(srcPath);
         return;
     }
 
     SkAutoTDelete<SkImageDecoder> ad(codec);
 
     codec->setSkipWritingZeroes(FLAGS_skip);
     codec->setSampleSize(FLAGS_sampleSize);
     stream.rewind();
 
     // 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(
                     "failed to decode %s, which is a known failure.", srcPath);
                 return;
             }
             if (jsExpectations.empty()) {
                 // This is a failure, but it is a new file. Mark it as missing, with
                 // a note that it should be marked failing.
                 gMissingExpectations.push_back().appendf(
@@ skipping 17 matching lines @@
             gDecodeFailures.push_back() = failure;
         } else {
             // Now check that the bounds match:
             if (dim.width() != bitmap.width() || dim.height() != bitmap.height()) {
                 SkString failure = SkStringPrintf("bounds do not match for %s", srcPath);
                 gDecodeFailures.push_back() = failure;
             }
         }
     }
 
-    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);
         int width, height;
         // Build the tile index for decoding subsets. If the image is 1x1, skip subset
         // decoding since there are no smaller subsets.
         if (codec->buildTileIndex(&stream, &width, &height) && width > 1 && height > 1) {
             SkASSERT(bitmap.width() == width && bitmap.height() == height);
             // Call decodeSubset multiple times:
             SkRandom rand(0);
             for (int i = 0; i < 5; i++) {
                 SkBitmap bitmapFromDecodeSubset;
                 // FIXME: Come up with a more representative set of rectangles.
                 SkIRect rect = generate_random_rect(&rand, width, height);
                 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",
                                                             subsetDim.c_str(), srcPath);
                 }
             }
         }
     }
 
     // Do not attempt to re-encode A8, since our image encoders do not support encoding to A8.
@@ skipping 204 matching lines @@
     print_strings("Known failures", gKnownFailures);
 
     return failed ? -1 : 0;
 }
 
 #if !defined SK_BUILD_FOR_IOS
 int main(int argc, char * const argv[]) {
     return tool_main(argc, (char**) argv);
 }
 #endif