Handle SkStream::rewind properly.

Handle SkStream::rewind properly.

include/core/SkStream.h:
Update documentation to state that rewinding a stream at the beginning
should return true. This is important because our decoders fail if
rewind returns false, assuming that the stream is not at the beginning.

src/images/SkImageDecoder_libpng.cpp:
If rewind fails, call png_error.

src/images/SkImageDecoder_libwebp.cpp:
If rewind fails, report an error and return false.

src/images/SkImageRef.cpp:
If rewind fails report an error and return false.
FIXME: Need to handle flattening properly. Should I perhaps move
writeStream into SkOrderedWriteBuffer?

src/images/SkJpegUtility.cpp:
Report a jpeg error on failure to rewind.

BUG=https://b.corp.google.com/issue?id=8432093
R=bungeman@google.com, djsollen@google.com, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=10977

[djsollen, 2013-08-22 19:59:17]

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

https://codereview.chromium.org/22861028/diff/1/include/core/SkStream.h#newco...
include/core/SkStream.h:100: *  Return true if the stream is already at the
beginning.
that is misleading for subclasses that don't override this method.

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libwebp.cpp:203: SkDEBUGF(("Failed to rewind webp
stream!"));
should we always print this?

[bungeman-skia, 2013-08-22 20:19:00]

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

https://codereview.chromium.org/22861028/diff/1/include/core/SkStream.h#newco...
include/core/SkStream.h:100: *  Return true if the stream is already at the
beginning.
On 2013/08/22 19:59:18, djsollen wrote:
> that is misleading for subclasses that don't override this method.

SkStreamRewindable makes this pure virtual so that such subclasses must override
this method. Though I see your point if no bytes have been read from the stream
yet then 'rewind' should return true. However, I'm not sure that all SkStreams
know this information. I suppose in some sense they must (in order to know what
to return from the next 'read').

I believe that there are currently no 'bare' SkStream implementations outside of
Android. I think this is implemented here and not pure virtual (like read) both
so that it could be added without breaking existing SkStream implementations and
because the thought was that if it did anything other than return false it would
be implemented fully. If all of the Android SkStreams get updated then this
could be made pure virtual, I suppose, with a note that SkStreams must
'partially' implement it. (ie, change this to 'Must return true if...')

[bungeman-skia, 2013-08-22 20:42:45]

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

https://codereview.chromium.org/22861028/diff/1/include/core/SkStream.h#newco...
include/core/SkStream.h:100: *  Return true if the stream is already at the
beginning.
On 2013/08/22 20:19:00, bungeman1 wrote:
> On 2013/08/22 19:59:18, djsollen wrote:
> > that is misleading for subclasses that don't override this method.
> 
> SkStreamRewindable makes this pure virtual so that such subclasses must
override
> this method. Though I see your point if no bytes have been read from the
stream
> yet then 'rewind' should return true. However, I'm not sure that all SkStreams
> know this information. I suppose in some sense they must (in order to know
what
> to return from the next 'read').

Actually, an SkStream which is just wrapping stdin doesn't already know this
information. It would need an extra flag which would be set on the first read so
that it would know it was 'dirty'. I suppose requiring an extra flag on such
streams isn't all that bad.

[djsollen, 2013-08-23 12:15:13]

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

https://codereview.chromium.org/22861028/diff/1/include/core/SkStream.h#newco...
include/core/SkStream.h:100: *  Return true if the stream is already at the
beginning.
I wasn't proposing that we change the behavior in this CL, just that the comment
was misleading.

[scroggo, 2013-08-23 21:14:07]

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

https://codereview.chromium.org/22861028/diff/1/include/core/SkStream.h#newco...
include/core/SkStream.h:100: *  Return true if the stream is already at the
beginning.
On 2013/08/23 12:15:13, djsollen wrote:
> I wasn't proposing that we change the behavior in this CL, just that the
comment
> was misleading.

New comment for your judgment:
Streams are expected to either return false always, if rewinding is not
supported, or rewind to the beginning and return true always, regardless of the
current position.

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libwebp.cpp:203: SkDEBUGF(("Failed to rewind webp
stream!"));
On 2013/08/22 19:59:18, djsollen wrote:
> should we always print this?

Any reason not to? This will tell the caller that they failed due to problems
rewinding the stream, rather than bad data, for example.

[djsollen, 2013-08-25 17:47:19]

lgtm

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libwebp.cpp:203: SkDEBUGF(("Failed to rewind webp
stream!"));
On 2013/08/23 21:14:07, scroggo wrote:
> On 2013/08/22 19:59:18, djsollen wrote:
> > should we always print this?
> 
> Any reason not to? This will tell the caller that they failed due to problems
> rewinding the stream, rather than bad data, for example.

I agree that we should print this always.   That is why I would use SkDebugf
instead of the debug only SkDEBUGF.

[scroggo, 2013-08-26 16:37:46]

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/22861028/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libwebp.cpp:203: SkDEBUGF(("Failed to rewind webp
stream!"));
On 2013/08/25 17:47:19, djsollen wrote:
> On 2013/08/23 21:14:07, scroggo wrote:
> > On 2013/08/22 19:59:18, djsollen wrote:
> > > should we always print this?
> > 
> > Any reason not to? This will tell the caller that they failed due to
problems
> > rewinding the stream, rather than bad data, for example.
> 
> I agree that we should print this always.   That is why I would use SkDebugf
> instead of the debug only SkDEBUGF.

Ah, now I understand your comment. I didn't know the difference between SkDebugf
and SkDEBUGF. I used SkDEBUGF because it was used by
SkImageDecoder_FactoryRegistrar.

Done.

[djsollen, 2013-08-26 19:17:36]

lgtm

[scroggo, 2013-08-26 20:44:31]

New patch, new comment for rewind()

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

https://codereview.chromium.org/22861028/diff/24001/include/core/SkStream.h#n...
include/core/SkStream.h:100: *  and after this call the stream is at the
beginning.
Per discussion with Ben, I modified this comment to allow streams that fail to
rewind sometimes.

The goal here is to prevent a stream implementation from saying "I failed to
rewind because I was already at the beginning." I further stated that the stream
must be rewindable so that non-rewindable streams need not return true when they
are at the beginning.

[bungeman-skia, 2013-08-26 22:24:58]

I'm language lawyared out.

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

https://codereview.chromium.org/22861028/diff/23006/include/core/SkStream.h#n...
include/core/SkStream.h:100: *  is NOT at the beginning.
I like this, but it means that this default implementation doesn't satisfy the
requirements. Maybe

Rewinds to the beginning of the stream. The current position is not affected
when false is returned.

Returns false iff one or both of the following is true:
  * the stream is known to be not at the beginning upon return
  * the stream is not known to be at the beginning upon return

[scroggo, 2013-08-27 19:49:26]

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

https://codereview.chromium.org/22861028/diff/23006/include/core/SkStream.h#n...
include/core/SkStream.h:100: *  is NOT at the beginning.
On 2013/08/26 22:24:58, bungeman1 wrote:
> I like this, but it means that this default implementation doesn't satisfy the
> requirements. Maybe
> 
> Rewinds to the beginning of the stream. The current position is not affected
> when false is returned.
> 
> Returns false iff one or both of the following is true:
>   * the stream is known to be not at the beginning upon return
>   * the stream is not known to be at the beginning upon return

I modified these because a slight movement of the word 'not' might be confusing
(especially for someone who doesn't speak English as their first language). I
also don't think they can both be true. Here's what I wrote:

Returns false iff after this call one of the following is true:
 * The stream is known to be somewhere other than the beginning.
 * It is unknown whether the stream is at the beginning.

Let me know what you think.

[bungeman-skia, 2013-08-27 19:55:17]

lgtm

[scroggo, 2013-08-27 19:58:34]

Mike, can you do an API review?

[reed1, 2013-08-27 20:00:36]

I find the comment above rewind() very lawyerly, and not "easy" to grok.

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

https://codereview.chromium.org/22861028/diff/35001/include/core/SkStream.h#n...
include/core/SkStream.h:104: *      * It is unknown whether the stream is at the
beginning.
Are both of these iff clauses true at the same times as "the position is not
affected" is true?

[scroggo, 2013-08-27 20:12:55]

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

https://codereview.chromium.org/22861028/diff/35001/include/core/SkStream.h#n...
include/core/SkStream.h:104: *      * It is unknown whether the stream is at the
beginning.
On 2013/08/27 20:00:37, reed1 wrote:
> Are both of these iff clauses true at the same times as "the position is not
> affected" is true?

No. An implementation could return false if the stream was already at the
beginning - this would satisfy the condition of the initial statement, but not
the iff clause.

Our decoders now fail if rewind returned false, so I want to make it clear to
anyone writing an implementation that they should not return false if the stream
was already at the beginning (which should not cause a failure to decode). The
previous comment leaves open to interpretation what it means to be able to
rewind, IMO.

[scroggo, 2013-08-27 20:37:25]

Please see the latest comment.

[reed1, 2013-08-27 21:33:12]

lgtm

[scroggo, 2013-08-28 13:09:01]

Committed patchset #8 manually as r10977 (presubmit successful).