Add support for multiple frames in SkCodec

Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new fields to SkCodec::Options:
- fFrameIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.fFrameIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fFrameIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
https://docs.google.com/a/google.com/document/d/12Qhf9T92MWfdWujQwCIjhCO3sw6pTJB5pJBwDM1T7Kc/

[1] https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZbchjlTC5EIz6HFy-6RI/

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

Committed: https://skia.googlesource.com/skia/+/19b91531e912283d237435d94516575b28713cba

[scroggo_chromium, 2016-06-08 14:30:51]

Description was changed from

==========
WIP: Support multiple frames in SkGifCodec

Experiment with an interface to decode frames beyond the first in
SkGifCodec.

Add a new API to SkCodec for retrieving information about a
particular frame. This API is intended to support progressive
decoding (although the current implementation does not decode
progressively - it decodes the whole image from the start). As such,
it does not allow querying for the number of frames, which can only
be determined by decoding the entire stream. Instead, it allows
requesting the i'th frame, under the assumption that the client
will request each frame sequentially. When the client reaches the
end, they will go back to the beginning.

The API does not do any compositing - each frame is returned
individually. If a frame needs to be blended with a previous frame,
it is up to the caller to do so. This allows the SkGifCodec to
continue to support index 8; if it handled blending it might be
required to blend frames that together have more than 256 colors.
It also means that the SkGifCodec need not cache previous frames.
(In practice, the current implementation relies on DGifSlurp, and
stores the resulting information, including the preswizzled pixels.
But the API does not require that.)

For simplicity, this removes scanline decoding (and therefore
sampling) and modifies the test to not test for it. I don't see any
reason this API cannot handle scanline decoding, but it will add
complexity that I'd rather hold off until we are satisfied with this
API (/whatever it evolves into).

The implementation should likely be replaced with Chromium's, which
is much better tested than mine, and also supports progressive
decoding. The current implementation *may* fail to produce partial
images/first frames if there is an error later in the stream; I am
not sure how DGifSlurp handles errors.

Add a GM that animates a GIF

This still does not do blending correctly, but it's more or less
a proof of concept. Next step will be to blend properly.

TODO: Support scanline decoding
TODO: Support progressive decoding
TODO: Test getting successive frames
TODO: Write an SkDrawable which uses it.
==========

to

==========
WIP: Support multiple frames in SkGifCodec

Experiment with an interface to decode frames beyond the first in
SkGifCodec.

Add a new API to SkCodec for retrieving information about a
particular frame. This API is intended to support progressive
decoding (although the current implementation does not decode
progressively - it decodes the whole image from the start). As such,
it does not allow querying for the number of frames, which can only
be determined by decoding the entire stream. Instead, it allows
requesting the i'th frame, under the assumption that the client
will request each frame sequentially. When the client reaches the
end, they will go back to the beginning.

The API does not do any compositing - each frame is returned
individually. If a frame needs to be blended with a previous frame,
it is up to the caller to do so. This allows the SkGifCodec to
continue to support index 8; if it handled blending it might be
required to blend frames that together have more than 256 colors.
It also means that the SkGifCodec need not cache previous frames.
(In practice, the current implementation relies on DGifSlurp, and
stores the resulting information, including the preswizzled pixels.
But the API does not require that.)

For simplicity, this removes scanline decoding (and therefore
sampling) and modifies the test to not test for it. I don't see any
reason this API cannot handle scanline decoding, but it will add
complexity that I'd rather hold off until we are satisfied with this
API (/whatever it evolves into).

The implementation should likely be replaced with Chromium's, which
is much better tested than mine, and also supports progressive
decoding. The current implementation *may* fail to produce partial
images/first frames if there is an error later in the stream; I am
not sure how DGifSlurp handles errors.

Add a GM that animates a GIF

This still does not do blending correctly, but it's more or less
a proof of concept. Next step will be to blend properly.

TODO: Support scanline decoding
TODO: Support progressive decoding
TODO: Test getting successive frames
TODO: Write an SkDrawable which uses it.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[scroggo_chromium, 2016-09-21 19:27:16]

Description was changed from

==========
WIP: Support multiple frames in SkGifCodec

Experiment with an interface to decode frames beyond the first in
SkGifCodec.

Add a new API to SkCodec for retrieving information about a
particular frame. This API is intended to support progressive
decoding (although the current implementation does not decode
progressively - it decodes the whole image from the start). As such,
it does not allow querying for the number of frames, which can only
be determined by decoding the entire stream. Instead, it allows
requesting the i'th frame, under the assumption that the client
will request each frame sequentially. When the client reaches the
end, they will go back to the beginning.

The API does not do any compositing - each frame is returned
individually. If a frame needs to be blended with a previous frame,
it is up to the caller to do so. This allows the SkGifCodec to
continue to support index 8; if it handled blending it might be
required to blend frames that together have more than 256 colors.
It also means that the SkGifCodec need not cache previous frames.
(In practice, the current implementation relies on DGifSlurp, and
stores the resulting information, including the preswizzled pixels.
But the API does not require that.)

For simplicity, this removes scanline decoding (and therefore
sampling) and modifies the test to not test for it. I don't see any
reason this API cannot handle scanline decoding, but it will add
complexity that I'd rather hold off until we are satisfied with this
API (/whatever it evolves into).

The implementation should likely be replaced with Chromium's, which
is much better tested than mine, and also supports progressive
decoding. The current implementation *may* fail to produce partial
images/first frames if there is an error later in the stream; I am
not sure how DGifSlurp handles errors.

Add a GM that animates a GIF

This still does not do blending correctly, but it's more or less
a proof of concept. Next step will be to blend properly.

TODO: Support scanline decoding
TODO: Support progressive decoding
TODO: Test getting successive frames
TODO: Write an SkDrawable which uses it.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
WIP: Support multiple frames in SkGifCodec

Experiment with an interface to decode frames beyond the first in
SkGifCodec.

Add the following new methods to SkCodec:
- getFrameCount
- getRequiredFrame
- getFrameDuration

Add the following fields on SkCodec::Options:
- fFrameIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.fFrameIndex. If the client does not have the frame's
required frame (the frame that this frame must be blended on top
of) cached, they pass false for Options.fHasPriorFrame. Unless
the frame is independent, the codec will then recursively decode
all frames necessary to decode fFrameIndex. If the client has the
required frame cached, they can put it in the dst they pass to
the codec, and the codec will only draw fFrameIndex onto it.

This API is intended to support progressive decoding (although
the current implementation does not decode progressively - it
decodes the whole image from the start).

For simplicity, this removes scanline decoding (and therefore
sampling) and modifies the test to not test for it. Scanline decoding
is being phased out anyway, so before this lands it will be
replaced with progressive decoding.

The implementation should likely be replaced with Chromium's, which
is much better tested than mine, and already supports progressive
decoding. The current implementation *may* fail to produce partial
images/first frames if there is an error later in the stream; I am
not sure how DGifSlurp handles errors.

Add a GM that animates a GIF.
Add a test for the new APIs.

TODO: Support progressive decoding
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[scroggo_chromium, 2016-09-21 19:30:28]

scroggo@chromium.org changed reviewers:
+ cblume@google.com, joostouwerling@google.com, msarett@google.com,
reed@google.com

[scroggo_chromium, 2016-09-21 19:30:28]

Here's a first draft of an API for SkCodec that supports animation while leaving
caching up to the client. PTAL and let me know what you think. The test and GM
demonstrate how it will be used.

(Probably no need to get bogged down in the details of the implementation, which
will drastically change. I just used DGifSlurp in gif_lib to demonstrate the
API.)

[msarett, 2016-09-21 22:23:48]

Just looked at the API.

Very exciting!  I wrote down a bunch of nits that are semi-related to this
change.  But I feel good about the direction.

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:245: : fZeroInitialized(kNo_ZeroInitialized)
I dislike having this option.  Obviously unrelated to your CL, but I'd like to
think about if we can delete it.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:247: , fFrameIndex(0)
I think I might prefer it if all of the "multi-frame" stuff was kept together
somehow - maybe a struct inside this struct?

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:249: {}
While I'm nitting on the Options object, here's another thought.  This seems to
hold all kinds of specialized things (which I think is necessary/fine) - but it
makes me think that the colorPtr and colorCountPtr also belong here (since they
are also specialized things).

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
Can we get away with not having this?  Seems like it allows the client to make
us do a bunch of extra work up front.  Maybe it's something they need to know
though...

[cblume, 2016-09-22 01:08:46]

cblume@chromium.org changed reviewers:
+ cblume@chromium.org

[cblume, 2016-09-22 01:08:47]

I'm also very excited about this. This is great!

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:293: bool            fHasPriorFrame;
I am curious about the design of this.

As I understand it, dst is an in/out param when fHasPriorFrame is true. So I
assume it will overwrite the contents.

However, gif has a dispose method which is effectively "display and discard". To
give an example, suppose:
frame 8 is our keyframe
frame 9 depends on 8, has display and discard
--So we clobber the contents of 8 with the contents of 9
frame 10 depends on 8, since 9 was discarded
--But we clobbered frame 8...

To get around this, we would have to make a copy of our keyframe.

This might be fine.

We have no way of knowing how much of the next frame will be clobbered so we
could end up doing a large copy just to replace most of it. In that case, it
might be better to have separate in and out params and let the decoder do the
copy as needed.

However, maybe the copy is fast enough that it would be better to copy and
clobber anyway.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/21 22:23:48, msarett wrote:
> Can we get away with not having this?  Seems like it allows the client to make
> us do a bunch of extra work up front.  Maybe it's something they need to know
> though...

In the case of gif and apng, we won't know in advance anyway.
If a buffer comes in that isn't the full image, we won't know if there are more
frames coming in the next buffer.

We could simply have this return an error code that specifies it isn't yet
known. But perhaps a better option would be let the client know "Here is how
many frames we are aware of so far. But we don't yet have the entire image
data." Maybe this can be achieved by having a separate bool isImageComplete() or
some such.


That said, from a client perspective it would be very helpful to know frame
count and dependency information prior to actually displaying that frame. Until
I know which frames are valuable to cache, I would be forced to defensively
cache them all.

Pulling out the metadata early would be useful to me.

[cblume, 2016-09-22 01:42:19]

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:293: bool            fHasPriorFrame;
On 2016/09/22 01:08:47, cblume wrote:
> I am curious about the design of this.
> 
> As I understand it, dst is an in/out param when fHasPriorFrame is true. So I
> assume it will overwrite the contents.
> 
> However, gif has a dispose method which is effectively "display and discard".
To
> give an example, suppose:
> frame 8 is our keyframe
> frame 9 depends on 8, has display and discard
> --So we clobber the contents of 8 with the contents of 9
> frame 10 depends on 8, since 9 was discarded
> --But we clobbered frame 8...
> 
> To get around this, we would have to make a copy of our keyframe.
> 
> This might be fine.
> 
> We have no way of knowing how much of the next frame will be clobbered so we
> could end up doing a large copy just to replace most of it. In that case, it
> might be better to have separate in and out params and let the decoder do the
> copy as needed.
> 
> However, maybe the copy is fast enough that it would be better to copy and
> clobber anyway.

Adding to this:
If we are still downloading the image and do not know if frame 8 will be used
again, we need that copy.

But if we have the frame information in advance or are after the first animation
cycle, we may actually know that frame 8 will never be used again. Imagine frame
11 is a new full-image keyframe.

In that case, it would be perhaps be okay to clobber frame 8 if we knew we would
want to purge it afterwords anyway.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/22 01:08:47, cblume wrote:
> On 2016/09/21 22:23:48, msarett wrote:
> > Can we get away with not having this?  Seems like it allows the client to
make
> > us do a bunch of extra work up front.  Maybe it's something they need to
know
> > though...
> 
> In the case of gif and apng, we won't know in advance anyway.
> If a buffer comes in that isn't the full image, we won't know if there are
more
> frames coming in the next buffer.
> 
> We could simply have this return an error code that specifies it isn't yet
> known. But perhaps a better option would be let the client know "Here is how
> many frames we are aware of so far. But we don't yet have the entire image
> data." Maybe this can be achieved by having a separate bool isImageComplete()
or
> some such.
> 
> 
> That said, from a client perspective it would be very helpful to know frame
> count and dependency information prior to actually displaying that frame.
Until
> I know which frames are valuable to cache, I would be forced to defensively
> cache them all.
> 
> Pulling out the metadata early would be useful to me.

Correcting myself:
We could still be wise about which frames we cache as we go. We can't be *as*
wise. But we don't need to defensively cache them all.

If we get the metadata as we go, we can be reasonable about what frames we want
to keep around for next loop through the animation.

[joostouwerling, 2016-09-22 15:43:17]

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/22 01:08:47, cblume wrote:
> On 2016/09/21 22:23:48, msarett wrote:
> > Can we get away with not having this?  Seems like it allows the client to
make
> > us do a bunch of extra work up front.  Maybe it's something they need to
know
> > though...
> 
> In the case of gif and apng, we won't know in advance anyway.
> If a buffer comes in that isn't the full image, we won't know if there are
more
> frames coming in the next buffer.

For apng, the acTL chunk with the number of frames appears before the full image
buffer so it would not be very expensive to implement this (please correct me if
I'm wrong). Does not change the arguments for gif, though.

[cblume, 2016-09-22 16:23:19]

On 2016/09/22 15:43:17, joostouwerling wrote:
> https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h
> File include/codec/SkCodec.h (right):
> 
>
https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
> include/codec/SkCodec.h:603: size_t getFrameCount() {
> On 2016/09/22 01:08:47, cblume wrote:
> > On 2016/09/21 22:23:48, msarett wrote:
> > > Can we get away with not having this?  Seems like it allows the client to
> make
> > > us do a bunch of extra work up front.  Maybe it's something they need to
> know
> > > though...
> > 
> > In the case of gif and apng, we won't know in advance anyway.
> > If a buffer comes in that isn't the full image, we won't know if there are
> more
> > frames coming in the next buffer.
> 
> For apng, the acTL chunk with the number of frames appears before the full
image
> buffer so it would not be very expensive to implement this (please correct me
if
> I'm wrong). Does not change the arguments for gif, though.

Oh sweet! Thank you :D

[scroggo, 2016-09-22 16:46:41]

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

[scroggo, 2016-09-22 16:46:43]

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:245: : fZeroInitialized(kNo_ZeroInitialized)
On 2016/09/21 22:23:48, msarett wrote:
> I dislike having this option.  Obviously unrelated to your CL, but I'd like to
> think about if we can delete it.

It's certainly awkward to have both fZeroInitialized and fHasPriorFrame, since
they're certainly not both true.

But Android relies on this feature to greatly reduce their memory footprint. I
think we're stuck with this until b/11657454 is solved. (Unless Android just
gets so much more memory that it doesn't matter anymore. Although I suspect
devices with more memory will also have higher resolution screens, for which
they'll use even larger assets with more empty space. And I think on AndroidOne
etc we're still targeting devices with lower memory anyway.)

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:247: , fFrameIndex(0)
On 2016/09/21 22:23:48, msarett wrote:
> I think I might prefer it if all of the "multi-frame" stuff was kept together
> somehow - maybe a struct inside this struct?

I considered that, but I'm not sure whether it's better or worse. PS 3 tries it
out. Let me know what you think.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:249: {}
On 2016/09/21 22:23:48, msarett wrote:
> While I'm nitting on the Options object, here's another thought.  This seems
to
> hold all kinds of specialized things (which I think is necessary/fine) - but
it
> makes me think that the colorPtr and colorCountPtr also belong here (since
they
> are also specialized things).

Yeah, we put those in the AndroidOptions object as an experiment, and I think we
liked that way of doing it. We just haven't followed through and done that here.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:293: bool            fHasPriorFrame;
On 2016/09/22 01:42:19, cblume wrote:
> On 2016/09/22 01:08:47, cblume wrote:
> > I am curious about the design of this.
> > 
> > As I understand it, dst is an in/out param when fHasPriorFrame is true. So I
> > assume it will overwrite the contents.
> > 
> > However, gif has a dispose method which is effectively "display and
discard".
> To
> > give an example, suppose:
> > frame 8 is our keyframe
> > frame 9 depends on 8, has display and discard
> > --So we clobber the contents of 8 with the contents of 9
> > frame 10 depends on 8, since 9 was discarded
> > --But we clobbered frame 8...
> > 
> > To get around this, we would have to make a copy of our keyframe.
> > 
> > This might be fine.
> > 
> > We have no way of knowing how much of the next frame will be clobbered so we
> > could end up doing a large copy just to replace most of it. In that case, it
> > might be better to have separate in and out params and let the decoder do
the
> > copy as needed.
> > 
> > However, maybe the copy is fast enough that it would be better to copy and
> > clobber anyway.
> 
> Adding to this:
> If we are still downloading the image and do not know if frame 8 will be used
> again, we need that copy.
> 
> But if we have the frame information in advance or are after the first
animation
> cycle, we may actually know that frame 8 will never be used again. Imagine
frame
> 11 is a new full-image keyframe.
> 
> In that case, it would be perhaps be okay to clobber frame 8 if we knew we
would
> want to purge it afterwords anyway.

Agreed.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:293: bool            fHasPriorFrame;
On 2016/09/22 01:08:47, cblume wrote:
> I am curious about the design of this.
> 
> As I understand it, dst is an in/out param when fHasPriorFrame is true. So I
> assume it will overwrite the contents.

That is correct.

> 
> However, gif has a dispose method which is effectively "display and discard".
To
> give an example, suppose:
> frame 8 is our keyframe
> frame 9 depends on 8, has display and discard
> --So we clobber the contents of 8 with the contents of 9
> frame 10 depends on 8, since 9 was discarded
> --But we clobbered frame 8...
> 
> To get around this, we would have to make a copy of our keyframe.
> 
> This might be fine.

Someone has to make a copy. Either the codec or the client. A goal of this API
is to leave caching decisions up to the client, and I think this falls into that
area. If the client does not want to continue caching frame 8, they can provide
8 as the dst. If they do want to keep it, they need to make a copy.

> 
> We have no way of knowing how much of the next frame will be clobbered so we
> could end up doing a large copy just to replace most of it.

That's a good counterpoint.

> In that case, it
> might be better to have separate in and out params and let the decoder do the
> copy as needed.

I think it makes the API a little more complicated (and asks more of the codec),
but maybe it's worth it if it speeds things up. One aspect to consider - if the
client does *not* want to keep caching frame 8, and asks for 9 which depends on
8, they'll need to pass 8 to the codec. If we want to avoid the copy in that
case, I think we'll need to do the following:

The client passes 8 as an in/out parameter. This is similar the current patch
set, but it's inconsistent with how the client uses the API when they want to
continue caching 8 (8 as in parameter, 9 as out parameter)

> 
> However, maybe the copy is fast enough that it would be better to copy and
> clobber anyway.

We also have to consider how often it happens, and how much we're clobbering
versus keeping.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/22 01:08:47, cblume wrote:
> On 2016/09/21 22:23:48, msarett wrote:
> > Can we get away with not having this?  Seems like it allows the client to
make
> > us do a bunch of extra work up front.  Maybe it's something they need to
know
> > though...

FWIW, my initial design (patch set 1, which I worked on a long time ago, and
plays with some other ideas) did not have this method. You just queried for the
frames in order (or not, if you want...), and when you ask for a frame out of
bounds it returns false.

As Chris states in his response, it sounds like it is useful to know this ahead
of time. The downside is that we need to do work upfront (or whenever this
method is called - the client doesn't have to call it!), and then we have to
backtrack in the data to do actual decoding. This is fine with Blink's
SegmentReader, but on Android we may have to copy the input stream (or parts of
it).

> 
> In the case of gif and apng, we won't know in advance anyway.
> If a buffer comes in that isn't the full image, we won't know if there are
more
> frames coming in the next buffer.

I think that's generally the case. An image format could specify how many frames
it has up front (I think some of the ones we support do), but you cannot really
trust that, can you? If it has more or less frames actually in the data, you're
not going to care about the reported value. (We see something similar in BMP,
where the stream reports how long it is, but there are plenty of cases where it
is wrong.)

> 
> We could simply have this return an error code that specifies it isn't yet
> known.

I don't think an error code would be useful. If we just count them as we go (and
no backtracking), then we won't know until we've decoded all the frames anyway,
so the client can just count.

> But perhaps a better option would be let the client know "Here is how
> many frames we are aware of so far. But we don't yet have the entire image
> data."

FWIW, reporting how many frames we're aware of so far is how blink::ImageDecoder
works, and it is how I imagined this method working. (That's not clear from the
comment. I started to update the comment, but it would be confusing because
SkStreams are expected to block until they receive more data. But I think we're
going to have to move off of SkStreams anyway to support progressive/incremental
decoding properly.)

> Maybe this can be achieved by having a separate bool isImageComplete() or
> some such.

I think really you just want to know whether the codec has seen all the data.
But the client should know that, since they're supplying the data. So I don't
see what the codec has to add that's interesting.

> 
> 
> That said, from a client perspective it would be very helpful to know frame
> count and dependency information prior to actually displaying that frame.
Until
> I know which frames are valuable to cache, I would be forced to defensively
> cache them all.
> 
> Pulling out the metadata early would be useful to me.

[scroggo, 2016-09-22 18:37:15]

FYI: I've added a design doc at
https://docs.google.com/a/google.com/document/d/12Qhf9T92MWfdWujQwCIjhCO3sw6p...

[cblume, 2016-09-22 21:29:02]

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:293: bool            fHasPriorFrame;
I agree with what you are saying. Good thinking.

If the next frame's info is ready, the client can ask about the update rect.
Actually, the client would also need to query the dispose method of that frame.

With dispose method + frame update rect, the client would know whether or not it
needs to copy. In which case, I think I agree that the client should be
responsible for copying.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/22 16:46:42, scroggo wrote:
> On 2016/09/22 01:08:47, cblume wrote:
> > On 2016/09/21 22:23:48, msarett wrote:
> > > Can we get away with not having this?  Seems like it allows the client to
> make
> > > us do a bunch of extra work up front.  Maybe it's something they need to
> know
> > > though...
> 
> FWIW, my initial design (patch set 1, which I worked on a long time ago, and
> plays with some other ideas) did not have this method. You just queried for
the
> frames in order (or not, if you want...), and when you ask for a frame out of
> bounds it returns false.
> 
> As Chris states in his response, it sounds like it is useful to know this
ahead
> of time. The downside is that we need to do work upfront (or whenever this
> method is called - the client doesn't have to call it!), and then we have to
> backtrack in the data to do actual decoding. This is fine with Blink's
> SegmentReader, but on Android we may have to copy the input stream (or parts
of
> it).

Oh man, that is a bummer.

As a client, having the metadata in advance only provides benefit on the first
animation loop. After the first loop I can reuse the metadata.

That said, I understand Android's constraint. I REALLY wish we could lift that.
But this is the hand we are dealt right now.

If it would be easier to not provide this info in advance I am okay with it.


> > 
> > We could simply have this return an error code that specifies it isn't yet
> > known.
> 
> I don't think an error code would be useful. If we just count them as we go
(and
> no backtracking), then we won't know until we've decoded all the frames
anyway,
> so the client can just count.
> 

A common-enough case in Chrome is we scroll down to reveal an image. At that
point, our currently behavior is to catch up to where the animation should be.
If the animation looped in that time, we might actually be back at the
beginning.

If we had the metadata in advance, we wouldn't need to decode all the frames
completely to the end just to display the first frame. We could simply get the
frame count and durations.


Even if we were to keep the metadata after the first loop, it would require that
we have gone through the loop. In the case of scrolling down a webpage, we won't
have gone through the loop yet.


Maybe this is something we can consider later, since there are other options:
Maybe we can lift the Android requirement of forward-only reads. Maybe we can
change the catching-up behavior (this one is my preferred method).


> > But perhaps a better option would be let the client know "Here is how
> > many frames we are aware of so far. But we don't yet have the entire image
> > data."
> 
> FWIW, reporting how many frames we're aware of so far is how
blink::ImageDecoder
> works, and it is how I imagined this method working. (That's not clear from
the
> comment. I started to update the comment, but it would be confusing because
> SkStreams are expected to block until they receive more data. But I think
we're
> going to have to move off of SkStreams anyway to support
progressive/incremental
> decoding properly.)

Okay, that makes sense.

So as a client I can call this to get the currently-known-frame-count. And also
as a client, I will know whether or not I have provided all of the data to the
decoder.

If we get metadata in advance, the client can know "I have provided all of the
data, this now returns the actual maximum.".

And if we do not read metadata in advance, the client can perhaps request the
frame count before decoding the next frame. That way we would know there is a
next frame vs. we are looping. This would require Sk*Codec to check for a next
frame every time it decodes a frame. Is this good?

> > Maybe this can be achieved by having a separate bool isImageComplete() or
> > some such.
> 
> I think really you just want to know whether the codec has seen all the data.
> But the client should know that, since they're supplying the data. So I don't
> see what the codec has to add that's interesting.

Yeah, I guess you are right. Good call.

[msarett, 2016-09-22 22:54:36]

Started looking through the implementation...thinking out loud a little bit.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkCodecAnimat...
File src/codec/SkCodecAnimation.h (right):

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkCodecAnimat...
src/codec/SkCodecAnimation.h:41: *  In a GIF, a value of 4 is treated as
RestorePrevious.
nit: also?

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

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:251:
fColorTable.reset(create_color_table(*gif->SColorMap, transIndex,
gif->SBackGroundColor));
So if there is a global color table we always use it to set fColorTable - I'm
guessing this is the FIXME?

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:258: // I'm guessing the background index only makes
sense for the global color table.
I have no strong feelings about using or not using the background color.  Would
be interesting how many gifs have frames that actually don't fill the entire
bounds.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:270: // FIXME: Probably want to intersect this with the
bounds, just in case.
I think I made a bunch of test gifs to exercise some of these cases - should be
on the bots.

How does Chrome handle this case?  I'm wondering if we shouldn't just "crop" the
parts of the frame outside of the image dimensions.  I think that's what you're
suggesting here.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:287: // FIXME: We could correct these after decoding
(i.e. some frames may turn out to be
Seems like an optimization to consider later...

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:297: case
SkCodecAnimation::RestoreBGColor_DisposalMethod:
I'm not sure I completely understand the logic in this case...

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:311: if ((int) index >= fFrameInfos.count()) {
Can we assert this?  Seems like client should be smart enough to not pass us an
invalid index?

Edit: Maybe I'm wrong here - see below.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:319: if ((int) index >= fFrameInfos.count()) {
Ditto.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:327: // Get the encoded dimension values for the first
frame. Some GIFs have frames that are bigger
I remember when we decided to do this...

But now I kind of think that decision doesn't make sense anymore.  I'd rather
totally respect the bounds reported by the gif header.  And then we can treat
frames that fall outside these bounds "consistently".

Not sure whether the answer is shifting or cropping though.  I would vote
cropping - in some cases, if we shift, we might still have to crop anyway.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:385: // Gifs have the option to specify the color at a
single index of the color
I know Chrome always fills incomplete images with transparent pixels...  How do
they handle out of range indices?

This is complicated but seems to make a bit of sense.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:422: // FIXME: It is possible that a later frame can be
decoded to index8, if it does one of the
Good questions, probably a good place to follow up.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:432: return gif_error("frame index out of range!\n",
kInvalidParameters);
Hmmm ok...  We are set up to fail nicely here on invalid parameters.  Maybe I'm
wrong above - we should try to be consistent.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:488: // FIXME: Is this the right color?
Is this what Chrome does?

[scroggo, 2016-09-23 15:53:16]

Patch set 5 fixes up a comment Matt pointed out.

Patch set 6 experiments with something reed@ suggested in person - returning all
the meta data upfront instead of just the frame count. So previously I allowed
querying the frame count, and then individual features could be requested on a
per frame basis (e.g. getDuration(index), getRequiredFrame(index)). In this
patch set, a vector containing all that information is returned in one call. I
think it's a little awkward for a couple of reasons:

- For a non-animated image, do we return an empty vector? Or a vector of size
one with meaningless info (e.g. duration)?
- What do we do for an animated image for which we've only seen one frame? Do we
know for sure it's not just a GIF with only one frame? (Maybe it's obvious by
the metadata)
- For an image that's loading, the client will need to update their vector just
to find out it's the same. (If that's a problem I suppose we could work around
it though.)

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:293: bool            fHasPriorFrame;
On 2016/09/22 21:29:02, cblume wrote:
> I agree with what you are saying. Good thinking.
> 
> If the next frame's info is ready, the client can ask about the update rect.
> Actually, the client would also need to query the dispose method of that
frame.
> 
> With dispose method + frame update rect, the client would know whether or not
it
> needs to copy. In which case, I think I agree that the client should be
> responsible for copying.

My preference would be to hide the dispose method + frame update rect until we
determine that there's a speed benefit to skipping the copy. (That said, it's
not that hard to expose it.)

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/22 21:29:02, cblume wrote:
> On 2016/09/22 16:46:42, scroggo wrote:
> > On 2016/09/22 01:08:47, cblume wrote:
> > > On 2016/09/21 22:23:48, msarett wrote:
> > > > Can we get away with not having this?  Seems like it allows the client
to
> > make
> > > > us do a bunch of extra work up front.  Maybe it's something they need to
> > know
> > > > though...
> > 
> > FWIW, my initial design (patch set 1, which I worked on a long time ago, and
> > plays with some other ideas) did not have this method. You just queried for
> the
> > frames in order (or not, if you want...), and when you ask for a frame out
of
> > bounds it returns false.
> > 
> > As Chris states in his response, it sounds like it is useful to know this
> ahead
> > of time. The downside is that we need to do work upfront (or whenever this
> > method is called - the client doesn't have to call it!), and then we have to
> > backtrack in the data to do actual decoding. This is fine with Blink's
> > SegmentReader, but on Android we may have to copy the input stream (or parts
> of
> > it).
> 
> Oh man, that is a bummer.
> 
> As a client, having the metadata in advance only provides benefit on the first
> animation loop. After the first loop I can reuse the metadata.
> 
> That said, I understand Android's constraint. I REALLY wish we could lift
that.
> But this is the hand we are dealt right now.
> 
> If it would be easier to not provide this info in advance I am okay with it.

It might be easier not to provide the metadata up front, but I'm not convinced
it's a good decision. As you say, we'll have that data the second time through
the loop, but the first time through the loop we won't know anything about frame
dependencies. So the client won't be able to make any decisions about caching.
So they might save frames that they don't want to (seems bad on a page with lots
of animated gifs), or they might discard frames they ought to save (which means
the second loop, which you'd expect to run more smoothly, might have hiccups?).

Not providing the meta data upfront also makes providing the required frame more
awkward. The client would need to know the disposal method. And you could
imagine a bad client providing the wrong prior frame. Maybe that's not so bad
though.
> 
> 
> > > 
> > > We could simply have this return an error code that specifies it isn't yet
> > > known.
> > 
> > I don't think an error code would be useful. If we just count them as we go
> (and
> > no backtracking), then we won't know until we've decoded all the frames
> anyway,
> > so the client can just count.
> > 
> 
> A common-enough case in Chrome is we scroll down to reveal an image. At that
> point, our currently behavior is to catch up to where the animation should be.
> If the animation looped in that time, we might actually be back at the
> beginning.
> 
> If we had the metadata in advance, we wouldn't need to decode all the frames
> completely to the end just to display the first frame. We could simply get the
> frame count and durations.
> 
> 
> Even if we were to keep the metadata after the first loop, it would require
that
> we have gone through the loop. In the case of scrolling down a webpage, we
won't
> have gone through the loop yet.

That seems like a strong argument for getting the metadata up front.

> 
> 
> Maybe this is something we can consider later, since there are other options:
> Maybe we can lift the Android requirement of forward-only reads.

In order to do that, I think we'd need a new API. (To be fair, we need a new API
to support animated GIF anyway.) The current decoding calls take a Java
InputStream, which cannot necessarily rewind. We can always copy the input,
though - that's what we do for BitmapRegionDecoder. And if Android wants to be
able to loop, copying the input would allow them to purge the Bitmaps and
re-decode - which could be cheaper than holding onto the Bitmaps.

> Maybe we can
> change the catching-up behavior (this one is my preferred method).

What do you think the catching-up behavior should be? Will that make us
different from other browsers?

> 
> 
> > > But perhaps a better option would be let the client know "Here is how
> > > many frames we are aware of so far. But we don't yet have the entire image
> > > data."
> > 
> > FWIW, reporting how many frames we're aware of so far is how
> blink::ImageDecoder
> > works, and it is how I imagined this method working. (That's not clear from
> the
> > comment. I started to update the comment, but it would be confusing because
> > SkStreams are expected to block until they receive more data. But I think
> we're
> > going to have to move off of SkStreams anyway to support
> progressive/incremental
> > decoding properly.)
> 
> Okay, that makes sense.
> 
> So as a client I can call this to get the currently-known-frame-count. And
also
> as a client, I will know whether or not I have provided all of the data to the
> decoder.
> 
> If we get metadata in advance, the client can know "I have provided all of the
> data, this now returns the actual maximum.".
> 
> And if we do not read metadata in advance, the client can perhaps request the
> frame count before decoding the next frame. That way we would know there is a
> next frame vs. we are looping. This would require Sk*Codec to check for a next
> frame every time it decodes a frame. Is this good?

It sounds like what you really want to know is whether there is another frame,
not what the frame count is. But that would mean reading ahead in the stream
anyway. I suppose it would be less, so it would be cheaper than reading through
the whole stream.

> 
> > > Maybe this can be achieved by having a separate bool isImageComplete() or
> > > some such.
> > 
> > I think really you just want to know whether the codec has seen all the
data.
> > But the client should know that, since they're supplying the data. So I
don't
> > see what the codec has to add that's interesting.
> 
> Yeah, I guess you are right. Good call.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkCodecAnimat...
File src/codec/SkCodecAnimation.h (right):

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkCodecAnimat...
src/codec/SkCodecAnimation.h:41: *  In a GIF, a value of 4 is treated as
RestorePrevious.
On 2016/09/22 22:54:34, msarett wrote:
> nit: also?

Haha, yes, that will be more clear.

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

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:251:
fColorTable.reset(create_color_table(*gif->SColorMap, transIndex,
gif->SBackGroundColor));
On 2016/09/22 22:54:35, msarett wrote:
> So if there is a global color table we always use it to set fColorTable - I'm
> guessing this is the FIXME?

Not sure. Most of this code is throw-away/proof-of-concept.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:258: // I'm guessing the background index only makes
sense for the global color table.
On 2016/09/22 22:54:35, msarett wrote:
> I have no strong feelings about using or not using the background color. 
Would
> be interesting how many gifs have frames that actually don't fill the entire
> bounds.

My guess would be a lot of them. Even if they fill the bounds, they might have
transparency. We can determine this with some UMA histograms, but I know I've
seen some e.g. emoji-like images that are characters that don't fill the bounds.

The tricky thing is that if we do not use the background color, we've changed
the behavior for Android. But if we do, we've changed the behavior for Chrome.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:270: // FIXME: Probably want to intersect this with the
bounds, just in case.
On 2016/09/22 22:54:35, msarett wrote:
> I think I made a bunch of test gifs to exercise some of these cases - should
be
> on the bots.
> 
> How does Chrome handle this case?  I'm wondering if we shouldn't just "crop"
the
> parts of the frame outside of the image dimensions.  I think that's what
you're
> suggesting here.

Yes, that is what they do. But I didn't want to handle that with this dummy
implementation.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:287: // FIXME: We could correct these after decoding
(i.e. some frames may turn out to be
On 2016/09/22 22:54:34, msarett wrote:
> Seems like an optimization to consider later...

Yeah, this is what we talked about yesterday (e.g. looking at whether a frame
used the transparent index at all.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:297: case
SkCodecAnimation::RestoreBGColor_DisposalMethod:
On 2016/09/22 22:54:34, msarett wrote:
> I'm not sure I completely understand the logic in this case...

Yeah, this is a confusing one. As I understand it, if frame i-1 is marked as
RestoreBGColor_DisposalMethod, it means that i will start with i-1, except the
space taken up by i-1's frameRect is cleared to the background color (which
Chrome interprets as transparent).

If i-1 fills the dimensions, then we'll be erasing the whole frame anyway, so i
is effectively independent. That is the first half of the or conjunction.

If i-1 is independent, then that means it only occupies the frameRect anyway, so
outside of the frameRect is transparent, and we clear inside the frameRect to
transparent, too, so i is independent. This is the second half of the or.

Otherwise, we need to depend on i - 1. When i is drawn, we'll start with i-1,
erase the frameRect, and see the "border" - i.e. whatever was drawn by i - 2
minus the frameRect for i - 1.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:311: if ((int) index >= fFrameInfos.count()) {
On 2016/09/22 22:54:36, msarett wrote:
> Can we assert this?  Seems like client should be smart enough to not pass us
an
> invalid index?
> 
> Edit: Maybe I'm wrong here - see below.

I lean towards returning a failure for a public API. But I could be convinced
(assuming we allow querying the count up front, which I think we have not
settled on).

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:327: // Get the encoded dimension values for the first
frame. Some GIFs have frames that are bigger
On 2016/09/22 22:54:36, msarett wrote:
> I remember when we decided to do this...
> 
> But now I kind of think that decision doesn't make sense anymore.  I'd rather
> totally respect the bounds reported by the gif header.  And then we can treat
> frames that fall outside these bounds "consistently".
> 
> Not sure whether the answer is shifting or cropping though.  I would vote
> cropping - in some cases, if we shift, we might still have to crop anyway.

I think we should stick with the behavior that Chromium currently has, and
consider changing that later on. Chrome expands based on the first frame. I'm
not sure whether they shift.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:385: // Gifs have the option to specify the color at a
single index of the color
On 2016/09/22 22:54:35, msarett wrote:
> I know Chrome always fills incomplete images with transparent pixels...  How
do
> they handle out of range indices?
> 
> This is complicated but seems to make a bit of sense.

Do you mean a transparent pixel that is out of range? They treat that as not
having a transparent pixel.

For other colors, if they are out of range, they skip that pixel (which is okay,
since they already cleared to transparent). There might be ways to optimize
that:
- For an 8 bit GIF, the values to be read out of the GIF are guaranteed to be <=
256, right? So the only way for the index to be out of range is for the table to
have less than 256 colors. So we can fill in up to 256 colors with transparent
(I think we might already do that?). That might be faster than the if statement,
not sure.
- I'm guessing this would be similarly true for 4 bit colors.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:432: return gif_error("frame index out of range!\n",
kInvalidParameters);
On 2016/09/22 22:54:34, msarett wrote:
> Hmmm ok...  We are set up to fail nicely here on invalid parameters.  Maybe
I'm
> wrong above - we should try to be consistent.

Again I lean towards failing gracefully in public APIs, and if we remove the
option to know how many frames there are up front, we need to be able to fail
gracefully for sure.

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:488: // FIXME: Is this the right color?
On 2016/09/22 22:54:34, msarett wrote:
> Is this what Chrome does?

Yes. But my concern is that if we're decoding to index 8, this will be the 0th
color, which may not be transparent.

[cblume, 2016-09-23 19:15:45]

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:293: bool            fHasPriorFrame;
On 2016/09/23 15:53:15, scroggo wrote:
> On 2016/09/22 21:29:02, cblume wrote:
> > I agree with what you are saying. Good thinking.
> > 
> > If the next frame's info is ready, the client can ask about the update rect.
> > Actually, the client would also need to query the dispose method of that
> frame.
> > 
> > With dispose method + frame update rect, the client would know whether or
not
> it
> > needs to copy. In which case, I think I agree that the client should be
> > responsible for copying.
> 
> My preference would be to hide the dispose method + frame update rect until we
> determine that there's a speed benefit to skipping the copy. (That said, it's
> not that hard to expose it.)

I agree. We can cross that bridge if it becomes necessary. Until then, it is
needlessly overengineering.

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/23 15:53:15, scroggo wrote:
> That seems like a strong argument for getting the metadata up front.

An alternative would be if Chrome changed its catching-up behavior. I would like
to do this either way. I'll explain more below.

> > Maybe we can
> > change the catching-up behavior (this one is my preferred method).
> 
> What do you think the catching-up behavior should be? Will that make us
> different from other browsers?

The current browser catch-up landscape is:

FF desktop: Catches up
FF mobile: Catches up
Safari desktop: Catches up
Safari mobile: Does not catch up
Edge desktop: Does not catch up
Edge mobile: I don't have a Windows phone to try. But since desktop doesn't,
I'll guess mobile doesn't either.

If we did not catch up, when we scroll down and reveal an image we could simply
start at the beginning of the animation. Similarly, when an image is scrolled
out of view and back into view, we can essentially "pause" the animation and
resume where we left off. The fastest code is no code, ya know? :D

Currently, every time we need to catch-up an animation we have burn a serious
amount of cpu and battery to churn through several frames. Worse, this is
effectively unbounded.

Another benefit is say we have a.gif in the DOM. Then a second later, we add a
new img tag to the DOM containing a.gif as well. We would need to maintain these
two images out of sync with each other since their start times are different.
But if we don't enforce the time-since-added-to-DOM, we could keep them on the
same frame and half our cache. Once again, you can have an effectively unlimited
number of a.gif images in your page all starting at different times.

Given that some other browsers already do this, I feel silly NOT doing it.

But just to be clear, we cannot ALWAYS keep images on the same frame:
https://crbug.com/527842


> It sounds like what you really want to know is whether there is another frame,
> not what the frame count is. But that would mean reading ahead in the stream
> anyway. I suppose it would be less, so it would be cheaper than reading
through
> the whole stream.

Yeah, you are right. I only need to know if there is a next frame.

When I read frame N, maybe the header for frame N+1 is immediately after? Maybe
it can be included as part of the read for frame N (and cached so we can query
if there is a next frame)?

[scroggo, 2016-09-23 21:15:00]

I don't think I'm going to get up another patch set that tries out the version
that returns info about the next frame along with the decode before leaving for
the weekend, but I wanted to post some comments in case you (i.e. cblume@) have
a chance to look at them today.

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/23 19:15:45, cblume wrote:
> On 2016/09/23 15:53:15, scroggo wrote:
> > That seems like a strong argument for getting the metadata up front.
> 
> An alternative would be if Chrome changed its catching-up behavior. I would
like
> to do this either way. I'll explain more below.
> 
> > > Maybe we can
> > > change the catching-up behavior (this one is my preferred method).
> > 
> > What do you think the catching-up behavior should be? Will that make us
> > different from other browsers?
> 
> The current browser catch-up landscape is:
> 
> FF desktop: Catches up
> FF mobile: Catches up
> Safari desktop: Catches up
> Safari mobile: Does not catch up
> Edge desktop: Does not catch up
> Edge mobile: I don't have a Windows phone to try. But since desktop doesn't,
> I'll guess mobile doesn't either.
> 
> If we did not catch up, when we scroll down and reveal an image we could
simply
> start at the beginning of the animation. Similarly, when an image is scrolled
> out of view and back into view, we can essentially "pause" the animation and
> resume where we left off. The fastest code is no code, ya know? :D
> 
> Currently, every time we need to catch-up an animation we have burn a serious
> amount of cpu and battery to churn through several frames. Worse, this is
> effectively unbounded.
> 
> Another benefit is say we have a.gif in the DOM. Then a second later, we add a
> new img tag to the DOM containing a.gif as well. We would need to maintain
these
> two images out of sync with each other since their start times are different.
> But if we don't enforce the time-since-added-to-DOM, we could keep them on the
> same frame and half our cache. Once again, you can have an effectively
unlimited
> number of a.gif images in your page all starting at different times.
> 
> Given that some other browsers already do this, I feel silly NOT doing it.
> 
> But just to be clear, we cannot ALWAYS keep images on the same frame:
> https://crbug.com/527842

Those sound like good reasons to change the catchup behavior, especially if
other browsers do not agree on the right answer. I had thought our motivation
was that a page may have multiple GIFs that the designer expects to stay in
sync. I don't know whether that's a common use case, and it sounds like it won't
work on all browsers anyway, so maybe that's not a good motivation.

> 
> 
> > It sounds like what you really want to know is whether there is another
frame,
> > not what the frame count is. But that would mean reading ahead in the stream
> > anyway. I suppose it would be less, so it would be cheaper than reading
> through
> > the whole stream.
> 
> Yeah, you are right. I only need to know if there is a next frame.
> 
> When I read frame N, maybe the header for frame N+1 is immediately after?

Essentially. (see [1]) It is possible there are other blocks in between, but
those look to be comments (we ignore) or another block without image data. So we
could go ahead and parse those.

[1]
http://giflib.sourceforge.net/whatsinagif/bits_and_bytes.html#logical_screen_...

> Maybe
> it can be included as part of the read for frame N

That's an interesting idea. So the client asks for frame N, and the codec
returns frame N *and* the information about N + 1, specifically:
- does N + 1 exist
- what is the required frame for N + 1

But I can imagine a case where it's awkward:
- Codec has enough data to decode just N frames (again, the client knows it's
not the full data)
- Decoding frame N reports that N + 1 does not exist
- Client knows that there *may be* more frames, and supplies more data
- How does the client determine the true nature of N + 1? (does it exist,
required frame)
(maybe your answer is down below)

> (and cached so we can query
> if there is a next frame)?

What would that API look like? A couple of options:

- struct FrameData nextFrameData(); (where FrameData contains required frame,
animation time perhaps, etc)
I do not like this one because it puts some more state into the codec (what is
the "next" frame?)

- struct FrameData getFrameData(size_t index);
This looks like it might read ahead to an arbitrary frame. If we implement it
that way, we might as well implement getFrameCount (as in patch set 2) or
vector<FrameData> getFrameData() (as in patch set 6). If we do not, it raises
questions about what we would return if the client tries to read ahead.

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

https://codereview.chromium.org/2045293002/diff/60001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:469: SkASSERT((int) frameIndex < fFrameInfos.count());
D'oh! After all my talk about failing gracefully I have this assert here...

[cblume, 2016-09-23 23:31:32]

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
On 2016/09/23 21:15:00, scroggo wrote:
> On 2016/09/23 19:15:45, cblume wrote:
> > Maybe
> > it can be included as part of the read for frame N
> 
> That's an interesting idea. So the client asks for frame N, and the codec
> returns frame N *and* the information about N + 1, specifically:
> - does N + 1 exist
> - what is the required frame for N + 1
> 
> But I can imagine a case where it's awkward:
> - Codec has enough data to decode just N frames (again, the client knows it's
> not the full data)
> - Decoding frame N reports that N + 1 does not exist
> - Client knows that there *may be* more frames, and supplies more data
> - How does the client determine the true nature of N + 1? (does it exist,
> required frame)
> (maybe your answer is down below)


Oh no no. I didn't mean that the API changes. I meant that internally, in
Sk*Codec, reading a frame would also read in enough of the next frame's header
(if available, you are right) to allow us to query that info.

I was under the impression that if Sk*Codec doesn't make a copy of the data and
only reads forward, requesting a frame is the only thing that would advance the
read. Maybe I was understanding this incorrectly.

> > (and cached so we can query
> > if there is a next frame)?
> 
> What would that API look like? A couple of options:
> 
> - struct FrameData nextFrameData(); (where FrameData contains required frame,
> animation time perhaps, etc)
> I do not like this one because it puts some more state into the codec (what is
> the "next" frame?)

I also don't like "next" frame for that exact reason - it forces SkCodec to keep
state.

> - struct FrameData getFrameData(size_t index);
> This looks like it might read ahead to an arbitrary frame. If we implement it
> that way, we might as well implement getFrameCount (as in patch set 2) or
> vector<FrameData> getFrameData() (as in patch set 6). If we do not, it raises
> questions about what we would return if the client tries to read ahead.

Oh. You are right.
I was thinking something like struct FrameData getFrameData(size_t index);
You are right that it seems like you could pass an arbitrary index which might
not have been read yet.
And even if it was the next frame index, the client may not have provided enough
data for it to be available.


All of this makes me feel more and more like we should read the metadata in
advance.


I think I like the vector<frameInfo> getFrameInfo() if we are allowed to read
the metadata in advance. As a client, I will know this is all the frame info we
are aware of so far.

[msarett, 2016-09-26 14:13:13]

> Patch set 6 experiments with something reed@ suggested in person - returning
all
> the meta data upfront instead of just the frame count. So previously I allowed
> querying the frame count, and then individual features could be requested on a
> per frame basis (e.g. getDuration(index), getRequiredFrame(index)). In this
> patch set, a vector containing all that information is returned in one call.

FWIW, this seems logical to me.  If we are going to have
a getFrameCount() (that needs to read the stream anyway),
can't we provide all of the metadata at minimal extra cost?

> I think it's a little awkward for a couple of reasons:
> 
> - For a non-animated image, do we return an empty vector? Or a vector of size
> one with meaningless info (e.g. duration)?

I agree that this is weird.  Does the following distinction
make sense?
empty vector: means that the image is definitely not animated
vector of size 1: might be the first frame of an animation,
might just be an image
vector size > 1: animation

Is there a drawback to treating single frame images as if
they might be animated?

> - What do we do for an animated image for which we've only seen one frame? Do
we
> know for sure it's not just a GIF with only one frame? (Maybe it's obvious by
> the metadata)

Seems like there are a few chunks that  might help us
guess, but I doubt we can be sure.

How does Chrome handle multi-frame gifs that are missing
animation information?  (Either the per-frame blob
containing delay time, disposal method, or the other
blob that tells us how many times to loop).

> - For an image that's loading, the client will need to update their vector
just
> to find out it's the same. (If that's a problem I suppose we could work around
> it though.)

What do you mean by this?

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

https://codereview.chromium.org/2045293002/diff/60001/include/codec/SkCodec.h...
include/codec/SkCodec.h:225: struct MultiFrameOptions {
I like how this looks, but don't feel strongly.  Might be convenient to have
this is a struct if we decide to add/remove multi-frame options.

Also might be nice to do something similar for "ColorTableOptions".

[scroggo, 2016-09-26 15:48:44]

Description was changed from

==========
WIP: Support multiple frames in SkGifCodec

Experiment with an interface to decode frames beyond the first in
SkGifCodec.

Add the following new methods to SkCodec:
- getFrameCount
- getRequiredFrame
- getFrameDuration

Add the following fields on SkCodec::Options:
- fFrameIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.fFrameIndex. If the client does not have the frame's
required frame (the frame that this frame must be blended on top
of) cached, they pass false for Options.fHasPriorFrame. Unless
the frame is independent, the codec will then recursively decode
all frames necessary to decode fFrameIndex. If the client has the
required frame cached, they can put it in the dst they pass to
the codec, and the codec will only draw fFrameIndex onto it.

This API is intended to support progressive decoding (although
the current implementation does not decode progressively - it
decodes the whole image from the start).

For simplicity, this removes scanline decoding (and therefore
sampling) and modifies the test to not test for it. Scanline decoding
is being phased out anyway, so before this lands it will be
replaced with progressive decoding.

The implementation should likely be replaced with Chromium's, which
is much better tested than mine, and already supports progressive
decoding. The current implementation *may* fail to produce partial
images/first frames if there is an error later in the stream; I am
not sure how DGifSlurp handles errors.

Add a GM that animates a GIF.
Add a test for the new APIs.

TODO: Support progressive decoding
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
WIP: Support multiple frames in SkGifCodec

Add an interface to decode frames beyond the first in
SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame


The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

This API is intended to support progressive decoding (although
the current implementation does not decode progressively - it
decodes the whole image from the start).

For simplicity, this removes scanline decoding (and therefore
sampling) and modifies the test to not test for it. Scanline decoding
is being phased out anyway, so before this lands it will be
replaced with progressive decoding.

The implementation should likely be replaced with Chromium's, which
is much better tested than mine, and already supports progressive
decoding. The current implementation *may* fail to produce partial
images/first frames if there is an error later in the stream; I am
not sure how DGifSlurp handles errors.

Add a GM that animates a GIF.
Add a test for the new APIs.

TODO: Support progressive decoding
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[scroggo, 2016-09-26 15:51:43]

PTAL: I think returning the vector metadata in one call is the way to go. If
you're all happy with this API, I can go ahead and start implementing for real
(including progressive decoding).

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

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:603: size_t getFrameCount() {
> All of this makes me feel more and more like we should read the metadata in
> advance.
> 
> 
> I think I like the vector<frameInfo> getFrameInfo() if we are allowed to read
> the metadata in advance. As a client, I will know this is all the frame info
we
> are aware of so far.

+1

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

https://codereview.chromium.org/2045293002/diff/60001/include/codec/SkCodec.h...
include/codec/SkCodec.h:225: struct MultiFrameOptions {
On 2016/09/26 14:13:13, msarett wrote:
> I like how this looks, but don't feel strongly.  Might be convenient to have
> this is a struct if we decide to add/remove multi-frame options.
> 
> Also might be nice to do something similar for "ColorTableOptions".

+1

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

https://codereview.chromium.org/2045293002/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:641: std::vector<FrameInfo> getFrameInfo() {
On 2016/09/26 14:13:13, msarett wrote:
> > Patch set 6 experiments with something reed@ suggested in person - returning
> all
> > the meta data upfront instead of just the frame count. So previously I
allowed
> > querying the frame count, and then individual features could be requested on
a
> > per frame basis (e.g. getDuration(index), getRequiredFrame(index)). In this
> > patch set, a vector containing all that information is returned in one call.
> 
> FWIW, this seems logical to me.  If we are going to have
> a getFrameCount() (that needs to read the stream anyway),
> can't we provide all of the metadata at minimal extra cost?
> 
> > I think it's a little awkward for a couple of reasons:
> > 
> > - For a non-animated image, do we return an empty vector? Or a vector of
size
> > one with meaningless info (e.g. duration)?
> 
> I agree that this is weird.  Does the following distinction
> make sense?
> empty vector: means that the image is definitely not animated
> vector of size 1: might be the first frame of an animation,
> might just be an image
> vector size > 1: animation

sgtm

> 
> 
> https://codereview.chromium.org/2045293002/diff/60001/include/codec/SkCodec.h
> File include/codec/SkCodec.h (right):
> 
>
https://codereview.chromium.org/2045293002/diff/60001/include/codec/SkCodec.h...
> include/codec/SkCodec.h:225: struct MultiFrameOptions {
> I like how this looks, but don't feel strongly.  Might be convenient to have
> this is a struct if we decide to add/remove multi-frame options.
> 
> Also might be nice to do something similar for "ColorTableOptions".


> Is there a drawback to treating single frame images as if
> they might be animated?

I guess there's not a huge downside - I don't want the caller to think they need
to animate or to think e.g. fDuration means something when it doesn't.

But as you say below, a GIF with only one frame received so far should have some
information about fDuration that will give us a hint that it is animated.

> 
> > - What do we do for an animated image for which we've only seen one frame?
Do
> we
> > know for sure it's not just a GIF with only one frame? (Maybe it's obvious
by
> > the metadata)
> 
> Seems like there are a few chunks that  might help us
> guess, but I doubt we can be sure.
> 
> How does Chrome handle multi-frame gifs that are missing
> animation information?  (Either the per-frame blob
> containing delay time, disposal method, or the other
> blob that tells us how many times to loop).

It looks like Chrome defaults to 0 delay time [1], keep disposal method [2], and
loop once [4]

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
(ImageFrame::DisposeNotSpecified is treated the same as keep [3])
[3]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[4]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

> 
> > - For an image that's loading, the client will need to update their vector
> just
> > to find out it's the same. (If that's a problem I suppose we could work
around
> > it though.)
> 
> What do you mean by this?

Each time the client calls getFrameInfo(), we're going to have to create a new
vector (or copy one, if we have it internally) with this API. This is true even
if nothing has changed.

That seems like unnecessary work/memory use, but that's just me prematurely
optimizing...

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

https://codereview.chromium.org/2045293002/diff/120001/include/codec/SkCodec....
include/codec/SkCodec.h:618: struct FrameInfo {
I also considered "FrameMetaData". Any preference for one or the other?

[msarett, 2016-09-26 17:30:54]

> PTAL: I think returning the vector metadata in one call is the way to go. If
> you're all happy with this API, I can go ahead and start implementing for real
> (including progressive decoding).

sgtm

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

https://codereview.chromium.org/2045293002/diff/120001/include/codec/SkCodec....
include/codec/SkCodec.h:618: struct FrameInfo {
On 2016/09/26 15:51:43, scroggo wrote:
> I also considered "FrameMetaData". Any preference for one or the other?

I prefer FrameInfo, but don't feel strongly.

https://codereview.chromium.org/2045293002/diff/120001/include/codec/SkCodec....
include/codec/SkCodec.h:626: *  Number of 1/100 seconds to show this frame.
I'm guessing "1/100 seconds" is what Chrome does?  Is there is a particular
reason?  Maybe because this is as precise as we can be?

Just asking because I imagine milliseconds is more intuitive for a client than
"centiseconds".

[scroggo, 2016-09-26 17:43:11]

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

https://codereview.chromium.org/2045293002/diff/120001/include/codec/SkCodec....
include/codec/SkCodec.h:626: *  Number of 1/100 seconds to show this frame.
On 2016/09/26 17:30:53, msarett wrote:
> I'm guessing "1/100 seconds" is what Chrome does?  Is there is a particular
> reason?  Maybe because this is as precise as we can be?
> 
> Just asking because I imagine milliseconds is more intuitive for a client than
> "centiseconds".

D'oh - good catch. I just used 1/100 seconds because that's how GIF stores it.
But Chrome multiplies that by 10 and stores the milliseconds. I've fixed this.

[scroggo_chromium, 2016-10-13 17:29:21]

Description was changed from

==========
WIP: Support multiple frames in SkGifCodec

Add an interface to decode frames beyond the first in
SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame


The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

This API is intended to support progressive decoding (although
the current implementation does not decode progressively - it
decodes the whole image from the start).

For simplicity, this removes scanline decoding (and therefore
sampling) and modifies the test to not test for it. Scanline decoding
is being phased out anyway, so before this lands it will be
replaced with progressive decoding.

The implementation should likely be replaced with Chromium's, which
is much better tested than mine, and already supports progressive
decoding. The current implementation *may* fail to produce partial
images/first frames if there is an error later in the stream; I am
not sure how DGifSlurp handles errors.

Add a GM that animates a GIF.
Add a test for the new APIs.

TODO: Support progressive decoding
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame


The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Instead of using gif_lib, use GIFImageReader, imported from Chromium
(along with its copyright headers). Then I made the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:

Add a GM that animates a GIF.
Add tests for the new APIs.

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

[scroggo_chromium, 2016-10-13 18:02:09]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame


The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Instead of using gif_lib, use GIFImageReader, imported from Chromium
(along with its copyright headers). Then I made the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:

Add a GM that animates a GIF.
Add tests for the new APIs.

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame


The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Add a GM that animates a GIF.
Add tests for the new APIs.

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

[scroggo_chromium, 2016-10-13 18:03:32]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame


The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Add a GM that animates a GIF.
Add tests for the new APIs.

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Add a GM that animates a GIF.
Add tests for the new APIs.

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

[scroggo_chromium, 2016-10-13 20:10:29]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Add a GM that animates a GIF.
Add tests for the new APIs.

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

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

[scroggo_chromium, 2016-10-13 20:38:16]

PTAL.

Patch set 14 is ready for review. I've repurposed blink's GIFImageReader, and if
you diff those files against patch set 11, which contains the original files,
you can see what I changed specifically. I tried to not change *too much* so the
difference is easier to see. (There are some trivial changes to be made e.g.
style, and there are probably other changes that would be more efficient.)

https://codereview.chromium.org/2045293002/diff/260001/tests/CodecTest.cpp
File tests/CodecTest.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/tests/CodecTest.cpp#ne...
tests/CodecTest.cpp:292: // FIXME: This is broken for gif.
Incremental decoding with GIF works for the case where we are sampling, when
SkSampledCodec provides a subset that cuts off lines that would be sampled out
anyway. But the API does not distinguish between that and arbitrary subsets,
which is currently broken for GIF. We do not actually need to support this use
case, since we only need true subsetting for png and jpeg (and webp), for
BitmapRegionDecoder. But a bad client could easily make us do the wrong thing
here.

A proper fix might be for SkSampledCodec to not repurpose the subset to mean
ignore these lines that fall outside the sampled area, which I think msarett@
pushed for anyway.

[msarett, 2016-10-14 14:58:51]

Looks good so far.  Still need to look at GifImageReader and the rest of
SkGifCodec.

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:504: const bool useIncremental = fPath.endsWith("png") ||
fPath.endsWith("gif") || fPath.endsWith("GIF");
nit: 100 chars
nit: Should we defensively add PNG here as well?

https://codereview.chromium.org/2045293002/diff/260001/gyp/codec.gyp
File gyp/codec.gyp (left):

https://codereview.chromium.org/2045293002/diff/260001/gyp/codec.gyp#oldcode20
gyp/codec.gyp:20: 'giflib.gyp:giflib',
Can this also be removed from our android_deps?

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkCodecAnima...
File src/codec/SkCodecAnimation.h (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkCodecAnima...
src/codec/SkCodecAnimation.h:52: //     BitmapImage.cpp to avoid special-casing
it, and simply treat all
Reference to a chromium class?

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (left):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:23: if (memcmp(GIF_STAMP,   buf, GIF_STAMP_LEN) == 0 ||
Why this change?

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:93: // FIXME: Except that we may not be able to decode
to index8 for a frame beyond the first :(
I'm guessing that users of the animated API are "smart".  Maybe they can handle
the fact that requests for Index8 may fail?

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:94: // FIXME: Gifs can actually be encoded with 4-bits
per pixel.  Can we support this?
I think the swizzler already supports this.  So yes, I think we should report
this accurately in SkEncodedInfo and let the swizzler deal with it.

Doesn't need to be part of this CL though.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:95: // FIXME: We do not yet know whether the image has
alpha, so report binary. More specifically,
This may be fine, but it's starting to feel like SkEncodedInfo is not great for
describing animated images.

Might be nice to actually report opacity to clients that know they only care
about the single-frame case.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.h
File src/codec/SkGifCodec.h (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.h...
src/codec/SkGifCodec.h:34: bool haveDecodedRow(size_t frameIndex, const unsigned
char* rowBegin,
Seems strange to me that this is public...  I guess we never really allow people
to know that they specifically have an SkGifCodec anyway.

Still I think I might prefer hiding this and friend classing GIFImageReader.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.h...
src/codec/SkGifCodec.h:69: *                        contain 8-bit indices, we
need a 256 entry color
Maybe delete this wrong comment?  (pretty sure I wrote it)

[joostouwerling, 2016-10-14 15:02:44]

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
File src/codec/GIFImageReader.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
src/codec/GIFImageReader.cpp:415: // GIFSizeQuery and GIFFrameCountQuery are
negative, so this is only meaningful when >= 0.
If I understand it correctly you can call this method with a non negative int
for |query|, if you want to parse up to frame |query|? Isn't that a little
obscure, as compared to, say:

struct GIFParseQuery {
    enum class Type {GIFSizeQuery, GIFFrameCountQuery, GIFParseUpToFrameQuery};
    size_t payload;
}

where you can indicate up to which frame you want to parse with the payload?

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
File src/codec/GIFImageReader.h (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
src/codec/GIFImageReader.h:65: constexpr size_t kNotFound =
static_cast<size_t>(-1);
I guess this is to be compatible with the WTF's kNotFound? Do you have an idea
why they did it this way? It seems a bit smelly...

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
src/codec/GIFImageReader.h:315: bool decode(size_t frameIndex, bool*
frameDecoded);
nit: Why did you decide to put the documentation for the return value in the
implementation file, in this case? It was not immediately clear to me what it is
supposed to indicate.

[cblume, 2016-10-14 16:52:36]

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:447: if (reqFrame != SkCodec::kIndependentFrame && reqFrame ==
cachedFrame
I wonder if we can find a better name than kIndependentFrame.

I understand it is an invalid frame index which tells us that there is no
required frame. But it is *this* frame which is independent, not the required
frame. So this ends up reading in a bit of a strange way.

This might sound silly but I think it would be easier to read as:
if (reqFrame != SkCodec::kInvalidFrameIndex

We could even provide:
bool isIndependentFrame(size_t index) {
    const size_t reqFrame = frameInfos[i].fRequiredFrame;
    return reqFrame == SkCodec::kInvalidFrameIndex;
}

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:477: }
I like this way of dealing with DisposeOverwritePrevious.

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:504: const bool useIncremental = fPath.endsWith("png") ||
fPath.endsWith("gif") || fPath.endsWith("GIF");
On 2016/10/14 14:58:50, msarett wrote:
> nit: 100 chars
> nit: Should we defensively add PNG here as well?

or just toLower

[scroggo_chromium, 2016-10-14 19:56:00]

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:447: if (reqFrame != SkCodec::kIndependentFrame && reqFrame ==
cachedFrame
On 2016/10/14 16:52:35, cblume wrote:
> I wonder if we can find a better name than kIndependentFrame.
> 
> I understand it is an invalid frame index which tells us that there is no
> required frame. But it is *this* frame which is independent, not the required
> frame. So this ends up reading in a bit of a strange way.
> 
> This might sound silly but I think it would be easier to read as:
> if (reqFrame != SkCodec::kInvalidFrameIndex
> 
> We could even provide:
> bool isIndependentFrame(size_t index) {
>     const size_t reqFrame = frameInfos[i].fRequiredFrame;
>     return reqFrame == SkCodec::kInvalidFrameIndex;
> }

I understand your confusion, and I'm not opposed to the name kInvalidFrameIndex
(maybe something simpler like kNone? Blink uses kNotFound, which doesn't quite
sound right to me). I've added a FIXME for now, until we agree on a name. reed@?

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:504: const bool useIncremental = fPath.endsWith("png") ||
fPath.endsWith("gif") || fPath.endsWith("GIF");
On 2016/10/14 14:58:50, msarett wrote:
> nit: 100 chars

done

> nit: Should we defensively add PNG here as well?

done

https://codereview.chromium.org/2045293002/diff/260001/gyp/codec.gyp
File gyp/codec.gyp (left):

https://codereview.chromium.org/2045293002/diff/260001/gyp/codec.gyp#oldcode20
gyp/codec.gyp:20: 'giflib.gyp:giflib',
On 2016/10/14 14:58:50, msarett wrote:
> Can this also be removed from our android_deps?

Where are you looking? The only place I see it, if I

$ git grep giflib.gyp

is in gyp/images.gyp. And that is still needed for SkGIFMovie.cpp. I know
there's been a lot of hate for SkMovie, but I think the simplest way to stop
requiring giflib is to switch that file to use SkCodec.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
File src/codec/GIFImageReader.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
src/codec/GIFImageReader.cpp:415: // GIFSizeQuery and GIFFrameCountQuery are
negative, so this is only meaningful when >= 0.
On 2016/10/14 15:02:44, joostouwerling wrote:
> If I understand it correctly you can call this method with a non negative int
> for |query|, if you want to parse up to frame |query|? Isn't that a little
> obscure, as compared to, say:
> 
> struct GIFParseQuery {
>     enum class Type {GIFSizeQuery, GIFFrameCountQuery,
GIFParseUpToFrameQuery};
>     size_t payload;
> }
> 
> where you can indicate up to which frame you want to parse with the payload?

I wouldn't call it obscure so much as getting all the information we can out of
one variable.

An awkward part of your struct is that "payload" is only meaningful for one
value of Type.

I've added some comments in the header to clarify the intent.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
File src/codec/GIFImageReader.h (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
src/codec/GIFImageReader.h:65: constexpr size_t kNotFound =
static_cast<size_t>(-1);
On 2016/10/14 15:02:44, joostouwerling wrote:
> I guess this is to be compatible with the WTF's kNotFound? Do you have an idea
> why they did it this way? It seems a bit smelly... 

It's for the same reason that WTF made their decision (and partially because it
meant changing less code). We need some invalid number. This is the maximum
size_t, which is guaranteed to be invalid. We could switch to using a signed
number and use -1, but the nice thing about using a number that's larger than
any valid number is we don't have to special case it. To determine if an index
is valid we can say:

if (index < 256) {
  // It's valid!
}

Whereas if we used -1, we'd have to say:

if (index >= 0 && index < 256) {
  // It's valid!
}

(We could get the same number by instead using
std::numeric_limits<size_t>::max() from <limits>.) Maybe that's trickier than
necessary, though. We could get the same effect by setting kNotFound to 256, or
we could use a separate boolean to track whether it's valid. I do not have a
strong preference.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/GIFImageRead...
src/codec/GIFImageReader.h:315: bool decode(size_t frameIndex, bool*
frameDecoded);
On 2016/10/14 15:02:44, joostouwerling wrote:
> nit: Why did you decide to put the documentation for the return value in the
> implementation file, in this case? It was not immediately clear to me what it
is
> supposed to indicate. 

The documentation just happened to already be in the cpp file. In writing some
new documentation here, I decided that "frameDecoded" would be more
appropriately named "frameComplete". I've changed that in the latest patch set.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkCodecAnima...
File src/codec/SkCodecAnimation.h (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkCodecAnima...
src/codec/SkCodecAnimation.h:52: //     BitmapImage.cpp to avoid special-casing
it, and simply treat all
On 2016/10/14 14:58:50, msarett wrote:
> Reference to a chromium class?

Removed.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (left):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:23: if (memcmp(GIF_STAMP,   buf, GIF_STAMP_LEN) == 0 ||
On 2016/10/14 14:58:51, msarett wrote:
> Why this change?

GIFImageReader does not check for GIF_STAMP (see [1]). I could instead update it
to do so, but the fact that Chromium does not check for it, and presumably it
hasn't been a problem, leads me to believe that we're not missing any real life
GIFs by not checking it.

I'm not opposed to reviving the check though.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:93: // FIXME: Except that we may not be able to decode
to index8 for a frame beyond the first :(
On 2016/10/14 14:58:50, msarett wrote:
> I'm guessing that users of the animated API are "smart".  Maybe they can
handle
> the fact that requests for Index8 may fail?

Currently, requests for Index8 for frames beyond the first *will* fail. But we
still want to report Index8 so we can continue to allow the Android framework to
support Index8 (without adding some extra code that knows a GIF is actually
Index8), yet this means a client could ask for getInfo(), frame 1, which would
fail. I don't know how to make this cleaner, but there may not be any good
"FIX"...

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:94: // FIXME: Gifs can actually be encoded with 4-bits
per pixel.  Can we support this?
On 2016/10/14 14:58:50, msarett wrote:
> I think the swizzler already supports this.  So yes, I think we should report
> this accurately in SkEncodedInfo and let the swizzler deal with it.
> 
> Doesn't need to be part of this CL though.

sgtm. Updated the comment.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:95: // FIXME: We do not yet know whether the image has
alpha, so report binary. More specifically,
On 2016/10/14 14:58:50, msarett wrote:
> This may be fine, but it's starting to feel like SkEncodedInfo is not great
for
> describing animated images.
> 
> Might be nice to actually report opacity to clients that know they only care
> about the single-frame case.

I'm not sure how to do that, though. At this point, we don't know whether the
client wants the first frame or a later frame. I figure this is a decent
conservative guess.

I've updated the FIXME a little.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.h
File src/codec/SkGifCodec.h (right):

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.h...
src/codec/SkGifCodec.h:34: bool haveDecodedRow(size_t frameIndex, const unsigned
char* rowBegin,
On 2016/10/14 14:58:51, msarett wrote:
> Seems strange to me that this is public...  I guess we never really allow
people
> to know that they specifically have an SkGifCodec anyway.

Indeed, it is a little awkward, though someone would have to go out of their way
to abuse it.

> 
> Still I think I might prefer hiding this and friend classing GIFImageReader.

That strikes me as more dangerous. The GIFImageReader can then call any method
on SkGifCodec, or change its private variables.

https://codereview.chromium.org/2045293002/diff/260001/src/codec/SkGifCodec.h...
src/codec/SkGifCodec.h:69: *                        contain 8-bit indices, we
need a 256 entry color
On 2016/10/14 14:58:51, msarett wrote:
> Maybe delete this wrong comment?  (pretty sure I wrote it)

I updated the comment to better reflect what happens.

[msarett, 2016-10-17 13:39:03]

https://codereview.chromium.org/2045293002/diff/260001/gyp/codec.gyp
File gyp/codec.gyp (left):

https://codereview.chromium.org/2045293002/diff/260001/gyp/codec.gyp#oldcode20
gyp/codec.gyp:20: 'giflib.gyp:giflib',
On 2016/10/14 19:55:59, scroggo_chromium wrote:
> On 2016/10/14 14:58:50, msarett wrote:
> > Can this also be removed from our android_deps?
> 
> Where are you looking? The only place I see it, if I
> 
> $ git grep giflib.gyp
> 
> is in gyp/images.gyp. And that is still needed for SkGIFMovie.cpp. I know
> there's been a lot of hate for SkMovie, but I think the simplest way to stop
> requiring giflib is to switch that file to use SkCodec.
> 

You're right, my mistake.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (right):

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:295: //   writeTransparentPixels will be false in this
case, based on
writeTransparentPixels?

Edit: Ok I understand.  We could potentially modify the codec to actually write
transprent pixels - then we don't need to fill here.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:299: //   afterwards for an incomplete image. (FIXME:
Does the first pass
Don't think so.  Interlaced gif is just out of order rows...  So we don't fill
all the rows until we are done with the image.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:306: const int scaledHeight =
get_scaled_dimension(dstInfo.height(), fSwizzler->sampleY());
nit: 100 chars

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:333: const GIFFrameContext* prevFrame =
fReader->frameContext(frameContext->getRequiredFrame());
nit: 100 chars

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:342: // fSwizzler->fill() would fill to the scaled
width of the frame, but we want to fill
nit: 100 chars

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:443: // the range we will draw to the destination.
nit: wording?

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:459: if (potentialRow < 0 ) {
Does fSwizzler->rowNeeded() not handle this?  Should it handle this?

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:515: // Although onGetFillValue returns a uint64_t, we
only use the low eight bits.
I guess this works because we are either in the kIndex8 case, or the fill value
is all zeros anyway?

[scroggo_chromium, 2016-10-17 17:22:38]

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (right):

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:295: //   writeTransparentPixels will be false in this
case, based on
On 2016/10/17 13:39:03, msarett wrote:
> writeTransparentPixels?
> 
> Edit: Ok I understand.  We could potentially modify the codec to actually
write
> transprent pixels - then we don't need to fill here.

Yep. Although I think we can take this further - even if we do fill here, we
still may be able to writeTransparentPixels - it is probably faster to use the
swizzler even though it's writing zeroes...

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:299: //   afterwards for an incomplete image. (FIXME:
Does the first pass
On 2016/10/17 13:39:03, msarett wrote:
> Don't think so.  Interlaced gif is just out of order rows...  So we don't fill
> all the rows until we are done with the image.

IIUC, the first pass is every 8th row (in order). The code in
GIFLZWContext::outputRow (in GIFImageReader [1]) duplicates lines on the first
few passes. I could imagine they cover all the rows (for the first pass, it
duplicates the row 7 times), although I haven't completely convinced myself that
it does.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:306: const int scaledHeight =
get_scaled_dimension(dstInfo.height(), fSwizzler->sampleY());
On 2016/10/17 13:39:03, msarett wrote:
> nit: 100 chars

Done.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:333: const GIFFrameContext* prevFrame =
fReader->frameContext(frameContext->getRequiredFrame());
On 2016/10/17 13:39:03, msarett wrote:
> nit: 100 chars

Done.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:342: // fSwizzler->fill() would fill to the scaled
width of the frame, but we want to fill
On 2016/10/17 13:39:03, msarett wrote:
> nit: 100 chars

Done.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:443: // the range we will draw to the destination.
On 2016/10/17 13:39:03, msarett wrote:
> nit: wording?

This comment (and the code to which it refers) has been removed.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:459: if (potentialRow < 0 ) {
On 2016/10/17 13:39:03, msarett wrote:
> Does fSwizzler->rowNeeded() not handle this?  Should it handle this?

No, and I don't think so.

This was necessary when I was subtracting fFirstRow. But I have since realized
that this is unnecessary; once I land crrev.com/2420843003 (PTAL, BTW),
fFirstRow no longer exists - SkGifCodec does not and should not support subsets.

https://codereview.chromium.org/2045293002/diff/280001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:515: // Although onGetFillValue returns a uint64_t, we
only use the low eight bits.
On 2016/10/17 13:39:03, msarett wrote:
> I guess this works because we are either in the kIndex8 case, or the fill
value
> is all zeros anyway?

Correct. Updated the comment.

[scroggo_chromium, 2016-10-17 20:53:56]

https://codereview.chromium.org/2045293002/diff/340001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2045293002/diff/340001/BUILD.gn#newcode347
BUILD.gn:347: "src/codec/GIFImageReader.cpp",
I've clearly done something wrong here. When I upload, I get the following
message:

`gn format --dry-run BUILD.gn` failed, try
	gn format BUILD.gn

When I try to run that, I get:

scroggo@scroggo-linux4:/usr/local/google/skia$ gn format BUILD.gn 
gn.py: Could not find gn executable at:
/usr/local/google/skia/./buildtools/linux64/gn

Will fix before submitting...

[scroggo_chromium, 2016-10-17 21:16:19]

https://codereview.chromium.org/2045293002/diff/340001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2045293002/diff/340001/BUILD.gn#newcode347
BUILD.gn:347: "src/codec/GIFImageReader.cpp",
On 2016/10/17 20:53:56, scroggo_chromium wrote:
> I've clearly done something wrong here. When I upload, I get the following
> message:
> 
> `gn format --dry-run BUILD.gn` failed, try
> 	gn format BUILD.gn
> 
> When I try to run that, I get:
> 
> scroggo@scroggo-linux4:/usr/local/google/skia$ gn format BUILD.gn 
> gn.py: Could not find gn executable at:
> /usr/local/google/skia/./buildtools/linux64/gn
> 
> Will fix before submitting...

Fixed.

[msarett, 2016-10-18 20:04:36]

lgtm

https://codereview.chromium.org/2045293002/diff/360001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2045293002/diff/360001/BUILD.gn#newcode343
BUILD.gn:343: public_defines = [ "SK_HAS_GIF_LIBRARY" ]
Now that we don't depend on giflib, can we get rid of this define and just
always compile SkGifCodec?

Might need to grep for SK_HAS_GIF_LIBRARY and delete it from a bunch of build
files...

[scroggo_chromium, 2016-10-18 20:41:47]

https://codereview.chromium.org/2045293002/diff/360001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2045293002/diff/360001/BUILD.gn#newcode343
BUILD.gn:343: public_defines = [ "SK_HAS_GIF_LIBRARY" ]
On 2016/10/18 20:04:36, msarett wrote:
> Now that we don't depend on giflib, can we get rid of this define and just
> always compile SkGifCodec?
> 
> Might need to grep for SK_HAS_GIF_LIBRARY and delete it from a bunch of build
> files...

Done.

https://codereview.chromium.org/2045293002/diff/380001/cmake/CMakeLists.txt
File cmake/CMakeLists.txt (right):

https://codereview.chromium.org/2045293002/diff/380001/cmake/CMakeLists.txt#n...
cmake/CMakeLists.txt:187: add_definitions(-DSK_HAS_GIF_LIBRARY)
I left this bit in because SkGIFMovie still requires giflib. When we switch to
generating our Android makefiles with GN we'll need to add that to the BUILD.gn
file.

[scroggo_chromium, 2016-10-18 20:42:02]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 20:42:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scroggo_chromium, 2016-10-18 20:42:20]

reed@, can you take a look at the public API?

[commit-bot: I haz the power, 2016-10-18 20:42:55]

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

[commit-bot: I haz the power, 2016-10-18 20:42:57]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-Clang-arm-Debug-GN_Android-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)
  Build-Ubuntu-Clang-arm64-Debug-GN_Android-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)
  Build-Ubuntu-Clang-mips64el-Debug-GN_Android-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-m...)
  Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[scroggo_chromium, 2016-10-18 21:09:45]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-18 21:09:46]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com
Link to the patchset: https://codereview.chromium.org/2045293002/#ps420001
(title: "Fixes for BUILD.gn")

[commit-bot: I haz the power, 2016-10-18 21:10:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 21:12:06]

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

[commit-bot: I haz the power, 2016-10-18 21:12:07]

Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
  skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[cblume, 2016-10-18 21:57:32]

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:447: if (reqFrame != SkCodec::kIndependentFrame && reqFrame ==
cachedFrame
On 2016/10/14 19:55:59, scroggo_chromium wrote:
> On 2016/10/14 16:52:35, cblume wrote:
> > I wonder if we can find a better name than kIndependentFrame.
> > 
> > I understand it is an invalid frame index which tells us that there is no
> > required frame. But it is *this* frame which is independent, not the
required
> > frame. So this ends up reading in a bit of a strange way.
> > 
> > This might sound silly but I think it would be easier to read as:
> > if (reqFrame != SkCodec::kInvalidFrameIndex
> > 
> > We could even provide:
> > bool isIndependentFrame(size_t index) {
> >     const size_t reqFrame = frameInfos[i].fRequiredFrame;
> >     return reqFrame == SkCodec::kInvalidFrameIndex;
> > }
> 
> I understand your confusion, and I'm not opposed to the name
kInvalidFrameIndex
> (maybe something simpler like kNone? Blink uses kNotFound, which doesn't quite
> sound right to me). I've added a FIXME for now, until we agree on a name.
reed@?

I agree that I'm not a fan of kNotFound. It wasn't "not found". It doesn't
exist.

I'm cool with kNone. That is short and sweet and clear.
if (reqFrame != SkCodec::kNone
easy

[reed1, 2016-10-20 17:00:19]

1. nice cl comment -- paste it into a design doc?
2. can we just add fFrameIndex and fHasPrevFrame into Options with default
values?

[scroggo_chromium, 2016-10-20 17:01:43]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

[scroggo_chromium, 2016-10-20 17:20:16]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Read the background index, which is potentially used to fill index8
  (matches old Android behavior)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

[scroggo_chromium, 2016-10-20 17:40:45]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

[scroggo_chromium, 2016-10-20 17:46:56]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
http://giflib.sourceforge.net/whatsinagif/bits_and_bytes.html#logical_screen_...

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

[scroggo_chromium, 2016-10-20 17:47:34]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
http://giflib.sourceforge.net/whatsinagif/bits_and_bytes.html#logical_screen_...

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
https://docs.google.com/a/google.com/document/d/12Qhf9T92MWfdWujQwCIjhCO3sw6p...

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

[scroggo_chromium, 2016-10-20 17:57:13]

On 2016/10/20 17:00:19, reed1 wrote:
> 1. nice cl comment -- paste it into a design doc?

It's not word for word, but I've updated the design doc to reflect comments here
(and changes to the code since I wrote the doc). I've also added a link to the
design doc in the description.

> 2. can we just add fFrameIndex and fHasPrevFrame into Options with default
> values?

That was actually my initial design. I modified it in response to Matt's
comment:

https://codereview.chromium.org/2045293002/diff/20001/include/codec/SkCodec.h...

I do not have a strong preference, but the nice thing about having its own
struct is that it keeps like fields together. Patch set 2 shows them in the
Options object directly. Does that look better to you?

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/2045293002/diff/260001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:447: if (reqFrame != SkCodec::kIndependentFrame && reqFrame ==
cachedFrame
On 2016/10/18 21:57:32, cblume wrote:
> On 2016/10/14 19:55:59, scroggo_chromium wrote:
> > On 2016/10/14 16:52:35, cblume wrote:
> > > I wonder if we can find a better name than kIndependentFrame.
> > > 
> > > I understand it is an invalid frame index which tells us that there is no
> > > required frame. But it is *this* frame which is independent, not the
> required
> > > frame. So this ends up reading in a bit of a strange way.
> > > 
> > > This might sound silly but I think it would be easier to read as:
> > > if (reqFrame != SkCodec::kInvalidFrameIndex
> > > 
> > > We could even provide:
> > > bool isIndependentFrame(size_t index) {
> > >     const size_t reqFrame = frameInfos[i].fRequiredFrame;
> > >     return reqFrame == SkCodec::kInvalidFrameIndex;
> > > }
> > 
> > I understand your confusion, and I'm not opposed to the name
> kInvalidFrameIndex
> > (maybe something simpler like kNone? Blink uses kNotFound, which doesn't
quite
> > sound right to me). I've added a FIXME for now, until we agree on a name.
> reed@?
> 
> I agree that I'm not a fan of kNotFound. It wasn't "not found". It doesn't
> exist.
> 
> I'm cool with kNone. That is short and sweet and clear.
> if (reqFrame != SkCodec::kNone
> easy

Done.

https://codereview.chromium.org/2045293002/diff/480001/src/codec/GIFImageRead...
File src/codec/GIFImageReader.cpp (right):

https://codereview.chromium.org/2045293002/diff/480001/src/codec/GIFImageRead...
src/codec/GIFImageReader.cpp:730: const bool isLocalColormapDefined =
currentComponent[8] & 0x80;
msarett@, PTAL at the new changes related to the background index etc. I've left
off specifying color/alpha types for frames beyond the first for now, but I
think this makes things a lot saner.

[msarett, 2016-10-20 18:20:37]

https://codereview.chromium.org/2045293002/diff/500001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (right):

https://codereview.chromium.org/2045293002/diff/500001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:97: const auto alphaType = reader->firstFrameHasAlpha()
? kPremul_SkAlphaType
Can we recommend kUnpremul here?

I *think* we want to train clients (especially ourselves) to ask for kUnpremul
decodes.  Premultiplying (as a part of a color correct pipeline) will likely not
be something we do at decode time.

Also this is the choice that we make in SkEncodedInfo::makeImageInfo().

https://codereview.chromium.org/2045293002/diff/500001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:99: const auto imageInfo =
SkImageInfo::Make(reader->screenWidth(), reader->screenHeight(),
I missed this in the earlier patch sets, but we should mark the color space as
sRGB (not nullptr).  Can you add
SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named) to the Make call?

[scroggo, 2016-10-20 18:34:11]

On 2016/10/20 17:00:19, reed1 wrote:
> 2. can we just add fFrameIndex and fHasPrevFrame into Options with default
> values?

I went ahead and made this change (see patch set 27). Again, I don't have a
strong preference, so let me know which you prefer.

https://codereview.chromium.org/2045293002/diff/500001/src/codec/SkGifCodec.cpp
File src/codec/SkGifCodec.cpp (right):

https://codereview.chromium.org/2045293002/diff/500001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:97: const auto alphaType = reader->firstFrameHasAlpha()
? kPremul_SkAlphaType
On 2016/10/20 18:20:37, msarett wrote:
> Can we recommend kUnpremul here?
> 
> I *think* we want to train clients (especially ourselves) to ask for kUnpremul
> decodes.  Premultiplying (as a part of a color correct pipeline) will likely
not
> be something we do at decode time.
> 
> Also this is the choice that we make in SkEncodedInfo::makeImageInfo().

Done.

https://codereview.chromium.org/2045293002/diff/500001/src/codec/SkGifCodec.c...
src/codec/SkGifCodec.cpp:99: const auto imageInfo =
SkImageInfo::Make(reader->screenWidth(), reader->screenHeight(),
On 2016/10/20 18:20:37, msarett wrote:
> I missed this in the earlier patch sets, but we should mark the color space as
> sRGB (not nullptr).  Can you add
> SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named) to the Make call?

As discussed in person, it looks like we previously used nullptr. I'd rather
make this a separate change, preferably with tests.

I've added a FIXME, though.

[msarett, 2016-10-20 20:33:24]

Still lgtm

[cblume, 2016-10-21 20:11:07]

lgtm

[reed1, 2016-10-24 13:10:58]

lgtm

[scroggo_chromium, 2016-10-24 13:12:49]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new struct to SkCodec::Options (MultiFrameOptions) containing
the following fields:
- fIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.MultiFrameOptions->fIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.MultiFrameOptions->fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
https://docs.google.com/a/google.com/document/d/12Qhf9T92MWfdWujQwCIjhCO3sw6p...

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new fields to SkCodec::Options:
- fFrameIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.fFrameIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fFrameIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
https://docs.google.com/a/google.com/document/d/12Qhf9T92MWfdWujQwCIjhCO3sw6p...

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

[scroggo_chromium, 2016-10-24 13:14:07]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-24 13:14:07]

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

[commit-bot: I haz the power, 2016-10-24 13:14:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 13:15:41]

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

[commit-bot: I haz the power, 2016-10-24 13:15:46]

Try jobs failed on following builders:
  skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[scroggo_chromium, 2016-10-24 13:55:04]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-24 13:55:04]

The patchset sent to the CQ was uploaded after l-g-t-m from cblume@chromium.org,
msarett@google.com, reed@google.com
Link to the patchset: https://codereview.chromium.org/2045293002/#ps600001
(title: "Move GIFImageReader to third_party")

[commit-bot: I haz the power, 2016-10-24 13:55:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 13:57:22]

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

[commit-bot: I haz the power, 2016-10-24 13:57:24]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)

[scroggo_chromium, 2016-10-24 14:34:58]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-24 14:34:59]

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

[commit-bot: I haz the power, 2016-10-24 14:35:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 14:38:34]

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

[commit-bot: I haz the power, 2016-10-24 14:38:35]

Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[scroggo_chromium, 2016-10-24 14:46:48]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-24 14:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from cblume@chromium.org,
msarett@google.com, reed@google.com
Link to the patchset: https://codereview.chromium.org/2045293002/#ps640001
(title: "Stop preventing copy elision")

[commit-bot: I haz the power, 2016-10-24 14:46:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 14:52:42]

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

[commit-bot: I haz the power, 2016-10-24 14:52:44]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[scroggo_chromium, 2016-10-24 15:07:38]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-24 15:07:38]

The patchset sent to the CQ was uploaded after l-g-t-m from cblume@chromium.org,
msarett@google.com, reed@google.com
Link to the patchset: https://codereview.chromium.org/2045293002/#ps660001
(title: "\#include <alorithm> for std::min")

[commit-bot: I haz the power, 2016-10-24 15:07:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 15:13:50]

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

[commit-bot: I haz the power, 2016-10-24 15:13:52]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[scroggo_chromium, 2016-10-24 15:15:47]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-24 15:15:47]

The patchset sent to the CQ was uploaded after l-g-t-m from cblume@chromium.org,
msarett@google.com, reed@google.com
Link to the patchset: https://codereview.chromium.org/2045293002/#ps680001
(title: "Include algorithm elsewhere")

[commit-bot: I haz the power, 2016-10-24 15:15:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 15:24:40]

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

[commit-bot: I haz the power, 2016-10-24 15:24:42]

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

[scroggo_chromium, 2016-10-24 15:34:51]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-10-24 15:34:52]

The patchset sent to the CQ was uploaded after l-g-t-m from cblume@chromium.org,
msarett@google.com, reed@google.com
Link to the patchset: https://codereview.chromium.org/2045293002/#ps700001
(title: "Fix a test - we now draw transparent background for missing color
table")

[commit-bot: I haz the power, 2016-10-24 15:35:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 16:03:28]

Description was changed from

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new fields to SkCodec::Options:
- fFrameIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.fFrameIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fFrameIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
https://docs.google.com/a/google.com/document/d/12Qhf9T92MWfdWujQwCIjhCO3sw6p...

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

to

==========
Add support for multiple frames in SkCodec

Add an interface to decode frames beyond the first in SkCodec, and
add an implementation for SkGifCodec.

Add getFrameData to SkCodec. This method reads ahead in the stream
to return a vector containing meta data about each frame in the image.
This is not required in order to decode frames beyond the first, but
it allows a client to learn extra information:
- how long the frame should be displayed
- whether a frame should be blended with a prior frame, allowing the
  client to provide the prior frame to speed up decoding

Add a new fields to SkCodec::Options:
- fFrameIndex
- fHasPriorFrame

The API is designed so that SkCodec never caches frames. If a
client wants a frame beyond the first, they specify the frame in
Options.fFrameIndex. If the client does not have the
frame's required frame (the frame that this frame must be blended on
top of) cached, they pass false for
Options.fHasPriorFrame. Unless the frame is
independent, the codec will then recursively decode all frames
necessary to decode fFrameIndex. If the client has the required frame
cached, they can put it in the dst they pass to the codec, and the
codec will only draw fFrameIndex onto it.

Replace SkGifCodec's scanline decoding support with progressive
decoding, and update the tests accordingly.

Implement new APIs in SkGifCodec. Instead of using gif_lib, use
GIFImageReader, imported from Chromium (along with its copyright
headers) with the following changes:
- SkGifCodec is now the client
- Replace blink types
- Combine GIFColorMap::buildTable and ::getTable into a method that
  creates and returns an SkColorTable
- Input comes from an SkStream, instead of a SegmentReader. Add
  SkStreamBuffer, which buffers the (potentially partial) stream in
  order to decode progressively.
  (FIXME: This requires copying data that previously was read directly
  from the SegmentReader. Does this hurt performance? If so, can we
  fix it?)
- Remove UMA code
- Instead of reporting screen width and height to the client, allow the
  client to query for it
- Fail earlier if the first frame AND screen have size of zero
- Compute required previous frame when adding a new one
- Move GIFParseQuery from GIFImageDecoder to GIFImageReader
- Allow parsing up to a specific frame (to skip parsing the rest of the
  stream if a client only wants the first frame)
- Compute whether the first frame has alpha and supports index 8, to
  create the SkImageInfo. This happens before reporting that the size
  has been decoded.

Add GIFImageDecoder::haveDecodedRow to SkGifCodec, imported from
Chromium (along with its copyright header), with the following changes:
- Add support for sampling
- Use the swizzler
- Keep track of the rows decoded
- Do *not* keep track of whether we've seen alpha

Remove SkCodec::kOutOfOrder_SkScanlineOrder, which was only used by GIF
scanline decoding.

Call onRewind even if there is no stream (SkGifCodec needs to clear its
decoded state so it will decode from the beginning).

Add a method to SkSwizzler to access the offset into the dst, taking
subsetting into account.

Add a GM that animates a GIF.
Add tests for the new APIs.

*** Behavior changes:
* Previously, we reported that an image with a subset frame and no transparent
index was opaque and used the background index (if present) to fill the
background. This is necessary in order to support index 8, but it does not
match viewers/browsers I have seen. Examples:
- Chromium and Gimp render the background transparent
- Firefox, Safari, Linux Image Viewer, Safari Preview clip to the frame (for
  a single frame image)
This CL matches Chromium's behavior and renders the background transparent.
This allows us to have consistent behavior across products and simplifies
the code (relative to what we would have to do to continue the old behavior
on Android). It also means that we will no longer support index 8 for some
GIFs.
* Stop checking for GIFSTAMP - all GIFs should be either 89a or 87a.
This matches Chromium. I suspect that bugs would have been reported if valid
GIFs started with "GIFVER" instead of "GIF89a" or "GIF87a" (but did not decode
in Chromium).

*** Future work not included in this CL:
* Move some checks out of haveDecodedRow, since they are the same for the
  entire frame e.g.
- intersecting the frameRect with the full image size
- whether there is a color table
* Change when we write transparent pixels
- In some cases, Chromium deemed this unnecessary, but I suspect it is slower
  than the fallback case. There will continue to be cases where we should
  *not* write them, but for e.g. the first pass where we have already
  cleared to transparent (which we may also be able to skip) writing the
  transparent pixels will not make anything incorrect.
* Report color type and alpha type per frame
- Depending on alpha values, disposal methods, frame rects, etc, subsequent
  frames may have different properties than the first.
* Skip copies of the encoded data
- We copy the encoded data in case the stream is one that cannot be rewound,
  so we can parse and then decode (possibly not immediately). For some input
  streams, this is unnecessary.
  - I was concerned this cause a performance regression, but on average the
    new code is faster than the old for the images I tested [1].
  - It may cause a performance regression for Chromium, though, where we can
    always move back in the stream, so this should be addressed.

Design doc:
https://docs.google.com/a/google.com/document/d/12Qhf9T92MWfdWujQwCIjhCO3sw6p...

[1]
https://docs.google.com/a/google.com/spreadsheets/d/19V-t9BfbFw5eiwBTKA1qOBkZ...

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

Committed:
https://skia.googlesource.com/skia/+/19b91531e912283d237435d94516575b28713cba
==========

[commit-bot: I haz the power, 2016-10-24 16:03:30]

Committed patchset #36 (id:700001) as
https://skia.googlesource.com/skia/+/19b91531e912283d237435d94516575b28713cba