GM: run mode comparisons for all raster configs.

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 1511 matching lines @@
 }
 
 template <typename T> void appendUnique(SkTDArray<T>* array, const T& value) {
     int index = array->find(value);
     if (index < 0) {
         *array->append() = value;
     }
 }
 
 /**
- * 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.
- */
-ErrorCombination run_multiple_configs(GMMain &gmmain, GM *gm,
-                                      const SkTDArray<size_t> &configs,
-                                      const SkTDArray<const PDFRasterizerData*> &pdfRasterizers,
-                                      GrContextFactory *grFactory);
-ErrorCombination run_multiple_configs(GMMain &gmmain, GM *gm,
-                                      const SkTDArray<size_t> &configs,
-                                      const SkTDArray<const PDFRasterizerData*> &pdfRasterizers,
-                                      GrContextFactory *grFactory) {
-    const char renderModeDescriptor[] = "";
-    ErrorCombination errorsForAllConfigs;
-    uint32_t gmFlags = gm->getFlags();
-
-    for (int i = 0; i < configs.count(); i++) {
-        ConfigData config = gRec[configs[i]];
-        const SkString shortNamePlusConfig = gmmain.make_shortname_plus_config(gm->shortName(),
-                                                                               config.fName);
-
-        // Skip any tests that we don't even need to try.
-        // If any of these were skipped on a per-GM basis, record them as
-        // kIntentionallySkipped.
-        if (kPDF_Backend == config.fBackend) {
-            if (gmFlags & GM::kSkipPDF_Flag) {
-                gmmain.RecordTestResults(kIntentionallySkipped_ErrorType, shortNamePlusConfig,
-                                         renderModeDescriptor);
-                errorsForAllConfigs.add(kIntentionallySkipped_ErrorType);
-                continue;
-            }
-        }
-        if ((gmFlags & GM::kSkip565_Flag) &&
-            (kRaster_Backend == config.fBackend) &&
-            (SkBitmap::kRGB_565_Config == config.fConfig)) {
-            gmmain.RecordTestResults(kIntentionallySkipped_ErrorType, shortNamePlusConfig,
-                                     renderModeDescriptor);
-            errorsForAllConfigs.add(kIntentionallySkipped_ErrorType);
-            continue;
-        }
-        if (((gmFlags & GM::kSkipGPU_Flag) && kGPU_Backend == config.fBackend) ||
-            ((gmFlags & GM::kGPUOnly_Flag) && kGPU_Backend != config.fBackend)) {
-            gmmain.RecordTestResults(kIntentionallySkipped_ErrorType, shortNamePlusConfig,
-                                     renderModeDescriptor);
-            errorsForAllConfigs.add(kIntentionallySkipped_ErrorType);
-            continue;
-        }
-
-        // Now we know that we want to run this test and record its
-        // success or failure.
-        ErrorCombination errorsForThisConfig;
-        GrSurface* gpuTarget = NULL;
-#if SK_SUPPORT_GPU
-        SkAutoTUnref<GrSurface> auGpuTarget;
-        if ((errorsForThisConfig.isEmpty()) && (kGPU_Backend == config.fBackend)) {
-            if (FLAGS_resetGpuContext) {
-                grFactory->destroyContexts();
-            }
-            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;
-                auGpuTarget.reset(gr->createUncachedTexture(desc, NULL, 0));
-                if (NULL != auGpuTarget) {
-                    gpuTarget = auGpuTarget;
-                    grSuccess = true;
-                    // Set the user specified cache limits if non-default.
-                    size_t bytes;
-                    int count;
-                    gr->getTextureCacheLimits(&count, &bytes);
-                    if (DEFAULT_CACHE_VALUE != gGpuCacheSizeBytes) {
-                        bytes = static_cast<size_t>(gGpuCacheSizeBytes);
-                    }
-                    if (DEFAULT_CACHE_VALUE != gGpuCacheSizeCount) {
-                        count = gGpuCacheSizeCount;
-                    }
-                    gr->setTextureCacheLimits(count, bytes);
-                }
-            }
-            if (!grSuccess) {
-                errorsForThisConfig.add(kNoGpuContext_ErrorType);
-            }
-        }
-#endif
-
-        SkBitmap comparisonBitmap;
-
-        const char* writePath;
-        if (FLAGS_writePath.count() == 1) {
-            writePath = FLAGS_writePath[0];
-        } else {
-            writePath = NULL;
-        }
-
-        if (errorsForThisConfig.isEmpty()) {
-            errorsForThisConfig.add(gmmain.test_drawing(gm, config, pdfRasterizers,
-                                                        writePath, gpuTarget,
-                                                        &comparisonBitmap));
-            gmmain.RecordTestResults(errorsForThisConfig, shortNamePlusConfig, "");
-        }
-
-        if (FLAGS_deferred && errorsForThisConfig.isEmpty() &&
-            (kGPU_Backend == config.fBackend || kRaster_Backend == config.fBackend)) {
-            errorsForThisConfig.add(gmmain.test_deferred_drawing(gm, config, comparisonBitmap,
-                                                                 gpuTarget));
-        }
-
-        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.
  */
 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,
@@ skipping 112 matching lines @@
         errorsForAllModes.add(gmmain.test_pipe_playback(gm, compareConfig, comparisonBitmap,
                                                         FLAGS_simulatePipePlaybackFailure));
         if (FLAGS_tiledPipe) {
             errorsForAllModes.add(gmmain.test_tiled_pipe_playback(gm, compareConfig,
                                                                   comparisonBitmap));
         }
     }
     return errorsForAllModes;
 }
 
+
+/**
+ * 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.
+ */
+ErrorCombination run_multiple_configs(GMMain &gmmain, GM *gm,
+                                      const SkTDArray<size_t> &configs,
+                                      const SkTDArray<const PDFRasterizerData*> &pdfRasterizers,
+                                      const SkTDArray<SkScalar> &tileGridReplayScales,
+                                      GrContextFactory *grFactory);
+ErrorCombination run_multiple_configs(GMMain &gmmain, GM *gm,
+                                      const SkTDArray<size_t> &configs,
+                                      const SkTDArray<const PDFRasterizerData*> &pdfRasterizers,
+                                      const SkTDArray<SkScalar> &tileGridReplayScales,
+                                      GrContextFactory *grFactory) {
+    const char renderModeDescriptor[] = "";
+    ErrorCombination errorsForAllConfigs;
+    uint32_t gmFlags = gm->getFlags();
+
+    for (int i = 0; i < configs.count(); i++) {
+        ConfigData config = gRec[configs[i]];
+        const SkString shortNamePlusConfig = gmmain.make_shortname_plus_config(gm->shortName(),
+                                                                               config.fName);
+
+        // Skip any tests that we don't even need to try.
+        // If any of these were skipped on a per-GM basis, record them as
+        // kIntentionallySkipped.
+        if (kPDF_Backend == config.fBackend) {
+            if (gmFlags & GM::kSkipPDF_Flag) {
+                gmmain.RecordTestResults(kIntentionallySkipped_ErrorType, shortNamePlusConfig,
+                                         renderModeDescriptor);
+                errorsForAllConfigs.add(kIntentionallySkipped_ErrorType);
+                continue;
+            }
+        }
+        if ((gmFlags & GM::kSkip565_Flag) &&
+            (kRaster_Backend == config.fBackend) &&
+            (SkBitmap::kRGB_565_Config == config.fConfig)) {
+            gmmain.RecordTestResults(kIntentionallySkipped_ErrorType, shortNamePlusConfig,
+                                     renderModeDescriptor);
+            errorsForAllConfigs.add(kIntentionallySkipped_ErrorType);
+            continue;
+        }
+        if (((gmFlags & GM::kSkipGPU_Flag) && kGPU_Backend == config.fBackend) ||
+            ((gmFlags & GM::kGPUOnly_Flag) && kGPU_Backend != config.fBackend)) {
+            gmmain.RecordTestResults(kIntentionallySkipped_ErrorType, shortNamePlusConfig,
+                                     renderModeDescriptor);
+            errorsForAllConfigs.add(kIntentionallySkipped_ErrorType);
+            continue;
+        }
+
+        // Now we know that we want to run this test and record its
+        // success or failure.
+        ErrorCombination errorsForThisConfig;
+        GrSurface* gpuTarget = NULL;
+#if SK_SUPPORT_GPU
+        SkAutoTUnref<GrSurface> auGpuTarget;
+        if ((errorsForThisConfig.isEmpty()) && (kGPU_Backend == config.fBackend)) {
+            if (FLAGS_resetGpuContext) {
+                grFactory->destroyContexts();
+            }
+            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;
+                auGpuTarget.reset(gr->createUncachedTexture(desc, NULL, 0));
+                if (NULL != auGpuTarget) {
+                    gpuTarget = auGpuTarget;
+                    grSuccess = true;
+                    // Set the user specified cache limits if non-default.
+                    size_t bytes;
+                    int count;
+                    gr->getTextureCacheLimits(&count, &bytes);
+                    if (DEFAULT_CACHE_VALUE != gGpuCacheSizeBytes) {
+                        bytes = static_cast<size_t>(gGpuCacheSizeBytes);
+                    }
+                    if (DEFAULT_CACHE_VALUE != gGpuCacheSizeCount) {
+                        count = gGpuCacheSizeCount;
+                    }
+                    gr->setTextureCacheLimits(count, bytes);
+                }
+            }
+            if (!grSuccess) {
+                errorsForThisConfig.add(kNoGpuContext_ErrorType);
+            }
+        }
+#endif
+
+        SkBitmap comparisonBitmap;
+
+        const char* writePath;
+        if (FLAGS_writePath.count() == 1) {
+            writePath = FLAGS_writePath[0];
+        } else {
+            writePath = NULL;
+        }
+
+        if (errorsForThisConfig.isEmpty()) {
+            errorsForThisConfig.add(gmmain.test_drawing(gm, config, pdfRasterizers,
+                                                        writePath, gpuTarget,
+                                                        &comparisonBitmap));
+            gmmain.RecordTestResults(errorsForThisConfig, shortNamePlusConfig, "");
+        }
+
+        // TODO: run only if gmmain.test_drawing succeeded.

[mtklein, 2014/01/08 20:05:53]

The diff is weird, but what's happened here is:
  - run_multiple_modes moves up the file verbatim to be visible to
run_multiple_configs
  - Only these next three lines (plus passing tileGridReplayScales) are new in
run_multiple_configs.  Nothing else changed.

[epoger, 2014/01/08 20:48:55]

Thanks for the explanation.

One way of avoiding the diff weirdness (at the expense of some time/effort) is
to upload the move as patchset 1, and the modifications as patchset 2.

+        if (kRaster_Backend == config.fBackend) {
+            run_multiple_modes(gmmain, gm, config, comparisonBitmap, tileGridReplayScales);
+        }
+
+        if (FLAGS_deferred && errorsForThisConfig.isEmpty() &&
+            (kGPU_Backend == config.fBackend || kRaster_Backend == config.fBackend)) {
+            errorsForThisConfig.add(gmmain.test_deferred_drawing(gm, config, comparisonBitmap,
+                                                                 gpuTarget));
+        }
+
+        errorsForAllConfigs.add(errorsForThisConfig);
+    }
+    return errorsForAllConfigs;
+}
+
+
 /**
  * Read individual lines from a file, pushing them into the given array.
  *
  * @param filename path to the file to read
  * @param lines array of strings to add the lines to
  * @returns true if able to read lines from the file
  */
 static bool read_lines_from_file(const char* filename, SkTArray<SkString> &lines) {
     SkAutoTUnref<SkStream> streamWrapper(SkStream::NewFromFile(filename));
     SkStream *stream = streamWrapper.get();
@@ skipping 497 matching lines @@
 
         if (SkCommandLineFlags::ShouldSkip(FLAGS_match, shortName)) {
             continue;
         }
 
         gmsRun++;
         SkISize size = gm->getISize();
         gm_fprintf(stdout, "%sdrawing... %s [%d %d]\n", moduloStr.c_str(), shortName,
                    size.width(), size.height());
 
-        run_multiple_configs(gmmain, gm, configs, pdfRasterizers, grFactory);
-
-        SkBitmap comparisonBitmap;
-        const ConfigData compareConfig =
-            { SkBitmap::kARGB_8888_Config, kRaster_Backend, kDontCare_GLContextType, 0, kRW_ConfigFlag, "comparison", false };
-        gmmain.generate_image(gm, compareConfig, NULL, &comparisonBitmap, false);
-
-        // TODO(epoger): only run this if gmmain.generate_image() succeeded?
-        // Otherwise, what are we comparing against?
-        run_multiple_modes(gmmain, gm, compareConfig, comparisonBitmap, tileGridReplayScales);
+        run_multiple_configs(gmmain, gm, configs, pdfRasterizers, tileGridReplayScales, grFactory);
     }
 
     SkTArray<SkString> modes;
     gmmain.GetRenderModesEncountered(modes);
     bool reportError = false;
     if (gmmain.NumSignificantErrors() > 0) {
         reportError = true;
     }
     int expectedNumberOfTests = gmsRun * (configs.count() + modes.count());

[epoger, 2014/01/08 20:48:55]

expectedNumberOfTests should now be calculated differently; if not, the
GenerateGMs step will start failing with a message like the one you saw in your
test run:

GM: expected 3348 tests, but ran or skipped 4956 tests

I think the number of expected tests is now something like:
gmsRun * (numRasterConfigs * modes.count()) +
gmsRun * numNonRasterConfigs

... where numRasterConfigs and numNonRasterConfigs are not-yet-existing
variables, such that
numRasterConfigs + numNonRasterConfigs = configs.count()

(I guess numRasterConfigs is 2, for 8888 and 565)

 
     // Output summary to stdout.

[epoger, 2014/01/08 20:48:55]

As expectedNumberOfTests indicates, this change will cause GM to take
significantly longer.  Do we feel that the benefits outweigh the costs?  (I'm
not sure how to make that determination; it probably has a lot to do with how
important we think 565 is)

     if (FLAGS_verbose) {
         gm_fprintf(stdout, "Ran %d GMs\n", gmsRun);

[epoger, 2014/01/08 20:48:55]

This change will modify the gm self-test results.  mtklein, are you developing
this change on a Linux machine?  If so, you should be able to run
gm/tests/run.sh (before and after your change) to see for yourself, and
gm/tests/rebaseline.sh to correct the self-test results within this same CL.

If you have trouble with that, or are NOT developing this CL on a Linux machine,
let's coordinate once this CL is otherwise ready-to-go and I will perform the gm
self-test rebaseline for you.

         gm_fprintf(stdout, "... over %2d configs [%s]\n", configs.count(),
                    list_all_config_names(configs).c_str());
         gm_fprintf(stdout, "...  and %2d modes   [%s]\n", modes.count(), list_all(modes).c_str());
         gm_fprintf(stdout, "... so there should be a total of %d tests.\n", expectedNumberOfTests);
     }
     gmmain.ListErrors(FLAGS_verbose);
 
     // TODO(epoger): Enable this check for Android, too, once we resolve
     // https://code.google.com/p/skia/issues/detail?id=1222
     // ('GM is unexpectedly skipping tests on Android')
@@ skipping 41 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