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

include/core/SkPicture.h

@@ -274,7 +274,8 @@ protected:
     // 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;

include/core/SkReadBuffer.h

@@ -177,12 +177,14 @@ public:
     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;

src/core/SkPicture.cpp

@@ -260,7 +260,19 @@ void SkPicture::draw(SkCanvas* surface, SkDrawPictureCallback* callback) {
 #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 @@ bool SkPicture::InternalOnly_StreamIsSKP(SkStream* stream, SkPictInfo* pInfo) {
     }
 
     // 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))) {
+    SkASSERT(sizeof(kMagic) == sizeof(info.fMagic));
-        return false;
+    if (!stream->read(&info, sizeof(info)) || !IsValidPictInfo(info)) {
-    }
-
-    if (info.fVersion < MIN_PICTURE_VERSION || info.fVersion > CURRENT_PICTURE_VERSION) {
         return false;
     }
 
@@ -291,19 +294,9 @@ bool SkPicture::InternalOnly_StreamIsSKP(SkStream* stream, SkPictInfo* pInfo) {
 
 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))) {
+    SkASSERT(sizeof(kMagic) == sizeof(info.fMagic));
-        return false;
+    if (!buffer.readByteArray(&info, sizeof(info)) || !IsValidPictInfo(info)) {
-    }
-
-    if (info.fVersion < MIN_PICTURE_VERSION || info.fVersion > CURRENT_PICTURE_VERSION) {
         return false;
     }
 
@@ -361,13 +354,13 @@ SkPicture* SkPicture::CreateFromBuffer(SkReadBuffer& buffer) {
     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 @@ void SkPicture::serialize(SkWStream* stream, EncodeBitmap encoder) const {
         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 @@ void SkPicture::flatten(SkWriteBuffer& buffer) const {
         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);

src/core/SkPicturePlayback.cpp

@@ -270,7 +270,7 @@ void SkPicturePlayback::init() {
 }
 
 SkPicturePlayback::~SkPicturePlayback() {
-    fOpData->unref();
+    SkSafeUnref(fOpData);
 
     SkSafeUnref(fBitmaps);
     SkSafeUnref(fPaints);
@@ -612,6 +612,41 @@ bool SkPicturePlayback::parseBufferTag(SkReadBuffer& buffer,
                 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;

src/core/SkPicturePlayback.h

@@ -40,6 +40,7 @@ struct SkPictInfo {
         kPtrIs64Bit_Flag        = 1 << 2,
     };
 
+    char        fMagic[8];
     uint32_t    fVersion;
     uint32_t    fWidth;
     uint32_t    fHeight;

src/core/SkValidatingReadBuffer.cpp

@@ -213,10 +213,10 @@ uint32_t SkValidatingReadBuffer::getArrayCount() {
 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 (!this->validate(!useBitmapHeap && (0 == length))) {
-    if (fError) {
         return;
     }
     bitmap->unflatten(*this);

src/core/SkValidatingReadBuffer.h

@@ -75,7 +75,6 @@ private:
         return SkIsAlign4((uintptr_t)ptr);
     }
 
-    SkReader32 fReader;
     bool fError;
 
     typedef SkReadBuffer INHERITED;

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 @@ static void InitializeFlattenables(int*) {
     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)

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 @@ static void InitializeFlattenables(int*) {
     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)

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 void TestBitmapSerialization(const SkBitmap& validBitmap,
     }
 }
 
+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 @@ DEF_TEST(Serialization, reporter) {
 
     // Test invalid deserializations
     {
-        SkImageInfo info = SkImageInfo::MakeN32Premul(256, 256);
+        SkImageInfo info = SkImageInfo::MakeN32Premul(kBitmapSize, kBitmapSize);
 
         SkBitmap validBitmap;
         validBitmap.setConfig(info);
@@ -306,4 +365,25 @@ DEF_TEST(Serialization, reporter) {
         // 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);
+    }
 }