search

GaseousStates / webp

Automatically exported from code.google.com/p/webp
0 stars 0 forks source link

[lossless] (32-bit) corrupted encode output #200

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
v0.4.0-225-gdf08e67

What steps will reproduce the problem?
1. Encode 'good.jpg' using a 32-bit WIC-enabled binary or 'good.png' on other 
platforms.
> ./cwebp.exe -lossless good.jpg -o lossless.webp
$ ./cwebp -lossless good.png -o lossless.webp

What is the expected output? What do you see instead?
The output is correct in v0.4.0, but will show visible artifacts currently.

The source is from an unrelated issue [1]. good.png was produced using WIC 
decode from cwebp followed by a WIC encode using the code from dwebp.

(Attachments pending as this project seems to have hit its quota)

bisects to:
bf182e837ea466bb6888b521ca37dbf975cfbc79 is the first bad commit
commit bf182e837ea466bb6888b521ca37dbf975cfbc79
Author: skal <pascal.massimino@gmail.com>
Date:   Tue Feb 11 09:12:45 2014 -0800

    VP8LBitWriter: use a bit-accumulator

    * simplify the endian logic
    * remove the need for memset()
    * write 16 or 32 at a time (likely aligned)

    Makes the code a bit faster on ARM (~1%)

    Change-Id: I650bc5654e8d0b0454318b7a78206b301c5f6c2c

:040000 040000 b028b878a89e4b5fa7e222873a2245d900a47b27 
93afee49ab96652d2cae0b16d5e62f9cd49ca666 M      src

[1] 
https://groups.google.com/a/webmproject.org/d/msg/webp-discuss/A3Xq2p4R01E/n8Ezr
h0qpbMJ

Original issue reported on code.google.com by jz...@google.com on 21 May 2014 at 2:07

GoogleCodeExporter commented 9 years ago 
The problem is here [1]. With a 32-bit build the code will over accumulate 
before writing, e.g., used=16 n_bits=33:

 279   bw->bits_ |= (vp8l_atype_t)bits << bw->used_;
 280   bw->used_ += n_bits;
 281   if (bw->used_ > VP8L_WRITER_BITS) {

[1] 
https://gerrit.chromium.org/gerrit/gitweb?p=webm/libwebp.git;a=blob;f=src/utils/
bit_writer.c;h=07465c10eeaa33bf6f9134f769ea1f81aa1c3d23;hb=HEAD#l280

Original comment by jz...@google.com on 21 May 2014 at 2:08

GoogleCodeExporter commented 9 years ago 
indeed, adding the following assert at line bit_writer.c:281

assert(bw->used_ <= 8 * (int)sizeof(bw->bits_));

makes this repro case crash. Will follow up with a patch...

Original comment by s...@google.com on 21 May 2014 at 5:29

GoogleCodeExporter commented 9 years ago 
should be fixed by https://gerrit.chromium.org/gerrit/#/c/70230/

Original comment by pascal.m...@gmail.com on 21 May 2014 at 11:48

GoogleCodeExporter commented 9 years ago 
patch submitted, fix verified.

Original comment by pascal.m...@gmail.com on 22 May 2014 at 5:44