Add currScanline() getter to SkCodec API

Add currScanline() getter to SkCodec API

This is more correct than using nextScanline() for the
SkGifCodec scanline decoder (since we will get a strange
result in the interlaced case) and is necessary if we want
to add scanline decoding to SkIcoCodec.

This does not actually fix bugs or change behavior.

BUG=skia:

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

[msarett, 2015-12-01 23:23:04]

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

[msarett, 2015-12-01 23:23:04]

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

https://codereview.chromium.org/1489163002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:511: const int currRow =
this->INHERITED::nextScanline();
Technically this works...

What I intended was for this call to return fCurrScanline.  What it actually
does is call SkGifCodec::onOutputScanline(fCurrScanline).

For kTopDown this works because SkGifCodec::onOutputScanline(fCurrScanline)
returns fCurrScanline.

For kOutOfOrder (interlaced) this works because we only really need to know
whether the current row is inside or outside of the image frame. 
SkGifCodec::onOutputScanline(fCurrScanline) gets this part right at least.  But
it also makes an unnecessary call to get_output_row_interlaced which will give
us a strange location within the image frame (this doesn't cause a problem
because we only ever get or skip one row at a time for kOutOfOrder images).

Regardless, it's simpler and more efficient to just call currScanline().

[scroggo, 2015-12-02 15:24:41]

lgtm

> This is useful for the SkGifCodec scanline decoder and for
> SkIcoCodec (if we decide to create a scanline decoder for ICOs).

Rather than "this is useful," maybe this should say something like "this is more
correct," and a note that this does not actually fix any bugs/change behavior?

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

https://codereview.chromium.org/1489163002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:516: void updateNextScanline(int newY) { fCurrScanline =
newY; }
Maybe this should be updateCurrScanline, to match currScanline? (It currently
matches nextScanline, but really it seems more analogous to currScanline.)

https://codereview.chromium.org/1489163002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:522: int currScanline() const { return fCurrScanline; }
Add some comments? We now have:

currScanline()
nextScanline()
outputScanline()

It would be nice to distinguish them.

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

https://codereview.chromium.org/1489163002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:511: const int currRow =
this->INHERITED::nextScanline();
On 2015/12/01 23:23:04, msarett wrote:
> Technically this works...
> 
> What I intended was for this call to return fCurrScanline.  What it actually
> does is call SkGifCodec::onOutputScanline(fCurrScanline).
> 
> For kTopDown this works because SkGifCodec::onOutputScanline(fCurrScanline)
> returns fCurrScanline.
> 
> For kOutOfOrder (interlaced) this works because we only really need to know
> whether the current row is inside or outside of the image frame. 
> SkGifCodec::onOutputScanline(fCurrScanline) gets this part right at least. 
But
> it also makes an unnecessary call to get_output_row_interlaced which will give
> us a strange location within the image frame (this doesn't cause a problem
> because we only ever get or skip one row at a time for kOutOfOrder images).
> 
> Regardless, it's simpler and more efficient to just call currScanline().

The old code is also weird because SkCodec::nextScanline is not virtual (maybe
it used to be, or we were calling another function, and the INHERITED:: stuck
around).

So, IIUC, for an interlaced subset frame gif, the old code returns the wrong
number, but this doesn't matter, because it correctly returns a number inside
the frame?

[msarett, 2015-12-02 15:35:25]

Description was changed from

==========
Add currScanline() getter to SkCodec API

This is useful for the SkGifCodec scanline decoder and for
SkIcoCodec (if we decide to create a scanline decoder for ICOs).

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

to

==========
Add currScanline() getter to SkCodec API

This is more correct than using nextScanline() for the
SkGifCodec scanline decoder (since we will get a strange
result in the interlaced case) and is necessary if we want
to add scanline decoding to SkIcoCodec.

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

[msarett, 2015-12-02 15:35:57]

Description was changed from

==========
Add currScanline() getter to SkCodec API

This is more correct than using nextScanline() for the
SkGifCodec scanline decoder (since we will get a strange
result in the interlaced case) and is necessary if we want
to add scanline decoding to SkIcoCodec.

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

to

==========
Add currScanline() getter to SkCodec API

This is more correct than using nextScanline() for the
SkGifCodec scanline decoder (since we will get a strange
result in the interlaced case) and is necessary if we want
to add scanline decoding to SkIcoCodec.

This does not actually fix bugs or change behavior.

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

[msarett, 2015-12-02 15:45:06]

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

https://codereview.chromium.org/1489163002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:516: void updateNextScanline(int newY) { fCurrScanline =
newY; }
On 2015/12/02 15:24:41, scroggo wrote:
> Maybe this should be updateCurrScanline, to match currScanline? (It currently
> matches nextScanline, but really it seems more analogous to currScanline.)

Agreed.

https://codereview.chromium.org/1489163002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:522: int currScanline() const { return fCurrScanline; }
On 2015/12/02 15:24:41, scroggo wrote:
> Add some comments? We now have:
> 
> currScanline()
> nextScanline()
> outputScanline()
> 
> It would be nice to distinguish them.

Yes this is confusing will do.

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

https://codereview.chromium.org/1489163002/diff/1/src/codec/SkCodec_libgif.cp...
src/codec/SkCodec_libgif.cpp:511: const int currRow =
this->INHERITED::nextScanline();
On 2015/12/02 15:24:41, scroggo wrote:
> On 2015/12/01 23:23:04, msarett wrote:
> > Technically this works...
> > 
> > What I intended was for this call to return fCurrScanline.  What it actually
> > does is call SkGifCodec::onOutputScanline(fCurrScanline).
> > 
> > For kTopDown this works because SkGifCodec::onOutputScanline(fCurrScanline)
> > returns fCurrScanline.
> > 
> > For kOutOfOrder (interlaced) this works because we only really need to know
> > whether the current row is inside or outside of the image frame. 
> > SkGifCodec::onOutputScanline(fCurrScanline) gets this part right at least. 
> But
> > it also makes an unnecessary call to get_output_row_interlaced which will
give
> > us a strange location within the image frame (this doesn't cause a problem
> > because we only ever get or skip one row at a time for kOutOfOrder images).
> > 
> > Regardless, it's simpler and more efficient to just call currScanline().
> 
> The old code is also weird because SkCodec::nextScanline is not virtual (maybe
> it used to be, or we were calling another function, and the INHERITED:: stuck
> around).
> 
> So, IIUC, for an interlaced subset frame gif, the old code returns the wrong
> number, but this doesn't matter, because it correctly returns a number inside
> the frame?

Yes that is correct.

[scroggo, 2015-12-02 16:28:30]

lgtm

[msarett, 2015-12-02 21:14:59]

PTAL

I changed the comments to reflect a behavior that SkIcoCodec may depend on.

[scroggo, 2015-12-02 21:29:37]

On 2015/12/02 21:14:59, msarett wrote:
> PTAL
> 
> I changed the comments to reflect a behavior that SkIcoCodec may depend on.

Still lgtm at patch set 3

[msarett, 2015-12-03 19:53:30]

Derek, can you take a look at the API?

[djsollen, 2015-12-03 20:10:02]

api lgtm

[msarett, 2015-12-03 20:10:33]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-12-03 20:10:41]

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

[commit-bot: I haz the power, 2015-12-03 20:23:45]

Description was changed from

==========
Add currScanline() getter to SkCodec API

This is more correct than using nextScanline() for the
SkGifCodec scanline decoder (since we will get a strange
result in the interlaced case) and is necessary if we want
to add scanline decoding to SkIcoCodec.

This does not actually fix bugs or change behavior.

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

to

==========
Add currScanline() getter to SkCodec API

This is more correct than using nextScanline() for the
SkGifCodec scanline decoder (since we will get a strange
result in the interlaced case) and is necessary if we want
to add scanline decoding to SkIcoCodec.

This does not actually fix bugs or change behavior.

BUG=skia:

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

[commit-bot: I haz the power, 2015-12-03 20:23:46]

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