Use the upstream version of libwebp, v0.4.3.

Use the upstream version of libwebp, v0.4.3.

DEPS:
Update to pull v0.4.3 of libwebp from upstream

gyp/libwebp.gyp:
Add new files, as referenced by the gyp file used by Chromium.

resource/tests:
Add regression tests for particular images.

BUG=skia:3442
BUG=skia:3315
BUG=skia:3429
Committed: https://skia.googlesource.com/skia/+/3aa0fb4d80c76b559ff4b82d5e569993aea06da1

Committed: https://skia.googlesource.com/skia/+/139491fbaa6fc926456a246bb28e09848e0e48f5

[scroggo, 2015-06-18 21:00:43]

scroggo@google.com changed reviewers:
+ djsollen@google.com, emmaleer@google.com, jzern@chromium.org,
msarett@google.com, scroggo@google.com

[msarett, 2015-06-19 12:07:46]

https://codereview.chromium.org/1178013008/diff/10001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1178013008/diff/10001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:67: bool hasAlpha = SkToBool(formatFlags & ALPHA_FLAG)
? 1 : 0;
Can this be simpler?
bool hasAlpha = SkToBool(formatFlags & ALPHA_FLAG);

[djsollen, 2015-06-19 13:02:56]

So before diving into review I have some high level questions.

1) Does chrome not pull in their version from DEPS.  Do they actually copy all
the files into their tree?  If so, why do they need a custom copy?

2)  Won't this break SkImageDecoder on Android?  In the OS build we link against
the copy of libwebp that Android has not our own copy.  Or is that why your not
pulling it as a DEPS?

[scroggo, 2015-06-19 18:59:58]

On 2015/06/19 13:02:56, djsollen wrote:
> So before diving into review I have some high level questions.
> 
> 1) Does chrome not pull in their version from DEPS.  Do they actually copy all
> the files into their tree?

Yes they do. I copied this directly from their copy in the Chromium tree.

> If so, why do they need a custom copy?

See https://code.google.com/p/skia/issues/detail?id=3442#c18

The overview is, they're different:
- Chromium has stuff not in the canonical (e.g. gyp/gn files)
- Chromium excludes some stuff (e.g. examples etc)
- Chromium only includes the src/ directory, all at the top level instead of
inside src (not sure why this is necessary)
- Chromium has its own changes

third_party/libwebp/README.chromium (added as part of this CL) tracks the
differences.

> 
> 2)  Won't this break SkImageDecoder on Android? 

I don't think so, but I do need to test. We call different functions to
determine the width/height/alpha, but those do exist in the version used by the
framework build, so I think they'll work. (FWIW, we should probably update that
version as well.)

> In the OS build we link against
> the copy of libwebp that Android has not our own copy. 

If you look at gyp/libwebp.gyp, you'll notice that I did not modify the
dependencies for the framework build, so we'll still be using the version in
external/webp, not the one in external/skia/third_party/libwebp. However, we
will still be merging a bunch of extra files :(

> Or is that why your not
> pulling it as a DEPS?

No, it's because in order to get the Chromium version, there is no DEPS to pull
in. I *could* create another mirror, which *is* the Chromium version (or I'd ask
you to :P), and switch Chromium over to the mirror (although potentially if/when
we've moved blink::ImageDecoder into Skia, they'll get it through Skia, assuming
they have no other dependencies on it).

https://codereview.chromium.org/1178013008/diff/10001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1178013008/diff/10001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:67: bool hasAlpha = SkToBool(formatFlags & ALPHA_FLAG)
? 1 : 0;
On 2015/06/19 12:07:46, msarett wrote:
> Can this be simpler?
> bool hasAlpha = SkToBool(formatFlags & ALPHA_FLAG);

Lol yes. This was copied from SkImageDecoder_libwebp, where hasAlpha is an int
instead of a bool.

[jzern, 2015-06-19 19:04:13]

On 2015/06/19 18:59:58, scroggo wrote:
> On 2015/06/19 13:02:56, djsollen wrote:
> > So before diving into review I have some high level questions.
> > 
> > 1) Does chrome not pull in their version from DEPS.  Do they actually copy
all
> > the files into their tree?
> 
> Yes they do. I copied this directly from their copy in the Chromium tree.
> 
> > If so, why do they need a custom copy?
> 
> See https://code.google.com/p/skia/issues/detail?id=3442#c18
> 
> The overview is, they're different:
> [..]
> - Chromium only includes the src/ directory, all at the top level instead of
> inside src (not sure why this is necessary)

It isn't. This flattening was done on the initial import and has been carried
around since.
Android has a similar custom tree, I started some cleanup there on the last
import. If the trees were aligned then it might be possible to reuse the
Android.mk from upstream.

[djsollen, 2015-06-22 12:43:49]

I would prefer to put this into a git repo in Skia that we can keep things
consolidated and then if/when Chrome switches their image decoders over to using
Skia.

[scroggo, 2015-06-22 15:35:15]

On 2015/06/22 12:43:49, djsollen wrote:
> I would prefer to put this into a git repo in Skia that we can keep things
> consolidated and then if/when Chrome switches their image decoders over to
using
> Skia.

sgtm. Where can I find instructions to do that?

[djsollen, 2015-06-22 15:37:34]

I don't think there are instructions for how to add a new repo, but I think
borenet@, scroggo@, and myself have the git on borg accesses necessary to create
the repository.

[scroggo, 2015-06-22 20:21:13]

On 2015/06/22 15:37:34, djsollen wrote:
> I don't think there are instructions for how to add a new repo, but I think
> borenet@, scroggo@, and myself have the git on borg accesses necessary to
create
> the repository.

Drats. I have the permissions, and I created the repository
(https://skia.googlesource.com/third_party/libwebp/), but I have not been able
to push to it (I get the error message "fatal: remote error: Git access
forbidden"). "gob-ctl show" shows the following:

repository: <
  repo: "skia/third_party/libwebp"
  row_key: "99:skia/third_party/libwebp"
  head: "refs/heads/master"
  meta: <
    storage: PLAINTEXT
  >
  owner: <
    mdb_user: "scroggo"
    mdb_group: "chrome-git-admins"
    ldap_user: "scroggo"
    ldap_group: "skia-git-admins"
  >
  writer: <
  >
  reader: <
    all: USERS
  >
>

As an owner, I would think I would be able to do a git push, but no luck :-/

[scroggo, 2015-07-07 20:23:29]

Now I am just using upstream's version 0.4.3, which requires no changes to
SkImageDecoder/SkCodec.

PTAL

[jzern, 2015-07-08 06:27:14]

https://codereview.chromium.org/1178013008/diff/30001/gyp/libwebp.gyp
File gyp/libwebp.gyp (right):

https://codereview.chromium.org/1178013008/diff/30001/gyp/libwebp.gyp#newcode94
gyp/libwebp.gyp:94: '../third_party/externals/libwebp/src/dsp/enc_neon.c',
0.4.3 has a lossless_neon.c too

[djsollen, 2015-07-08 12:27:49]

https://chromium.googlesource.com/chromium/src/+/master/third_party/libwebp/R...

This states that a particular commit
(https://chromium.googlesource.com/webm/libwebp/+/f7fc4bc) was reverted.  Will
not reverting this patch cause us to have any problems in our testing?

[scroggo, 2015-07-08 16:06:43]

On 2015/07/08 12:27:49, djsollen wrote:
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/libwebp/R...
> 
> This states that a particular commit
> (https://chromium.googlesource.com/webm/libwebp/+/f7fc4bc) was reverted.  Will
> not reverting this patch cause us to have any problems in our testing?

IIUC, this relates to the following bug:
https://code.google.com/p/chromium/issues/detail?id=255934

That is a security bug, involving an invalid image. IIUC, the invalid image
reports different sizes depending on how you read it. The image was resulting in
an ASSERT in Chromium here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I do not think our testing will be affected by this. IIUC, the issue is if we
want someone to use this code, there may be a security risk (at least using
Chromium's WEBPImageDecoder).

https://codereview.chromium.org/1178013008/diff/30001/gyp/libwebp.gyp
File gyp/libwebp.gyp (right):

https://codereview.chromium.org/1178013008/diff/30001/gyp/libwebp.gyp#newcode94
gyp/libwebp.gyp:94: '../third_party/externals/libwebp/src/dsp/enc_neon.c',
On 2015/07/08 06:27:14, jzern wrote:
> 0.4.3 has a lossless_neon.c too

Added.

[djsollen, 2015-07-08 16:38:01]

lgtm

[scroggo, 2015-07-08 17:40:56]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-07-08 17:41:10]

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

[commit-bot: I haz the power, 2015-07-08 17:48:45]

Committed patchset #6 (id:70001) as
https://skia.googlesource.com/skia/+/3aa0fb4d80c76b559ff4b82d5e569993aea06da1

[scroggo, 2015-07-08 17:57:05]

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

The reason for reverting is: Breaking the build e.g.
http://build.chromium.org/p/client.skia/builders/Perf-Mac10.8-Clang-MacMini4....

../../../third_party/externals/libwebp/src/utils/./endian_inl.h:51:10: error:
use of unknown builtin '__builtin_bswap16' [-Wimplicit-function-declaration]
  return __builtin_bswap16(x);
         ^
1 error generated..

[scroggo, 2015-07-08 20:06:18]

On 2015/07/08 17:57:05, scroggo wrote:
> A revert of this CL (patchset #6 id:70001) has been created in
> https://codereview.chromium.org/1223583004/ by mailto:scroggo@google.com.
> 
> The reason for reverting is: Breaking the build e.g.
>
http://build.chromium.org/p/client.skia/builders/Perf-Mac10.8-Clang-MacMini4....
> 
> ../../../third_party/externals/libwebp/src/utils/./endian_inl.h:51:10: error:
> use of unknown builtin '__builtin_bswap16' [-Wimplicit-function-declaration]
>   return __builtin_bswap16(x);
>          ^
> 1 error generated..

Not sure what's going on here. In endian_inl.h, HAVE_BUILTIN_BSWAP16 gets
defined (meaning that we call __builtin_bswap16) if the clang version is at
least 3.3 (or if the version of gcc is at least 4.8, but we appear to be using
clang). I do not know how to tell what version of clang we have on that builder.

The warning ([-Wimplicit-function-declaration]) seems to imply that we should
have declared it?

We also have the following error:

from:
http://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Nex...

ERROR: dlopen failed: cannot locate symbol "VP8DspInitNEON" referenced by
"/data/local/tmp/libskia_android.so"...EXIT_CODE is 255

I have not been able to repro that error locally :-/

[scroggo, 2015-07-09 16:13:27]

On 2015/07/08 20:06:18, scroggo wrote:
>
http://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Nex...
> 
> ERROR: dlopen failed: cannot locate symbol "VP8DspInitNEON" referenced by
> "/data/local/tmp/libskia_android.so"...EXIT_CODE is 255
> 
> I have not been able to repro that error locally :-/

VP8DspInitNEON is defined in dec_neon.c. According to our gyp file, we build
dec_neon.c if arm_version >= 7. Looking in the full output from this build,
arm_version is set to 7 in a GYP_DEFINE, so it should have been built.

[scroggo, 2015-07-10 15:42:59]

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

[scroggo, 2015-07-10 15:43:00]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1178013008/#ps130001
(title: "Workaround for missing builtins")

[commit-bot: I haz the power, 2015-07-10 15:43:04]

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

[scroggo, 2015-07-10 15:51:02]

Derek, can you take another look? I had to do some gyp-fu to work around this.

[commit-bot: I haz the power, 2015-07-10 15:52:40]

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

[commit-bot: I haz the power, 2015-07-10 15:52:41]

Dry run: This issue passed the CQ dry run.

[scroggo, 2015-07-10 16:16:43]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-07-10 16:16:44]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1178013008/#ps170001
(title: "Remove README, put more info in config.h")

[commit-bot: I haz the power, 2015-07-10 16:16:51]

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

[commit-bot: I haz the power, 2015-07-10 16:21:06]

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

[commit-bot: I haz the power, 2015-07-10 16:21:07]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

(exceeded global retry quota)

[scroggo, 2015-07-10 16:26:04]

On 2015/07/10 16:21:07, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
> (JOB_FAILED,
>
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
> 
> (exceeded global retry quota)

The latest errors seem unrelated to my change. Trying again...

[scroggo, 2015-07-10 16:26:11]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-07-10 16:26:24]

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

[commit-bot: I haz the power, 2015-07-10 16:32:13]

Committed patchset #11 (id:170001) as
https://skia.googlesource.com/skia/+/139491fbaa6fc926456a246bb28e09848e0e48f5