Remove position from FrontBufferedStream

Remove position from FrontBufferedStream

When FrontBufferedStream was written, it implemented getPosition()
and returned true for hasPosition(). The CL that introduced it
(crrev.com/23717055) does not have any indication why, but I'm guessing
it was because it was easy to implement. None of our decoders rely on
this (only some tests do).

Now that we have a decoder (SkRawCodec) that expects to be able to seek
a stream if it has a position (which makes sense - SkStream.h
associates that with SkStreamSeekable, and there is no other way to
check to see if a stream is seekable), it is failing because
FrontBufferedStream reports it has a position and the decoder tries to
seek.

Remove FrontBufferedStream::hasPosition() (reverting to the default,
false) and ::getPosition() (so it will return 0).

Fix tests - do not call FrontBufferedStream::getPosition()

Update CodexTest to test using an FrontBufferedStream, like Android
does.

BUG=b/27218441
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1703293002

Committed: https://skia.googlesource.com/skia/+/ef0fed3bf06de231b66be5f5279468afd01e6f75

[scroggo, 2016-02-17 23:07:15]

Description was changed from

==========
Remove position from FrontBufferedStream

When FrontBufferedStream was written, it implemented getPosition()
and returned true for hasPosition(). The CL that introduced it
(crrev.com/23717055) does not have any indication why, but I'm guessing
it was because it was easy to implement. None of our decoders rely on
this (only some tests do).

Now that we have a decoder (SkRawCodec) that expects to be able to seek
a stream if it has a position (which makes sense - SkStream.h
associates that with SkStreamSeekable, and there is no other way to
check to see if a stream is seekable), it is failing because
FrontBufferedStream reports it has a position and the decoder tries to
seek.

Remove FrontBufferedStream::hasPosition() (reverting to the default,
false) and ::getPosition() (so it will return 0).

Fix tests - do not call FrontBufferedStream::getPosition()

Update CodexTest to test using an FrontBufferedStream, like Android
does.

BUG=b/27218441
==========

to

==========
Remove position from FrontBufferedStream

When FrontBufferedStream was written, it implemented getPosition()
and returned true for hasPosition(). The CL that introduced it
(crrev.com/23717055) does not have any indication why, but I'm guessing
it was because it was easy to implement. None of our decoders rely on
this (only some tests do).

Now that we have a decoder (SkRawCodec) that expects to be able to seek
a stream if it has a position (which makes sense - SkStream.h
associates that with SkStreamSeekable, and there is no other way to
check to see if a stream is seekable), it is failing because
FrontBufferedStream reports it has a position and the decoder tries to
seek.

Remove FrontBufferedStream::hasPosition() (reverting to the default,
false) and ::getPosition() (so it will return 0).

Fix tests - do not call FrontBufferedStream::getPosition()

Update CodexTest to test using an FrontBufferedStream, like Android
does.

BUG=b/27218441
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[scroggo, 2016-02-17 23:09:19]

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

[scroggo, 2016-02-17 23:09:20]

https://codereview.chromium.org/1703293002/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1703293002/diff/1/tests/CodexTest.cpp#newcode328
tests/CodexTest.cpp:328: // Test using SkFrontBufferedStream, as Android does
Should've had this test anyway. This would've caught the problem in SkRawCodec.

[scroggo, 2016-02-17 23:10:05]

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

[bungeman-skia, 2016-02-17 23:44:35]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[bungeman-skia, 2016-02-17 23:44:43]

lgtm

[commit-bot: I haz the power, 2016-02-17 23:44:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1703293002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1703293002/1

[commit-bot: I haz the power, 2016-02-17 23:54:58]

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

[commit-bot: I haz the power, 2016-02-17 23:54:59]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[bungeman-skia, 2016-02-18 00:12:17]

On 2016/02/17 23:54:59, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
> (JOB_FAILED,
>
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

Hmmmm... looks like test_peeking_front_buffered_stream was using the position to
test things. Do we need a 'canSeek' method? I think the original intent was that
if the subclass of SkStream could not seek, then there wasn't any point in
determining what the position is (maybe there is, but it doesn't seem common,
just this test right?). Was the issue just that SkRawCodec assumed that seeking
couldn't fail if hasPosition() returned true? Or was it that it wants to know up
front if it's 'seekable' or not and code paths diverge at some point?

[scroggo, 2016-02-18 13:31:44]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-02-18 13:31:44]

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/1703293002/#ps20001
(title: "Fix test_peeking_front_buffered_stream")

[commit-bot: I haz the power, 2016-02-18 13:32:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1703293002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1703293002/20001

[scroggo, 2016-02-18 13:42:44]

On 2016/02/18 00:12:17, bungeman1 wrote:
> On 2016/02/17 23:54:59, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
> > (JOB_FAILED,
> >
>
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
> 
> Hmmmm... looks like test_peeking_front_buffered_stream was using the position
to
> test things.

FrontBufferedStreamTest also used the position to test things, but some simple
changes allowed us to work around that while still effectively testing the
FrontBufferedStream. Patch set 2 fixes the StreamTest in the same way (stop
calling FrontBufferedStream::getPosition()).

> Do we need a 'canSeek' method? I think the original intent was that
> if the subclass of SkStream could not seek, then there wasn't any point in
> determining what the position is (maybe there is, but it doesn't seem common,
> just this test right?).

'canSeek' might be interesting, although the fact that we are only using
getPosition without expecting to seek in tests is a sign that we probably don't
need to use it that way.

> Was the issue just that SkRawCodec assumed that seeking
> couldn't fail if hasPosition() returned true? Or was it that it wants to know
up
> front if it's 'seekable' or not and code paths diverge at some point?

The latter. SkRawCodec needs a seekable stream. So if it's not seekable, it will
buffer the whole thing. But if it is seekable, we want to avoid all that
buffering.

[commit-bot: I haz the power, 2016-02-18 13:59:27]

Description was changed from

==========
Remove position from FrontBufferedStream

When FrontBufferedStream was written, it implemented getPosition()
and returned true for hasPosition(). The CL that introduced it
(crrev.com/23717055) does not have any indication why, but I'm guessing
it was because it was easy to implement. None of our decoders rely on
this (only some tests do).

Now that we have a decoder (SkRawCodec) that expects to be able to seek
a stream if it has a position (which makes sense - SkStream.h
associates that with SkStreamSeekable, and there is no other way to
check to see if a stream is seekable), it is failing because
FrontBufferedStream reports it has a position and the decoder tries to
seek.

Remove FrontBufferedStream::hasPosition() (reverting to the default,
false) and ::getPosition() (so it will return 0).

Fix tests - do not call FrontBufferedStream::getPosition()

Update CodexTest to test using an FrontBufferedStream, like Android
does.

BUG=b/27218441
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Remove position from FrontBufferedStream

When FrontBufferedStream was written, it implemented getPosition()
and returned true for hasPosition(). The CL that introduced it
(crrev.com/23717055) does not have any indication why, but I'm guessing
it was because it was easy to implement. None of our decoders rely on
this (only some tests do).

Now that we have a decoder (SkRawCodec) that expects to be able to seek
a stream if it has a position (which makes sense - SkStream.h
associates that with SkStreamSeekable, and there is no other way to
check to see if a stream is seekable), it is failing because
FrontBufferedStream reports it has a position and the decoder tries to
seek.

Remove FrontBufferedStream::hasPosition() (reverting to the default,
false) and ::getPosition() (so it will return 0).

Fix tests - do not call FrontBufferedStream::getPosition()

Update CodexTest to test using an FrontBufferedStream, like Android
does.

BUG=b/27218441
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/ef0fed3bf06de231b66be5f5279468afd01e6f75
==========

[commit-bot: I haz the power, 2016-02-18 13:59:32]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/ef0fed3bf06de231b66be5f5279468afd01e6f75

[bungeman-skia, 2016-02-18 13:59:34]

On 2016/02/18 13:42:44, scroggo wrote:
> On 2016/02/18 00:12:17, bungeman1 wrote:
> > On 2016/02/17 23:54:59, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
> > > (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
> > 
> > Hmmmm... looks like test_peeking_front_buffered_stream was using the
position
> to
> > test things.
> 
> FrontBufferedStreamTest also used the position to test things, but some simple
> changes allowed us to work around that while still effectively testing the
> FrontBufferedStream. Patch set 2 fixes the StreamTest in the same way (stop
> calling FrontBufferedStream::getPosition()).
> 
> > Do we need a 'canSeek' method? I think the original intent was that
> > if the subclass of SkStream could not seek, then there wasn't any point in
> > determining what the position is (maybe there is, but it doesn't seem
common,
> > just this test right?).
> 
> 'canSeek' might be interesting, although the fact that we are only using
> getPosition without expecting to seek in tests is a sign that we probably
don't
> need to use it that way.
> 
> > Was the issue just that SkRawCodec assumed that seeking
> > couldn't fail if hasPosition() returned true? Or was it that it wants to
know
> up
> > front if it's 'seekable' or not and code paths diverge at some point?
> 
> The latter. SkRawCodec needs a seekable stream. So if it's not seekable, it
will
> buffer the whole thing. But if it is seekable, we want to avoid all that
> buffering.

sgtm, still lgtm