Test decoders when hasLength returns false.

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 166 matching lines @@
 static bool expect_to_fail(const char* filename) {
     if (NULL == gJsonExpectations.get()) {
         return false;
     }
     skiagm::Expectations jsExpectations = gJsonExpectations->get(filename);
     return jsExpectations.ignoreFailure();
 }
 
 /**
  *  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.
  *  @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 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);
         }
         return false;
     }
 
     if (NULL == gJsonExpectations.get()) {
         return false;
     }
 
     skiagm::Expectations jsExpectation = gJsonExpectations->get(filename);
     if (jsExpectation.empty()) {
         if (missingArray != NULL) {
             missingArray->push_back().printf("decoded %s, but could not find expectation.",
                                              filename);
         }
         return false;
     }
 
-    if (jsExpectation.match(resultDigest)) {
+    if (jsExpectation.match(digest)) {
         return true;
     }
 
     if (failureArray != NULL) {
         failureArray->push_back().printf("decoded %s, but the result does not match "
                                          "expectations.",
                                          filename);
     }
     return false;
 }
@@ skipping 62 matching lines @@
                                                 "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.

[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);
     if (!stream.isValid()) {
         gInvalidStreams.push_back().set(srcPath);
         return;
     }
 
     SkImageDecoder* codec = SkImageDecoder::Factory(&stream);
     if (NULL == codec) {
@@ skipping 29 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;
             }
         }
     }
 
-    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(),
                                               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());
         } else {
             gEncodeFailures.push_back().set(outPath);
         }
     }
 
+#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");
         if (write_bitmap(outPath.c_str(), bitmap)) {
             gSuccessfulDecodes.push_back().appendf("\twrote %s", outPath.c_str());
         } else {
             gEncodeFailures.push_back().set(outPath);
         }
     }
 
@@ skipping 10 matching lines @@
             // 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());
-                    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)) {
                         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);
                     }
 
@@ skipping 214 matching lines @@
     }
 
     return failed ? -1 : 0;
 }
 
 #if !defined SK_BUILD_FOR_IOS
 int main(int argc, char * const argv[]) {
     return tool_main(argc, (char**) argv);
 }
 #endif