Add a method to read a stream without advancing it.

Add a method to read a stream without advancing it.

Add a virtual method on SkStream which will do a "peek" some bytes, so
that those bytes are read, but the next call to read will be
unaffected.

Implement peek for SkMemoryStream, where the implementation is simple
and obvious.

Implement peek on SkFrontBufferedStream.

Add tests.

Motivated by decoding streams which cannot be rewound.

TBR=reed@google.com

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/028a4135aa6404ccd3a494813befe6e1a35e5e6c

[scroggo, 2015-03-30 17:17:27]

scroggo@google.com changed reviewers:
+ bungeman@google.com, djsollen@google.com

[bungeman-skia, 2015-03-30 19:09:00]

lgtm with possible move of declaration.

https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h#new...
include/core/SkStream.h:79: virtual size_t peek(void* buffer, size_t size) {
return 0; }
Since SkStreamSeekable must support this (slightly different semantics, since if
you call SkStreamSeekable::peek(<non-zero>) and you get back 0 that means an
error really occurred as opposed to maybe peeking is unsupported), perhaps this
should be declared a few line down in the SkStreamSeekable section.

[scroggo, 2015-03-30 19:31:50]

https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h#new...
include/core/SkStream.h:79: virtual size_t peek(void* buffer, size_t size) {
return 0; }
On 2015/03/30 19:09:00, bungeman1 wrote:
> Since SkStreamSeekable must support this (slightly different semantics, since
if
> you call SkStreamSeekable::peek(<non-zero>) and you get back 0 that means an
> error really occurred as opposed to maybe peeking is unsupported), perhaps
this
> should be declared a few line down in the SkStreamSeekable section.

FWIW, an SkStreamSeekable supports it incidentally, since the functions required
for SkStreamSeekable can be (and are) used to implement this. This seems
slightly different from saying that an SkStreamSeekable must support it. That's
not to say that I think it *doesn't* belong in that section, but it might be
something to consider.

It does seem a little odd that an error looks the same as not supported (and as
we were at the end!). We could add out parameters or use particular return
values as sentinels, but I do like the fact that this signature mirrors read().

[djsollen, 2015-03-30 19:56:54]

https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h#new...
include/core/SkStream.h:79: virtual size_t peek(void* buffer, size_t size) {
return 0; }
On 2015/03/30 19:31:49, scroggo wrote:
> On 2015/03/30 19:09:00, bungeman1 wrote:
> > Since SkStreamSeekable must support this (slightly different semantics,
since
> if
> > you call SkStreamSeekable::peek(<non-zero>) and you get back 0 that means an
> > error really occurred as opposed to maybe peeking is unsupported), perhaps
> this
> > should be declared a few line down in the SkStreamSeekable section.
> 
> FWIW, an SkStreamSeekable supports it incidentally, since the functions
required
> for SkStreamSeekable can be (and are) used to implement this. This seems
> slightly different from saying that an SkStreamSeekable must support it.
That's
> not to say that I think it *doesn't* belong in that section, but it might be
> something to consider.
> 
> It does seem a little odd that an error looks the same as not supported (and
as
> we were at the end!). We could add out parameters or use particular return
> values as sentinels, but I do like the fact that this signature mirrors
read().

I think it is important to disambiguate between the 2 states.  Particularly
since the error state means my stream is now unreliable.

[scroggo, 2015-03-31 20:14:32]

On 2015/03/30 19:56:54, djsollen wrote:
> https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h
> File include/core/SkStream.h (right):
> 
>
https://codereview.chromium.org/1044953002/diff/1/include/core/SkStream.h#new...
> include/core/SkStream.h:79: virtual size_t peek(void* buffer, size_t size) {
> return 0; }
> On 2015/03/30 19:31:49, scroggo wrote:
> > On 2015/03/30 19:09:00, bungeman1 wrote:
> > > Since SkStreamSeekable must support this (slightly different semantics,
> since
> > if
> > > you call SkStreamSeekable::peek(<non-zero>) and you get back 0 that means
an
> > > error really occurred as opposed to maybe peeking is unsupported), perhaps
> > this
> > > should be declared a few line down in the SkStreamSeekable section.
> > 
> > FWIW, an SkStreamSeekable supports it incidentally, since the functions
> required
> > for SkStreamSeekable can be (and are) used to implement this. This seems
> > slightly different from saying that an SkStreamSeekable must support it.
> That's
> > not to say that I think it *doesn't* belong in that section, but it might be
> > something to consider.
> > 
> > It does seem a little odd that an error looks the same as not supported (and
> as
> > we were at the end!). We could add out parameters or use particular return
> > values as sentinels, but I do like the fact that this signature mirrors
> read().
> 
> I think it is important to disambiguate between the 2 states.  Particularly
> since the error state means my stream is now unreliable.

Done.

I'm happy to move peek() into the SkStreamSeekable once approved, but
left it here for better diffing.

[bungeman-skia, 2015-03-31 22:04:49]

https://codereview.chromium.org/1044953002/diff/20001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1044953002/diff/20001/include/core/SkStream.h...
include/core/SkStream.h:70: static const size_t PEEK_UNSUPPORTED = (size_t) -1;
I think we tend toward enums with 'k' prefixes (though I'm fine with them not
being enums, I think we require 'enum foo : size_t' now). These all caps things
look like defines.

https://codereview.chromium.org/1044953002/diff/20001/include/core/SkStream.h...
include/core/SkStream.h:92: virtual size_t peek(void* buffer, size_t size) {
return PEEK_UNSUPPORTED; }
We already have the pattern of either returning bool (rewind, seek, move) or
having a bool capability checker (hasPosition, hasLength). I'm not sure what to
do about this one, since the root issue is that 'read' doesn't do a good job of
error reporting itself.

[scroggo, 2015-04-01 18:38:31]

https://codereview.chromium.org/1044953002/diff/20001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1044953002/diff/20001/include/core/SkStream.h...
include/core/SkStream.h:70: static const size_t PEEK_UNSUPPORTED = (size_t) -1;
On 2015/03/31 22:04:48, bungeman1 wrote:
> I think we tend toward enums with 'k' prefixes

I'm fine with renaming them to use 'k' prefixes (prefices?).

> (though I'm fine with them not
> being enums, I think we require 'enum foo : size_t' now). These all caps
things
> look like defines.

I was unfamiliar with that option. Sounds good to me.

Done.

[bungeman-skia, 2015-04-01 19:04:40]

PS3 lgtm with possible nits.

https://codereview.chromium.org/1044953002/diff/40001/tests/StreamTest.cpp
File tests/StreamTest.cpp (right):

https://codereview.chromium.org/1044953002/diff/40001/tests/StreamTest.cpp#ne...
tests/StreamTest.cpp:230: // A stream should never be able to peek less than it
can read.
Should this be

// A stream should never be able to peek *more* than it can read.

https://codereview.chromium.org/1044953002/diff/40001/tests/StreamTest.cpp#ne...
tests/StreamTest.cpp:237: SkStream* stream) {
Seems that this would fit on the previous line?

[scroggo, 2015-04-02 18:04:53]

https://codereview.chromium.org/1044953002/diff/40001/tests/StreamTest.cpp
File tests/StreamTest.cpp (right):

https://codereview.chromium.org/1044953002/diff/40001/tests/StreamTest.cpp#ne...
tests/StreamTest.cpp:237: SkStream* stream) {
On 2015/04/01 19:04:40, bungeman1 wrote:
> Seems that this would fit on the previous line?

82 chars. I'm never sure how to treat our soft limit of 80 characters...

https://codereview.chromium.org/1044953002/diff/60001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1044953002/diff/60001/include/core/SkStream.h...
include/core/SkStream.h:74: *  @param buffer Must not be NULL. Destination to
copy bytes.
Alternatively, we could return false if the buffer is NULL. (Though we cannot
check if the buffer is long enough...)

https://codereview.chromium.org/1044953002/diff/60001/tests/StreamTest.cpp
File tests/StreamTest.cpp (right):

https://codereview.chromium.org/1044953002/diff/60001/tests/StreamTest.cpp#ne...
tests/StreamTest.cpp:255: // Test an arbitrary file stream. file streams do not
support peeking.
If we decide to implement SkFILEStream::peek to vary based on whether the FILE
itself can truly peek, this test is no longer valid.

[bungeman-skia, 2015-04-02 18:24:09]

https://codereview.chromium.org/1044953002/diff/60001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.chromium.org/1044953002/diff/60001/src/core/SkStream.cpp#n...
src/core/SkStream.cpp:378: #ifdef SK_DEBUG
Should this be SkDEBUGCODE(...) instead? Then it would fit on the one line, like

SkDEBUGCODE(const size_t bytesRead =) nonConstThis->read(buffer, size);

[scroggo, 2015-04-02 19:49:43]

https://codereview.chromium.org/1044953002/diff/60001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.chromium.org/1044953002/diff/60001/src/core/SkStream.cpp#n...
src/core/SkStream.cpp:378: #ifdef SK_DEBUG
On 2015/04/02 18:24:09, bungeman1 wrote:
> Should this be SkDEBUGCODE(...) instead? Then it would fit on the one line,
like
> 
> SkDEBUGCODE(const size_t bytesRead =) nonConstThis->read(buffer, size); 

Yes. Much better.

https://codereview.chromium.org/1044953002/diff/60001/src/utils/SkFrontBuffer...
File src/utils/SkFrontBufferedStream.cpp (right):

https://codereview.chromium.org/1044953002/diff/60001/src/utils/SkFrontBuffer...
src/utils/SkFrontBufferedStream.cpp:174: return bytesRead;
Oops, this should be true. Good thing I tested in Release!

[scroggo, 2015-04-02 19:50:04]

scroggo@google.com changed reviewers:
+ reed@google.com

[scroggo, 2015-04-02 19:50:09]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-04-02 19:50:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com
Link to the patchset: https://codereview.chromium.org/1044953002/#ps80001
(title: "SkDEBUGCODE; fix a bug")

[commit-bot: I haz the power, 2015-04-02 19:50:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1044953002/80001

[commit-bot: I haz the power, 2015-04-02 19:52:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-02 19:52:57]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[scroggo, 2015-04-02 20:09:46]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-04-02 20:09:46]

The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com
Link to the patchset: https://codereview.chromium.org/1044953002/#ps100001
(title: "Stop declaring parameters I do not use.")

[scroggo, 2015-04-02 20:09:58]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-04-02 20:10:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1044953002/100001

[commit-bot: I haz the power, 2015-04-02 20:19:56]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/028a4135aa6404ccd3a494813befe6e1a35e5e6c