Adding png scanline decoding to kIndex8

Requires a minor API change to pass the color table to getScanlineDecoder, as we do with getPixels().

BUG=skia:3722
Committed: https://skia.googlesource.com/skia/+/9e43cabaa1ff26167e990f0bcb9a5c20d662d52c

[msarett, 2015-04-21 20:55:20]

Patchset #1 (id:1) has been deleted

[msarett, 2015-04-21 20:57:25]

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

[scroggo, 2015-04-21 21:43:04]

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

[scroggo, 2015-04-21 21:43:05]

lgtm

Adding reed@ for API approval.

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

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:76: *      populated if the png has a color palette and
we ask to decode in
nit: "we ask to decode in" -> dstInfo.colorType() is

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:79: *      modified in kIndex8 mode if we have a color
palette.
modified to what?

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:88: SkScanlineDecoder* getScanlineDecoder(const
SkImageInfo& dstInfo, const Options* options,
Should we have a version that does not take Options or color table stuff, like
getPixels?

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:140: *      populated if the png has a color palette and
we ask to decode in
same wording nit.

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:143: *      modified in kIndex8 mode if we have a color
palette.
same question

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

https://codereview.chromium.org/1061713007/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:471: if (kIndex_8_SkColorType ==
requestedInfo.colorType()) {
I think these same 5 lines are copied elsewhere. Factor it out to a helper in
SkCodecPriv?

https://codereview.chromium.org/1061713007/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:478: // Create the swizzler, SkPngCodec retains
ownership of the color table
nit: this seems like it should be two sentences.

[msarett, 2015-04-22 14:32:52]

Added simpler version of getScanlineDecoder.

Improved API comments.

Shared copy_color_table() code.

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

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:76: *      populated if the png has a color palette and
we ask to decode in
On 2015/04/21 21:43:05, scroggo wrote:
> nit: "we ask to decode in" -> dstInfo.colorType() is

Done.

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:79: *      modified in kIndex8 mode if we have a color
palette.
On 2015/04/21 21:43:04, scroggo wrote:
> modified to what?

Done.

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:88: SkScanlineDecoder* getScanlineDecoder(const
SkImageInfo& dstInfo, const Options* options,
On 2015/04/21 21:43:05, scroggo wrote:
> Should we have a version that does not take Options or color table stuff, like
> getPixels?

I have added this version.

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:140: *      populated if the png has a color palette and
we ask to decode in
On 2015/04/21 21:43:05, scroggo wrote:
> same wording nit.

Done.

https://codereview.chromium.org/1061713007/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:143: *      modified in kIndex8 mode if we have a color
palette.
On 2015/04/21 21:43:05, scroggo wrote:
> same question

Done.

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

https://codereview.chromium.org/1061713007/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:471: if (kIndex_8_SkColorType ==
requestedInfo.colorType()) {
On 2015/04/21 21:43:05, scroggo wrote:
> I think these same 5 lines are copied elsewhere. Factor it out to a helper in
> SkCodecPriv?

Done.

https://codereview.chromium.org/1061713007/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:478: // Create the swizzler, SkPngCodec retains
ownership of the color table
On 2015/04/21 21:43:05, scroggo wrote:
> nit: this seems like it should be two sentences.

Done.

[scroggo, 2015-04-22 17:43:54]

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

https://codereview.chromium.org/1061713007/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:80: *      be a pointer to 256.  It will be modified to
the actual size of
It does not make a lot of sense to me to say it must be a pointer to 256 - it
seems like it doesn't even need to be initialized if we always expect it to be
256.

[msarett, 2015-04-22 18:05:39]

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

https://codereview.chromium.org/1061713007/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:80: *      be a pointer to 256.  It will be modified to
the actual size of
On 2015/04/22 17:43:53, scroggo wrote:
> It does not make a lot of sense to me to say it must be a pointer to 256 - it
> seems like it doesn't even need to be initialized if we always expect it to be
> 256.
> 

I agree with you.  I was trying to match the requirements of getPixels().  I
will change and remove the appropriate asserts.

[scroggo, 2015-04-22 18:41:35]

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

[scroggo, 2015-04-22 18:41:36]

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

https://codereview.chromium.org/1061713007/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:80: *      be a pointer to 256.  It will be modified to
the actual size of
On 2015/04/22 18:05:38, msarett wrote:
> On 2015/04/22 17:43:53, scroggo wrote:
> > It does not make a lot of sense to me to say it must be a pointer to 256 -
it
> > seems like it doesn't even need to be initialized if we always expect it to
be
> > 256.
> > 
> 
> I agree with you.  I was trying to match the requirements of getPixels().  I
> will change and remove the appropriate asserts.

I *think* the idea was that a client could use a smaller number than 256, to
state that they had storage for a smaller number of colors.

In practice, I am not sure how to deal with that. Why would they use a smaller
storage? Do they think they know how many colors the image uses?

Upon re-reading the comments for getPixels, it wasn't clear to me that passing a
number less than 256 actually is supposed to behave that way, though. Hal, any
thoughts?

[msarett, 2015-04-22 20:03:32]

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

https://codereview.chromium.org/1061713007/diff/40001/include/codec/SkCodec.h...
include/codec/SkCodec.h:80: *      be a pointer to 256.  It will be modified to
the actual size of
On 2015/04/22 18:41:36, scroggo wrote:
> On 2015/04/22 18:05:38, msarett wrote:
> > On 2015/04/22 17:43:53, scroggo wrote:
> > > It does not make a lot of sense to me to say it must be a pointer to 256 -
> it
> > > seems like it doesn't even need to be initialized if we always expect it
to
> be
> > > 256.
> > > 
> > 
> > I agree with you.  I was trying to match the requirements of getPixels().  I
> > will change and remove the appropriate asserts.
> 
> I *think* the idea was that a client could use a smaller number than 256, to
> state that they had storage for a smaller number of colors.
> 
> In practice, I am not sure how to deal with that. Why would they use a smaller
> storage? Do they think they know how many colors the image uses?
> 
> Upon re-reading the comments for getPixels, it wasn't clear to me that passing
a
> number less than 256 actually is supposed to behave that way, though. Hal, any
> thoughts?

I think I misinterpreted the comments in SkImageGenerator.h as well - hopefully
I understand them better now.

It seems to me that we always require storage for at least 256 colors and a
non-NULL count_ptr, when decoding to kIndex8.  Then, we will set the value of
the count_ptr based on the actual number of colors in the image, ignoring the
previous value.

This seems reasonable enough to me, given that it may be tricky to deal with
situations when storage is less than 256.  I believe the latest patch set
follows this convention.

[scroggo, 2015-04-23 15:48:26]

lgtm

[msarett, 2015-04-23 17:38:34]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-04-23 17:38:52]

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

[commit-bot: I haz the power, 2015-04-23 17:39:00]

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

[commit-bot: I haz the power, 2015-04-23 17:39:01]

Presubmit check for 1061713007-60001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

[msarett, 2015-04-23 17:40:41]

Oh sorry forgot that Mike was added.

[msarett, 2015-04-28 13:35:18]

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

[djsollen, 2015-04-29 14:02:59]

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

https://codereview.chromium.org/1061713007/diff/60001/include/codec/SkCodec.h...
include/codec/SkCodec.h:90: SkScanlineDecoder* getScanlineDecoder(const
SkImageInfo& dstInfo, const Options* options,
const* for options here and const& on the protected version. Seems like we
should stick with one.

[scroggo, 2015-04-29 14:13:23]

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

https://codereview.chromium.org/1061713007/diff/60001/include/codec/SkCodec.h...
include/codec/SkCodec.h:90: SkScanlineDecoder* getScanlineDecoder(const
SkImageInfo& dstInfo, const Options* options,
On 2015/04/29 14:02:58, djsollen wrote:
> const* for options here and const& on the protected version. Seems like we
> should stick with one.

This follows a pattern I've used elsewhere in SkCodec/SkImageGenerator, and I
believe I've seen it elsewhere.

It is deliberate - Options is optional, so we make the public version a pointer,
allowing a client to pass NULL. Rather than making the protected version also
optional, resulting in each implementation checking for NULL, and potentially
deciding differently what the default should be, the implementation for the
public version passes a default version if the client passed NULL.

[djsollen, 2015-04-29 14:26:58]

then API lgtm

[msarett, 2015-04-29 14:33:17]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-04-29 14:33:26]

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

[commit-bot: I haz the power, 2015-04-29 14:38:46]

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