Cleaner external buffer handling in SkWriter32

include/core/SkWriter32.h

Index: include/core/SkWriter32.h
diff --git a/include/core/SkWriter32.h b/include/core/SkWriter32.h
index b193cbe7c407663f7a82a613b8a65b5e1e637a47..daadff56f7a1691c378ba7844e34919278c02c2d 100644
--- a/include/core/SkWriter32.h
+++ b/include/core/SkWriter32.h
@@ -30,12 +30,18 @@ public:
      *  first time an allocation doesn't fit.  From then it will use dynamically allocated storage.
      *  This used to be optional behavior, but pipe now relies on it.
      */
-    SkWriter32(void* external = NULL, size_t externalBytes = 0) {
-        this->reset(external, externalBytes);
+    SkWriter32(void* external = NULL, size_t externalBytes = 0)
+        : fData((uint8_t*)external)

[reed1, 2014/02/06 21:41:34]

Seems a tiny shame that the constructor and reset() don't share code.

[mtklein, 2014/02/06 22:01:24]

Agreed.  It's equivalent to write this, right?

SkWriter32(void* external = NULL, size_t externalBytes = 0) {
  this->reset(external, externalBytes);
}

[iancottrell, 2014/02/07 12:20:41]

It's functionally equivalent, but I have always thought it was wrong to do that.
Reset ought to be able to assume it is starting from a valid state, a
constructor cannot. Also a compiler will do a much better job dealing with an
initializer list than a method call.
If there was any actual logic in here, I would extract it to something called by
both.
I will happily change this if you like, just explaining my reasoning.

+        , fCapacity(externalBytes)
+        , fUsed(0)
+        , fExternal(external)
+    {
+        SkASSERT(SkIsAlign4((uintptr_t)external));
+        SkASSERT(SkIsAlign4(externalBytes));
     }
 
     // return the current offset (will always be a multiple of 4)
-    size_t bytesWritten() const { return fCount * 4; }
+    size_t bytesWritten() const { return fUsed; }
 
     SK_ATTR_DEPRECATED("use bytesWritten")
     size_t size() const { return this->bytesWritten(); }
@@ -43,45 +49,43 @@ public:
     void reset(void* external = NULL, size_t externalBytes = 0) {
         SkASSERT(SkIsAlign4((uintptr_t)external));
         SkASSERT(SkIsAlign4(externalBytes));
-        fExternal = (uint32_t*)external;
-        fExternalLimit = SkToInt(externalBytes/4);
-        fCount = 0;
-        fInternal.rewind();
+
+        fData = (uint8_t*)external;
+        fCapacity = externalBytes;
+        fUsed = 0;
+        fExternal = external;
     }
 
-    // If all data written is contiguous, then this returns a pointer to it, otherwise NULL.
-    // This will work if we've only written to the externally supplied block of storage, or if we've
-    // only written to our internal dynamic storage, but will fail if we have written into both.
+    // Returns the current buffer.
+    // The pointer may be invalidated by any future write calls.
     const uint32_t* contiguousArray() const {
-        if (this->externalCount()  == 0) {
-            return fInternal.begin();
-        } else if (fInternal.isEmpty()) {
-            return fExternal;
-        }
-        return NULL;
+        return (uint32_t*)fData;
     }
 
     // size MUST be multiple of 4
     uint32_t* reserve(size_t size) {
         SkASSERT(SkAlign4(size) == size);
-        const int count = SkToInt(size/4);
-
-        uint32_t* p;
-        // Once we start writing to fInternal, we never write to fExternal again.
-        // This simplifies tracking what data is where.
-        if (fInternal.isEmpty() && this->externalCount() + count <= fExternalLimit) {
-            p = fExternal + fCount;
-        } else {
-            p = fInternal.append(count);
+        size_t offset = fUsed;
+        size_t used = fUsed + size;

[mtklein, 2014/02/06 22:01:24]

This might make more sense if you rename used to required.

size_t offset = fUsed;
size_t required = fUsed + size;
if (required > fCapacity) {
    this->grow(required);
}
fUsed = required;
return (uint32_t*)(fData + offset);

[iancottrell, 2014/02/07 12:20:41]

Done.

+        if (used > fCapacity) {
+            grow(used);
         }
-
-        fCount += count;
-        return p;
+        fUsed = used;
+        return (uint32_t*)(fData + offset);
     }
 
     // Read or write 4 bytes at offset, which must be a multiple of 4 <= size().
-    uint32_t read32At(size_t offset) { return this->atOffset(offset); }
-    void write32At(size_t offset, uint32_t val) { this->atOffset(offset) = val; }
+    uint32_t read32At(size_t offset) {
+        SkASSERT(SkAlign4(offset) == offset);
+        SkASSERT(offset < fUsed);
+        return *(uint32_t*)(fData + offset);
+    }
+
+    void write32At(size_t offset, uint32_t val) {
+        SkASSERT(SkAlign4(offset) == offset);
+        SkASSERT(offset < fUsed);
+        *(uint32_t*)(fData + offset) = val;
+    }
 
     bool writeBool(bool value) {
         this->write32(value);
@@ -157,8 +161,6 @@ public:
      */
     void write(const void* values, size_t size) {
         SkASSERT(SkAlign4(size) == size);
-        // TODO: If we're going to spill from fExternal to fInternal, we might want to fill
-        // fExternal as much as possible before writing to fInternal.
         memcpy(this->reserve(size), values, size);
     }
 
@@ -209,26 +211,17 @@ public:
      */
     void rewindToOffset(size_t offset) {
         SkASSERT(SkAlign4(offset) == offset);
-        const int count = SkToInt(offset/4);
-        if (count < this->externalCount()) {
-            fInternal.setCount(0);
-        } else {
-            fInternal.setCount(count - this->externalCount());
-        }
-        fCount = count;
+        SkASSERT(offset <= bytesWritten());
+        fUsed = offset;
     }
 
     // copy into a single buffer (allocated by caller). Must be at least size()
     void flatten(void* dst) const {

[reed1, 2014/02/06 21:41:34]

A future iteration of the API might deprecate this signature, in favor of one
where the caller also passes the size of their buffer, just for sanity checking.

[mtklein, 2014/02/06 22:01:24]

Ack.  With a .release() and a .writeToStream(), we may be able to even eliminate
this method altogether.

[iancottrell, 2014/02/07 12:20:41]

Yes, the stuff to release the buffer is coming in cl number 3 (2 is the one that
changes the array growth pattern). Once we have that, we should deprecate
flatten and contiguousArray

-        const size_t externalBytes = this->externalCount()*4;
-        memcpy(dst, fExternal, externalBytes);
-        dst = (uint8_t*)dst + externalBytes;
-        memcpy(dst, fInternal.begin(), fInternal.bytes());
+        memcpy(dst, fData, fUsed);
     }
 
     bool writeToStream(SkWStream* stream) const {
-        return stream->write(fExternal, this->externalCount()*4)
-            && stream->write(fInternal.begin(), fInternal.bytes());
+        return stream->write(fData, fUsed);
     }
 
     // read from the stream, and write up to length bytes. Return the actual
@@ -238,25 +231,13 @@ public:
     }
 
 private:
-    uint32_t& atOffset(size_t offset) {
-        SkASSERT(SkAlign4(offset) == offset);
-        const int count = SkToInt(offset/4);
-        SkASSERT(count < fCount);
-
-        if (count < this->externalCount()) {
-            return fExternal[count];
-        }
-        return fInternal[count - this->externalCount()];
-    }
-
-
-    // Number of uint32_t written into fExternal.  <= fExternalLimit.
-    int externalCount() const { return fCount - fInternal.count(); }
+    void grow(size_t size);
 
-    int fCount;                     // Total number of uint32_t written.
-    int fExternalLimit;             // Number of uint32_t we can write to fExternal.
-    uint32_t* fExternal;            // Unmanaged memory block.
-    SkTDArray<uint32_t> fInternal;  // Managed memory block.
+    uint8_t* fData;                // Points to either fInternal or fExternal.
+    size_t fCapacity;              // Number of bytes we can write to fData.
+    size_t fUsed;                  // Number of bytes written.
+    void* fExternal;               // Unmanaged memory block.
+    SkTDArray<uint8_t> fInternal;  // Managed memory block.
 };
 
 /**