change old picture serialization to really handle images

src/core/SkPictureData.cpp

Index: src/core/SkPictureData.cpp
diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index ddd074e9a49882d6d762c5063aec8476c1d07c5e..32d965f6fc9eb495e3f0c78335adae2215da6aa3 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -68,6 +68,15 @@ SkPictureData::SkPictureData(const SkPictureRecord& record,
             fTextBlobRefs[i] = SkRef(blobs[i]);
         }
     }
+        
+    const SkTDArray<const SkImage*>& imgs = record.getImageRefs();
+    fImageCount = imgs.count();
+    if (fImageCount > 0) {
+        fImageRefs = SkNEW_ARRAY(const SkImage*, fImageCount);
+        for (int i = 0; i < fImageCount; ++i) {
+            fImageRefs[i] = SkRef(imgs[i]);
+        }
+    }
 }
 
 void SkPictureData::init() {
@@ -75,6 +84,8 @@ void SkPictureData::init() {
     fPictureCount = 0;
     fTextBlobRefs = NULL;
     fTextBlobCount = 0;
+    fImageRefs = NULL;
+    fImageCount = 0;
     fOpData = NULL;
     fFactoryPlayback = NULL;
 }
@@ -91,12 +102,17 @@ SkPictureData::~SkPictureData() {
         fTextBlobRefs[i]->unref();
     }
     SkDELETE_ARRAY(fTextBlobRefs);
-
+    
+    for (int i = 0; i < fImageCount; i++) {
+        fImageRefs[i]->unref();
+    }
+    SkDELETE_ARRAY(fImageRefs);
+    
     SkDELETE(fFactoryPlayback);
 }
 
 bool SkPictureData::containsBitmaps() const {
-    if (fBitmaps.count() > 0) {
+    if (fBitmaps.count() > 0 || fImageCount > 0) {
         return true;
     }
     for (int i = 0; i < fPictureCount; ++i) {
@@ -217,6 +233,13 @@ void SkPictureData::flattenToBuffer(SkWriteBuffer& buffer) const {
             fTextBlobRefs[i]->flatten(buffer);
         }
     }
+    
+    if (fImageCount > 0) {
+        write_tag_size(buffer, SK_PICT_IMAGE_BUFFER_TAG, fImageCount);
+        for (i = 0; i  < fImageCount; ++i) {
+            buffer.writeImage(fImageRefs[i]);
+        }
+    }
 }
 
 void SkPictureData::serialize(SkWStream* stream,
@@ -403,8 +426,62 @@ bool SkPictureData::parseStreamTag(SkStream* stream,
     return true;    // success
 }
 
-bool SkPictureData::parseBufferTag(SkReadBuffer& buffer,
-                                   uint32_t tag, uint32_t size) {
+static const SkImage* create_image_from_buffer(SkReadBuffer& buffer) {
+    int width = buffer.read32();
+    int height = buffer.read32();
+    if (width < 0 || height < 0) {

[scroggo, 2015/06/22 17:40:01]

I thought images had to have non-zero dimensions?

[reed1, 2015/06/22 18:41:50]

Done.

+        buffer.validate(false);
+        return NULL;
+    }
+
+    SkAutoTUnref<SkData> encoded(buffer.readByteArrayAsData());
+    int originX = buffer.read32();
+    int originY = buffer.read32();
+    if (!encoded || originX < 0 || originY < 0) {

[scroggo, 2015/06/22 17:40:01]

It looks like readByteArrayAsData() always returns non-NULL. Should we instead
check if it is empty?

[reed1, 2015/06/22 18:41:51]

Done.

+        buffer.validate(false);
+        return NULL;
+    }
+
+    const SkIRect subset = SkIRect::MakeXYWH(originX, originY, width, height);
+    return SkImage::NewFromEncoded(encoded, &subset);
+}
+
+static const SkPicture* create_picture_from_buffer(SkReadBuffer& buffer) {

[scroggo, 2015/06/22 17:40:01]

Can we not just use SkPicture::CreateFromBuffer directly (like we do for
SkTextBlob)?

[reed1, 2015/06/22 18:41:51]

That is defined to return SkPicture*, but the other methods return const T*, so
I needed a const-wrapper. Will add dox to that effect.

+    return SkPicture::CreateFromBuffer(buffer);
+}
+
+template <typename T>
+bool new_array_from_buffer(SkReadBuffer& buffer, uint32_t size,

[scroggo, 2015/06/22 17:40:01]

I found this signature confusing until I found the place where it's used. Can we
change the names to distinguish between size and count? Maybe size should be
encodedSize or reportedSize or something?

[reed1, 2015/06/22 18:41:50]

Done.

+                           const T**& array, int& count, const T* (*factory)(SkReadBuffer&)) {

[scroggo, 2015/06/22 17:40:01]

nit: Don't we typically use pointers for variables we're going to modify, rather
than non-const references?

[reed1, 2015/06/22 18:41:51]

Done.

+    if (!buffer.validate((0 == count) && (NULL == array))) {
+        return false;
+    }
+    count = size;
+    array = SkNEW_ARRAY(const T*, count);

[scroggo, 2015/06/22 17:40:01]

If size is 0, we dutifully create a 0 size array. Should we early exit in that
case?

[reed1, 2015/06/22 18:41:50]

Done.

+    bool success = true;
+    int i = 0;
+    for (; i < count; i++) {
+        array[i] = factory(buffer);
+        if (NULL == array[i]) {
+            success = false;
+            break;
+        }
+    }
+    if (!success) {
+        // Delete all of the blobs that were already created (up to but excluding i):
+        for (int j = 0; j < i; j++) {
+            array[j]->unref();
+        }
+        // Delete the array
+        SkDELETE_ARRAY(array);
+        array = NULL;
+        count = 0;
+        return false;

[scroggo, 2015/06/22 17:40:01]

Should we validate(false) the buffer? Or do we assume that factory did that when
it returned NULL?

[reed1, 2015/06/22 18:41:51]

I think we assume the factories did it, if something was invalid. We could be
"valid" but still fail to instantiate a subclass for some other reason. Anyway,
the old logic did it this way.

+    }
+    return true;
+}
+
+bool SkPictureData::parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t size) {
     switch (tag) {
         case SK_PICT_BITMAP_BUFFER_TAG: {
             const int count = SkToInt(size);
@@ -433,33 +510,18 @@ bool SkPictureData::parseBufferTag(SkReadBuffer& buffer,
                     buffer.readPath(&fPaths[i]);
                 }
             } break;
-        case SK_PICT_TEXTBLOB_BUFFER_TAG: {
-            if (!buffer.validate((0 == fTextBlobCount) && (NULL == fTextBlobRefs))) {
+        case SK_PICT_TEXTBLOB_BUFFER_TAG:
+            if (!new_array_from_buffer(buffer, size, fTextBlobRefs, fTextBlobCount,
+                                       SkTextBlob::CreateFromBuffer)) {
                 return false;
             }
-            fTextBlobCount = size;
-            fTextBlobRefs = SkNEW_ARRAY(const SkTextBlob*, fTextBlobCount);
-            bool success = true;
-            int i = 0;
-            for ( ; i < fTextBlobCount; i++) {
-                fTextBlobRefs[i] = SkTextBlob::CreateFromBuffer(buffer);
-                if (NULL == fTextBlobRefs[i]) {
-                    success = false;
-                    break;
-                }
-            }
-            if (!success) {
-                // Delete all of the blobs that were already created (up to but excluding i):
-                for (int j = 0; j < i; j++) {
-                    fTextBlobRefs[j]->unref();
-                }
-                // Delete the array
-                SkDELETE_ARRAY(fTextBlobRefs);
-                fTextBlobRefs = NULL;
-                fTextBlobCount = 0;
+            break;
+        case SK_PICT_IMAGE_BUFFER_TAG:
+            if (!new_array_from_buffer(buffer, size, fImageRefs, fImageCount,
+                                       create_image_from_buffer)) {
                 return false;
             }
-        } break;
+            break;
         case SK_PICT_READER_TAG: {
             SkAutoDataUnref data(SkData::NewUninitialized(size));
             if (!buffer.readByteArray(data->writable_data(), size) ||
@@ -469,32 +531,11 @@ bool SkPictureData::parseBufferTag(SkReadBuffer& buffer,
             SkASSERT(NULL == fOpData);
             fOpData = data.detach();
         } break;
-        case SK_PICT_PICTURE_TAG: {
-            if (!buffer.validate((0 == fPictureCount) && (NULL == fPictureRefs))) {
-                return false;
-            }
-            fPictureCount = size;
-            fPictureRefs = SkNEW_ARRAY(const 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;
+        case SK_PICT_PICTURE_TAG:
+            if (!new_array_from_buffer(buffer, size, fPictureRefs, fPictureCount,
+                                       create_picture_from_buffer)) {
                 return false;
             }
-        } break;
         default:
             // The tag was invalid.
             return false;