Merge SkCodec with SkScanlineDecoder

Merge SkCodec with SkScanlineDecoder

Benefits:
- This mimics other decoding APIs (including the ones SkCodec relies
on, e.g. a png_struct, which can be used to decode an entire image or
one line at a time).

- It allows a client to ask us to do what we can do efficiently - i.e.
start from encoded data and either decode the whole thing or scanlines.

- It removes the duplicate methods which appeared in both SkCodec and
SkScanlineDecoder (some of which, e.g. in SkJpegScanlineDecoder, just
call fCodec->sameMethod()).

- It simplifies moving more checks into the base class (e.g. the
examples in skbug.com/4284).

BUG=skia:4175
BUG=skia:4284
=====================================================================

SkScanlineDecoder.h/.cpp:
Removed.

SkCodec.h/.cpp:
Add methods, enums, and variables which were previously in
SkScanlineDecoder.
Default fCurrScanline to -1, as a sentinel that start has not been
called.

General changes:
Convert SkScanlineDecoders to SkCodecs.

General changes in SkCodec subclasses:
Merge SkScanlineDecoder implementation into SkCodec. Most (all?) owned
an SkCodec, so they now call this-> instead of fCodec->.

SkBmpCodec.h/.cpp:
Replace the unused rowOrder method with an override for
onGetScanlineOrder.
Make getDstRow const, since it is called by onGetY, which is const.

SkCodec_libpng.h/.cpp:
Make SkPngCodec an abstract class, with two subclasses which handle
scanline decoding separately (they share code for decoding the entire
image). Reimplement onReallyHasAlpha so that it can return the most
recent result (e.g. after a scanline decode which only decoded part
of the image) or a better answer (e.g. if the whole image is known to
be opaque).
Compute fNumberPasses early, so we know which subclass to instantiate.
Make SkPngInterlaceScanlineDecoder use the base class' fCurrScanline
rather than a separate variable.

CodexTest.cpp:
Add tests for the state changes in SkCodec (need to call start before
decoding scanlines; calling getPixels means that start will need to
be called again before decoding more scanlines).
Add a test which decodes in stripes, currently only used for an
interlaced PNG.

TODO: Add tests for onReallyHasAlpha.

Committed: https://skia.googlesource.com/skia/+/46c574725676b26ada63ac15e42cda309dcd5090

[scroggo, 2015-09-25 16:07:48]

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

[scroggo, 2015-09-25 16:07:50]

Please look at patch set 1 (https://codereview.chromium.org/1365313002/#ps1) - I
made a bunch of comments there which are worth looking at. Patch set 2 just does
some minor cleanups.

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

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:262: Result start(const SkImageInfo& dstInfo, const
SkCodec::Options* options,
I left the name "start" to make the changeover simpler. If this name is too
ambiguous, I previously used the name "prepareForScanlineDecode", which is a
little long. Thoughts?

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:283: if (fCurrScanline < 0) {
Alternatively, I could have checked fDstInfo, which I would need to reset where
I reset fCurrScanline.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:284: return kScanlineDecodingNotStarted;
I know (msarett@) you have a CL out for review which makes this method return a
number of scanlines decoded. Once that lands, maybe this should return 0. It's
kind of too bad we don't provide more information in that case, but maybe it's
okay?

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:470: void updateY(int newY) { fCurrScanline = newY; }
I named this updateY to mirror getY, but I might prefer something that
references "next" or "current" "scanline".

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libgif.h#...
src/codec/SkCodec_libgif.h:176: friend class SkGifScanlineDecoder;
Removed in next patch.

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:165: if (transLessThanFF >= 0) {
If I understand this code correctly, the old code here decided that there was
really alpha if one of the colors in the palette has alpha, regardless of
whether those colors are actually used. This defeats the purpose of the swizzler
function which returns whether a single line had alpha (for the indexed png
case).

Maybe it's unlikely that the client will attempt to decode a non-transparent
piece of a transparent image. We have outstanding questions about whether we
should bother doing the check at all (skbug.com/3582). So long as it's here,
though, I think it makes sense to only modify the alpha if we found no colors
with alpha, to the new state which is known to be opaque.

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.h
File src/codec/SkCodec_libpng.h (right):

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.h#...
src/codec/SkCodec_libpng.h:49: //png_infop info_ptr() { return fInfo_ptr; }
Will remove in next patch set.

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:171: Result rowResult =
this->readRow(fSrcBuffer.get());
The performance of SkWbmpCodec should not be a high priority, but it seems like
we could write a version of onSkipScanlines which just calls readRow. Added a
FIXME.

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:384: SkCodec::Result
SkJpegCodec::initializeSwizzler(const SkImageInfo& info, const SkCodec::Options&
options) {
Will fix line length in next patch set.

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:462: // FIXME: Is it safe to call when it doesn't need
to be called?
Matt, any thoughts? It seems to be safe, since nothing bad apparently happens
when I run tests (at least it doesn't crash...).

But it might be unnecessary. Not sure how expensive jpeg_finish_decompress is if
we already called it, but if it is, we could always skip this if output_scanline
is already height.

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:516: // TODO (msarett): Make this a member function
and avoid reallocating the
Alternatively, we could just rely on the default implementation of
onSkipScanlines ifndef TURBO_HAS_SKIP

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp#newcode151
tests/CodexTest.cpp:151: == SkCodec::kScanlineDecodingNotStarted);
Again, these can check for 0/false once we change the return values, as in
Matt's other CL.

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp#newcode273
tests/CodexTest.cpp:273: // Test interlaced PNG in stripes, similar to DM's
kStripe_Mode
Maybe this test is redundant with kStripe_Mode, but I like it because it allows
me to test locally without searching for an interlaced image I want to test.

It's also not really interlace specific; I wanted to make sure I didn't break
interlacing (since I now use the base class's fCurrScanline instead of a
separate variable), but it could easily be run on other types (and it could use
37, as kStripe_Mode does, if we want to replace it - not sure whether it's more
useful to have here versus run on more images in DM).

I could also move this code into check, which might simplify things a little
bit.

https://codereview.chromium.org/1365313002/diff/1/tools/SkBitmapRegionDecoder...
File tools/SkBitmapRegionDecoderInterface.cpp (right):

https://codereview.chromium.org/1365313002/diff/1/tools/SkBitmapRegionDecoder...
tools/SkBitmapRegionDecoderInterface.cpp:33: // FIXME: Don't we need to check
start now?
Actually, I don't think this changes anything here. Will remove this comment.

[msarett, 2015-09-28 14:48:51]

I'm really happy with this change - I think it makes life a lot easier.

I have a few nits, but most of my comments are just comments.

I'm in favor of landing this ASAP since I have few CLs that need to rebase on
this change :).

lgtm

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

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:241: *  Initialize on the first scanline, with the
specified options.
"Initialize on the first scanline" is a little unclear to me.

Maybe "Prepare for a scanline decode" or "Initialize scanline decoder with the
specified image info and options"?

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:262: Result start(const SkImageInfo& dstInfo, const
SkCodec::Options* options,
On 2015/09/25 16:07:49, scroggo wrote:
> I left the name "start" to make the changeover simpler. If this name is too
> ambiguous, I previously used the name "prepareForScanlineDecode", which is a
> little long. Thoughts?

I like a shorter name but that's because I know what this function does.  I
guess it's not really a clear name unless it includes some reference to
scanlines?

A couple ideas:
prepareScanlines() : A shorter name, and this is kind of uniform with
getScanlines() and skipScanlines(), but I guess you could argue that we're not
really preparing the scanlines themselves.
startScanlineDecode() : Not as clear as prepareForSD, but it's a little shorter.

prepareScanlines() gets my vote, but I'm indifferent on this.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:283: if (fCurrScanline < 0) {
On 2015/09/25 16:07:49, scroggo wrote:
> Alternatively, I could have checked fDstInfo, which I would need to reset
where
> I reset fCurrScanline.

I like what you've chosen to do.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:284: return kScanlineDecodingNotStarted;
On 2015/09/25 16:07:49, scroggo wrote:
> I know (msarett@) you have a CL out for review which makes this method return
a
> number of scanlines decoded. Once that lands, maybe this should return 0. It's
> kind of too bad we don't provide more information in that case, but maybe it's
> okay?

Yeah I think that's ok.  Maybe we can provide more information by also adding an
SkASSERT() before returning kScanlineDecodingNotStarted?

Considering this on its own (without thinking about future CLs), I think this
makes sense as is.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:399: SkASSERT(kNone_SkScanlineOrder !=
this->getScanlineOrder());
Given that the interlaced png scanline decoder now makes use of onGetY(), it
seems odd that we have gone to the trouble of forbidding clients from calling
getY() when decoding interlaced pngs.

It is inefficient to call getScanlines() more than once for interlaced pngs,
which was my original reasoning for discouraging the use of getY().  Stated
another way, calling getY() should be unnecessary if we only plan to
getScanlines() once.

However, I think your use of onGetY() in the SkPngInterlacedSD makes a lot of
sense.  I think maybe we should remove this SkASSERT (and the comment above),
and trust that we are intelligent enough clients of our own code to not misuse
getScanlines() with interlaced pngs.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:470: void updateY(int newY) { fCurrScanline = newY; }
On 2015/09/25 16:07:49, scroggo wrote:
> I named this updateY to mirror getY, but I might prefer something that
> references "next" or "current" "scanline".

I'm in favor of renaming these.

I think my original name for getY() was getCurrScanline() but I felt that was
confusing since it seemed like it should actually decode a scanline.  Maybe that
confusion is avoided with currScanline() or nextScanline()?

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:165: if (transLessThanFF >= 0) {
On 2015/09/25 16:07:49, scroggo wrote:
> If I understand this code correctly, the old code here decided that there was
> really alpha if one of the colors in the palette has alpha, regardless of
> whether those colors are actually used. This defeats the purpose of the
swizzler
> function which returns whether a single line had alpha (for the indexed png
> case).
> 
> Maybe it's unlikely that the client will attempt to decode a non-transparent
> piece of a transparent image. We have outstanding questions about whether we
> should bother doing the check at all (skbug.com/3582). So long as it's here,
> though, I think it makes sense to only modify the alpha if we found no colors
> with alpha, to the new state which is known to be opaque.
> 

I agree with this interpretation.

In this case, will we still check all the alpha values as we set the pixels or
will we skip the check since we know already that the image is opaque?

EDIT: Reading below, it looks like we will still check all of the alpha values. 
Maybe add a FIXME?

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:498: // FIXME: We could split these out based on
subclass.
Yeah it really feels like these should be separate classes, given that we decode
differently and the scanline decoders are already separate.  The shared code at
the top of this function could be in refactored into the base class.

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.h
File src/codec/SkCodec_libpng.h (right):

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.h#...
src/codec/SkCodec_libpng.h:54: enum AlphaState {
I think this is good.

Not in this CL, but maybe in the future, we will want to consider sharing this
with other codecs that might benefit this optimization.

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:171: Result rowResult =
this->readRow(fSrcBuffer.get());
On 2015/09/25 16:07:50, scroggo wrote:
> The performance of SkWbmpCodec should not be a high priority, but it seems
like
> we could write a version of onSkipScanlines which just calls readRow. Added a
> FIXME.

Yes agreed.  We should implement on onSkipScanlines() in all of the codecs,
since we can always do better than just calling onGetScanlines().  I think bmp
and gif have similar FIXMEs.

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:462: // FIXME: Is it safe to call when it doesn't need
to be called?
On 2015/09/25 16:07:50, scroggo wrote:
> Matt, any thoughts? It seems to be safe, since nothing bad apparently happens
> when I run tests (at least it doesn't crash...).
> 
> But it might be unnecessary. Not sure how expensive jpeg_finish_decompress is
if
> we already called it, but if it is, we could always skip this if
output_scanline
> is already height.

It's not expensive to call twice, since it'll realize that it has already done
its work.  It will essentially make a function call and a few checks and do
nothing.

However, I don't think we ever need to call jpeg_finish_decompress().
skbug.com/4040

I was supposed to fix this a while back.

I would suggest landing this as is, and then I'll follow up with a CL that
removes the calls jpeg_finish_decompress().  It'll be nice to document the
potential risks of removing this call separately.

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:516: // TODO (msarett): Make this a member function
and avoid reallocating the
On 2015/09/25 16:07:50, scroggo wrote:
> Alternatively, we could just rely on the default implementation of
> onSkipScanlines ifndef TURBO_HAS_SKIP

Yes definitely!  I think that is a better way to fix this!

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp#newcode170
tests/CodexTest.cpp:170: // Cannot continue to decode scanlines beyond the end
+1 for new tests

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp#newcode273
tests/CodexTest.cpp:273: // Test interlaced PNG in stripes, similar to DM's
kStripe_Mode
On 2015/09/25 16:07:50, scroggo wrote:
> Maybe this test is redundant with kStripe_Mode, but I like it because it
allows
> me to test locally without searching for an interlaced image I want to test.
> 
> It's also not really interlace specific; I wanted to make sure I didn't break
> interlacing (since I now use the base class's fCurrScanline instead of a
> separate variable), but it could easily be run on other types (and it could
use
> 37, as kStripe_Mode does, if we want to replace it - not sure whether it's
more
> useful to have here versus run on more images in DM).
> 
> I could also move this code into check, which might simplify things a little
> bit.

I like this as a separate test - I think it's good to verify this functionality
of scanline decoding for interlaced pngs (and for all top-down image types if we
wanted to generalize this test).  I'm in favor of keeping it and am not really
bothered that it's similar to the DM test.

Did we ever discuss forbidding the client from calling getScanlines() more than
once on interlaced pngs (since it is so inefficient)?  If we wanted to do that,
it would break this test.

It's probably worth noting that the kStripe_Mode test in DM is skipped unless
the image is kTopDown (interlaced pngs are kNone).  There's actually no reason
why the DM test won't work with kNone (it'll just be extremely inefficient).

AFAICT, this test, just like the test in DM, will fail on kBottomUp and
kOutOfOrder images.

IMO, this test is not enough to replace kStripe_Mode in DM.  I think it's
important to keep that for regression testing on future updates of
libjpeg-turbo.  Testing on interesting values of numStripes would help, but it's
also nice to test on all of the jpegs we have on the bots.

[scroggo, 2015-09-28 16:01:53]

Derek, can you look over the API?

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

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:241: *  Initialize on the first scanline, with the
specified options.
On 2015/09/28 14:48:50, msarett wrote:
> "Initialize on the first scanline" is a little unclear to me.
> 
> Maybe "Prepare for a scanline decode" or "Initialize scanline decoder with the
> specified image info and options"?

Sgtm. I think it made a little more sense back when it was on SkScanlineDecoder,
which was only used for scanline decodes.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:262: Result start(const SkImageInfo& dstInfo, const
SkCodec::Options* options,
On 2015/09/28 14:48:50, msarett wrote:
> On 2015/09/25 16:07:49, scroggo wrote:
> > I left the name "start" to make the changeover simpler. If this name is too
> > ambiguous, I previously used the name "prepareForScanlineDecode", which is a
> > little long. Thoughts?
> 
> I like a shorter name but that's because I know what this function does.  I
> guess it's not really a clear name unless it includes some reference to
> scanlines?
> 
> A couple ideas:
> prepareScanlines() : A shorter name, and this is kind of uniform with
> getScanlines() and skipScanlines(), but I guess you could argue that we're not
> really preparing the scanlines themselves.
> startScanlineDecode() : Not as clear as prepareForSD, but it's a little
shorter.
> 
> prepareScanlines() gets my vote, but I'm indifferent on this.

These sound fine with me. I'll wait for API review to do a search and replace
(if any).

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:284: return kScanlineDecodingNotStarted;
On 2015/09/28 14:48:50, msarett wrote:
> On 2015/09/25 16:07:49, scroggo wrote:
> > I know (msarett@) you have a CL out for review which makes this method
return
> a
> > number of scanlines decoded. Once that lands, maybe this should return 0.
It's
> > kind of too bad we don't provide more information in that case, but maybe
it's
> > okay?
> 
> Yeah I think that's ok.  Maybe we can provide more information by also adding
an
> SkASSERT() before returning kScanlineDecodingNotStarted?

I think returning kScanlineDecodingNoStarted is probably enough, but I could be
convinced to add it.

> 
> Considering this on its own (without thinking about future CLs), I think this
> makes sense as is.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:399: SkASSERT(kNone_SkScanlineOrder !=
this->getScanlineOrder());
On 2015/09/28 14:48:50, msarett wrote:
> Given that the interlaced png scanline decoder now makes use of onGetY(), it
> seems odd that we have gone to the trouble of forbidding clients from calling
> getY() when decoding interlaced pngs.
> 
> It is inefficient to call getScanlines() more than once for interlaced pngs,
> which was my original reasoning for discouraging the use of getY().  Stated
> another way, calling getY() should be unnecessary if we only plan to
> getScanlines() once.
> 
> However, I think your use of onGetY() in the SkPngInterlacedSD makes a lot of
> sense.  I think maybe we should remove this SkASSERT (and the comment above),
> and trust that we are intelligent enough clients of our own code to not misuse
> getScanlines() with interlaced pngs.

I've removed the assert. FWIW, interlaced png calls onGetY directly since getY
doesn't add anything for a subclass, rather than to get around the assert.

https://codereview.chromium.org/1365313002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:470: void updateY(int newY) { fCurrScanline = newY; }
On 2015/09/28 14:48:50, msarett wrote:
> On 2015/09/25 16:07:49, scroggo wrote:
> > I named this updateY to mirror getY, but I might prefer something that
> > references "next" or "current" "scanline".
> 
> I'm in favor of renaming these.
> 
> I think my original name for getY() was getCurrScanline() but I felt that was
> confusing since it seemed like it should actually decode a scanline.  Maybe
that
> confusion is avoided with currScanline() or nextScanline()?

I'll add a FIXME to discuss for API review. I would probably lean towards
something like nextScanline.

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:165: if (transLessThanFF >= 0) {
On 2015/09/28 14:48:50, msarett wrote:
> On 2015/09/25 16:07:49, scroggo wrote:
> > If I understand this code correctly, the old code here decided that there
was
> > really alpha if one of the colors in the palette has alpha, regardless of
> > whether those colors are actually used. This defeats the purpose of the
> swizzler
> > function which returns whether a single line had alpha (for the indexed png
> > case).
> > 
> > Maybe it's unlikely that the client will attempt to decode a non-transparent
> > piece of a transparent image. We have outstanding questions about whether we
> > should bother doing the check at all (skbug.com/3582). So long as it's here,
> > though, I think it makes sense to only modify the alpha if we found no
colors
> > with alpha, to the new state which is known to be opaque.
> > 
> 
> I agree with this interpretation.
> 
> In this case, will we still check all the alpha values as we set the pixels or
> will we skip the check since we know already that the image is opaque?
> 
> EDIT: Reading below, it looks like we will still check all of the alpha
values. 
> Maybe add a FIXME?

Well, swizzle() returns the alpha value, and I don't want to create a separate,
mostly identical loop to ignore it (unless we find it makes a difference), but I
do think there's room for improvement here. Rather than calling IsOpaque in each
iteration, and or-ing that with hasAlpha, I think I can just store a ResultAlpha
and check it at the end. Added a FIXME.

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.h
File src/codec/SkCodec_libpng.h (right):

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkCodec_libpng.h#...
src/codec/SkCodec_libpng.h:54: enum AlphaState {
On 2015/09/28 14:48:50, msarett wrote:
> I think this is good.
> 
> Not in this CL, but maybe in the future, we will want to consider sharing this
> with other codecs that might benefit this optimization.

Yeah, no other codecs currently do any checking, but I agree it would make sense
for them to share if and when they do.

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

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:462: // FIXME: Is it safe to call when it doesn't need
to be called?
On 2015/09/28 14:48:50, msarett wrote:
> On 2015/09/25 16:07:50, scroggo wrote:
> > Matt, any thoughts? It seems to be safe, since nothing bad apparently
happens
> > when I run tests (at least it doesn't crash...).
> > 
> > But it might be unnecessary. Not sure how expensive jpeg_finish_decompress
is
> if
> > we already called it, but if it is, we could always skip this if
> output_scanline
> > is already height.
> 
> It's not expensive to call twice, since it'll realize that it has already done
> its work.  It will essentially make a function call and a few checks and do
> nothing.
> 
> However, I don't think we ever need to call jpeg_finish_decompress().
> skbug.com/4040
> 
> I was supposed to fix this a while back.
> 
> I would suggest landing this as is, and then I'll follow up with a CL that
> removes the calls jpeg_finish_decompress().  It'll be nice to document the
> potential risks of removing this call separately.

sgtm

https://codereview.chromium.org/1365313002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:516: // TODO (msarett): Make this a member function
and avoid reallocating the
On 2015/09/28 14:48:50, msarett wrote:
> On 2015/09/25 16:07:50, scroggo wrote:
> > Alternatively, we could just rely on the default implementation of
> > onSkipScanlines ifndef TURBO_HAS_SKIP
> 
> Yes definitely!  I think that is a better way to fix this!

Haha, that seems to go against your comment in wbmp, that we can always do
better than the default version of onSkipScanlines.

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1365313002/diff/1/tests/CodexTest.cpp#newcode273
tests/CodexTest.cpp:273: // Test interlaced PNG in stripes, similar to DM's
kStripe_Mode
On 2015/09/28 14:48:50, msarett wrote:
> On 2015/09/25 16:07:50, scroggo wrote:
> > Maybe this test is redundant with kStripe_Mode, but I like it because it
> allows
> > me to test locally without searching for an interlaced image I want to test.
> > 
> > It's also not really interlace specific; I wanted to make sure I didn't
break
> > interlacing (since I now use the base class's fCurrScanline instead of a
> > separate variable), but it could easily be run on other types (and it could
> use
> > 37, as kStripe_Mode does, if we want to replace it - not sure whether it's
> more
> > useful to have here versus run on more images in DM).
> > 
> > I could also move this code into check, which might simplify things a little
> > bit.
> 
> I like this as a separate test - I think it's good to verify this
functionality
> of scanline decoding for interlaced pngs (and for all top-down image types if
we
> wanted to generalize this test).  I'm in favor of keeping it and am not really
> bothered that it's similar to the DM test.

Thanks for the feedback. FWIW, I don't think anything about this test is
actually interlaced specific - it should work for top-down as is (unless I've
forgotten about something...).

> 
> Did we ever discuss forbidding the client from calling getScanlines() more
than
> once on interlaced pngs (since it is so inefficient)?  If we wanted to do
that,
> it would break this test.

I don't know if we did discuss. I don't think we should forbid it though. It's
possible that a client wants to get multiple subsets without rewinding (e.g.
top-left and then bottom-left). That would be a valid use case for calling it
more than once.

> 
> It's probably worth noting that the kStripe_Mode test in DM is skipped unless
> the image is kTopDown (interlaced pngs are kNone).  There's actually no reason
> why the DM test won't work with kNone (it'll just be extremely inefficient).

Yeah, when Emmalee initially added some interlaced images she changed DM to
decode all the lines desired (rather than one at a time), since it was so slow.
We probably don't want to run all interlaced images through stripe mode, just in
the interest of speed.

> 
> AFAICT, this test, just like the test in DM, will fail on kBottomUp and
> kOutOfOrder images.

Agreed. Added a check for that.

> 
> IMO, this test is not enough to replace kStripe_Mode in DM.  I think it's
> important to keep that for regression testing on future updates of
> libjpeg-turbo.  Testing on interesting values of numStripes would help, but
it's
> also nice to test on all of the jpegs we have on the bots.

sgtm

[djsollen, 2015-09-29 20:22:17]

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

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:141: kScanlineDecodingNotStarted,
move this above unimplemented.

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:264: Result start(const SkImageInfo& dstInfo, const
SkCodec::Options* options,
now that this is no longer part of a scanline class I think we should make the
name more descriptive.  One suggestion may be startScanlineDecode().

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:284: Result getScanlines(void* dst, int countLines,
size_t rowBytes) {
any reason not to move this to the cpp file since it is doing a non-trivial
amount of work?

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:400: *  FIXME: Rename to nextScanline()?
I agree that this should be renamed

[scroggo, 2015-09-30 13:32:20]

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

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:141: kScanlineDecodingNotStarted,
On 2015/09/29 20:22:17, djsollen wrote:
> move this above unimplemented.

Done.

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:264: Result start(const SkImageInfo& dstInfo, const
SkCodec::Options* options,
On 2015/09/29 20:22:17, djsollen wrote:
> now that this is no longer part of a scanline class I think we should make the
> name more descriptive.  One suggestion may be startScanlineDecode().

Matt included the same name in his suggestions. Done.

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:284: Result getScanlines(void* dst, int countLines,
size_t rowBytes) {
On 2015/09/29 20:22:17, djsollen wrote:
> any reason not to move this to the cpp file since it is doing a non-trivial
> amount of work?

Done. I also moved skipScanlines into cpp

https://codereview.chromium.org/1365313002/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:400: *  FIXME: Rename to nextScanline()?
On 2015/09/29 20:22:17, djsollen wrote:
> I agree that this should be renamed

Renamed to nextScanline().

I also renamed updateY() to updateNextScanline() so they match.

One thing I don't like about this name is that the virtual, by our convention,
is now onNextScanline(), which sounds like it means "when the next scanline is
decoded". I don't have a better suggestion though.

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

https://codereview.chromium.org/1365313002/diff/60001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:627: // FIXME: nextScanline is not virtual, so
using "INHERITED" does not change
Matt, I noticed this when I did the search and replace. I assume this was
intended to be this->INHERITED::onGetY(), but if our tests cover it (i.e. it's
working as intended), then it would have been calling this->getY(). Do you have
an interlaced gif where the frame is subset? (I assume that's the case that
matters, since if it is not interlaced, both implementations are the same.)

[msarett, 2015-09-30 13:53:30]

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

https://codereview.chromium.org/1365313002/diff/60001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:627: // FIXME: nextScanline is not virtual, so
using "INHERITED" does not change
On 2015/09/30 13:32:20, scroggo wrote:
> Matt, I noticed this when I did the search and replace. I assume this was
> intended to be this->INHERITED::onGetY(), but if our tests cover it (i.e. it's
> working as intended), then it would have been calling this->getY(). Do you
have
> an interlaced gif where the frame is subset? (I assume that's the case that
> matters, since if it is not interlaced, both implementations are the same.)

My intention was to call INHERITED::onNextScanline() - I think this code becomes
correct with that change.

I don't have any interlaced gifs where fFrameIsSubset.  I would guess that they
are extremely rare or don't exist at all.  I doubt I could find one, but I'm
sure I can make one.  I need to make one and add a test because I suspect we
have some broken code in this case.

My guess is that, in the interlaced frame is subset case,
SkGifCodec::onNextScanline() needs to say:
return fFrameDims.top() + get_output_row_interlaced(fCurrScanline -
fFrameDims.top())
or something like that.

Nice catch on this bug!

[djsollen, 2015-09-30 15:07:01]

api lgtm

[scroggo, 2015-09-30 15:11:45]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-09-30 15:11:45]

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/1365313002/#ps60001
(title: "Respond to derek's comments in patch set 3")

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

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

[commit-bot: I haz the power, 2015-09-30 15:12:49]

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

[commit-bot: I haz the power, 2015-09-30 15:12:49]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[scroggo, 2015-09-30 15:22:01]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-09-30 15:22:02]

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

[commit-bot: I haz the power, 2015-09-30 15:22:14]

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

[commit-bot: I haz the power, 2015-09-30 15:24:05]

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

[commit-bot: I haz the power, 2015-09-30 15:24:06]

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...)

[scroggo, 2015-09-30 15:28:29]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-09-30 15:28:29]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1365313002/#ps100001
(title: "Fix a warning")

[commit-bot: I haz the power, 2015-09-30 15:28:50]

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

[commit-bot: I haz the power, 2015-09-30 15:32:01]

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

[commit-bot: I haz the power, 2015-09-30 15:32:02]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[scroggo, 2015-09-30 15:48:53]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-09-30 15:48:54]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1365313002/#ps120001
(title: "Skip ICO in SkScaledCodec for now")

[commit-bot: I haz the power, 2015-09-30 15:49:05]

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

[commit-bot: I haz the power, 2015-09-30 15:57:20]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/46c574725676b26ada63ac15e42cda309dcd5090