Revert of fix contents of render_pictures JSON summary (https://codereview.chromium...

Reason for revert:
This appears to have caused regressions such as this one: http://108.170.220.120:10117/builders/Perf-Win7-ShuttleA-HD2000-x86-Release/builds/2117/steps/CheckForRegressions/logs/stdio

Original issue's description:
> fix contents of render_pictures JSON summary
>
> BUG=skia:2043,skia:2044,skia:1942
>
> Committed: http://code.google.com/p/skia/source/detail?r=14391

R=scroggo@google.com, epoger@google.com
TBR=epoger@google.com, scroggo@google.com
NOTREECHECKS=true
NOTRY=true
BUG=skia:2043,skia:2044,skia:1942

Author: caryclark@google.com

Review URL: https://codereview.chromium.org/251103004

git-svn-id: http://skia.googlecode.com/svn/trunk@14399 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/tools/PictureRenderer.cpp b/tools/PictureRenderer.cpp
index c712ae6..e531a30 100644 (file)
--- a/tools/PictureRenderer.cpp
+++ b/tools/PictureRenderer.cpp
@@ -323,17 +323,26 @@ uint32_t PictureRenderer::recordFlags() {
 }
 
 /**
- * Write the canvas to an image file and/or JSON summary.
+ * Write the canvas to the specified path.
  *
  * @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;
- *     if empty, don't write out the image at all.
+ * @param outputDir If nonempty, write the binary image to a file within this directory.
  * @param inputFilename If we are writing out a binary image, use this to build its filename.
- * @param jsonSummaryPtr If not null, add image results (checksum) to this summary.
+ * @param jsonSummaryPtr If not null, add image results 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.
  *
- * @return bool True if the operation completed successfully.
+ * 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.
  */
 static bool write(SkCanvas* canvas, const SkString& outputDir, const SkString& inputFilename,
                   ImageResultsSummary *jsonSummaryPtr, bool useChecksumBasedFilenames,
@@ -398,10 +407,8 @@ static bool write(SkCanvas* canvas, const SkString& outputDir, const SkString& i
                             hash, tileNumberPtr);
     }
 
-    if (outputDir.isEmpty()) {
-        return true;
-    }
-
+    SkASSERT(!outputDir.isEmpty()); // TODO(epoger): we want to remove this constraint,
+                                    // as noted above
     SkString dirPath;
     if (outputSubdirPtr) {
         dirPath = SkOSPath::SkPathJoin(outputDir.c_str(), outputSubdirPtr);
@@ -463,13 +470,16 @@ 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 write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
-                 fUseChecksumBasedFilenames);
+    return true;
 }
 
 SkString PipePictureRenderer::getConfigNameInternal() {
@@ -493,13 +503,18 @@ 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 write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
-                 fUseChecksumBasedFilenames);
+
+    return true;
 }
 
 SkString SimplePictureRenderer::getConfigNameInternal() {
@@ -707,8 +722,10 @@ bool TiledPictureRenderer::render(SkBitmap** out) {
     bool success = true;
     for (int i = 0; i < fTileRects.count(); ++i) {
         draw_tile_to_canvas(fCanvas, fTileRects[i], fPicture);
-        success &= write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
-                         fUseChecksumBasedFilenames, &i);
+        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.
@@ -790,8 +807,9 @@ public:
 
         for (int i = fStart; i < fEnd; i++) {
             draw_tile_to_canvas(fCanvas, fRects[i], fClone);
-            if (!write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
-                       fUseChecksumBasedFilenames, &i)
+            if ((!fOutputDir.isEmpty())
+                && !write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
+                          fUseChecksumBasedFilenames, &i)
                 && fSuccess != NULL) {
                 *fSuccess = false;
                 // If one tile fails to write to a file, do not continue drawing the rest.

diff --git a/tools/render_pictures_main.cpp b/tools/render_pictures_main.cpp
index f11eb0b..352dbf3 100644 (file)
--- a/tools/render_pictures_main.cpp
+++ b/tools/render_pictures_main.cpp
@@ -35,8 +35,10 @@ 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.");
-DEFINE_string2(writePath, w, "", "Directory to write the rendered images into.");
+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_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 "
@@ -339,6 +341,8 @@ 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);

diff --git a/tools/tests/render_pictures_test.py b/tools/tests/render_pictures_test.py
index d378a54..3ebed93 100755 (executable)
--- a/tools/tests/render_pictures_test.py
+++ b/tools/tests/render_pictures_test.py
@@ -26,92 +26,6 @@ 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):
 
@@ -125,20 +39,13 @@ class RenderPicturesTest(base_unittest.TestCase):
     shutil.rmtree(self._temp_dir)
 
   def test_tiled_whole_image(self):
-    """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
-    """
+    """Run render_pictures with tiles and --writeWholeImage flag."""
     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',
@@ -149,12 +56,22 @@ class RenderPicturesTest(base_unittest.TestCase):
         "header" : EXPECTED_HEADER_CONTENTS,
         "actual-results" : {
             "red.skp": {
-                "tiled-images": RED_TILES,
-                "whole-image": RED_WHOLEIMAGE,
+                # Manually verified: 640x400 red rectangle with black border
+                "whole-image": {
+                    "checksumAlgorithm" : "bitmap-64bitMD5",
+                    "checksumValue" : 11092453015575919668,
+                    "comparisonResult" : "no-comparison",
+                    "filepath" : "red_skp.png",
+                },
             },
             "green.skp": {
-                "tiled-images": GREEN_TILES,
-                "whole-image": GREEN_WHOLEIMAGE,
+                # Manually verified: 640x400 green rectangle with black border
+                "whole-image": {
+                    "checksumAlgorithm" : "bitmap-64bitMD5",
+                    "checksumValue" : 8891695120562235492,
+                    "comparisonResult" : "no-comparison",
+                    "filepath" : "green_skp.png",
+                },
             }
         }
     }
@@ -169,14 +86,28 @@ 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": {
-                "whole-image": RED_WHOLEIMAGE,
+                # Manually verified: 640x400 red rectangle with black border
+                "whole-image": {
+                    "checksumAlgorithm" : "bitmap-64bitMD5",
+                    "checksumValue" : 11092453015575919668,
+                    "comparisonResult" : "no-comparison",
+                    "filepath" : "red_skp.png",
+                },
             },
             "green.skp": {
-                "whole-image": GREEN_WHOLEIMAGE,
+                # Manually verified: 640x400 green rectangle with black border
+                "whole-image": {
+                    "checksumAlgorithm" : "bitmap-64bitMD5",
+                    "checksumValue" : 8891695120562235492,
+                    "comparisonResult" : "no-comparison",
+                    "filepath" : "green_skp.png",
+                },
             }
         }
     }
@@ -226,44 +157,36 @@ class RenderPicturesTest(base_unittest.TestCase):
         ['bitmap-64bitMD5_8891695120562235492.png'])
 
   def test_untiled_validate(self):
-    """Same as test_untiled, but with --validate."""
+    """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')
+    """
     output_json_path = os.path.join(self._temp_dir, 'output.json')
     self._generate_skps()
-    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'])
+    with self.assertRaises(Exception):
+      self._run_render_pictures(['-r', self._input_skp_dir,
+                                 '--validate',
+                                 '--writePath', self._temp_dir,
+                                 '--writeJsonSummaryPath', output_json_path])
 
   def test_untiled_without_writePath(self):
-    """Same as test_untiled, but without --writePath."""
+    """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')
+    """
     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" : {
-            "red.skp": {
-                "whole-image": RED_WHOLEIMAGE,
-            },
-            "green.skp": {
-                "whole-image": GREEN_WHOLEIMAGE,
-            }
-        }
+        "actual-results" : None,
     }
     self._assert_json_contents(output_json_path, expected_summary_dict)
 
@@ -280,10 +203,76 @@ class RenderPicturesTest(base_unittest.TestCase):
         "header" : EXPECTED_HEADER_CONTENTS,
         "actual-results" : {
             "red.skp": {
-                "tiled-images": RED_TILES,
+                # 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",
+                }],
             },
             "green.skp": {
-                "tiled-images": GREEN_TILES,
+                # 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",
+                }],
             }
         }
     }