libwebp: update to 0.5.0

libwebp: update to 0.5.0

BUG=571248
Committed: https://crrev.com/f03ecfc1e80a0e089b10aa06a769065d22c808cf
Cr-Commit-Position: refs/heads/master@{#367101}

[jzern, 2015-12-23 20:22:54]

Description was changed from

==========
libwebp: update to 0.5.0-rc1

BUG=571248
==========

to

==========
libwebp: update to 0.5.0

BUG=571248
==========

[jzern, 2015-12-24 15:29:05]

jzern@chromium.org changed reviewers:
+ urvang@chromium.org

[jzern, 2015-12-24 15:31:51]

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
File third_party/libwebp/libwebp.gyp (right):

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:89: 'OTHER_CFLAGS': [ '-DWEBP_HAVE_SSE2',
'-DWEBP_HAVE_SSE41' ]
I haven't checked if OTHER_CFLAGS is necessary for defines.

[Nico, 2015-12-24 15:38:48]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-12-24 15:38:49]

Thanks!

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/BUI...
File third_party/libwebp/BUILD.gn (right):

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:107: "-DWEBP_HAVE_SSE41",
gn has a defines thing too:
https://code.google.com/p/chromium/codesearch#search/&q=%5Cbdefines%20file:%5...

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:152: cflags = [ "-msse2" ]
sse2 is required by chromium and passed by default. You don't need a separate
target for sse2 (but having one doesn't hurt)

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
File third_party/libwebp/libwebp.gyp (right):

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:89: 'OTHER_CFLAGS': [ '-DWEBP_HAVE_SSE2',
'-DWEBP_HAVE_SSE41' ]
On 2015/12/24 15:31:51, jzern wrote:
> I haven't checked if OTHER_CFLAGS is necessary for defines.

The usual way to do defines is to have a 'defines' section (which will be
translated to the Right Thing on each OS). Examples:
https://code.google.com/p/chromium/codesearch#search/&q=%5C'defines%20file:gy...

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:102: 'target_name': 'libwebp_dsp_sse2',
same here

[Nico, 2015-12-24 15:39:21]

(These are all mostly just nits, so if you need to land this urgently and
addressing them is hard, then it's ok to not address them for this roll.)

[jzern, 2015-12-24 17:32:32]

On 2015/12/24 15:39:21, Nico wrote:
> (These are all mostly just nits, so if you need to land this urgently and
> addressing them is hard, then it's ok to not address them for this roll.)

It's easy enough to deal with them here and there's not a real hurry. I was just
trying to give this some bake time before branch (mostly for build issues as I
already demonstrated...). Thanks for the help

[jzern, 2015-12-24 17:39:22]

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/BUI...
File third_party/libwebp/BUILD.gn (right):

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:107: "-DWEBP_HAVE_SSE41",
On 2015/12/24 15:38:49, Nico wrote:
> gn has a defines thing too:
>
https://code.google.com/p/chromium/codesearch#search/&q=%5Cbdefines%20file:%5...

Thanks. I didn't look around after I hit on some yasm_defines using -D.

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:152: cflags = [ "-msse2" ]
On 2015/12/24 15:38:49, Nico wrote:
> sse2 is required by chromium and passed by default. You don't need a separate
> target for sse2 (but having one doesn't hurt)

true it was working before, I just thought I'd do this for consistency.

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
File third_party/libwebp/libwebp.gyp (right):

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:89: 'OTHER_CFLAGS': [ '-DWEBP_HAVE_SSE2',
'-DWEBP_HAVE_SSE41' ]
On 2015/12/24 15:38:49, Nico wrote:
> On 2015/12/24 15:31:51, jzern wrote:
> > I haven't checked if OTHER_CFLAGS is necessary for defines.
> 
> The usual way to do defines is to have a 'defines' section (which will be
> translated to the Right Thing on each OS). Examples:
> https://code.google.com/p/chromium/codesearch#search/&q=%5C

Done.

https://codereview.chromium.org/1546003002/diff/40001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:102: 'target_name': 'libwebp_dsp_sse2',
On 2015/12/24 15:38:49, Nico wrote:
> same here

Acknowledged.

[Nico, 2015-12-25 23:24:29]

lgtm

[urvang, 2015-12-28 19:50:36]

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
File third_party/libwebp/libwebp.gyp (right):

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:117: 'cflags': [ '-msse2', ],
Shouldn't this be using "defines"?

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:148: 'cflags': [ '-msse4.1', ],
And this too?

[jzern, 2015-12-29 00:16:17]

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
File third_party/libwebp/libwebp.gyp (right):

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:117: 'cflags': [ '-msse2', ],
On 2015/12/28 19:50:36, urvang wrote:
> Shouldn't this be using "defines"?

Do you mean add one for WEBP_HAVE_SSE2? It isn't strictly necessary, the final
check is done on WEBP_USE_SSE2 which is set if HAVE_SSE2 is present or __SSE2__.
If I add it then in the sse4 path there will be 2 instances for each condition.

[urvang, 2015-12-29 00:29:22]

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
File third_party/libwebp/libwebp.gyp (right):

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:117: 'cflags': [ '-msse2', ],
On 2015/12/29 00:16:17, jzern wrote:
> On 2015/12/28 19:50:36, urvang wrote:
> > Shouldn't this be using "defines"?
> 
> Do you mean add one for WEBP_HAVE_SSE2? It isn't strictly necessary, the final
> check is done on WEBP_USE_SSE2 which is set if HAVE_SSE2 is present or
__SSE2__.
> If I add it then in the sse4 path there will be 2 instances for each
condition.

I was actually looking at this diff:
https://codereview.chromium.org/1546003002/diff2/40001:50001/third_party/libw...

I see that at line#87, 'cflags' and 'xcode_settings' combo was replaced by
'defines' (based on Nico's suggestion earlier).

So my question was: shouldn't the same replacement happen in this line as well?

[jzern, 2015-12-29 17:33:39]

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
File third_party/libwebp/libwebp.gyp (right):

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:117: 'cflags': [ '-msse2', ],
On 2015/12/29 00:29:22, urvang wrote:
> On 2015/12/29 00:16:17, jzern wrote:
> > On 2015/12/28 19:50:36, urvang wrote:
> > > Shouldn't this be using "defines"?
> > 
> > Do you mean add one for WEBP_HAVE_SSE2? It isn't strictly necessary, the
final
> > check is done on WEBP_USE_SSE2 which is set if HAVE_SSE2 is present or
> __SSE2__.
> > If I add it then in the sse4 path there will be 2 instances for each
> condition.
> 
> I was actually looking at this diff:
>
https://codereview.chromium.org/1546003002/diff2/40001:50001/third_party/libw...
> 
> I see that at line#87, 'cflags' and 'xcode_settings' combo was replaced by
> 'defines' (based on Nico's suggestion earlier).
> 
> So my question was: shouldn't the same replacement happen in this line as
well?

we still want the machine flags, though they aren't strictly necessary for sse2;
I left them to be consistent with the sse4 section. for these modules we want
the instruction set enabled, the code will be built due to the presence of
__SSE2__ thanks to the -m.

https://codereview.chromium.org/1546003002/diff/50001/third_party/libwebp/lib...
third_party/libwebp/libwebp.gyp:148: 'cflags': [ '-msse4.1', ],
On 2015/12/28 19:50:36, urvang wrote:
> And this too?

this one was the source of the revert. we detected clang-cl as an msvc build and
tried to include the instrinsics, but clang (and gcc) need this to enable the
instructions (msvc itself only requires /arch for AVX/AVX2.

[urvang, 2015-12-29 17:45:16]

The CQ bit was checked by urvang@chromium.org

[urvang, 2015-12-29 17:45:18]

lgtm

[commit-bot: I haz the power, 2015-12-29 17:45:39]

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

[commit-bot: I haz the power, 2015-12-29 17:48:30]

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

[commit-bot: I haz the power, 2015-12-29 17:48:31]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[urvang, 2015-12-29 17:52:11]

On 2015/12/29 17:48:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
>   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

CQ seems to have failed.
I'm guessing a rebase is needed.

[jzern, 2015-12-29 19:00:17]

On 2015/12/29 17:52:11, urvang wrote:
> On 2015/12/29 17:48:31, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
> >   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
> >   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
> 
> CQ seems to have failed.
> I'm guessing a rebase is needed.

done

[urvang, 2015-12-29 19:03:39]

The CQ bit was checked by urvang@chromium.org

[urvang, 2015-12-29 19:03:40]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
urvang@chromium.org
Link to the patchset: https://codereview.chromium.org/1546003002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-29 19:03:57]

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

[commit-bot: I haz the power, 2015-12-29 20:39:57]

Description was changed from

==========
libwebp: update to 0.5.0

BUG=571248
==========

to

==========
libwebp: update to 0.5.0

BUG=571248
==========

[commit-bot: I haz the power, 2015-12-29 20:39:58]

Committed patchset #7 (id:60001)

[commit-bot: I haz the power, 2015-12-29 20:40:46]

Description was changed from

==========
libwebp: update to 0.5.0

BUG=571248
==========

to

==========
libwebp: update to 0.5.0

BUG=571248

Committed: https://crrev.com/f03ecfc1e80a0e089b10aa06a769065d22c808cf
Cr-Commit-Position: refs/heads/master@{#367101}
==========

[commit-bot: I haz the power, 2015-12-29 20:40:47]

Patchset 7 (id:??) landed as
https://crrev.com/f03ecfc1e80a0e089b10aa06a769065d22c808cf
Cr-Commit-Position: refs/heads/master@{#367101}