feat(runlength): RunLengthDecode encode + decode + chain wiring (PR #02, stacked on #4)#5
Conversation
… PR #2) Implements PDF 1.7 §7.4.5 RunLengthDecode on top of the chain dispatcher from PR #5. Adds two `case` arms in apply_filter_chain_decode / apply_filter_chain_encode — orthogonal to ASCIIHexDecode (#1); the two filter PRs do not interact. New helpers on PDFObject (both `protected static`, PascalCase per upstream's filter-name convention): - RunLengthDecode($_stream, $params): string|false Single-pass state machine. Literal blocks (L ∈ [0,127] → copy L+1 bytes), EOD (L = 128 → stop), repeat blocks (L ∈ [129,255] → repeat next byte 257-L times). Bounds-checks every substr / byte read; truncation / missing EOD → p_error + return raw input. - RunLengthEncode($_stream, $params): string Greedy run-or-literal accumulator. Detects 2+ adjacent identical bytes as a repeat block (capped at 128 per block), otherwise accumulates a literal block (also capped at 128). Always emits trailing 0x80 EOD, including for empty input (output is a single byte) per OQ1. Dispatcher `case 'RunLengthDecode':` arms added to both encode + decode chain helpers. Spec correction during impl: the original REQ-2 literal-only encode scenario used `"Hello"` as the example — but `"Hello"` contains the `"ll"` run, so the greedy encoder correctly emits a repeat block, not a pure-literal block. Updated the spec scenario to use any 5-byte string with no adjacent duplicates (`"abcde"` in the test) so the contract is verifiable. Contract pinned by openspec/changes/feat-runlength-decode/ (proposal + design D1-D4 + spec REQ-1 through REQ-5 with MODIFIED filter-chain-dispatch capability + tasks). Verification: - examples/poc-filter-roundtrip-runlength.php (new) — 5 REQs: literal-run / repeat-run / mixed / EOD-halts decode scenarios; literal / repeat / empty encode scenarios; 1024-byte mixed- pattern lossless round-trip; 3 truncation fail-safes; chain integration (single-filter + RunLength outer / Flate inner). - examples/poc-replace-text.php — baseline still green. - examples/poc-filter-chain-roundtrip.php — dispatcher tests green. - examples/poc-filter-roundtrip-asciihex.php — sibling filter still green (#1 unaffected). Closes openspec/changes/feat-runlength-decode/tasks.md 22/22.
| @@ -420,6 +539,13 @@ protected static function apply_filter_chain_decode($bytes, array $filters, arra | |||
| } | |||
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFObject.php:539; finding concerns src/PDFObject.php:488.)
🟡 Concern — Greedy run detection bloats output
The literal-accumulation peek-ahead breaks out of a literal block as soon as ANY 2-byte run is detected. For input "ABBC", this emits \x00A\xFFB\x00C\x80 = 7 bytes; a single 4-byte literal \x03ABBC\x80 would be 6 bytes. The PR body acknowledges 'suboptimal vs Lempel-Ziv-style by ~5% on pathological inputs' but the threshold for breaking to repeat mode should arguably be 3+ adjacent identical bytes.
Suggested fix: Either raise the run threshold to 3, or add a dedicated ABBCDDEF-style fixture to REQ-2 documenting the size regression.
There was a problem hiding this comment.
🟡 Still open — encoder bloat unaddressed
The blocker and most concerns are fixed, but the encoder's 1-byte peek that aborts a literal block on the first 2-byte run is unchanged. For input "ABBC" the encoder still emits \x00A\xFFB\x00C\x80 (7 bytes) instead of the 6-byte literal \x03ABBC\x80. Prior suggested fix — raise the run threshold to 3, OR add an ABBC-style fixture to REQ-2 documenting the regression — remains valid.
Non-blocker under Standard mode. Suggest a // TODO(opt): + follow-up issue, OR a fixture in this PR — either is fine. Leaving the thread open for the follow-up.
| echo "RESULT: VERIFIED — all 5 REQs covered, no assertion failures.\n"; | ||
| exit(0); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟢 Minor — Error-channel assumption
Lines wrap each truncation call in ob_start() / ob_end_clean() to silence p_error. Doesn't assert that p_error was actually called.
Suggested fix: Use ob_get_clean() and assert the captured string contains the expected error fragment.
There was a problem hiding this comment.
🟢 Still open — p_error not asserted (optional)
The fix added the chain-failure-propagation test (good), but the existing truncation tests at examples/poc-filter-roundtrip-runlength.php:152-154, 161-163, 170-172, 192-194 still pair ob_start() with ob_end_clean(), discarding the captured output. A regression where p_error() stops being called (e.g. someone replaces it with a silent return false) would still pass.
Optional — fine to defer to a follow-up.
| @@ -420,6 +539,13 @@ protected static function apply_filter_chain_decode($bytes, array $filters, arra | |||
| } | |||
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFObject.php:539; finding concerns src/PDFObject.php:497.)
🟢 Minor — No defensive guard
Static analysis: literal mode is only entered when $runLen < 2, so the first iteration cannot break immediately. Safe in practice. A defensive assert($literalLen > 0) would survive future refactors of the surrounding logic.
Suggested fix: Add a one-line guard.
There was a problem hiding this comment.
🟢 Still open — defensive guard not added (optional)
Encoder literal-mode entry at src/PDFObject.php:318-321 still relies implicitly on $runLen < 2 falling through from the earlier branch. A one-line assert($literalLen >= 1 || $i >= $len) or a clarifying comment about the branch dependency would survive future refactors.
Optional — fine to defer.
| foreach ($failures as $msg) { | ||
| echo " - $msg\n"; | ||
| } | ||
| exit(1); |
There was a problem hiding this comment.
(Anchored to nearest diff line examples/poc-filter-roundtrip-runlength.php:212; finding concerns examples/poc-filter-roundtrip-runlength.php:265.)
🟢 Minor — Two-filter chain test under-specifies structure
The two-filter [/RunLengthDecode /FlateDecode] test only verifies plaintext round-trip equality. Doesn't assert the encoded stream's last byte is \x80 (RunLength EOD at outer layer) or that the Flate-decoded inner layer is a valid Flate stream.
Suggested fix: Add substr($obj->get_stream(true), -1) === "\x80" assertion mirroring the single-filter case.
There was a problem hiding this comment.
🟢 Partial — happy-path structural assertion still missing
The new failure-path test at examples/poc-filter-roundtrip-runlength.php:183-199 proves the dispatcher short-circuits on outer-RLE truncation — nice addition. But the happy-path two-filter test at examples/poc-filter-roundtrip-runlength.php:224-241 still only asserts plaintext-equality on round-trip; nothing asserts the encoded stream ends with \x80 (outer RLE EOD).
Suggested fix unchanged: add a substr($obj->get_stream(true), -1) === "\x80" check mirroring the single-filter case at examples/poc-filter-roundtrip-runlength.php:214-216. Optional.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Strict-mode review (REQUEST_CHANGES)
Functionally correct RLE encoder/decoder with verifiable round-trip, but ships a silent-fail dispatcher bug (decoder never returns false despite docstring claim, making the chain's failure short-circuit dead code), several documentation inconsistencies, and a non-optimal 2-byte-run encoding that bloats output without test coverage.
Findings: 🔴 1 blockers · 🟡 4 concerns · 🟢 7 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:333 — Decoder returns raw $_stream on failure but PHPDoc / dispatcher expect
false→ dead-code short-circuit, silent data corruption in filter chain
(Inline comments include **Impact:** and **Suggested fix:** sections per Strict-mode body template.)
Blocker:
- RunLengthDecode: 3 failure paths return $_stream → return false. The
chain dispatcher's `=== false` short-circuit was dead code; downstream
filters could see partial output. Matches ASCIIHexDecode's fix pattern.
Concerns:
- spec.md REQ-4: return-value semantics now say false (not raw input);
added missing-EOD scenario + chain-failure-propagation scenario.
- spec.md REQ-2 literal-only: note that "Hello" doesn't qualify as
literal-only — its `ll` pair is a 2-byte run the greedy encoder
flushes as a separate repeat block.
- PoC: 3 truncation tests now expect false; new chain-failure-
propagation test (outer truncated RLE + inner Flate; dispatcher
MUST return false before Flate runs).
- PoC: 128-byte and 256-byte boundary tests for the repeat-block
cap (max block length per PDF 1.7 §7.4.5).
- PHPDoc: documents the 64× decode-amplification trust assumption
(caller is responsible for input bounds; explicit cap is a follow-up).
- tasks.md: dispatcher case label is bare 'RunLengthDecode' (no slash).
Minors:
- docs/upstream-prs/02-runlength-decode/{proposal,design,tasks}.md:
replaced duplicated 12-line "Implementation note" blocks with a
pointer to the canonical openspec/changes/feat-runlength-decode/
design.md (single source of truth).
Verification: all 4 gates green (poc-replace-text,
poc-filter-chain-roundtrip, poc-filter-roundtrip-asciihex,
poc-filter-roundtrip-runlength).
…runlength-decode # Conflicts: # openspec/changes/feat-runlength-decode/specs/runlength-decode-filter/spec.md
feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #5 — foundation)
Blockers:
- ASCII85Decode silently accepted 1-char trailing partial groups,
emitting 0 bytes via `$emit = $groupLen - 1` when $groupLen=1.
PDF 1.7 §7.4.3 partial-group rule requires `2 ≤ k ≤ 4`. Now
rejects with p_error + `return false`.
- REQ-1 test fixtures used `87cURD~>` claiming it encodes "Hell"
— actual canonical encoding is `87cUR~>` (5 chars). The `D` was
a stray 1-char partial that the spec-violation above accepted as
zero bytes. Test now uses verified canonical encodings.
- PHPDoc + design + proposal + tasks all claimed `uuuuu` = exactly
2^32-1. Arithmetic shows `uuuuu` = 84*(85^4+85^3+85^2+85+1) =
4,437,053,124, which is 142,085,829 OVER the 2^32-1 cap. The
spec-imposed max 5-char group is actually `s8W-!`. Comments and
docs now state this correctly; overflow guard text rewritten.
Concerns:
- Same dead-code pattern as ASCIIHex/RunLength: all decoder failure
paths now `return false` (not `$_stream`), matching dispatcher
contract.
- Error path was returning the post-`<~`-strip mutated stream as
"raw input" — moot now that all failure paths return false.
- Defensive 32-bit overflow code in encoder (`& 0xFFFFFFFF` + signed
promote) was dead on 64-bit PHP and broken on 32-bit (literal
`0xFFFFFFFF` parses as float on 32-bit, bitwise-AND coerces and
breaks the math). Deleted with comment that 64-bit ints are
required (upstream composer constraint already guarantees this).
- preg_replace whitespace-strip now null-guarded (PCRE compile
failure / limit → p_error + return false, no PHP 8.1+ deprecation
on strlen(null)).
- New negative tests: `~` mid-stream, `z` mid-group, 1-char partial
group, overflow guard fires on `tttt~>` (yields 4,384,231,064 >
2^32-1).
- Boundary coverage extended to n=1..9 (previously skipped
multiples of 4 — exactly the boundary between full-group and
partial-group code paths).
- New empty-payload edge cases: `<~~>` and bare `~>`.
- New chain-failure-propagation test (outer ASCII85 illegal char
MUST short-circuit before inner FlateDecode runs).
Minors:
- docs/upstream-prs/03-ascii85-decode/{proposal,design,tasks}.md:
replaced 16-line copy-pasted "Implementation note" blocks with
pointers to canonical openspec/changes/feat-ascii85-decode/
(same pattern as PRs #1/#2/#5).
- spec.md REQ-001 rewritten with verified canonical byte literals
(Hell, Hello, Hello world!); REQ-004 expanded to cover all 6
spec-violation paths.
Verification: all 5 gates green (poc-replace-text,
poc-filter-chain-roundtrip, poc-filter-roundtrip-asciihex,
poc-filter-roundtrip-runlength, poc-filter-roundtrip-ascii85).
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}).
|
|
||
| --- | ||
|
|
||
| > **Implementation note**: canonical design lives in `openspec/changes/feat-runlength-decode/design.md` (D1–D6). The truncation fail-safe ships as `return false` per the chain dispatcher's `=== false` short-circuit contract; this matches upstream `p_error()`'s default return and ensures downstream filters never see a partially-decoded buffer. |
There was a problem hiding this comment.
🟡 Concern — canonical design.md D3 contradicts shipped code
This new pointer note advertises the canonical openspec design as the source of truth, but the canonical's D3 (at openspec/changes/feat-runlength-decode/design.md:36-38) still reads:
D3 — Decode rejects truncated input via
p_error+ return raw
p_error()is called and the raw input is returned unchanged.
That contradicts the shipped code (Thread 1 fix: 3 paths return false) and the updated spec.md REQ-4. The Risks table at openspec/.../design.md:49 also still says "raise p_error and return raw input on underflow".
Suggested fix: rename D3 heading to "Decode rejects truncated input via p_error + return false", rewrite the body to reference the chain dispatcher's === false short-circuit contract (mirroring the PHPDoc at src/PDFObject.php:220-224), and update the matching Risks row.
Non-blocker under Standard mode — but since the new pointer note explicitly directs readers there, the inconsistency is now visible to anyone following the link.
|
|
||
| --- | ||
|
|
||
| > **Implementation note**: canonical design lives in `openspec/changes/feat-runlength-decode/design.md` (D1–D6). The truncation fail-safe ships as `return false` per the chain dispatcher's `=== false` short-circuit contract; this matches upstream `p_error()`'s default return and ensures downstream filters never see a partially-decoded buffer. |
There was a problem hiding this comment.
🟢 Minor — pointer overstates the canonical's decision count
This note says "canonical design lives in openspec/.../design.md (D1–D6)", but the canonical file only defines D1–D4. Same overcount in docs/upstream-prs/02-runlength-decode/proposal.md:24: "D1–D6 decisions, including the truncation fail-safe contract".
Suggested fix: either trim both pointer notes to "(D1–D4)", OR add D5 (chain-failure-propagation contract) and D6 (trust-amplification trade-off) to the canonical design as named decisions. Either is fine.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review verdict: APPROVE (Standard mode)
Blocker fully fixed end-to-end: decoder returns false on truncation (src/PDFObject.php:260,268,278), dispatcher short-circuits at === false (src/PDFObject.php:576-578), spec.md REQ-4 mandates the false-sentinel contract, and the new chain-failure-propagation PoC verifies it. Solid fix.
Prior 12 comments: 8 verified + resolved (1 🔴 + 3 🟡 + 4 🟢), 4 left open as polite follow-ups:
- Thread 3 (🟡, encoder bloat — design trade-off)
- Thread 10 (🟢,
p_errornot asserted in PoC) - Thread 11 (🟢, defensive guard)
- Thread 12 (🟢, partial — happy-path structural assertion)
2 new findings flagged from this re-review (both non-blocking under Standard):
- 🟡 Finding A — canonical
design.mdD3 still says "return raw", contradicts shipped code - 🟢 Finding B — pointer note says "D1–D6" but canonical only defines D1–D4
Finding A is the only one worth tackling before this lands upstream — the new pointer note advertises a canonical that contradicts itself, which is a worse state than before the fix. A small follow-up commit (rename D3 heading + update body + Risks row) would close it cleanly. The rest are fine as follow-ups.
Wilco's re-review on PR #5 flagged spec drift between the canonical openspec design and the shipped code: - D3 in openspec/changes/feat-runlength-decode/design.md still read "return raw input unchanged" while the shipped code (and updated spec.md REQ-4) return `false`. Rewrote D3 to match the chain dispatcher's `=== false` short-circuit contract and `p_error()`'s default return. - Pointer notes in docs/upstream-prs/02-runlength-decode/ referenced "D1–D6" but the canonical only had D1–D4. Added D5 (chain-failure- propagation contract) and D6 (decode-amplification caller-trust contract) to the canonical design — both reflect the as-shipped behaviour with code references. - Risks table updated: removed "return raw input" wording; added fixture-worthy `"ABBC"` example illustrating greedy-encoder bloat. Other items from the re-review: - Happy-path two-filter test now asserts the encoded stream's last byte is `\x80` (outer RLE EOD), mirroring the single-filter structural check. Guards against a dispatcher regression that silently bypasses the outer encoder. - RunLengthEncode PHPDoc adds a TODO(opt) explaining the greedy run-detection threshold trade-off (2 vs. 3 adjacent identical bytes; the `"ABBC"` shape bloats by 1 byte). Verification: all 4 gates green (poc-replace-text, poc-filter-chain-roundtrip, poc-filter-roundtrip-asciihex, poc-filter-roundtrip-runlength).
Blockers: - Only the first match per TJ operator was processed. processTjArray returned on the first hit; outer caller advanced past the entire TJ. Multi-needle TJs and same-needle-twice-in-one-TJ under-counted replacements_per_needle. Rewrote with an outer while-loop that re-resolves fragment texts after each splice and continues until no remaining needle matches. New REQ-006 in spec.md anchors the contract with two-needle and same-needle- twice scenarios. - Missing isset check on $stats['replacements_per_needle'][$needle]++ in processTjArray. Lazy-init kept as a belt-and-braces guard (the entry point already pre-keys via array_fill_keys, but the guard survives future entry-point refactors). - Odd-length hex silently dropped a char (@hex2bin($hex) ?: ''). PDF 1.7 §7.3.4.3 mandates implicit trailing-zero padding. Now pads via `if (strlen($hex) & 1) $hex .= '0'`. New REQ-005 scenario covers this. - Non-spec leniency: numeric tokenizer accepted [-+\d.eE]. PDF 1.7 §7.3.3 forbids exponent notation in Numeric Objects. Tightened to [-+.0-9] so a producer's `e` glyph after a hex CID can't be swallowed as part of a number. New REQ-005 scenario covers this. - Phantom-ticked tasks 5.4 and 5.5 (no fixtures). Reverted to unchecked with explicit deferral notes. Concerns: - Dead $matchedOnce assignment removed via the rewrite. - Comments not treated as whitespace inside TJ arrays. Now skipped per PDF 1.7 §7.2.4. REQ-005 scenario. - tj_arrays_modified diagnostic key now pre-initialised at top of replace_text_in_document (contract no longer fragile). - preg_match-per-byte for whitespace replaced with strpos against the 6-byte PDF whitespace set (~50× faster on large TJ arrays). - Shape inheritance: placeholder now always emitted as literal regardless of first matched fragment's shape (matches D2 documented contract). - `@hex2bin` defensive `@` removed — the odd-length check above makes the warning unreachable; PHP errors for other malformed shapes now surface to callers explicitly. - end() / reset() pointer-advancing avoided in the new render path (group-based emission uses numeric indexing). - Triplicate Implementation Note blocks in docs/upstream-prs/07-tj- flattening/{proposal,design,tasks}.md replaced with canonical pointers (same pattern as PRs #1/#2/#3/#4/#5/#6). - Tf set inside q/Q leaks documented as a known gap (q/Q stack not maintained); deferred to a follow-up. Verification: all 7 gates green (poc-replace-text, poc-tounicode-cmap, poc-tj-flattening, plus the 4 filter PoCs). Also fixed in this commit: poc-tj-flattening.php was calling $doc->replaceTextInDocument() (camelCase) which doesn't exist after PR #1's snake_case rename. Updated to replace_text_in_document.
feat(runlength): /RunLengthDecode filter (SAPP PR #2)
2 participants
Summary
Implements PDF 1.7 §7.4.5 RunLengthDecode as the second of four filter PRs that attach to the chain dispatcher from #3. Orthogonal to ASCIIHexDecode (#4); the two add their own
casearms and don't interact.Stacked on #4 feat/asciihex-decode. This PR's base will auto-retarget to
feat/filter-chain-dispatch→work/text-replacementas the stack rolls forward.What landed
PDFObject,protected static)RunLengthDecode($_stream, $params): string|falseRunLengthEncode($_stream, $params): string\x80EODBoth wired into
apply_filter_chain_decode/apply_filter_chain_encodeviacase 'RunLengthDecode':.Spec contract
Pinned by
openspec/changes/feat-runlength-decode/:p_error+ raw passthrough)/RunLengthDecode(MODIFIED capability from feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation) #3)Spec correction during impl
Original REQ-2 literal-only scenario used
"Hello"— but"Hello"contains"ll", which the greedy encoder correctly detects as a repeat block. Updated the spec scenario to use any 5-byte string with no adjacent duplicates ("abcde"in the test) so the literal-only-block contract is actually verifiable. Caught by the round-trip test failing during implementation.Test plan
php examples/poc-filter-roundtrip-runlength.php— 5 REQs, 0 assertion failuresphp examples/poc-replace-text.php— baseline still greenphp examples/poc-filter-chain-roundtrip.php— dispatcher tests greenphp examples/poc-filter-roundtrip-asciihex.php— sibling filter unaffectedphp -lclean on touched filesFiles changed (436 +, 47 −)
src/PDFObject.php— RunLengthDecode + RunLengthEncode helpers, dispatcher armsexamples/poc-filter-roundtrip-runlength.php— new round-trip gatedocs/upstream-prs/02-runlength-decode/{proposal,design,tasks}.md— implementation notesopenspec/changes/feat-runlength-decode/{tasks.md,specs/.../spec.md}— 22/22 complete + literal-only scenario fixDependencies + ordering
casearm; orthogonal codecs)Out of scope
/DecodeParmsfor RunLengthDecode — none defined per PDF 1.7 Table 5