gm: change ErrorBitfield to ErrorType/ErrorCombination

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.
  *
  * If you make changes to this, re-run the self-tests at gm/tests/run.sh
  * to make sure they still pass... you may need to change the expected
  * results of the self-test.
  */
 
 #include "gm.h"
+#include "gm_error.h"
 #include "gm_expectations.h"
 #include "system_preferences.h"
 #include "SkBitmap.h"
 #include "SkBitmapChecksummer.h"
 #include "SkColorPriv.h"
 #include "SkCommandLineFlags.h"
 #include "SkData.h"
 #include "SkDeferredCanvas.h"
 #include "SkDevice.h"
 #include "SkDrawFilter.h"
@@ skipping 46 matching lines @@
     #include "SkXPSDevice.h"
 #endif
 
 #ifdef SK_BUILD_FOR_MAC
     #include "SkCGUtils.h"
     #define CAN_IMAGE_PDF   1
 #else
     #define CAN_IMAGE_PDF   0
 #endif
 
-typedef int ErrorBitfield;
-// an empty bitfield means no errors:
-const static ErrorBitfield kEmptyErrorBitfield                 = 0x00;
-// individual error types:
-const static ErrorBitfield kNoGpuContext_ErrorBitmask          = 0x01;
-const static ErrorBitfield kImageMismatch_ErrorBitmask         = 0x02;
-const static ErrorBitfield kMissingExpectations_ErrorBitmask   = 0x04;
-const static ErrorBitfield kWritingReferenceImage_ErrorBitmask = 0x08;
-// we typically ignore any errors matching this bitmask:
-const static ErrorBitfield kIgnorable_ErrorBitmask = kMissingExpectations_ErrorBitmask;
-
 using namespace skiagm;
 
 struct FailRec {
     SkString    fName;
     bool        fIsPixelError;
 
     FailRec() : fIsPixelError(false) {}
     FailRec(const SkString& name) : fName(name), fIsPixelError(false) {}
 };
 
@@ skipping 84 matching lines @@
     { " cross-process, shared address", SkGPipeWriter::kCrossProcess_Flag
         | SkGPipeWriter::kSharedAddressSpace_Flag }
 };
 
 class GMMain {
 public:
     GMMain() {
         // Set default values of member variables, which tool_main()
         // may override.
         fUseFileHierarchy = false;
+        fIgnorableErrorCombination.add(kMissingExpectations_ErrorType);
         fMismatchPath = NULL;
     }
 
     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 {
@@ skipping 33 matching lines @@
     static bool write_bitmap(const SkString& path, const SkBitmap& bitmap) {
         // 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 an error in fFailedTests, if we want to record errors
-    // of this type.
-    void RecordError(ErrorBitfield errorType, const SkString& name,
+    /**
+     * Records the errors encountered in fFailedTests, except for any error
+     * types we want to ignore.
+     */
+    void RecordError(const ErrorCombination& errorCombination, const SkString& name,
                      const char renderModeDescriptor []) {
         // The common case: no error means nothing to record.
-        if (kEmptyErrorBitfield == errorType) {
+        if (errorCombination.isEmpty()) {
             return;
         }
 
         // If only certain error type(s) were reported, we know we can ignore them.
-        if (errorType == (errorType & kIgnorable_ErrorBitmask)) {
+        if (errorCombination.minus(fIgnorableErrorCombination).isEmpty()) {
             return;
         }
 
-        FailRec& rec = fFailedTests.push_back(make_name(
-            name.c_str(), renderModeDescriptor));
-        rec.fIsPixelError =
-            (kEmptyErrorBitfield != (errorType & kImageMismatch_ErrorBitmask));
+        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());
             }
@@ skipping 92 matching lines @@
 
         if (!isPDF) {
             canvas->concat(gm->getInitialTransform());
         }
         installFilter(canvas);
         gm->setCanvasIsDeferred(isDeferred);
         gm->draw(canvas);
         canvas->setDrawFilter(NULL);
     }
 
-    static ErrorBitfield generate_image(GM* gm, const ConfigData& gRec,
-                                        GrContext* context,
-                                        GrRenderTarget* rt,
-                                        SkBitmap* bitmap,
-                                        bool deferred) {
+    static ErrorCombination generate_image(GM* gm, const ConfigData& gRec,
+                                           GrContext* context,
+                                           GrRenderTarget* rt,
+                                           SkBitmap* bitmap,
+                                           bool deferred) {
         SkISize size (gm->getISize());
         setup_bitmap(gRec, size, bitmap);
 
         SkAutoTUnref<SkCanvas> canvas;
 
         if (gRec.fBackend == kRaster_Backend) {
             SkAutoTUnref<SkDevice> device(new SkDevice(*bitmap));
             if (deferred) {
                 canvas.reset(new SkDeferredCanvas(device));
             } else {
                 canvas.reset(new SkCanvas(device));
             }
             invokeGM(gm, canvas, false, deferred);
             canvas->flush();
         }
 #if SK_SUPPORT_GPU
         else {  // GPU
             if (NULL == context) {
-                return kNoGpuContext_ErrorBitmask;
+                return kNoGpuContext_ErrorType;

[borenet, 2013/03/22 13:53:29]

Should this be an instance of ErrorCombination, rather than an enum?

[epoger, 2013/03/22 14:31:23]

The compiler knows how to automatically convert it, thanks to the existence of
ErrorCombination::ErrorCombination(ErrorType).

If you prefer, I could make it explicit:
return ErrorCombination(kNoGpuContext_ErrorType)

Let me know if you'd like me to do that.

[borenet, 2013/03/22 14:37:12]

That would be my personal preference, but I'll leave it up to you.

[epoger, 2013/03/22 15:41:37]

Done here, and in one other place where it was also automatically being
converted.

             }
             SkAutoTUnref<SkDevice> device(new SkGpuDevice(context, rt));
             if (deferred) {
                 canvas.reset(new SkDeferredCanvas(device));
             } else {
                 canvas.reset(new SkCanvas(device));
             }
             invokeGM(gm, canvas, false, deferred);
             // the device is as large as the current rendertarget, so
             // we explicitly only readback the amount we expect (in
             // size) overwrite our previous allocation
             bitmap->setConfig(SkBitmap::kARGB_8888_Config, size.fWidth,
                               size.fHeight);
             canvas->readPixels(bitmap, 0, 0);
         }
 #endif
         complete_bitmap(bitmap);
-        return kEmptyErrorBitfield;
+        return kEmpty_ErrorCombination;
     }
 
     static void generate_image_from_picture(GM* gm, const ConfigData& gRec,
                                             SkPicture* pict, SkBitmap* bitmap,
                                             SkScalar scale = SK_Scalar1) {
         SkISize size = gm->getISize();
         setup_bitmap(gRec, size, bitmap);
         SkCanvas canvas(*bitmap);
         installFilter(&canvas);
         canvas.scale(scale, scale);
@@ skipping 48 matching lines @@
         SkCanvas c(dev);
         dev->beginPortfolio(&xps);
         dev->beginSheet(unitsPerMeter, pixelsPerMeter, trimSize);
         invokeGM(gm, &c, false, false);
         dev->endSheet();
         dev->endPortfolio();
 
 #endif
     }
 
-    ErrorBitfield write_reference_image(
-      const ConfigData& gRec, const char writePath [],
-      const char renderModeDescriptor [], const SkString& name,
-        SkBitmap& bitmap, SkDynamicMemoryWStream* document) {
+    ErrorCombination write_reference_image(const ConfigData& gRec, const char writePath [],
+                                           const char renderModeDescriptor [], const SkString& name,
+                                           SkBitmap& bitmap, SkDynamicMemoryWStream* document) {
         SkString path;
         bool success = false;
         if (gRec.fBackend == kRaster_Backend ||
             gRec.fBackend == kGPU_Backend ||
             (gRec.fBackend == kPDF_Backend && CAN_IMAGE_PDF)) {
 
             path = make_filename(writePath, renderModeDescriptor, name.c_str(),
                                  "png");
             success = write_bitmap(path, bitmap);
         }
         if (kPDF_Backend == gRec.fBackend) {
             path = make_filename(writePath, renderModeDescriptor, name.c_str(),
                                  "pdf");
             success = write_document(path, *document);
         }
         if (kXPS_Backend == gRec.fBackend) {
             path = make_filename(writePath, renderModeDescriptor, name.c_str(),
                                  "xps");
             success = write_document(path, *document);
         }
         if (success) {
-            return kEmptyErrorBitfield;
+            return kEmpty_ErrorCombination;
         } else {
             gm_fprintf(stderr, "FAILED to write %s\n", path.c_str());
-            RecordError(kWritingReferenceImage_ErrorBitmask, name,
-                        renderModeDescriptor);
-            return kWritingReferenceImage_ErrorBitmask;
+            ErrorCombination errors(kWritingReferenceImage_ErrorType);
+            RecordError(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) {
         const int expectedWidth = expectedBitmap.width();
@@ skipping 40 matching lines @@
                                                  (int)SkGetPackedA32(actualPixel)));
                 }
             }
         }
         gm_fprintf(stderr, "---- %s: %d (of %d) differing pixels,"
                    " max per-channel mismatch R=%d G=%d B=%d A=%d\n",
                    testName, differingPixels, width*height, errR, errG, errB, errA);
     }
 
     /**
-     * Compares actual checksum to expectations.  Returns
-     * kEmptyErrorBitfield if they match, or some combination of
-     * _ErrorBitmask values otherwise.
+     * Compares actual checksum to expectations, returning the set of errors
+     * (if any) that we saw along the way.
      *
      * If fMismatchPath has been set, and there are pixel diffs, then the
      * actual bitmap will be written out to a file within fMismatchPath.
      *
      * @param expectations what expectations to compare actualBitmap against
      * @param actualBitmap the image we actually generated
      * @param baseNameString name of test without renderModeDescriptor added
      * @param renderModeDescriptor e.g., "-rtree", "-deferred"
      * @param addToJsonSummary whether to add these results (both actual and
      *        expected) to the JSON summary
      *
      * TODO: For now, addToJsonSummary is only set to true within
      * compare_test_results_to_stored_expectations(), so results of our
      * in-memory comparisons (Rtree vs regular, etc.) are not written to the
      * JSON summary.  We may wish to change that.
      */
-    ErrorBitfield compare_to_expectations(Expectations expectations,
-                                          const SkBitmap& actualBitmap,
-                                          const SkString& baseNameString,
-                                          const char renderModeDescriptor[],
-                                          bool addToJsonSummary=false) {
-        ErrorBitfield retval;
+    ErrorCombination compare_to_expectations(Expectations expectations,
+                                             const SkBitmap& actualBitmap,
+                                             const SkString& baseNameString,
+                                             const char renderModeDescriptor[],
+                                             bool addToJsonSummary=false) {
+        ErrorCombination errors;
         Checksum actualChecksum = SkBitmapChecksummer::Compute64(actualBitmap);
         SkString completeNameString = baseNameString;
         completeNameString.append(renderModeDescriptor);
         const char* completeName = completeNameString.c_str();
 
         if (expectations.empty()) {
-            retval = kMissingExpectations_ErrorBitmask;
-        } else if (expectations.match(actualChecksum)) {
-            retval = kEmptyErrorBitfield;
-        } else {
-            retval = kImageMismatch_ErrorBitmask;
+            errors.add(kMissingExpectations_ErrorType);
+        } else if (!expectations.match(actualChecksum)) {
+            errors.add(kImageMismatch_ErrorType);
 
             // Write out the "actuals" for any mismatches, if we have
             // been directed to do so.
             if (fMismatchPath) {
                 SkString path =
                     make_filename(fMismatchPath, renderModeDescriptor,
                                   baseNameString.c_str(), "png");
                 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(retval, baseNameString, renderModeDescriptor);
+        RecordError(errors, baseNameString, renderModeDescriptor);
 
         if (addToJsonSummary) {
-            add_actual_results_to_json_summary(completeName, actualChecksum,
-                                               retval,
+            add_actual_results_to_json_summary(completeName, actualChecksum, errors,
                                                expectations.ignoreFailure());
             add_expected_results_to_json_summary(completeName, expectations);
         }
 
-        return retval;
+        return errors;
     }
 
     /**
      * Add this result to the appropriate JSON collection of actual results,
      * depending on status.
      */
     void add_actual_results_to_json_summary(const char testName[],
                                             Checksum actualChecksum,
-                                            ErrorBitfield result,
+                                            ErrorCombination result,
                                             bool ignoreFailure) {
         Json::Value actualResults;
         actualResults[kJsonKey_ActualResults_AnyStatus_Checksum] =
             asJsonValue(actualChecksum);
-        if (kEmptyErrorBitfield == result) {
+        if (result.isEmpty()) {
             this->fJsonActualResults_Succeeded[testName] = actualResults;
         } else {
             if (ignoreFailure) {
                 // TODO: Once we have added the ability to compare
                 // actual results against expectations in a JSON file
                 // (where we can set ignore-failure to either true or
                 // false), add test cases that exercise ignored
-                // failures (both for kMissingExpectations_ErrorBitmask
-                // and kImageMismatch_ErrorBitmask).
+                // failures (both for kMissingExpectations_ErrorType
+                // and kImageMismatch_ErrorType).
                 this->fJsonActualResults_FailureIgnored[testName] =
                     actualResults;
             } else {
-                if (kEmptyErrorBitfield != (result & kMissingExpectations_ErrorBitmask)) {
+                if (result.includes(kMissingExpectations_ErrorType)) {
                     // TODO: What about the case where there IS an
                     // expected image checksum, but that gm test
                     // doesn't actually run?  For now, those cases
                     // will always be ignored, because gm only looks
                     // at expectations that correspond to gm tests
                     // that were actually run.
                     //
                     // Once we have the ability to express
                     // expectations as a JSON file, we should fix this
                     // (and add a test case for which an expectation
                     // is given but the test is never run).
                     this->fJsonActualResults_NoComparison[testName] =
                         actualResults;
                 }
-                if (kEmptyErrorBitfield != (result & kImageMismatch_ErrorBitmask)) {
+                if (result.includes(kImageMismatch_ErrorType)) {
                     this->fJsonActualResults_Failed[testName] = actualResults;
                 }
             }
         }
     }
 
     /**
      * Add this test to the JSON collection of expected results.
      */
     void add_expected_results_to_json_summary(const char testName[],
@@ skipping 12 matching lines @@
     /**
      * Compare actualBitmap to expectations stored in this->fExpectationsSource.
      *
      * @param gm which test generated the actualBitmap
      * @param gRec
      * @param writePath unless this is NULL, write out actual images into this
      *        directory
      * @param actualBitmap bitmap generated by this run
      * @param pdf
      */
-    ErrorBitfield compare_test_results_to_stored_expectations(
+    ErrorCombination compare_test_results_to_stored_expectations(
         GM* gm, const ConfigData& gRec, const char writePath[],
         SkBitmap& actualBitmap, SkDynamicMemoryWStream* pdf) {
 
         SkString name = make_name(gm->shortName(), gRec.fName);
-        ErrorBitfield retval = kEmptyErrorBitfield;
+        ErrorCombination errors;
 
-        ExpectationsSource *expectationsSource =
-            this->fExpectationsSource.get();
+        ExpectationsSource *expectationsSource = this->fExpectationsSource.get();
         if (expectationsSource && (gRec.fFlags & kRead_ConfigFlag)) {
             /*
              * Get the expected results for this test, as one or more allowed
              * checksums. The current implementation of expectationsSource
              * get this by computing the checksum of a single PNG file on disk.
              *
              * TODO(epoger): This relies on the fact that
              * force_all_opaque() was called on the bitmap before it
              * was written to disk as a PNG in the first place. If
              * not, the checksum returned here may not match the
              * checksum of actualBitmap, which *has* been run through
              * force_all_opaque().
              * See comments above complete_bitmap() for more detail.
              */
             Expectations expectations = expectationsSource->get(name.c_str());
-            retval |= compare_to_expectations(expectations, actualBitmap,
-                                              name, "", true);
+            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,
-                                               kMissingExpectations_ErrorBitmask,
+                                               kMissingExpectations_ErrorType,
                                                false);
         }
 
         // 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)) {
-            retval |= write_reference_image(gRec, writePath, "",
-                                            name, actualBitmap, pdf);
+            errors.add(write_reference_image(gRec, writePath, "",
+                                             name, actualBitmap, pdf));
         }
 
-        return retval;
+        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
      */
-    ErrorBitfield compare_test_results_to_reference_bitmap(
+    ErrorCombination compare_test_results_to_reference_bitmap(
         GM* gm, const ConfigData& gRec, const char renderModeDescriptor [],
         SkBitmap& actualBitmap, const SkBitmap* referenceBitmap) {
 
         SkASSERT(referenceBitmap);
         SkString name = make_name(gm->shortName(), gRec.fName);
         Expectations expectations(*referenceBitmap);
         return compare_to_expectations(expectations, actualBitmap,
                                        name, renderModeDescriptor);
     }
 
@@ skipping 41 matching lines @@
         //char* dst = new char [streamSize];
         //@todo thudson 22 April 2011 when can we safely delete [] dst?
         storage.copyTo(dst);
         SkMemoryStream pictReadback(dst, streamSize);
         SkPicture* retval = new SkPicture (&pictReadback);
         return retval;
     }
 
     // Test: draw into a bitmap or pdf.
     // Depending on flags, possibly compare to an expected image.
-    ErrorBitfield test_drawing(GM* gm,
-                               const ConfigData& gRec,
-                               const char writePath [],
-                               GrContext* context,
-                               GrRenderTarget* rt,
-                               SkBitmap* bitmap) {
+    ErrorCombination test_drawing(GM* gm,
+                                  const ConfigData& gRec,
+                                  const char writePath [],
+                                  GrContext* context,
+                                  GrRenderTarget* rt,
+                                  SkBitmap* bitmap) {
         SkDynamicMemoryWStream document;
 
         if (gRec.fBackend == kRaster_Backend ||
             gRec.fBackend == kGPU_Backend) {
             // Early exit if we can't generate the image.
-            ErrorBitfield errors = generate_image(gm, gRec, context, rt, bitmap,
-                                                  false);
-            if (kEmptyErrorBitfield != errors) {
+            ErrorCombination errors = generate_image(gm, gRec, context, rt, bitmap, false);
+            if (!errors.isEmpty()) {
                 // TODO: Add a test to exercise what the stdout and
                 // JSON look like if we get an "early error" while
                 // trying to generate the image.
                 return errors;
             }
         } else if (gRec.fBackend == kPDF_Backend) {
             generate_pdf(gm, document);
 #if CAN_IMAGE_PDF
             SkAutoDataUnref data(document.copyToData());
             SkMemoryStream stream(data->data(), data->size());
             SkPDFDocumentToBitmap(&stream, bitmap);
 #endif
         } else if (gRec.fBackend == kXPS_Backend) {
             generate_xps(gm, document);
         }
         return compare_test_results_to_stored_expectations(
             gm, gRec, writePath, *bitmap, &document);
     }
 
-    ErrorBitfield test_deferred_drawing(GM* gm,
-                                        const ConfigData& gRec,
-                                        const SkBitmap& referenceBitmap,
-                                        GrContext* context,
-                                        GrRenderTarget* rt) {
+    ErrorCombination test_deferred_drawing(GM* gm,
+                                           const ConfigData& gRec,
+                                           const SkBitmap& referenceBitmap,
+                                           GrContext* context,
+                                           GrRenderTarget* rt) {
         SkDynamicMemoryWStream document;
 
         if (gRec.fBackend == kRaster_Backend ||
             gRec.fBackend == kGPU_Backend) {
             SkBitmap bitmap;
             // Early exit if we can't generate the image, but this is
             // expected in some cases, so don't report a test failure.
-            if (!generate_image(gm, gRec, context, rt, &bitmap, true)) {
-                return kEmptyErrorBitfield;
+            ErrorCombination errors = generate_image(gm, gRec, context, rt, &bitmap, true);
+            // TODO(epoger): This logic is the opposite of what is
+            // described above... if we succeeded in generating the
+            // -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.
+            if (errors.isEmpty()) {

[epoger, 2013/03/22 02:39:44]

Yikes, this "fixes" the "problem" revealed by the trybots... more accurately,
this preserves the earlier programming error that has been hiding -deferred
drawing errors for who-knows-how-long!!!

Before this CL, this code read:
            if (!generate_image(gm, gRec, context, rt, &bitmap, true)) {
                return kEmptyErrorBitfield;
            }

In other words... if generate_image() returned kEmptyErrorBitfield, then exit
early and report success (without comparing the generated image against
referenceBitmap)!

I will send this to a couple of trybots... I think they will succeed now.  By
still hiding these failures. :-(

[borenet, 2013/03/22 13:53:29]

File bugs!

[epoger, 2013/03/22 14:31:23]

Filed... I will update that bug, and assign it, once this CL is committed.

+                return kEmpty_ErrorCombination;
             }
             return compare_test_results_to_reference_bitmap(
                 gm, gRec, "-deferred", bitmap, &referenceBitmap);
         }
-        return kEmptyErrorBitfield;
+        return kEmpty_ErrorCombination;
     }
 
-    ErrorBitfield test_pipe_playback(GM* gm,
-                                     const ConfigData& gRec,
-                                     const SkBitmap& referenceBitmap) {
-        ErrorBitfield errors = kEmptyErrorBitfield;
+    ErrorCombination test_pipe_playback(GM* gm,
+                                        const ConfigData& gRec,
+                                        const SkBitmap& referenceBitmap) {
+        ErrorCombination errors;
         for (size_t i = 0; i < SK_ARRAY_COUNT(gPipeWritingFlagCombos); ++i) {
             SkBitmap bitmap;
             SkISize size = gm->getISize();
             setup_bitmap(gRec, size, &bitmap);
             SkCanvas canvas(bitmap);
             installFilter(&canvas);
             PipeController pipeController(&canvas);
             SkGPipeWriter writer;
             SkCanvas* pipeCanvas = writer.startRecording(
               &pipeController, gPipeWritingFlagCombos[i].flags);
             invokeGM(gm, pipeCanvas, false, false);
             complete_bitmap(&bitmap);
             writer.endRecording();
             SkString string("-pipe");
             string.append(gPipeWritingFlagCombos[i].name);
-            errors |= compare_test_results_to_reference_bitmap(
-                gm, gRec, string.c_str(), bitmap, &referenceBitmap);
-            if (errors != kEmptyErrorBitfield) {
+            errors.add(compare_test_results_to_reference_bitmap(
+                gm, gRec, string.c_str(), bitmap, &referenceBitmap));
+            if (!errors.isEmpty()) {
                 break;
             }
         }
         return errors;
     }
 
-    ErrorBitfield test_tiled_pipe_playback(
-      GM* gm, const ConfigData& gRec, const SkBitmap& referenceBitmap) {
-        ErrorBitfield errors = kEmptyErrorBitfield;
+    ErrorCombination test_tiled_pipe_playback(GM* gm, const ConfigData& gRec,
+                                              const SkBitmap& referenceBitmap) {
+        ErrorCombination errors;
         for (size_t i = 0; i < SK_ARRAY_COUNT(gPipeWritingFlagCombos); ++i) {
             SkBitmap bitmap;
             SkISize size = gm->getISize();
             setup_bitmap(gRec, size, &bitmap);
             SkCanvas canvas(bitmap);
             installFilter(&canvas);
             TiledPipeController pipeController(bitmap);
             SkGPipeWriter writer;
             SkCanvas* pipeCanvas = writer.startRecording(
               &pipeController, gPipeWritingFlagCombos[i].flags);
             invokeGM(gm, pipeCanvas, false, false);
             complete_bitmap(&bitmap);
             writer.endRecording();
             SkString string("-tiled pipe");
             string.append(gPipeWritingFlagCombos[i].name);
-            errors |= compare_test_results_to_reference_bitmap(
-                gm, gRec, string.c_str(), bitmap, &referenceBitmap);
-            if (errors != kEmptyErrorBitfield) {
+            errors.add(compare_test_results_to_reference_bitmap(
+                gm, gRec, string.c_str(), bitmap, &referenceBitmap));
+            if (!errors.isEmpty()) {
                 break;
             }
         }
         return errors;
     }
 
     //
     // 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;
 
     // Where to read expectations (expected image checksums, etc.) from.
     // If unset, we don't do comparisons.
     SkAutoTUnref<ExpectationsSource> fExpectationsSource;
 
@@ skipping 179 matching lines @@
     }
 }
 
 /**
  * Run this test in a number of different configs (8888, 565, PDF,
  * etc.), confirming that the resulting bitmaps match expectations
  * (which may be different for each config).
  *
  * Returns all errors encountered while doing so.
  */
-ErrorBitfield run_multiple_configs(GMMain &gmmain, GM *gm, const SkTDArray<size_t> &configs,
-                                   GrContextFactory *grFactory);
-ErrorBitfield run_multiple_configs(GMMain &gmmain, GM *gm, const SkTDArray<size_t> &configs,
-                                   GrContextFactory *grFactory) {
-    ErrorBitfield errorsForAllConfigs = kEmptyErrorBitfield;
+ErrorCombination run_multiple_configs(GMMain &gmmain, GM *gm, const SkTDArray<size_t> &configs,
+                                      GrContextFactory *grFactory, int gpuCacheSizeBytes,
+                                      int gpuCacheSizeCount);

[borenet, 2013/03/22 13:53:29]

This was previously hidden behind SK_SUPPORT_GPU.  How much do we care about
keeping it that way?

[epoger, 2013/03/22 14:31:23]

The problem: now that these values are passed as a function parameter, we would
have to put an #ifdef around part of the function declaration to keep them
hidden.  I'm leery of changing the signature of a function based on preprocessor
directives, but if you think that's the right approach I can do it.

I think it's cleaner to parse FLAGS_gpuCacheSize outside of this function, for 2
reasons:
1. I think it's good to validate user flags as early as possible
2. If the user flag validation fails within this function, we need to return
some new ErrorType (kFlagValidation_ErrorType ?)

One approach could be: I create a new GpuCacheDescription class that is used as
the parameter here (instead of the two integers).  If GPU is not supported,
GpuCacheDescription is defined as some class with all private accessors (so the
compiler will barf if the code tries to read it on non-GPU builds).

Should I try that approach, or do you have some other suggestion?

[borenet, 2013/03/22 14:37:12]

That IS ugly.
Makes sense.
The only other thing I can think of is to make those variables global, which is
arguably worse.  I'll leave it up to you.

[epoger, 2013/03/22 15:41:37]

I went with globals, because creating the new GpuCacheDescription class was
gonna get more complicated.  And besides, the flag values are already global, so
it doesn't seem any worse than it was before.

+ErrorCombination run_multiple_configs(GMMain &gmmain, GM *gm, const SkTDArray<size_t> &configs,
+                                      GrContextFactory *grFactory, int gpuCacheSizeBytes,
+                                      int gpuCacheSizeCount) {
+    ErrorCombination errorsForAllConfigs;
     uint32_t gmFlags = gm->getFlags();
 
-#if SK_SUPPORT_GPU
-    struct {
-        int     fBytes;
-        int     fCount;
-    } gpuCacheSize = { -1, -1 }; // -1s mean use the default
-
-    if (FLAGS_gpuCacheSize.count() > 0) {
-        if (FLAGS_gpuCacheSize.count() != 2) {
-            gm_fprintf(stderr, "--gpuCacheSize requires two arguments\n");
-            return -1;
-        }
-        gpuCacheSize.fBytes = atoi(FLAGS_gpuCacheSize[0]);
-        gpuCacheSize.fCount = atoi(FLAGS_gpuCacheSize[1]);
-    }
-#endif
-
     for (int i = 0; i < configs.count(); i++) {
         ConfigData config = gRec[configs[i]];
 
         // Skip any tests that we don't even need to try.
         if ((kPDF_Backend == config.fBackend) &&
             (!FLAGS_pdf|| (gmFlags & GM::kSkipPDF_Flag))) {
                 continue;
             }
         if ((gmFlags & GM::kSkip565_Flag) &&
             (kRaster_Backend == config.fBackend) &&
             (SkBitmap::kRGB_565_Config == config.fConfig)) {
             continue;
         }
         if ((gmFlags & GM::kSkipGPU_Flag) &&
             kGPU_Backend == config.fBackend) {
             continue;
         }
 
         // Now we know that we want to run this test and record its
         // success or failure.
-        ErrorBitfield errorsForThisConfig = kEmptyErrorBitfield;
+        ErrorCombination errorsForThisConfig;
         GrRenderTarget* renderTarget = NULL;
 #if SK_SUPPORT_GPU
         SkAutoTUnref<GrRenderTarget> rt;
         AutoResetGr autogr;
-        if ((kEmptyErrorBitfield == errorsForThisConfig) && (kGPU_Backend == config.fBackend)) {
+        if ((errorsForThisConfig.isEmpty()) && (kGPU_Backend == config.fBackend)) {
             GrContext* gr = grFactory->get(config.fGLContextType);
             bool grSuccess = false;
             if (gr) {
                 // create a render target to back the device
                 GrTextureDesc desc;
                 desc.fConfig = kSkia8888_GrPixelConfig;
                 desc.fFlags = kRenderTarget_GrTextureFlagBit;
                 desc.fWidth = gm->getISize().width();
                 desc.fHeight = gm->getISize().height();
                 desc.fSampleCnt = config.fSampleCnt;
                 GrTexture* tex = gr->createUncachedTexture(desc, NULL, 0);
                 if (tex) {
                     rt.reset(tex->asRenderTarget());
                     rt.get()->ref();
                     tex->unref();
                     autogr.set(gr);
                     renderTarget = rt.get();
                     grSuccess = NULL != renderTarget;
                 }
                 // Set the user specified cache limits if non-default.
                 size_t bytes;
                 int count;
                 gr->getTextureCacheLimits(&count, &bytes);
-                if (-1 != gpuCacheSize.fBytes) {
-                    bytes = static_cast<size_t>(gpuCacheSize.fBytes);
+                if (-1 != gpuCacheSizeBytes) {
+                    bytes = static_cast<size_t>(gpuCacheSizeBytes);
                 }
-                if (-1 != gpuCacheSize.fCount) {
-                    count = gpuCacheSize.fCount;
+                if (-1 != gpuCacheSizeCount) {
+                    count = gpuCacheSizeCount;
                 }
                 gr->setTextureCacheLimits(count, bytes);
             }
             if (!grSuccess) {
-                errorsForThisConfig |= kNoGpuContext_ErrorBitmask;
+                errorsForThisConfig.add(kNoGpuContext_ErrorType);
             }
         }
 #endif
 
         SkBitmap comparisonBitmap;
 
         const char* writePath;
         if (FLAGS_writePath.count() == 1) {
             writePath = FLAGS_writePath[0];
         } else {
             writePath = NULL;
         }
-        if (kEmptyErrorBitfield == errorsForThisConfig) {
-            errorsForThisConfig |= gmmain.test_drawing(gm, config, writePath, GetGr(),
-                                                       renderTarget, &comparisonBitmap);
+        if (errorsForThisConfig.isEmpty()) {
+            errorsForThisConfig.add(gmmain.test_drawing(gm, config, writePath, GetGr(),
+                                                        renderTarget, &comparisonBitmap));
         }
 
-        if (FLAGS_deferred && !errorsForThisConfig &&
-            (kGPU_Backend == config.fBackend ||
-             kRaster_Backend == config.fBackend)) {
-            errorsForThisConfig |= gmmain.test_deferred_drawing(gm, config, comparisonBitmap,
-                                                                GetGr(), renderTarget);
+        if (FLAGS_deferred && errorsForThisConfig.isEmpty() &&
+            (kGPU_Backend == config.fBackend || kRaster_Backend == config.fBackend)) {
+            errorsForThisConfig.add(gmmain.test_deferred_drawing(gm, config, comparisonBitmap,
+                                                                 GetGr(), renderTarget));
         }
 
-        errorsForAllConfigs |= errorsForThisConfig;
+        errorsForAllConfigs.add(errorsForThisConfig);
     }
     return errorsForAllConfigs;
 }
 
 /**
  * Run this test in a number of different drawing modes (pipe,
  * deferred, tiled, etc.), confirming that the resulting bitmaps all
  * *exactly* match comparisonBitmap.
  *
  * Returns all errors encountered while doing so.
  */
-ErrorBitfield run_multiple_modes(GMMain &gmmain, GM *gm, const ConfigData &compareConfig,
-                                 const SkBitmap &comparisonBitmap);
-ErrorBitfield run_multiple_modes(GMMain &gmmain, GM *gm, const ConfigData &compareConfig,
-                                 const SkBitmap &comparisonBitmap) {
-    SkTDArray<SkScalar> tileGridReplayScales;
-    *tileGridReplayScales.append() = SK_Scalar1; // By default only test at scale 1.0
-    if (FLAGS_tileGridReplayScales.count() > 0) {
-        tileGridReplayScales.reset();
-        for (int i = 0; i < FLAGS_tileGridReplayScales.count(); i++) {
-            double val = atof(FLAGS_tileGridReplayScales[i]);
-            if (0 < val) {
-                *tileGridReplayScales.append() = SkDoubleToScalar(val);
-            }
-        }
-        if (0 == tileGridReplayScales.count()) {
-            // Should have at least one scale
-            gm_fprintf(stderr, "--tileGridReplayScales requires at least one scale.\n");
-            return -1;
-        }
-    }
-
-    ErrorBitfield errorsForAllModes = kEmptyErrorBitfield;
+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
     if (!(gmFlags & GM::kSkipPicture_Flag)) {
 
-        ErrorBitfield pictErrors = kEmptyErrorBitfield;
+        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;
             gmmain.generate_image_from_picture(gm, compareConfig, pict, &bitmap);
-            pictErrors |= gmmain.compare_test_results_to_reference_bitmap(
-                gm, compareConfig, "-replay", bitmap, &comparisonBitmap);
+            pictErrors.add(gmmain.compare_test_results_to_reference_bitmap(
+                gm, compareConfig, "-replay", bitmap, &comparisonBitmap));
         }
 
-        if ((kEmptyErrorBitfield == pictErrors) && FLAGS_serialize) {
+        if ((pictErrors.isEmpty()) && FLAGS_serialize) {
             SkPicture* repict = gmmain.stream_to_new_picture(*pict);
             SkAutoUnref aurr(repict);
 
             SkBitmap bitmap;
             gmmain.generate_image_from_picture(gm, compareConfig, repict, &bitmap);
-            pictErrors |= gmmain.compare_test_results_to_reference_bitmap(
-                gm, compareConfig, "-serialize", bitmap, &comparisonBitmap);
+            pictErrors.add(gmmain.compare_test_results_to_reference_bitmap(
+                gm, compareConfig, "-serialize", bitmap, &comparisonBitmap));
         }
 
         if (FLAGS_writePicturePath.count() == 1) {
             const char* pictureSuffix = "skp";
             SkString path = make_filename(FLAGS_writePicturePath[0], "",
                                           gm->shortName(), pictureSuffix);
             SkFILEWStream stream(path.c_str());
             pict->serialize(&stream);
         }
 
-        errorsForAllModes |= pictErrors;
+        errorsForAllModes.add(pictErrors);
     }
 
     // TODO: add a test in which the RTree rendering results in a
     // different bitmap than the standard rendering.  It should
     // show up as failed in the JSON summary, and should be listed
     // in the stdout also.
     if (!(gmFlags & GM::kSkipPicture_Flag) && FLAGS_rtree) {
         SkPicture* pict = gmmain.generate_new_picture(
             gm, kRTree_BbhType, SkPicture::kUsePathBoundsForClip_RecordingFlag);
         SkAutoUnref aur(pict);
         SkBitmap bitmap;
         gmmain.generate_image_from_picture(gm, compareConfig, pict, &bitmap);
-        errorsForAllModes |= gmmain.compare_test_results_to_reference_bitmap(
-            gm, compareConfig, "-rtree", bitmap, &comparisonBitmap);
+        errorsForAllModes.add(gmmain.compare_test_results_to_reference_bitmap(
+            gm, compareConfig, "-rtree", bitmap, &comparisonBitmap));
     }
 
     if (!(gmFlags & GM::kSkipPicture_Flag) && FLAGS_tileGrid) {
         for(int scaleIndex = 0; scaleIndex < tileGridReplayScales.count(); ++scaleIndex) {
             SkScalar replayScale = tileGridReplayScales[scaleIndex];
             if ((gmFlags & GM::kSkipScaledReplay_Flag) && replayScale != 1) {
                 continue;
             }
             // We record with the reciprocal scale to obtain a replay
             // result that can be validated against comparisonBitmap.
             SkScalar recordScale = SkScalarInvert(replayScale);
             SkPicture* pict = gmmain.generate_new_picture(
                 gm, kTileGrid_BbhType, SkPicture::kUsePathBoundsForClip_RecordingFlag, recordScale);
             SkAutoUnref aur(pict);
             SkBitmap bitmap;
             gmmain.generate_image_from_picture(gm, compareConfig, pict, &bitmap, replayScale);
             SkString suffix("-tilegrid");
             if (SK_Scalar1 != replayScale) {
                 suffix += "-scale-";
                 suffix.appendScalar(replayScale);
             }
-            errorsForAllModes |= gmmain.compare_test_results_to_reference_bitmap(
-                gm, compareConfig, suffix.c_str(), bitmap, &comparisonBitmap);
+            errorsForAllModes.add(gmmain.compare_test_results_to_reference_bitmap(
+                gm, compareConfig, suffix.c_str(), bitmap, &comparisonBitmap));
         }
     }
 
     // run the pipe centric GM steps
     if (!(gmFlags & GM::kSkipPipe_Flag)) {
 
-        ErrorBitfield pipeErrors = kEmptyErrorBitfield;
+        ErrorCombination pipeErrors;
 
         if (FLAGS_pipe) {
-            pipeErrors |= gmmain.test_pipe_playback(gm, compareConfig, comparisonBitmap);
+            pipeErrors.add(gmmain.test_pipe_playback(gm, compareConfig, comparisonBitmap));
         }
 
-        if ((kEmptyErrorBitfield == pipeErrors) &&
+        if ((pipeErrors.isEmpty()) &&
             FLAGS_tiledPipe && !(gmFlags & GM::kSkipTiled_Flag)) {
-            pipeErrors |= gmmain.test_tiled_pipe_playback(gm, compareConfig, comparisonBitmap);
+            pipeErrors.add(gmmain.test_tiled_pipe_playback(gm, compareConfig, comparisonBitmap));
         }
 
-        errorsForAllModes |= pipeErrors;
+        errorsForAllModes.add(pipeErrors);
     }
     return errorsForAllModes;
 }
 
 int tool_main(int argc, char** argv);
 int tool_main(int argc, char** argv) {
 
 #if SK_ENABLE_INST_COUNT
     gPrintInstCount = true;
 #endif
@@ skipping 45 matching lines @@
 
     if (FLAGS_modulo.count() == 2) {
         moduloRemainder = atoi(FLAGS_modulo[0]);
         moduloDivisor = atoi(FLAGS_modulo[1]);
         if (moduloRemainder < 0 || moduloDivisor <= 0 || moduloRemainder >= moduloDivisor) {
             gm_fprintf(stderr, "invalid modulo values.");
             return -1;
         }
     }
 
+    int gpuCacheSizeBytes = -1; // -1 means use the default
+    int gpuCacheSizeCount = -1; // -1 means use the default

[borenet, 2013/03/22 13:53:29]

To me, it feels like these are "escaping" their ifdef prison.  Maybe it's not a
bad thing, and they should run free, but I think it's worth talking about (and
verifying that there are no problems on the NoGPU bot).

[epoger, 2013/03/22 14:31:23]

There's plenty not to like here, including the magical -1 values which I copied
from the existing code. :-(

In general, I think it's best to have as few #ifdefs in the code as possible
(for increased maintainability).  But to date, yes, we have used #ifdefs
liberally so that the gpuCacheSize code is completely turned off on non-GPU
builds.  I'm not sure how to balance out those competing interests.

Here's my proposal: let's see what we can figure out for the
run_multiple_configs() parameters, and then I will put as much as possible of
the gpuCacheSize code back in its prison.

+#if SK_SUPPORT_GPU
+    if (FLAGS_gpuCacheSize.count() > 0) {
+        if (FLAGS_gpuCacheSize.count() != 2) {
+            gm_fprintf(stderr, "--gpuCacheSize requires two arguments\n");
+            return -1;
+        }
+        gpuCacheSizeBytes = atoi(FLAGS_gpuCacheSize[0]);
+        gpuCacheSizeCount = atoi(FLAGS_gpuCacheSize[1]);
+    }
+#endif
+
+    SkTDArray<SkScalar> tileGridReplayScales;
+    *tileGridReplayScales.append() = SK_Scalar1; // By default only test at scale 1.0
+    if (FLAGS_tileGridReplayScales.count() > 0) {
+        tileGridReplayScales.reset();
+        for (int i = 0; i < FLAGS_tileGridReplayScales.count(); i++) {
+            double val = atof(FLAGS_tileGridReplayScales[i]);
+            if (0 < val) {
+                *tileGridReplayScales.append() = SkDoubleToScalar(val);
+            }
+        }
+        if (0 == tileGridReplayScales.count()) {
+            // Should have at least one scale
+            gm_fprintf(stderr, "--tileGridReplayScales requires at least one scale.\n");
+            return -1;
+        }
+    }
+
     if (!userConfig) {
         // if no config is specified by user, add the defaults
         for (size_t i = 0; i < SK_ARRAY_COUNT(gRec); ++i) {
             if (gRec[i].fRunByDefault) {
                 *configs.append() = i;
             }
         }
     }
     // now remove any explicitly excluded configs
     for (int i = 0; i < excludeConfigs.count(); ++i) {
@@ skipping 118 matching lines @@
         const char* shortName = gm->shortName();
         if (skip_name(FLAGS_match, shortName)) {
             SkDELETE(gm);
             continue;
         }
 
         SkISize size = gm->getISize();
         gm_fprintf(stdout, "%sdrawing... %s [%d %d]\n", moduloStr.c_str(), shortName,
                    size.width(), size.height());
 
-        ErrorBitfield testErrors = kEmptyErrorBitfield;
-        testErrors |= run_multiple_configs(gmmain, gm, configs, grFactory);
+        ErrorCombination testErrors;
+        testErrors.add(run_multiple_configs(gmmain, gm, configs, grFactory,
+                                            gpuCacheSizeBytes, gpuCacheSizeCount));
 
         SkBitmap comparisonBitmap;
         const ConfigData compareConfig =
             { SkBitmap::kARGB_8888_Config, kRaster_Backend, kDontCare_GLContextType, 0, kRW_ConfigFlag, "comparison", false };
-        testErrors |= gmmain.generate_image(gm, compareConfig, NULL, NULL, &comparisonBitmap, false);
+        testErrors.add(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 |= run_multiple_modes(gmmain, gm, compareConfig, comparisonBitmap);
+        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() ||
-            (kEmptyErrorBitfield != (kMissingExpectations_ErrorBitmask & testErrors))) {
+            (testErrors.includes(kMissingExpectations_ErrorType))) {
             testsMissingReferenceImages++;
         }
-        if (testErrors == (testErrors & kIgnorable_ErrorBitmask)) {
+        if (testErrors.minus(gmmain.fIgnorableErrorCombination).isEmpty()) {
             testsPassed++;
         } else {
             testsFailed++;
         }
 
         SkDELETE(gm);
     }
     gm_fprintf(stdout, "Ran %d tests: %d passed, %d failed, %d missing reference images\n",
                testsRun, testsPassed, testsFailed, testsMissingReferenceImages);
     gmmain.ListErrors();
@@ skipping 42 matching lines @@
     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