Allow SkStream::peek() to partially succeed

src/core/SkStream.cpp

 
 /*
  * Copyright 2006 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 "SkStream.h"
@@ skipping 349 matching lines @@
     if (size > dataSize - fOffset) {
         size = dataSize - fOffset;
     }
     if (buffer) {
         memcpy(buffer, fData->bytes() + fOffset, size);
     }
     fOffset += size;
     return size;
 }
 
-bool SkMemoryStream::peek(void* buffer, size_t size) const {
+size_t SkMemoryStream::peek(void* buffer, size_t size) const {
     SkASSERT(buffer != nullptr);
+
+    if (this->isAtEnd()) {

[bungeman-skia, 2015/12/07 16:17:36]

This block seems redundant, though maybe an optimization for a low frequency
case (the user should already be checking this anyway if looping). The following
code will already read zero bytes and return 0 anyway.

[scroggo, 2015/12/07 16:52:02]

Removed.

+        return 0;
+    }
+
+    SkASSERT(fData->size() > fOffset);

[bungeman-skia, 2015/12/07 16:17:36]

This assert depends on the above block to remove the 'fOffset == fData->size()'
case. 'SkASSERT(fData->size() >= fOffset);' is a general invariant of this class
anyway, if there were a nice syntax for pre/post condition invariant asserts,
this would be one. Such a thing would want to go into 'read' and 'seek', but
this seems like a strange place to put it.

[scroggo, 2015/12/07 16:52:02]

Removed.

     const size_t position = fOffset;

[bungeman-skia, 2015/12/07 16:17:36]

It seems this be clearer (or at least more consistent) if 'position' were named
'currentOffset' or something like that.

[scroggo, 2015/12/07 16:52:02]

Done.

-    if (size > fData->size() - position) {
-        // The stream is not large enough to satisfy this request.
-        return false;
-    }
+    size = SkTMin(size, fData->size() - position);

[bungeman-skia, 2015/12/07 16:17:36]

Is there a reason to calculate this? This is what 'read' will return, is it not?
The old test here was to prevent the overhead of doing a partial read but then
having to return false. With the new semantics, this is fine. Basically, the
SkDEBUGCODE can be removes (leaving the code) and the SkASSERT can be removed.

[scroggo, 2015/12/07 16:52:02]

Done.

+
     SkMemoryStream* nonConstThis = const_cast<SkMemoryStream*>(this);
     SkDEBUGCODE(const size_t bytesRead =) nonConstThis->read(buffer, size);
     SkASSERT(bytesRead == size);
     nonConstThis->fOffset = position;
-    return true;
+    return size;
 }
 
 bool SkMemoryStream::isAtEnd() const {
     return fOffset == fData->size();
 }
 
 bool SkMemoryStream::rewind() {
     fOffset = 0;
     return true;
 }
@@ skipping 326 matching lines @@
             fCurrentOffset = 0;
         }
         SkASSERT(false);
         return 0;
     }
 
     bool isAtEnd() const override {
         return fOffset == fSize;
     }
 
-    bool peek(void* buff, size_t size) const override {
+    size_t peek(void* buff, size_t size) const override {
         SkASSERT(buff != nullptr);
-        if (fOffset + size > fSize) {
-            return false;
+
+        if (this->isAtEnd()) {

[bungeman-skia, 2015/12/07 16:17:36]

As above, this appears to be unneeded.

[scroggo, 2015/12/07 16:52:02]

Removed

+            return 0;
         }
+
+        SkASSERT(fSize > fOffset);

[bungeman-skia, 2015/12/07 16:17:36]

And 'SkASSERT(fSize >= fOffset);' should be a general invariant of this class
(note isAtEnd already depends on this).

[scroggo, 2015/12/07 16:52:02]

Removed

+        size = SkTMin(size, fSize - fOffset);
+
+        const size_t totalBytesToPeek = size;

[bungeman-skia, 2015/12/07 16:17:36]

Since we have to maintain size now to return it, I would prefer that the naming
here be changed. I'd like it to match the terms used in 'read', but those might
not be the best either. It may be clearer for 'size' here to be named
'bytesLeftToPeek' and 'totalBytesToPeek' here to be named 'bytesToPeek'.

[scroggo, 2015/12/07 16:52:02]

Changed. I think I did as you requested; let me know if you meant something
else.

         char* buffer = static_cast<char*>(buff);
         const SkDynamicMemoryWStream::Block* current = fCurrent;
         size_t currentOffset = fCurrentOffset;
         while (size) {
             SkASSERT(current);
             size_t bytesFromCurrent =
                     SkTMin(current->written() - currentOffset, size);
             memcpy(buffer, current->start() + currentOffset, bytesFromCurrent);
             size -= bytesFromCurrent;
             buffer += bytesFromCurrent;
             current = current->fNext;
             currentOffset = 0;
         }
-        return true;
+        return totalBytesToPeek;
     }
 
     bool rewind() override {
         fCurrent = fBlockMemory->fHead;
         fOffset = 0;
         fCurrentOffset = 0;
         return true;
     }
 
     SkBlockMemoryStream* duplicate() const override {
@@ skipping 215 matching lines @@
     while (true) {
         count = input->read(scratch, sizeof(scratch));
         if (0 == count) {
             return true;
         }
         if (!out->write(scratch, count)) {
             return false;
         }
     }
 }