Enable both static and dynamically linked libpng

Enable both static and dynamically linked libpng
    
All platforms except android are configured to use the statically linked copy of libpng.  Android uses the system provided dynamic copy for SkImageDecoder and the static copy for SkCodec. The exception being android framework builds that currently use the dynamic copy everywhere.
    
This CL also enables NEON optimizations for libpng.

Committed: https://skia.googlesource.com/skia/+/2469c999518e7b0063d35e9e2eb074a0477c21ac

[djsollen, 2015-03-26 16:39:11]

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

[djsollen, 2015-03-26 16:39:12]

https://codereview.chromium.org/1032253003/diff/1/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1032253003/diff/1/gyp/codec.gyp#newcode20
gyp/codec.gyp:20: 'libpng.gyp:libpng_static',
need to make this conditional so that the framework build still used the old
library until we can prove the overhead of the newer library is worth it.

[scroggo, 2015-03-26 17:19:02]

Other than my comments, it looks good, thanks!

https://codereview.chromium.org/1032253003/diff/1/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1032253003/diff/1/gyp/codec.gyp#newcode20
gyp/codec.gyp:20: 'libpng.gyp:libpng_static',
On 2015/03/26 16:39:12, djsollen wrote:
> need to make this conditional so that the framework build still used the old
> library until we can prove the overhead of the newer library is worth it.

I'm not sure what you mean by this comment. Is this a TODO? It seems like this
won't build on the framework right now, since we don't have
third_party/externals.

https://codereview.chromium.org/1032253003/diff/1/third_party/libpng/pnglibco...
File third_party/libpng/pnglibconf.h (right):

https://codereview.chromium.org/1032253003/diff/1/third_party/libpng/pnglibco...
third_party/libpng/pnglibconf.h:209: #define PNG_PREFIX skia_
I think it would be good to add some sort of comment that this is a modification
we made. It would be easy to glance at this long file (plus comments elsewhere,
like in the gyp file that state that it is "generated" and README.google that
says it's just a rename) and assume it's a standard copy of the public png file.

[djsollen, 2015-04-01 15:12:43]

https://codereview.chromium.org/1032253003/diff/1/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1032253003/diff/1/gyp/codec.gyp#newcode20
gyp/codec.gyp:20: 'libpng.gyp:libpng_static',
On 2015/03/26 17:19:02, scroggo wrote:
> On 2015/03/26 16:39:12, djsollen wrote:
> > need to make this conditional so that the framework build still used the old
> > library until we can prove the overhead of the newer library is worth it.
> 
> I'm not sure what you mean by this comment. Is this a TODO? It seems like this
> won't build on the framework right now, since we don't have
> third_party/externals.

This is addressed in my next patch.

[djsollen, 2015-04-01 15:13:05]

The CQ bit was checked by djsollen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-01 15:13:19]

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

[commit-bot: I haz the power, 2015-04-01 15:13:22]

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

[commit-bot: I haz the power, 2015-04-01 15:13:23]

Failed to apply patch for gyp/codec.gyp:
While running git apply --index -3 -p1;
  error: patch failed: gyp/codec.gyp:17
  Falling back to three-way merge...
  Applied patch to 'gyp/codec.gyp' with conflicts.
  U gyp/codec.gyp

Patch:       gyp/codec.gyp
Index: gyp/codec.gyp
diff --git a/gyp/codec.gyp b/gyp/codec.gyp
index
ee8eaad003ec14360c4db9135f3dc73681f32672..4bf9b1ca2e4bcf67f8bab70053024bb88d3c3e36
100644
--- a/gyp/codec.gyp
+++ b/gyp/codec.gyp
@@ -17,7 +17,6 @@
       'standalone_static_library': 1,
       'dependencies': [
         'core.gyp:*',
-        'libpng.gyp:libpng',
       ],
       'cflags':[
         # FIXME: This gets around a longjmp warning. See
@@ -43,6 +42,21 @@
           '../include/codec',
         ],
       },
+      'conditions': [
+        [ 'skia_android_framework == 1',
+          {
+            # TODO(djsollen): this is a temporary dependency until we can
update
+            # the android framework to a more recent version of libpng.
+            'dependencies': [
+              'libpng.gyp:libpng',
+            ],
+          }, {  # !skia_android_framework
+            'dependencies': [
+              'libpng.gyp:libpng_static',
+            ],
+          }
+        ]
+      ]
     },
   ],
 }

[djsollen, 2015-04-01 15:15:40]

The CQ bit was checked by djsollen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-01 15:15:53]

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

[commit-bot: I haz the power, 2015-04-01 15:29:17]

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

[commit-bot: I haz the power, 2015-04-01 15:29:18]

This issue passed the CQ dry run.

[scroggo, 2015-04-01 15:40:21]

> Enable both static and dynamically linked libpng

Could you add remarks about the following:
Now the version of png we build uses neon optimizations.
One of these is used by codec, the other by image decoder.

https://codereview.chromium.org/1032253003/diff/40001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1032253003/diff/40001/gyp/codec.gyp#newcode52
gyp/codec.gyp:52: # TODO(djsollen): this is a temporary dependency until we can
update
So if I understand correctly, this will make it so the framework build uses the
same libpng for both codec and image decoder?

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp
File gyp/libpng.gyp (right):

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode39
gyp/libpng.gyp:39: # Needed for generated pnglibconf.h
and pngprefix.h

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode52
gyp/libpng.gyp:52: # Needed for generated pnglibconf.h
and pngprefix.h

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode63
gyp/libpng.gyp:63: 'defines': [
spacing?

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode89
gyp/libpng.gyp:89: '../third_party/externals/libpng/arm/arm_init.c',
Maybe add a comment to the description that you're also enabling neon?

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode95
gyp/libpng.gyp:95: # not sure if I need this
I assume this replaces my old FIXME?

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

https://codereview.chromium.org/1032253003/diff/40001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.h:16: #ifdef SKIA_PNG_PREFIXED
nit: four space indents?

https://codereview.chromium.org/1032253003/diff/40001/src/images/SkImageDecod...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/1032253003/diff/40001/src/images/SkImageDecod...
src/images/SkImageDecoder_libpng.cpp:22: #ifdef SKIA_PNG_PREFIXED
nit: Why are you using two space indents?

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/READ...
File third_party/libpng/README.google (right):

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/READ...
third_party/libpng/README.google:12: Q: Why does this directory exist?
It seems like we also need it for pngprefix.h?

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/pngl...
File third_party/libpng/pnglibconf.h (right):

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/pngl...
third_party/libpng/pnglibconf.h:214: #define PNG_PREFIX skia_
Is this used internally by PNG? It seems weird that we have this here *and* we
define each symbol to be skia_symbol (without using the PNG_PREFIX macro). Not
saying it's wrong; just that I don't understand it.

[scroggo, 2015-04-01 16:30:04]

besides my comments, lgtm

[djsollen, 2015-04-01 17:19:01]

https://codereview.chromium.org/1032253003/diff/40001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1032253003/diff/40001/gyp/codec.gyp#newcode52
gyp/codec.gyp:52: # TODO(djsollen): this is a temporary dependency until we can
update
On 2015/04/01 15:40:20, scroggo wrote:
> So if I understand correctly, this will make it so the framework build uses
the
> same libpng for both codec and image decoder?

Yes.

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp
File gyp/libpng.gyp (right):

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode39
gyp/libpng.gyp:39: # Needed for generated pnglibconf.h
On 2015/04/01 15:40:20, scroggo wrote:
> and pngprefix.h

Done.

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode52
gyp/libpng.gyp:52: # Needed for generated pnglibconf.h
On 2015/04/01 15:40:20, scroggo wrote:
> and pngprefix.h

Done.

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode63
gyp/libpng.gyp:63: 'defines': [
On 2015/04/01 15:40:20, scroggo wrote:
> spacing?

Done.

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode89
gyp/libpng.gyp:89: '../third_party/externals/libpng/arm/arm_init.c',
On 2015/04/01 15:40:20, scroggo wrote:
> Maybe add a comment to the description that you're also enabling neon?

Done.

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode95
gyp/libpng.gyp:95: # not sure if I need this
On 2015/04/01 15:40:20, scroggo wrote:
> I assume this replaces my old FIXME?

Done.

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

https://codereview.chromium.org/1032253003/diff/40001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.h:16: #ifdef SKIA_PNG_PREFIXED
On 2015/04/01 15:40:20, scroggo wrote:
> nit: four space indents?

Done.

https://codereview.chromium.org/1032253003/diff/40001/src/images/SkImageDecod...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/1032253003/diff/40001/src/images/SkImageDecod...
src/images/SkImageDecoder_libpng.cpp:22: #ifdef SKIA_PNG_PREFIXED
On 2015/04/01 15:40:20, scroggo wrote:
> nit: Why are you using two space indents?

Done.

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/READ...
File third_party/libpng/README.google (right):

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/READ...
third_party/libpng/README.google:12: Q: Why does this directory exist?
On 2015/04/01 15:40:21, scroggo wrote:
> It seems like we also need it for pngprefix.h?

Done.

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/pngl...
File third_party/libpng/pnglibconf.h (right):

https://codereview.chromium.org/1032253003/diff/40001/third_party/libpng/pngl...
third_party/libpng/pnglibconf.h:214: #define PNG_PREFIX skia_
On 2015/04/01 15:40:21, scroggo wrote:
> Is this used internally by PNG? It seems weird that we have this here *and* we
> define each symbol to be skia_symbol (without using the PNG_PREFIX macro). Not
> saying it's wrong; just that I don't understand it.

This source files for libpng include png_priv.h which uses this prefix to mangle
the names of all of it's functions.  However, when including png.h the library
doesn't mangle the function names which is why we need to include pngprefix.h to
make sure that our client code can correctly resolve the symbols.

[scroggo, 2015-04-01 17:20:48]

lgtm

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp
File gyp/libpng.gyp (right):

https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode95
gyp/libpng.gyp:95: # not sure if I need this
On 2015/04/01 17:19:01, djsollen wrote:
> On 2015/04/01 15:40:20, scroggo wrote:
> > I assume this replaces my old FIXME?
> 
> Done.

No longer needed after all?

[djsollen, 2015-04-01 17:24:51]

On 2015/04/01 17:20:48, scroggo wrote:
> lgtm
> 
> https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp
> File gyp/libpng.gyp (right):
> 
> https://codereview.chromium.org/1032253003/diff/40001/gyp/libpng.gyp#newcode95
> gyp/libpng.gyp:95: # not sure if I need this
> On 2015/04/01 17:19:01, djsollen wrote:
> > On 2015/04/01 15:40:20, scroggo wrote:
> > > I assume this replaces my old FIXME?
> > 
> > Done.
> 
> No longer needed after all?

After we added the additional NEON defines in pnglibconf.h that gets defined
automatically now.

[djsollen, 2015-04-01 17:24:57]

The CQ bit was checked by djsollen@google.com

[commit-bot: I haz the power, 2015-04-01 17:25:12]

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

[commit-bot: I haz the power, 2015-04-01 17:34:00]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/2469c999518e7b0063d35e9e2eb074a0477c21ac

[djsollen, 2015-04-01 18:19:38]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1050183002/ by djsollen@google.com.

The reason for reverting is: breaking the nexus_9 and ios builds..