impl SkSerial picture procs

The picture serialization code is a bit of a mess, with duplicated functions for streams and buffers.
Could not see how to fix that and land this at the same time, but I will try to circle back and
simplify if possible afterwards.

Bug: skia:
Change-Id: I9053fdc476c60f483df013d021e248258181c199
Reviewed-on: https://skia-review.googlesource.com/83943
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Mike Reed <reed@google.com>

9 files changed, 218 insertions, 31 deletions

diff --git a/include/core/SkWriteBuffer.h b/include/core/SkWriteBuffer.h
index 42d8f96eb9..b0b6fd993a 100644
--- a/include/core/SkWriteBuffer.h
+++ b/include/core/SkWriteBuffer.h
@@ -37,6 +37,8 @@ public:
 
     virtual bool isCrossProcess() const = 0;
 
+    virtual void writePad32(const void* buffer, size_t bytes) = 0;
+
     virtual void writeByteArray(const void* data, size_t size) = 0;
     void writeDataAsByteArray(SkData* data) {
         this->writeByteArray(data->data(), data->size());
@@ -83,9 +85,14 @@ public:
      */
     void setClientContext(void* ctx) { fClientCtx = ctx; }
 
+    void setSerialProcs(const SkSerialProcs& procs) { fProcs = procs; }
+
 protected:
-    SkDeduper* fDeduper = nullptr;
-    void*      fClientCtx = nullptr;
+    SkDeduper*      fDeduper = nullptr;
+    void*           fClientCtx = nullptr;
+    SkSerialProcs   fProcs;
+
+    friend class SkPicture; // fProcs
 };
 
 /**
@@ -108,7 +115,7 @@ public:
     void write(const void* buffer, size_t bytes) {
         fWriter.write(buffer, bytes);
     }
-    void writePad32(const void* buffer, size_t bytes) {
+    void writePad32(const void* buffer, size_t bytes) override {
         fWriter.writePad(buffer, bytes);
     }
 
@@ -150,8 +157,6 @@ public:
     SkFactorySet* setFactoryRecorder(SkFactorySet*);
     SkRefCntSet* setTypefaceRecorder(SkRefCntSet*);
 
-    void setSerialProcs(const SkSerialProcs& procs) { fProcs = procs; }
-
 #ifdef SK_SUPPORT_LEGACY_SERIAL_BUFFER_OBJECTS
     void setPixelSerializer(sk_sp<SkPixelSerializer>);
 #endif
@@ -162,7 +167,6 @@ private:
     SkWriter32 fWriter;
 
     SkRefCntSet*    fTFSet;
-    SkSerialProcs   fProcs;
 
     // Only used if we do not have an fFactorySet
     SkTHashMap<SkString, uint32_t> fFlattenableDict;
diff --git a/src/core/SkMathPriv.h b/src/core/SkMathPriv.h
index ddb231507f..6c33e36747 100644
--- a/src/core/SkMathPriv.h
+++ b/src/core/SkMathPriv.h
@@ -49,6 +49,21 @@ static inline unsigned SkClampUMax(unsigned value, unsigned max) {
     return value;
 }
 
+// If a signed int holds min_int (e.g. 0x80000000) it is undefined what happens when
+// we negate it (even though we *know* we're 2's complement and we'll get the same
+// value back). So we create this helper function that casts to size_t (unsigned) first,
+// to avoid the complaint.
+static inline size_t sk_negate_to_size_t(int32_t value) {
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 4146)  // Thanks MSVC, we know what we're negating an unsigned
+#endif
+    return -static_cast<size_t>(value);
+#if defined(_MSC_VER)
+#pragma warning(pop)
+#endif
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 /** Return a*b/255, truncating away any fractional bits. Only valid if both
diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index 5442738368..edac1349ed 100644
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -8,6 +8,7 @@
 #include "SkAtomics.h"
 #include "SkImageDeserializer.h"
 #include "SkImageGenerator.h"
+#include "SkMathPriv.h"
 #include "SkPicture.h"
 #include "SkPictureCommon.h"
 #include "SkPictureData.h"
@@ -175,7 +176,25 @@ sk_sp<SkPicture> SkPicture::MakeFromStream(SkStream* stream, const SkDeserialPro
 sk_sp<SkPicture> SkPicture::MakeFromStream(SkStream* stream, const SkDeserialProcs& procs,
                                            SkTypefacePlayback* typefaces) {
     SkPictInfo info;
-    if (!InternalOnly_StreamIsSKP(stream, &info) || !stream->readBool()) {
+    if (!InternalOnly_StreamIsSKP(stream, &info)) {
+        return nullptr;
+    }
+
+    // size should be 0, 1, or negative
+    int32_t ssize = stream->readS32();
+    if (ssize < 0) {
+        if (!procs.fPictureProc) {
+            return nullptr;
+        }
+        size_t size = sk_negate_to_size_t(ssize);
+        auto data = SkData::MakeUninitialized(size);
+        if (stream->read(data->writable_data(), size) != size) {
+            return nullptr;
+        }
+        return procs.fPictureProc(data->data(), size, procs.fPictureCtx);
+    }
+    if (ssize != 1) {
+        // 1 is the magic 'size' that means SkPictureData follows
         return nullptr;
     }
     std::unique_ptr<SkPictureData> data(
@@ -185,10 +204,24 @@ sk_sp<SkPicture> SkPicture::MakeFromStream(SkStream* stream, const SkDeserialPro
 
 sk_sp<SkPicture> SkPicture::MakeFromBuffer(SkReadBuffer& buffer) {
     SkPictInfo info;
-    if (!InternalOnly_BufferIsSKP(&buffer, &info) || !buffer.readBool()) {
+    if (!InternalOnly_BufferIsSKP(&buffer, &info)) {
         return nullptr;
     }
-    std::unique_ptr<SkPictureData> data(SkPictureData::CreateFromBuffer(buffer, info));
+    // size should be 0, 1, or negative
+    int32_t ssize = buffer.read32();
+    if (ssize < 0) {
+        const SkDeserialProcs& procs = buffer.fProcs;
+        if (!procs.fPictureProc) {
+            return nullptr;
+        }
+        size_t size = sk_negate_to_size_t(ssize);
+        return procs.fPictureProc(buffer.skip(size), size, procs.fPictureCtx);
+    }
+    if (ssize != 1) {
+        // 1 is the magic 'size' that means SkPictureData follows
+        return nullptr;
+    }
+   std::unique_ptr<SkPictureData> data(SkPictureData::CreateFromBuffer(buffer, info));
     return Forwardport(info, data.get(), &buffer);
 }
 
@@ -222,17 +255,49 @@ sk_sp<SkData> SkPicture::serialize(const SkSerialProcs& procs) const {
     return stream.detachAsData();
 }
 
+static sk_sp<SkData> custom_serialize(const SkPicture* picture, const SkSerialProcs& procs) {
+    if (procs.fPictureProc) {
+        auto data = procs.fPictureProc(const_cast<SkPicture*>(picture), procs.fPictureCtx);
+        if (data) {
+            size_t size = data->size();
+            if (!sk_64_isS32(size) || size <= 1) {
+                return SkData::MakeEmpty();
+            }
+            return data;
+        }
+    }
+    return nullptr;
+}
+
+static bool write_pad32(SkWStream* stream, const void* data, size_t size) {
+    if (!stream->write(data, size)) {
+        return false;
+    }
+    if (size & 3) {
+        uint32_t zero = 0;
+        return stream->write(&zero, 4 - (size & 3));
+    }
+    return true;
+}
+
 void SkPicture::serialize(SkWStream* stream, const SkSerialProcs& procs,
                           SkRefCntSet* typefaceSet) const {
     SkPictInfo info = this->createHeader();
-    std::unique_ptr<SkPictureData> data(this->backport());
-
     stream->write(&info, sizeof(info));
+
+    if (auto custom = custom_serialize(this, procs)) {
+        int32_t size = SkToS32(custom->size());
+        stream->write32(-size);    // negative for custom format
+        write_pad32(stream, custom->data(), size);
+        return;
+    }
+
+    std::unique_ptr<SkPictureData> data(this->backport());
     if (data) {
-        stream->writeBool(true);
+        stream->write32(1); // special size meaning SkPictureData
         data->serialize(stream, procs, typefaceSet);
     } else {
-        stream->writeBool(false);
+        stream->write32(0); // signal no content
     }
 }
 
@@ -244,11 +309,19 @@ void SkPicture::flatten(SkWriteBuffer& buffer) const {
     buffer.writeUInt(info.getVersion());
     buffer.writeRect(info.fCullRect);
     buffer.writeUInt(info.fFlags);
+
+    if (auto custom = custom_serialize(this, buffer.fProcs)) {
+        int32_t size = SkToS32(custom->size());
+        buffer.write32(-size);    // negative for custom format
+        buffer.writePad32(custom->data(), size);
+        return;
+    }
+
     if (data) {
-        buffer.writeBool(true);
+        buffer.write32(1); // special size meaning SkPictureData
         data->flatten(buffer);
     } else {
-        buffer.writeBool(false);
+        buffer.write32(0); // signal no content
     }
 }
 
diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index a86aee2fa0..3018b879d2 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -305,8 +305,9 @@ void SkPictureData::serialize(SkWStream* stream, const SkSerialProcs& procs,
         bool write(const void*, size_t size) override { fBytesWritten += size; return true; }
         size_t bytesWritten() const override { return fBytesWritten; }
     } devnull;
+    SkSerialProcs nullProcs;
     for (int i = 0; i < fPictureCount; i++) {
-        fPictureRefs[i]->serialize(&devnull, procs, typefaceSet);
+        fPictureRefs[i]->serialize(&devnull, nullProcs, typefaceSet);
     }
 
     // We need to write factories before we write the buffer.
diff --git a/src/core/SkReadBuffer.cpp b/src/core/SkReadBuffer.cpp
index c0be4f3490..94bf34af91 100644
--- a/src/core/SkReadBuffer.cpp
+++ b/src/core/SkReadBuffer.cpp
@@ -11,27 +11,13 @@
 #include "SkImageDeserializer.h"
 #include "SkImageGenerator.h"
 #include "SkMakeUnique.h"
+#include "SkMathPriv.h"
 #include "SkMatrixPriv.h"
 #include "SkReadBuffer.h"
 #include "SkStream.h"
 #include "SkTypeface.h"
 
 namespace {
-    // If a signed int holds min_int (e.g. 0x80000000) it is undefined what happens when
-    // we negate it (even though we *know* we're 2's complement and we'll get the same
-    // value back). So we create this helper function that casts to size_t (unsigned) first,
-    // to avoid the complaint.
-    size_t sk_negate_to_size_t(int32_t value) {
-#if defined(_MSC_VER)
-#pragma warning(push)
-#pragma warning(disable : 4146)  // Thanks MSVC, we know what we're negating an unsigned
-#endif
-        return -static_cast<size_t>(value);
-#if defined(_MSC_VER)
-#pragma warning(pop)
-#endif
-    }
-
     // This generator intentionally should always fail on all attempts to get its pixels,
     // simulating a bad or empty codec stream.
     class EmptyImageGenerator final : public SkImageGenerator {
diff --git a/src/core/SkReadBuffer.h b/src/core/SkReadBuffer.h
index 61f1915e19..4c0ca9bec2 100644
--- a/src/core/SkReadBuffer.h
+++ b/src/core/SkReadBuffer.h
@@ -292,6 +292,7 @@ private:
     SkTHashMap<SkString, SkFlattenable::Factory> fCustomFactory;
 
     SkDeserialProcs fProcs;
+    friend class SkPicture;
 
 #ifdef DEBUG_NON_DETERMINISTIC_ASSERT
     // Debugging counter to keep track of how many bitmaps we
diff --git a/tests/SerialProcsTest.cpp b/tests/SerialProcsTest.cpp
index d975abb5db..0d13c035ec 100644
--- a/tests/SerialProcsTest.cpp
+++ b/tests/SerialProcsTest.cpp
@@ -9,6 +9,7 @@
 #include "Resources.h"
 #include "sk_tool_utils.h"
 #include "SkCanvas.h"
+#include "SkImageSource.h"
 #include "SkPicture.h"
 #include "SkPictureRecorder.h"
 #include "SkSerialProcs.h"
@@ -81,3 +82,98 @@ DEF_TEST(serial_procs_image, reporter) {
     }
 }
 
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
+static sk_sp<SkPicture> make_pic(const std::function<void(SkCanvas*)>& drawer) {
+    SkPictureRecorder rec;
+    drawer(rec.beginRecording(128, 128));
+    return rec.finishRecordingAsPicture();
+}
+
+static SkSerialProcs makes(SkSerialPictureProc proc, void* ctx = nullptr) {
+    SkSerialProcs procs;
+    procs.fPictureProc = proc;
+    procs.fPictureCtx = ctx;
+    return procs;
+}
+
+static SkDeserialProcs maked(SkDeserialPictureProc proc, const void* ctx = nullptr) {
+    SkDeserialProcs procs;
+    procs.fPictureProc = proc;
+    procs.fPictureCtx = const_cast<void*>(ctx);
+    return procs;
+}
+
+// packages the picture's point in the skdata, and records it in the ctx as an array
+struct Context {
+    SkTDArray<SkPicture*>   fArray;
+    SkPicture*              fSkipMe = nullptr;
+};
+
+static sk_sp<SkData> array_serial_proc(SkPicture* pic, void* ctx) {
+    Context* c = (Context*)ctx;
+    if (c->fSkipMe == pic) {
+        return nullptr;
+    }
+    *c->fArray.append() = pic;
+    return SkData::MakeWithCopy(&pic, sizeof(pic));
+}
+
+static sk_sp<SkPicture> array_deserial_proc(const void* data, size_t size, void* ctx) {
+    SkASSERT(sizeof(SkPicture*) == size);
+
+    Context* c = (Context*)ctx;
+    SkPicture* pic;
+    memcpy(&pic, data, size);
+
+    int index = c->fArray.find(pic);
+    SkASSERT(index >= 0);
+    c->fArray.removeShuffle(index);
+
+    return sk_ref_sp(pic);
+}
+
+static void test_pictures(skiatest::Reporter* reporter, sk_sp<SkPicture> p0, int count,
+                          bool skipRoot) {
+    Context ctx;
+    if (skipRoot) {
+        ctx.fSkipMe = p0.get();
+    }
+
+    auto d0 = p0->serialize(makes(array_serial_proc, &ctx));
+    REPORTER_ASSERT(reporter, ctx.fArray.count() == count);
+    p0 = SkPicture::MakeFromData(d0.get(), maked(array_deserial_proc, &ctx));
+    REPORTER_ASSERT(reporter, ctx.fArray.count() == 0);
+}
+
+DEF_TEST(serial_procs_picture, reporter) {
+
+    auto p1 = make_pic([](SkCanvas* c) {
+        // need to be large enough that drawPictures doesn't "unroll" us
+        for (int i = 0; i < 20; ++i) {
+            c->drawColor(SK_ColorRED);
+        }
+    });
+
+    // now use custom serialization
+    auto p0 = make_pic([](SkCanvas* c) { c->drawColor(SK_ColorBLUE); });
+    test_pictures(reporter, p0, 1, false);
+
+    // test inside effect
+    p0 = make_pic([p1](SkCanvas* c) {
+        SkPaint paint;
+        SkShader::TileMode tm = SkShader::kClamp_TileMode;
+        paint.setShader(SkShader::MakePictureShader(p1, tm, tm, nullptr, nullptr));
+        c->drawPaint(paint);
+    });
+    test_pictures(reporter, p0, 1, true);
+
+    // test nested picture
+    p0 = make_pic([p1](SkCanvas* c) {
+        c->drawColor(SK_ColorRED);
+        c->drawPicture(p1);
+        c->drawColor(SK_ColorBLUE);
+    });
+    test_pictures(reporter, p0, 1, true);
+}
+
diff --git a/tools/debugger/SkJsonWriteBuffer.cpp b/tools/debugger/SkJsonWriteBuffer.cpp
index bdabc8aea5..9a9a032d6b 100644
--- a/tools/debugger/SkJsonWriteBuffer.cpp
+++ b/tools/debugger/SkJsonWriteBuffer.cpp
@@ -15,6 +15,16 @@ void SkJsonWriteBuffer::append(const char* type, const Json::Value& value) {
     fJson[fullName.c_str()] = value;
 }
 
+void SkJsonWriteBuffer::writePad32(const void* data, size_t size) {
+    Json::Value jsonArray(Json::arrayValue);
+    const uint8_t* bytes = reinterpret_cast<const uint8_t*>(data);
+    for (size_t i = 0; i < size; ++i) {
+        SkString hexByte = SkStringPrintf("%02x", bytes[i]);
+        jsonArray.append(hexByte.c_str());
+    }
+    this->append("rawBytes", jsonArray);
+}
+
 void SkJsonWriteBuffer::writeByteArray(const void* data, size_t size) {
     Json::Value jsonArray(Json::arrayValue);
     const uint8_t* bytes = reinterpret_cast<const uint8_t*>(data);
diff --git a/tools/debugger/SkJsonWriteBuffer.h b/tools/debugger/SkJsonWriteBuffer.h
index c1a72313e0..efd896c989 100644
--- a/tools/debugger/SkJsonWriteBuffer.h
+++ b/tools/debugger/SkJsonWriteBuffer.h
@@ -23,6 +23,7 @@ public:
 
     bool isCrossProcess() const override { return false; }
 
+    void writePad32(const void* buffer, size_t bytes) override;
     void writeByteArray(const void* data, size_t size) override;
     void writeBool(bool value) override;
     void writeScalar(SkScalar value) override;