Implement SkGifCodec::onSkipScanlines()

Implement SkGifCodec::onSkipScanlines()

This should give a performance improvment because we are able to skip
swizzling pixels.

This should also fix valgrind failures caused by color table lookups
with uninitialized memory.

BUG=skia:4270
Committed: https://skia.googlesource.com/skia/+/72261c0afd4f1b0718bda05213a769120baa55c9

[msarett, 2015-11-19 21:35:05]

Patchset #1 (id:1) has been deleted

[msarett, 2015-11-19 21:37:22]

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

[msarett, 2015-11-19 21:37:23]

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:531: this->handleScanlineFrame(count,
&rowsBeforeFrame, &rowsInFrame);
We could do without the count parameter since we set rowsInFrame to count before
calling.  But I'm not sure if that's too confusing.

[msarett, 2015-11-19 21:39:49]

Description was changed from

==========
Implement SkGifCodec::onSkipScanlines()

This should give a performance improvment because we are able to skip
swizzling pixels.

This should also fix valgrind failures caused by color table lookups
with uninitialized memory.

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

to

==========
Implement SkGifCodec::onSkipScanlines()

This should give a performance improvment because we are able to skip
swizzling pixels.

This should also fix valgrind failures caused by color table lookups
with uninitialized memory.

BUG=skia:4270
==========

[scroggo, 2015-11-19 22:23:06]

lgtm

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:509: void SkGifCodec::handleScanlineFrame(int
count, int* rowsBeforeFrame, int* rowsInFrame) {
SkASSERT(fFrameIsSubset);

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:514: *rowsBeforeFrame = SkTMin(count, SkTMax(0,
fFrameRect.top() - INHERITED::nextScanline()));
Would it make sense to cache the value of nextScanline locally?

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:517: int rowsLeft = count - *rowsBeforeFrame;
could be const?

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:531: this->handleScanlineFrame(count,
&rowsBeforeFrame, &rowsInFrame);
On 2015/11/19 21:37:23, msarett wrote:
> We could do without the count parameter since we set rowsInFrame to count
before
> calling.  But I'm not sure if that's too confusing.

Either way. We sometimes use the same variable as input/output, but it might be
clearer to use separate variables.

As an alternative, if you always called handleScanlineFrame, and it checked
fFrameIsSubset, then you could let handleScanlineFrame always initialize it. I
suppose a downside is that you'll then check fFrameIsSubset twice.

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgif.h
File src/codec/SkCodec_libgif.h (right):

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.h:158: * the image frame and how many of the "count"
lines are acutally inside
actually*

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.h:162: * a subset of the size of the entire image.
i.e. fFrameIsSubset

Alternatively, this method could do the following:

if (fFrameIsSubset) {
    // Calculate the output vars
} else {
    // Set them to 0 and count
}

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.h:165: * @param rowsBeforeFrame The number of lines
before we reach the top of
Maybe specify that these are output variables?

[msarett, 2015-11-19 22:39:23]

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:509: void SkGifCodec::handleScanlineFrame(int
count, int* rowsBeforeFrame, int* rowsInFrame) {
On 2015/11/19 22:23:06, scroggo wrote:
> SkASSERT(fFrameIsSubset);

Acknowledged.

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:514: *rowsBeforeFrame = SkTMin(count, SkTMax(0,
fFrameRect.top() - INHERITED::nextScanline()));
On 2015/11/19 22:23:06, scroggo wrote:
> Would it make sense to cache the value of nextScanline locally?

Yeah I think so.

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:517: int rowsLeft = count - *rowsBeforeFrame;
On 2015/11/19 22:23:06, scroggo wrote:
> could be const?

Done.

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:531: this->handleScanlineFrame(count,
&rowsBeforeFrame, &rowsInFrame);
On 2015/11/19 22:23:06, scroggo wrote:
> On 2015/11/19 21:37:23, msarett wrote:
> > We could do without the count parameter since we set rowsInFrame to count
> before
> > calling.  But I'm not sure if that's too confusing.
> 
> Either way. We sometimes use the same variable as input/output, but it might
be
> clearer to use separate variables.
> 
> As an alternative, if you always called handleScanlineFrame, and it checked
> fFrameIsSubset, then you could let handleScanlineFrame always initialize it. I
> suppose a downside is that you'll then check fFrameIsSubset twice. 

Yeah I thought about doing it that way...

I think you're right, I prefer that strategy.

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgif.h
File src/codec/SkCodec_libgif.h (right):

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.h:158: * the image frame and how many of the "count"
lines are acutally inside
On 2015/11/19 22:23:06, scroggo wrote:
> actually*

Done.

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.h:162: * a subset of the size of the entire image.
On 2015/11/19 22:23:06, scroggo wrote:
> i.e. fFrameIsSubset
> 
> Alternatively, this method could do the following:
> 
> if (fFrameIsSubset) {
>     // Calculate the output vars
> } else {
>     // Set them to 0 and count
> }

Yeah, let's do it this way.

https://codereview.chromium.org/1460073002/diff/20001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.h:165: * @param rowsBeforeFrame The number of lines
before we reach the top of
On 2015/11/19 22:23:06, scroggo wrote:
> Maybe specify that these are output variables?

Done.

[scroggo, 2015-11-19 22:42:44]

LGTM

[msarett, 2015-11-19 22:43:15]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-11-19 22:43:26]

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

[commit-bot: I haz the power, 2015-11-19 22:45:59]

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

[commit-bot: I haz the power, 2015-11-19 22:45:59]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[msarett, 2015-11-19 23:13:34]

The CQ bit was checked by msarett@google.com

[msarett, 2015-11-19 23:13:34]

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/1460073002/#ps60001
(title: "onSkipScanlines() returns a bool")

[commit-bot: I haz the power, 2015-11-19 23:13:39]

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

[commit-bot: I haz the power, 2015-11-19 23:29:29]

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