add --writeChecksumBasedFilenames flag to render_pictures

BUG=skia:1455,skia:2230
R=robertphillips@google.com, robertphillips@chromium.org

Author: epoger@google.com

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

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

debugger/QT/SkDebuggerGUI.cpp

@@ -319,10 +319,10 @@ void SkDebuggerGUI::run(SkTimedPicture* pict,
         return;
     }
 
-    renderer->init(pict);
+    renderer->init(pict, NULL, NULL, false);
 
     renderer->setup();
-    renderer->render(NULL);
+    renderer->render();
     renderer->resetState(true);    // flush, swapBuffers and Finish
 
     // We throw this away the first batch of times to remove first time effects (such as paging in this program)
@@ -330,7 +330,7 @@ void SkDebuggerGUI::run(SkTimedPicture* pict,
 
     for (int i = 0; i < repeats; ++i) {
         renderer->setup();
-        renderer->render(NULL);
+        renderer->render();
         renderer->resetState(false);  // flush & swapBuffers, but don't Finish
     }
     renderer->resetState(true);    // flush, swapBuffers and Finish

tools/CopyTilesRenderer.cpp

@@ -20,11 +20,17 @@ namespace sk_tools {
     : fXTilesPerLargeTile(x)
     , fYTilesPerLargeTile(y) {
     }
-    void CopyTilesRenderer::init(SkPicture* pict) {
+    void CopyTilesRenderer::init(SkPicture* pict, const SkString* outputDir,
+                                 const SkString* inputFilename, bool useChecksumBasedFilenames) {
+        // Do not call INHERITED::init(), which would create a (potentially large) canvas which is
+        // not used by bench_pictures.
         SkASSERT(pict != NULL);
         // Only work with absolute widths (as opposed to percentages).
         SkASSERT(this->getTileWidth() != 0 && this->getTileHeight() != 0);
         fPicture = pict;
+        this->CopyString(&fOutputDir, outputDir);
+        this->CopyString(&fInputFilename, inputFilename);
+        fUseChecksumBasedFilenames = useChecksumBasedFilenames;
         fPicture->ref();
         this->buildBBoxHierarchy();
         // In order to avoid allocating a large canvas (particularly important for GPU), create one
@@ -34,7 +40,7 @@ namespace sk_tools {
         fCanvas.reset(this->INHERITED::setupCanvas(fLargeTileWidth, fLargeTileHeight));
     }
 
-    bool CopyTilesRenderer::render(const SkString* path, SkBitmap** out) {
+    bool CopyTilesRenderer::render(SkBitmap** out) {
         int i = 0;
         bool success = true;
         SkBitmap dst;
@@ -59,10 +65,14 @@ namespace sk_tools {
                         SkDEBUGCODE(bool extracted =)
                         baseBitmap.extractSubset(&dst, subset);
                         SkASSERT(extracted);
-                        if (path != NULL) {
+                        if (!fOutputDir.isEmpty()) {
-                            // Similar to writeAppendNumber in PictureRenderer.cpp, but just encodes
+                            // Similar to write() in PictureRenderer.cpp, but just encodes
                             // a bitmap directly.
-                            SkString pathWithNumber(*path);
+                            // TODO: Share more common code with write() to do this, to properly
+                            // write out the JSON summary, etc.
+                            SkString pathWithNumber;
+                            make_filepath(&pathWithNumber, fOutputDir, fInputFilename);
+                            pathWithNumber.remove(pathWithNumber.size() - 4, 4);
                             pathWithNumber.appendf("%i.png", i++);
                             SkBitmap copy;
 #if SK_SUPPORT_GPU

tools/CopyTilesRenderer.h

@@ -23,13 +23,14 @@ namespace sk_tools {
 
     public:
         CopyTilesRenderer(int x, int y);
-        virtual void init(SkPicture* pict) SK_OVERRIDE;
+        virtual void init(SkPicture* pict, const SkString* outputDir, const SkString* inputFilename,
+                          bool useChecksumBasedFilenames) SK_OVERRIDE;
 
         /**
          *  Similar to TiledPictureRenderer, this will draw a PNG for each tile. However, the
          *  numbering (and actual tiles) will be different.
          */
-        virtual bool render(const SkString* path, SkBitmap** out) SK_OVERRIDE;
+        virtual bool render(SkBitmap** out) SK_OVERRIDE;
 
         virtual bool supportsTimingIndividualTiles() SK_OVERRIDE { return false; }
 

tools/PictureBenchmark.cpp

@@ -73,7 +73,7 @@ void PictureBenchmark::run(SkPicture* pict) {
         return;
     }
 
-    fRenderer->init(pict);
+    fRenderer->init(pict, NULL, NULL, false);
 
     // We throw this away to remove first time effects (such as paging in this program)
     fRenderer->setup();

tools/PictureRenderer.cpp

@@ -58,15 +58,19 @@ const static char kJsonKey_ActualResults[]   = "actual-results";
 const static char kJsonKey_ActualResults_NoComparison[]  = "no-comparison";
 const static char kJsonKey_Hashtype_Bitmap_64bitMD5[]  = "bitmap-64bitMD5";
 
-void ImageResultsSummary::add(const char *testName, const SkBitmap& bitmap) {
+void ImageResultsSummary::add(const char *testName, uint64_t hash) {
-    uint64_t hash;
-    SkAssertResult(SkBitmapHasher::ComputeDigest(bitmap, &hash));
     Json::Value jsonTypeValuePair;
     jsonTypeValuePair.append(Json::Value(kJsonKey_Hashtype_Bitmap_64bitMD5));
     jsonTypeValuePair.append(Json::UInt64(hash));
     fActualResultsNoComparison[testName] = jsonTypeValuePair;
 }
 
+void ImageResultsSummary::add(const char *testName, const SkBitmap& bitmap) {
+    uint64_t hash;
+    SkAssertResult(SkBitmapHasher::ComputeDigest(bitmap, &hash));
+    this->add(testName, hash);
+}
+
 void ImageResultsSummary::writeToFile(const char *filename) {
     Json::Value actualResults;
     actualResults[kJsonKey_ActualResults_NoComparison] = fActualResultsNoComparison;
@@ -77,7 +81,12 @@ void ImageResultsSummary::writeToFile(const char *filename) {
     stream.write(jsonStdString.c_str(), jsonStdString.length());
 }
 
-void PictureRenderer::init(SkPicture* pict) {
+void PictureRenderer::init(SkPicture* pict, const SkString* outputDir,
+                           const SkString* inputFilename, bool useChecksumBasedFilenames) {
+    this->CopyString(&fOutputDir, outputDir);
+    this->CopyString(&fInputFilename, inputFilename);
+    fUseChecksumBasedFilenames = useChecksumBasedFilenames;
+
     SkASSERT(NULL == fPicture);
     SkASSERT(NULL == fCanvas.get());
     if (fPicture != NULL || NULL != fCanvas.get()) {
@@ -94,6 +103,14 @@ void PictureRenderer::init(SkPicture* pict) {
     fCanvas.reset(this->setupCanvas());
 }
 
+void PictureRenderer::CopyString(SkString* dest, const SkString* src) {
+    if (NULL != src) {
+        dest->set(*src);
+    } else {
+        dest->reset();
+    }
+}
+
 class FlagsDrawFilter : public SkDrawFilter {
 public:
     FlagsDrawFilter(PictureRenderer::DrawFilterFlags* flags) :
@@ -281,10 +298,13 @@ uint32_t PictureRenderer::recordFlags() {
 
 /**
  * Write the canvas to the specified path.
+ *
  * @param canvas Must be non-null. Canvas to be written to a file.
- * @param path Path for the file to be written. Should have no extension; write() will append
+ * @param outputDir If nonempty, write the binary image to a file within this directory.
- *             an appropriate one. Passed in by value so it can be modified.
+ * @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 useChecksumBasedFilenames If true, use checksum-based filenames when writing to disk.
+ * @param numberToAppend If not null, append this number to the filename.
  * @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
@@ -295,53 +315,64 @@ uint32_t PictureRenderer::recordFlags() {
  * 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* path, ImageResultsSummary *jsonSummaryPtr) {
+static bool write(SkCanvas* canvas, const SkString& outputDir, const SkString& inputFilename,
+                  ImageResultsSummary *jsonSummaryPtr, bool useChecksumBasedFilenames,
+                  const int* numberToAppend=NULL) {
     SkASSERT(canvas != NULL);
     if (NULL == canvas) {
         return false;
     }
 
-    SkASSERT(path != NULL);  // TODO(epoger): we want to remove this constraint, as noted above
-    SkString fullPathname(*path);
-    fullPathname.append(".png");
-
     SkBitmap bitmap;
     SkISize size = canvas->getDeviceSize();
     sk_tools::setup_bitmap(&bitmap, size.width(), size.height());
 
+    // Make sure we only compute the bitmap hash once (at most).
+    uint64_t hash;
+    bool generatedHash = false;
+
     canvas->readPixels(&bitmap, 0, 0);
     sk_tools::force_all_opaque(bitmap);
 
+    SkString outputFilename(inputFilename);
+    outputFilename.remove(outputFilename.size() - 4, 4);
+    if (NULL != numberToAppend) {
+        outputFilename.appendf("%i", *numberToAppend);
+    }
+    outputFilename.append(".png");
+    // TODO(epoger): what about including the config type within outputFilename?  That way,
+    // we could combine results of different config types without conflicting filenames.
+
     if (NULL != jsonSummaryPtr) {
-        // EPOGER: This is a hacky way of constructing the filename associated with the
+        if (!generatedHash) {
-        // image checksum; we assume that outputDir is not NULL, and we remove outputDir
+            SkAssertResult(SkBitmapHasher::ComputeDigest(bitmap, &hash));
-        // from fullPathname.
+            generatedHash = true;
-        //
+        }
-        // EPOGER: what about including the config type within hashFilename?  That way,
+        jsonSummaryPtr->add(outputFilename.c_str(), hash);
-        // we could combine results of different config types without conflicting filenames.
-        SkString hashFilename;
-        sk_tools::get_basename(&hashFilename, fullPathname);
-        jsonSummaryPtr->add(hashFilename.c_str(), bitmap);
     }
 
+    // Update outputFilename AFTER adding to JSON summary, but BEFORE writing out the image file.
+    if (useChecksumBasedFilenames) {
+        if (!generatedHash) {
+            SkAssertResult(SkBitmapHasher::ComputeDigest(bitmap, &hash));
+            generatedHash = true;
+        }
+        outputFilename.set(kJsonKey_Hashtype_Bitmap_64bitMD5);
+        outputFilename.append("_");
+        outputFilename.appendU64(hash);
+        outputFilename.append(".png");
+    }
+
+    SkASSERT(!outputDir.isEmpty()); // TODO(epoger): we want to remove this constraint,
+                                    // as noted above
+    SkString fullPathname;
+    make_filepath(&fullPathname, outputDir, outputFilename);
     return SkImageEncoder::EncodeFile(fullPathname.c_str(), bitmap, SkImageEncoder::kPNG_Type, 100);
 }
 
-/**
- * If path is non NULL, append number to it, and call write() to write the
- * provided canvas to a file. Returns true if path is NULL or if write() succeeds.
- */
-static bool writeAppendNumber(SkCanvas* canvas, const SkString* path, int number,
-                              ImageResultsSummary *jsonSummaryPtr) {
-    if (NULL == path) {
-        return true;
-    }
-    SkString pathWithNumber(*path);
-    pathWithNumber.appendf("%i", number);
-    return write(canvas, &pathWithNumber, jsonSummaryPtr);
-}
-
 ///////////////////////////////////////////////////////////////////////////////////////////////
 
 SkCanvas* RecordPictureRenderer::setupCanvas(int width, int height) {
@@ -354,18 +385,17 @@ static SkData* encode_bitmap_to_data(size_t*, const SkBitmap& bm) {
     return SkImageEncoder::EncodeData(bm, SkImageEncoder::kPNG_Type, 100);
 }
 
-bool RecordPictureRenderer::render(const SkString* path, SkBitmap** out) {
+bool RecordPictureRenderer::render(SkBitmap** out) {
     SkAutoTUnref<SkPicture> replayer(this->createPicture());
     SkCanvas* recorder = replayer->beginRecording(this->getViewWidth(), this->getViewHeight(),
                                                   this->recordFlags());
     this->scaleToScaleFactor(recorder);
     fPicture->draw(recorder);
     replayer->endRecording();
-    if (path != NULL) {
+    if (!fOutputDir.isEmpty()) {
         // Record the new picture as a new SKP with PNG encoded bitmaps.
-        SkString skpPath(*path);
+        SkString skpPath;
-        // ".skp" was removed from 'path' before being passed in here.
+        make_filepath(&skpPath, fOutputDir, fInputFilename);
-        skpPath.append(".skp");
         SkFILEWStream stream(skpPath.c_str());
         replayer->serialize(&stream, &encode_bitmap_to_data);
         return true;
@@ -379,7 +409,7 @@ SkString RecordPictureRenderer::getConfigNameInternal() {
 
 ///////////////////////////////////////////////////////////////////////////////////////////////
 
-bool PipePictureRenderer::render(const SkString* path, SkBitmap** out) {
+bool PipePictureRenderer::render(SkBitmap** out) {
     SkASSERT(fCanvas.get() != NULL);
     SkASSERT(fPicture != NULL);
     if (NULL == fCanvas.get() || NULL == fPicture) {
@@ -392,8 +422,9 @@ bool PipePictureRenderer::render(const SkString* path, SkBitmap** out) {
     pipeCanvas->drawPicture(*fPicture);
     writer.endRecording();
     fCanvas->flush();
-    if (NULL != path) {
+    if (!fOutputDir.isEmpty()) {
-        return write(fCanvas, path, fJsonSummaryPtr);
+        return write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
+                     fUseChecksumBasedFilenames);
     }
     if (NULL != out) {
         *out = SkNEW(SkBitmap);
@@ -409,12 +440,13 @@ SkString PipePictureRenderer::getConfigNameInternal() {
 
 ///////////////////////////////////////////////////////////////////////////////////////////////
 
-void SimplePictureRenderer::init(SkPicture* picture) {
+void SimplePictureRenderer::init(SkPicture* picture, const SkString* outputDir,
-    INHERITED::init(picture);
+                                 const SkString* inputFilename, bool useChecksumBasedFilenames) {
+    INHERITED::init(picture, outputDir, inputFilename, useChecksumBasedFilenames);
     this->buildBBoxHierarchy();
 }
 
-bool SimplePictureRenderer::render(const SkString* path, SkBitmap** out) {
+bool SimplePictureRenderer::render(SkBitmap** out) {
     SkASSERT(fCanvas.get() != NULL);
     SkASSERT(fPicture != NULL);
     if (NULL == fCanvas.get() || NULL == fPicture) {
@@ -423,8 +455,9 @@ bool SimplePictureRenderer::render(const SkString* path, SkBitmap** out) {
 
     fCanvas->drawPicture(*fPicture);
     fCanvas->flush();
-    if (NULL != path) {
+    if (!fOutputDir.isEmpty()) {
-        return write(fCanvas, path, fJsonSummaryPtr);
+        return write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
+                     fUseChecksumBasedFilenames);
     }
 
     if (NULL != out) {
@@ -452,7 +485,8 @@ TiledPictureRenderer::TiledPictureRenderer()
     , fTilesX(0)
     , fTilesY(0) { }
 
-void TiledPictureRenderer::init(SkPicture* pict) {
+void TiledPictureRenderer::init(SkPicture* pict, const SkString* outputDir,
+                                const SkString* inputFilename, bool useChecksumBasedFilenames) {
     SkASSERT(pict != NULL);
     SkASSERT(0 == fTileRects.count());
     if (NULL == pict || fTileRects.count() != 0) {
@@ -462,6 +496,9 @@ void TiledPictureRenderer::init(SkPicture* pict) {
     // Do not call INHERITED::init(), which would create a (potentially large) canvas which is not
     // used by bench_pictures.
     fPicture = pict;
+    this->CopyString(&fOutputDir, outputDir);
+    this->CopyString(&fInputFilename, inputFilename);
+    fUseChecksumBasedFilenames = useChecksumBasedFilenames;
     fPicture->ref();
     this->buildBBoxHierarchy();
 
@@ -623,7 +660,7 @@ void TiledPictureRenderer::drawCurrentTile() {
     draw_tile_to_canvas(fCanvas, fTileRects[fCurrentTileOffset], fPicture);
 }
 
-bool TiledPictureRenderer::render(const SkString* path, SkBitmap** out) {
+bool TiledPictureRenderer::render(SkBitmap** out) {
     SkASSERT(fPicture != NULL);
     if (NULL == fPicture) {
         return false;
@@ -638,8 +675,9 @@ bool TiledPictureRenderer::render(const SkString* path, SkBitmap** out) {
     bool success = true;
     for (int i = 0; i < fTileRects.count(); ++i) {
         draw_tile_to_canvas(fCanvas, fTileRects[i], fPicture);
-        if (NULL != path) {
+        if (!fOutputDir.isEmpty()) {
-            success &= writeAppendNumber(fCanvas, path, i, fJsonSummaryPtr);
+            success &= write(fCanvas, fOutputDir, fInputFilename, fJsonSummaryPtr,
+                             fUseChecksumBasedFilenames, &i);
         }
         if (NULL != out) {
             if (fCanvas->readPixels(&bitmap, 0, 0)) {
@@ -698,16 +736,16 @@ class CloneData : public SkRunnable {
 
 public:
     CloneData(SkPicture* clone, SkCanvas* canvas, SkTDArray<SkRect>& rects, int start, int end,
-              SkRunnable* done, ImageResultsSummary* jsonSummaryPtr)
+              SkRunnable* done, ImageResultsSummary* jsonSummaryPtr, bool useChecksumBasedFilenames)
         : fClone(clone)
         , fCanvas(canvas)
-        , fPath(NULL)
         , fRects(rects)
         , fStart(start)
         , fEnd(end)
         , fSuccess(NULL)
         , fDone(done)
-        , fJsonSummaryPtr(jsonSummaryPtr) {
+        , fJsonSummaryPtr(jsonSummaryPtr)
+        , fUseChecksumBasedFilenames(useChecksumBasedFilenames) {
         SkASSERT(fDone != NULL);
     }
 
@@ -722,7 +760,9 @@ public:
 
         for (int i = fStart; i < fEnd; i++) {
             draw_tile_to_canvas(fCanvas, fRects[i], fClone);
-            if ((fPath != NULL) && !writeAppendNumber(fCanvas, fPath, i, fJsonSummaryPtr)
+            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.
@@ -743,8 +783,10 @@ public:
         fDone->run();
     }
 
-    void setPathAndSuccess(const SkString* path, bool* success) {
+    void setPathsAndSuccess(const SkString& outputDir, const SkString& inputFilename,
-        fPath = path;
+                            bool* success) {
+        fOutputDir.set(outputDir);
+        fInputFilename.set(inputFilename);
         fSuccess = success;
     }
 
@@ -757,7 +799,8 @@ private:
     SkPicture*         fClone;      // Picture to draw from. Each CloneData has a unique one which
                                     // is threadsafe.
     SkCanvas*          fCanvas;     // Canvas to draw to. Reused for each tile.
-    const SkString*    fPath;       // If non-null, path to write the result to as a PNG.
+    SkString           fOutputDir;  // If not empty, write results into this directory.
+    SkString           fInputFilename; // Filename of input SkPicture file.
     SkTDArray<SkRect>& fRects;      // All tiles of the picture.
     const int          fStart;      // Range of tiles drawn by this thread.
     const int          fEnd;
@@ -766,6 +809,7 @@ private:
     SkRunnable*        fDone;
     SkBitmap*          fBitmap;
     ImageResultsSummary* fJsonSummaryPtr;
+    bool               fUseChecksumBasedFilenames;
 };
 
 MultiCorePictureRenderer::MultiCorePictureRenderer(int threadCount)
@@ -778,9 +822,10 @@ MultiCorePictureRenderer::MultiCorePictureRenderer(int threadCount)
     fCloneData = SkNEW_ARRAY(CloneData*, fNumThreads);
 }
 
-void MultiCorePictureRenderer::init(SkPicture *pict) {
+void MultiCorePictureRenderer::init(SkPicture *pict, const SkString* outputDir,
+                                    const SkString* inputFilename, bool useChecksumBasedFilenames) {
     // Set fPicture and the tiles.
-    this->INHERITED::init(pict);
+    this->INHERITED::init(pict, outputDir, inputFilename, useChecksumBasedFilenames);
     for (int i = 0; i < fNumThreads; ++i) {
         *fCanvasPool.append() = this->setupCanvas(this->getTileWidth(), this->getTileHeight());
     }
@@ -802,15 +847,15 @@ void MultiCorePictureRenderer::init(SkPicture *pict) {
         const int end = SkMin32(start + chunkSize, fTileRects.count());
         fCloneData[i] = SkNEW_ARGS(CloneData,
                                    (pic, fCanvasPool[i], fTileRects, start, end, &fCountdown,
-                                    fJsonSummaryPtr));
+                                    fJsonSummaryPtr, useChecksumBasedFilenames));
     }
 }
 
-bool MultiCorePictureRenderer::render(const SkString *path, SkBitmap** out) {
+bool MultiCorePictureRenderer::render(SkBitmap** out) {
     bool success = true;
-    if (path != NULL) {
+    if (!fOutputDir.isEmpty()) {
         for (int i = 0; i < fNumThreads-1; i++) {
-            fCloneData[i]->setPathAndSuccess(path, &success);
+            fCloneData[i]->setPathsAndSuccess(fOutputDir, fInputFilename, &success);
         }
     }
 
@@ -868,7 +913,7 @@ void PlaybackCreationRenderer::setup() {
     fPicture->draw(recorder);
 }
 
-bool PlaybackCreationRenderer::render(const SkString*, SkBitmap** out) {
+bool PlaybackCreationRenderer::render(SkBitmap** out) {
     fReplayer->endRecording();
     // Since this class does not actually render, return false.
     return false;
@@ -915,14 +960,14 @@ SkPicture* PictureRenderer::createPicture() {
 
 class GatherRenderer : public PictureRenderer {
 public:
-    virtual bool render(const SkString* path, SkBitmap** out = NULL)
+    virtual bool render(SkBitmap** out = NULL)
             SK_OVERRIDE {
         SkRect bounds = SkRect::MakeWH(SkIntToScalar(fPicture->width()),
                                        SkIntToScalar(fPicture->height()));
         SkData* data = SkPictureUtils::GatherPixelRefs(fPicture, bounds);
         SkSafeUnref(data);
 
-        return NULL == path;    // we don't have anything to write
+        return (fOutputDir.isEmpty());    // we don't have anything to write
     }
 
 private:
@@ -939,14 +984,14 @@ PictureRenderer* CreateGatherPixelRefsRenderer() {
 
 class PictureCloneRenderer : public PictureRenderer {
 public:
-    virtual bool render(const SkString* path, SkBitmap** out = NULL)
+    virtual bool render(SkBitmap** out = NULL)
             SK_OVERRIDE {
         for (int i = 0; i < 100; ++i) {
             SkPicture* clone = fPicture->clone();
             SkSafeUnref(clone);
         }
 
-        return NULL == path;    // we don't have anything to write
+        return (fOutputDir.isEmpty());    // we don't have anything to write
     }
 
 private:

tools/PictureRenderer.h

@@ -43,6 +43,14 @@ class TiledPictureRenderer;
  */
 class ImageResultsSummary {
 public:
+    /**
+     * Adds this bitmap hash to the summary of results.
+     *
+     * @param testName name of the test
+     * @param hash hash to store
+     */
+    void add(const char *testName, uint64_t hash);
+
     /**
      * Adds this bitmap's hash to the summary of results.
      *
@@ -105,8 +113,16 @@ public:
 
     /**
      * Called with each new SkPicture to render.
+     *
+     * @param pict The SkPicture to render.
+     * @param outputDir The output directory within which this renderer should write files,
+     *     or NULL if this renderer should not write files at all.
+     * @param inputFilename The name of the input file we are rendering.
+     * @param useChecksumBasedFilenames Whether to use checksum-based filenames when writing
+     *     bitmap images to disk.
      */
-    virtual void init(SkPicture* pict);
+    virtual void init(SkPicture* pict, const SkString* outputDir,
+                      const SkString* inputFilename, bool useChecksumBasedFilenames);
 
     /**
      *  Set the viewport so that only the portion listed gets drawn.
@@ -125,15 +141,20 @@ public:
     virtual void setup() {}
 
     /**
-     * Perform work that is to be timed. Typically this is rendering, but is also used for recording
+     * Perform the work.  If this is being called within the context of bench_pictures,
-     * and preparing picture for playback by the subclasses which do those.
+     * this is the step that will be timed.
-     * If path is non-null, subclass implementations should call write().
+     *
-     * @param path If non-null, also write the output to the file specified by path. path should
+     * Typically "the work" is rendering an SkPicture into a bitmap, but in some subclasses
-     *             have no extension; it will be added by write().
+     * it is recording the source SkPicture into another SkPicture.
-     * @return bool True if rendering succeeded and, if path is non-null, the output was
+     *
-     *             successfully written to a file.
+     * If fOutputDir has been specified, the result of the work will be written to that dir.
+     *
+     * @param out If non-null, the implementing subclass MAY allocate an SkBitmap, copy the
+     *            output image into it, and return it here.  (Some subclasses ignore this parameter)
+     * @return bool True if rendering succeeded and, if fOutputDir had been specified, the output
+     *              was successfully written to a file.
      */
-    virtual bool render(const SkString* path, SkBitmap** out = NULL) = 0;
+    virtual bool render(SkBitmap** out = NULL) = 0;
 
     /**
      * Called once finished with a particular SkPicture, before calling init again, and before
@@ -371,11 +392,14 @@ public:
 protected:
     SkAutoTUnref<SkCanvas> fCanvas;
     SkPicture*             fPicture;
+    bool                   fUseChecksumBasedFilenames;
     ImageResultsSummary*   fJsonSummaryPtr;
     SkDeviceTypes          fDeviceType;
     BBoxHierarchyType      fBBoxHierarchyType;
     DrawFilterFlags        fDrawFilters[SkDrawFilter::kTypeCount];
     SkString               fDrawFiltersConfig;
+    SkString               fOutputDir;
+    SkString               fInputFilename;
     SkTileGridPicture::TileGridInfo fGridInfo; // used when fBBoxHierarchyType is TileGrid
 
     void buildBBoxHierarchy();
@@ -402,6 +426,11 @@ protected:
     SkCanvas* setupCanvas();
     virtual SkCanvas* setupCanvas(int width, int height);
 
+    /**
+     * Copy src to dest; if src==NULL, set dest to empty string.
+     */
+    static void CopyString(SkString* dest, const SkString* src);
+
 private:
     SkISize                fViewport;
     SkScalar               fScaleFactor;
@@ -421,7 +450,7 @@ private:
  * to time.
  */
 class RecordPictureRenderer : public PictureRenderer {
-    virtual bool render(const SkString*, SkBitmap** out = NULL) SK_OVERRIDE;
+    virtual bool render(SkBitmap** out = NULL) SK_OVERRIDE;
 
     virtual SkString getPerIterTimeFormat() SK_OVERRIDE { return SkString("%.4f"); }
 
@@ -436,7 +465,7 @@ private:
 
 class PipePictureRenderer : public PictureRenderer {
 public:
-    virtual bool render(const SkString*, SkBitmap** out = NULL) SK_OVERRIDE;
+    virtual bool render(SkBitmap** out = NULL) SK_OVERRIDE;
 
 private:
     virtual SkString getConfigNameInternal() SK_OVERRIDE;
@@ -446,9 +475,10 @@ private:
 
 class SimplePictureRenderer : public PictureRenderer {
 public:
-    virtual void init(SkPicture* pict) SK_OVERRIDE;
+    virtual void init(SkPicture* pict, const SkString* outputDir,
+                      const SkString* inputFilename, bool useChecksumBasedFilenames) SK_OVERRIDE;
 
-    virtual bool render(const SkString*, SkBitmap** out = NULL) SK_OVERRIDE;
+    virtual bool render(SkBitmap** out = NULL) SK_OVERRIDE;
 
 private:
     virtual SkString getConfigNameInternal() SK_OVERRIDE;
@@ -460,14 +490,16 @@ class TiledPictureRenderer : public PictureRenderer {
 public:
     TiledPictureRenderer();
 
-    virtual void init(SkPicture* pict) SK_OVERRIDE;
+    virtual void init(SkPicture* pict, const SkString* outputDir,
+                      const SkString* inputFilename, bool useChecksumBasedFilenames) SK_OVERRIDE;
 
     /**
-     * Renders to tiles, rather than a single canvas. If a path is provided, a separate file is
+     * Renders to tiles, rather than a single canvas.
+     * If fOutputDir was provided, a separate file is
      * created for each tile, named "path0.png", "path1.png", etc.
      * Multithreaded mode currently does not support writing to a file.
      */
-    virtual bool render(const SkString* path, SkBitmap** out = NULL) SK_OVERRIDE;
+    virtual bool render(SkBitmap** out = NULL) SK_OVERRIDE;
 
     virtual void end() SK_OVERRIDE;
 
@@ -583,12 +615,13 @@ public:
 
     ~MultiCorePictureRenderer();
 
-    virtual void init(SkPicture* pict) SK_OVERRIDE;
+    virtual void init(SkPicture* pict, const SkString* outputDir,
+                      const SkString* inputFilename, bool useChecksumBasedFilenames) SK_OVERRIDE;
 
     /**
      * Behaves like TiledPictureRenderer::render(), only using multiple threads.
      */
-    virtual bool render(const SkString* path, SkBitmap** out = NULL) SK_OVERRIDE;
+    virtual bool render(SkBitmap** out = NULL) SK_OVERRIDE;
 
     virtual void end() SK_OVERRIDE;
 
@@ -615,7 +648,7 @@ class PlaybackCreationRenderer : public PictureRenderer {
 public:
     virtual void setup() SK_OVERRIDE;
 
-    virtual bool render(const SkString*, SkBitmap** out = NULL) SK_OVERRIDE;
+    virtual bool render(SkBitmap** out = NULL) SK_OVERRIDE;
 
     virtual SkString getPerIterTimeFormat() SK_OVERRIDE { return SkString("%.4f"); }
 

tools/bbh_shootout.cpp

@@ -67,16 +67,16 @@ static void do_benchmark_work(sk_tools::PictureRenderer* renderer,
         BenchTimer* timer) {
     renderer->setBBoxHierarchyType(bBoxType);
     renderer->setGridSize(FLAGS_tilesize, FLAGS_tilesize);
-    renderer->init(pic);
+    renderer->init(pic, NULL, NULL, false);
 
     SkDebugf("%s %d times...\n", renderer->getConfigName().c_str(), numRepeats);
     for (int i = 0; i < numRepeats; ++i) {
         renderer->setup();
         // Render once to fill caches
-        renderer->render(NULL, NULL);
+        renderer->render();
         // Render again to measure
         timer->start();
-        renderer->render(NULL);
+        renderer->render();
         timer->end();
     }
 }

tools/render_pictures_main.cpp

@@ -29,6 +29,8 @@ DEFINE_int32(maxComponentDiff, 256, "Maximum diff on a component, 0 - 256. Compo
              "by more than this amount are considered errors, though all diffs are reported. "
              "Requires --validate.");
 DECLARE_string(readPath);
+DEFINE_bool(writeChecksumBasedFilenames, false,
+            "When writing out images, use checksum-based filenames.");
 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.");
@@ -145,6 +147,10 @@ static bool render_picture_internal(const SkString& inputPath, const SkString* o
                                     SkBitmap** out) {
     SkString inputFilename;
     sk_tools::get_basename(&inputFilename, inputPath);
+    SkString outputDirString;
+    if (NULL != outputDir && outputDir->size() > 0 && !FLAGS_writeEncodedImages) {
+        outputDirString.set(*outputDir);
+    }
 
     SkFILEStream inputStream;
     inputStream.setPath(inputPath.c_str());
@@ -189,7 +195,7 @@ static bool render_picture_internal(const SkString& inputPath, const SkString* o
     SkDebugf("drawing... [%i %i] %s\n", picture->width(), picture->height(),
              inputPath.c_str());
 
-    renderer.init(picture);
+    renderer.init(picture, &outputDirString, &inputFilename, FLAGS_writeChecksumBasedFilenames);
 
     if (FLAGS_preprocess) {
         if (NULL != renderer.getCanvas()) {
@@ -199,18 +205,9 @@ static bool render_picture_internal(const SkString& inputPath, const SkString* o
 
     renderer.setup();
 
-    SkString* outputPath = NULL;
+    bool success = renderer.render(out);
-    if (NULL != outputDir && outputDir->size() > 0 && !FLAGS_writeEncodedImages) {
+    if (!success) {
-        outputPath = SkNEW(SkString);
+        SkDebugf("Failed to render %s\n", inputFilename.c_str());
-        make_output_filepath(outputPath, *outputDir, inputFilename);
-    }
-
-    bool success = renderer.render(outputPath, out);
-    if (outputPath) {
-        if (!success) {
-            SkDebugf("Could not write to file %s\n", outputPath->c_str());
-        }
-        SkDELETE(outputPath);
     }
 
     renderer.end();
@@ -352,13 +349,13 @@ static bool render_picture(const SkString& inputPath, const SkString* outputDir,
         sk_tools::force_all_opaque(*bitmap);
 
         if (NULL != jsonSummaryPtr) {
-            // EPOGER: This is a hacky way of constructing the filename associated with the
+            // TODO(epoger): This is a hacky way of constructing the filename associated with the
             // image checksum; we basically are repeating the logic of make_output_filepath()
             // and code below here, within here.
             // 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.
             //
-            // EPOGER: what about including the config type within hashFilename?  That way,
+            // TODO(epoger): what about including the config type within hashFilename?  That way,
             // we could combine results of different config types without conflicting filenames.
             SkString hashFilename;
             sk_tools::get_basename(&hashFilename, inputPath);

tools/tests/render_pictures_test.py

@@ -37,10 +37,15 @@ class RenderPicturesTest(base_unittest.TestCase):
     """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.png and green.png to the current working
+    # directory.  We should fix that... if --writePath is not specified, this
+    # probably shouldn't write out red.png and green.png at all!
     self._run_render_pictures(['-r', self._input_skp_dir,
                                '--bbh', 'grid', '256', '256',
                                '--mode', 'tile', '256', '256',
                                '--writeJsonSummaryPath', output_json_path,
+                               '--writePath', self._temp_dir,
                                '--writeWholeImage'])
     expected_summary_dict = {
         "actual-results" : {
@@ -53,6 +58,8 @@ class RenderPicturesTest(base_unittest.TestCase):
         }
     }
     self._assert_json_contents(output_json_path, expected_summary_dict)
+    self._assert_directory_contents(
+        self._temp_dir, ['red.png', 'green.png', 'output.json'])
 
   def test_untiled(self):
     """Run without tiles."""
@@ -72,6 +79,32 @@ class RenderPicturesTest(base_unittest.TestCase):
         }
     }
     self._assert_json_contents(output_json_path, expected_summary_dict)
+    self._assert_directory_contents(
+        self._temp_dir, ['red.png', 'green.png', 'output.json'])
+
+  def test_untiled_writeChecksumBasedFilenames(self):
+    """Same as test_untiled, but with --writeChecksumBasedFilenames."""
+    output_json_path = os.path.join(self._temp_dir, 'output.json')
+    self._generate_skps()
+    self._run_render_pictures(['-r', self._input_skp_dir,
+                               '--writeChecksumBasedFilenames',
+                               '--writePath', self._temp_dir,
+                               '--writeJsonSummaryPath', output_json_path])
+    expected_summary_dict = {
+        "actual-results" : {
+            "no-comparison" : {
+                # Manually verified: 640x400 red rectangle with black border
+                "red.png" : ["bitmap-64bitMD5", 11092453015575919668],
+                # Manually verified: 640x400 green rectangle with black border
+                "green.png" : ["bitmap-64bitMD5", 8891695120562235492],
+            }
+        }
+    }
+    self._assert_json_contents(output_json_path, expected_summary_dict)
+    self._assert_directory_contents(
+        self._temp_dir, ['bitmap-64bitMD5_11092453015575919668.png',
+                         'bitmap-64bitMD5_8891695120562235492.png',
+                         'output.json'])
 
   def test_untiled_validate(self):
     """Same as test_untiled, but with --validate.
@@ -142,6 +175,62 @@ class RenderPicturesTest(base_unittest.TestCase):
         }
     }
     self._assert_json_contents(output_json_path, expected_summary_dict)
+    self._assert_directory_contents(
+        self._temp_dir,
+        ['red0.png', 'red1.png', 'red2.png', 'red3.png', 'red4.png', 'red5.png',
+         'green0.png', 'green1.png', 'green2.png', 'green3.png', 'green4.png',
+         'green5.png', 'output.json'])
+
+  def test_tiled_writeChecksumBasedFilenames(self):
+    """Same as test_tiled, but with --writeChecksumBasedFilenames."""
+    output_json_path = os.path.join(self._temp_dir, 'output.json')
+    self._generate_skps()
+    self._run_render_pictures(['-r', self._input_skp_dir,
+                               '--bbh', 'grid', '256', '256',
+                               '--mode', 'tile', '256', '256',
+                               '--writeChecksumBasedFilenames',
+                               '--writePath', self._temp_dir,
+                               '--writeJsonSummaryPath', output_json_path])
+    expected_summary_dict = {
+        "actual-results" : {
+            "no-comparison" : {
+                # Manually verified these 6 images, all 256x256 tiles,
+                # consistent with a tiled version of the 640x400 red rect
+                # with black borders.
+                "red0.png" : ["bitmap-64bitMD5", 5815827069051002745],
+                "red1.png" : ["bitmap-64bitMD5", 9323613075234140270],
+                "red2.png" : ["bitmap-64bitMD5", 16670399404877552232],
+                "red3.png" : ["bitmap-64bitMD5", 2507897274083364964],
+                "red4.png" : ["bitmap-64bitMD5", 7325267995523877959],
+                "red5.png" : ["bitmap-64bitMD5", 2181381724594493116],
+                # Manually verified these 6 images, all 256x256 tiles,
+                # consistent with a tiled version of the 640x400 green rect
+                # with black borders.
+                "green0.png" : ["bitmap-64bitMD5", 12587324416545178013],
+                "green1.png" : ["bitmap-64bitMD5", 7624374914829746293],
+                "green2.png" : ["bitmap-64bitMD5", 5686489729535631913],
+                "green3.png" : ["bitmap-64bitMD5", 7980646035555096146],
+                "green4.png" : ["bitmap-64bitMD5", 17817086664365875131],
+                "green5.png" : ["bitmap-64bitMD5", 10673669813016809363],
+            }
+        }
+    }
+    self._assert_json_contents(output_json_path, expected_summary_dict)
+    self._assert_directory_contents(
+        self._temp_dir,
+        ['bitmap-64bitMD5_5815827069051002745.png',
+         'bitmap-64bitMD5_9323613075234140270.png',
+         'bitmap-64bitMD5_16670399404877552232.png',
+         'bitmap-64bitMD5_2507897274083364964.png',
+         'bitmap-64bitMD5_7325267995523877959.png',
+         'bitmap-64bitMD5_2181381724594493116.png',
+         'bitmap-64bitMD5_12587324416545178013.png',
+         'bitmap-64bitMD5_7624374914829746293.png',
+         'bitmap-64bitMD5_5686489729535631913.png',
+         'bitmap-64bitMD5_7980646035555096146.png',
+         'bitmap-64bitMD5_17817086664365875131.png',
+         'bitmap-64bitMD5_10673669813016809363.png',
+         'output.json'])
 
   def _run_render_pictures(self, args):
     binary = self.find_path_to_program('render_pictures')
@@ -179,6 +268,20 @@ class RenderPicturesTest(base_unittest.TestCase):
                              '--writePath', str(output_path),
                              ])
 
+  def _assert_directory_contents(self, dir_path, expected_filenames):
+    """Asserts that files found in a dir are identical to expected_filenames.
+
+    Args:
+      dir_path: Path to a directory on local disk.
+      expected_filenames: Set containing the expected filenames within the dir.
+
+    Raises:
+      AssertionError: contents of the directory are not identical to
+                      expected_filenames.
+    """
+    self.assertEqual(set(os.listdir(dir_path)), set(expected_filenames))
+
+
   def _assert_json_contents(self, json_path, expected_dict):
     """Asserts that contents of a JSON file are identical to expected_dict.