Fixing SkPicture serialization
Fixed a few issues while attempting to use the new
serialization path for SkPicture inside a fuzzer:
- SkReadBuffer and SkValidatingReadBuffer both had a fReader
member instead of sharing the same member, which leads to
problems if a base class function is used
- In SkPicture, a header is now written as a single chunk of
data, so it also has to be read as a single chunk of data
- In the SkPicturePlayback destructor, a bad deserialization
would lead to a crash if we don't safely unref fOpData
- Also in SkPicturePlayback, if we only use a ReadBuffer for
the whole deserialization, additional tags must be added to
parseBufferTag()
- SkValidatingReadBuffer::readBitmap() was broken, but this
path wasn't usen't since the only use case for
SkValidatingReadBuffer is currently image filters and
bitmaps are unflattened as part of the deserialization of
SkBitmapSource
- SkPictureImageFilter was not deserializable. Added it to
SkGlobalInitialization*
- Added a test that exercises the SkPicture serialization /
deserialization code
BUG=skia:
R=senorblanco@google.com, senorblanco@chromium.org, reed@google.com, robertphillips@google.com
Author: sugoi@chromium.org
Review URL: https://codereview.chromium.org/195223003
git-svn-id: http://skia.googlecode.com/svn/trunk@13764 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index d36d162..ef6f4a2 100644
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -274,7 +274,8 @@
// SkBBoxHierarchy implementation
virtual SkBBoxHierarchy* createBBoxHierarchy() const;
private:
- void createHeader(void* header) const;
+ void createHeader(SkPictInfo* info) const;
+ static bool IsValidPictInfo(const SkPictInfo& info);
friend class SkFlatPicture;
friend class SkPicturePlayback;
diff --git a/include/core/SkReadBuffer.h b/include/core/SkReadBuffer.h
index 75cc64b..dd3301e 100644
--- a/include/core/SkReadBuffer.h
+++ b/include/core/SkReadBuffer.h
@@ -177,12 +177,14 @@
virtual bool isValid() const { return true; }
virtual bool validateAvailable(size_t size) { return true; }
+protected:
+ SkReader32 fReader;
+
private:
bool readArray(void* value, size_t size, size_t elementSize);
uint32_t fFlags;
- SkReader32 fReader;
void* fMemoryPtr;
SkBitmapHeapReader* fBitmapStorage;
diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index f83a5fb..9270236 100644
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -260,7 +260,19 @@
#include "SkStream.h"
static const char kMagic[] = { 's', 'k', 'i', 'a', 'p', 'i', 'c', 't' };
-static const size_t kHeaderSize = sizeof(kMagic) + sizeof(SkPictInfo);
+
+bool SkPicture::IsValidPictInfo(const SkPictInfo& info) {
+ if (0 != memcmp(info.fMagic, kMagic, sizeof(kMagic))) {
+ return false;
+ }
+
+ if (info.fVersion < MIN_PICTURE_VERSION ||
+ info.fVersion > CURRENT_PICTURE_VERSION) {
+ return false;
+ }
+
+ return true;
+}
bool SkPicture::InternalOnly_StreamIsSKP(SkStream* stream, SkPictInfo* pInfo) {
if (NULL == stream) {
@@ -268,18 +280,9 @@
}
// Check magic bytes.
- char magic[sizeof(kMagic)];
- if (!stream->read(magic, sizeof(kMagic)) ||
- (0 != memcmp(magic, kMagic, sizeof(kMagic)))) {
- return false;
- }
-
SkPictInfo info;
- if (!stream->read(&info, sizeof(SkPictInfo))) {
- return false;
- }
-
- if (info.fVersion < MIN_PICTURE_VERSION || info.fVersion > CURRENT_PICTURE_VERSION) {
+ SkASSERT(sizeof(kMagic) == sizeof(info.fMagic));
+ if (!stream->read(&info, sizeof(info)) || !IsValidPictInfo(info)) {
return false;
}
@@ -291,19 +294,9 @@
bool SkPicture::InternalOnly_BufferIsSKP(SkReadBuffer& buffer, SkPictInfo* pInfo) {
// Check magic bytes.
- char magic[sizeof(kMagic)];
-
- if (!buffer.readByteArray(magic, sizeof(kMagic)) ||
- (0 != memcmp(magic, kMagic, sizeof(kMagic)))) {
- return false;
- }
-
SkPictInfo info;
- if (!buffer.readByteArray(&info, sizeof(SkPictInfo))) {
- return false;
- }
-
- if (info.fVersion < MIN_PICTURE_VERSION || info.fVersion > CURRENT_PICTURE_VERSION) {
+ SkASSERT(sizeof(kMagic) == sizeof(info.fMagic));
+ if (!buffer.readByteArray(&info, sizeof(info)) || !IsValidPictInfo(info)) {
return false;
}
@@ -361,13 +354,13 @@
return SkNEW_ARGS(SkPicture, (playback, info.fWidth, info.fHeight));
}
-void SkPicture::createHeader(void* header) const {
+void SkPicture::createHeader(SkPictInfo* info) const {
// Copy magic bytes at the beginning of the header
SkASSERT(sizeof(kMagic) == 8);
- memcpy(header, kMagic, sizeof(kMagic));
+ SkASSERT(sizeof(kMagic) == sizeof(info->fMagic));
+ memcpy(info->fMagic, kMagic, sizeof(kMagic));
// Set picture info after magic bytes in the header
- SkPictInfo* info = (SkPictInfo*)(((char*)header) + sizeof(kMagic));
info->fVersion = CURRENT_PICTURE_VERSION;
info->fWidth = fWidth;
info->fHeight = fHeight;
@@ -387,9 +380,9 @@
playback = SkNEW_ARGS(SkPicturePlayback, (*fRecord));
}
- char header[kHeaderSize];
+ SkPictInfo header;
this->createHeader(&header);
- stream->write(header, kHeaderSize);
+ stream->write(&header, sizeof(header));
if (playback) {
stream->writeBool(true);
playback->serialize(stream, encoder);
@@ -409,9 +402,9 @@
playback = SkNEW_ARGS(SkPicturePlayback, (*fRecord));
}
- char header[kHeaderSize];
+ SkPictInfo header;
this->createHeader(&header);
- buffer.writeByteArray(header, kHeaderSize);
+ buffer.writeByteArray(&header, sizeof(header));
if (playback) {
buffer.writeBool(true);
playback->flatten(buffer);
diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index 08e53fa..05c2c4e 100644
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -270,7 +270,7 @@
}
SkPicturePlayback::~SkPicturePlayback() {
- fOpData->unref();
+ SkSafeUnref(fOpData);
SkSafeUnref(fBitmaps);
SkSafeUnref(fPaints);
@@ -612,6 +612,41 @@
fPathHeap.reset(SkNEW_ARGS(SkPathHeap, (buffer)));
}
break;
+ case SK_PICT_READER_TAG: {
+ SkAutoMalloc storage(size);
+ if (!buffer.readByteArray(storage.get(), size) ||
+ !buffer.validate(NULL == fOpData)) {
+ return false;
+ }
+ SkASSERT(NULL == fOpData);
+ fOpData = SkData::NewFromMalloc(storage.detach(), size);
+ } break;
+ case SK_PICT_PICTURE_TAG: {
+ if (!buffer.validate((0 == fPictureCount) && (NULL == fPictureRefs))) {
+ return false;
+ }
+ fPictureCount = size;
+ fPictureRefs = SkNEW_ARRAY(SkPicture*, fPictureCount);
+ bool success = true;
+ int i = 0;
+ for ( ; i < fPictureCount; i++) {
+ fPictureRefs[i] = SkPicture::CreateFromBuffer(buffer);
+ if (NULL == fPictureRefs[i]) {
+ success = false;
+ break;
+ }
+ }
+ if (!success) {
+ // Delete all of the pictures that were already created (up to but excluding i):
+ for (int j = 0; j < i; j++) {
+ fPictureRefs[j]->unref();
+ }
+ // Delete the array
+ SkDELETE_ARRAY(fPictureRefs);
+ fPictureCount = 0;
+ return false;
+ }
+ } break;
default:
// The tag was invalid.
return false;
diff --git a/src/core/SkPicturePlayback.h b/src/core/SkPicturePlayback.h
index 178a408..512f24a 100644
--- a/src/core/SkPicturePlayback.h
+++ b/src/core/SkPicturePlayback.h
@@ -40,6 +40,7 @@
kPtrIs64Bit_Flag = 1 << 2,
};
+ char fMagic[8];
uint32_t fVersion;
uint32_t fWidth;
uint32_t fHeight;
diff --git a/src/core/SkValidatingReadBuffer.cpp b/src/core/SkValidatingReadBuffer.cpp
index ae7a836..8db2c68 100644
--- a/src/core/SkValidatingReadBuffer.cpp
+++ b/src/core/SkValidatingReadBuffer.cpp
@@ -213,10 +213,10 @@
void SkValidatingReadBuffer::readBitmap(SkBitmap* bitmap) {
const int width = this->readInt();
const int height = this->readInt();
+ const bool useBitmapHeap = this->readBool();
const size_t length = this->readUInt();
// A size of zero means the SkBitmap was simply flattened.
- this->validate(length == 0);
- if (fError) {
+ if (!this->validate(!useBitmapHeap && (0 == length))) {
return;
}
bitmap->unflatten(*this);
diff --git a/src/core/SkValidatingReadBuffer.h b/src/core/SkValidatingReadBuffer.h
index 59b5c1a..d5e192e 100644
--- a/src/core/SkValidatingReadBuffer.h
+++ b/src/core/SkValidatingReadBuffer.h
@@ -75,7 +75,6 @@
return SkIsAlign4((uintptr_t)ptr);
}
- SkReader32 fReader;
bool fError;
typedef SkReadBuffer INHERITED;
diff --git a/src/ports/SkGlobalInitialization_chromium.cpp b/src/ports/SkGlobalInitialization_chromium.cpp
index 79ed46c..dd24eaf 100644
--- a/src/ports/SkGlobalInitialization_chromium.cpp
+++ b/src/ports/SkGlobalInitialization_chromium.cpp
@@ -50,6 +50,7 @@
#include "SkOffsetImageFilter.h"
#include "SkOnce.h"
#include "SkPerlinNoiseShader.h"
+#include "SkPictureImageFilter.h"
#include "SkPixelXorXfermode.h"
#include "SkRectShaderImageFilter.h"
#include "SkResizeImageFilter.h"
@@ -88,6 +89,7 @@
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkLine2DPathEffect)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPath2DPathEffect)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPerlinNoiseShader)
+ SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPictureImageFilter)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPixelXorXfermode)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkRectShaderImageFilter)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkResizeImageFilter)
diff --git a/src/ports/SkGlobalInitialization_default.cpp b/src/ports/SkGlobalInitialization_default.cpp
index 79ed46c..dd24eaf 100644
--- a/src/ports/SkGlobalInitialization_default.cpp
+++ b/src/ports/SkGlobalInitialization_default.cpp
@@ -50,6 +50,7 @@
#include "SkOffsetImageFilter.h"
#include "SkOnce.h"
#include "SkPerlinNoiseShader.h"
+#include "SkPictureImageFilter.h"
#include "SkPixelXorXfermode.h"
#include "SkRectShaderImageFilter.h"
#include "SkResizeImageFilter.h"
@@ -88,6 +89,7 @@
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkLine2DPathEffect)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPath2DPathEffect)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPerlinNoiseShader)
+ SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPictureImageFilter)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPixelXorXfermode)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkRectShaderImageFilter)
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkResizeImageFilter)
diff --git a/tests/SerializationTest.cpp b/tests/SerializationTest.cpp
index 7b2914a..f3dafc8 100644
--- a/tests/SerializationTest.cpp
+++ b/tests/SerializationTest.cpp
@@ -15,6 +15,7 @@
#include "Test.h"
static const uint32_t kArraySize = 64;
+static const int kBitmapSize = 256;
template<typename T>
static void TestAlignment(T* testObj, skiatest::Reporter* reporter) {
@@ -229,6 +230,64 @@
}
}
+static bool setup_bitmap_for_canvas(SkBitmap* bitmap) {
+ SkImageInfo info = SkImageInfo::Make(
+ kBitmapSize, kBitmapSize, kPMColor_SkColorType, kPremul_SkAlphaType);
+ return bitmap->allocPixels(info);
+}
+
+static bool make_checkerboard_bitmap(SkBitmap& bitmap) {
+ bool success = setup_bitmap_for_canvas(&bitmap);
+
+ SkCanvas canvas(bitmap);
+ canvas.clear(0x00000000);
+ SkPaint darkPaint;
+ darkPaint.setColor(0xFF804020);
+ SkPaint lightPaint;
+ lightPaint.setColor(0xFF244484);
+ const int i = kBitmapSize / 8;
+ const SkScalar f = SkIntToScalar(i);
+ for (int y = 0; y < kBitmapSize; y += i) {
+ for (int x = 0; x < kBitmapSize; x += i) {
+ canvas.save();
+ canvas.translate(SkIntToScalar(x), SkIntToScalar(y));
+ canvas.drawRect(SkRect::MakeXYWH(0, 0, f, f), darkPaint);
+ canvas.drawRect(SkRect::MakeXYWH(f, 0, f, f), lightPaint);
+ canvas.drawRect(SkRect::MakeXYWH(0, f, f, f), lightPaint);
+ canvas.drawRect(SkRect::MakeXYWH(f, f, f, f), darkPaint);
+ canvas.restore();
+ }
+ }
+
+ return success;
+}
+
+static bool drawSomething(SkCanvas* canvas) {
+ SkPaint paint;
+ SkBitmap bitmap;
+ bool success = make_checkerboard_bitmap(bitmap);
+
+ canvas->save();
+ canvas->scale(0.5f, 0.5f);
+ canvas->drawBitmap(bitmap, 0, 0, NULL);
+ canvas->restore();
+
+ const char beforeStr[] = "before circle";
+ const char afterStr[] = "after circle";
+
+ paint.setAntiAlias(true);
+
+ paint.setColor(SK_ColorRED);
+ canvas->drawData(beforeStr, sizeof(beforeStr));
+ canvas->drawCircle(SkIntToScalar(kBitmapSize/2), SkIntToScalar(kBitmapSize/2), SkIntToScalar(kBitmapSize/3), paint);
+ canvas->drawData(afterStr, sizeof(afterStr));
+ paint.setColor(SK_ColorBLACK);
+ paint.setTextSize(SkIntToScalar(kBitmapSize/3));
+ canvas->drawText("Picture", 7, SkIntToScalar(kBitmapSize/2), SkIntToScalar(kBitmapSize/4), paint);
+
+ return success;
+}
+
DEF_TEST(Serialization, reporter) {
// Test matrix serialization
{
@@ -292,7 +351,7 @@
// Test invalid deserializations
{
- SkImageInfo info = SkImageInfo::MakeN32Premul(256, 256);
+ SkImageInfo info = SkImageInfo::MakeN32Premul(kBitmapSize, kBitmapSize);
SkBitmap validBitmap;
validBitmap.setConfig(info);
@@ -306,4 +365,25 @@
// even when the device fails to initialize, due to its size
TestBitmapSerialization(validBitmap, invalidBitmap, true, reporter);
}
+
+ // Test simple SkPicture serialization
+ {
+ SkPicture* pict = new SkPicture;
+ SkAutoUnref aur(pict);
+ bool didDraw = drawSomething(pict->beginRecording(kBitmapSize, kBitmapSize));
+ REPORTER_ASSERT(reporter, didDraw);
+ pict->endRecording();
+
+ // Serialize picture
+ SkWriteBuffer writer(SkWriteBuffer::kValidation_Flag);
+ pict->flatten(writer);
+ size_t size = writer.bytesWritten();
+ void* data = sk_malloc_throw(size);
+ writer.writeToMemory(data);
+
+ // Deserialize picture
+ SkValidatingReadBuffer reader(data, size);
+ SkPicture* readPict(SkPicture::CreateFromBuffer(reader));
+ REPORTER_ASSERT(reporter, NULL != readPict);
+ }
}