Interlaced gifs without the iterator

Interlaced gifs without the iterator

BUG=skia:

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

[msarett, 2015-08-24 16:13:59]

I think it might be useful to consider this change separately from the creation
of the gif scanline decoder, even though this change was motivated by the
scanline decoder.

In the bmp/gif scanline decoders, I'm proposing that we add a const function to
SkScanlineDecoder (getY() or something) that tells the client what scanline it
will get next when the scanlines are out of order.

It is awkward to use the GifInterlaceIter, which has state, with this function. 
It would be better if we had a stateless way to do this.  I've implemented a
function to do this.

Pros of the new function:
*stateless
*IMO is actually simpler than the old approach

Cons of the new function:
*a little slower than the iterator (seems to max out at 2% slower)
*internally giflib uses the iterator approach

I'm not sure if we really need this type of change or not.  I'm interested to
see what you both think.  If we do decide this is a good approach, I can try to
optimize the new function and see if we can get any of the 2% back.

[msarett, 2015-08-24 16:14:47]

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

[msarett, 2015-08-24 16:14:47]

Adding reviewers this time.

[msarett, 2015-08-25 19:26:51]

Derek had an interesting idea for an alternate approach.

At the end of every call to getScanlines(), we advance the iterator and store
the next scanline y-value in a variable.

In calls to getY(), we then just return the variable.  In this way, getY() is
const and can be called multiple times and still return the same value.

But we still get the performance benefit of the iterator.

[scroggo, 2015-08-28 13:42:25]

On 2015/08/25 19:26:51, msarett wrote:
> Derek had an interesting idea for an alternate approach.
> 
> At the end of every call to getScanlines(), we advance the iterator and store
> the next scanline y-value in a variable.
> 
> In calls to getY(), we then just return the variable.  In this way, getY() is
> const and can be called multiple times and still return the same value.
> 
> But we still get the performance benefit of the iterator.

lgtm.

I do not have a strong preference between this version and what I proposed in
your other CL (https://codereview.chromium.org/1305123002/ DIFFERENT ISSUE)

[scroggo, 2015-08-28 13:42:32]

https://codereview.chromium.org/1315583003/diff/1/src/codec/SkCodec_libgif.cpp
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1315583003/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:137: } else if (encodedRow * 4 < height) {
nit: These do not need to be else statements, since we already returned above.

[msarett, 2015-08-28 14:29:43]

Patchset #3 (id:40001) has been deleted

[msarett, 2015-08-28 14:30:36]

https://codereview.chromium.org/1315583003/diff/1/src/codec/SkCodec_libgif.cpp
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1315583003/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:137: } else if (encodedRow * 4 < height) {
On 2015/08/28 13:42:32, scroggo wrote:
> nit: These do not need to be else statements, since we already returned above.

Fixing.

[msarett, 2015-08-28 14:31:12]

The CQ bit was checked by msarett@google.com

[msarett, 2015-08-28 14:31:12]

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

[commit-bot: I haz the power, 2015-08-28 14:31:22]

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

[commit-bot: I haz the power, 2015-08-28 14:36:58]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/d02b99f106702bc739d3aa4e3c21f8a0618cbb51