gm: display summary of all results by ErrorType

gm/gmmain.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.
  */
 
 /*
  * Code for the "gm" (Golden Master) rendering comparison tool.
  *
@@ skipping 68 matching lines @@
 
 #ifdef SK_BUILD_FOR_MAC
     #include "SkCGUtils.h"
     #define CAN_IMAGE_PDF   1
 #else
     #define CAN_IMAGE_PDF   0
 #endif
 
 using namespace skiagm;
 
-struct FailRec {
-    SkString    fName;
-    bool        fIsPixelError;
-
-    FailRec() : fIsPixelError(false) {}
-    FailRec(const SkString& name) : fName(name), fIsPixelError(false) {}
-};
-
 class Iter {
 public:
     Iter() {
         this->reset();
     }
 
     void reset() {
         fReg = GMRegistry::Head();
     }
 
@@ skipping 76 matching lines @@
 };
 
 class GMMain {
 public:
     GMMain() {
         // Set default values of member variables, which tool_main()
         // may override.
         fUseFileHierarchy = false;
         fIgnorableErrorCombination.add(kMissingExpectations_ErrorType);
         fMismatchPath = NULL;
+        fTestsRun = 0;
     }
 
     SkString make_name(const char shortName[], const char configName[]) {
         SkString name;
         if (0 == strlen(configName)) {
             name.append(shortName);
         } else if (fUseFileHierarchy) {
             name.appendf("%s%c%s", configName, SkPATH_SEPARATOR, shortName);
         } else {
             name.appendf("%s_%s", shortName, configName);
@@ skipping 33 matching lines @@
         // TODO(epoger): Now that we have removed force_all_opaque()
         // from this method, we should be able to get rid of the
         // transformation to 8888 format also.
         SkBitmap copy;
         bitmap.copyTo(&copy, SkBitmap::kARGB_8888_Config);
         return SkImageEncoder::EncodeFile(path.c_str(), copy,
                                           SkImageEncoder::kPNG_Type, 100);
     }
 
     /**
-     * Records the errors encountered in fFailedTests, except for any error
-     * types we want to ignore.
+     * Records the results of this test in fTestsRun and fFailedTests.
+     *
+     * We even record successes, and errors that we regard as
+     * "ignorable"; we can filter them out later.
      */
-    void RecordError(const ErrorCombination& errorCombination, const SkString& name,
-                     const char renderModeDescriptor []) {
-        // The common case: no error means nothing to record.
+    void RecordTestResults(const ErrorCombination& errorCombination, const SkString& name,
+                           const char renderModeDescriptor []) {
+        fTestsRun++;
         if (errorCombination.isEmpty()) {
             return;
         }
-
-        // If only certain error type(s) were reported, we know we can ignore them.
-        if (errorCombination.minus(fIgnorableErrorCombination).isEmpty()) {
-            return;
-        }
-
-        FailRec& rec = fFailedTests.push_back(make_name(name.c_str(), renderModeDescriptor));
-        rec.fIsPixelError = errorCombination.includes(kImageMismatch_ErrorType);
-    }
-
-    // List contents of fFailedTests via SkDebug.
-    void ListErrors() {
-        for (int i = 0; i < fFailedTests.count(); ++i) {
-            if (fFailedTests[i].fIsPixelError) {
-                gm_fprintf(stderr, "\t\t%s pixel_error\n", fFailedTests[i].fName.c_str());
-            } else {
-                gm_fprintf(stderr, "\t\t%s\n", fFailedTests[i].fName.c_str());
+        SkString fullName = make_name(name.c_str(), renderModeDescriptor);
+        for (int typeInt = 0; typeInt <= kLast_ErrorType; typeInt++) {
+            ErrorType type = static_cast<ErrorType>(typeInt);
+            if (errorCombination.includes(type)) {
+                fFailedTests[type].push_back(fullName);
             }
         }
     }
 
+    /**
+     * Return the number of significant (non-ignorable) errors we have
+     * encountered so far.
+     */
+    int NumSignificantErrors() {
+        int significantErrors = 0;
+        for (int typeInt = 0; typeInt <= kLast_ErrorType; typeInt++) {
+            ErrorType type = static_cast<ErrorType>(typeInt);
+            if (!fIgnorableErrorCombination.includes(type)) {
+                significantErrors += fFailedTests[type].count();
+            }
+        }
+        return significantErrors;
+    }
+
+    /**
+     * List contents of fFailedTests to stdout.
+     */
+    void ListErrors() {
+        // First, print a single summary line.
+        SkString summary;
+        summary.appendf("Ran %d tests:", fTestsRun);
+        for (int typeInt = 0; typeInt <= kLast_ErrorType; typeInt++) {
+            ErrorType type = static_cast<ErrorType>(typeInt);
+            summary.appendf(" %s=%d", getErrorTypeName(type), fFailedTests[type].count());
+        }
+        gm_fprintf(stdout, "%s\n", summary.c_str());
+
+        // Now, for each failure type, list the tests that failed that way.
+        for (int typeInt = 0; typeInt <= kLast_ErrorType; typeInt++) {
+            SkString line;
+            ErrorType type = static_cast<ErrorType>(typeInt);
+            if (fIgnorableErrorCombination.includes(type)) {
+                line.append("[ ] ");
+            } else {
+                line.append("[*] ");
+            }
+
+            SkTArray<SkString> *failedTestsOfThisType = &fFailedTests[type];
+            int count = failedTestsOfThisType->count();
+            line.appendf("%d %s:", count, getErrorTypeName(type));
+            for (int i = 0; i < count; ++i) {
+                line.append(" ");
+                line.append((*failedTestsOfThisType)[i]);
+            }
+            gm_fprintf(stdout, "%s\n", line.c_str());
+        }
+        gm_fprintf(stdout, "(results marked with [*] will cause nonzero return value)\n");
+    }
+
     static bool write_document(const SkString& path,
                                const SkDynamicMemoryWStream& document) {
         SkFILEWStream stream(path.c_str());
         SkAutoDataUnref data(document.copyToData());
         return stream.writeData(data.get());
     }
 
     /**
      * Prepare an SkBitmap to render a GM into.
      *
@@ skipping 217 matching lines @@
         if (kXPS_Backend == gRec.fBackend) {
             path = make_filename(writePath, renderModeDescriptor, name.c_str(),
                                  "xps");
             success = write_document(path, *document);
         }
         if (success) {
             return kEmpty_ErrorCombination;
         } else {
             gm_fprintf(stderr, "FAILED to write %s\n", path.c_str());
             ErrorCombination errors(kWritingReferenceImage_ErrorType);
-            RecordError(errors, name, renderModeDescriptor);
+            // TODO(epoger): Don't call RecordTestResults() here...

[epoger, 2013/04/02 16:00:14]

This TODO indicates a relatively minor problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

+            // Instead, we should make sure to call RecordTestResults
+            // exactly ONCE per test.  (Otherwise, gmmain.fTestsRun
+            // will be incremented twice for this test: once in
+            // compare_test_results_to_stored_expectations() before
+            // that method calls this one, and again here.)
+            //
+            // When we make that change, we should probably add a
+            // WritingReferenceImage test to the gm self-tests.)
+            RecordTestResults(errors, name, renderModeDescriptor);
             return errors;
         }
     }
 
     /**
      * Log more detail about the mistmatch between expectedBitmap and
      * actualBitmap.
      */
     void report_bitmap_diffs(const SkBitmap& expectedBitmap, const SkBitmap& actualBitmap,
                              const char *testName) {
@@ skipping 91 matching lines @@
                 write_bitmap(path, actualBitmap);
             }
 
             // If we have access to a single expected bitmap, log more
             // detail about the mismatch.
             const SkBitmap *expectedBitmapPtr = expectations.asBitmap();
             if (NULL != expectedBitmapPtr) {
                 report_bitmap_diffs(*expectedBitmapPtr, actualBitmap, completeName);
             }
         }
-        RecordError(errors, baseNameString, renderModeDescriptor);
+        RecordTestResults(errors, baseNameString, renderModeDescriptor);
 
         if (addToJsonSummary) {
             add_actual_results_to_json_summary(completeName, actualChecksum, errors,
                                                expectations.ignoreFailure());
             add_expected_results_to_json_summary(completeName, expectations);
         }
 
         return errors;
     }
 
@@ skipping 95 matching lines @@
             errors.add(compare_to_expectations(expectations, actualBitmap,
                                                name, "", true));
         } else {
             // If we are running without expectations, we still want to
             // record the actual results.
             Checksum actualChecksum =
                 SkBitmapChecksummer::Compute64(actualBitmap);
             add_actual_results_to_json_summary(name.c_str(), actualChecksum,
                                                ErrorCombination(kMissingExpectations_ErrorType),
                                                false);
+            RecordTestResults(ErrorCombination(kMissingExpectations_ErrorType), name, "");
         }
 
         // TODO: Consider moving this into compare_to_expectations(),
         // similar to fMismatchPath... for now, we don't do that, because
         // we don't want to write out the actual bitmaps for all
         // renderModes of all tests!  That would be a lot of files.
         if (writePath && (gRec.fFlags & kWrite_ConfigFlag)) {
             errors.add(write_reference_image(gRec, writePath, "",
                                              name, actualBitmap, pdf));
         }
 
         return errors;
     }
 
     /**
      * Compare actualBitmap to referenceBitmap.
      *
      * @param gm which test generated the bitmap
      * @param gRec
      * @param renderModeDescriptor
      * @param actualBitmap actual bitmap generated by this run
      * @param referenceBitmap bitmap we expected to be generated
      */
     ErrorCombination compare_test_results_to_reference_bitmap(
         GM* gm, const ConfigData& gRec, const char renderModeDescriptor [],
         SkBitmap& actualBitmap, const SkBitmap* referenceBitmap) {
 
+        // TODO(epoger): This method is run to compare results across
+        // different rendering modes (as opposed to
+        // compare_test_results_to_stored_expectations(), which
+        // compares results against expectations stored on disk).  If
+        // we would like the GenerateGMs step to distinguish between
+        // those two types of mismatches, we should report image
+        // mismatches in here with a different ErrorType.

[borenet, 2013/03/29 05:55:14]

The number of TODOs in this CL is worrisome to me.  Are these things you plan on
addressing very soon?

[epoger, 2013/04/02 16:00:14]

I have added a review comment at each TODO...

This TODO indicates a problem that already existed before this CL, and a
proposed solution that I intend to implement within the next week (I think it
should be in a separate CL).

         SkASSERT(referenceBitmap);
         SkString name = make_name(gm->shortName(), gRec.fName);
         Expectations expectations(*referenceBitmap);
         return compare_to_expectations(expectations, actualBitmap,
                                        name, renderModeDescriptor);
     }
 
     static SkPicture* generate_new_picture(GM* gm, BbhType bbhType, uint32_t recordFlags,
                                            SkScalar scale = SK_Scalar1) {
         // Pictures are refcounted so must be on heap
@@ skipping 95 matching lines @@
             // -deferred image, we exit early!  We should fix this
             // ASAP, because it is hiding -deferred errors... but for
             // now, I'm leaving the logic as it is so that the
             // refactoring change
             // https://codereview.chromium.org/12992003/ is unblocked.
             //
             // Filed as https://code.google.com/p/skia/issues/detail?id=1180
             // ('image-surface gm test is failing in "deferred" mode,
             // and gm is not reporting the failure')
             if (errors.isEmpty()) {
+                // TODO(epoger): Report this as a new ErrorType,
+                // something like kImageGeneration_ErrorType?

[epoger, 2013/04/02 16:00:14]

This TODO indicates a relatively minor problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

                 return kEmpty_ErrorCombination;
             }
             return compare_test_results_to_reference_bitmap(
                 gm, gRec, "-deferred", bitmap, &referenceBitmap);
         }
         return kEmpty_ErrorCombination;
     }
 
     ErrorCombination test_pipe_playback(GM* gm,
                                         const ConfigData& gRec,
@@ skipping 53 matching lines @@
     //
     // member variables.
     // They are public for now, to allow easier setting by tool_main().
     //
 
     bool fUseFileHierarchy;
     ErrorCombination fIgnorableErrorCombination;
 
     const char* fMismatchPath;
 
-    // information about all failed tests we have encountered so far
-    SkTArray<FailRec> fFailedTests;
+    // collection of tests that have failed with each ErrorType
+    SkTArray<SkString> fFailedTests[kLast_ErrorType+1];
+    int fTestsRun;
 
     // Where to read expectations (expected image checksums, etc.) from.
     // If unset, we don't do comparisons.
     SkAutoTUnref<ExpectationsSource> fExpectationsSource;
 
     // JSON summaries that we generate as we go (just for output).
     Json::Value fJsonExpectedResults;
     Json::Value fJsonActualResults_Failed;
     Json::Value fJsonActualResults_FailureIgnored;
     Json::Value fJsonActualResults_NoComparison;
@@ skipping 290 matching lines @@
  */
 ErrorCombination run_multiple_modes(GMMain &gmmain, GM *gm, const ConfigData &compareConfig,
                                     const SkBitmap &comparisonBitmap,
                                     const SkTDArray<SkScalar> &tileGridReplayScales);
 ErrorCombination run_multiple_modes(GMMain &gmmain, GM *gm, const ConfigData &compareConfig,
                                     const SkBitmap &comparisonBitmap,
                                     const SkTDArray<SkScalar> &tileGridReplayScales) {
     ErrorCombination errorsForAllModes;
     uint32_t gmFlags = gm->getFlags();
 
-    // run the picture centric GM steps
+    // TODO(epoger): We should start recording any per-GM skipped

[epoger, 2013/04/02 16:00:14]

This TODO indicates a problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

+    // modes (i.e. those we skipped due to gmFlags) with a new
+    // ErrorType, perhaps named kIntentionallySkipped_ErrorType.
     if (!(gmFlags & GM::kSkipPicture_Flag)) {
 
         ErrorCombination pictErrors;
 
         //SkAutoTUnref<SkPicture> pict(generate_new_picture(gm));
         SkPicture* pict = gmmain.generate_new_picture(gm, kNone_BbhType, 0);
         SkAutoUnref aur(pict);
 
         if (FLAGS_replay) {
             SkBitmap bitmap;
@@ skipping 73 matching lines @@
         if ((pipeErrors.isEmpty()) &&
             FLAGS_tiledPipe && !(gmFlags & GM::kSkipTiled_Flag)) {
             pipeErrors.add(gmmain.test_tiled_pipe_playback(gm, compareConfig, comparisonBitmap));
         }
 
         errorsForAllModes.add(pipeErrors);
     }
     return errorsForAllModes;
 }
 
+/**
+ * Return a list of all entries in an array of strings as a single string
+ * of this form:
+ * "item1", "item2", "item3"
+ */
+SkString list_all(const SkTArray<SkString> &stringArray);
+SkString list_all(const SkTArray<SkString> &stringArray) {
+    SkString total;
+    for (int i = 0; i < stringArray.count(); i++) {
+        if (i > 0) {
+            total.append(", ");
+        }
+        total.append("\"");
+        total.append(stringArray[i]);
+        total.append("\"");
+    }
+    return total;
+}
+
+/**
+ * Return a list of configuration names, as a single string of this form:
+ * "item1", "item2", "item3"
+ *
+ * @param configs configurations, as a list of indices into gRec
+ */
+SkString list_all_config_names(const SkTDArray<size_t> &configs);
+SkString list_all_config_names(const SkTDArray<size_t> &configs) {
+    SkString total;
+    for (int i = 0; i < configs.count(); i++) {
+        if (i > 0) {
+            total.append(", ");
+        }
+        total.append("\"");
+        total.append(gRec[configs[i]].fName);
+        total.append("\"");
+    }
+    return total;
+}
+
 int tool_main(int argc, char** argv);
 int tool_main(int argc, char** argv) {
 
 #if SK_ENABLE_INST_COUNT
     gPrintInstCount = true;
 #endif
 
     SkGraphics::Init();
     // we don't need to see this during a run
     gSkSuppressFontCachePurgeSpew = true;
@@ skipping 161 matching lines @@
         gm_fprintf(stderr, "reading resources from %s\n", FLAGS_resourcePath[0]);
     }
 
     if (moduloDivisor <= 0) {
         moduloRemainder = -1;
     }
     if (moduloRemainder < 0 || moduloRemainder >= moduloDivisor) {
         moduloRemainder = -1;
     }
 
-    // Accumulate success of all tests.
-    int testsRun = 0;
-    int testsPassed = 0;
-    int testsFailed = 0;
-    int testsMissingReferenceImages = 0;
-
+    int gmsRun = 0;
     int gmIndex = -1;
     SkString moduloStr;
 
     // If we will be writing out files, prepare subdirectories.
     if (FLAGS_writePath.count() == 1) {
         if (!sk_mkdir(FLAGS_writePath[0])) {
             return -1;
         }
         if (gmmain.fUseFileHierarchy) {
             for (int i = 0; i < configs.count(); i++) {
@@ skipping 19 matching lines @@
             }
             moduloStr.printf("[%d.%d] ", gmIndex, moduloDivisor);
         }
 
         const char* shortName = gm->shortName();
         if (skip_name(FLAGS_match, shortName)) {
             SkDELETE(gm);
             continue;
         }
 
+        gmsRun++;
         SkISize size = gm->getISize();
         gm_fprintf(stdout, "%sdrawing... %s [%d %d]\n", moduloStr.c_str(), shortName,
                    size.width(), size.height());
 
-        ErrorCombination testErrors;
-        testErrors.add(run_multiple_configs(gmmain, gm, configs, grFactory));
+        run_multiple_configs(gmmain, gm, configs, grFactory);
 
         SkBitmap comparisonBitmap;
         const ConfigData compareConfig =
             { SkBitmap::kARGB_8888_Config, kRaster_Backend, kDontCare_GLContextType, 0, kRW_ConfigFlag, "comparison", false };
-        testErrors.add(gmmain.generate_image(
-            gm, compareConfig, NULL, NULL, &comparisonBitmap, false));
+        gmmain.generate_image(gm, compareConfig, NULL, NULL, &comparisonBitmap, false);
 
         // TODO(epoger): only run this if gmmain.generate_image() succeeded?
         // Otherwise, what are we comparing against?
-        testErrors.add(run_multiple_modes(gmmain, gm, compareConfig, comparisonBitmap,
-                                          tileGridReplayScales));
-
-        // Update overall results.
-        // We only tabulate the particular error types that we currently
-        // care about (e.g., missing reference images). Later on, if we
-        // want to also tabulate other error types, we can do so.
-        testsRun++;
-        if (!gmmain.fExpectationsSource.get() ||
-            (testErrors.includes(kMissingExpectations_ErrorType))) {
-            testsMissingReferenceImages++;
-        }
-        if (testErrors.minus(gmmain.fIgnorableErrorCombination).isEmpty()) {
-            testsPassed++;
-        } else {
-            testsFailed++;
-        }
+        run_multiple_modes(gmmain, gm, compareConfig, comparisonBitmap, tileGridReplayScales);
 
         SkDELETE(gm);
     }
-    gm_fprintf(stdout, "Ran %d tests: %d passed, %d failed, %d missing reference images\n",
-               testsRun, testsPassed, testsFailed, testsMissingReferenceImages);
+
+    // Assemble the list of modes we ran each test through.
+    //
+    // TODO(epoger): Instead of assembling this list of modes here,

[epoger, 2013/04/02 16:00:14]

This TODO indicates something suboptimal about code added in *this* CL.  Do you
think the suggested remedy is good?  If so, I'll make the change within this
CL...

[borenet, 2013/04/02 16:17:56]

Sounds good to me.

[epoger, 2013/04/03 03:56:50]

Done in patchset 5.  Do you think this change is positive, on the whole?  I'm
torn.  (The new way is automated, but incurs some runtime performance cost I
have not measured.)

+    // can/should we assemble it as we actually run the tests in
+    // run_multiple_modes()?
+    SkTArray<SkString> modes;
+    if (FLAGS_replay) {
+        modes.push_back(SkString("replay"));
+    }
+    if (FLAGS_serialize) {
+        modes.push_back(SkString("serialize"));
+    }
+    if (FLAGS_rtree) {
+        modes.push_back(SkString("rtree"));
+    }
+    if (FLAGS_tileGrid) {
+        for (int i = 0; i < tileGridReplayScales.count(); i++) {
+            SkString modeName("tileGrid");
+            modeName.appendf("%f", tileGridReplayScales[i]);
+            modes.push_back(modeName);
+        }
+    }
+    if (FLAGS_pipe) {
+        for (size_t i = 0; i < SK_ARRAY_COUNT(gPipeWritingFlagCombos); i++) {
+            SkString modeName("pipe");
+            modeName.append(gPipeWritingFlagCombos[i].name);
+            modes.push_back(modeName);
+        }
+    }
+    if (FLAGS_tiledPipe) {
+        for (size_t i = 0; i < SK_ARRAY_COUNT(gPipeWritingFlagCombos); i++) {
+            SkString modeName("tiledPipe");
+            modeName.append(gPipeWritingFlagCombos[i].name);
+            modes.push_back(modeName);
+        }
+    }
+
+    // Output summary to stdout.
+    gm_fprintf(stdout, "Ran %d GMs, each with %d configs [%s] and %d modes [%s],"
+               " so there should be a total of %d tests\n",
+               gmsRun, configs.count(), list_all_config_names(configs).c_str(),
+               modes.count(), list_all(modes).c_str(), gmsRun * (configs.count() + modes.count()));
+    // TODO(epoger): Ultimately, we should signal an error if the

[epoger, 2013/04/02 16:00:14]

This TODO indicates a problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

+    // expected total number of tests (displayed above) does not match
+    // gmmain.fTestsRun.  But for now, there are cases where those
+    // numbers won't match: specifically, if some configs/modes are
+    // skipped on a per-GM basis (due to gm->getFlags() for a specific
+    // GM).  Later on, we should record tests like that using some new
+    // ErrorType, like kIntentionallySkipped_ErrorType.  Then we could
+    // signal an error if the totals didn't match up.
     gmmain.ListErrors();
 
     if (FLAGS_writeJsonSummaryPath.count() == 1) {
         Json::Value actualResults;
         actualResults[kJsonKey_ActualResults_Failed] =
             gmmain.fJsonActualResults_Failed;
         actualResults[kJsonKey_ActualResults_FailureIgnored] =
             gmmain.fJsonActualResults_FailureIgnored;
         actualResults[kJsonKey_ActualResults_NoComparison] =
             gmmain.fJsonActualResults_NoComparison;
@@ skipping 19 matching lines @@
             gm_fprintf(stdout, "config: %s %x\n", config.fName, gr);
             gr->printCacheStats();
         }
     }
 #endif
 
     delete grFactory;
 #endif
     SkGraphics::Term();
 
-    return (0 == testsFailed) ? 0 : -1;
+    return (0 == gmmain.NumSignificantErrors()) ? 0 : -1;
 }
 
 void GMMain::installFilter(SkCanvas* canvas) {
     if (FLAGS_forceBWtext) {
         canvas->setDrawFilter(SkNEW(BWTextDrawFilter))->unref();
     }
 }
 
 #if !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_NACL)
 int main(int argc, char * const argv[]) {
     return tool_main(argc, (char**) argv);
 }
 #endif