Allow creating multiple scanline decoders.

Allow creating multiple scanline decoders.

Make getScanlineDecoder return a new object each time, which is
owned by the caller, and independent from any existing scanline
decoders and the SkCodec itself.

Since the SkCodec already contains the entire state machine, and it
is used by the scanline decoders, simply create a new SkCodec which
is now owned by the scanline decoder.

Move code that cleans up after using a scanline decoder into its
destructor

One side effect is that creating the first scanline decoder requires
a duplication of the stream and re-reading the header. (With some
more complexity/changes, we could pass the state machine to the
scanline decoder and make the SkCodec recreate its own state machine
instead.) The typical client of the scanline decoder (region decoder)
uses an SkMemoryStream, so the duplication is cheap, although we
should consider the extra time to reread the header/recreate the state
machine. (If/when we use the scanline decoder for other purposes,
where the stream may not be cheaply duplicated, we should consider
passing the state machine.)

One (intended) result of this change is that a client can create a
new scanline decoder in a new thread, and decode different pieces of
the image simultaneously.

In SkPngCodec::decodePalette, use fBitDepth rather than a parameter.

Committed: https://skia.googlesource.com/skia/+/9b2cdbf4811477487f107a78edc130c733b309ea

[scroggo, 2015-07-09 20:52:31]

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

[scroggo, 2015-07-09 20:52:31]

https://codereview.chromium.org/1230033004/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1230033004/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:587: // FIXME: Is this necessary?
I copied the below comment from SkImageDecoder_libpng.cpp, but as I made this
change I started to wonder whether it was true. It states that it reads the rest
of the file, which made me wonder what would happen in the case of a scanline
decoder, where we potentially have not read the rest of the image. Does it work
in that case?

Looking at the libpng documentation, it sounds like we only need to call it if
we want to find out information stored in the end of the stream, or if we want
to continue reading the stream after the PNG itself - i.e. the PNG is embedded
in some larger stream we want to keep reading from where the PNG left off. This
may be the case where we did a single decode (perhaps we're reading from some
kind of Asset stream on Android that we are expected to leave at the end of the
PNG so something else can continue reading the stream), but if we've already
copied into an SkMemoryStream (as is the case for region decoding), this call
seems unnecessary. Not sure how expensive it is though.

Emmalee, any thoughts?

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

https://codereview.chromium.org/1230033004/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:394:
turbo_jpeg_finish_decompress(fCodec->fDecoderMgr->dinfo());
Matt, I'm questioning this since I questioned the analogous case in libpng.
According to the documentation, this "release[s] working memory associated with
the JPEG object". Is that memory that would not be released by
jpeg_destroy_decompress?

[msarett, 2015-07-09 21:32:26]

LGTM!

I think this is a great change.  Glad we are questioning
the need to call finish() and I'm glad that we are getting
multithreaded.

"One side effect is that creating the first scanline decoder requires
a duplication of the stream and re-reading the header. (With some
more complexity/changes, we could pass the state machine to the
scanline decoder and make the SkCodec recreate its own state machine
instead.) The typical client of the scanline decoder (region decoder)
uses an SkMemoryStream, so the duplication is cheap, although we
should consider the extra time to reread the header/recreate the state
machine. (If/when we use the scanline decoder for other purposes,
where the stream may not be cheaply duplicated, we should consider
passing the state machine.)"

I think we will want to test the performance impact of this at some point.
But not now, I think this is a step in the right direction.

"One (intended) result of this change is that a client can create a
new scanline decoder in a new thread, and decode different pieces of
the image simultaneously."

Very excited about this!

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

https://codereview.chromium.org/1230033004/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:394:
turbo_jpeg_finish_decompress(fCodec->fDecoderMgr->dinfo());
On 2015/07/09 20:52:31, scroggo wrote:
> Matt, I'm questioning this since I questioned the analogous case in libpng.
> According to the documentation, this "release[s] working memory associated
with
> the JPEG object". Is that memory that would not be released by
> jpeg_destroy_decompress?

Good question.  I originally chose this function as the place to call free() on
the extra row buffer that was used by jpeg_skip_scanlines().  But this is no
longer the case (we now free in jpeg_destroy_decompress).

So do we actually need to call this function?

Before I implemented skipping, we were getting away with not always calling
finish(), so I would expect that now that skipping does not need this function,
we can stop calling it again.

Here are some things it does:
1) Reads to the end of the image.  Seems clear that we don't care about this.
2) Calls term_source on the data source that we provide.  We currently do not do
anything in our implementation of term_source.  But we would want to be very
clear that we also CANNOT do anything important in term_source if we stop
guaranteeing that it will be called.
3) Calls finish_output_pass().  This wraps up any color quantizing (essentially
conversions to using color tables) that we may have been doing.  I think its
highly unlikely that we will ever care about this feature.
4) Calls jpeg_abort().  This does memory cleanup and prepares the jpeg_struct to
be reused if we desire.  AFAICT, jpeg_destroy() will perform the exact same
memory cleanup if jpeg_abort() hasn't already done it.

So that is a very long-winded way of saying, I think you're right.  I don't
*think* we ever need to call jpeg_finish_decompress().

I think it's probably best to leave this in for now.  Then after I finish fixing
the current jpeg-related valgrind bot errors, maybe I can try to land this and
see what happens.

[scroggo, 2015-07-10 15:05:30]

ping - I'll still need an API review for the changes to SkCodec.h and
SkScanlineDecoder.h

[djsollen, 2015-07-10 15:20:41]

api lgtm

[scroggo, 2015-07-10 17:19:07]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-07-10 17:19:19]

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

[commit-bot: I haz the power, 2015-07-10 17:31:27]

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

[commit-bot: I haz the power, 2015-07-10 17:31:28]

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

[scroggo, 2015-07-10 17:38:41]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-07-10 17:38:42]

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/1230033004/#ps80001
(title: "Use fNumberPasses from the correct SkPngCodec.")

[commit-bot: I haz the power, 2015-07-10 17:38:54]

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

[commit-bot: I haz the power, 2015-07-10 17:42:36]

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

[commit-bot: I haz the power, 2015-07-10 17:42:37]

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-07-10 18:59:29]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-07-10 18:59:30]

lgtm

Thank you try bots, for catching all my bugs! This version should work...

https://codereview.chromium.org/1230033004/diff/80001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1230033004/diff/80001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:673: if (fRewindNeeded) {
FYI: This was used to skip a call to handleRewind, which was called in
getScanlineDecoder. Now that getScanlineDecoder no longer calls that, this one
needs to call it (so the second call to onGetScanlines will actually do a
rewind, rather than just marking it as needing to rewind next time).

[scroggo, 2015-07-10 18:59:30]

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/1230033004/#ps100001
(title: "Progressive scanline decoder needs to handle rewind on first call.")

[commit-bot: I haz the power, 2015-07-10 18:59:42]

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

[commit-bot: I haz the power, 2015-07-10 19:07:06]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/9b2cdbf4811477487f107a78edc130c733b309ea