Fix expectations in GM PDF rendering

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 841 matching lines @@
      */
     void add_expected_results_to_json_summary(const char testName[],
                                               Expectations expectations) {
         this->fJsonExpectedResults[testName] = expectations.asJsonValue();
     }
 
     /**
      * Compare actualBitmap to expectations stored in this->fExpectationsSource.
      *
      * @param gm which test generated the actualBitmap
      * @param gRec

[epoger, 2013/08/21 17:55:32]

If you make any other changes before committing, please add @param configName to
doc.  (Otherwise, I wouldn't bother.)

[ducky, 2013/08/21 18:01:34]

Why not, fixed.

      * @param actualBitmapAndDigest ptr to bitmap generated by this run, or NULL
      *        if we don't have a usable bitmap representation
      */
     ErrorCombination compare_test_results_to_stored_expectations(
-        GM* gm, const ConfigData& gRec,
+        GM* gm, const ConfigData& gRec, const char* configName,
         const BitmapAndDigest* actualBitmapAndDigest) {
 
-        SkString shortNamePlusConfig = make_shortname_plus_config(gm->shortName(), gRec.fName);
+        SkString shortNamePlusConfig = make_shortname_plus_config(gm->shortName(), configName);
 
         ErrorCombination errors;
 
         if (NULL == actualBitmapAndDigest) {
             // Note that we intentionally skipped validating the results for
             // this test, because we don't know how to generate an SkBitmap
             // version of the output.
             errors.add(ErrorCombination(kIntentionallySkipped_ErrorType));
         } else if (!(gRec.fFlags & kWrite_ConfigFlag)) {
             // We don't record the results for this test or compare them
@@ skipping 17 matching lines @@
                  * 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 hash digest returned here may not match the
                  * hash digest of actualBitmap, which *has* been run through
                  * force_all_opaque().
                  * See comments above complete_bitmap() for more detail.
                  */
                 Expectations expectations = expectationsSource->get(nameWithExtension.c_str());
                 errors.add(compare_to_expectations(expectations, *actualBitmapAndDigest,
-                                                   gm->shortName(), gRec.fName, "", true));
+                                                   gm->shortName(), configName, "", true));
             } else {
                 // If we are running without expectations, we still want to
                 // record the actual results.
                 add_actual_results_to_json_summary(nameWithExtension.c_str(),
                                                    actualBitmapAndDigest->fDigest,
                                                    ErrorCombination(kMissingExpectations_ErrorType),
                                                    false);
                 errors.add(ErrorCombination(kMissingExpectations_ErrorType));
             }
         }
@@ skipping 96 matching lines @@
             // Early exit if we can't generate the image.
             errors.add(generate_image(gm, gRec, gpuTarget, 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;
             }
             BitmapAndDigest bitmapAndDigest(*bitmap);
             errors.add(compare_test_results_to_stored_expectations(
-                           gm, gRec, &bitmapAndDigest));
+                           gm, gRec, gRec.fName, &bitmapAndDigest));
 
             if (writePath && (gRec.fFlags & kWrite_ConfigFlag)) {
                 path = make_bitmap_filename(writePath, gm->shortName(), gRec.fName,
                                             "", bitmapAndDigest.fDigest);
                 errors.add(write_bitmap(path, bitmapAndDigest.fBitmap));
             }
         } else if (gRec.fBackend == kPDF_Backend) {
             generate_pdf(gm, document);
 
             SkAutoTUnref<SkStreamAsset> documentStream(document.detachAsStream());
             if (writePath && (gRec.fFlags & kWrite_ConfigFlag)) {
                 path = make_filename(writePath, gm->shortName(), gRec.fName, "", "pdf");
                 errors.add(write_document(path, documentStream));
             }
 
             if (!(gm->getFlags() & GM::kSkipPDFRasterization_Flag)) {
                 for (int i = 0; i < pdfRasterizers.count(); i++) {
                     SkBitmap pdfBitmap;
                     SkASSERT(documentStream->rewind());
                     bool success = (*pdfRasterizers[i]->fRasterizerFunction)(
                             documentStream.get(), &pdfBitmap);
                     if (!success) {
                         gm_fprintf(stderr, "FAILED to render PDF for %s using renderer %s\n",
                                    gm->shortName(),
                                    pdfRasterizers[i]->fName);
                         continue;
                     }
 
+                    SkString configName(gRec.fName);

[epoger, 2013/08/21 17:55:32]

I guess it wouldn't work for gRec.fName to return "pdf-mac" in this case?

[ducky, 2013/08/21 18:01:34]

Nope. gRec doesn't have any fields to run arbitrary functions to generate the
images (and doing such a thing would require a massive GM refactor), so the PDF
rasterizers are stored in a separate struct which contains a pointer to a
function that takes in PDF data and outputs a bitmap.

[epoger, 2013/08/21 18:05:20]

Gotcha, thanks for the explanation.

Would it make sense for the PDF rasterizer struct to subclass gRec, and thus
various gRec accessors (including gRec.fName) would return more detailed
information in the PDF case?  I'm not suggesting making that change right now,
but I'm curious whether it would be a Good Thing.

+                    configName.append("-");
+                    configName.append(pdfRasterizers[i]->fName);
+
                     BitmapAndDigest bitmapAndDigest(pdfBitmap);
                     errors.add(compare_test_results_to_stored_expectations(
-                               gm, gRec, &bitmapAndDigest));
-
-                    SkString configName(gRec.fName);
-                    configName.append("_");
-                    configName.append(pdfRasterizers[i]->fName);
+                               gm, gRec, configName.c_str(), &bitmapAndDigest));
 
                     if (writePath && (gRec.fFlags & kWrite_ConfigFlag)) {
                         path = make_bitmap_filename(writePath, gm->shortName(), configName.c_str(),
                                                     "", bitmapAndDigest.fDigest);
                         errors.add(write_bitmap(path, bitmapAndDigest.fBitmap));
                     }
                 }
             } else {
                 errors.add(kIntentionallySkipped_ErrorType);
             }
         } else if (gRec.fBackend == kXPS_Backend) {
             generate_xps(gm, document);
             SkAutoTUnref<SkStreamAsset> documentStream(document.detachAsStream());
 
             errors.add(compare_test_results_to_stored_expectations(
-                           gm, gRec, NULL));
+                           gm, gRec, gRec.fName, NULL));
 
             if (writePath && (gRec.fFlags & kWrite_ConfigFlag)) {
                 path = make_filename(writePath, gm->shortName(), gRec.fName, "", "xps");
                 errors.add(write_document(path, documentStream));
             }
         } else {
             SkASSERT(false);
         }
         return errors;
     }
@@ skipping 731 matching lines @@
             ConfigData config = gRec[configs[i]];
             SkString subdir;
             subdir.appendf("%s%c%s", root, SkPATH_SEPARATOR, config.fName);
             if (!sk_mkdir(subdir.c_str())) {
                 return false;
             }
 
             if (config.fBackend == kPDF_Backend) {
                 for (int j = 0; j < pdfRasterizers.count(); j++) {
                     SkString pdfSubdir = subdir;
-                    pdfSubdir.appendf("_%s", pdfRasterizers[j]->fName);
+                    pdfSubdir.appendf("-%s", pdfRasterizers[j]->fName);
                     if (!sk_mkdir(pdfSubdir.c_str())) {
                         return false;
                     }
                 }
             }
         }
     }
     return true;
 }
 
@@ skipping 459 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