change old picture serialization to really handle images

src/core/SkPictureData.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 #include <new>
 #include "SkPictureData.h"
 #include "SkPictureRecord.h"
 #include "SkReadBuffer.h"
@@ skipping 50 matching lines @@
 
     // templatize to consolidate with similar picture logic?
     const SkTDArray<const SkTextBlob*>& blobs = record.getTextBlobRefs();
     fTextBlobCount = blobs.count();
     if (fTextBlobCount > 0) {
         fTextBlobRefs = SkNEW_ARRAY(const SkTextBlob*, fTextBlobCount);
         for (int i = 0; i < fTextBlobCount; ++i) {
             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() {
     fPictureRefs = NULL;
     fPictureCount = 0;
     fTextBlobRefs = NULL;
     fTextBlobCount = 0;
+    fImageRefs = NULL;
+    fImageCount = 0;
     fOpData = NULL;
     fFactoryPlayback = NULL;
 }
 
 SkPictureData::~SkPictureData() {
     SkSafeUnref(fOpData);
 
     for (int i = 0; i < fPictureCount; i++) {
         fPictureRefs[i]->unref();
     }
     SkDELETE_ARRAY(fPictureRefs);
 
     for (int i = 0; i < fTextBlobCount; i++) {
         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) {
         if (fPictureRefs[i]->willPlayBackBitmaps()) {
             return true;
         }
     }
     return false;
 }
 
@@ skipping 100 matching lines @@
             buffer.writePath(fPaths[i]);
         }
     }
 
     if (fTextBlobCount > 0) {
         write_tag_size(buffer, SK_PICT_TEXTBLOB_BUFFER_TAG, fTextBlobCount);
         for (i = 0; i  < fTextBlobCount; ++i) {
             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,
                               SkPixelSerializer* pixelSerializer) const {
     write_tag_size(stream, SK_PICT_READER_TAG, fOpData->size());
     stream->write(fOpData->bytes(), fOpData->size());
 
     if (fPictureCount > 0) {
         write_tag_size(stream, SK_PICT_PICTURE_TAG, fPictureCount);
         for (int i = 0; i < fPictureCount; i++) {
@@ skipping 166 matching lines @@
             }
             if (!buffer.isValid()) {
                 return false;
             }
             SkDEBUGCODE(haveBuffer = true;)
         } break;
     }
     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);
             fBitmaps.reset(count);
             for (int i = 0; i < count; ++i) {
                 SkBitmap* bm = &fBitmaps[i];
                 if (buffer.readBitmap(bm)) {
                     bm->setImmutable();
                 } else {
                     return false;
                 }
             }
         } break;
         case SK_PICT_PAINT_BUFFER_TAG: {
             const int count = SkToInt(size);
             fPaints.reset(count);
             for (int i = 0; i < count; ++i) {
                 buffer.readPaint(&fPaints[i]);
             }
         } break;
         case SK_PICT_PATH_BUFFER_TAG:
             if (size > 0) {
                 const int count = buffer.readInt();
                 fPaths.reset(count);
                 for (int i = 0; i < count; i++) {
                     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) ||
                 !buffer.validate(NULL == fOpData)) {
                 return false;
             }
             SkASSERT(NULL == fOpData);
             fOpData = data.detach();
         } break;
-        case SK_PICT_PICTURE_TAG: {
-            if (!buffer.validate((0 == fPictureCount) && (NULL == fPictureRefs))) {
+        case SK_PICT_PICTURE_TAG:
+            if (!new_array_from_buffer(buffer, size, fPictureRefs, fPictureCount,
+                                       create_picture_from_buffer)) {
                 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;
-                return false;
-            }
-        } break;
         default:
             // The tag was invalid.
             return false;
     }
     return true;    // success
 }
 
 SkPictureData* SkPictureData::CreateFromStream(SkStream* stream,
                                                const SkPictInfo& info,
                                                SkPicture::InstallPixelRefProc proc) {
@@ skipping 67 matching lines @@
     }
 }
 
 bool SkPictureData::suitableForLayerOptimization() const {
     return fContentInfo.numLayers() > 0;
 }
 #endif
 ///////////////////////////////////////////////////////////////////////////////