Test decoders when hasLength returns false.

tools/skimage_main.cpp

Index: tools/skimage_main.cpp
diff --git a/tools/skimage_main.cpp b/tools/skimage_main.cpp
index 23fbd08a89c8733e249c776aeffcbc7797acecdf..854e0f36399639b6bae872045b5f3e9d90fb890b 100644
--- a/tools/skimage_main.cpp
+++ b/tools/skimage_main.cpp
@@ -184,7 +184,8 @@ static bool expect_to_fail(const char* filename) {
 
 /**
  *  Compare against an expectation for this filename, if there is one.
- *  @param bitmap SkBitmap to compare to the expected value.
+ *  @param digest GmResultDigest, computed from the decoded bitmap, to compare to the
+ *           expectation.
  *  @param filename String used to find the expected value.
  *  @param failureArray Array to add a failure message to on failure.
  *  @param missingArray Array to add missing expectation to on failure.
@@ -196,11 +197,11 @@ static bool expect_to_fail(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 SkBitmap& bitmap, const char* filename,
+static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& digest,
+                                                 const char* filename,
                                                  SkTArray<SkString, false>* failureArray,
                                                  SkTArray<SkString, false>* missingArray) {
-    skiagm::GmResultDigest resultDigest(bitmap);
-    if (!resultDigest.isValid()) {
+    if (!digest.isValid()) {
         if (failureArray != NULL) {
             failureArray->push_back().printf("decoded %s, but could not create a GmResultDigest.",
                                              filename);
@@ -221,7 +222,7 @@ static bool compare_to_expectations_if_necessary(const SkBitmap& bitmap, const c
         return false;
     }
 
-    if (jsExpectation.match(resultDigest)) {
+    if (jsExpectation.match(digest)) {
         return true;
     }
 
@@ -304,6 +305,68 @@ static bool write_subset(const char* writePath, const char* filename, const char
     return true;
 }
 
+// FIXME: This test could be run on windows/mac once we remove their dependence on
+// getLength.

[epoger, 2013/08/28 14:47:56]

Can we leave the code in place, but only CALL it on Android/Linux (ifdef at line
435)?  I'd rather keep our ifdef's at a minimum.

Also, is there a bug filed to track this functionality on Windows/Mac?  If so,
please reference it here.

[scroggo, 2013/08/28 17:54:38]

Won't our warnings complain that there is a static function which is never used?
I find it kind of nice that functions we won't call are not compiled to begin
with.
Now there is: 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.
+ */
+class FILEStreamWithoutLength : public SkFILEStream {
+public:
+    FILEStreamWithoutLength(const char path[])
+    : INHERITED(path) {}
+
+    virtual bool hasLength() const SK_OVERRIDE {
+        return false;
+    }
+
+private:
+    typedef SkFILEStream INHERITED;
+};
+
+/**
+ *  Test that decoding a stream which reports to not have a length still results in the
+ *  same image as if it did report to have a length. Assumes that codec was used to
+ *  successfully decode the file using SkFILEStream.
+ *  @param srcPath The path to the file, for recreating the length-less stream.
+ *  @param codec The SkImageDecoder originally used to decode srcPath, which will be used
+ *      again to decode the length-less stream.
+ *  @param digest GmResultDigest computed from decoding the stream the first time.
+ *      Decoding the length-less stream is expected to result in a matching digest.
+ */
+static void test_stream_without_length(const char srcPath[], SkImageDecoder* codec,
+                                       const skiagm::GmResultDigest& digest) {
+    if (!digest.isValid()) {
+        // An error was already reported.
+        return;
+    }
+    SkASSERT(srcPath);
+    SkASSERT(codec);
+    FILEStreamWithoutLength stream(srcPath);
+    // This will only be called after a successful decode. Creating a stream from the same
+    // path should never fail.
+    SkASSERT(stream.isValid());
+    SkBitmap bm;
+    if (!codec->decode(&stream, &bm, gPrefConfig, SkImageDecoder::kDecodePixels_Mode)) {
+        gDecodeFailures.push_back().appendf("Without using getLength, %s failed to decode\n",
+                                            srcPath);
+        return;
+    }
+    skiagm::GmResultDigest lengthLessDigest(bm);
+    if (!lengthLessDigest.isValid()) {
+        gDecodeFailures.push_back().appendf("Without using getLength, %s failed to build "
+                                            "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)
+
 static void decodeFileAndWrite(const char srcPath[], const SkString* writePath) {
     SkBitmap bitmap;
     SkFILEStream stream(srcPath);
@@ -353,7 +416,8 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
         }
     }
 
-    if (compare_to_expectations_if_necessary(bitmap, filename,
+    skiagm::BitmapAndDigest bandd(bitmap);

[epoger, 2013/08/28 14:47:56]

Minor suggestion: maybe "bitmapAndDigest" instead of "bandd"?  (It took me a few
seconds to realize what "bandd" meant)

[scroggo, 2013/08/28 17:54:38]

Done.

+    if (compare_to_expectations_if_necessary(bandd.fDigest, filename,
                                              &gDecodeFailures,
                                              &gMissingExpectations)) {
         gSuccessfulDecodes.push_back().printf("%s [%d %d]", srcPath, bitmap.width(),
@@ -368,6 +432,10 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
         }
     }
 
+#if defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
+    test_stream_without_length(srcPath, codec, bandd.fDigest);
+#endif
+
     if (writePath != NULL) {
         SkString outPath;
         make_outname(&outPath, writePath->c_str(), srcPath, ".png");
@@ -398,7 +466,8 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
                                                     rect.fRight, rect.fBottom);
                 if (codec->decodeSubset(&bitmapFromDecodeSubset, rect, gPrefConfig)) {
                     SkString subsetName = SkStringPrintf("%s_%s", filename, subsetDim.c_str());
-                    if (compare_to_expectations_if_necessary(bitmapFromDecodeSubset,
+                    skiagm::BitmapAndDigest subsetWithDigest(bitmapFromDecodeSubset);
+                    if (compare_to_expectations_if_necessary(subsetWithDigest.fDigest,

[scroggo, 2013/08/27 22:13:11]

I could have left this as bitmapFromDecodeSubset, since the constructor for
GmResultDigest was not marked explicit. Was that on purpose? Should I use the
implicit constructor?

[epoger, 2013/08/28 14:47:56]

I don't know what the guidelines are for using the implicit constructor or not. 
Personally, I like being explicit about it (as you were here), so it is as clear
as possible what's going on.

I don't understand why you create the BitmapAndDigest object, though, since you
never use it except to get its fDigest.  Maybe this would be simpler?

if (compare_to_expectations_if_necessary(
    GmResultDigest(bitmapFromDecodeSubset),
    subsetName.c_str(),
    ...

Or, using the implicit constructor, whichever you prefer.

[scroggo, 2013/08/28 17:54:38]

Oh right, I do not need a BitmapAndDigest. (In fact I never need the class in
this file, so I'm just sticking to GmResultDigest.) I would have created the
GmResultDigest inline, like you suggested, but that makes my long line even
longer.

                                                              subsetName.c_str(),
                                                              &gFailedSubsetDecodes,
                                                              &gMissingSubsetExpectations)) {