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>

3 files changed, 23 insertions, 53 deletions

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) {