Issue 551 in webp: VP8L specification contains errors

[jz… via monorail]

Updates:
Cc: jy...@google.com

Comment #8 on issue 551 by jz...@google.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c8

(No comment was entered for this change.)

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[j… via monorail]

Comment #9 on issue 551 by j...@minteronline.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c9

Not sure if there is any progress on this but I found another issue:
In section 3 when describing the color transform:
[code]
void ColorTransform(uint8 red, uint8 blue, uint8 green,
ColorTransformElement *trans,
uint8 *new_red, uint8 *new_blue) {
// Transformed values of red and blue components
uint32 tmp_red = red;
uint32 tmp_blue = blue;
// Applying transform is just adding the transform deltas
tmp_red += ColorTransformDelta(trans->green_to_red, green);
tmp_blue += ColorTransformDelta(trans->green_to_blue, green);
tmp_blue += ColorTransformDelta(trans->red_to_blue, red);
*new_red = tmp_red & 0xff;
*new_blue = tmp_blue & 0xff;
}
[/code]
Note that the original red value is used for the red->blue transform, not the modified red with the green->red transform added. Also, it says that during decoding the inverse transform is used which is shown as:
[code]
void InverseTransform(uint8 red, uint8 green, uint8 blue,
ColorTransformElement *p,
uint8 *new_red, uint8 *new_blue) {
// Applying inverse transform is just subtracting the
// color transform deltas
red -= ColorTransformDelta(p->green_to_red_, green);
blue -= ColorTransformDelta(p->green_to_blue_, green);
blue -= ColorTransformDelta(p->red_to_blue_, red & 0xff);
*new_red = red & 0xff;
*new_blue = blue & 0xff;
}
[/code]

However yet again this is wrong and in the real implementation (libwebp in dsp/lossless.c) the "inverse" decoding transform is the one that adds the delta values (not subtracts like shown in the spec) and the modified red (with the green->red transform added) is used for the red->blue transform:
[code]
void VP8LTransformColorInverse_C(const VP8LMultipliers* const m,
const uint32_t* src, int num_pixels,
uint32_t* dst) {
int i;
for (i = 0; i < num_pixels; ++i) {
const uint32_t argb = src[i];
const int8_t green = (int8_t)(argb >> 8);
const uint32_t red = argb >> 16;
int new_red = red & 0xff;
int new_blue = argb & 0xff;
new_red += ColorTransformDelta(m->green_to_red_, green);
new_red &= 0xff;
new_blue += ColorTransformDelta(m->green_to_blue_, green);
new_blue += ColorTransformDelta(m->red_to_blue_, (int8_t)new_red);
new_blue &= 0xff;
dst[i] = (argb & 0xff00ff00u) | (new_red << 16) | (new_blue);
}
}
[/code]

The green and predict transforms are correct, and the color-indexing transform seems to be correct but I haven't looked into it that much and I don't have a test image for it yet. Overall though, I have almost-fully implemented the VP8L/lossless half of WebP decoding (except for color-indexing): https://github.com/matanui159/jebp

Also, even though this probably isn't the correct issue tracker for this but it is related, I have started to work on the VP8/lossy half of my decoder and have already found an issue in THAT spec as well:
In section 9.3 it mentions: the mode of segment feature data (segment_feature_mode), can be absolute-value mode (0) or delta value mode (1).
Which disagrees with section 19.2: segment_feature_mode indicates the feature data update mode, 0 for delta and 1 for the absolute value.
From my understanding the later mention (19.2) is the correct one.

[jz… via monorail]

Updates:
Blocking: 448
Cc: -vra...@google.com
Owner: vra...@google.com

Comment #10 on issue 551 by jz...@google.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c10

(No comment was entered for this change.)

[Git Watcher via monorail]

Comment #11 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c11

The following revision refers to this bug:
https://chromium.googlesource.com/webm/libwebp/+/f0e9351ccea5dec6bc01a52e16647f5f19de232d

commit f0e9351ccea5dec6bc01a52e16647f5f19de232d
Author: James Zern <jz...@google.com>
Date: Fri Mar 11 21:09:27 2022

webp-lossless-bitstream-spec,cosmetics: fix some typos

Bug: webp:551
Change-Id: I9ff90601b77deb9034ed7bef9cfd421d1f6e2e2b

[modify] https://crrev.com/f0e9351ccea5dec6bc01a52e16647f5f19de232d/doc/webp-lossless-bitstream-spec.txt

[jz… via monorail]

Comment #12 on issue 551 by jz...@google.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c12

> Also, even though this probably isn't the correct issue tracker for this but
> it is related, I have started to work on the VP8/lossy half of my decoder and
> have already found an issue in THAT spec as well: In section 9.3 it mentions:
> the mode of segment feature data (segment_feature_mode), can be
> absolute-value mode (0) or delta value mode (1). Which disagrees with section
> 19.2: segment_feature_mode indicates the feature data update mode, 0 for
> delta and 1 for the absolute value. From my understanding the later mention
> (19.2) is the correct one.

Thanks for pointing this out. I think https://crbug.com/webm/1687 raised the
same issue. It looks like we still need to file an errata.

[jz… via monorail]

Comment #13 on issue 551 by jz...@google.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c13

> The green and predict transforms are correct, and the color-indexing transform seems to be correct but I haven't looked into it that much and I don't have a test image for it yet.

There is some test data in a separate repo [1] if you haven't seen it.

[1] https://chromium.googlesource.com/webm/libwebp-test-data

[vrab… via monorail]

Comment #14 on issue 551 by vra...@google.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c14

I tried to fill in the blanks about the Huffman code in https://chromium-review.googlesource.com/c/webm/libwebp/+/3578761, please review.

[vrab… via monorail]

Comment #15 on issue 551 by vra...@google.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c15

Hi, any comment on the previous pull request ? Thx.

[Git Watcher via monorail]

Comment #16 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c16

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/86f94ee010347dd3adb5940e054c3dac4702b3b1

commit 86f94ee010347dd3adb5940e054c3dac4702b3b1
Author: vra...@google.com <vra...@google.com>
Date: Wed May 11 22:40:45 2022

Update lossless spec with Huffman codes.

Bug: webp:551
Change-Id: I28b57c8e87a8023b727fe8359c2e2809cf92752d

[modify] https://crrev.com/86f94ee010347dd3adb5940e054c3dac4702b3b1/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #17 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c17

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/a2093acc4b7dae420d9f73bbaf4c4f20bc0097b1

commit a2093acc4b7dae420d9f73bbaf4c4f20bc0097b1
Author: James Zern <jz...@google.com>
Date: Fri Mar 25 02:36:30 2022

webp-lossless-bitstream-spec: add amendment note

for recent changes to color transform, simple code length code, image
structure and details of decoding prefix codes

Bug: webp:551
Change-Id: I36a4d1ee3e25b8a56f5d926f4770374ace99bc2f

[modify] https://crrev.com/a2093acc4b7dae420d9f73bbaf4c4f20bc0097b1/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #18 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c18

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/86c669303ac5e19049c783fb4e3b183276a53c2c

commit 86c669303ac5e19049c783fb4e3b183276a53c2c
Author: James Zern <jz...@google.com>
Date: Sat Mar 12 18:52:33 2022

webp-lossless-bitstream-spec: fix BNF

color cache info precedes the meta huffman info

Bug: webp:551
Change-Id: Ibb69b4ffad6bff5693015be3fe6c600c53fd06c0

[modify] https://crrev.com/86c669303ac5e19049c783fb4e3b183276a53c2c/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #19 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c19

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/232f22da5a27219308ca791ae371e92e1cb45cfa

commit 232f22da5a27219308ca791ae371e92e1cb45cfa
Author: James Zern <jz...@google.com>
Date: Sat Mar 12 03:21:26 2022

webp-lossless-bitstream-spec: fix 'simple code' snippet

based on https://crbug.com/webp/551#c3 & https://crbug.com/webp/551#c4

Bug: webp:551
Change-Id: I4bcc19643b0bdda999247e9c3457d6954a49e35f

[modify] https://crrev.com/232f22da5a27219308ca791ae371e92e1cb45cfa/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #20 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c20

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/44dd765defa2eb54c0baf68068ae2607739ad851

commit 44dd765defa2eb54c0baf68068ae2607739ad851
Author: James Zern <jz...@google.com>
Date: Fri Mar 11 21:51:51 2022

webp-lossless-bitstream-spec: fix ColorTransform impl

and the corresponding InverseTransform(); the operations were swapped.
The forward transform subtracts the deltas, the inverse adds them.

Bug: webp:551
Change-Id: Ieb90a24f843cee7cdfe46cbb15ec7d716ca9d269

[modify] https://crrev.com/44dd765defa2eb54c0baf68068ae2607739ad851/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #21 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c21

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/7a7e33e97728d71050ff61d49763ba5bccc0e344

commit 7a7e33e97728d71050ff61d49763ba5bccc0e344
Author: James Zern <jz...@google.com>
Date: Fri Mar 11 21:10:05 2022

webp-lossless-bitstream-spec: fix TR-pixel right border note

the value of the TR-pixel on the right border is the leftmost pixel on
the current row, not the previous one

Bug: webp:551
Change-Id: I157e8c16ce43a9f52c36aa880be1be3bef19c063

[modify] https://crrev.com/7a7e33e97728d71050ff61d49763ba5bccc0e344/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #22 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c22

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/e74f8a62b2bb0a7c839d6403a03f15775a6685aa

commit e74f8a62b2bb0a7c839d6403a03f15775a6685aa
Author: James Zern <jz...@google.com>
Date: Sun May 15 03:50:47 2022

webp-lossless-bitstream-spec,cosmetics: normalize range syntax

[N, M] -> [N..M] the latter is more common in this doc

Bug: webp:551
Change-Id: I2d317c4b8bb31d313123e981f93cd1feb346a98a

[modify] https://crrev.com/e74f8a62b2bb0a7c839d6403a03f15775a6685aa/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #23 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c23

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/5a709ec0d76eaecbc347d60d649b59f6612efa60

commit 5a709ec0d76eaecbc347d60d649b59f6612efa60
Author: James Zern <jz...@google.com>
Date: Sun May 15 03:49:51 2022

webp-lossless-bitstream-spec,cosmetics: fix code typo

code_lengths -> code_length_code_lengths
after:
86f94ee0 Update lossless spec with Huffman codes.

Bug: webp:551
Change-Id: I9e77f6db0fca6fe8294cb31a3015e723050094b5

[modify] https://crrev.com/5a709ec0d76eaecbc347d60d649b59f6612efa60/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #24 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c24

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/404c1622f89f8def606eb571e17aca1bda39be19

commit 404c1622f89f8def606eb571e17aca1bda39be19
Author: Jyrki Alakuijala <jy...@google.com>
Date: Thu Jun 16 17:46:18 2022

Rename Huffman coding to prefix coding in the bitstream spec

... since no guarantee of Huffman coding can be introduced at decoding
time and encoding didn't actually use Huffman coding in the first place

Bug: webp:551
Change-Id: I400466bb3b4a1d5506353eb3f287d658603164ee

[modify] https://crrev.com/404c1622f89f8def606eb571e17aca1bda39be19/doc/webp-lossless-bitstream-spec.txt

[jz… via monorail]

Updates:
Status: Fixed

Comment #25 on issue 551 by jz...@google.com: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c25

(No comment was entered for this change.)

[Git Watcher via monorail]

Comment #27 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c27

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/e91451b69bfc08691a71a995265c66a890920ad7

commit e91451b69bfc08691a71a995265c66a890920ad7
Author: Vincent Rabaud <vra...@google.com>
Date: Mon Jul 25 20:34:54 2022

Fix the lossless specs a bit more.

Bug: webp:551

Change-Id: I03f729f69d660f17e27cc601e91c703b241f1b83

[modify] https://crrev.com/e91451b69bfc08691a71a995265c66a890920ad7/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #28 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c28

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/c6ac672dbcfd63d2e6354aea83f3d343cb2cbbfd

commit c6ac672dbcfd63d2e6354aea83f3d343cb2cbbfd
Author: James Zern <jz...@google.com>
Date: Mon Nov 21 19:02:45 2022

webp-lossless-bitstream-spec: fix num_code_lengths check

in the 'Normal Code Length Code' description the number of valid code
lengths is 19, not 18.

Bug: webp:448
Bug: webp:551
Change-Id: Id929604e1d771cb09b2d0ac617e83f21077f21de

[modify] https://crrev.com/c6ac672dbcfd63d2e6354aea83f3d343cb2cbbfd/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #29 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c29

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/786497e48aa03179b2425a89a57b3cf89efd3be4

commit 786497e48aa03179b2425a89a57b3cf89efd3be4
Author: James Zern <jz...@google.com>
Date: Mon Nov 21 18:41:44 2022

webp-lossless-bitstream-spec: fix inv color txfm description

The prose describing the process was missed in:
44dd765d webp-lossless-bitstream-spec: fix ColorTransform impl

Bug: webp:448
Bug: webp:551

Change-Id: If5fb95103ffedeed113e3ad62510f3a19bfd280e

[modify] https://crrev.com/786497e48aa03179b2425a89a57b3cf89efd3be4/doc/webp-lossless-bitstream-spec.txt

[Git Watcher via monorail]

Comment #30 on issue 551 by Git Watcher: VP8L specification contains errors
https://bugs.chromium.org/p/webp/issues/detail?id=551#c30

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libwebp/+/0ceeeab987cba59ef7ee3369bdc167067094ddc2

commit 0ceeeab987cba59ef7ee3369bdc167067094ddc2
Author: James Zern <jz...@google.com>
Date: Mon Nov 21 19:24:16 2022

webp-lossless-bitstream-spec: add amendment note

for fixes to the distance_map pseudo-code, the inverse color transform
description and the num_code_lengths check.

Bug: webp:448
Bug: webp:551

Change-Id: Id7e791b97704dd64bb9519657ce48b92cb457ae4

[modify] https://crrev.com/0ceeeab987cba59ef7ee3369bdc167067094ddc2/doc/webp-lossless-bitstream-spec.txt