Make SkCodec support peek() and read()

Make SkCodec support peek() and read()

- Update SkCodec's dox to point out that some of the data must be
  read twice in order to decode
- Add an accessor that reports how much is needed for reading twice
- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
  - NOTE: SkWbmpCodec needs the following number of bytes for the header
    + 1 (type)
    + 1 (reserved)
    + 3 (width - bytes needed to support up to 0xFFFF)
    + 3 (height - bytes needed to support up to 0xFFFF)
    = 8
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

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

[scroggo, 2015-11-24 19:00:01]

Description was changed from

==========
Make SkCodec support peek() and read()

- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
- in SkWbmpCodec, make read_header less restrictive. SkWbmpCodec
  previously would have rejected some images that SkWbmpImageDecoder
  would have supported. (FIXME: Add tests!)
- make SkWbmpCodec::IsWbmp less restrictive. This change was just for
  simplicity. Not sure how many bytes exactly the header needs; I could
  instead read enough for the max size, which should fit inside 14
  bytes. FIXME: determine how much to read
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

BUG=skia:3257
==========

to

==========
Make SkCodec support peek() and read()

- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
- in SkWbmpCodec, make read_header less restrictive. SkWbmpCodec
  previously would have rejected some images that SkWbmpImageDecoder
  would have supported. (FIXME: Add tests!)
- make SkWbmpCodec::IsWbmp less restrictive. This change was just for
  simplicity. Not sure how many bytes exactly the header needs; I could
  instead read enough for the max size, which should fit inside 14
  bytes. FIXME: determine how much to read
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

[scroggo, 2015-11-25 19:12:29]

Description was changed from

==========
Make SkCodec support peek() and read()

- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
- in SkWbmpCodec, make read_header less restrictive. SkWbmpCodec
  previously would have rejected some images that SkWbmpImageDecoder
  would have supported. (FIXME: Add tests!)
- make SkWbmpCodec::IsWbmp less restrictive. This change was just for
  simplicity. Not sure how many bytes exactly the header needs; I could
  instead read enough for the max size, which should fit inside 14
  bytes. FIXME: determine how much to read
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

to

==========
Make SkCodec support peek() and read()

- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
- make SkWbmpCodec::IsWbmp less restrictive. This change was just for
  simplicity. Not sure how many bytes exactly the header needs; I could
  instead read enough for the max size, which should fit inside 14
  bytes. FIXME: determine how much to read
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

[scroggo, 2015-11-25 22:00:16]

Description was changed from

==========
Make SkCodec support peek() and read()

- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
- make SkWbmpCodec::IsWbmp less restrictive. This change was just for
  simplicity. Not sure how many bytes exactly the header needs; I could
  instead read enough for the max size, which should fit inside 14
  bytes. FIXME: determine how much to read
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

to

==========
Make SkCodec support peek() and read()

- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
  - NOTE: SkWbmpCodec needs the following number of bytes for the header
    + 1 (type)
    + 1 (reserved)
    + 3 (width - bytes needed to support up to 0xFFFF)
    + 3 (height - bytes needed to support up to 0xFFFF)
    = 8
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

[scroggo, 2015-11-25 22:08:34]

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

[scroggo, 2015-11-25 22:08:35]

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:46: // 14 is enough to read all of the supported types.
I've convinced myself that 14 bytes is enough to support the largest wbmp we
support, but have failed at writing a test to prove it so.

[msarett, 2015-11-30 14:08:11]

lgtm

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:46: // 14 is enough to read all of the supported types.
On 2015/11/25 22:08:35, scroggo wrote:
> I've convinced myself that 14 bytes is enough to support the largest wbmp we
> support, but have failed at writing a test to prove it so.

This matches my understanding as well.  Looks like 1 + 1 + 3 (width) + 3
(height) is the most we should need.

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:54: // fail, even though we read enough?)
Good question.  I don't know, but I would guess that there is.  Maybe a 1x1
wbmp?

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec_wbmp.cpp
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:69: if (!read_mbf(stream, &width) || width > 0xFFFF
|| !width) {
read_header is called by IsWbmp and NewFromStream.

We could consider not even calculating the width and height for IsWbmp.  We
would just need to surround this entire block by:
if (size) {
  // width, height code
}

I think this might be a bad idea because wbmp doesn't have much of a signature. 
There is probably a risk of non-wbmps being considered wbmps.

[scroggo, 2015-11-30 20:02:31]

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:46: // 14 is enough to read all of the supported types.
On 2015/11/30 14:08:11, msarett wrote:
> On 2015/11/25 22:08:35, scroggo wrote:
> > I've convinced myself that 14 bytes is enough to support the largest wbmp we
> > support, but have failed at writing a test to prove it so.
> 
> This matches my understanding as well.  Looks like 1 + 1 + 3 (width) + 3
> (height) is the most we should need.

Added a test.

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:54: // fail, even though we read enough?)
On 2015/11/30 14:08:11, msarett wrote:
> Good question.  I don't know, but I would guess that there is.  Maybe a 1x1
> wbmp?

Yeah, that should do it:

1 + 1 (headers) + 1 (width) + 1 (height) + 1 (1 bit, padded to a byte). I'll
update the comment.

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec_wbmp.cpp
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1472123002/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:69: if (!read_mbf(stream, &width) || width > 0xFFFF
|| !width) {
On 2015/11/30 14:08:11, msarett wrote:
> read_header is called by IsWbmp and NewFromStream.
> 
> We could consider not even calculating the width and height for IsWbmp.  We
> would just need to surround this entire block by:
> if (size) {
>   // width, height code
> }
> 
> I think this might be a bad idea because wbmp doesn't have much of a
signature. 
> There is probably a risk of non-wbmps being considered wbmps.

Yeah, I considered something like that. You're correct; we might consider
non-wbmps to be wbmps, but only initially. We would then call read_header again
in the constructor, at which point we would realize it's not a wbmp. The only
way I think this would really be a problem is if wbmp were not the last one in
the list we check, and any following format might match something we mistakenly
thought was a wbmp. For example, we may add RAW/DNG to the end (that said, it is
possible it should come before wbmp). I don't know whether an image whose first
two bytes look like a wbmp could also look like a RAW/DNG, but the extra checks
make us more certain, and shouldn't take too long.

https://codereview.chromium.org/1472123002/diff/60001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/60001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:55: // less than 14 bytes.
Some thoughts on the overall goal here:

We need to be able to somehow read the first few bytes, but then pass the entire
stream to the correct decoder. Originally we did this by calling rewind() on the
SkStream. This meant calling reset() on the Java InputStream, which may fail.
Worse, we have to call mark() on it first in order to have a place to "reset"
to. mark() takes an int parameter which basically says "buffer up to this size".
At one point, we called mark() on the passed in stream, with an arbitrary size
(one we thought was big enough). This could be a problem if someone else called
mark - we've overridden their mark, potentially with a smaller buffer.

Since we really only need to buffer the first X bytes, we solved this problem
with the SkFrontBufferedStream. It buffers the first X bytes (passed in by the
caller), allowing rewinding from within that range, but not after that (which is
never necessary).

We currently set X to 64, because the way SkWEBPImageDecoder was written, it
required reading 64 bytes to determine whether it was a webp (only 14 are
needed, as noted here), *and* because it has to read enough to know the size of
the image to allocate before starting the decode. (FWIW, SkWEBPImageDecoder
reads 64 bytes to determine this, but we've since learned that only 30 bytes are
needed.)

Regardless of the value of X, this seems backwards; BitmapFactory.cpp needs to
know how much SkImageDecoder needs to buffer in order to create its buffer.

The idea behind peek() is that BitmapFactory need not wrap its stream in an
SkFrontBufferedStream (using a buffer that it knows happens to be large enough).
Instead, SkCodec peeks at the bytes it knows it needs (to be fair, in this CL
I've sort of pushed the intimate knowledge just one level deeper; SkCodec
happens to know how many bytes each of its subclasses needs - I took this
approach because it seemed simpler, and I think it makes sense for SkCodec to
know about its subclasses).

peek() is easy on an SkMemoryStream, but for Android to take advantage of it it
also needs to be supported on JavaInputStreamAdaptor. This class already buffers
so that it can convert from Java bytes to a C buffer. My thought that we could
repurpose this buffer for peeking. This means we are limited to peeking the size
of the Java byte buffer. The byte buffer is provided by the caller, so in theory
it could be too small. In practice, the API (see
http://developer.android.com/reference/android/graphics/BitmapFactory.Options...)
recommends 16 KB (which is also the default), so this shouldn't be an issue.

The trickier part comes from very small images. You proposed a small wbmp, which
I agree is a possibility, and I could imagine there might be others. In these
cases, using read + rewind does not seem like a good fallback, since it means we
have to buffer anyway (defeating the purpose of peek()).

I could fix this by peeking inside the subclass - i.e. make IsWbmp (etc) take a
stream (like it/they do without this CL) and call peek() (this also eliminates
SkCodec's intimate knowledge). This eliminates the problem where peek failed
because the stream was smaller (since e.g. SkWbmpCodec will only peek as much as
it needs, although it will be messy, since it will need to peek a larger amount
each time to include what was already peeked plus new data). 

A downside is that if we want to continue to support reading + rewinding as a
fallback (which should no longer be a problem, since we've addressed the problem
of not having peeked enough), IsWbmp (etc) needs to report two things:
- did the stream match my format
- did it call read() (caller must rewind()) OR rewind() failed (caller must
abort)

Not terrible, but a little bit messier... (this was in fact how I started
writing this CL, but I switched to the current version which seemed cleaner).

I'll start working on that version. Let me know if you have any thoughts about
this.

[scroggo, 2015-12-01 22:33:49]

Please take another look.

We should probably update the header's comments, too, describing what to expect
regarding peeking/rewinding.

https://codereview.chromium.org/1472123002/diff/60001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/60001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:55: // less than 14 bytes.
On 2015/11/30 20:02:30, scroggo wrote:
> Some thoughts on the overall goal here:
> 
> We need to be able to somehow read the first few bytes, but then pass the
entire
> stream to the correct decoder. Originally we did this by calling rewind() on
the
> SkStream. This meant calling reset() on the Java InputStream, which may fail.
> Worse, we have to call mark() on it first in order to have a place to "reset"
> to. mark() takes an int parameter which basically says "buffer up to this
size".
> At one point, we called mark() on the passed in stream, with an arbitrary size
> (one we thought was big enough). This could be a problem if someone else
called
> mark - we've overridden their mark, potentially with a smaller buffer.
> 
> Since we really only need to buffer the first X bytes, we solved this problem
> with the SkFrontBufferedStream. It buffers the first X bytes (passed in by the
> caller), allowing rewinding from within that range, but not after that (which
is
> never necessary).
> 
> We currently set X to 64, because the way SkWEBPImageDecoder was written, it
> required reading 64 bytes to determine whether it was a webp (only 14 are
> needed, as noted here), *and* because it has to read enough to know the size
of
> the image to allocate before starting the decode. (FWIW, SkWEBPImageDecoder
> reads 64 bytes to determine this, but we've since learned that only 30 bytes
are
> needed.)
> 
> Regardless of the value of X, this seems backwards; BitmapFactory.cpp needs to
> know how much SkImageDecoder needs to buffer in order to create its buffer.
> 
> The idea behind peek() is that BitmapFactory need not wrap its stream in an
> SkFrontBufferedStream (using a buffer that it knows happens to be large
enough).
> Instead, SkCodec peeks at the bytes it knows it needs (to be fair, in this CL
> I've sort of pushed the intimate knowledge just one level deeper; SkCodec
> happens to know how many bytes each of its subclasses needs - I took this
> approach because it seemed simpler, and I think it makes sense for SkCodec to
> know about its subclasses).
> 
> peek() is easy on an SkMemoryStream, but for Android to take advantage of it
it
> also needs to be supported on JavaInputStreamAdaptor. This class already
buffers
> so that it can convert from Java bytes to a C buffer. My thought that we could
> repurpose this buffer for peeking. This means we are limited to peeking the
size
> of the Java byte buffer. The byte buffer is provided by the caller, so in
theory
> it could be too small. In practice, the API (see
>
http://developer.android.com/reference/android/graphics/BitmapFactory.Options...)
> recommends 16 KB (which is also the default), so this shouldn't be an issue.
> 
> The trickier part comes from very small images. You proposed a small wbmp,
which
> I agree is a possibility, and I could imagine there might be others. In these
> cases, using read + rewind does not seem like a good fallback, since it means
we
> have to buffer anyway (defeating the purpose of peek()).
> 
> I could fix this by peeking inside the subclass - i.e. make IsWbmp (etc) take
a
> stream (like it/they do without this CL) and call peek() (this also eliminates
> SkCodec's intimate knowledge). This eliminates the problem where peek failed
> because the stream was smaller (since e.g. SkWbmpCodec will only peek as much
as
> it needs, although it will be messy, since it will need to peek a larger
amount
> each time to include what was already peeked plus new data). 
> 
> A downside is that if we want to continue to support reading + rewinding as a
> fallback (which should no longer be a problem, since we've addressed the
problem
> of not having peeked enough), IsWbmp (etc) needs to report two things:
> - did the stream match my format
> - did it call read() (caller must rewind()) OR rewind() failed (caller must
> abort)
> 
> Not terrible, but a little bit messier... (this was in fact how I started
> writing this CL, but I switched to the current version which seemed cleaner).
> 
> I'll start working on that version. Let me know if you have any thoughts about
> this.

You've convinced me to do what I should've done all along - make peek() return a
size_t. That simplifies this solution. PTAL.

https://codereview.chromium.org/1472123002/diff/100001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/100001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:38: if (bytesPeeked != WEBP_VP8_HEADER_SIZE) {
I should probably add a test for this. I forgot to fix this the first time, and
it didn't affect anything.

Not sure what the best behavior is here; maybe if it's > 0, but <
WEBP_VP8_HEADER_SIZE, I should assume (like SkCodec does) that the stream is
less than WEBP_VP8_HEADER_SIZE, which would be a failure.

[scroggo, 2015-12-04 22:14:48]

Description was changed from

==========
Make SkCodec support peek() and read()

- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
  - NOTE: SkWbmpCodec needs the following number of bytes for the header
    + 1 (type)
    + 1 (reserved)
    + 3 (width - bytes needed to support up to 0xFFFF)
    + 3 (height - bytes needed to support up to 0xFFFF)
    = 8
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

to

==========
Make SkCodec support peek() and read()

- Update SkCodec's dox to point out that some of the data must be
  read twice in order to decode
- Add an accessor that reports how much is needed for reading twice
- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
  - NOTE: SkWbmpCodec needs the following number of bytes for the header
    + 1 (type)
    + 1 (reserved)
    + 3 (width - bytes needed to support up to 0xFFFF)
    + 3 (height - bytes needed to support up to 0xFFFF)
    = 8
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

[scroggo, 2015-12-04 22:15:04]

On 2015/12/01 22:33:49, scroggo wrote:
> Please take another look.
> 
> We should probably update the header's comments, too, describing what to
expect
> regarding peeking/rewinding.

Done.

Please take a look.

https://codereview.chromium.org/1472123002/diff/100001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/100001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:38: if (bytesPeeked != WEBP_VP8_HEADER_SIZE) {
On 2015/12/01 22:33:49, scroggo wrote:
> I should probably add a test for this. I forgot to fix this the first time,
and
> it didn't affect anything.
> 
> Not sure what the best behavior is here; maybe if it's > 0, but <
> WEBP_VP8_HEADER_SIZE, I should assume (like SkCodec does) that the stream is
> less than WEBP_VP8_HEADER_SIZE, which would be a failure.

Added tests for this.

[msarett, 2015-12-07 15:29:36]

lgtm

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:52: SkASSERT(bytesToRead <= BufferedBytesNeeded());
So we only need 14 bytes to determine the image type, but we require that 30
bytes be buffered so that webp_parse_header() can work correctly?

I think that is fine, 30 bytes is not very much.

[scroggo, 2015-12-07 16:06:28]

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:52: SkASSERT(bytesToRead <= BufferedBytesNeeded());
On 2015/12/07 15:29:36, msarett wrote:
> So we only need 14 bytes to determine the image type, but we require that 30
> bytes be buffered so that webp_parse_header() can work correctly?

That's correct. I almost called that out, and mentioned that we would only need
30 if it's a webp, but to be fair, we only need 14 for webp, too (the other
signatures are all shorter).

> 
> I think that is fine, 30 bytes is not very much.

[scroggo, 2015-12-07 21:46:20]

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

[scroggo, 2015-12-07 21:46:21]

Mike, can you take a look at the API?

[reed1, 2015-12-08 15:04:58]

lgtm

https://codereview.chromium.org/1472123002/diff/120001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1472123002/diff/120001/include/codec/SkCodec....
include/codec/SkCodec.h:39: static size_t BufferedBytesNeeded();
MinBufferedBytesNeeded?

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:23: static bool IsBmp(const char*, size_t);
const void* ?

In general, skia always uses void* instead of char* for blind memory

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.h (right):

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.h:22: static bool IsGif(const char*, size_t);
const void* ?

[scroggo, 2015-12-08 18:35:44]

https://codereview.chromium.org/1472123002/diff/120001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1472123002/diff/120001/include/codec/SkCodec....
include/codec/SkCodec.h:39: static size_t BufferedBytesNeeded();
On 2015/12/08 15:04:57, reed1 wrote:
> MinBufferedBytesNeeded?

Done.

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkBmpCodec.h...
src/codec/SkBmpCodec.h:23: static bool IsBmp(const char*, size_t);
On 2015/12/08 15:04:57, reed1 wrote:
> const void* ?
> 
> In general, skia always uses void* instead of char* for blind memory

Done.

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.h (right):

https://codereview.chromium.org/1472123002/diff/120001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.h:22: static bool IsGif(const char*, size_t);
On 2015/12/08 15:04:57, reed1 wrote:
> const void* ?

Done.

[scroggo, 2015-12-08 18:35:50]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-12-08 18:35:51]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1472123002/#ps140001
(title: "Respond to reed's comments in patch set 7")

[commit-bot: I haz the power, 2015-12-08 18:36:04]

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

[commit-bot: I haz the power, 2015-12-08 18:36:48]

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

[commit-bot: I haz the power, 2015-12-08 18:36:49]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-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-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[scroggo, 2015-12-08 21:35:58]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-12-08 21:35:58]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1472123002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-08 21:36:06]

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

[commit-bot: I haz the power, 2015-12-09 02:54:15]

Description was changed from

==========
Make SkCodec support peek() and read()

- Update SkCodec's dox to point out that some of the data must be
  read twice in order to decode
- Add an accessor that reports how much is needed for reading twice
- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
  - NOTE: SkWbmpCodec needs the following number of bytes for the header
    + 1 (type)
    + 1 (reserved)
    + 3 (width - bytes needed to support up to 0xFFFF)
    + 3 (height - bytes needed to support up to 0xFFFF)
    = 8
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257
==========

to

==========
Make SkCodec support peek() and read()

- Update SkCodec's dox to point out that some of the data must be
  read twice in order to decode
- Add an accessor that reports how much is needed for reading twice
- Make SkCodec default to use peek()
  If an input stream supports peek()ing, peek() instead of reading.
  This way the stream need not implement rewind()
- Make SkCodec use read() + rewind() as a backup
  So that streams without peek() implemented can still function
  properly (assuming they can rewind).
- read everything we may need to determine the format once
  In SkCodec::NewFromStream, peek()/read() 14 bytes, which is enough
  to read all of the types we support. Pass the buffer to each subtype,
  which will have enough info to determine whether it is the right
  type. This simplifies the code and results in less reading and
  rewinding.
  - NOTE: SkWbmpCodec needs the following number of bytes for the header
    + 1 (type)
    + 1 (reserved)
    + 3 (width - bytes needed to support up to 0xFFFF)
    + 3 (height - bytes needed to support up to 0xFFFF)
    = 8
- in SkWebpCodec, support using read + rewind as a backup if peek does
  not work.

A change in Android will add peek() to JavaInputStreamAdapter.

BUG=skia:3257

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

[commit-bot: I haz the power, 2015-12-09 02:54:18]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/db30be2f9470d21fe37b63d145c1fcca9a6ad98c