feat(lzw): LZWDecode + FlateDecode predictor refactor (PR #04, stacked on #6)#7
Conversation
…actor (SAPP PR #4) Implements PDF 1.7 §7.4.4 LZWDecode — the fourth and final filter PR in the chain-dispatcher series. Includes: 1. Refactor: lift PNG predictor out of FlateDecode into a new `applyPngPredictor` helper so both FlateDecode and LZWDecode can share it (D4). FlateDecode becomes a one-line wrapper for backward compatibility; every existing caller observes byte-for-byte identical behaviour. 2. LZW state machines: - LZWDecode($_stream, $params): string|false — variable-width MSB-first decoder (9-12 bits). Handles clear (256), EOD (257), KwKwK special case, EarlyChange parameter, PNG predictor invocation. Dict overflow at 4096 entries → p_error + return raw input. - LZWEncode($_stream, $params): string — standard LZW encoder. Leading CLEAR (Acrobat-compatible per OQ2). On dict overflow at 4096 entries: emit CLEAR and reset. 3. Bit-stream helpers (lzw_read_code / lzw_write_code) — MSB-first variable-width code IO with a caller-maintained 32-bit pending register and bit-position cursor. Dispatcher `case 'LZWDecode':` arms added to both encode + decode chain helpers. Encoder/decoder codeWidth synchronisation note for the eventual upstream PR: classical LZW has a one-entry off-by-one between encoder and decoder dictionary state (the decoder's first post-CLEAR read doesn't add an entry because `prev` is null). To keep both sides in lockstep on bit-width transitions, the decoder's threshold check uses `($nextCode + 1) >= $threshold` instead of `$nextCode >= $threshold`. Surfaced by the round-trip test failing on 2048-byte random inputs during implementation. Contract pinned by openspec/changes/feat-lzw-decode/ (proposal + design D1-D6 + spec REQ-1 through REQ-5 with MODIFIED filter-chain- dispatch capability + tasks). Verification: - examples/poc-filter-roundtrip-lzw.php (new) — 5 REQs covered: short distinct, repetition, empty round-trip; EarlyChange=0 on 2048 bytes; default round-trip on 2048 random; PNG predictor 12 parity with FlateDecode via shared applyPngPredictor; chain integration (single-filter and LZW+Flate two-filter). - All 5 prior gates remain green after the FlateDecode refactor (poc-replace-text, poc-filter-chain-roundtrip, asciihex, runlength, ascii85 round-trips). This commit completes the four-filter codec series. The chain dispatcher from PR #5 now supports the full spec-coverage set: FlateDecode (pre-existing) + ASCIIHexDecode + ASCII85Decode + RunLengthDecode + LZWDecode. Closes openspec/changes/feat-lzw-decode/tasks.md 35/35.
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Strict-mode review (REQUEST_CHANGES)
LZWDecode + FlateDecode predictor refactor with a working bit-stream codec, round-trip PoC, and chain wiring; the predictor refactor is mostly verbatim and parity-preserving, but several real defects exist: the LZWDecode error sentinel does not match the dispatcher's check, a documented predictor-failure branch is dead code due to a return-type contract drift, and EarlyChange wording vs implementation inconsistency.
Findings: 🔴 2 blockers · 🟡 6 concerns · 🟢 8 minors. All findings have been posted as inline comments above. Per Strict-mode rules, any 🟡 or 🔴 holds the verdict; address each thread before requesting re-review.
🔴 Blockers
- src/PDFObject.php:360 — LZWDecode error path returns encoded bytes, not false — chain dispatcher's
=== falsecheck never fires - src/PDFObject.php:420 — applyPngPredictor returns false on parameter rejection but its docblock + caller assume null — early-return branch in LZWDecode is dead
(Inline comments include **Impact:** and **Suggested fix:** sections per Strict-mode body template.)
Blockers: - All 4 LZWDecode error paths returned $_stream (the encoded bytes) while the docblock claimed @return string|false. The chain dispatcher's `=== false` check was dead code; truncated bit stream, dict overflow, KwKwK overflow, and out-of-range code all silently leaked encoded LZW bytes through the rest of the chain. All four now `return false` per the dispatcher contract. - applyPngPredictor's docblock said `@return string|null` but the method only returned `string|false` (via `return p_error(...)`). LZWDecode guarded with `if ($predicted === null)` — always false, so `$out = $predicted` assigned `false` (a bool) to a string-typed output. Fixed the docblock + LZWDecode now checks `=== false`. Concerns: - Same dead-code dispatcher pattern as other filters: addressed by the blocker fix above. - EarlyChange/decoder-lag comment rewritten: the `+1` in `($nextCode + 1) >= $threshold` is the decoder-lag correction (orthogonal to EarlyChange — both EarlyChange=0 and =1 work with it). Previous comment conflated the two. - Pre-existing PNG Sub-filter bug (`$data[$i] = ($data[$i] + $data[$i-1]) % 256` doing string-arithmetic on single-char strings) now reached through LZW too. Fixed to use the same ord()/chr() wrapping the Up filter already had. - applyPngPredictor docblock now states the documented carried-over limitations (Predictor=2 silently treated as <10, Colors=1 only, BitsPerComponent=8 only, Paeth/Average filter bytes rejected) so reviewers know LZW+Predictor=15 (auto-select) image streams may silently fail. - Asymmetric encode-path/predictor for `set_stream(false)` with /Predictor=12 is a pre-existing chain-dispatcher concern, documented as out-of-scope for this PR. Minors: - New tests: truncated bit stream (single 0x80 byte), out-of-range code (300 with empty dict), predictor rejection (Predictor=15 + Colors=3 triggers applyPngPredictor → false propagation), Sub filter byte coverage (exercises the previously-broken string- arithmetic loop), chain-failure propagation (outer truncated LZW MUST short-circuit before inner Flate runs). - docs/upstream-prs/04-lzw-decode/{proposal,design,tasks}.md: 34-line copy-pasted "Implementation note" blocks replaced with pointers to canonical openspec/changes/feat-lzw-decode/ (same pattern as PRs #1/#2/#3/#5). Verification: all 6 gates green (poc-replace-text, poc-filter-chain-roundtrip, poc-filter-roundtrip-{asciihex, runlength, ascii85, lzw}).
WilcoLouwerse
left a comment
There was a problem hiding this comment.
✅ APPROVE — Strict re-review against a218023.
All 2 🔴 Blockers and 6 🟡 Concerns from the first-pass review resolved or acceptably deferred. Both blocker fixes verified at HEAD with the dispatcher's === false contract now honoured across all four LZW failure paths; applyPngPredictor docblock and propagation path corrected.
Verified locally on a fresh clone at a218023 — all 6 PoC gates pass:
poc-filter-roundtrip-lzw(this PR — REQ-1..REQ-5 incl. new truncated / out-of-range / predictor-rejection / chain-failure tests)poc-replace-text,poc-filter-chain-roundtrip,poc-filter-roundtrip-{asciihex,runlength,ascii85}— confirms theFlateDecode→applyPngPredictorrefactor is behaviour-preserving for every existing caller.
Resolutions (each verified at HEAD; reply posted on each thread before resolving):
| Prior | Status | Where |
|---|---|---|
| 🔴 #1 Dispatcher dead-code | ✅ | All 4 paths return false at PDFObject.php:381 / 400 / 412 / 419 |
| 🔴 #2 Return-type contract drift | ✅ | docblock + === false check at :158-165, :454 |
| 🟡 #3 EarlyChange / decoder-lag framing | ✅ | orthogonal-concerns block at :422-441 |
| 🟡 #4 Carried-over Flate limits | ✅ | enumerated in applyPngPredictor docblock |
| 🟡 #5 PNG Sub-filter bug | ✅ | chr(ord+ord)%256 wrapping at :222-232 + new Sub-filter PoC test |
| 🟡 #6 Docblock contradicts impl | ✅ | same fix as #2 |
| 🟡 #7 Asymmetric encode path | Pre-existing FlateDecode/encoder concern (I originally noted "Pre-existing in FlateDecode too"). Acknowledged in commit body but not yet persisted to issue.md "Out of scope" — flagged as follow-up |
|
| 🟡 #8 Triplicate Implementation Note | ✅ | all 3 docs files reduced to 1-line canonical-pointers |
| 🟢 dealfonso#11 buildObj cosmetic redundancy | ✅ | chain-failure test now uses $rawStream parameter |
| 🟢 dealfonso#12 Predictor coverage narrow | ✅ | new Sub-filter test at poc:154-175 |
| 🟢 dealfonso#13 REQ-5 conflates concerns | ✅ | spec.md REQ-005 + 4 new PoC scenarios |
| 🟢 #9 / #10 / dealfonso#14 / dealfonso#15 / dealfonso#16 | acknowledged | author's call — not blocking |
Excellent fix-commit hygiene: structured by severity, code references in the commit body, all six PoC gates kept green. Ready to merge into feat/ascii85-decode.
feat(lzw): /LZWDecode filter + applyPngPredictor refactor (SAPP PR #4)
feat(tj): TJ-array flattening + multi-match in same TJ (SAPP PR #7)
Summary
Implements PDF 1.7 §7.4.4 LZWDecode — the fourth and final filter PR in the codec series. With this PR the chain dispatcher supports all five standard filters: FlateDecode (pre-existing) + ASCIIHexDecode + RunLengthDecode + ASCII85Decode + LZWDecode.
Stacked on #6 feat/ascii85-decode.
What landed
Refactor: PNG predictor extracted from FlateDecode
FlateDecodeis now a one-line wrapper around the newapplyPngPredictorhelper. Behaviour is byte-for-byte identical for every existing caller (the PoC verify gate is the regression check).LZWDecodecallsapplyPngPredictordirectly after its own decompression step. Done first so the LZW decoder doesn't duplicate the row-filter loop (D4).LZW state machines
PDFObject,protected static)LZWDecode($_stream, $params): string|falseLZWEncode($_stream, $params): stringlzw_read_code($stream, &$bitPos, $codeWidth): int|nulllzw_write_code(&$out, &$pending, &$pendingBits, $code, $codeWidth): voidBoth wired into
apply_filter_chain_decode/apply_filter_chain_encodeviacase 'LZWDecode':.Spec contract
Pinned by
openspec/changes/feat-lzw-decode/:EarlyChangeparameter (default 1; explicit 0)applyPngPredictorImplementation note worth surfacing in upstream review
Classical LZW has a one-entry off-by-one between encoder and decoder dictionary state: the decoder's first post-CLEAR read doesn't add an entry (because
previs null), so the decoder'snextCodelags by exactly one. To keep both sides synchronised on bit-width transitions, the decoder's threshold check uses($nextCode + 1) >= $thresholdinstead of$nextCode >= $threshold. Without this compensation, the encoder advances to 10-bit codes one iteration before the decoder, and the round-trip fails on inputs > ~250 bytes. Surfaced during implementation by the 2048-byte round-trip test failing.Test plan
php examples/poc-filter-roundtrip-lzw.php— 5 REQs, 0 assertion failuresphp -lclean on touched filesFiles changed (675 +, 64 −)
src/PDFObject.php— LZWDecode + LZWEncode + bit-stream helpers + FlateDecode predictor extractionexamples/poc-filter-roundtrip-lzw.php— new round-trip gatedocs/upstream-prs/04-lzw-decode/{proposal,design,tasks}.md— implementation notesopenspec/changes/feat-lzw-decode/tasks.md— 35/35 completeDependencies + ordering
Out of scope
applyPngPredictorrejects these withp_error; can be added if a real-world fixture demands it)