Handle zero-length encoded images gracefully during deserialization

src/core/SkPictureData.cpp

Index: src/core/SkPictureData.cpp
diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index 5089e5d8d6240f7d3542a933ddef3d7c9e688373..1770fbdb5fad02af08759593d96b7f8a432cfc9f 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -8,6 +8,7 @@
 #include "SkPictureData.h"
 #include "SkPictureRecord.h"
 #include "SkReadBuffer.h"
+#include "SkSurface.h"
 #include "SkTextBlob.h"
 #include "SkTypeface.h"
 #include "SkWriteBuffer.h"
@@ -433,6 +434,29 @@ bool SkPictureData::parseStreamTag(SkStream* stream,
     return true;    // success
 }
 
+static const SkImage* make_checkerboard_image(int width, int height) {
+    SkAutoTUnref<SkSurface> surface(SkSurface::NewRasterN32Premul(width, height));
+    if (!surface) {
+        return nullptr;
+    }
+
+    SkCanvas* canvas = surface->getCanvas();
+    canvas->clear(SK_ColorWHITE);
+
+    SkPaint paint;
+    paint.setColor(SK_ColorGRAY);
+
+    static const int kSize = 12;
+    for (int y = 0; y < height; y += kSize * 2) {
+        for (int x = 0; x < width; x += kSize * 2) {
+            canvas->drawRect(SkRect::MakeXYWH(x, y, kSize, kSize), paint);
+            canvas->drawRect(SkRect::MakeXYWH(x + kSize, y + kSize, kSize, kSize), paint);
+        }
+    }
+
+    return surface->newImageSnapshot();
+}
+
 static const SkImage* create_image_from_buffer(SkReadBuffer& buffer) {
     int width = buffer.read32();
     int height = buffer.read32();
@@ -442,9 +466,15 @@ static const SkImage* create_image_from_buffer(SkReadBuffer& buffer) {
     }
 
     SkAutoTUnref<SkData> encoded(buffer.readByteArrayAsData());
+    if (encoded->size() == 0) {
+        // The image could not be encoded at serialization time.
+        // Use a checkerboard instead.
+        return make_checkerboard_image(width, height);

[reed1, 2015/09/04 17:26:24]

This seems odd to me, partly because its arbitrary drawing, partly because it
can be arbitrarily big (ram), and partly because it hides the fact that there
was a failure. I was thinking we should just return NULL (and perhaps also
return width/height in case the caller wanted to know that).

[f(malita), 2015/09/04 17:50:41]

Agreed.
That's problematic because the caller (new_array_from_buffer) will immediately
fail if it sees a null value (and delete all previous images too).

In some sense, hiding the failure is exactly what we're trying to accomplish:
should loading such an SKP succeed or not?  If the answer is yes, then we're
effectively hiding the failure at some level, and the question becomes how to
best do that.

One option is to allocate a picture-backed SkImage which draws nothing.  It
saves the RAM and also preserves the SkImage metadata (dimensions).  WDYT?

+    }
+
     int originX = buffer.read32();
     int originY = buffer.read32();
-    if (0 == encoded->size() || originX < 0 || originY < 0) {
+    if (originX < 0 || originY < 0) {
         buffer.validate(false);
         return nullptr;
     }