[image] vp8l: vp8l: avoid allocating many unused Huffman tree groups
Damien Neil (Gerrit)
Damien Neil voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Neal Patel
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 1 comment![]( "Open in Gerrit")
(Not sure if you're still reviewing x/image CLs, Nigel, but if you are then you're the actual expert here.)
Related details
- Neal Patel
- Nigel Tao
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nigel Tao (Gerrit)
Nigel Tao added 9 comments![]( "Open in Gerrit")
Sorry for the slow reply. I was on vacation last week.
vp8l: vp8l: avoid allocating many unused Huffman tree groupsDuplicate "vp8l:".
if maxHGroupIndex+1 > maxHuffImageSize {`if maxHGroupIndex >= maxHuffImageSize`
Ditto (turning `x + 1 > y` into `x >= y`) a few lines below.
"compress/bzip2"If `testdata/large-huffman-index.lossless.webp.bz2` is only 107 bytes long then it doesn't seem worth adding a `compress/bzip2` dependency here. Just commit the raw file.
"gopher-doc.8bpp",I wouldd slip "gopher-doc.skip-hgroup.lossless.webp" here instead, perhaps renaming it to "gopher-doc.8bpp.skip-hgroup". See also 💙.
f1, err := os.Open("../testdata/" + tc + ".png")You might then need a little fix-up here:
```
pngFilename := "../testdata/" + tc + ".png"
if tc == "gopher-doc.8bpp.skip-hgroup" {
pngFilename = "../testdata/gopher-doc.8bpp.png"
}
f1, err := os.Open(pngFilename)
```
switch ext := path.Ext(filename); ext {Instead of switching on the filename extension, the idiomatic approach is to basically just say `return image.Decode(reader)`, per https://pkg.go.dev/image#Decode
But then, if you also drop the bz2 thing, I don't think this `decodeFile` function wins you much.
for _, test := range []struct {Using an anonymous struct type (and `t.Run`) seems overkill if you only have one test case. I'd delete everything from lines 281 to 292 and hard-code name, f0 and f1.
But also, per 💙, we could probably delete TestDecodeComparisons entirely.
Ditto (re anonymous struct type) for TestDecodeErrors.
Related details
- Damien Neil
- Neal Patel
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nigel Tao (Gerrit)
Nigel Tao added 1 comment![]( "Open in Gerrit")
Sorry for the slow reply. I was on vacation last week.
dneil / neal, if you prefer (and it might take fewer SWE-hours overall), I'm happy to patch in this CL, make the suggested changes myself, and send out a revised CL for you to review.
Damien Neil (Gerrit)
Damien Neil uploaded new patchset![]( "Open in Gerrit")
- SLSA-Policy-Verified: +1 by SLSA Policy Verification Service
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
- Neal Patel
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted and added 9 comments![]( "Open in Gerrit")
Votes added by Damien Neil
| Commit-Queue | +1 |
Nigel TaoSorry for the slow reply. I was on vacation last week.
dneil / neal, if you prefer (and it might take fewer SWE-hours overall), I'm happy to patch in this CL, make the suggested changes myself, and send out a revised CL for you to review.
Sure, if you prefer. I've made some updates, but haven't quite followed your suggestions; PTAL and I'm happy to either handle more comments or an alternate CL.
vp8l: vp8l: avoid allocating many unused Huffman tree groupsDamien NeilDuplicate "vp8l:".
Done
`int` could be `int16`.
Done
`if maxHGroupIndex >= maxHuffImageSize`
Ditto (turning `x + 1 > y` into `x >= y`) a few lines below.
If `testdata/large-huffman-index.lossless.webp.bz2` is only 107 bytes long then it doesn't seem worth adding a `compress/bzip2` dependency here. Just commit the raw file.
Uncompressed size is 163879 bytes, which I think is worth it.
I wouldd slip "gopher-doc.skip-hgroup.lossless.webp" here instead, perhaps renaming it to "gopher-doc.8bpp.skip-hgroup". See also 💙.
Reworked this test a bit to handle the new test files.
- webp and png images don't have to have the same base name
- images can be bz2 compressed (this helps a lot with one case, see below)
- test cases can include expected decode failures
You might then need a little fix-up here:
```
pngFilename := "../testdata/" + tc + ".png"
if tc == "gopher-doc.8bpp.skip-hgroup" {
pngFilename = "../testdata/gopher-doc.8bpp.png"
}
f1, err := os.Open(pngFilename)
```
I'd really rather keep this in the table rather than add in special cases here.
Instead of switching on the filename extension, the idiomatic approach is to basically just say `return image.Decode(reader)`, per https://pkg.go.dev/image#Decode
But then, if you also drop the bz2 thing, I don't think this `decodeFile` function wins you much.
Acknowledged
Using an anonymous struct type (and `t.Run`) seems overkill if you only have one test case. I'd delete everything from lines 281 to 292 and hard-code name, f0 and f1.
But also, per 💙, we could probably delete TestDecodeComparisons entirely.
Ditto (re anonymous struct type) for TestDecodeErrors.
Overkill for only one case, but this seems like the sort of thing where more cases are inevitable.
Moved this into TestDecodeVP8L.
Related details
- Neal Patel
- Nigel Tao
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nigel Tao (Gerrit)
Nigel Tao voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nigel Tao
| Code-Review | +1 |
2 comments
for i := 0; i < maxHGroupIndex+1; i++ {`for i := range maxHGroupIndex+1`
"compress/bzip2"Damien NeilIf `testdata/large-huffman-index.lossless.webp.bz2` is only 107 bytes long then it doesn't seem worth adding a `compress/bzip2` dependency here. Just commit the raw file.
Uncompressed size is 163879 bytes, which I think is worth it.
- Damien Neil
- Neal Patel
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nigel Tao (Gerrit)
Nigel Tao voted Code-Review+2![]( "Open in Gerrit")
| Code-Review | +2 |
Related details
- Damien Neil
- Neal Patel
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neal Patel (Gerrit)
Neal Patel voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Damien Neil
- Neal Patel
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil uploaded new patchset![]( "Open in Gerrit")
- SLSA-Policy-Verified: +1 by SLSA Policy Verification Service
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Damien Neil
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
`for i := range maxHGroupIndex+1`
I keep forgetting x/ repos can use range-over-int now. Done.
Related details
- Nigel Tao
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: vp8l/decode.go
Insertions: 1, Deletions: 1.
@@ -388,7 +388,7 @@
}
hGroups = make([]hGroup, numHGroups)
- for i := 0; i < maxHGroupIndex+1; i++ {
+ for i := range maxHGroupIndex + 1 {
var hg *hGroup
if mapping == nil {
hg = &hGroups[i]
```
Change information
vp8l: avoid allocating many unused Huffman tree groups
A VP8L image contains a number of Huffman tree groups.
The number of groups is derived from the largest group index
observed in the file. The largest possible index is 2^16-1.
A VP8L file may included unused groups.
Decoding and storing all unused groups can cause excessive
memory allocation from a small input file.
Use the same approach to unused groups as libwebp:
When there are many groups (for some definition of "many"),
store the groups as a compacted array with unreferenced
groups removed, and rewrite all references to index into
the new array.
In addition, limit the total number of groups to 2600.
The libwebp encoder never produces more than this number of
groups, and there don't seem to be any other encoders in
common use.
Fixes golang/go#80069
Fixes CVE-2026-46603
- A testdata/gopher-doc.skip-hgroup.lossless.webp
- A testdata/large-huffman-index.lossless.webp.bz2
- M vp8l/decode.go
- M webp/decode_test.go
Code-Review: +2 by Nigel Tao, +1 by Neal Patel, +1 by Nigel Tao
SLSA-Policy-Verified: +1 by SLSA Policy Verification Service
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |