Add a buffered SkStream class.

src/utils/SkBufferedStream.cpp

Index: src/utils/SkBufferedStream.cpp
diff --git a/src/utils/SkBufferedStream.cpp b/src/utils/SkBufferedStream.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..0778fbb15f781193bb5e504b1a158f07fcc0136c
--- /dev/null
+++ b/src/utils/SkBufferedStream.cpp
@@ -0,0 +1,137 @@
+/*
+ * Copyright 2013 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkBufferedStream.h"
+
+SkBufferedStream* SkBufferedStream::Create(SkStream* stream, size_t bufferSize) {
+    if (NULL == stream) {
+        return NULL;
+    }
+    return SkNEW_ARGS(SkBufferedStream, (stream, bufferSize));
+}
+
+SkBufferedStream::SkBufferedStream(SkStream* stream, size_t bufferSize)
+    : fStream(SkRef(stream))
+    , fOffset(0)
+    , fBufferedSoFar(0)
+    , fBufferSize(bufferSize)
+    , fBuffer(bufferSize) {}
+
+SkBufferedStream::~SkBufferedStream() {
+    fStream->unref();
+}
+
+bool SkBufferedStream::isAtEnd() const {
+    if (fOffset < fBufferedSoFar) {
+        // Even if the underlying stream is at the end, this stream has been

[mtklein, 2013/09/17 19:18:32]

Can you sketch out the invariants we're maintaining between fOffset,
fBufferedSoFar and fBufferSize somewhere in here?  Ideally as a bool validate()
method?

Am I getting it right that it should be 0 <= fOffset <= fBufferedSoFar <
fBufferSize?

[scroggo, 2013/09/17 20:12:52]

Almost. It should be:

0 <= fBufferedSoFar <= fBufferSize.

fOffset can be anywhere.

Since these are size_ts, I think the compiler may complain about me comparing
against zero, leaving a not very interesting test. I'll see if I can add some
more comments for clarity.

+        // rewound after buffering, so it is not at the end.
+        return false;
+    }
+
+    SkASSERT(fStream != NULL);
+    return fStream->isAtEnd();
+}
+
+bool SkBufferedStream::rewind() {
+    // Only allow a rewind if we have not exceeded the buffer.
+    if (fOffset <= fBufferSize) {

[mtklein, 2013/09/17 19:18:32]

Isn't this off by one?  When fOffset == fBufferSize, we're outside the buffer?

[scroggo, 2013/09/17 20:12:52]

No, fOffset is the number of bytes that were 'read', so if fOffset ==
fBufferSize, that means we have read the exact number of bytes that can be
buffered (and therefore rewound).

+        fOffset = 0;
+        return true;
+    }
+    return false;
+}
+
+bool SkBufferedStream::hasLength() const {
+    SkASSERT(fStream != NULL);

[mtklein, 2013/09/17 19:18:32]

I guess the more SkASSERTs the better, but these seem really, obviously
impossible to fail, given the SkBufferedStream creation API.

[scroggo, 2013/09/17 20:12:52]

They're impossible to fail, at least until maintenance happens. Hopefully
someone modifying this file would understand that it is expected to be non-NULL.
I am not opposed to removing these asserts.

+    return fStream->hasLength();
+}
+
+size_t SkBufferedStream::getLength() const {
+    SkASSERT(fStream != NULL);
+    return fStream->getLength();
+}
+
+// Increment a void* by a number of bytes.
+static void add_to_void_ptr(void** ptr, size_t bytes) {

[mtklein, 2013/09/17 19:18:32]

Makes me think we should be using a char* everywhere.  It's not just a void*
now, it's a void* with 1-byte alignment, so we might as well just use a char* or
uint8_t*.

[scroggo, 2013/09/17 20:12:52]

Done.

+    SkASSERT(*ptr != NULL);
+    char* charPtr = reinterpret_cast<char*>(*ptr);
+    charPtr += bytes;
+    *ptr = reinterpret_cast<void*>(charPtr);
+}
+
+size_t SkBufferedStream::read(void* dst, size_t size) {

[mtklein, 2013/09/17 19:18:32]

This is a pretty long/complex method.  I'm sort of glazing over half way through
trying to keep track of the state and invariants.  Can you break it up into
smaller steps?

[scroggo, 2013/09/17 20:12:52]

Next piece to chew off....

+    SkASSERT(fStream != NULL);
+    SkDEBUGCODE(const size_t totalSize = size;)
+    size_t readSoFar = 0;
+    if (fOffset < fBufferedSoFar) {
+        // Some data has already been copied to fBuffer. Read up to the
+        // lesser of the size requested and the remainder of the buffered
+        // data.
+        const size_t bytesToCopy = SkTMin(size, fBufferedSoFar - fOffset);
+        memcpy(dst, this->getBufferAtOffset(), bytesToCopy);

[mtklein, 2013/09/17 19:18:32]

This is just a suggestion.  Writing the API here as this->bufferAt(fOffset)
isn't any longer, but makes the call a little less context dependent.

[scroggo, 2013/09/17 20:12:52]

In this case, I only ever want the buffer at fOffset, and I am only changing
fOffset after the call. I understand the concern about context, but I also tend
to think it's safer as is.

+
+        // Update fOffset to the new position. It is guaranteed to be
+        // within the buffered data.
+        fOffset += bytesToCopy;
+        SkASSERT(fOffset <= fBufferedSoFar);
+
+        if (bytesToCopy == size) {
+            // The entire requested read was inside the already buffered
+            // data.
+            SkASSERT(fOffset <= fBufferedSoFar);
+            return size;
+        }
+
+        // The requested read extends beyond the buffered data. Update
+        // the remaining number of bytes needed to read, the number of
+        // bytes read so far, and the destination buffer.
+        size -= bytesToCopy;
+        readSoFar += bytesToCopy;
+        SkASSERT(size + readSoFar == totalSize);
+        add_to_void_ptr(&dst, bytesToCopy);
+    }
+
+    // If we got here, we have read everything that was already buffered.
+    SkASSERT(fOffset >= fBufferedSoFar);
+
+    if (fBufferedSoFar < fBufferSize) {
+        // Data needs to be buffered. Buffer up to the lesser of the size requested
+        // and the remainder of the max buffer size.
+        const size_t bytesToBuffer = SkTMin(size, fBufferSize - fBufferedSoFar);
+        void* buffer = this->getBufferAtOffset();
+        const size_t buffered = fStream->read(buffer, bytesToBuffer);
+        fBufferedSoFar += buffered;
+        fOffset = fBufferedSoFar;
+
+        // Copy the buffer to the destination buffer and update the amount read.
+        memcpy(dst, buffer, buffered);
+        readSoFar += buffered;
+
+        if (buffered == size || fStream->isAtEnd()) {
+            // We were able to buffer all of the data requested (or all the data
+            // remaining in the stream) and provide it to the caller.
+            SkASSERT(fBufferedSoFar <= fBufferSize);
+            SkASSERT(totalSize == readSoFar || fStream->isAtEnd());
+            return readSoFar;
+        }
+
+        // The requested read extends beyond the length of the buffer. Update
+        // the remaining number of bytes needed to read and the destination
+        // buffer.
+        size -= buffered;
+        SkASSERT(size + readSoFar == totalSize);
+        add_to_void_ptr(&dst, buffered);
+    }
+
+    // If we get here, we have buffered all that can be buffered.
+    SkASSERT(fBufferSize == fBufferedSoFar && fOffset >= fBufferSize);
+
+    // Read directly from the stream.
+    const size_t bytesReadDirectly = fStream->read(dst, size);
+    fOffset += bytesReadDirectly;
+    return bytesReadDirectly + readSoFar;
+}