alpha artifacts

[GoogleCodeExporter]

When I encode and then decode images using simple api there are sometimes 
defects in the alpha plane.
The defects vary depending on compression level and the original image.
Only few of hundreds images are affected (however I spot them by eye).

This is present if I compile current webp sources (latest attempt from 
da0912126b62f2f9390bf2a149784f7a0ed547de).
I tried 4.2.0 sources and could not spot any problem.

01.tga, quality=90
defective alpha at (66,24): value=255
source value=2

00.tga, quality=90
defective alpha blocks at (19,42), (570,42): value=57
source value=0

Original issue reported on code.google.com by shekh.an...@gmail.com on 22 Jan 2015 at 1:24

Attachments:

• images.zip

[GoogleCodeExporter]

thanks for the report!
i can confirm some values are badly compressed on the alpha plane.
See attached the Y/U/V/A planes for a 00.tga compressed with 0.4.2 vs HEAD.
Investigating...

Original comment by pascal.m...@gmail.com on 22 Jan 2015 at 2:55

• Changed state: Accepted

Attachments:

• 00_HEAD_bad.png
• 00_0.4.0_good.png

[GoogleCodeExporter]

looks like the regression was introduced by 
https://gerrit.chromium.org/gerrit/#/c/72101/1/src/enc/vp8l.c
with a bad interaction between references tokens and increased color_cache bits.

I will submit an "emergency fix" to shut this new feature off until the problem 
is figured out.
->   https://gerrit.chromium.org/gerrit/73565

btw: attaching the good 00_HEAD_bad.png this time, for future reference.

Original comment by pascal.m...@gmail.com on 22 Jan 2015 at 6:15

Attachments:

• 00_HEAD_bad.png

[GoogleCodeExporter]

v0.3.1 appears to be correct, so using that as a base this bisects to:

64c844863ab0d13e8dd51a59001ab55fbe64065e is the first bad commit
commit 64c844863ab0d13e8dd51a59001ab55fbe64065e
Author: Urvang Joshi <urvang@google.com>
Date:   Mon May 13 16:24:49 2013 -0700

    Further reduce memory to decode lossy+alpha images

    Earlier such images were using roughly 9 * width * height bytes for
    decoding. Now, they take 6 * width * height memory.

    Change-Id: Ie4a681ca5074d96d64f30b2597fafdca648dd8f7

:040000 040000 da89ac00cb7fa3ca1bd34073037c38d58f5cf420 
8a8f8f98f2e1854a47a5eadf606e9809a9fd89ad M      src

Original comment by jz...@chromium.org on 22 Jan 2015 at 9:11

[GoogleCodeExporter]

strange, 0.4.2 is correct here. The error seemed to have been introduced 
since...

Original comment by pascal.m...@gmail.com on 22 Jan 2015 at 9:51

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

As discussed offline, the regression James found was fixed in: 
https://gerrit.chromium.org/gerrit/#/c/58842/, and the image decodes fine at 
that point.

So, I reran bisect from that commit, and here's what I got.

a4d5f59d9e0869009433103818e87d131a56270d is the first bad commit
commit a4d5f59d9e0869009433103818e87d131a56270d
Author: skal <pascal.massimino@gmail.com>
Date:   Mon Jun 24 09:34:30 2013 +0200

    Faster lossless decoding

    Specialize and simplify the alpha-decoding case, which is used when:
     - no color-cache is use
     - all red/blue/alpha values are the same (and hence their Huffman tree has
     only 1 symbol. We don't need to consume any bits for reading these).

     + revamped the loop to use size_t and offsets instead of pointers.

     ~2-3% faster on Unix (gcc) but up to 25% faster lossy+alpha decoding
     on Mac (llvm) and ARM.

    Change-Id: I43c9688d1e4811cab0ecf0108a5b8f45781083e6

:040000 040000 3f13501c31f3b1e28d9023d7336f16a8a8d03279 
7504e82c2f23ca486a377eb4a6d93b1e342a8aea M      src

Original comment by urv...@google.com on 22 Jan 2015 at 9:58

[GoogleCodeExporter]

I ended up at the same point, earlier I was bisecting across 2 working branches 
with a merge in between which may have caused the difference. When I check from 
that merge (30e77d0 Merge branch '0.3.0') I hit the same commit.

Original comment by jz...@google.com on 22 Jan 2015 at 10:24

[GoogleCodeExporter]

Pascal, try to directly decode the alpha plane of the image encoded with HEAD.
This works fine before the commit a4d5f59d9e0869009433103818e87d131a56270d I 
mentioned above, but not anytime after. (0.4.2 is bad as well).

Original comment by urv...@google.com on 22 Jan 2015 at 10:25

[GoogleCodeExporter]

This seems to be related to a change in DecodeImageData() from the earlier 
macro implementation:

-      if (src < src_end) {                                                     
\
-        htree_group = GetHtreeGroupForPos(hdr, col, row);                      
\
+      if (src < src_last) {
+        if (col & mask) htree_group = GetHtreeGroupForPos(hdr, col, row);

specifically the if condition, reverting that to src_end works here where (row, 
col, last_row)==(40, 40, 40); I haven't looked more closely at the logic 
itself...

Original comment by jz...@google.com on 22 Jan 2015 at 11:55

[GoogleCodeExporter]

Indeed, that is the problem.
In fact, no if condition is needed, as src <= src_end is implied at that point.
Sending a patch shortly.

Original comment by urv...@google.com on 23 Jan 2015 at 1:13

[GoogleCodeExporter]

i think "if (src < src_end) { ... }" is indeed correctly fixing the issue

Original comment by pascal.m...@gmail.com on 23 Jan 2015 at 7:16

[GoogleCodeExporter]

Fixed here: https://gerrit.chromium.org/gerrit/#/c/73580/
Thanks again for the report!

Original comment by urv...@google.com on 23 Jan 2015 at 9:56

• Changed state: Fixed