libwebp: update snapshot to v0.3.0-rc6

libwebp: update snapshot to v0.3.0-rc6

adds metadata (XMP/EXIF), ICC profile and animation support to the format.

BUG=https://bugs.webkit.org/show_bug.cgi?id=112761
TEST=build,webkit layout
TBR=cpu

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190302

[jzern, 2013-03-21 06:56:08]

Note this change requires at least r189219 (WebKit r146288) for toDataURL-webp
to pass (fragile encode test), which is the cause of the earlier lkgr based
layout failures.

Otherwise I don't see this introducing any new warnings to the other builds.

[skal, 2013-03-21 08:35:40]

LGTM

patch locally tested OK on x86_64. I'll let the bots do the rest...

[jzern, 2013-03-21 18:30:07]

On 2013/03/21 06:56:08, jzern wrote:
> Note this change requires at least r189219 (WebKit r146288) for toDataURL-webp
> to pass (fragile encode test), which is the cause of the earlier lkgr based
> layout failures.
> 
> Otherwise I don't see this introducing any new warnings to the other builds.

Original webkit-bug for the layouts: 
  https://bugs.webkit.org/show_bug.cgi?id=93310

[jzern, 2013-03-21 18:31:56]

On 2013/03/21 06:56:08, jzern wrote:
> Note this change requires at least r189219 (WebKit r146288) for toDataURL-webp
> to pass (fragile encode test), which is the cause of the earlier lkgr based
> layout failures.
> 
> Otherwise I don't see this introducing any new warnings to the other builds.

Regressions: Unexpected image-only failures (1)
  platform/chromium/virtual/gpu/fast/canvas/canvas-toDataURL-webp.html [
ImageOnlyFailure ]

This is from -rHEAD in linux_layout; I assume this has other TestExpectations,
but I don't know where those live.

[fbarchard, 2013-03-21 20:32:55]

try bots show layout failures?

[jzern, 2013-03-21 20:36:06]

On 2013/03/21 18:31:56, jzern wrote:
> On 2013/03/21 06:56:08, jzern wrote:
> > Note this change requires at least r189219 (WebKit r146288) for
toDataURL-webp
> > to pass (fragile encode test), which is the cause of the earlier lkgr based
> > layout failures.
> > 
> > Otherwise I don't see this introducing any new warnings to the other builds.
> 
> Regressions: Unexpected image-only failures (1)
>   platform/chromium/virtual/gpu/fast/canvas/canvas-toDataURL-webp.html [
> ImageOnlyFailure ]
> 
> This is from -rHEAD in linux_layout; I assume this has other TestExpectations,
> but I don't know where those live.

OK I can reproduce this with the run_webkit_tests.py / layout_test_wrapper.py,
it's just a different platform target... The expectations  are in the same file,
but platform/chromium/virtual/gpu/fast/canvas/canvas-toDataURL-webp.html is
needed. Seems I'll need another webkit change for 93310.

I can add this to webkit/tools/layout_tests/test_expectations.txt while that's
in progress to verify with the current bots.

[jzern, 2013-03-22 03:05:32]

On 2013/03/21 20:36:06, jzern wrote:
> I can add this to webkit/tools/layout_tests/test_expectations.txt while that's
> in progress to verify with the current bots.

This is failing in the expected way @r189418 with my local edits, so should be
good after the new WebKit change lands.
Any other build bots to throw at it?

[jzern, 2013-03-22 07:38:04]

On 2013/03/22 03:05:32, jzern wrote:
> On 2013/03/21 20:36:06, jzern wrote:
> > I can add this to webkit/tools/layout_tests/test_expectations.txt while
that's
> > in progress to verify with the current bots.
> 
> This is failing in the expected way @r189418 with my local edits, so should be
> good after the new WebKit change lands.
> Any other build bots to throw at it?

The latest expectations change landed @r146566.

[fbarchard, 2013-03-22 18:56:44]

This is a very large CL... prefer more frequent, small CLs.
Some feedback on first few decoder files.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/f...
File third_party/libwebp/dec/frame.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/f...
third_party/libwebp/dec/frame.c:682: memcpy(u_out + j * dec->cache_uv_stride_,
u_dst + j * BPS, 8);
memcpy may not be best practice, performance wise.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
File third_party/libwebp/dec/idec.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/idec.c:628: if (!is_external_memory) {    // Overwrite
parameters to sane values.
2 spaces before //

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/idec.c:632: colorspace = MODE_YUVA;
planar YUV with alpha?
i may want to support this via fourcc in libyuv, if you have a standard, for
high speed conversion to argb.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/io.c
File third_party/libwebp/dec/io.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/io.c:197: --*num_rows;
this may be a tad slower than you hope.  If performance/size matters, consider
inlining this function.
Or copy the old value into an int at the top, increment the int (register) and
store it back only once.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/io.c:224: uint8_t* dst = base_rgba + (alpha_first ? 0 :
3);
this is a little scarey, but works
argb, abgr, bgra and rgba have a at 0 or 3.
Those are the 4 formats supported by skia layouts.  Maybe worth a comment.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/io.c:230: const uint32_t alpha_value = alpha[i];
this code would lend itself to simd.
but as C, it'll be fairly slow.  If you want a little speed, unroll it to 2 at a
time.

for (i = 0; i < mb_w - 1; i += 2) {
do 2 of each step using offsets.
const uint32_t alpha_value = alpha[i];
dst[4 * i] = alpha_value;
alpha_mask &= alpha_value;
alpha_value = alpha[i + 1];
dst[4 * i + 4] = alpha_value;
alpha_mask &= alpha_value;

The reason its slow is lsunit is doing bytes.  Using fewer loads/stores helps,
even with overhead of masking.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/io.c:260: for (i = 0; i < mb_w; ++i) {
ditto.  unrolling to 2 at a time will help

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/v...
File third_party/libwebp/dec/vp8i.h (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/v...
third_party/libwebp/dec/vp8i.h:162: unsigned int skip_:1;      // block type
consider padding explicitely to 32 bits.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/v...
File third_party/libwebp/dec/vp8l.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/v...
third_party/libwebp/dec/vp8l.c:946: uint32_t* data = NULL;
shouldnt we be using uint32* etc?

[fbarchard, 2013-03-22 19:08:27]

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/demux...
File third_party/libwebp/demux/demux.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/demux...
third_party/libwebp/demux/demux.c:42: size_t size_;
size_t is 64 bit on -m64.  Are you sure this is what you intended?  The RIFF
format is 32 bit size fields.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
File third_party/libwebp/dsp/dec.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec.c:432: const uint64_t v = (uint64_t)value *
0x0101010101010101ULL;
ULL is not guaranteed to be 64 bit.  May want to use UINT64_C()

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
File third_party/libwebp/dsp/dec_neon.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:15: #if defined(__cplusplus) ||
defined(c_plusplus)
pick one?

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:344: "vadd.s32        q0, q0, q4             
    \n" // dc = tmp[0] + 3
vadd can do immediate 3, no?
If you must waste a register, initialize it further up to avoid interlock. 
avoid q4.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:351: "vshrn.s32       d0, q0, #3             
    \n" // (a0 + a1) >> 3
consider rounding.. its free.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:360: "vst1.16         d0[0], [%[out]],
%[kStep]   \n"
Can this be done with 1 vst?  out continuous, vdup value.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:380: "q5", "q6", "q7", "q8", "q9"  //
clobbered
avoid q4-q7.  use q0-q3, q8-q15.

[jzern, 2013-03-22 19:10:36]

Good points all, I've collected them, but these kind of changes belong upstream
not in a tested release archive.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
File third_party/libwebp/dec/idec.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/idec.c:628: if (!is_external_memory) {    // Overwrite
parameters to sane values.
On 2013/03/22 18:56:44, fbarchard wrote:
> 2 spaces before //

minimum. horizontal whitespace is allowed if it adds readability. I'll admit
that some of this may have gotten ragged in it's own right, but this is a
snapshot update of a stable release. cosmetics can go upstream...

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/i...
third_party/libwebp/dec/idec.c:632: colorspace = MODE_YUVA;
On 2013/03/22 18:56:44, fbarchard wrote:
> planar YUV with alpha?
> i may want to support this via fourcc in libyuv, if you have a standard, for
> high speed conversion to argb.

Just a full plane (WxH) of alpha [0, 255] here

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/v...
File third_party/libwebp/dec/vp8l.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dec/v...
third_party/libwebp/dec/vp8l.c:946: uint32_t* data = NULL;
On 2013/03/22 18:56:44, fbarchard wrote:
> shouldnt we be using uint32* etc?

no. we use posix types in this (C) project

[jzern, 2013-03-22 19:18:24]

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/demux...
File third_party/libwebp/demux/demux.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/demux...
third_party/libwebp/demux/demux.c:42: size_t size_;
On 2013/03/22 19:08:27, fbarchard wrote:
> size_t is 64 bit on -m64.  Are you sure this is what you intended?  The RIFF
> format is 32 bit size fields.

Yes it's just to avoid transitioning back and forth and the cast fest that
ensues. This code is 32-bit protected given the restrictions imposed by the RIFF
container

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
File third_party/libwebp/dsp/dec_neon.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:15: #if defined(__cplusplus) ||
defined(c_plusplus)
On 2013/03/22 19:08:27, fbarchard wrote:
> pick one?

skal was the source of that way back. actually I don't know which compiler uses
the latter...

[Johann, 2013-03-22 19:19:15]

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
File third_party/libwebp/dsp/dec_neon.c (right):

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:344: "vadd.s32        q0, q0, q4             
    \n" // dc = tmp[0] + 3
On 2013/03/22 19:08:27, fbarchard wrote:
> vadd can do immediate 3, no?

Not that I'm aware of:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0204j/CIHJCAAG.html

> If you must waste a register, initialize it further up to avoid interlock. 
> avoid q4.

Yes it could be moved up some.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:351: "vshrn.s32       d0, q0, #3             
    \n" // (a0 + a1) >> 3
On 2013/03/22 19:08:27, fbarchard wrote:
> consider rounding.. its free.

It wouldn't match the intended output.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:360: "vst1.16         d0[0], [%[out]],
%[kStep]   \n"
On 2013/03/22 19:08:27, fbarchard wrote:
> Can this be done with 1 vst?  out continuous, vdup value.

I'm not aware of a vst which takes kStep.

https://codereview.chromium.org/12942006/diff/13001/third_party/libwebp/dsp/d...
third_party/libwebp/dsp/dec_neon.c:380: "q5", "q6", "q7", "q8", "q9"  //
clobbered
On 2013/03/22 19:08:27, fbarchard wrote:
> avoid q4-q7.  use q0-q3, q8-q15.

Yes that should have been done.

[jzern, 2013-03-22 19:29:48]

On 2013/03/22 18:56:44, fbarchard wrote:
> This is a very large CL... prefer more frequent, small CLs.
> Some feedback on first few decoder files.
> 
I prefer working with stable and well tested release in this kind of context.
Chrome isn't our test ground.

[fbarchard, 2013-03-22 20:57:29]

LGTM
Please ensure layout and builds pass and be ready to rollback.

[jzern, 2013-03-22 21:10:35]

On 2013/03/22 20:57:29, fbarchard wrote:
> LGTM
> Please ensure layout and builds pass and be ready to rollback.

rebase @r189900;

layouts pass with the rebase.

http/tests/images/\*webp\* fast/canvas/\*webp\*
platform/chromium/virtual/gpu/fast/canvas/canvas-toDataURL-webp.html

[commit-bot: I haz the power, 2013-03-22 21:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jzern@chromium.org/12942006/25001

[commit-bot: I haz the power, 2013-03-22 21:32:23]

Presubmit check for 12942006-25001 failed and returned exit status 1.

INFO:root:Found 63 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/third_party/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/webkit/tools/layout_tests/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    third_party/libwebp/utils/utils.c
    third_party/libwebp/dec/webp.c
    third_party/libwebp/utils/huffman_encode.c
    third_party/libwebp/dsp/enc.c
    third_party/libwebp/enc/picture.c
    third_party/libwebp/enc/cost.h
    third_party/libwebp/dsp/upsampling.c
    third_party/libwebp/utils/utils.h
    third_party/libwebp/enc/cost.c
    third_party/libwebp/webp/decode.h
    third_party/libwebp/utils/quant_levels_dec.c
    third_party/libwebp/dec/vp8l.c
    third_party/libwebp/utils/thread.c
    third_party/libwebp/webp/encode.h
    third_party/libwebp/README.chromium
    third_party/libwebp/utils/thread.h
    third_party/libwebp/enc/quant.c
    third_party/libwebp/dsp/lossless.h
    third_party/libwebp/utils/quant_levels_dec.h
    third_party/libwebp/libwebp.gyp
    third_party/libwebp/dec/idec.c
    third_party/libwebp/dsp/upsampling_sse2.c
    third_party/libwebp/dsp/lossless.c
    third_party/libwebp/dec/frame.c
    third_party/libwebp/enc/backward_references.h
    third_party/libwebp/dsp/dec_sse2.c
    third_party/libwebp/enc/backward_references.c
    third_party/libwebp/utils/bit_reader.h
    third_party/libwebp/enc/webpenc.c
    third_party/libwebp/dsp/dsp.h
    third_party/libwebp/utils/bit_reader.c
    third_party/libwebp/utils/filters.c
    third_party/libwebp/utils/quant_levels.c
    third_party/libwebp/dsp/dec_neon.c
    third_party/libwebp/enc/frame.c
    third_party/libwebp/webp/format_constants.h
    third_party/libwebp/dsp/yuv.c
    third_party/libwebp/enc/syntax.c
    third_party/libwebp/utils/quant_levels.h
    third_party/libwebp/utils/filters.h
    third_party/libwebp/dsp/dec.c
    third_party/libwebp/enc/tree.c
    third_party/libwebp/enc/vp8enci.h
    third_party/libwebp/dsp/upsampling_neon.c
    third_party/libwebp/webp/mux_types.h
    third_party/libwebp/dsp/enc_neon.c
    third_party/libwebp/dec/vp8.c
    third_party/libwebp/enc/alpha.c
    third_party/libwebp/webp/demux.h
    third_party/libwebp/enc/histogram.c
    third_party/libwebp/enc/analysis.c
    third_party/libwebp/dec/alpha.c
    third_party/libwebp/demux/demux.c
    third_party/libwebp/enc/token.c
    third_party/libwebp/enc/histogram.h
    third_party/libwebp/dec/io.c
    third_party/libwebp/enc/config.c
    third_party/libwebp/enc/vp8l.c
    third_party/libwebp/webp/mux.h
    third_party/libwebp/dec/vp8i.h
    third_party/libwebp/dsp/enc_sse2.c
    third_party/libwebp/dsp/yuv.h

Presubmit checks took 3.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[jzern, 2013-03-22 21:43:39]

> Presubmit check for 12942006-25001 failed and returned exit status 1.
> 
> INFO:root:Found 63 file(s).
> 
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/third_party/PRESUBMIT.py
> Running
/b/commit-queue/workdir/chromium/webkit/tools/layout_tests/PRESUBMIT.py
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     third_party/libwebp/utils/utils.c
> [...]

OK, brettw@, on to you...

[jzern, 2013-03-22 22:45:41]

Seems brettw@, might be away. cpu@ any chance I can land this?

[cpu_(ooo_6.6-7.5), 2013-03-22 22:55:38]

you don't need owners approval for updating code in third_party. Only new third
party libraries need owners approval. 

Simply tbr any of the owners.

[jzern, 2013-03-22 23:01:49]

On 2013/03/22 22:55:38, cpu wrote:
> you don't need owners approval for updating code in third_party. Only new
third
> party libraries need owners approval. 
> 
> Simply tbr any of the owners.

OK thanks, I just wanted to check first given the authority of the presubmit...

[commit-bot: I haz the power, 2013-03-22 23:25:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jzern@chromium.org/12942006/25001

[commit-bot: I haz the power, 2013-03-23 15:54:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jzern@chromium.org/12942006/25001

[brettw, 2013-03-24 05:33:58]

Is there somebody/people you can add to a new owners file for this for future
rolls? I think we're trying to get owners for third_party libs.

[commit-bot: I haz the power, 2013-03-24 09:29:26]

List of reviewers changed. brettw@chromium.org did a drive-by without LGTM'ing!

[commit-bot: I haz the power, 2013-03-24 20:03:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jzern@chromium.org/12942006/25001

[commit-bot: I haz the power, 2013-03-24 20:12:12]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-03-24 20:16:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jzern@chromium.org/12942006/25001

[commit-bot: I haz the power, 2013-03-24 21:38:40]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-03-24 21:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jzern@chromium.org/12942006/25001

[commit-bot: I haz the power, 2013-03-24 23:13:33]

Change committed as 190302