Update libpng to 1.6.22

Update libpng to 1.6.22

BUG=599917
BUG=618061
Committed: https://crrev.com/8c0ac8d882561cca77d5184248708b0ce9c851d4
Cr-Commit-Position: refs/heads/master@{#399464}

[msarett, 2016-06-01 14:16:52]

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

[commit-bot: I haz the power, 2016-06-01 14:17:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/30001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/30001

[commit-bot: I haz the power, 2016-06-01 14:25:14]

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

[commit-bot: I haz the power, 2016-06-01 14:25:15]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[msarett, 2016-06-01 14:37:11]

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

[commit-bot: I haz the power, 2016-06-01 14:37:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/40001

[commit-bot: I haz the power, 2016-06-01 14:49:09]

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

[commit-bot: I haz the power, 2016-06-01 14:49:10]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[msarett, 2016-06-01 15:17:11]

Patchset #5 (id:40001) has been deleted

[msarett, 2016-06-01 15:17:45]

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

[commit-bot: I haz the power, 2016-06-01 15:18:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/50001

[commit-bot: I haz the power, 2016-06-01 15:30:20]

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

[commit-bot: I haz the power, 2016-06-01 15:30:21]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett, 2016-06-01 16:36:18]

Patchset #6 (id:60001) has been deleted

[msarett, 2016-06-01 16:36:27]

Patchset #6 (id:70001) has been deleted

[msarett, 2016-06-01 16:38:40]

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

[commit-bot: I haz the power, 2016-06-01 16:39:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/80001

[commit-bot: I haz the power, 2016-06-01 16:43:31]

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

[commit-bot: I haz the power, 2016-06-01 16:43:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msarett, 2016-06-01 16:44:53]

Patchset #6 (id:80001) has been deleted

[msarett, 2016-06-01 16:49:12]

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

[commit-bot: I haz the power, 2016-06-01 16:49:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/90001

[commit-bot: I haz the power, 2016-06-01 17:04:49]

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

[commit-bot: I haz the power, 2016-06-01 17:04:50]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett, 2016-06-01 18:48:14]

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

[commit-bot: I haz the power, 2016-06-01 18:48:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/100001

[commit-bot: I haz the power, 2016-06-01 18:53:42]

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

[commit-bot: I haz the power, 2016-06-01 18:53:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett, 2016-06-01 18:54:42]

Patchset #7 (id:100001) has been deleted

[msarett, 2016-06-01 18:56:27]

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

[commit-bot: I haz the power, 2016-06-01 18:57:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/110001

[commit-bot: I haz the power, 2016-06-01 19:05:25]

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

[commit-bot: I haz the power, 2016-06-01 19:05:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[msarett, 2016-06-01 19:08:48]

Patchset #7 (id:110001) has been deleted

[msarett, 2016-06-01 19:16:00]

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

[commit-bot: I haz the power, 2016-06-01 19:16:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/120001

[commit-bot: I haz the power, 2016-06-01 19:29:55]

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

[commit-bot: I haz the power, 2016-06-01 19:29:57]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett, 2016-06-01 21:19:13]

Patchset #7 (id:120001) has been deleted

[msarett, 2016-06-01 21:19:30]

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

[commit-bot: I haz the power, 2016-06-01 21:20:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/130001

[commit-bot: I haz the power, 2016-06-01 21:35:06]

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

[commit-bot: I haz the power, 2016-06-01 21:35:07]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett, 2016-06-01 21:54:57]

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

[commit-bot: I haz the power, 2016-06-01 21:55:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/140001

[commit-bot: I haz the power, 2016-06-01 22:10:05]

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

[commit-bot: I haz the power, 2016-06-01 22:10:08]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett, 2016-06-01 22:59:08]

Patchset #8 (id:140001) has been deleted

[msarett, 2016-06-01 23:01:12]

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

[commit-bot: I haz the power, 2016-06-01 23:03:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/150001

[commit-bot: I haz the power, 2016-06-01 23:22:28]

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

[commit-bot: I haz the power, 2016-06-01 23:22:30]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett, 2016-06-02 15:03:24]

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

[commit-bot: I haz the power, 2016-06-02 15:03:50]

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

[msarett, 2016-06-02 15:47:45]

Patchset #8 (id:150001) has been deleted

[msarett, 2016-06-02 16:03:12]

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

[commit-bot: I haz the power, 2016-06-02 16:03:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021403002/170001

[msarett, 2016-06-02 17:02:42]

Patchset #8 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-06-02 17:36:57]

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

[commit-bot: I haz the power, 2016-06-02 17:36:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msarett, 2016-06-03 16:40:46]

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

[commit-bot: I haz the power, 2016-06-03 16:41:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/180001

[commit-bot: I haz the power, 2016-06-03 18:11:43]

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

[commit-bot: I haz the power, 2016-06-03 18:11:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msarett, 2016-06-03 19:33:16]

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

[commit-bot: I haz the power, 2016-06-03 19:33:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/200001

[msarett, 2016-06-03 20:18:49]

msarett@google.com changed reviewers:
+ kcc@chromium.org, ryansturm@chromium.org, scroggo@chromium.org,
thakis@chromium.org

[msarett, 2016-06-03 20:18:52]

Please take a look!

third_party/WebKit
scroggo@
ryansturm@ (for changes to dimensions location logging)

testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc
kcc@

third_party/libpng*
thakis@
scroggo@

*Interesting files to look at are those that are not taken directly from libpng:
BUILD.gn
README.chromium
libpng.gyp
pnglibconf.h
pngprefix.h

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:53: crbug.com/599917
css2.1/20110323/inline-replaced-width-014.htm [ NeedsRebaseline ]
Multi-platform rebaselines:

libpng has known pixel differences between major versions.

These are due to changes in the implementation of png_set_gamma().  The max
pixel diff is 1 and the difference is not visually noticeable.

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:57: crbug.com/599917
fast/images/png-color-profile-ignore-gamma.html [ NeedsRebaseline ]
This test is actually a real diff.  It now renders with a green background,
which is the correct behavior.  It must have been broken at some point or never
fixed properly.
https://bugs.chromium.org/p/chromium/issues/detail?id=354885

Updating to libpng 1.6 fixes a known gamma bug in 1.2.

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:74: crbug.com/599917 [ Mac ]
compositing/masks/direct-image-mask.html [ NeedsManualRebaseline ]
Mac rebaselines:

I'm not sure why Mac has so many more tests to rebaseline.  There isn't really
anything interestingly different in libpng for Mac vs. Linux vs. Windows.  Maybe
Mac just has more tests?

I've marked these as manual rebaselines so I can view the images while
rebaselining.  I don't have a Mac set-up, so I haven't been able to view these
yet.

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:187:
dimensionsLocationHistogram.count(m_reader->getReadOffset() -
png->current_buffer_size - 1);
@ryansturm

libpng 1.6 has a stricter API that does not allow direct access to the
png_structp.  I'm not aware that there is a way to get this information anymore.
 What is the impact of this metric overestimating a little bit?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/png...
File third_party/libpng/pnglibconf.h (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/png...
third_party/libpng/pnglibconf.h:21: /*#undef PNG_ARM_NEON_API_SUPPORTED*/
Our config file for libpng 1.2 tried to turn off all unused features to
"minimize attack surface".

I do the same here.  It's a real pain.  libpng 1.6 provides way more ability to
turn things on/off, and I'm not really sure that we even want this much
flexibility.

Ex: If we forget to turn on PNG_READ16_BIT_SUPPORTED, some rare PNGs silently
fail to decode.  If we forget to turn on PNG_WRITE_FILTER_FUNCTIONS, our png
encodes will result in non-optimally large file sizes. etc.

Further, many combinations of options being on/off result in the library failing
to compile.

This configuration is somewhat minimal, compiles, and works as we expect.  FWIW,
I'd prefer to not bother to modify this file and to use the default config.

[Nico, 2016-06-03 20:27:45]

nice!

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:239:
#if (PNG_LIBPNG_VER < 10500)
looks like you can tweak this instead of casting?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
File third_party/libpng/BUILD.gn (left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:42: "png.h",
what happened to png.h?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:13: #"toolsets": ["target", "host"],
remove this if, it doesn't do anything

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:30: # Upstream uses self-assignment to avoid
warnings.
do we still need -Wno-self-assign then?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:33:
"-Wno-tautological-constant-out-of-range-compare",
is there an upstream bug for this?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
File third_party/libpng/README.chromium (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
third_party/libpng/README.chromium:11: - Use custom configuration file
pnglibconf.h, which turns off all features that
aha; list that in the gyp / gn files then

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
third_party/libpng/README.chromium:14: pdfium's copy of libpng.
i think pdfium is supposed to use chromium's libpng when it's part of a chromium
build and its own copy else. both copies should never compete with each other.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
File third_party/libpng/libpng.gyp (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:15: '-Wno-self-assign',
why does the gn file have that tautological-compare thing but this file doesn't?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:47: # to compile the optimization files.  This
works fine on GN.
what's the error if you don't do this?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:97: 'toolsets': ['target', 'host'],
why is this no longer needed for chromecast==1?

[kcc2, 2016-06-03 20:30:58]

LGTM for testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc

[RyanSturm, 2016-06-03 20:31:51]

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:187:
dimensionsLocationHistogram.count(m_reader->getReadOffset() -
png->current_buffer_size - 1);
On 2016/06/03 20:18:51, msarett wrote:
> @ryansturm
> 
> libpng 1.6 has a stricter API that does not allow direct access to the
> png_structp.  I'm not aware that there is a way to get this information
anymore.
>  What is the impact of this metric overestimating a little bit?

You can remove the Blink.DecodedImage.EffectiveDimensionsLocation.PNG histogram.
We have enough data from canary/dev, and don't expect much to change.

[msarett, 2016-06-03 21:16:00]

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

[commit-bot: I haz the power, 2016-06-03 21:16:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/210001

[msarett, 2016-06-03 21:26:01]

Many thanks for the fast reviews!

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:187:
dimensionsLocationHistogram.count(m_reader->getReadOffset() -
png->current_buffer_size - 1);
On 2016/06/03 20:31:51, RyanSturm wrote:
> On 2016/06/03 20:18:51, msarett wrote:
> > @ryansturm
> > 
> > libpng 1.6 has a stricter API that does not allow direct access to the
> > png_structp.  I'm not aware that there is a way to get this information
> anymore.
> >  What is the impact of this metric overestimating a little bit?
> 
> You can remove the Blink.DecodedImage.EffectiveDimensionsLocation.PNG
histogram.
> We have enough data from canary/dev, and don't expect much to change.

Got it thanks!  It's been removed.

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:239:
#if (PNG_LIBPNG_VER < 10500)
On 2016/06/03 20:27:44, Nico wrote:
> looks like you can tweak this instead of casting?

png_get_iCCP needs profile to be an unsigned char* and
setColorProfileAndTransform needs it to be a char*.  I think we need a cast
somewhere.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
File third_party/libpng/BUILD.gn (left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:42: "png.h",
On 2016/06/03 20:27:44, Nico wrote:
> what happened to png.h?

I'll put it back.  Is it typical to list *.h files in the sources list?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:13: #"toolsets": ["target", "host"],
On 2016/06/03 20:27:44, Nico wrote:
> remove this if, it doesn't do anything

SGTM

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:30: # Upstream uses self-assignment to avoid
warnings.
On 2016/06/03 20:27:44, Nico wrote:
> do we still need -Wno-self-assign then?

Removing it.  The bots should tell us, I think.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:33:
"-Wno-tautological-constant-out-of-range-compare",
On 2016/06/03 20:27:44, Nico wrote:
> is there an upstream bug for this?

libpng checks that the width (uint32_t)  is not greater than PNG_SIZE_MAX.  On
platforms where size_t is 64-bits, this comparison will always be false.

I'm not sure this is necessarily a bug?  But we need to turn the warning off to
compile on Win clang.

I've added a comment.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
File third_party/libpng/README.chromium (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
third_party/libpng/README.chromium:11: - Use custom configuration file
pnglibconf.h, which turns off all features that
On 2016/06/03 20:27:45, Nico wrote:
> aha; list that in the gyp / gn files then

Done!

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
third_party/libpng/README.chromium:14: pdfium's copy of libpng.
On 2016/06/03 20:27:45, Nico wrote:
> i think pdfium is supposed to use chromium's libpng when it's part of a
chromium
> build and its own copy else. both copies should never compete with each other.

That seems to make sense.  I did try to compile without prefixing, and things
failed to link due to namespace conflicts.  So I decided to leave the prefixing
as is for now.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
File third_party/libpng/libpng.gyp (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:15: '-Wno-self-assign',
On 2016/06/03 20:27:45, Nico wrote:
> why does the gn file have that tautological-compare thing but this file
doesn't?

Possibly because we don't have a Windows/clang/gyp bot?  Do you think it makes
sense to have the flag here, even if it's unnecessary?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:47: # to compile the optimization files.  This
works fine on GN.
On 2016/06/03 20:27:45, Nico wrote:
> what's the error if you don't do this?

Seeing a lot of this on ios_device:
Undefined symbols for architecture armv7:
  "_wk_png_init_filter_functions_neon", referenced from:
      _wk_png_read_filter_row in libpng.a(libpng.pngrutil.o)
ld: symbol(s) not found for architecture armv7
clang: error: linker command failed with exit code 1 (use -v to see invocation)

That symbol is defined in the arm/* files.  Patch Set 6 makes a fairly blatant
attempt to get ios to compile these files, but no luck.  I also tried some
things with "target_subarch" which similarly didn't work.  I'm happy to file a
bug.  How close are we to only caring about GN (not GYP)?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:97: 'toolsets': ['target', 'host'],
On 2016/06/03 20:27:45, Nico wrote:
> why is this no longer needed for chromecast==1?

If you look closely at the base, these were only ever needed for Android.  The
difference is that we now enable PNG_READ_PACK_SUPPORT all the time.  IMO,
there's no reason to have different configs for different platforms.

[Nico, 2016-06-03 21:36:49]

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:239:
#if (PNG_LIBPNG_VER < 10500)
Hm, the other caller of setColorProfileAndTransform() also casts:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

If each caller wants to pass an unsigned char*, maybe we should change
setColorProfileAndTransform() to take that ... but looking a bit, that compares
the data to string literals, so those would then need a cast instead. Ok, but
please use a c++-style reinterpret_cast then.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
File third_party/libpng/BUILD.gn (left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:42: "png.h",
On 2016/06/03 21:26:01, msarett wrote:
> On 2016/06/03 20:27:44, Nico wrote:
> > what happened to png.h?
> 
> I'll put it back.  Is it typical to list *.h files in the sources list?

Yes, normally we list them. (Since nothing checks for that, I'm sure we're
missing some of them.)

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
File third_party/libpng/README.chromium (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
third_party/libpng/README.chromium:14: pdfium's copy of libpng.
On 2016/06/03 21:26:01, msarett wrote:
> On 2016/06/03 20:27:45, Nico wrote:
> > i think pdfium is supposed to use chromium's libpng when it's part of a
> chromium
> > build and its own copy else. both copies should never compete with each
other.
> 
> That seems to make sense.  I did try to compile without prefixing, and things
> failed to link due to namespace conflicts.  So I decided to leave the
prefixing
> as is for now.

What exactly fails? That sounds like something we should probably fix.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
File third_party/libpng/libpng.gyp (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:15: '-Wno-self-assign',
On 2016/06/03 21:26:01, msarett wrote:
> On 2016/06/03 20:27:45, Nico wrote:
> > why does the gn file have that tautological-compare thing but this file
> doesn't?
> 
> Possibly because we don't have a Windows/clang/gyp bot?  Do you think it makes
> sense to have the flag here, even if it's unnecessary?

Ah, maybe that's the reason. You can send a try job to win_clang_x64_rel to get
a 64-bit gyp clang/win try result (it's backed by just 3 slaves and doesn't use
goma, so it's slow)

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:47: # to compile the optimization files.  This
works fine on GN.
On 2016/06/03 21:26:01, msarett wrote:
> On 2016/06/03 20:27:45, Nico wrote:
> > what's the error if you don't do this?
> 
> Seeing a lot of this on ios_device:
> Undefined symbols for architecture armv7:
>   "_wk_png_init_filter_functions_neon", referenced from:
>       _wk_png_read_filter_row in libpng.a(libpng.pngrutil.o)
> ld: symbol(s) not found for architecture armv7
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)
> 
> That symbol is defined in the arm/* files.  Patch Set 6 makes a fairly blatant
> attempt to get ios to compile these files, but no luck.  I also tried some
> things with "target_subarch" which similarly didn't work.  I'm happy to file a
> bug.  How close are we to only caring about GN (not GYP)?

Hm, that sounds like maybe the prefix header isn't set on ios for some  reason
when building the arm files -- how is the prefix header supposed to be included
there?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:73: 'arm/filter_neon_intrinsics.c',
btw, don't you have to add the .S file here?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:97: 'toolsets': ['target', 'host'],
On 2016/06/03 21:26:01, msarett wrote:
> On 2016/06/03 20:27:45, Nico wrote:
> > why is this no longer needed for chromecast==1?
> 
> If you look closely at the base, these were only ever needed for Android.  The
> difference is that we now enable PNG_READ_PACK_SUPPORT all the time.  IMO,
> there's no reason to have different configs for different platforms.

Ah, thanks.

[RyanSturm, 2016-06-03 21:55:31]

lgtm with 1 nit.

https://codereview.chromium.org/2021403002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2021403002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:42:
#include "wtf/Threading.h"
You shouldn't need wtf/threading.h since you removed the
DEFINE_THREAD_SAFE_STATIC_LOCAL

[commit-bot: I haz the power, 2016-06-04 00:33:44]

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

[commit-bot: I haz the power, 2016-06-04 00:33:46]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-06-06 14:18:16]

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:239:
#if (PNG_LIBPNG_VER < 10500)
On 2016/06/03 21:36:49, Nico wrote:
> Hm, the other caller of setColorProfileAndTransform() also casts:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> 
> If each caller wants to pass an unsigned char*, maybe we should change
> setColorProfileAndTransform() to take that ... but looking a bit, that
compares
> the data to string literals, so those would then need a cast instead. Ok, but
> please use a c++-style reinterpret_cast then.

Done.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
File third_party/libpng/BUILD.gn (left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:42: "png.h",
On 2016/06/03 21:36:49, Nico wrote:
> On 2016/06/03 21:26:01, msarett wrote:
> > On 2016/06/03 20:27:44, Nico wrote:
> > > what happened to png.h?
> > 
> > I'll put it back.  Is it typical to list *.h files in the sources list?
> 
> Yes, normally we list them. (Since nothing checks for that, I'm sure we're
> missing some of them.)

Acknowledged.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/BUI...
third_party/libpng/BUILD.gn:30: # Upstream uses self-assignment to avoid
warnings.
On 2016/06/03 21:26:01, msarett wrote:
> On 2016/06/03 20:27:44, Nico wrote:
> > do we still need -Wno-self-assign then?
> 
> Removing it.  The bots should tell us, I think.

Looks like we are fine without this, thanks :).

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
File third_party/libpng/README.chromium (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/REA...
third_party/libpng/README.chromium:14: pdfium's copy of libpng.
On 2016/06/03 21:36:49, Nico wrote:
> On 2016/06/03 21:26:01, msarett wrote:
> > On 2016/06/03 20:27:45, Nico wrote:
> > > i think pdfium is supposed to use chromium's libpng when it's part of a
> > chromium
> > > build and its own copy else. both copies should never compete with each
> other.
> > 
> > That seems to make sense.  I did try to compile without prefixing, and
things
> > failed to link due to namespace conflicts.  So I decided to leave the
> prefixing
> > as is for now.
> 
> What exactly fails?

../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/third_party/libpng/libpng_sources/png.o: multiple definition of
'png_access_version_number'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/third_party/pdfium/third_party/fx_lpng/png.o: previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/third_party/libpng/libpng_sources/png.o: multiple definition of
'png_build_gamma_table'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/third_party/pdfium/third_party/fx_lpng/png.o: previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/third_party/libpng/libpng_sources/png.o: multiple definition of
'png_calculate_crc'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/third_party/pdfium/third_party/fx_lpng/png.o: previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/third_party/libpng/libpng_sources/png.o: multiple definition of
'png_check_IHDR'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/third_party/pdfium/third_party/fx_lpng/png.o: previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/third_party/libpng/libpng_sources/png.o: multiple definition of
'png_chunk_unknown_handling'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/third_party/pdfium/third_party/fx_lpng/png.o: previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/third_party/libpng/libpng_sources/png.o: multiple definition of
'png_colorspace_set_ICC'

etc...

> That sounds like something we should probably fix.

Agreed.  I'm guessing we would need to modify the pdfium build files to only
compile their own copy of libpng when not on Chrome?  Can I handle this in a
follow-up change?

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
File third_party/libpng/libpng.gyp (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:15: '-Wno-self-assign',
On 2016/06/03 21:36:49, Nico wrote:
> On 2016/06/03 21:26:01, msarett wrote:
> > On 2016/06/03 20:27:45, Nico wrote:
> > > why does the gn file have that tautological-compare thing but this file
> > doesn't?
> > 
> > Possibly because we don't have a Windows/clang/gyp bot?  Do you think it
makes
> > sense to have the flag here, even if it's unnecessary?
> 
> Ah, maybe that's the reason. You can send a try job to win_clang_x64_rel to
get
> a 64-bit gyp clang/win try result (it's backed by just 3 slaves and doesn't
use
> goma, so it's slow)

Sent a tryjob, will follow up on this.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:47: # to compile the optimization files.  This
works fine on GN.
On 2016/06/03 21:36:49, Nico wrote:
> On 2016/06/03 21:26:01, msarett wrote:
> > On 2016/06/03 20:27:45, Nico wrote:
> > > what's the error if you don't do this?
> > 
> > Seeing a lot of this on ios_device:
> > Undefined symbols for architecture armv7:
> >   "_wk_png_init_filter_functions_neon", referenced from:
> >       _wk_png_read_filter_row in libpng.a(libpng.pngrutil.o)
> > ld: symbol(s) not found for architecture armv7
> > clang: error: linker command failed with exit code 1 (use -v to see
> invocation)
> > 
> > That symbol is defined in the arm/* files.  Patch Set 6 makes a fairly
blatant
> > attempt to get ios to compile these files, but no luck.  I also tried some
> > things with "target_subarch" which similarly didn't work.  I'm happy to file
a
> > bug.  How close are we to only caring about GN (not GYP)?
> 
> Hm, that sounds like maybe the prefix header isn't set on ios for some  reason
> when building the arm files -- how is the prefix header supposed to be
included
> there?

What makes you think it's the prefix header?  FWIW, pngprefix.h is included from
pngpriv.h and pnglibconf.h and is not platform specific.

Additionally, looking at the build output, it appears that are arm/arm_init.c
and arm/filter_neon_intrinsics.c are not compiled with the rest of the png
sources.

[1/659] CC obj/third_party/libpng/libpng.pngrio.o
[2/659] CC obj/third_party/libpng/libpng.pngmem.o
[3/659] CC obj/third_party/libpng/libpng.pngget.o
[4/659] CC obj/third_party/libpng/libpng.pngwio.o
[5/659] CC obj/third_party/libpng/libpng.pngread.o
[6/659] CC obj/third_party/libpng/libpng.pngerror.o
[7/659] CC obj/third_party/libpng/libpng.pngtrans.o
[8/659] CC obj/third_party/libpng/libpng.pngwtran.o
[9/659] CC obj/third_party/libpng/libpng.pngset.o
[10/659] CC obj/third_party/libpng/libpng.pngpread.o
[11/659] CC obj/third_party/libpng/libpng.pngwrite.o
[12/659] CC obj/third_party/libpng/libpng.pngwutil.o
[13/659] CC obj/third_party/libpng/libpng.png.o
[14/659] CXX obj/third_party/skia/src/fonts/skia_library.SkRemotableFontMgr.o
[15/659] CXX obj/third_party/skia/src/opts/skia_opts_none.SkBlitRow_opts_none.o
[16/659] CXX obj/third_party/skia/src/opts/skia_opts_none.SkBlitMask_opts_none.o
[17/659] CC obj/third_party/libpng/libpng.pngrtran.o
[18/659] CC obj/third_party/libpng/libpng.pngrutil.o
[19/659] CXX obj/third_party/skia/src/ports/skia_library.SkTLS_pthread.o
[20/659] CXX
obj/third_party/skia/src/opts/skia_opts_none.SkBitmapProcState_opts_none.o
....

They are compiled on GN, which works fine.
[132/445] CC obj/third_party/libpng/libpng_sources/filter_neon_intrinsics.o
[135/445] CC obj/third_party/libpng/libpng_sources/arm_init.o

....

I originally thought it was likely a GYP bug, but I am further confused by the
compile output for ios on the recent (working) patch sets.

[9/958] CC obj/third_party/libpng/contrib/intel/libpng.intel_init.o
[10/958] CC obj/third_party/libpng/contrib/intel/libpng.filter_sse2_intrinsics.o

It's actually compiling the x86/64 optimizations (even though IOS is ARM?). 
Maybe this is because the host device is x86/64?  Still I don't get it.  But I
was happy enough with the green build.

---------------------------------

Any thoughts on this?  At the least, I think I should file a bug.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:73: 'arm/filter_neon_intrinsics.c',
On 2016/06/03 21:36:49, Nico wrote:
> btw, don't you have to add the .S file here?

filter_neon_intrinsics.c and filter_neon.S implement the same functions (one
with intrinsics, one with assembly).  The performance is virtually identical.

The intrinsics work on any Arm platform with Neon, while the assembly only works
on some Arm32 devices.  Additionally the maintainers have said that the assembly
is deprecated - though it doesn't seem to be formally deprecated.

I think it makes sense for us to always to compile and use the intrinsics when
on Arm, which is controlled by PNG_ARM_NEON_IMPLEMENTATION=1.

https://codereview.chromium.org/2021403002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2021403002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:42:
#include "wtf/Threading.h"
On 2016/06/03 21:55:31, RyanSturm wrote:
> You shouldn't need wtf/threading.h since you removed the
> DEFINE_THREAD_SAFE_STATIC_LOCAL

Thanks! Done.

[scroggo_chromium, 2016-06-06 14:51:40]

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(left):

https://codereview.chromium.org/2021403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:187:
dimensionsLocationHistogram.count(m_reader->getReadOffset() -
png->current_buffer_size - 1);
On 2016/06/03 21:26:01, msarett wrote:
> On 2016/06/03 20:31:51, RyanSturm wrote:
> > On 2016/06/03 20:18:51, msarett wrote:
> > > @ryansturm
> > > 
> > > libpng 1.6 has a stricter API that does not allow direct access to the
> > > png_structp.  I'm not aware that there is a way to get this information
> > anymore.
> > >  What is the impact of this metric overestimating a little bit?
> > 
> > You can remove the Blink.DecodedImage.EffectiveDimensionsLocation.PNG
> histogram.
> > We have enough data from canary/dev, and don't expect much to change.
> 
> Got it thanks!  It's been removed.

I suppose it's safe to remove the rest of them, too? (In a separate change.)

https://codereview.chromium.org/2021403002/diff/220001/third_party/libpng/REA...
File third_party/libpng/README.chromium (right):

https://codereview.chromium.org/2021403002/diff/220001/third_party/libpng/REA...
third_party/libpng/README.chromium:8: Description:
Did you also apply the intel patch?

https://codereview.chromium.org/2021403002/diff/220001/third_party/libpng/REA...
third_party/libpng/README.chromium:11: - Use custom configuration file
pnglibconf.h, which turns off all features that
Is there an interesting section of pnglibconf.h (which is a large file)? The
version used by Skia is mostly a copy/rename of pnglibconf.h.prebuilt.
(Actually, now that I look at it again, we no longer turn on/off any features,
so it's just a rename, plus it's out of date (specifies the wrong version...).)

It would be helpful if there was a section that is just the features we turned
off at the end.

[msarett, 2016-06-06 18:01:57]

https://codereview.chromium.org/2021403002/diff/220001/third_party/libpng/REA...
File third_party/libpng/README.chromium (right):

https://codereview.chromium.org/2021403002/diff/220001/third_party/libpng/REA...
third_party/libpng/README.chromium:8: Description:
On 2016/06/06 14:51:40, scroggo_chromium wrote:
> Did you also apply the intel patch?

Ahh yes, added bullet here.

https://codereview.chromium.org/2021403002/diff/220001/third_party/libpng/REA...
third_party/libpng/README.chromium:11: - Use custom configuration file
pnglibconf.h, which turns off all features that
On 2016/06/06 14:51:40, scroggo_chromium wrote:
> Is there an interesting section of pnglibconf.h (which is a large file)? The
> version used by Skia is mostly a copy/rename of pnglibconf.h.prebuilt.
> (Actually, now that I look at it again, we no longer turn on/off any features,
> so it's just a rename, plus it's out of date (specifies the wrong
version...).)
> 
> It would be helpful if there was a section that is just the features we turned
> off at the end.

Yes, agreed!  I've rearranged the file to group together "default settings" and
"chromium settings".

[scroggo_chromium, 2016-06-06 18:15:28]

lgtm

[msarett, 2016-06-07 19:59:15]

Description was changed from

==========
Update libpng to 1.6.22

BUG=599917
==========

to

==========
Update libpng to 1.6.22

BUG=599917
BUG=618061
==========

[msarett, 2016-06-07 20:06:44]

Please take a look.

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
File third_party/libpng/libpng.gyp (right):

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:15: '-Wno-self-assign',
On 2016/06/06 14:18:15, msarett wrote:
> On 2016/06/03 21:36:49, Nico wrote:
> > On 2016/06/03 21:26:01, msarett wrote:
> > > On 2016/06/03 20:27:45, Nico wrote:
> > > > why does the gn file have that tautological-compare thing but this file
> > > doesn't?
> > > 
> > > Possibly because we don't have a Windows/clang/gyp bot?  Do you think it
> makes
> > > sense to have the flag here, even if it's unnecessary?
> > 
> > Ah, maybe that's the reason. You can send a try job to win_clang_x64_rel to
> get
> > a 64-bit gyp clang/win try result (it's backed by just 3 slaves and doesn't
> use
> > goma, so it's slow)
> 
> Sent a tryjob, will follow up on this.

The flag is not needed on win_clang_x64_rel.  Maybe win-clang-gn just has a
particular version of clang?

Anyway, I think this is fine as is (without the no-tautological-compare).

https://codereview.chromium.org/2021403002/diff/200001/third_party/libpng/lib...
third_party/libpng/libpng.gyp:47: # to compile the optimization files.  This
works fine on GN.
On 2016/06/06 14:18:15, msarett wrote:
> On 2016/06/03 21:36:49, Nico wrote:
> > On 2016/06/03 21:26:01, msarett wrote:
> > > On 2016/06/03 20:27:45, Nico wrote:
> > > > what's the error if you don't do this?
> > > 
> > > Seeing a lot of this on ios_device:
> > > Undefined symbols for architecture armv7:
> > >   "_wk_png_init_filter_functions_neon", referenced from:
> > >       _wk_png_read_filter_row in libpng.a(libpng.pngrutil.o)
> > > ld: symbol(s) not found for architecture armv7
> > > clang: error: linker command failed with exit code 1 (use -v to see
> > invocation)
> > > 
> > > That symbol is defined in the arm/* files.  Patch Set 6 makes a fairly
> blatant
> > > attempt to get ios to compile these files, but no luck.  I also tried some
> > > things with "target_subarch" which similarly didn't work.  I'm happy to
file
> a
> > > bug.  How close are we to only caring about GN (not GYP)?
> > 
> > Hm, that sounds like maybe the prefix header isn't set on ios for some 
reason
> > when building the arm files -- how is the prefix header supposed to be
> included
> > there?
> 
> What makes you think it's the prefix header?  FWIW, pngprefix.h is included
from
> pngpriv.h and pnglibconf.h and is not platform specific.
> 
> Additionally, looking at the build output, it appears that are arm/arm_init.c
> and arm/filter_neon_intrinsics.c are not compiled with the rest of the png
> sources.
> 
> [1/659] CC obj/third_party/libpng/libpng.pngrio.o
> [2/659] CC obj/third_party/libpng/libpng.pngmem.o
> [3/659] CC obj/third_party/libpng/libpng.pngget.o
> [4/659] CC obj/third_party/libpng/libpng.pngwio.o
> [5/659] CC obj/third_party/libpng/libpng.pngread.o
> [6/659] CC obj/third_party/libpng/libpng.pngerror.o
> [7/659] CC obj/third_party/libpng/libpng.pngtrans.o
> [8/659] CC obj/third_party/libpng/libpng.pngwtran.o
> [9/659] CC obj/third_party/libpng/libpng.pngset.o
> [10/659] CC obj/third_party/libpng/libpng.pngpread.o
> [11/659] CC obj/third_party/libpng/libpng.pngwrite.o
> [12/659] CC obj/third_party/libpng/libpng.pngwutil.o
> [13/659] CC obj/third_party/libpng/libpng.png.o
> [14/659] CXX obj/third_party/skia/src/fonts/skia_library.SkRemotableFontMgr.o
> [15/659] CXX
obj/third_party/skia/src/opts/skia_opts_none.SkBlitRow_opts_none.o
> [16/659] CXX
obj/third_party/skia/src/opts/skia_opts_none.SkBlitMask_opts_none.o
> [17/659] CC obj/third_party/libpng/libpng.pngrtran.o
> [18/659] CC obj/third_party/libpng/libpng.pngrutil.o
> [19/659] CXX obj/third_party/skia/src/ports/skia_library.SkTLS_pthread.o
> [20/659] CXX
> obj/third_party/skia/src/opts/skia_opts_none.SkBitmapProcState_opts_none.o
> ....
> 
> They are compiled on GN, which works fine.
> [132/445] CC obj/third_party/libpng/libpng_sources/filter_neon_intrinsics.o
> [135/445] CC obj/third_party/libpng/libpng_sources/arm_init.o
> 
> ....
> 
> I originally thought it was likely a GYP bug, but I am further confused by the
> compile output for ios on the recent (working) patch sets.
> 
> [9/958] CC obj/third_party/libpng/contrib/intel/libpng.intel_init.o
> [10/958] CC
obj/third_party/libpng/contrib/intel/libpng.filter_sse2_intrinsics.o
> 
> It's actually compiling the x86/64 optimizations (even though IOS is ARM?). 
> Maybe this is because the host device is x86/64?  Still I don't get it.  But I
> was happy enough with the green build.
> 
> ---------------------------------
> 
> Any thoughts on this?  At the least, I think I should file a bug.

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=618061

[msarett, 2016-06-07 20:46:04]

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

[commit-bot: I haz the power, 2016-06-07 21:02:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021403002/230001

[commit-bot: I haz the power, 2016-06-07 22:56:51]

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

[commit-bot: I haz the power, 2016-06-07 22:56:53]

Dry run: This issue passed the CQ dry run.

[Nico, 2016-06-13 14:11:57]

lgtm

[msarett, 2016-06-13 14:17:42]

The CQ bit was checked by msarett@google.com

[msarett, 2016-06-13 14:17:43]

The patchset sent to the CQ was uploaded after l-g-t-m from kcc@chromium.org,
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2021403002/#ps230001
(title: " Rearrange pnglibconf.h")

[commit-bot: I haz the power, 2016-06-13 14:18:09]

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

[commit-bot: I haz the power, 2016-06-13 16:18:06]

Description was changed from

==========
Update libpng to 1.6.22

BUG=599917
BUG=618061
==========

to

==========
Update libpng to 1.6.22

BUG=599917
BUG=618061
==========

[commit-bot: I haz the power, 2016-06-13 16:18:08]

Committed patchset #14 (id:230001)

[commit-bot: I haz the power, 2016-06-13 16:19:55]

Description was changed from

==========
Update libpng to 1.6.22

BUG=599917
BUG=618061
==========

to

==========
Update libpng to 1.6.22

BUG=599917
BUG=618061

Committed: https://crrev.com/8c0ac8d882561cca77d5184248708b0ce9c851d4
Cr-Commit-Position: refs/heads/master@{#399464}
==========

[commit-bot: I haz the power, 2016-06-13 16:19:58]

Patchset 14 (id:??) landed as
https://crrev.com/8c0ac8d882561cca77d5184248708b0ce9c851d4
Cr-Commit-Position: refs/heads/master@{#399464}

[Nico, 2016-06-14 13:44:45]

This added a warning to 64-bit gyp builds with clang on Windows:
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%28dll%29/buil...

FAILED: obj/third_party/libpng/libpng.png.obj 
ninja -t msvc -e environment.x64 -- E:\b\build\slave\cache\cipd\goma/gomacc
"..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo
/showIncludes /FC @obj\third_party\libpng\libpng.png.obj.rsp /c
..\..\third_party\libpng\png.c /Foobj\third_party\libpng\libpng.png.obj
/Fdobj\third_party\libpng\libpng.c.pdb 
..\..\third_party\libpng\png.c(2500,8):  error: comparison of constant
2305843009213693944 with expression of type 'unsigned int' is always false
[-Werror,-Wtautological-constant-out-of-range-compare]
   if (png_gt(((width + 7) & (~7)),
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
..\..\third_party\libpng\png.c(2476,29):  note: expanded from macro 'png_gt'
#   define png_gt(a,b) ((a) > (b))

Just suppressing the warning is fine I suppose. Can you take a look?

[Nico, 2016-06-14 13:45:25]

(it's probably just readdding the -Wno-tautological-foo flag I suggested
removing further up :-P)

[msarett, 2016-06-14 13:54:02]

On 2016/06/14 13:45:25, Nico (traveling...slow) wrote:
> (it's probably just readdding the -Wno-tautological-foo flag I suggested
> removing further up :-P)

Ahh yes, we added this flag to the GN build.  You suggested it on GYP as well,
but I didn't add it because I couldn't get the trybots to reproduce the error.

Looks like that was the wrong decision, I'll add it to the GYP build as well.