From f1754ec69131801c1a6ed3c704501a9400bbf324 Mon Sep 17 00:00:00 2001 From: "scroggo@google.com" Date: Fri, 28 Jun 2013 21:32:00 +0000 Subject: [PATCH] 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 --- debugger/QT/SkDebuggerGUI.cpp | 62 +++++++++++++++++------------------------- gm/gmmain.cpp | 5 +--- gyp/effects.gyp | 2 ++ gyp/utils.gyp | 2 ++ include/core/SkPicture.h | 28 +++++++++++-------- samplecode/SampleApp.cpp | 6 ++-- samplecode/SamplePictFile.cpp | 2 +- src/core/SkPicture.cpp | 54 ++++++++++++++++++++---------------- src/core/SkPicturePlayback.cpp | 7 ++--- tests/PictureTest.cpp | 5 ++-- tools/bench_pictures_main.cpp | 11 ++++---- tools/filtermain.cpp | 2 +- tools/lua/lua_pictures.cpp | 8 +----- tools/pinspect.cpp | 7 ++--- tools/render_pdfs_main.cpp | 9 ++---- tools/render_pictures_main.cpp | 19 +++++++------ 16 files changed, 109 insertions(+), 120 deletions(-) diff --git a/debugger/QT/SkDebuggerGUI.cpp b/debugger/QT/SkDebuggerGUI.cpp index f5f9bc6..6727707 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& deletedCommands) { - if (success) { - *success = false; - } - fRecord = NULL; - fPlayback = NULL; - fWidth = fHeight = 0; - + static SkTimedPicture* CreateTimedPicture(SkStream* stream, + SkPicture::InstallPixelRefProc proc, + const SkTDArray& 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 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 ad7d55f..8c015e7 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 91458eb..d69c820 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 4ac9284..1b51b18 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 d01cadc..0bac3f7 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 380511f..3825197 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 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 847894c..867118d 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 ab2faea..2b488de 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 898d379..fe1a037 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 23ff689..49bc57b 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 46d97de..720621e 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 ad(picture); + SkAutoTUnref 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 4114a9d..39c484d 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 02b9a57..f1bca28 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 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 f6304e6..969d87c 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 1821548..f443502 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 - picture(SkNEW_ARGS(SkPicture, (&inputStream, &success, &SkImageDecoder::DecodeMemory))); + SkAutoTUnref 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 4a7e708..de477d3 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()); -- 2.7.4