reland r14391 ('fix contents of render_pictures JSON summary')

BUG=skia:2043,skia:2044,skia:1942,skia:2466 R=caryclark@google.com TBR=caryclark Author: epoger@google.com Review URL: https://codereview.chromium.org/262453002 git-svn-id: http://skia.googlecode.com/svn/trunk@14443 2bbb7eff-a529-9590-31e7-b0007b416f81
2014-04-29 19:39:22 +00:00 · 2014-04-29 19:39:22 +00:00 · 4610a465b6
commit 4610a465b6
parent 21a6d13a2b
3 changed files with 153 additions and 164 deletions
--- a/tools/PictureRenderer.cpp
+++ b/tools/PictureRenderer.cpp
@ -323,26 +323,17 @@ uint32_t PictureRenderer::recordFlags() {
 }

 /**
- * Write the canvas to the specified path.
+ * Write the canvas to an image file and/or JSON summary.
 *
 * @param canvas Must be non-null. Canvas to be written to a file.
- * @param outputDir If nonempty, write the binary image to a file within this directory.
+ * @param outputDir If nonempty, write the binary image to a file within this directory;
+ *     if empty, don't write out the image at all.
 * @param inputFilename If we are writing out a binary image, use this to build its filename.
- * @param jsonSummaryPtr If not null, add image results to this summary.
+ * @param jsonSummaryPtr If not null, add image results (checksum) to this summary.
 * @param useChecksumBasedFilenames If true, use checksum-based filenames when writing to disk.
 * @param tileNumberPtr If not null, which tile number this image contains.
- * @return bool True if the Canvas is written to a file.
 *
- * TODO(epoger): Right now, all canvases must pass through this function in order to be appended
- * to the ImageResultsSummary.  We need some way to add bitmaps to the ImageResultsSummary
- * even if --writePath has not been specified (and thus this function is not called).
- *
- * One fix would be to pass in these path elements separately, and allow this function to be
- * called even if --writePath was not specified...
- *  const char *outputDir   // NULL if we don't want to write image files to disk
- *  const char *filename    // name we use within JSON summary, and as the filename within outputDir
- *
- * UPDATE: Now that outputDir and inputFilename are passed separately, we should be able to do that.
+ * @return bool True if the operation completed successfully.
 */
 static bool write(SkCanvas* canvas, const SkString& outputDir, const SkString& inputFilename,
                  ImageResultsSummary *jsonSummaryPtr, bool useChecksumBasedFilenames,
@ -407,8 +398,10 @@ static bool write(SkCanvas* canvas, const SkString& outputDir, const SkString& i
                            hash, tileNumberPtr);
    }

-    SkASSERT(!outputDir.isEmpty()); // TODO(epoger): we want to remove this constraint,
-                                    // as noted above
+    if (outputDir.isEmpty()) {
+        return true;
+    }
+
    SkString dirPath;
    if (outputSubdirPtr) {
        dirPath = SkOSPath::SkPathJoin(outputDir.c_str(), outputSubdirPtr);
@ -470,16 +463,13 @@ bool PipePictureRenderer::render(SkBitmap** out) {
    pipeCanvas->drawPicture(*fPicture);
    writer.endRecording();
    fCanvas->flush();
-    if (!fOutputDir.isEmpty()) {
-        return write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
-                     fUseChecksumBasedFilenames);
-    }
    if (NULL != out) {
        *out = SkNEW(SkBitmap);
        setup_bitmap(*out, fPicture->width(), fPicture->height());
        fCanvas->readPixels(*out, 0, 0);
    }
-    return true;
+    return write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
+                 fUseChecksumBasedFilenames);
 }

 SkString PipePictureRenderer::getConfigNameInternal() {
@ -503,18 +493,13 @@ bool SimplePictureRenderer::render(SkBitmap** out) {

    fCanvas->drawPicture(*fPicture);
    fCanvas->flush();
-    if (!fOutputDir.isEmpty()) {
-        return write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
-                     fUseChecksumBasedFilenames);
-    }
-
    if (NULL != out) {
        *out = SkNEW(SkBitmap);
        setup_bitmap(*out, fPicture->width(), fPicture->height());
        fCanvas->readPixels(*out, 0, 0);
    }
-
-    return true;
+    return write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
+                 fUseChecksumBasedFilenames);
 }

 SkString SimplePictureRenderer::getConfigNameInternal() {
@ -722,10 +707,8 @@ bool TiledPictureRenderer::render(SkBitmap** out) {
    bool success = true;
    for (int i = 0; i < fTileRects.count(); ++i) {
        draw_tile_to_canvas(fCanvas, fTileRects[i], fPicture);
-        if (!fOutputDir.isEmpty()) {
        success &= write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
                         fUseChecksumBasedFilenames, &i);
-        }
        if (NULL != out) {
            if (fCanvas->readPixels(&bitmap, 0, 0)) {
                // Add this tile to the entire bitmap.
@ -807,8 +790,7 @@ public:

        for (int i = fStart; i < fEnd; i++) {
            draw_tile_to_canvas(fCanvas, fRects[i], fClone);
-            if ((!fOutputDir.isEmpty())
-                && !write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
+            if (!write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
                       fUseChecksumBasedFilenames, &i)
                && fSuccess != NULL) {
                *fSuccess = false;
--- a/tools/render_pictures_main.cpp
+++ b/tools/render_pictures_main.cpp
@ -35,10 +35,8 @@ DEFINE_bool(writeChecksumBasedFilenames, false,
 DEFINE_bool(writeEncodedImages, false, "Any time the skp contains an encoded image, write it to a "
            "file rather than decoding it. Requires writePath to be set. Skips drawing the full "
            "skp to a file. Not compatible with deferImageDecoding.");
-DEFINE_string(writeJsonSummaryPath, "", "File to write a JSON summary of image results to. "
-              "TODO(epoger): Currently, this only works if --writePath is also specified. "
-              "See https://code.google.com/p/skia/issues/detail?id=2043 .");
-DEFINE_string2(writePath, w, "", "Directory to write the rendered images.");
+DEFINE_string(writeJsonSummaryPath, "", "File to write a JSON summary of image results to.");
+DEFINE_string2(writePath, w, "", "Directory to write the rendered images into.");
 DEFINE_bool(writeWholeImage, false, "In tile mode, write the entire rendered image to a "
            "file, instead of an image for each tile.");
 DEFINE_bool(validate, false, "Verify that the rendered image contains the same pixels as "
@ -341,8 +339,6 @@ static bool render_picture(const SkString& inputPath, const SkString* outputDir,
    if (FLAGS_writeWholeImage) {
        sk_tools::force_all_opaque(*bitmap);

-        // TODO(epoger): It would be better for the filename (without outputDir) to be passed in
-        // here, and used both for the checksum file and writing into outputDir.
        SkString inputFilename, outputPath;
        sk_tools::get_basename(&inputFilename, inputPath);
        sk_tools::make_filepath(&outputPath, *outputDir, inputFilename);
--- a/tools/tests/render_pictures_test.py
+++ b/tools/tests/render_pictures_test.py
@ -26,6 +26,92 @@ EXPECTED_HEADER_CONTENTS = {
    "revision" : 1,
 }

+# Manually verified: 640x400 red rectangle with black border
+RED_WHOLEIMAGE = {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 11092453015575919668,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "red_skp.png",
+}
+
+# Manually verified: 640x400 green rectangle with black border
+GREEN_WHOLEIMAGE = {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 8891695120562235492,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "green_skp.png",
+}
+
+# Manually verified these 6 images, all 256x256 tiles,
+# consistent with a tiled version of the 640x400 red rect
+# with black borders.
+RED_TILES = [{
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 5815827069051002745,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "red_skp-tile0.png",
+},{
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 9323613075234140270,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "red_skp-tile1.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 16670399404877552232,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "red_skp-tile2.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 2507897274083364964,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "red_skp-tile3.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 7325267995523877959,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "red_skp-tile4.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 2181381724594493116,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "red_skp-tile5.png",
+}]
+
+# Manually verified these 6 images, all 256x256 tiles,
+# consistent with a tiled version of the 640x400 green rect
+# with black borders.
+GREEN_TILES = [{
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 12587324416545178013,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "green_skp-tile0.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 7624374914829746293,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "green_skp-tile1.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 5686489729535631913,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "green_skp-tile2.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 7980646035555096146,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "green_skp-tile3.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 17817086664365875131,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "green_skp-tile4.png",
+}, {
+    "checksumAlgorithm" : "bitmap-64bitMD5",
+    "checksumValue" : 10673669813016809363,
+    "comparisonResult" : "no-comparison",
+    "filepath" : "green_skp-tile5.png",
+}]
+

 class RenderPicturesTest(base_unittest.TestCase):

@ -39,13 +125,20 @@ class RenderPicturesTest(base_unittest.TestCase):
    shutil.rmtree(self._temp_dir)

  def test_tiled_whole_image(self):
-    """Run render_pictures with tiles and --writeWholeImage flag."""
+    """Run render_pictures with tiles and --writeWholeImage flag.
+
+    TODO(epoger): This test generates undesired results!  The JSON summary
+    includes both whole-image and tiled-images (as it should), but only
+    whole-images are written out to disk.  See http://skbug.com/2463
+
+    TODO(epoger): I noticed that when this is run without --writePath being
+    specified, this test writes red_skp.png and green_skp.png to the current
+    directory.  We should fix that... if --writePath is not specified, this
+    probably shouldn't write out red_skp.png and green_skp.png at all!
+    See http://skbug.com/2464
+    """
    output_json_path = os.path.join(self._temp_dir, 'output.json')
    self._generate_skps()
-    # TODO(epoger): I noticed that when this is run without --writePath being
-    # specified, this test writes red_skp.png and green_skp.png to the current
-    # directory.  We should fix that... if --writePath is not specified, this
-    # probably shouldn't write out red_skp.png and green_skp.png at all!
    self._run_render_pictures(['-r', self._input_skp_dir,
                               '--bbh', 'grid', '256', '256',
                               '--mode', 'tile', '256', '256',
@ -56,22 +149,12 @@ class RenderPicturesTest(base_unittest.TestCase):
        "header" : EXPECTED_HEADER_CONTENTS,
        "actual-results" : {
            "red.skp": {
-                # Manually verified: 640x400 red rectangle with black border
-                "whole-image": {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 11092453015575919668,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp.png",
-                },
+                "tiled-images": RED_TILES,
+                "whole-image": RED_WHOLEIMAGE,
            },
            "green.skp": {
-                # Manually verified: 640x400 green rectangle with black border
-                "whole-image": {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 8891695120562235492,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp.png",
-                },
+                "tiled-images": GREEN_TILES,
+                "whole-image": GREEN_WHOLEIMAGE,
            }
        }
    }
@ -86,28 +169,14 @@ class RenderPicturesTest(base_unittest.TestCase):
    self._run_render_pictures(['-r', self._input_skp_dir,
                               '--writePath', self._temp_dir,
                               '--writeJsonSummaryPath', output_json_path])
-    # TODO(epoger): These expectations are the same as for above unittest.
-    # Define the expectations once, and share them.
    expected_summary_dict = {
        "header" : EXPECTED_HEADER_CONTENTS,
        "actual-results" : {
            "red.skp": {
-                # Manually verified: 640x400 red rectangle with black border
-                "whole-image": {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 11092453015575919668,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp.png",
-                },
+                "whole-image": RED_WHOLEIMAGE,
            },
            "green.skp": {
-                # Manually verified: 640x400 green rectangle with black border
-                "whole-image": {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 8891695120562235492,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp.png",
-                },
+                "whole-image": GREEN_WHOLEIMAGE,
            }
        }
    }
@ -157,36 +226,44 @@ class RenderPicturesTest(base_unittest.TestCase):
        ['bitmap-64bitMD5_8891695120562235492.png'])

  def test_untiled_validate(self):
-    """Same as test_untiled, but with --validate.
-
-    TODO(epoger): This test generates undesired results!  The call
-    to render_pictures should succeed, and generate the same output as
-    test_untiled.
-    See https://code.google.com/p/skia/issues/detail?id=2044 ('render_pictures:
-    --validate fails')
-    """
+    """Same as test_untiled, but with --validate."""
    output_json_path = os.path.join(self._temp_dir, 'output.json')
    self._generate_skps()
-    with self.assertRaises(Exception):
    self._run_render_pictures(['-r', self._input_skp_dir,
                               '--validate',
                               '--writePath', self._temp_dir,
                               '--writeJsonSummaryPath', output_json_path])
+    expected_summary_dict = {
+        "header" : EXPECTED_HEADER_CONTENTS,
+        "actual-results" : {
+            "red.skp": {
+                "whole-image": RED_WHOLEIMAGE,
+            },
+            "green.skp": {
+                "whole-image": GREEN_WHOLEIMAGE,
+            }
+        }
+    }
+    self._assert_json_contents(output_json_path, expected_summary_dict)
+    self._assert_directory_contents(
+        self._temp_dir, ['red_skp.png', 'green_skp.png', 'output.json'])

  def test_untiled_without_writePath(self):
-    """Same as test_untiled, but without --writePath.
-
-    TODO(epoger): This test generates undesired results!
-    See https://code.google.com/p/skia/issues/detail?id=2043 ('render_pictures:
-    --writeJsonSummaryPath fails unless --writePath is specified')
-    """
+    """Same as test_untiled, but without --writePath."""
    output_json_path = os.path.join(self._temp_dir, 'output.json')
    self._generate_skps()
    self._run_render_pictures(['-r', self._input_skp_dir,
                               '--writeJsonSummaryPath', output_json_path])
    expected_summary_dict = {
        "header" : EXPECTED_HEADER_CONTENTS,
-        "actual-results" : None,
+        "actual-results" : {
+            "red.skp": {
+                "whole-image": RED_WHOLEIMAGE,
+            },
+            "green.skp": {
+                "whole-image": GREEN_WHOLEIMAGE,
+            }
+        }
    }
    self._assert_json_contents(output_json_path, expected_summary_dict)

@ -203,76 +280,10 @@ class RenderPicturesTest(base_unittest.TestCase):
        "header" : EXPECTED_HEADER_CONTENTS,
        "actual-results" : {
            "red.skp": {
-                # Manually verified these 6 images, all 256x256 tiles,
-                # consistent with a tiled version of the 640x400 red rect
-                # with black borders.
-                "tiled-images": [{
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 5815827069051002745,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp-tile0.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 9323613075234140270,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp-tile1.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 16670399404877552232,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp-tile2.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 2507897274083364964,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp-tile3.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 7325267995523877959,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp-tile4.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 2181381724594493116,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "red_skp-tile5.png",
-                }],
+                "tiled-images": RED_TILES,
            },
            "green.skp": {
-                # Manually verified these 6 images, all 256x256 tiles,
-                # consistent with a tiled version of the 640x400 green rect
-                # with black borders.
-                "tiled-images": [{
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 12587324416545178013,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp-tile0.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 7624374914829746293,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp-tile1.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 5686489729535631913,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp-tile2.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 7980646035555096146,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp-tile3.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 17817086664365875131,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp-tile4.png",
-                }, {
-                    "checksumAlgorithm" : "bitmap-64bitMD5",
-                    "checksumValue" : 10673669813016809363,
-                    "comparisonResult" : "no-comparison",
-                    "filepath" : "green_skp-tile5.png",
-                }],
+                "tiled-images": GREEN_TILES,
            }
        }
    }