fix(core): read WebP VP8X canvas height from the correct byte offset - #5194
Conversation
| const width = (buffer.readUInt32LE(24) & 0xffffff) + 1; | ||
| const height = (buffer.readUInt32LE(26) & 0xffffff) + 1; | ||
| const width = buffer.readUIntLE(24, 3) + 1; | ||
| const height = buffer.readUIntLE(27, 3) + 1; |
There was a problem hiding this comment.
[Suggestion] The byte offsets 24 and 27 are undocumented magic numbers. The surrounding codebase has an established convention: extractPngDimensions documents "Width/height at bytes 16-19 and 20-23", and extractJpegDimensions documents "Dimensions at offset +5 (height) and +7 (width)". The VP8X branch is the only format parser without such documentation.
The bug this PR fixes was precisely caused by a wrong offset (26 instead of 27) — the absence of a spec-reference comment makes this code fragile against future edits.
| const height = buffer.readUIntLE(27, 3) + 1; | |
| } else if (format === 'VP8X') { | |
| // VP8X extended format: canvas dimensions are 24-bit LE values | |
| // stored as (dimension - 1). Width at bytes 24-26, height at 27-29. | |
| const width = buffer.readUIntLE(24, 3) + 1; | |
| const height = buffer.readUIntLE(27, 3) + 1; | |
| return { width, height }; |
— qwen3.7-max via Qwen Code /review
✅ Local verification — fix confirmed correct, test has teethI ran a real local verification of this PR in a Root cause (byte layout confirmed)In a VP8X chunk the canvas dimensions are two 24-bit little-endian values:
The old height read started one byte early: it took byte 26 (the high byte of width) as its LSB and dropped byte 29. Width was unaffected, so the symptom was correct width + garbage height. Verification matrix
EvidenceA — fixed code, including real VP8X files (not just the synthetic header in the unit test; generated with Pillow 12.1.1 as RGBA-lossy WebP, which B — revert the source fix only, re-run the same tests. Every failure is height-only with the predicted wrong value; width is always correct: This proves the new unit test genuinely catches the bug (it is not a tautological/always-green test), and the bug is real on actual encoder output, not only a hand-built header. D — why Because Impact
中文说明(点击展开)✅ 本地验证 —— 修复正确,且新增测试确实能抓到 Bug我在 Linux(Node 22.22.2、vitest 3.2.4) 上用真实 根因(字节布局已确认)VP8X chunk 中画布尺寸是两个 24 位小端值:
旧的高度读取早了一个字节:把第 26 字节(宽度的高位字节)当成了最低位,并丢掉了第 29 字节。宽度不受影响,所以表现为宽度正确、高度乱码。 验证矩阵
证据A —— 修复后代码,包含真实 VP8X 文件(不只是单测里手工构造的头;用 Pillow 12.1.1 生成的 RGBA 有损 WebP, B —— 仅回退源码修复,重跑同一批测试。 每个失败都只发生在高度上、且值与预测一致;宽度始终正确: 这证明新增单测确实能抓到该 Bug(不是恒为绿的无效测试),且该 Bug 在真实编码器输出上同样存在,而非仅限于手工构造的头。 D —— 为何用 由于 影响
Verification performed locally in tmux on Linux; the temporary real-file test was removed afterward and the working tree left identical to the PR's committed state. |
Address review: the VP8/VP8L/VP8X branches read width and height from bare numeric offsets. Add a short comment per branch noting the byte positions and little-endian layout, matching the convention already used by extractPngDimensions/extractJpegDimensions/extractBmpDimensions.
|
Good call — added a short comment to each branch (VP8/VP8L/VP8X) spelling out the byte offsets and little-endian layout, matching the style already used in |
|
@qwen-code /triage |
|
Thanks for the PR, @he-yufeng! Template looks good ✓ On direction: textbook bug fix — the VP8X height offset was one byte too early ( On approach: exactly the right scope — 2 source lines corrected, 1 focused unit test added, 3 clarifying comments on the sibling branches (VP8/VP8L) for consistency. Nothing extraneous. The choice of Moving on to code review. 🔍 中文说明感谢 @he-yufeng 的贡献! 模板完整 ✓ 方向:教科书式 bug 修复 —— VP8X 高度读取偏移早了一个字节( 方案:范围恰到好处 —— 仅修正 2 行源码,新增 1 个聚焦的单元测试,并为 VP8/VP8L 分支补充了 3 行一致性注释。没有多余内容。选择 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading diff): The VP8X spec puts canvas width-1 at bytes 24–26 and height-1 at bytes 27–29, both 24-bit little-endian. The old code read height with Diff matches my proposal exactly. The two-line fix is correct and minimal. The added comments on all three branches (VP8, VP8L, VP8X) document byte offsets consistent with the style already used in No correctness bugs, no security holes, no AGENTS.md violations. Nothing to flag. TestingUnit tests — fixed code (10/10 pass)Teeth check — revert source fix only, keep tests (1 fails as predicted)Width 100 is already correct (not affected by the offset bug); only height is wrong — matches the PR description exactly. The new test genuinely catches the bug. Tmux real-scenario noteThis fix affects internal image-token estimation for VP8X WebP files. The symptom was silently corrupted token counts (not a crash or visible error), so there's no meaningful TUI before/after to capture in tmux. Collaborator @wenshao independently verified against real Pillow-encoded VP8X files (321×123, 640×16) and confirmed correct parsing on the fix, wrong heights on the original code. SummaryThe fix is correct, the test has teeth, and the scope is minimal. LGTM from code review and testing. ✅ 中文说明代码审查独立方案(读 diff 之前): VP8X 规范将画布 width-1 放在第 24–26 字节,height-1 放在第 27–29 字节,均为 24 位小端。旧代码用 Diff 与我的方案完全一致。 两行修复正确且最小化。三个分支(VP8、VP8L、VP8X)上的注释记录了字节偏移,与 无正确性 bug、无安全漏洞、无 AGENTS.md 违规。无需标记的问题。 测试单元测试——修复后代码(10/10 通过)有效性检查——回退源码修复,保留测试(1 个如预期失败)宽度 100 本就正确(不受偏移 bug 影响);仅高度错误——与 PR 描述完全吻合。新测试确实能抓到该 bug。 Tmux 真实场景说明本修复影响 VP8X WebP 文件的内部图像 token 估算。症状是 token 数静默错误(非崩溃或可见错误),因此没有有意义的 TUI 前后对比可供 tmux 捕获。协作者 @wenshao 已独立用真实 Pillow 编码的 VP8X 文件(321×123、640×16)验证了修复后解析正确、原代码高度错误。 总结修复正确,测试有效,范围最小。代码审查和测试通过。✅ — Qwen Code · qwen3.7-max |
|
@qwen-code /triage |
ReflectionThis is one of those PRs that makes you glad someone cares about the details. The VP8X height offset bug was subtle — width happened to be correct because The fix is exactly what I would have written: The test has teeth: reverting the source fix produces The scope is minimal — 2 lines fixed, 1 test added, 3 comments for consistency with the VP8/VP8L siblings. Nothing extraneous. No AGENTS.md concerns, no security issues, no direction questions. If I had to maintain this in six months, I'd thank the author. The comments document the byte layout, the test is self-explanatory, and the PR description is a mini-spec for the VP8X header format. Verdict: ship it. ✅ 中文说明反思这个 PR 让人欣慰——有人在意这些细节。VP8X 高度偏移 bug 很隐蔽:宽度恰好正确是因为 修复方案和我自己会写的一样:用 测试有实际效果:回退源码修复后得到 范围最小化——2 行修复、1 个测试、3 行与 VP8/VP8L 分支一致的注释。没有多余内容。无 AGENTS.md 问题,无安全隐患,无方向疑问。 如果六个月后我要维护这段代码,我会感谢作者。注释记录了字节布局,测试自解释,PR 描述本身就是一份 VP8X 头部格式的迷你规范。 结论:可以合入。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
What this PR does
Fixes the canvas height read for WebP images in the extended (VP8X) format. The height was being read starting one byte too early, so the parser returned a garbage height for any VP8X image.
Why it's needed
In a VP8X chunk the canvas dimensions are two 24-bit little-endian values: width-minus-one at byte 24, height-minus-one at byte 27. The code read width correctly with
readUInt32LE(24) & 0xffffffbut read height from offset 26 (readUInt32LE(26) & 0xffffff), which spans bytes 26-28 instead of 27-29. So width comes out right and height is wrong.I switched both lines to
readUIntLE(offset, 3), which reads exactly the 3 bytes we want. This also avoids a subtler trap: a naivereadUInt32LE(27)would touch byte 30 and throw a RangeError on a minimal 30-byte VP8X header, which is still allowed by the existingbuffer.length < 30guard.readUIntLE(27, 3)stays in bounds.Reviewer Test Plan
How to verify
Added a unit test that builds a minimal 30-byte VP8X header for a 100x80 canvas and asserts the parsed dimensions.
Before the fix the new test fails with
expected 20225 to be 80(width 100 is already correct). After the fix all 10 tests pass.Evidence (Before & After)
N/A (not user-visible; covered by the unit test output above).
Tested on
Environment (optional)
Unit tests only.
Risk & Scope
Linked Issues
None.
中文说明
本 PR 修正 WebP 扩展格式(VP8X)画布高度的读取偏移。VP8X chunk 里画布尺寸是两个 24 位小端值:width-1 在第 24 字节,height-1 在第 27 字节。原代码用
readUInt32LE(24) & 0xffffff正确读出宽度,但高度从偏移 26 读(readUInt32LE(26) & 0xffffff),覆盖的是第 26-28 字节而非 27-29,导致宽度正确、高度错误。改为两行都用
readUIntLE(offset, 3),正好读取需要的 3 个字节。这也避开了一个隐患:直接写readUInt32LE(27)会触及第 30 字节,在仅 30 字节的最小 VP8X 头上抛 RangeError,而这种长度仍被现有的buffer.length < 30守卫放行;readUIntLE(27, 3)不会越界。新增单元测试构造一个 100x80 画布的最小 30 字节 VP8X 头并断言解析结果。修复前该测试报
expected 20225 to be 80(宽度 100 本就正确),修复后全部 10 个测试通过。