Replace SkPicture(SkStream) constructors with a factory.

SkPicture:
Remove the constructors which take an SkStream as an argument. Rather
than having to check a variable for success, the factory will return
NULL on failure.
Add a protected function for determining if an SkStream is an SKP
to share code with SkTimedPicture.
In the factory, check for a NULL SkStream.
Use a default decoder (from BUG:
https://code.google.com/p/skia/issues/detail?id=1325)

SkDebuggerGUI:
Call SkPicture::CreateFromStream when necessary.
Write a factory for creating SkTimedPictures and use it.

Use the factory throughout tools.

Add include/lazy to utils and effects gyp include_dirs so SkPicture.h
can reference SkImageDecoder.h which references SkBitmapFactory.h (in
include/lazy).

Changes code Chromium uses, so this will require a temporary Skia
and then a change to Chromium to use the new Skia code.

TODO: Create a decoder that does nothing to be used by pinspect,
lua pictures, etc, and allow it to not assert in SkOrderedReadBuffer.

R=reed@google.com

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

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

16 files changed, 109 insertions, 120 deletions

diff --git a/debugger/QT/SkDebuggerGUI.cpp b/debugger/QT/SkDebuggerGUI.cpp
index f5f9bc6331..67277070e5 100644
--- a/debugger/QT/SkDebuggerGUI.cpp
+++ b/debugger/QT/SkDebuggerGUI.cpp
@@ -237,35 +237,24 @@ private:
 // Wrap SkPicture to allow installation of an SkTimedPicturePlayback object
 class SkTimedPicture : public SkPicture {
 public:
-    explicit SkTimedPicture(SkStream* stream, bool* success, SkPicture::InstallPixelRefProc proc,
-                            const SkTDArray<bool>& deletedCommands) {
-        if (success) {
-            *success = false;
-        }
-        fRecord = NULL;
-        fPlayback = NULL;
-        fWidth = fHeight = 0;
-
+    static SkTimedPicture* CreateTimedPicture(SkStream* stream,
+                                              SkPicture::InstallPixelRefProc proc,
+                                              const SkTDArray<bool>& deletedCommands) {
         SkPictInfo info;
-
-        if (!stream->read(&info, sizeof(info))) {
-            return;
-        }
-        if (SkPicture::PICTURE_VERSION != info.fVersion) {
-            return;
+        if (!StreamIsSKP(stream, &info)) {
+            return NULL;
         }
 
+        SkTimedPicturePlayback* playback;
+        // Check to see if there is a playback to recreate.
         if (stream->readBool()) {
-            fPlayback = SkNEW_ARGS(SkTimedPicturePlayback,
-                                   (stream, info, proc, deletedCommands));
+            playback = SkNEW_ARGS(SkTimedPicturePlayback,
+                                  (stream, info, proc, deletedCommands));
+        } else {
+            playback = NULL;
         }
 
-        // do this at the end, so that they will be zero if we hit an error.
-        fWidth = info.fWidth;
-        fHeight = info.fHeight;
-        if (success) {
-            *success = true;
-        }
+        return SkNEW_ARGS(SkTimedPicture, (playback, info.fWidth, info.fHeight));
     }
 
     void resetTimes() { ((SkTimedPicturePlayback*) fPlayback)->resetTimes(); }
@@ -282,6 +271,9 @@ public:
 private:
     // disallow default ctor b.c. we don't have a good way to setup the fPlayback ptr
     SkTimedPicture();
+    // Private ctor only used by CreateTimedPicture, which has created the playback.
+    SkTimedPicture(SkTimedPicturePlayback* playback, int width, int height)
+        : INHERITED(playback, width, height) {}
     // disallow the copy ctor - enabling would require copying code from SkPicture
     SkTimedPicture(const SkTimedPicture& src);
 
@@ -338,10 +330,9 @@ void SkDebuggerGUI::actionProfile() {
         return;
     }
 
-    bool success = false;
-    SkTimedPicture picture(&inputStream, &success, &SkImageDecoder::DecodeMemory,
-                           fSkipCommands);
-    if (!success) {
+    SkAutoTUnref<SkTimedPicture> picture(SkTimedPicture::CreateTimedPicture(&inputStream,
+                                         &SkImageDecoder::DecodeMemory, fSkipCommands));
+    if (NULL == picture.get()) {
         return;
     }
 
@@ -377,20 +368,20 @@ void SkDebuggerGUI::actionProfile() {
 
     static const int kNumRepeats = 10;
 
-    run(&picture, renderer, kNumRepeats);
+    run(picture.get(), renderer, kNumRepeats);
 
-    SkASSERT(picture.count() == fListWidget.count());
+    SkASSERT(picture->count() == fListWidget.count());
 
     // extract the individual command times from the SkTimedPlaybackPicture
-    for (int i = 0; i < picture.count(); ++i) {
-        double temp = picture.time(i);
+    for (int i = 0; i < picture->count(); ++i) {
+        double temp = picture->time(i);
 
         QListWidgetItem* item = fListWidget.item(i);
 
         item->setData(Qt::UserRole + 4, 100.0*temp);
     }
 
-    setupOverviewText(picture.typeTimes(), picture.totTime(), kNumRepeats);
+    setupOverviewText(picture->typeTimes(), picture->totTime(), kNumRepeats);
 }
 
 void SkDebuggerGUI::actionCancel() {
@@ -905,12 +896,9 @@ void SkDebuggerGUI::loadPicture(const SkString& fileName) {
     fLoading = true;
     SkStream* stream = SkNEW_ARGS(SkFILEStream, (fileName.c_str()));
 
-    bool success = false;
-
-    SkPicture* picture = SkNEW_ARGS(SkPicture,
-                                    (stream, &success, &SkImageDecoder::DecodeMemory));
+    SkPicture* picture = SkPicture::CreateFromStream(stream);
 
-    if (!success) {
+    if (NULL == picture) {
         QMessageBox::critical(this, "Error loading file", "Couldn't read file, sorry.");
         SkSafeUnref(stream);
         return;
diff --git a/gm/gmmain.cpp b/gm/gmmain.cpp
index ad7d55f088..8c015e7a15 100644
--- a/gm/gmmain.cpp
+++ b/gm/gmmain.cpp
@@ -1025,10 +1025,7 @@ public:
         //@todo thudson 22 April 2011 when can we safely delete [] dst?
         storage.copyTo(dst);
         SkMemoryStream pictReadback(dst, streamSize);
-        bool success;
-        // Pass a decoding bitmap function so that the factory GM (which has an SkBitmap with
-        // encoded data) does not fail.
-        SkPicture* retval = new SkPicture (&pictReadback, &success, &SkImageDecoder::DecodeMemory);
+        SkPicture* retval = SkPicture::CreateFromStream(&pictReadback);
         return retval;
     }
 
diff --git a/gyp/effects.gyp b/gyp/effects.gyp
index 91458eb96f..d69c82035f 100644
--- a/gyp/effects.gyp
+++ b/gyp/effects.gyp
@@ -1,3 +1,4 @@
+# Gyp file for effects
 {
   'targets': [
     {
@@ -12,6 +13,7 @@
         '../include/config',
         '../include/core',
         '../include/effects',
+        '../include/lazy',
         '../include/utils',
         '../src/core',
       ],
diff --git a/gyp/utils.gyp b/gyp/utils.gyp
index 4ac9284d53..1b51b18c10 100644
--- a/gyp/utils.gyp
+++ b/gyp/utils.gyp
@@ -1,3 +1,4 @@
+# Gyp for utils.
 {
   'targets': [
     {
@@ -10,6 +11,7 @@
         '../include/core',
         '../include/effects',
         '../include/images',
+        '../include/lazy',
         '../include/pipe',
         '../include/utils',
         '../include/utils/mac',
diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index d01cadce89..0bac3f7e9e 100644
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -11,6 +11,7 @@
 #define SkPicture_DEFINED
 
 #include "SkBitmap.h"
+#include "SkImageDecoder.h"
 #include "SkRefCnt.h"
 
 class SkBBoxHierarchy;
@@ -22,6 +23,8 @@ class SkPictureRecord;
 class SkStream;
 class SkWStream;
 
+struct SkPictInfo;
+
 /** \class SkPicture
 
     The SkPicture class records the drawing commands made to a canvas, to
@@ -42,13 +45,6 @@ public:
     SkPicture(const SkPicture& src);
 
     /**
-     *  Recreate a picture that was serialized into a stream.
-     *  On failure, silently creates an empty picture.
-     *  @param SkStream Serialized picture data.
-     */
-    explicit SkPicture(SkStream*);
-
-    /**
      *  Function signature defining a function that sets up an SkBitmap from encoded data. On
      *  success, the SkBitmap should have its Config, width, height, rowBytes and pixelref set.
      *  If the installed pixelref has decoded the data into pixels, then the src buffer need not be
@@ -64,12 +60,13 @@ public:
     /**
      *  Recreate a picture that was serialized into a stream.
      *  @param SkStream Serialized picture data.
-     *  @param success Output parameter. If non-NULL, will be set to true if the picture was
-     *                 deserialized successfully and false otherwise.
      *  @param proc Function pointer for installing pixelrefs on SkBitmaps representing the
      *              encoded bitmap data from the stream.
+     *  @return A new SkPicture representing the serialized data, or NULL if the stream is
+     *          invalid.
      */
-    SkPicture(SkStream*, bool* success, InstallPixelRefProc proc);
+    static SkPicture* CreateFromStream(SkStream*,
+                                       InstallPixelRefProc proc = &SkImageDecoder::DecodeMemory);
 
     virtual ~SkPicture();
 
@@ -220,13 +217,20 @@ protected:
     SkPictureRecord* fRecord;
     int fWidth, fHeight;
 
+    // Create a new SkPicture from an existing SkPicturePlayback. Ref count of
+    // playback is unchanged.
+    SkPicture(SkPicturePlayback*, int width, int height);
+
     // For testing. Derived classes may instantiate an alternate
     // SkBBoxHierarchy implementation
     virtual SkBBoxHierarchy* createBBoxHierarchy() const;
 
+    // Return true if the SkStream represents a serialized picture, and fills out
+    // SkPictInfo. After this function returns, the SkStream is not rewound; it
+    // will be ready to be parsed to create an SkPicturePlayback.
+    // If false is returned, SkPictInfo is unmodified.
+    static bool StreamIsSKP(SkStream*, SkPictInfo*);
 private:
-    void initFromStream(SkStream*, bool* success, InstallPixelRefProc);
-
     friend class SkFlatPicture;
     friend class SkPicturePlayback;
 
diff --git a/samplecode/SampleApp.cpp b/samplecode/SampleApp.cpp
index 380511f7ba..3825197690 100644
--- a/samplecode/SampleApp.cpp
+++ b/samplecode/SampleApp.cpp
@@ -1379,8 +1379,10 @@ void SampleWindow::afterChildren(SkCanvas* orig) {
 
             SkAutoDataUnref data(ostream.copyToData());
             SkMemoryStream istream(data->data(), data->size());
-            SkPicture pict(&istream);
-            orig->drawPicture(pict);
+            SkAutoTUnref<SkPicture> pict(SkPicture::CreateFromStream(&istream));
+            if (pict.get() != NULL) {
+                orig->drawPicture(*pict.get());
+            }
         } else {
             fPicture->draw(orig);
             fPicture->unref();
diff --git a/samplecode/SamplePictFile.cpp b/samplecode/SamplePictFile.cpp
index 847894ce1a..867118dee3 100644
--- a/samplecode/SamplePictFile.cpp
+++ b/samplecode/SamplePictFile.cpp
@@ -48,7 +48,7 @@ class PictFileView : public SampleView {
         } else {
             SkFILEStream stream(path);
             if (stream.isValid()) {
-                pic = SkNEW_ARGS(SkPicture, (&stream, NULL, &SkImageDecoder::DecodeMemory));
+                pic = SkPicture::CreateFromStream(&stream);
             } else {
                 SkDebugf("coun't load picture at \"path\"\n", path);
             }
diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index ab2faea6b6..2b488dec02 100644
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -264,41 +264,47 @@ void SkPicture::draw(SkCanvas* surface, SkDrawPictureCallback* callback) {
 
 #include "SkStream.h"
 
-SkPicture::SkPicture(SkStream* stream) {
-    this->initFromStream(stream, NULL, NULL);
-}
+bool SkPicture::StreamIsSKP(SkStream* stream, SkPictInfo* pInfo) {
+    if (NULL == stream) {
+        return false;
+    }
 
-SkPicture::SkPicture(SkStream* stream, bool* success, InstallPixelRefProc proc) {
-    this->initFromStream(stream, success, proc);
-}
+    SkPictInfo info;
+    if (!stream->read(&info, sizeof(SkPictInfo))) {
+        return false;
+    }
+    if (PICTURE_VERSION != info.fVersion) {
+        return false;
+    }
 
-void SkPicture::initFromStream(SkStream* stream, bool* success, InstallPixelRefProc proc) {
-    if (success) {
-        *success = false;
+    if (pInfo != NULL) {
+        *pInfo = info;
     }
-    fRecord = NULL;
-    fPlayback = NULL;
-    fWidth = fHeight = 0;
+    return true;
+}
+
+SkPicture::SkPicture(SkPicturePlayback* playback, int width, int height)
+    : fPlayback(playback)
+    , fRecord(NULL)
+    , fWidth(width)
+    , fHeight(height) {}
 
+SkPicture* SkPicture::CreateFromStream(SkStream* stream, InstallPixelRefProc proc) {
     SkPictInfo info;
 
-    if (!stream->read(&info, sizeof(info))) {
-        return;
-    }
-    if (PICTURE_VERSION != info.fVersion) {
-        return;
+    if (!StreamIsSKP(stream, &info)) {
+        return NULL;
     }
 
+    SkPicturePlayback* playback;
+    // Check to see if there is a playback to recreate.
     if (stream->readBool()) {
-        fPlayback = SkNEW_ARGS(SkPicturePlayback, (stream, info, proc));
+        playback = SkNEW_ARGS(SkPicturePlayback, (stream, info, proc));
+    } else {
+        playback = NULL;
     }
 
-    // do this at the end, so that they will be zero if we hit an error.
-    fWidth = info.fWidth;
-    fHeight = info.fHeight;
-    if (success) {
-        *success = true;
-    }
+    return SkNEW_ARGS(SkPicture, (playback, info.fWidth, info.fHeight));
 }
 
 void SkPicture::serialize(SkWStream* stream, EncodeBitmap encoder) const {
diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index 898d379578..fe1a037cfc 100644
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -520,15 +520,14 @@ void SkPicturePlayback::parseStreamTag(SkStream* stream, const SkPictInfo& info,
         case PICT_PICTURE_TAG: {
             fPictureCount = size;
             fPictureRefs = SkNEW_ARRAY(SkPicture*, fPictureCount);
-            bool success;
             for (int i = 0; i < fPictureCount; i++) {
-                fPictureRefs[i] = SkNEW_ARGS(SkPicture, (stream, &success, proc));
-                // Success can only be false if PICTURE_VERSION does not match
+                fPictureRefs[i] = SkPicture::CreateFromStream(stream, proc);
+                // CreateFromStream can only fail if PICTURE_VERSION does not match
                 // (which should never happen from here, since a sub picture will
                 // have the same PICTURE_VERSION as its parent) or if stream->read
                 // returns 0. In the latter case, we have a bug when writing the
                 // picture to begin with, which will be alerted to here.
-                SkASSERT(success);
+                SkASSERT(fPictureRefs[i] != NULL);
             }
         } break;
         case PICT_BUFFER_SIZE_TAG: {
diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index 23ff6893c0..49bc57b1af 100644
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -420,10 +420,9 @@ static void test_bitmap_with_encoded_data(skiatest::Reporter* reporter) {
     context.fReporter = reporter;
     SkSetErrorCallback(assert_one_parse_error_cb, &context);
     SkMemoryStream pictureStream(picture1);
-    bool success;
     SkClearLastError();
-    SkPicture pictureFromStream(&pictureStream, &success, NULL);
-    REPORTER_ASSERT(reporter, success);
+    SkAutoUnref pictureFromStream(SkPicture::CreateFromStream(&pictureStream, NULL));
+    REPORTER_ASSERT(reporter, pictureFromStream.get() != NULL);
     SkClearLastError();
     SkSetErrorCallback(NULL, NULL);
 }
diff --git a/tools/bench_pictures_main.cpp b/tools/bench_pictures_main.cpp
index 46d97de742..720621ec9a 100644
--- a/tools/bench_pictures_main.cpp
+++ b/tools/bench_pictures_main.cpp
@@ -173,16 +173,15 @@ static bool run_single_benchmark(const SkString& inputPath,
         gLruImageCache.setImageCacheLimit(0);
     }
 
-    bool success = false;
-    SkPicture* picture;
+    SkPicture::InstallPixelRefProc proc;
     if (FLAGS_deferImageDecoding) {
-        picture = SkNEW_ARGS(SkPicture, (&inputStream, &success, &lazy_decode_bitmap));
+        proc = &lazy_decode_bitmap;
     } else {
-        picture = SkNEW_ARGS(SkPicture, (&inputStream, &success, &SkImageDecoder::DecodeMemory));
+        proc = &SkImageDecoder::DecodeMemory;
     }
-    SkAutoTDelete<SkPicture> ad(picture);
+    SkAutoTUnref<SkPicture> picture(SkPicture::CreateFromStream(&inputStream, proc));
 
-    if (!success) {
+    if (NULL == picture.get()) {
         SkString err;
         err.printf("Could not read an SkPicture from %s\n", inputPath.c_str());
         gLogger.logError(err);
diff --git a/tools/filtermain.cpp b/tools/filtermain.cpp
index 4114a9db41..39c484d724 100644
--- a/tools/filtermain.cpp
+++ b/tools/filtermain.cpp
@@ -666,7 +666,7 @@ static int filter_picture(const SkString& inFile, const SkString& outFile) {
 
     SkFILEStream inStream(inFile.c_str());
     if (inStream.isValid()) {
-        inPicture.reset(SkNEW_ARGS(SkPicture, (&inStream, NULL, &SkImageDecoder::DecodeMemory)));
+        inPicture.reset(SkPicture::CreateFromStream(&inStream));
     }
 
     if (NULL == inPicture.get()) {
diff --git a/tools/lua/lua_pictures.cpp b/tools/lua/lua_pictures.cpp
index 02b9a57fb4..f1bca287ad 100644
--- a/tools/lua/lua_pictures.cpp
+++ b/tools/lua/lua_pictures.cpp
@@ -46,13 +46,7 @@ static SkPicture* load_picture(const char path[]) {
     SkAutoTUnref<SkStream> stream(SkStream::NewFromFile(path));
     SkPicture* pic = NULL;
     if (stream.get()) {
-        bool success;
-        pic = SkNEW_ARGS(SkPicture, (stream.get(), &success,
-                                     &lazy_decode_bitmap));
-        if (!success) {
-            SkDELETE(pic);
-            pic = NULL;
-        }
+        pic = SkPicture::CreateFromStream(stream.get(), &lazy_decode_bitmap);
     }
     return pic;
 }
diff --git a/tools/pinspect.cpp b/tools/pinspect.cpp
index f6304e6b40..969d87c4fe 100644
--- a/tools/pinspect.cpp
+++ b/tools/pinspect.cpp
@@ -40,11 +40,10 @@ static SkPicture* inspect(const char path[]) {
     }
 
     stream.rewind();
-    bool success = false;
-    SkPicture* pic = SkNEW_ARGS(SkPicture, (&stream, &success, &lazy_decode_bitmap));
-    if (!success) {
+    SkPicture* pic = SkPicture::CreateFromStream(&stream, &lazy_decode_bitmap);
+    if (NULL == pic) {
         SkDebugf("Could not create SkPicture: %s\n", path);
-        return pic;
+        return NULL;
     }
     printf("picture size:[%d %d]\n", pic->width(), pic->height());
     return pic;
diff --git a/tools/render_pdfs_main.cpp b/tools/render_pdfs_main.cpp
index 1821548aa3..f4435021c3 100644
--- a/tools/render_pdfs_main.cpp
+++ b/tools/render_pdfs_main.cpp
@@ -9,7 +9,6 @@
 #include "SkDevice.h"
 #include "SkForceLinking.h"
 #include "SkGraphics.h"
-#include "SkImageDecoder.h"
 #include "SkImageEncoder.h"
 #include "SkOSFile.h"
 #include "SkPicture.h"
@@ -169,11 +168,9 @@ static bool render_pdf(const SkString& inputPath, const SkString& outputDir,
         return false;
     }
 
-    bool success = false;
-    SkAutoTUnref<SkPicture>
-        picture(SkNEW_ARGS(SkPicture, (&inputStream, &success, &SkImageDecoder::DecodeMemory)));
+    SkAutoTUnref<SkPicture> picture(SkPicture::CreateFromStream(&inputStream));
 
-    if (!success) {
+    if (NULL == picture.get()) {
         SkDebugf("Could not read an SkPicture from %s\n", inputPath.c_str());
         return false;
     }
@@ -185,7 +182,7 @@ static bool render_pdf(const SkString& inputPath, const SkString& outputDir,
 
     renderer.render();
 
-    success = write_output(outputDir, inputFilename, renderer);
+    bool success = write_output(outputDir, inputFilename, renderer);
 
     renderer.end();
     return success;
diff --git a/tools/render_pictures_main.cpp b/tools/render_pictures_main.cpp
index 4a7e708c13..de477d3336 100644
--- a/tools/render_pictures_main.cpp
+++ b/tools/render_pictures_main.cpp
@@ -149,21 +149,22 @@ static bool render_picture(const SkString& inputPath, const SkString* outputDir,
         return false;
     }
 
-    SkDebugf("deserializing... %s\n", inputPath.c_str());
-
-    bool success = false;
-    SkPicture* picture;
+    SkPicture::InstallPixelRefProc proc;
     if (FLAGS_deferImageDecoding) {
-        picture = SkNEW_ARGS(SkPicture, (&inputStream, &success, &lazy_decode_bitmap));
+        proc = &lazy_decode_bitmap;
     } else if (FLAGS_writeEncodedImages) {
         SkASSERT(!FLAGS_writePath.isEmpty());
         reset_image_file_base_name(inputFilename);
-        picture = SkNEW_ARGS(SkPicture, (&inputStream, &success, &write_image_to_file));
+        proc = &write_image_to_file;
     } else {
-        picture = SkNEW_ARGS(SkPicture, (&inputStream, &success, &SkImageDecoder::DecodeMemory));
+        proc = &SkImageDecoder::DecodeMemory;
     }
 
-    if (!success) {
+    SkDebugf("deserializing... %s\n", inputPath.c_str());
+
+    SkPicture* picture = SkPicture::CreateFromStream(&inputStream, proc);
+
+    if (NULL == picture) {
         SkDebugf("Could not read an SkPicture from %s\n", inputPath.c_str());
         return false;
     }
@@ -186,7 +187,7 @@ static bool render_picture(const SkString& inputPath, const SkString* outputDir,
         make_output_filepath(outputPath, *outputDir, inputFilename);
     }
 
-    success = renderer.render(outputPath, out);
+    bool success = renderer.render(outputPath, out);
     if (outputPath) {
         if (!success) {
             SkDebugf("Could not write to file %s\n", outputPath->c_str());