Fixing SkPicture serialization

src/core/SkPicture.cpp

 
 /*
  * Copyright 2007 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #include "SkPictureFlat.h"
@@ skipping 242 matching lines @@
     if (fPlayback) {
         fPlayback->draw(*surface, callback);
     }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 #include "SkStream.h"
 
 static const char kMagic[] = { 's', 'k', 'i', 'a', 'p', 'i', 'c', 't' };
-static const size_t kHeaderSize = sizeof(kMagic) + sizeof(SkPictInfo);
 
 bool SkPicture::InternalOnly_StreamIsSKP(SkStream* stream, SkPictInfo* pInfo) {
     if (NULL == stream) {
         return false;
     }
 
     // Check magic bytes.

[reed1, 2014/03/11 21:13:12]

should we have a static helper 

static bool is_valid_pictinfo(const SkPictInfo&) { ... }

it seems like the stream and readbuffer could both call it.

-    char magic[sizeof(kMagic)];
-    if (!stream->read(magic, sizeof(kMagic)) ||
-        (0 != memcmp(magic, kMagic, sizeof(kMagic)))) {
+    SkPictInfo header;
+    SkASSERT(sizeof(kMagic) == sizeof(header.fMagic));
+    if (!stream->read(&header, sizeof(header)) ||
+        (0 != memcmp(header.fMagic, kMagic, sizeof(kMagic)))) {
         return false;
     }
 
-    SkPictInfo info;
-    if (!stream->read(&info, sizeof(SkPictInfo))) {
-        return false;
-    }
-
-    if (info.fVersion < MIN_PICTURE_VERSION || info.fVersion > CURRENT_PICTURE_VERSION) {
+    if (header.fVersion < MIN_PICTURE_VERSION ||
+        header.fVersion > CURRENT_PICTURE_VERSION) {
         return false;
     }
 
     if (pInfo != NULL) {
-        *pInfo = info;
+        *pInfo = header;
     }
     return true;
 }
 
 bool SkPicture::InternalOnly_BufferIsSKP(SkReadBuffer& buffer, SkPictInfo* pInfo) {
     // Check magic bytes.
-    char magic[sizeof(kMagic)];
-
-    if (!buffer.readByteArray(magic, sizeof(kMagic)) ||
-        (0 != memcmp(magic, kMagic, sizeof(kMagic)))) {
+    SkPictInfo header;
+    SkASSERT(sizeof(kMagic) == sizeof(header.fMagic));
+    if (!buffer.readByteArray(&header, sizeof(header)) ||
+        (0 != memcmp(header.fMagic, kMagic, sizeof(kMagic)))) {
         return false;
     }
 
-    SkPictInfo info;
-    if (!buffer.readByteArray(&info, sizeof(SkPictInfo))) {
-        return false;
-    }
-
-    if (info.fVersion < MIN_PICTURE_VERSION || info.fVersion > CURRENT_PICTURE_VERSION) {
+    if (header.fVersion < MIN_PICTURE_VERSION ||
+        header.fVersion > CURRENT_PICTURE_VERSION) {
         return false;
     }
 
     if (pInfo != NULL) {
-        *pInfo = info;
+        *pInfo = header;
     }
     return true;
 }
 
 SkPicture::SkPicture(SkPicturePlayback* playback, int width, int height)
     : fPlayback(playback)
     , fRecord(NULL)
     , fWidth(width)
     , fHeight(height) {}
 
@@ skipping 32 matching lines @@
         if (NULL == playback) {
             return NULL;
         }
     } else {
         playback = NULL;
     }
 
     return SkNEW_ARGS(SkPicture, (playback, info.fWidth, info.fHeight));
 }
 
-void SkPicture::createHeader(void* header) const {
+void SkPicture::createHeader(SkPictInfo* header) const {
     // Copy magic bytes at the beginning of the header
     SkASSERT(sizeof(kMagic) == 8);
-    memcpy(header, kMagic, sizeof(kMagic));
+    SkASSERT(sizeof(kMagic) == sizeof(header->fMagic));
+    memcpy(header->fMagic, kMagic, sizeof(kMagic));
 
     // Set picture info after magic bytes in the header
-    SkPictInfo* info = (SkPictInfo*)(((char*)header) + sizeof(kMagic));
-    info->fVersion = CURRENT_PICTURE_VERSION;
-    info->fWidth = fWidth;
-    info->fHeight = fHeight;
-    info->fFlags = SkPictInfo::kCrossProcess_Flag;
+    header->fVersion = CURRENT_PICTURE_VERSION;

[reed1, 2014/03/11 21:13:12]

nit: calling this info instead of header would be a smaller change, and just as
meaningful (since the parameter type is SkPictInfo). Just fyi.

+    header->fWidth = fWidth;
+    header->fHeight = fHeight;
+    header->fFlags = SkPictInfo::kCrossProcess_Flag;
     // TODO: remove this flag, since we're always float (now)
-    info->fFlags |= SkPictInfo::kScalarIsFloat_Flag;
+    header->fFlags |= SkPictInfo::kScalarIsFloat_Flag;
 
     if (8 == sizeof(void*)) {
-        info->fFlags |= SkPictInfo::kPtrIs64Bit_Flag;
+        header->fFlags |= SkPictInfo::kPtrIs64Bit_Flag;
     }
 }
 
 void SkPicture::serialize(SkWStream* stream, EncodeBitmap encoder) const {
     SkPicturePlayback* playback = fPlayback;
 
     if (NULL == playback && fRecord) {
         playback = SkNEW_ARGS(SkPicturePlayback, (*fRecord));
     }
 
-    char header[kHeaderSize];
+    SkPictInfo header;
     this->createHeader(&header);
-    stream->write(header, kHeaderSize);
+    stream->write(&header, sizeof(header));
     if (playback) {
         stream->writeBool(true);
         playback->serialize(stream, encoder);
         // delete playback if it is a local version (i.e. cons'd up just now)
         if (playback != fPlayback) {
             SkDELETE(playback);
         }
     } else {
         stream->writeBool(false);
     }
 }
 
 void SkPicture::flatten(SkWriteBuffer& buffer) const {
     SkPicturePlayback* playback = fPlayback;
 
     if (NULL == playback && fRecord) {
         playback = SkNEW_ARGS(SkPicturePlayback, (*fRecord));
     }
 
-    char header[kHeaderSize];
+    SkPictInfo header;
     this->createHeader(&header);
-    buffer.writeByteArray(header, kHeaderSize);
+    buffer.writeByteArray(&header, sizeof(header));
     if (playback) {
         buffer.writeBool(true);
         playback->flatten(buffer);
         // delete playback if it is a local version (i.e. cons'd up just now)
         if (playback != fPlayback) {
             SkDELETE(playback);
         }
     } else {
         buffer.writeBool(false);
     }
 }
 
 bool SkPicture::willPlayBackBitmaps() const {
     if (!fPlayback) return false;
     return fPlayback->containsBitmaps();
 }
 
 #ifdef SK_BUILD_FOR_ANDROID
 void SkPicture::abortPlayback() {
     if (NULL == fPlayback) {
         return;
     }
     fPlayback->abort();
 }
 #endif