Change behavior of custom image serial/deserial

New behavior is to *always* call the client's deserial image proc for all data.
This allows the client to make decisions even on "std" image data like PNG.

The change also means that if there is no client deserial image proc, Skia will
still attempt to create an image from the data, even if it was written by a
custom serial proc.

Bug: skia:7706
Change-Id: Ia58bdd10b86d497f02187082c6373c029e9c8293
Reviewed-on: https://skia-review.googlesource.com/113302
Reviewed-by: Florin Malita <fmalita@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@google.com>
Commit-Queue: Mike Reed <reed@google.com>

6 files changed, 41 insertions, 60 deletions

diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index b2beb56b86..1a5ecb360b 100644
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -168,10 +168,11 @@ private:
     // V59: No more LocalSpace option on PictureImageFilter
     // V60: Remove flags in picture header
     // V61: Change SkDrawPictureRec to take two colors rather than two alphas
+    // V62: Don't negate size of custom encoded images (don't write origin x,y either)
 
     // Only SKPs within the min/current picture version range (inclusive) can be read.
     static const uint32_t     MIN_PICTURE_VERSION = 56;     // august 2017
-    static const uint32_t CURRENT_PICTURE_VERSION = 61;
+    static const uint32_t CURRENT_PICTURE_VERSION = 62;
 
     static bool IsValidPictInfo(const SkPictInfo& info);
     static sk_sp<SkPicture> Forwardport(const SkPictInfo&,
diff --git a/include/core/SkSerialProcs.h b/include/core/SkSerialProcs.h
index 0c20f08b5c..e5a886a332 100644
--- a/include/core/SkSerialProcs.h
+++ b/include/core/SkSerialProcs.h
@@ -18,7 +18,7 @@
  *  If null is returned, then Skia will take its default action.
  *
  *  The default action for pictures is to use Skia's internal format.
- *  The default action for images is to encode using PNG.
+ *  The default action for images is to encode either in its native format or PNG.
  *  The default action for typefaces is to use Skia's internal format.
  */
 
@@ -27,13 +27,24 @@ typedef sk_sp<SkData> (*SkSerialImageProc)(SkImage*, void* ctx);
 typedef sk_sp<SkData> (*SkSerialTypefaceProc)(SkTypeface*, void* ctx);
 
 /**
- *  A deserial-proc is given the serialized form previously returned by the corresponding
- *  serial-proc, and should return the re-constituted object. In case of an error, the proc
- *  can return nullptr.
+ *  Called with the encoded form of a picture (previously written with a custom
+ *  SkSerialPictureProc proc). Return a picture object, or nullptr indicating failure.
  */
-
 typedef sk_sp<SkPicture> (*SkDeserialPictureProc)(const void* data, size_t length, void* ctx);
+
+/**
+ *  Called with the encoded from of an image. The proc can return an image object, or if it
+ *  returns nullptr, then Skia will take its default action to try to create an image from the data.
+ *
+ *  Note that unlike SkDeserialPictureProc and SkDeserialTypefaceProc, return nullptr from this
+ *  does not indicate failure, but is a signal for Skia to take its default action.
+ */
 typedef sk_sp<SkImage> (*SkDeserialImageProc)(const void* data, size_t length, void* ctx);
+
+/**
+ *  Called with the encoded form of a typeface (previously written with a custom
+ *  SkSerialTypefaceProc proc). Return a typeface object, or nullptr indicating failure.
+ */
 typedef sk_sp<SkTypeface> (*SkDeserialTypefaceProc)(const void* data, size_t length, void* ctx);
 
 struct SK_API SkSerialProcs {
diff --git a/src/core/SkReadBuffer.cpp b/src/core/SkReadBuffer.cpp
index 73e7af0fd7..1bd01d504a 100644
--- a/src/core/SkReadBuffer.cpp
+++ b/src/core/SkReadBuffer.cpp
@@ -279,48 +279,37 @@ sk_sp<SkImage> SkReadBuffer::readImage() {
         return nullptr;
     }
 
-    /*
-     *  What follows is a 32bit encoded size.
-     *   0 : failure, nothing else to do
-     *  <0 : negative (int32_t) of a custom encoded blob using SerialProcs
-     *  >0 : standard encoded blob size (use MakeFromEncoded)
-     */
-
-    int32_t encoded_size = this->read32();
-    if (encoded_size == 0) {
+    int32_t size = this->read32();
+
+    // we used to negate the size for "custom" encoded images -- ignore that signal (Dec-2017)
+    size = SkAbs32(size);
+
+    if (size == 0) {
         // The image could not be encoded at serialization time - return an empty placeholder.
         return MakeEmptyImage(width, height);
     }
-    if (encoded_size == 1) {
+    if (size == 1) {
         // legacy check (we stopped writing this for "raw" images Nov-2017)
         this->validate(false);
         return nullptr;
     }
 
-    size_t size = SkAbs32(encoded_size);
     sk_sp<SkData> data = SkData::MakeUninitialized(size);
     if (!this->readPad32(data->writable_data(), size)) {
         this->validate(false);
         return nullptr;
     }
-    int32_t originX = this->read32();
-    int32_t originY = this->read32();
-    if (originX < 0 || originY < 0) {
-        this->validate(false);
-        return nullptr;
+    if (this->isVersionLT(kDontNegateImageSize_Version)) {
+        (void)this->read32();   // originX
+        (void)this->read32();   // originY
     }
 
     sk_sp<SkImage> image;
-    if (encoded_size < 0) {     // custom encoded, need serial proc
-        if (fProcs.fImageProc) {
-            image = fProcs.fImageProc(data->data(), data->size(), fProcs.fImageCtx);
-        } else {
-            // Nothing to do (no client proc), but since we've already "read" the custom data,
-            // wee just leave image as nullptr.
-        }
-    } else {
-        SkIRect subset = SkIRect::MakeXYWH(originX, originY, width, height);
-        image = SkImage::MakeFromEncoded(std::move(data), &subset);
+    if (fProcs.fImageProc) {
+        image = fProcs.fImageProc(data->data(), data->size(), fProcs.fImageCtx);
+    }
+    if (!image) {
+        image = SkImage::MakeFromEncoded(std::move(data));
     }
     // Question: are we correct to return an "empty" image instead of nullptr, if the decoder
     //           failed for some reason?
diff --git a/src/core/SkReadBuffer.h b/src/core/SkReadBuffer.h
index c3d6d4eb30..05b65dc493 100644
--- a/src/core/SkReadBuffer.h
+++ b/src/core/SkReadBuffer.h
@@ -77,6 +77,7 @@ public:
         kRemovePictureImageFilterLocalSpace = 59,
         kRemoveHeaderFlags_Version         = 60,
         kTwoColorDrawShadow_Version        = 61,
+        kDontNegateImageSize_Version       = 62,
     };
 
     /**
diff --git a/src/core/SkWriteBuffer.cpp b/src/core/SkWriteBuffer.cpp
index 34650719b3..fb3ad47383 100644
--- a/src/core/SkWriteBuffer.cpp
+++ b/src/core/SkWriteBuffer.cpp
@@ -138,40 +138,20 @@ void SkBinaryWriteBuffer::writeImage(const SkImage* image) {
     this->writeInt(image->width());
     this->writeInt(image->height());
 
-    auto write_data = [this](sk_sp<SkData> data, int sign) {
-        size_t size = data ? data->size() : 0;
-        if (!sk_64_isS32(size)) {
-            size = 0;   // too big to store
-        }
-        if (size) {
-            this->write32(SkToS32(size) * sign);
-            this->writePad32(data->data(), size);    // does nothing if size == 0
-            this->write32(0);   // origin-x
-            this->write32(0);   // origin-y
-        } else {
-            this->write32(0);   // signal no image
-        }
-    };
-
-    /*
-     *  What follows is a 32bit encoded size.
-     *   0 : failure, nothing else to do
-     *  <0 : negative (int32_t) of a custom encoded blob using SerialProcs
-     *  >0 : standard encoded blob size (use MakeFromEncoded)
-     */
     sk_sp<SkData> data;
-    int sign = 1;   // +1 signals standard encoder
     if (fProcs.fImageProc) {
         data = fProcs.fImageProc(const_cast<SkImage*>(image), fProcs.fImageCtx);
-        sign = -1;  // +1 signals custom encoder
     }
-    // We check data, since a custom proc can return nullptr, in which case we behave as if
-    // there was no custom proc.
     if (!data) {
         data = image->encodeToData();
-        sign = 1;
     }
-    write_data(std::move(data), sign);
+
+    size_t size = data ? data->size() : 0;
+    if (!sk_64_isS32(size)) {
+        size = 0;   // too big to store
+    }
+    this->write32(SkToS32(size));           // writing 0 signals failure
+    this->writePad32(data->data(), size);   // does nothing if size == 0
 }
 
 void SkBinaryWriteBuffer::writeTypeface(SkTypeface* obj) {
diff --git a/tests/SerialProcsTest.cpp b/tests/SerialProcsTest.cpp
index c408dc7eb6..fd82079147 100644
--- a/tests/SerialProcsTest.cpp
+++ b/tests/SerialProcsTest.cpp
@@ -38,7 +38,6 @@ DEF_TEST(serial_procs_image, reporter) {
     };
     const SkDeserialImageProc dprocs[] = {
         [](const void* data, size_t length, void*) -> sk_sp<SkImage> {
-            SK_ABORT("should not get called");
             return nullptr;
         },
         [](const void* data, size_t length, void*) {