Add SkPngChunkReader.

Add SkPngChunkReader.

This class allows a client of SkCodec to read chunks in the data
stream that are not recognized by libpng. This is used by Android
to specify ninepatch data.

Taken from SkImageDecoder::Peeker. Modify the name of the class
and its method to be more specific to their use. Make
SkImageDecoder::Peeker a subclass of the new class, to help stage
the change in Android.

Add a test to verify that it works.

BUG=skia:4574
BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/3389e00136188800b98ca69488c0418c374fd78b

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

[scroggo, 2015-03-26 21:16:43]

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

[djsollen, 2015-03-27 01:23:46]

what do we anticipate this being used for since we don't expose it in the Java
API?

[scroggo, 2015-03-27 12:32:37]

On 2015/03/27 01:23:46, djsollen wrote:
> what do we anticipate this being used for since we don't expose it in the Java
> API?

See description. Like SkImageDecoder::Peeker, this will be used for ninepatches.
The native code will attach a subclass to read ninepatch data (e.g.
NinePatchPeeker, which is attached to SkImageDecoder in BitmapFactory.cpp).

I've given it some further thought, though, and maybe this isn't the API we
want.

Instead of attaching it to the SkCodec, we could attach it to the Options
struct, passed to getPixels. This keeps SkCodec relatively stateless, besides
the underlying stream. (As a counter point, you would think that if a client
wanted a Peeker for one call to getPixels, they'd probably want it for further
calls as well.)

Further, we could change it to a function pointer, since it does not need to
have any state. (Android's NinePatchPeeker stores a pointer to its
SkImageDecoder so that it can call setPreservesSrcDepth on it, but SkCodec will
always preserve src depth, so we don't need to make that call.)

On another note, we could move it outside of SkCodec, so we can share the same
object between SkImageDecoder and SkCodec.

Thoughts?

[scroggo, 2015-03-27 15:00:29]

On 2015/03/27 12:32:37, scroggo wrote:
> On 2015/03/27 01:23:46, djsollen wrote:
> > what do we anticipate this being used for since we don't expose it in the
Java
> > API?
> 
> See description. Like SkImageDecoder::Peeker, this will be used for
ninepatches.
> The native code will attach a subclass to read ninepatch data (e.g.
> NinePatchPeeker, which is attached to SkImageDecoder in BitmapFactory.cpp).
> 
> I've given it some further thought, though, and maybe this isn't the API we
> want.
> 
> Instead of attaching it to the SkCodec, we could attach it to the Options
> struct, passed to getPixels. This keeps SkCodec relatively stateless, besides
> the underlying stream. (As a counter point, you would think that if a client
> wanted a Peeker for one call to getPixels, they'd probably want it for further
> calls as well.)
> 
> Further, we could change it to a function pointer, since it does not need to
> have any state. (Android's NinePatchPeeker stores a pointer to its
> SkImageDecoder so that it can call setPreservesSrcDepth on it, but SkCodec
will
> always preserve src depth, so we don't need to make that call.)
> 
> On another note, we could move it outside of SkCodec, so we can share the same
> object between SkImageDecoder and SkCodec.
> 
> Thoughts?

Mike and I talked about this a little bit, and agreed to add the Peeker in the
constructor. This ensures that it's set early enough to handle early chunks
(looking at SkImageDecoder_libpng, it appears that a chunk is read early, since
it changes the preferred config, which we also read early). It also means the
SkCodec either starts with or without a Peeker, and that never changes.

I don't know why I thought the Peeker was stateless; NinePatchPeeker, in
Android, has state that will be needed for SkCodec's version, so it will need to
be an object.

I lean towards pulling it out into a separate file, so we can share one between
SkCodec and SkImageDecoder. This allows us to avoid having duplicate
NinePatchPeekers in Android.

[scroggo, 2015-03-27 19:19:46]

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

[scroggo, 2015-03-27 19:19:59]

PTAL.

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

https://codereview.chromium.org/1040453002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:14: #include "SkRefCnt.h"
No longer needed.

https://codereview.chromium.org/1040453002/diff/20001/include/core/SkChunkRea...
File include/core/SkChunkReader.h (right):

https://codereview.chromium.org/1040453002/diff/20001/include/core/SkChunkRea...
include/core/SkChunkReader.h:20: class SkChunkReader : public SkRefCnt {
How's the name? Or maybe it should be ChunkParser? Or ChunkHandler? I don't have
a strong preference.

[reed1, 2015-03-27 20:13:47]

Just being conservative in introducing new public apis/classes

https://codereview.chromium.org/1040453002/diff/20001/include/core/SkChunkRea...
File include/core/SkChunkReader.h (right):

https://codereview.chromium.org/1040453002/diff/20001/include/core/SkChunkRea...
include/core/SkChunkReader.h:15: *  ChunkReader
ImageChunkReader, or PNGChunkReader?

It just feels like "chunk" is awfully general, and our only contemplated use at
the moment is PNG image chunks.

[scroggo, 2015-04-01 14:53:48]

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

https://codereview.chromium.org/1040453002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:14: #include "SkRefCnt.h"
On 2015/03/27 19:19:58, scroggo wrote:
> No longer needed.

Done.

https://codereview.chromium.org/1040453002/diff/20001/include/core/SkChunkRea...
File include/core/SkChunkReader.h (right):

https://codereview.chromium.org/1040453002/diff/20001/include/core/SkChunkRea...
include/core/SkChunkReader.h:15: *  ChunkReader
On 2015/03/27 20:13:47, reed1 wrote:
> ImageChunkReader, or PNGChunkReader?
> 
> It just feels like "chunk" is awfully general, and our only contemplated use
at
> the moment is PNG image chunks.

Done.

[djsollen, 2015-04-01 16:11:14]

looks fine to me from a code perspective, but not sure if Mike still wants to
iterate on the API.

[reed1, 2015-04-03 20:05:42]

lgtm, but I think we should more strongly determine/document the lifecycle of
the peeker...

1. when is it called
  - stackframe of the factory?
  - lazily later on when we get around to it?
  - either?

2. can it be called more than once, if we "rewind" and decode again?

3. document it may be called from a diff thread than where it was created.

[scroggo, 2015-11-18 20:45:24]

Description was changed from

==========
Add SkPngChunkReader.

Taken straight from SkImageDecoder::Peeker. This allows
Android to use ninepatch images with SkCodec.

This class also replaces SkImageDecoder::Peeker. As a result, 
Android can share its NinePatchPeeker between SkImageDecoder
and SkCodec (while SkImageDecoder is still around).

Will require a change to Android (NinePatchPeeker needs to
inherit from SkChunkReader).

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

to

==========
Add SkPngChunkReader.

Taken straight from SkImageDecoder::Peeker. This allows
Android to use ninepatch images with SkCodec.

This class also replaces SkImageDecoder::Peeker. As a result, 
Android can share its NinePatchPeeker between SkImageDecoder
and SkCodec (while SkImageDecoder is still around).

Will require a change to Android (NinePatchPeeker needs to
inherit from SkChunkReader).

BUG=skia:4574
BUG=skia:3257
==========

[scroggo, 2015-11-18 20:45:24]

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

[scroggo, 2015-11-20 18:31:48]

Description was changed from

==========
Add SkPngChunkReader.

Taken straight from SkImageDecoder::Peeker. This allows
Android to use ninepatch images with SkCodec.

This class also replaces SkImageDecoder::Peeker. As a result, 
Android can share its NinePatchPeeker between SkImageDecoder
and SkCodec (while SkImageDecoder is still around).

Will require a change to Android (NinePatchPeeker needs to
inherit from SkChunkReader).

BUG=skia:4574
BUG=skia:3257
==========

to

==========
Add SkPngChunkReader.

This class allows a client of SkCodec to read chunks in the data
stream that are not recognized by libpng. This is used by Android
to specify ninepatch data.

Taken from SkImageDecoder::Peeker. Modify the name of the class
and its method to be more specific to their use. Make
SkImageDecoder::Peeker a subclass of the new class, to help stage
the change in Android.

Add a test to verify that it works.

BUG=skia:4574
BUG=skia:3257
==========

[scroggo, 2015-11-20 18:40:02]

PTAL. New changes since the last review:
- Added a test
- Handle staging better (Android will not need to be updated to not break)
- Add documentation

On 2015/04/03 20:05:42, reed1 wrote:
> lgtm, but I think we should more strongly determine/document the lifecycle of
> the peeker...
> 
> 1. when is it called
>   - stackframe of the factory?
>   - lazily later on when we get around to it?
>   - either?

As I have since stated in skbug.com/4574, it appears to be either. I have
updated the docs for SkPngChunkReader to reflect that.

> 
> 2. can it be called more than once, if we "rewind" and decode again?

Yes it can. I have updated the docs and added a test.

> 
> 3. document it may be called from a diff thread than where it was created.

Done

[msarett, 2015-11-20 20:21:23]

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

https://codereview.chromium.org/1040453002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:38: static SkCodec* NewFromStream(SkStream*,
SkPngChunkReader* = NULL);
This is in contrast to SkImageDecoder's setPeeker() API?

Any thoughts on the tradeoffs?  I'm just curious, I don't really have a
preference.

https://codereview.chromium.org/1040453002/diff/140001/include/core/SkImageDe...
File include/core/SkImageDecoder.h (right):

https://codereview.chromium.org/1040453002/diff/140001/include/core/SkImageDe...
include/core/SkImageDecoder.h:19: //#define SK_LEGACY_PEEKER
Did you intend to leave this here?

https://codereview.chromium.org/1040453002/diff/140001/include/core/SkImageDe...
include/core/SkImageDecoder.h:132: #ifdef SK_LEGACY_PEEKER
This code gets removed after you update Android?

https://codereview.chromium.org/1040453002/diff/140001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1040453002/diff/140001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:222: // Reads the header and initializes the output
fields, if not NULL.
+1 for comments.

[scroggo, 2015-11-20 21:14:58]

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

https://codereview.chromium.org/1040453002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:38: static SkCodec* NewFromStream(SkStream*,
SkPngChunkReader* = NULL);
On 2015/11/20 20:21:23, msarett wrote:
> This is in contrast to SkImageDecoder's setPeeker() API?
> 
> Any thoughts on the tradeoffs?  I'm just curious, I don't really have a
> preference.

Mike and I talked about this a while ago (this is from my earlier comments,
which you can view in the discussion at the bottom):

> Mike and I talked about this a little bit, and agreed to add the Peeker in the
> constructor. This ensures that it's set early enough to handle early chunks
> (looking at SkImageDecoder_libpng, it appears that a chunk is read early,
since
> it changes the preferred config, which we also read early). It also means the
> SkCodec either starts with or without a Peeker, and that never changes.

I dug into it a bit more, and I learned that libpng's chunks come in the
following order:
IHDR
PLTE
tRNS (if present)
IDAT

Android always uses PNG_HAVE_PLTE when inserting custom chunks, which is
documented to be after PLTE and before IDAT, but I'm not sure whether it comes
before tRNS or after. We read tRNS in the factory, so if it comes before, that
would be a problem. If we want to install it elsewhere (Mike doesn't like the
setPeeker API - he prefers something that is not stateful, but we could attach
it to Options), we need to verify that tRNS comes before, but if libpng does not
promise that order (I haven't found that promise), I'm nervous about relying on
it.

If we were to supply it later, we also wouldn't be able to support PNG_HAVE_IHDR
(not used by Android, but it is an option - see
http://www.ludism.org/~rwhe/LSB/png/libpng-png-set-unknown-chunks-1.html), so we
would need to document that as well.

https://codereview.chromium.org/1040453002/diff/140001/include/core/SkImageDe...
File include/core/SkImageDecoder.h (right):

https://codereview.chromium.org/1040453002/diff/140001/include/core/SkImageDe...
include/core/SkImageDecoder.h:132: #ifdef SK_LEGACY_PEEKER
On 2015/11/20 20:21:23, msarett wrote:
> This code gets removed after you update Android?

Yes.

[msarett, 2015-11-20 21:27:26]

lgtm

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

https://codereview.chromium.org/1040453002/diff/140001/include/codec/SkCodec....
include/codec/SkCodec.h:38: static SkCodec* NewFromStream(SkStream*,
SkPngChunkReader* = NULL);
On 2015/11/20 21:14:58, scroggo wrote:
> On 2015/11/20 20:21:23, msarett wrote:
> > This is in contrast to SkImageDecoder's setPeeker() API?
> > 
> > Any thoughts on the tradeoffs?  I'm just curious, I don't really have a
> > preference.
> 
> Mike and I talked about this a while ago (this is from my earlier comments,
> which you can view in the discussion at the bottom):
> 
> > Mike and I talked about this a little bit, and agreed to add the Peeker in
the
> > constructor. This ensures that it's set early enough to handle early chunks
> > (looking at SkImageDecoder_libpng, it appears that a chunk is read early,
> since
> > it changes the preferred config, which we also read early). It also means
the
> > SkCodec either starts with or without a Peeker, and that never changes.
> 
> I dug into it a bit more, and I learned that libpng's chunks come in the
> following order:
> IHDR
> PLTE
> tRNS (if present)
> IDAT
> 
> Android always uses PNG_HAVE_PLTE when inserting custom chunks, which is
> documented to be after PLTE and before IDAT, but I'm not sure whether it comes
> before tRNS or after. We read tRNS in the factory, so if it comes before, that
> would be a problem. If we want to install it elsewhere (Mike doesn't like the
> setPeeker API - he prefers something that is not stateful, but we could attach
> it to Options), we need to verify that tRNS comes before, but if libpng does
not
> promise that order (I haven't found that promise), I'm nervous about relying
on
> it.
> 
> If we were to supply it later, we also wouldn't be able to support
PNG_HAVE_IHDR
> (not used by Android, but it is an option - see
> http://www.ludism.org/~rwhe/LSB/png/libpng-png-set-unknown-chunks-1.html), so
we
> would need to document that as well.

I think I would prefer it in Options, but, if that may not be early enough, then
I'm in favor of this.

[scroggo, 2015-11-23 15:08:19]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-11-23 15:08:19]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
msarett@google.com
Link to the patchset: https://codereview.chromium.org/1040453002/#ps160001
(title: "Put ChunkReader on the stack")

[commit-bot: I haz the power, 2015-11-23 15:08:24]

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

[commit-bot: I haz the power, 2015-11-23 15:20:59]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/3389e00136188800b98ca69488c0418c374fd78b

[fmalita_google_do_not_use, 2015-11-23 15:54:39]

On 2015/11/23 15:20:59, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:160001) as
> https://skia.googlesource.com/skia/+/3389e00136188800b98ca69488c0418c374fd78b

Busted Chromium builds: 

../../third_party/skia/src/ports/SkImageDecoder_empty.cpp:63:17: error: no type
named 'Peeker' in 'SkImageDecoder'
SkImageDecoder::Peeker* SkImageDecoder::setPeeker(Peeker*) {
~~~~~~~~~~~~~~~~^
../../third_party/skia/src/ports/SkImageDecoder_empty.cpp:63:51: error: unknown
type name 'Peeker'
SkImageDecoder::Peeker* SkImageDecoder::setPeeker(Peeker*) {
                                                  ^
2 errors generated.

[scroggo, 2015-11-23 15:56:29]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1472863003/ by scroggo@google.com.

The reason for reverting is: Busted Chromium builds: 

../../third_party/skia/src/ports/SkImageDecoder_empty.cpp:63:17: error: no type
named 'Peeker' in 'SkImageDecoder'
SkImageDecoder::Peeker* SkImageDecoder::setPeeker(Peeker*) {
~~~~~~~~~~~~~~~~^
../../third_party/skia/src/ports/SkImageDecoder_empty.cpp:63:51: error: unknown
type name 'Peeker'
SkImageDecoder::Peeker* SkImageDecoder::setPeeker(Peeker*) {.

[scroggo, 2015-11-23 16:00:23]

Description was changed from

==========
Add SkPngChunkReader.

This class allows a client of SkCodec to read chunks in the data
stream that are not recognized by libpng. This is used by Android
to specify ninepatch data.

Taken from SkImageDecoder::Peeker. Modify the name of the class
and its method to be more specific to their use. Make
SkImageDecoder::Peeker a subclass of the new class, to help stage
the change in Android.

Add a test to verify that it works.

BUG=skia:4574
BUG=skia:3257

Committed:
https://skia.googlesource.com/skia/+/3389e00136188800b98ca69488c0418c374fd78b
==========

to

==========
Add SkPngChunkReader.

This class allows a client of SkCodec to read chunks in the data
stream that are not recognized by libpng. This is used by Android
to specify ninepatch data.

Taken from SkImageDecoder::Peeker. Modify the name of the class
and its method to be more specific to their use. Make
SkImageDecoder::Peeker a subclass of the new class, to help stage
the change in Android.

Add a test to verify that it works.

BUG=skia:4574
BUG=skia:3257

Committed:
https://skia.googlesource.com/skia/+/3389e00136188800b98ca69488c0418c374fd78b
==========

[scroggo, 2015-11-23 16:01:32]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-11-23 16:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1040453002/#ps180001
(title: "Update SkImageDecoder_empty")

[commit-bot: I haz the power, 2015-11-23 16:01:35]

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

[commit-bot: I haz the power, 2015-11-23 16:14:45]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/cf98fa93116cf5efcc42d48c22fb4bd76896acdc