Add SkCodec, including PNG implementation.

Add SkCodec, including PNG implementation.

DM:
Add a flag to use SkCodec instead of SkImageDecoder.

SkCodec:
Base class for codecs, allowing creation from an SkStream or an SkData.
An SkCodec, on creation, knows properties of the data like its width and height. Further calls can be used to generate the image.
TODO: Add scanline iterator

SkPngCodec:
New decoder for png. Wraps libpng. The code has been repurposed from SkImageDecoder_libpng.
TODO: Handle other destination colortypes
TODO: Substitute the transpose color
TODO: Allow silencing warnings
TODO: Use RGB instead of filler?
TODO: sRGB

SkSwizzler:
Simplified version of SkScaledSampler. Unlike the sampler, this object does no sampling.
TODO: Implement other swizzles.

Requires a gclient sync to pull down libpng.

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/ca358852b4fed656d11107b2aaf28318a4518b49
(and then reverted)

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

[scroggo, 2015-02-25 14:17:01]

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

[scroggo, 2015-02-25 14:17:02]

PTAL

There's still a lot of work to be done, but it at least works for the basic
cases (e.g. decode to 8888 once), and I think it's ready to be checked in.

(One exception: it doesn't currently build on Mac. I'd like to stick with the
same decoder on all platforms (skbug.com/2461), but we need to either build png
as a static library or make Mac users install png.)

[reed1, 2015-02-25 15:37:22]

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

[djsollen, 2015-02-25 16:02:20]

https://codereview.chromium.org/930283002/diff/180001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/930283002/diff/180001/dm/DMSrcSink.cpp#newcode66
dm/DMSrcSink.cpp:66: SkAutoLockPixels alp(bitmap);
aren't the pixels locked during allocPixels if you use the default (heap)
allocator?  Also what about calling tryAllocPixels and throw an error in case
the image was so large we couldn't hold it?  I think it is better to not crash
in that event and let the other tests continue to run.

https://codereview.chromium.org/930283002/diff/180001/dm/DMSrcSink.cpp#newcode72
dm/DMSrcSink.cpp:72: } else {
else if

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp#newcode28
gyp/codec.gyp:28: 'export_dependent_settings': [
why do we want to expose these settings further up the stack?

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp#newcode50
gyp/codec.gyp:50: 'android_deps.gyp:png',
for android we should link in the most recent copy of libpng not the one that
SkImageDecoder uses.

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

https://codereview.chromium.org/930283002/diff/180001/include/codec/SkCodec.h...
include/codec/SkCodec.h:21: *  Abstraction layer directly on top of a codec.
Seems ambiguous as to the type of codec.  how about: top of an image codec

https://codereview.chromium.org/930283002/diff/180001/include/codec/SkCodec.h...
include/codec/SkCodec.h:85: bool couldRewindIfNeeded();
call it rewindIfNeeded as the "could" makes it seem like a query that doesn't
actually rewind.

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:19: if (!stream->rewind()) {
why do we have to be able to rewind the stream?  At the least it seems like
there needs to be a TODO here to implement a stream class that wraps this stream
to allow for temporary rewinding.

I imagine something like...

SkCodecStream tmp(stream);
const bool isPng = SkPngCodec::IsPng(tmp);
tmp.rewind();  // will always return true since it has buffered the contents
read in so far
... // check other codecs if needed
tmp.rewind(); // final rewind
tmp.stopBuffering(); // stops the tmp stream from buffering contents so rewind
may return false if underlying stream doesn't support it.

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

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:47: SkDEBUGF(("------ png error %s\n", msg));
I think we may want to always log this error not just in debug mode.

[reed1, 2015-02-25 16:20:50]

api lgtm

I will try to add the getScaledSize call to SkImageGenerator, at which time I
bet we can make SkCodec a private class!

[scroggo, 2015-02-25 18:00:55]

https://codereview.chromium.org/930283002/diff/180001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/930283002/diff/180001/dm/DMSrcSink.cpp#newcode66
dm/DMSrcSink.cpp:66: SkAutoLockPixels alp(bitmap);
On 2015/02/25 16:02:19, djsollen wrote:
> aren't the pixels locked during allocPixels if you use the default (heap)
> allocator? 

Yes. In fact I initially left this line out, if you look at my earlier
patchsets. That said, the fact that they are locked appears to be an
implementation detail. The fact that they are locked is not guaranteed by the
comments.

> Also what about calling tryAllocPixels and throw an error in case
> the image was so large we couldn't hold it?  I think it is better to not crash
> in that event and let the other tests continue to run.

Done.

https://codereview.chromium.org/930283002/diff/180001/dm/DMSrcSink.cpp#newcode72
dm/DMSrcSink.cpp:72: } else {
On 2015/02/25 16:02:19, djsollen wrote:
> else if

In this case, I actually like the current code better, since it more clearly (at
least to me) delineates between FLAGS_codec and !FLAGS_codec.

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp#newcode28
gyp/codec.gyp:28: 'export_dependent_settings': [
On 2015/02/25 16:02:19, djsollen wrote:
> why do we want to expose these settings further up the stack?

This copies images.gyp almost exactly. Not sure what our reasoning was there.

I removed it though. If it causes any problems we should find out in testing.

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp#newcode50
gyp/codec.gyp:50: 'android_deps.gyp:png',
On 2015/02/25 16:02:19, djsollen wrote:
> for android we should link in the most recent copy of libpng not the one that
> SkImageDecoder uses.

Done.

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

https://codereview.chromium.org/930283002/diff/180001/include/codec/SkCodec.h...
include/codec/SkCodec.h:21: *  Abstraction layer directly on top of a codec.
On 2015/02/25 16:02:19, djsollen wrote:
> Seems ambiguous as to the type of codec.  how about: top of an image codec

Done.

https://codereview.chromium.org/930283002/diff/180001/include/codec/SkCodec.h...
include/codec/SkCodec.h:85: bool couldRewindIfNeeded();
On 2015/02/25 16:02:19, djsollen wrote:
> call it rewindIfNeeded as the "could" makes it seem like a query that doesn't
> actually rewind.

Naming is hard...

This function does two things:

1) Rewinds if needed.
2) Returns whether it's okay to continue.

I thought that "could" helped to clarify (2) (e.g. the fact that there is a
return value and what it means). But I see your point that it makes (1) unclear.

Maybe a better name is "atBeginningOfStream". That doesn't let the reader know
that it rewinds if necessary though. Some other ideas:

rewindIfNeededOrDie()
rewindIfNeededAndReportFailure()

Had we not merged this class with SkImageGenerator, I would have put this inside
getPixels, so the subclass would not need to worry about it (and I wouldn't have
had to name this method!). (I toyed with the idea of making subclasses just
override a *different* method, which is called by onGetPixels. I don't like that
idea though...)

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:19: if (!stream->rewind()) {
On 2015/02/25 16:02:19, djsollen wrote:
> why do we have to be able to rewind the stream?  At the least it seems like
> there needs to be a TODO here to implement a stream class that wraps this
stream
> to allow for temporary rewinding.
> 
> I imagine something like...
> 
> SkCodecStream tmp(stream);
> const bool isPng = SkPngCodec::IsPng(tmp);
> tmp.rewind();  // will always return true since it has buffered the contents
> read in so far
> ... // check other codecs if needed
> tmp.rewind(); // final rewind
> tmp.stopBuffering(); // stops the tmp stream from buffering contents so rewind
> may return false if underlying stream doesn't support it.

I added a TODO, but have not specified the solution.

I definitely want to explore ways to avoid rewinding. That said, this seemed to
be the simplest way to get things started.

The thing I don't like about wrapping the stream with some kind of buffer is
that if the input stream could have rewound, we have unnecessarily paid the cost
of the extra buffer. Maybe we'll time it and decide the cost is negligible.

As implemented here, we can pass an SkFrontBufferedStream and everything will
work (the fragility there, present in the old system, is that
SkFrontBufferedStream has to know the size to buffer). It also works for the
streams we use in testing (typically an SkMemoryStream). So I think this
implementation is good enough for now.

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

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:47: SkDEBUGF(("------ png error %s\n", msg));
On 2015/02/25 16:02:20, djsollen wrote:
> I think we may want to always log this error not just in debug mode.

Oops, copied from SkImageDecoder. Done.

FWIW, by the time we replace SkImageDecoder, I actually want to make it
optional, since some users may not want to see it...

[scroggo, 2015-02-25 20:29:21]

On 2015/02/25 14:17:02, scroggo wrote:
> (One exception: it doesn't currently build on Mac. I'd like to stick with the
> same decoder on all platforms (skbug.com/2461), but we need to either build
png
> as a static library or make Mac users install png.)

Now works on Mac, by pulling in libpng and building as a static library.

PTAL.

[djsollen, 2015-02-25 20:52:15]

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

https://codereview.chromium.org/930283002/diff/180001/include/codec/SkCodec.h...
include/codec/SkCodec.h:85: bool couldRewindIfNeeded();
On 2015/02/25 18:00:54, scroggo wrote:
> On 2015/02/25 16:02:19, djsollen wrote:
> > call it rewindIfNeeded as the "could" makes it seem like a query that
doesn't
> > actually rewind.
> 
> Naming is hard...
> 
> This function does two things:
> 
> 1) Rewinds if needed.
> 2) Returns whether it's okay to continue.
> 
> I thought that "could" helped to clarify (2) (e.g. the fact that there is a
> return value and what it means). But I see your point that it makes (1)
unclear.
> 
> Maybe a better name is "atBeginningOfStream". That doesn't let the reader know
> that it rewinds if necessary though. Some other ideas:
> 
> rewindIfNeededOrDie()
> rewindIfNeededAndReportFailure()
> 
> Had we not merged this class with SkImageGenerator, I would have put this
inside
> getPixels, so the subclass would not need to worry about it (and I wouldn't
have
> had to name this method!). (I toyed with the idea of making subclasses just
> override a *different* method, which is called by onGetPixels. I don't like
that
> idea though...)

I see your dilemma, but would personally call it rewindIfNeeded() and just warn
if the return type is unused. The fact that this returns a bool, while a weak
signal, implies that you should check for failure.

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:19: if (!stream->rewind()) {
On 2015/02/25 18:00:54, scroggo wrote:
> On 2015/02/25 16:02:19, djsollen wrote:
> > why do we have to be able to rewind the stream?  At the least it seems like
> > there needs to be a TODO here to implement a stream class that wraps this
> stream
> > to allow for temporary rewinding.
> > 
> > I imagine something like...
> > 
> > SkCodecStream tmp(stream);
> > const bool isPng = SkPngCodec::IsPng(tmp);
> > tmp.rewind();  // will always return true since it has buffered the contents
> > read in so far
> > ... // check other codecs if needed
> > tmp.rewind(); // final rewind
> > tmp.stopBuffering(); // stops the tmp stream from buffering contents so
rewind
> > may return false if underlying stream doesn't support it.
> 
> I added a TODO, but have not specified the solution.
> 
> I definitely want to explore ways to avoid rewinding. That said, this seemed
to
> be the simplest way to get things started.
> 
> The thing I don't like about wrapping the stream with some kind of buffer is
> that if the input stream could have rewound, we have unnecessarily paid the
cost
> of the extra buffer. Maybe we'll time it and decide the cost is negligible.
> 
> As implemented here, we can pass an SkFrontBufferedStream and everything will
> work (the fragility there, present in the old system, is that
> SkFrontBufferedStream has to know the size to buffer). It also works for the
> streams we use in testing (typically an SkMemoryStream). So I think this
> implementation is good enough for now.

I'm fine with it just having a TODO, but think we should have a solution for
this to avoid having clients (i.e. Android APIs) require that streams are
rewindable.  In my suggested stream impl you could check to see if the stream is
rewindable and if so not bother to front buffer.

https://codereview.chromium.org/930283002/diff/220001/DEPS
File DEPS (right):

https://codereview.chromium.org/930283002/diff/220001/DEPS#newcode20
DEPS:20: "third_party/externals/libpng":
"https://github.com/glennrp/libpng.git@c6b29ff896a013961b41375c7fdac94b20513ae0",
why not git://git.code.sf.net/p/libpng/code libpng-code?  Can we also depend on
the tag libpng-1.6.16-signed

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp#newcode27
gyp/codec.gyp:27: [ 'skia_os in ["linux", "freebsd", "openbsd", "solaris",
"android", "mac", "ios"]', {
I think you can remove this condition and the one below it and just make it a
constant dependency.

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp#newcode29
gyp/codec.gyp:29: 'libpng.gyp:libpng',
this gyp file still uses the android_deps.gyp internally unless you set the
skia_libpng_static variable.  Not sure if you can set it here and have it be
picked up by the dependency or not.

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp#newcode39
gyp/codec.gyp:39: '../include/utils/mac',
is this needed?

[scroggo, 2015-02-25 21:33:52]

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

https://codereview.chromium.org/930283002/diff/180001/include/codec/SkCodec.h...
include/codec/SkCodec.h:85: bool couldRewindIfNeeded();
On 2015/02/25 20:52:15, djsollen wrote:
> On 2015/02/25 18:00:54, scroggo wrote:
> > On 2015/02/25 16:02:19, djsollen wrote:
> > > call it rewindIfNeeded as the "could" makes it seem like a query that
> doesn't
> > > actually rewind.
> > 
> > Naming is hard...
> > 
> > This function does two things:
> > 
> > 1) Rewinds if needed.
> > 2) Returns whether it's okay to continue.
> > 
> > I thought that "could" helped to clarify (2) (e.g. the fact that there is a
> > return value and what it means). But I see your point that it makes (1)
> unclear.
> > 
> > Maybe a better name is "atBeginningOfStream". That doesn't let the reader
know
> > that it rewinds if necessary though. Some other ideas:
> > 
> > rewindIfNeededOrDie()
> > rewindIfNeededAndReportFailure()
> > 
> > Had we not merged this class with SkImageGenerator, I would have put this
> inside
> > getPixels, so the subclass would not need to worry about it (and I wouldn't
> have
> > had to name this method!). (I toyed with the idea of making subclasses just
> > override a *different* method, which is called by onGetPixels. I don't like
> that
> > idea though...)
> 
> I see your dilemma, but would personally call it rewindIfNeeded() and just
warn
> if the return type is unused. The fact that this returns a bool, while a weak
> signal, implies that you should check for failure.

Done.

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/930283002/diff/180001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:19: if (!stream->rewind()) {
On 2015/02/25 20:52:15, djsollen wrote:
> On 2015/02/25 18:00:54, scroggo wrote:
> > On 2015/02/25 16:02:19, djsollen wrote:
> > > why do we have to be able to rewind the stream?  At the least it seems
like
> > > there needs to be a TODO here to implement a stream class that wraps this
> > stream
> > > to allow for temporary rewinding.
> > > 
> > > I imagine something like...
> > > 
> > > SkCodecStream tmp(stream);
> > > const bool isPng = SkPngCodec::IsPng(tmp);
> > > tmp.rewind();  // will always return true since it has buffered the
contents
> > > read in so far
> > > ... // check other codecs if needed
> > > tmp.rewind(); // final rewind
> > > tmp.stopBuffering(); // stops the tmp stream from buffering contents so
> rewind
> > > may return false if underlying stream doesn't support it.
> > 
> > I added a TODO, but have not specified the solution.
> > 
> > I definitely want to explore ways to avoid rewinding. That said, this seemed
> to
> > be the simplest way to get things started.
> > 
> > The thing I don't like about wrapping the stream with some kind of buffer is
> > that if the input stream could have rewound, we have unnecessarily paid the
> cost
> > of the extra buffer. Maybe we'll time it and decide the cost is negligible.
> > 
> > As implemented here, we can pass an SkFrontBufferedStream and everything
will
> > work (the fragility there, present in the old system, is that
> > SkFrontBufferedStream has to know the size to buffer). It also works for the
> > streams we use in testing (typically an SkMemoryStream). So I think this
> > implementation is good enough for now.
> 
> I'm fine with it just having a TODO, but think we should have a solution for
> this to avoid having clients (i.e. Android APIs) require that streams are
> rewindable.

We already have a solution: SkFrontBufferedStream. When Android has a stream
which it suspects is not rewindable, it wraps it with an SkFrontBufferedStream.
I don't think it's the best solution, but we can do it if we must.

> In my suggested stream impl you could check to see if the stream is
> rewindable and if so not bother to front buffer.

SkStream does not have a method for checking whether rewind is supported.

https://codereview.chromium.org/930283002/diff/220001/DEPS
File DEPS (right):

https://codereview.chromium.org/930283002/diff/220001/DEPS#newcode20
DEPS:20: "third_party/externals/libpng":
"https://github.com/glennrp/libpng.git@c6b29ff896a013961b41375c7fdac94b20513ae0",
On 2015/02/25 20:52:15, djsollen wrote:
> why not git://git.code.sf.net/p/libpng/code libpng-code?  Can we also depend
on
> the tag libpng-1.6.16-signed

Ok, I updated it to pull from SourceForge at commit
070a616b8275277e18ef8ee91e2ca23f7bdc67d5, which appears to be the commit with
that tag.

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp#newcode27
gyp/codec.gyp:27: [ 'skia_os in ["linux", "freebsd", "openbsd", "solaris",
"android", "mac", "ios"]', {
On 2015/02/25 20:52:15, djsollen wrote:
> I think you can remove this condition and the one below it and just make it a
> constant dependency.

Done. This explains my Windows trybot failure, I think...

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp#newcode29
gyp/codec.gyp:29: 'libpng.gyp:libpng',
On 2015/02/25 20:52:15, djsollen wrote:
> this gyp file still uses the android_deps.gyp internally unless you set the
> skia_libpng_static variable.  Not sure if you can set it here and have it be
> picked up by the dependency or not.

I set it in common_variables.

https://codereview.chromium.org/930283002/diff/220001/gyp/codec.gyp#newcode39
gyp/codec.gyp:39: '../include/utils/mac',
On 2015/02/25 20:52:15, djsollen wrote:
> is this needed?

No. Removed.

[scroggo, 2015-02-26 00:58:00]

https://codereview.chromium.org/930283002/diff/300001/gyp/libpng.gyp
File gyp/libpng.gyp (right):

https://codereview.chromium.org/930283002/diff/300001/gyp/libpng.gyp#newcode55
gyp/libpng.gyp:55: 'action': ['<(skia_copy)', '<(prebuilt)', '<(generated)'],
This did not work on windows (Couldn't find the file?) so I tried writing a
python script, which worked.

I'm on to another windows failure, dealing with zlib.h. I think Hal/Sunshine
recently did something special for zlib for Windows, so I'll go find that.

[scroggo, 2015-02-26 16:05:19]

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

[scroggo, 2015-02-26 16:05:20]

mtklein@,

My PNG decoder depends on libpng, which I'm trying to build as a static library
on mac, ios and win. On windows I'm failing because of a lack of zlib. I know
you have been dealing with zlib lately; what do you recommend? Should I use
miniz?

[mtklein, 2015-02-26 16:40:13]

On 2015/02/26 16:05:20, scroggo wrote:
> mtklein@,
> 
> My PNG decoder depends on libpng, which I'm trying to build as a static
library
> on mac, ios and win. On windows I'm failing because of a lack of zlib. I know
> you have been dealing with zlib lately; what do you recommend? Should I use
> miniz?

We've just been avoiding the problem by not using libpng or zlib on Windows.

Our PDF backend also uses zlib directly (optionally: Chrome uses it everywhere,
we use it everywhere but our Windows bots).
I think we could probably use miniz instead.   I'll feel that out.  It'd be nice
to eliminate that variability in testing our PDF backend.

Now, I don't know whether we can make libpng use miniz.  miniz claims it's
possible.

[scroggo, 2015-02-26 17:45:36]

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp#newcode50
gyp/codec.gyp:50: 'android_deps.gyp:png',
On 2015/02/25 18:00:54, scroggo wrote:
> On 2015/02/25 16:02:19, djsollen wrote:
> > for android we should link in the most recent copy of libpng not the one
that
> > SkImageDecoder uses.
> 
> Done.

I neglected to pay attention to the fact that libpng.gyp, when not building with
skia_libpng_static, makes Android pull in android_deps, which in turn pulls in
dependencies, which builds a different static library - the one with the Android
mods. Latest patch set fixes this for real - we now set skia_libpng_static on
Android, so it statically builds the right version of libpng.

Derek, is that the right thing to do? Will there be any problems with codec (a
static library) including one static libpng and images (another static library)
including a different static libpng?

[scroggo, 2015-02-26 18:08:28]

On 2015/02/26 17:45:36, scroggo wrote:
> https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp
> File gyp/codec.gyp (right):
> 
> https://codereview.chromium.org/930283002/diff/180001/gyp/codec.gyp#newcode50
> gyp/codec.gyp:50: 'android_deps.gyp:png',
> On 2015/02/25 18:00:54, scroggo wrote:
> > On 2015/02/25 16:02:19, djsollen wrote:
> > > for android we should link in the most recent copy of libpng not the one
> that
> > > SkImageDecoder uses.
> > 
> > Done.
> 
> I neglected to pay attention to the fact that libpng.gyp, when not building
with
> skia_libpng_static, makes Android pull in android_deps, which in turn pulls in
> dependencies, which builds a different static library - the one with the
Android
> mods. Latest patch set fixes this for real - we now set skia_libpng_static on
> Android, so it statically builds the right version of libpng.
> 
> Derek, is that the right thing to do? Will there be any problems with codec (a
> static library) including one static libpng and images (another static
library)
> including a different static libpng?

Nope (not the right thing to do). From
http://build.chromium.org/p/client.skia.android/builders/Test-Android-Nexus10...
:

ninja: warning: multiple rules generate obj/gyp/libpng.a. builds involving this
target will not be correct; continuing anyway

(And the build later fails.)

I didn't run into this building locally, but I think the solution will be fairly
straightforward - rename one of them (or both, perhaps, for clarity).

[scroggo, 2015-02-26 18:45:43]

On 2015/02/26 djsollen wrote:
> On 2015/02/26 18:08:28, scroggo wrote:
>> I didn't run into this building locally, but I think the solution will be
fairly
>> straightforward - rename one of them (or both, perhaps, for clarity).
>
> I think that is the right approach, but I'm afraid that the two versions of
> libpng are going to have symbol conflicts when they all get jammed in
> the same library.

Oh, good point. I just uploaded a CL trying that approach, and I'm running a
trybot. If that doesn't work, I propose letting Android continue to use the
Android customized version until we can find a workaround.

[scroggo, 2015-02-26 19:45:45]

On 2015/02/26 18:45:43, scroggo wrote:
> On 2015/02/26 djsollen wrote:
> > On 2015/02/26 18:08:28, scroggo wrote:
> >> I didn't run into this building locally, but I think the solution will be
> fairly
> >> straightforward - rename one of them (or both, perhaps, for clarity).
> >
> > I think that is the right approach, but I'm afraid that the two versions of
> > libpng are going to have symbol conflicts when they all get jammed in
> > the same library.
> 
> Oh, good point. I just uploaded a CL trying that approach, and I'm running a
> trybot. If that doesn't work, I propose letting Android continue to use the
> Android customized version until we can find a workaround.

As you feared, that did not work. I've reuploaded patchset 17 (with a small
change to copy_file). I still need to fix the windows build (hopefully with
https://codereview.chromium.org/957323003/ and some more changes I can run using
miniz) and the iOS build.

[scroggo, 2015-02-26 20:34:21]

https://codereview.chromium.org/930283002/diff/400001/gyp/libpng.gyp
File gyp/libpng.gyp (right):

https://codereview.chromium.org/930283002/diff/400001/gyp/libpng.gyp#newcode41
gyp/libpng.gyp:41: 'PNG_ARM_NEON_OPT=0',
Does someone who knows NEON better than I do (I think we have a few on this CL)
have an idea why this is needed?

I'm trying to build for iOS using the following GYP_DEFINES:
"skia_os='ios' skia_arch_type='arm' armv7=1 arm_neon=0"

(This appears to be how we build iOS on the trybot I tried; part of that is
redundant, but oh well.)

It looks like __ARM_NEON is incorrectly defined, but I haven't figured out how.

[mtklein, 2015-02-26 20:41:14]

https://codereview.chromium.org/930283002/diff/340001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/930283002/diff/340001/dm/DMSrcSink.cpp#newcode52
dm/DMSrcSink.cpp:52: SkAutoTDelete<SkCodec>
codec(SkCodec::NewFromData(encoded));
You might want to split off a CodecSrc as a separate src type from ImageSrc,
enabled by something like --config codec-8888.  That way you can run both the
old and the new paths in the same run (for local testing, for the bots,
whatever).

[scroggo, 2015-02-26 21:00:12]

https://codereview.chromium.org/930283002/diff/340001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/930283002/diff/340001/dm/DMSrcSink.cpp#newcode52
dm/DMSrcSink.cpp:52: SkAutoTDelete<SkCodec>
codec(SkCodec::NewFromData(encoded));
On 2015/02/26 20:41:13, mtklein wrote:
> You might want to split off a CodecSrc as a separate src type from ImageSrc,
> enabled by something like --config codec-8888.  That way you can run both the
> old and the new paths in the same run (for local testing, for the bots,
> whatever).

Thanks for the feedback. This is really a stopgap measure, allowing me/Matt to
run locally. I want to expand the image testing to test various aspects of
SkCodec. See skbug.com/3475 for more info.

[scroggo, 2015-02-26 22:55:34]

On 2015/02/26 16:40:13, mtklein wrote:
> On 2015/02/26 16:05:20, scroggo wrote:
> > mtklein@,
> > 
> > My PNG decoder depends on libpng, which I'm trying to build as a static
> library
> > on mac, ios and win. On windows I'm failing because of a lack of zlib. I
know
> > you have been dealing with zlib lately; what do you recommend? Should I use
> > miniz?
> 
> We've just been avoiding the problem by not using libpng or zlib on Windows.
> 
> Our PDF backend also uses zlib directly (optionally: Chrome uses it
everywhere,
> we use it everywhere but our Windows bots).
> I think we could probably use miniz instead.   I'll feel that out.  It'd be
nice
> to eliminate that variability in testing our PDF backend.
> 
> Now, I don't know whether we can make libpng use miniz.  miniz claims it's
> possible.

No success, so far. There's a bug filed (hasn't been acknowledged by miniz
developer): https://code.google.com/p/miniz/issues/detail?id=36

The site does claim that it works though. I wonder if that was using an older
version of libpng and/or miniz. Interestingly, I came across an older version of
miniz (possibly modified -
https://github.com/libretro/instancingviewer-camera/blob/master/miniz.h) which
declares inflateReset (which libpng needs and is preventing me from building
with miniz) but never defines it anywhere.

I'll try emailing the developer to try to figure out the issue.

If we cannot use miniz, I think we have the following options:
- Use zlib on windows (what is the problem we're avoiding by not using it?)
- Manually install libpng on Windows bots, and require Windows developers to
install it, too (or will that run into the same zlib problems?)
- Write a separate SkCodec for PNGs on Windows
- Disable SkPngCodec on Windows

Anything else? I'd rather use the same codecs for all platforms (less platform
specific code to use and test; skbug.com/2461). Since I'm primarily focused on
Android, disabling on Windows seems like the most expedient option....

[mtklein, 2015-02-26 23:17:18]

> If we cannot use miniz, I think we have the following options:
I'm in favor of your options in exactly the order you present them.  If we do
get zlib working on Windows, I would certainly want to build and use the same
code for all our bots, for SkPngCodec and for SkFlate both.  No need to keep
miniz around at that point.

> - Use zlib on windows (what is the problem we're avoiding by not using it?)
I'm not really sure.  Code is code after all.  Why don't we try doing this the
next time we're in the office near a Windows machine?  Or I guess start with
Linux/Mac, then try Windows when they're working?  Pretty sure Chrome does use
zlib on Windows, so we can perhaps crib from them.

> - Manually install libpng on Windows bots, and require Windows developers to
> install it, too (or will that run into the same zlib problems?)
If we manually install libpng and zlib, no problems.

> - Write a separate SkCodec for PNGs on Windows
This is sort of what we do now, right?  Going via WIC?

> - Disable SkPngCodec on Windows
We will not be defeated.

[scroggo, 2015-02-26 23:23:41]

On 2015/02/26 23:17:18, mtklein wrote:
> > If we cannot use miniz, I think we have the following options:
> I'm in favor of your options in exactly the order you present them.  If we do
> get zlib working on Windows, I would certainly want to build and use the same
> code for all our bots, for SkPngCodec and for SkFlate both.  No need to keep
> miniz around at that point.
> 
> > - Use zlib on windows (what is the problem we're avoiding by not using it?)
> I'm not really sure.  Code is code after all.  Why don't we try doing this the
> next time we're in the office near a Windows machine?  Or I guess start with
> Linux/Mac, then try Windows when they're working?  Pretty sure Chrome does use
> zlib on Windows, so we can perhaps crib from them.

Good idea. I'll see what Chrome does.

> 
> > - Manually install libpng on Windows bots, and require Windows developers to
> > install it, too (or will that run into the same zlib problems?)
> If we manually install libpng and zlib, no problems.
> 
> > - Write a separate SkCodec for PNGs on Windows
> This is sort of what we do now, right?  Going via WIC?
> 
> > - Disable SkPngCodec on Windows
> We will not be defeated.

:)

[scroggo, 2015-03-02 17:01:30]

On 2015/02/26 23:23:41, scroggo wrote:
> On 2015/02/26 23:17:18, mtklein wrote:
> > > If we cannot use miniz, I think we have the following options:
> > I'm in favor of your options in exactly the order you present them.  If we
do
> > get zlib working on Windows, I would certainly want to build and use the
same
> > code for all our bots, for SkPngCodec and for SkFlate both.  No need to keep
> > miniz around at that point.
> > 
> > > - Use zlib on windows (what is the problem we're avoiding by not using
it?)
> > I'm not really sure.  Code is code after all.  Why don't we try doing this
the
> > next time we're in the office near a Windows machine?  Or I guess start with
> > Linux/Mac, then try Windows when they're working?  Pretty sure Chrome does
use
> > zlib on Windows, so we can perhaps crib from them.
> 
> Good idea. I'll see what Chrome does.
> 
> > 
> > > - Manually install libpng on Windows bots, and require Windows developers
to
> > > install it, too (or will that run into the same zlib problems?)
> > If we manually install libpng and zlib, no problems.
> > 
> > > - Write a separate SkCodec for PNGs on Windows
> > This is sort of what we do now, right?  Going via WIC?
> > 
> > > - Disable SkPngCodec on Windows
> > We will not be defeated.
> 
> :)

I went with option 1: I have landed https://codereview.chromium.org/966963002/
(DIFFERENT ISSUE) which allows us to have zlib on Windows (using Chromium's
version of zlib), so we can now support Windows with SkPngCodec. PTAL.

[djsollen, 2015-03-02 20:08:42]

lgtm

https://codereview.chromium.org/930283002/diff/460001/DEPS
File DEPS (right):

https://codereview.chromium.org/930283002/diff/460001/DEPS#newcode21
DEPS:21: "third_party/externals/libpng"  :
"git://git.code.sf.net/p/libpng/code@070a616b8275277e18ef8ee91e2ca23f7bdc67d5",
FYI...you can use the tag instead of the git hash.

[scroggo, 2015-03-02 20:09:58]

https://codereview.chromium.org/930283002/diff/460001/DEPS
File DEPS (right):

https://codereview.chromium.org/930283002/diff/460001/DEPS#newcode21
DEPS:21: "third_party/externals/libpng"  :
"git://git.code.sf.net/p/libpng/code@070a616b8275277e18ef8ee91e2ca23f7bdc67d5",
On 2015/03/02 20:08:42, djsollen wrote:
> FYI...you can use the tag instead of the git hash.

I take it that's what we're doing for freetype, up above?

[scroggo, 2015-03-02 20:10:05]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-03-02 20:10:05]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/930283002/#ps460001
(title: "Remove extraneous patch file.")

[commit-bot: I haz the power, 2015-03-02 20:11:41]

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

[commit-bot: I haz the power, 2015-03-02 20:23:53]

Committed patchset #24 (id:460001) as
https://skia.googlesource.com/skia/+/ca358852b4fed656d11107b2aaf28318a4518b49

[scroggo, 2015-03-02 20:29:08]

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

The reason for reverting is: Breaking windows bots all over the place :(.

[scroggo, 2015-03-03 16:50:44]

On 2015/03/02 20:29:08, scroggo wrote:
> A revert of this CL (patchset #24 id:460001) has been created in
> https://codereview.chromium.org/972743003/ by mailto:scroggo@google.com.
> 
> The reason for reverting is: Breaking windows bots all over the place :(.

FYI: Latest patch set includes two offshoots:
1) Check in pnglibconf.h directly (from
https://codereview.chromium.org/973823002 - DIFFERENT ISSUE)
2) Build libpng as a static library everywhere but Android framework
(https://codereview.chromium.org/964293003/ - DIFFERENT ISSUE)

1 fixes the Windows build (there is a trybot failure on iOS, but that is due to
a broken tree). 2 makes us consistent.

[scroggo, 2015-03-03 16:51:44]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-03-03 16:51:44]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/930283002/#ps500001
(title: "Check in prebuilt file directly")

[commit-bot: I haz the power, 2015-03-03 16:52:31]

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

[commit-bot: I haz the power, 2015-03-03 16:59:24]

Committed patchset #26 (id:500001) as
https://skia.googlesource.com/skia/+/f24f2247c25b842327e12c70e44efe4cc1b28dfa