|
| 1 | +# Refactor Roadmap |
| 2 | + |
| 3 | +This document sequences the design-debt work tracked in issues #23–#30 against the |
| 4 | +existing hardening, format, and performance issues. LAC is experimental, but these |
| 5 | +changes are correctness work: the lossless round-trip |
| 6 | +(`supported WAV -> encode -> .lac -> decode -> restored WAV`) must hold at every |
| 7 | +step, not only at the end. |
| 8 | + |
| 9 | +The ordering is dependency-driven, not severity-driven. Several high-value items |
| 10 | +(#23, #24, #26) carry the highest risk of silently breaking losslessness, so they |
| 11 | +are deliberately placed *after* the test-strengthening and undefined-behavior work |
| 12 | +that protects them. |
| 13 | + |
| 14 | +## Guiding Principles |
| 15 | + |
| 16 | +- The bit-perfect round-trip and the malformed-input rejection behavior are |
| 17 | + invariants. No step may regress either; each step lands with the round-trip and |
| 18 | + malformed-input tests green. |
| 19 | +- Strengthen the test safety net before risky de-duplication. |
| 20 | +- Establish a single source of truth for a contract before restructuring the code |
| 21 | + that depends on it. |
| 22 | +- Group changes that touch the same files (the codec arithmetic hot paths) so the |
| 23 | + same code is not rewritten twice. |
| 24 | +- Prefer no observable change to emitted/accepted bytes during pure refactors; |
| 25 | + protect that with fixtures that must still round-trip bit-for-bit. |
| 26 | + |
| 27 | +## Sequenced Plan |
| 28 | + |
| 29 | +### Phase 1 — Foundation and test safety net |
| 30 | + |
| 31 | +Low-risk work that removes noise and makes everything afterwards easier to test. |
| 32 | + |
| 33 | +1. **#30 — Remove dead code and empty placeholders; move `selftest` into the test |
| 34 | + suite.** Deleting `Rice::compute_k`, the unused `Block::Encoder` order member, |
| 35 | + and the empty placeholder files reduces confusion before refactoring. Moving |
| 36 | + `lac_cli selftest` into the test suite and extending it to cover per-block |
| 37 | + stereo (mode 2) and mono strengthens the regression net used by every later |
| 38 | + phase. |
| 39 | +2. **#27 — Remove hidden environment coupling from the library; unify |
| 40 | + configuration.** Once the codec no longer reads `LAC_THREADS` / `LAC_DEBUG_*` |
| 41 | + from inside `encode`/`decode`, encoder and decoder become deterministic to |
| 42 | + drive from tests, which the later phases depend on. |
| 43 | + |
| 44 | +Gate: round-trip, predictor/residual, partitioning, and zero-run tests all pass; |
| 45 | +`selftest` coverage now includes mode 2 and mono. |
| 46 | + |
| 47 | +### Phase 2 — Correct and unify the core arithmetic |
| 48 | + |
| 49 | +These items all touch the predictor / Rice / LPC hot paths, so they are done as one |
| 50 | +cluster to avoid rewriting the same code repeatedly. |
| 51 | + |
| 52 | +3. **#8 (existing) — Eliminate undefined / implementation-defined arithmetic.** |
| 53 | + Fix the UB and unchecked narrowing first, so the code that is about to be |
| 54 | + unified is already correct. |
| 55 | +4. **#23 — Define a single source of truth for the format contract.** Extract the |
| 56 | + predictor formulas, zigzag mapping, stateless `adapt_k`, partition-size math, |
| 57 | + the Q15 shift/scale, and the wire-format tag/mode constants into shared |
| 58 | + definitions consumed by encoder, decoder, and SIMD. This is the foundation for |
| 59 | + #24, #25, and #26. |
| 60 | +5. **#26 — Guarantee SIMD/scalar bit-exactness; unify NEON LPC range-safety.** |
| 61 | + With the arithmetic unified (#23) and UB removed (#8), collapse the three |
| 62 | + residual engines onto one shared kernel, add a test asserting NEON output is |
| 63 | + byte-identical to the scalar reference, and move range-checking into the path |
| 64 | + that writes residuals. |
| 65 | + |
| 66 | +Gate: a SIMD-vs-scalar equality test passes on representative and near-overflow |
| 67 | +inputs; sanitizer builds are clean on the codec hot paths. |
| 68 | + |
| 69 | +### Phase 3 — Consolidate the decode path |
| 70 | + |
| 71 | +4. **#24 — Consolidate the duplicated v3 decode path, validation helpers, resource |
| 72 | + limits, and WAV writer.** Done together with **#3 (bound decoder allocations)** |
| 73 | + and **#7 (reject non-canonical metadata)**, since all three touch the same |
| 74 | + parser surface. This collapses the second attacker-facing parser in |
| 75 | + `main.cpp` onto the shared one and reuses the validators unified in #23. |
| 76 | +5. **#28 — Adopt one error-handling strategy across encode, decode, I/O, and |
| 77 | + CLI.** Apply it as the decode path is consolidated, so the convention is |
| 78 | + settled in one place and aligns with #7's decode-time rejection requirements. |
| 79 | + |
| 80 | +Gate: a single v3 block-table parser is exercised by both the CLI fast path and |
| 81 | +`LAC::Decoder`; malformed-input tests assert on specific rejection reasons; the |
| 82 | +resource-limit constants exist in exactly one place. |
| 83 | + |
| 84 | +### Phase 4 — Structure and platform |
| 85 | + |
| 86 | +6. **#25 — Decompose the monolithic block/frame encode functions.** With the |
| 87 | + formulas extracted in #23, `Block::Encoder::encode` shrinks; separating |
| 88 | + plan/select/serialize makes the emit routines unit-testable and unblocks |
| 89 | + **#15 (reduce peak memory and repeated analysis)**. |
| 90 | +7. **#29 — Address the x86 SIMD gap, misleading `neon_available()`, and unguarded |
| 91 | + endianness.** Platform expansion comes last. The compile-time endianness |
| 92 | + `static_assert` is a small, safe change that may be pulled forward at any time. |
| 93 | + |
| 94 | +Gate: encode decomposition lands with no change to emitted bytes; platform |
| 95 | +behavior (SIMD availability, supported endianness) is documented. |
| 96 | + |
| 97 | +## Dependency Summary |
| 98 | + |
| 99 | +- #30, #27 → enable reliable, deterministic testing for everything after. |
| 100 | +- #8 → precedes #23/#26 (fix before restructure). |
| 101 | +- #23 → unblocks #24 (shared validators), #25 (smaller encode), #26 (shared |
| 102 | + kernel). |
| 103 | +- #24 → precedes/co-lands with #28 (settle the error convention on the |
| 104 | + consolidated path); pairs with #3 and #7. |
| 105 | +- #25 → unblocks #15. |
| 106 | +- #29 → independent; last. |
| 107 | + |
| 108 | +## Cross-Cutting Work |
| 109 | + |
| 110 | +- **#5 (multithreaded encoder startup exception-safety)** is an independent, small |
| 111 | + hardening item that can be slotted in at any time; it pairs naturally with the |
| 112 | + worker-pool unification in #24. |
| 113 | +- **#11 (malformed-input regression tests)** and **#12 (fuzzing harnesses)** are |
| 114 | + ongoing. Landing them before or alongside Phase 3 increases confidence in the |
| 115 | + decode-path consolidation. |
| 116 | + |
| 117 | +## Status |
| 118 | + |
| 119 | +This is a living plan. Update it when an issue is closed, reordered, or when a |
| 120 | +dependency assumption changes. Per `maintainer-workflow.md`, verified work should |
| 121 | +still leave normal artifacts behind (issues, PRs with correctness/security notes, |
| 122 | +CI runs, regression tests). |
0 commit comments