diff --git a/README.md b/README.md index 24b744e..bd2e6c1 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,9 @@ tools/analysis/ measurement scripts, numbered in the order they were written (01/02 marked BROKEN deliberately, kept as regression refs). Run from the repo root — they import from tools/encoder/. 07 finds the hottest sustained window in a stream; 08 renders - source | decoded | block-mode map as .webm. + source | decoded | block-mode map as .webm; 09 is a regression + test for the rate-control desync (FINDINGS 26) and exits + non-zero until it is fixed. tools/bench/ MAME Lua injection harness + 68000 benchmark sources. `check.sh` re-runs both display regression tests (~40 s). `blit.s`/`blit.lua` time the full-frame GVRAM blit on the diff --git a/docs/FINDINGS.md b/docs/FINDINGS.md index 5bdd4d1..8622550 100644 --- a/docs/FINDINGS.md +++ b/docs/FINDINGS.md @@ -1003,3 +1003,73 @@ One 10 s window of one stream, at fixed lam, with `_paint` still a Python loop. The full-disc survey is still not done, and the numbers above are the *worst* window rather than a distribution over content. What has changed is that the worst case is now a measurement rather than a worry. + +--- + +## 26. Rate control is unsound as written — found before wiring it up (session 5) + +FINDINGS 25.3 promoted rate control from insurance to a requirement. Reading +`ratectl.py` before wiring it into `encode.py` turned up a correctness bug that +would have produced exactly the kind of plausible-looking wrong result this +project keeps catching (FINDINGS 4, 9, 14, 18). + +### 26.1 The lam ladder desynchronises the encoder from the decoder +`H.encode()` is **temporally recursive**: SKIP blocks are copied from the +previous *reconstruction*, and `prev = out` closes the loop +(`vq_hybrid.py:84-109`). A frame's output therefore depends on every frame +before it in that same run. + +`encode_rate_controlled()` runs `H.encode()` once per lam over the **whole +sequence**, building a ladder of independent temporal chains, then picks each +frame from whichever rung fits the budget. When frame *f* comes from rung *i* +and frame *f-1* was emitted from rung *j != i*, the SKIP blocks in *f* reference +a reconstruction **the decoder never saw**. + +Measured on the Singe window (`tools/analysis/09_ratectl_drift.py`, 120 frames, +5 rungs, target 110 KB/s): + +| | | +|---|---| +| rung switches | **67** over 120 frames | +| frames whose emitted output differs from what the encoder recorded | **111 / 120** | +| worst frame | **21,339 px = 43.4% of the frame** | +| encoder-vs-decoder agreement, worst frame | 27.1 dB | +| reported PSNR overstatement | **0.36 dB** | + +The 0.36 dB is the least interesting number here. The encoder is reporting +quality for a reconstruction that will never exist, and 43% of a frame differing +is a visible artefact whatever the mean says. + +**The fix is structural, not a tuning change:** `H.encode()` must become +frame-drivable — take `prev` and one lam, return one frame — so rate control can +feed back the frame it actually emitted. The current whole-sequence signature is +what makes the ladder tempting in the first place. + +### 26.2 The ladder spans 250x past the shippable range +`lam_hi=2e5`, but FINDINGS 15 puts the quality cliff between lam=800 and +lam=2000 and says do not ship past lam~800. Every rung above ~800 is +unshippable, so a frame that only fits at lam=9457 has not been rate-controlled, +it has been destroyed. Cap `lam_hi` at 800 and let a frame that cannot fit +overrun the bucket — a visible overrun is a better failure than silent garbage. + +### 26.3 The ladder is far too coarse where it matters +With `steps=5` the geomspace lands on 1 / 21 / 447 / 9457 / 200000, and **only +two rungs were ever chosen**. The budget is 8,721 B/frame; the two straddling +rungs deliver 23,183 B (lam=21) and 3,071 B (lam=447) — a **7.5x** gap across +the operating point. Rate control cannot land near a target it has to jump over. + +The module docstring already describes the right approach — *"per frame we +binary-search lam to land inside a byte budget"* — but the implementation is a +fixed precomputed ladder. Doc and code disagree; the doc is correct. + +### 26.4 What does work +The leaky bucket lands the mean where it should: **109.1 KB/s against a 110 +target**, with 32% of frames over the per-frame budget and banked by the bucket. +That mechanism is sound and worth keeping. It is the per-frame lam *selection* +underneath it that needs rebuilding, not the bucket. + +### 26.5 Cost note before starting +Each rung is a full-sequence encode and `_paint` is still a Python per-block +loop, so a 5-rung run over 120 frames takes minutes. **Vectorise `_paint` +first** — it is already on the list for the full-disc survey and it makes the +rate-control work practical rather than merely faster. diff --git a/docs/STATUS.md b/docs/STATUS.md index 0b40fa4..6eef3b2 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -1,5 +1,44 @@ # Status & next-session handoff — end of session 5 (2026-08-23) +## NEXT SESSION: wire rate control into `encode.py` + +Decided with the user at the end of session 5. Everything needed is below; read +**FINDINGS 26** in full before editing `ratectl.py`, because the module does not +work the way its docstring says it does. + +**Why it is now top of the list.** FINDINGS 25.3: on the worst sustained window +on the disc, the fixed-`lam` CLI overshoots both shipping targets — `sasi` +110 -> 129.6 KB/s (+18%), `scsi` 280 -> 373.8 KB/s (+34%). This item sat at +priority 4 marked "insurance, not a fix" for three sessions; that was true of the +1.2-1.7 s clips it was judged on and is not true of a sustained action sequence. + +**Do NOT just call `encode_rate_controlled()` from `encode.py`.** It is unsound +as written (FINDINGS 26.1), and it fails quietly — it returns a plausible PSNR +for a reconstruction no decoder will ever produce. + +Order of work: + +1. **Vectorise `_paint`** (`vq_hybrid.py:122`, a Python per-block loop). Not + cosmetic: rate control runs the encoder once per lam rung, so everything + below is minutes-per-experiment until this is done. FINDINGS 26.5. +2. **Make `H.encode()` frame-drivable** — take `prev` and one lam, return one + frame. The current whole-sequence signature is *why* the broken ladder + exists. This is the actual fix for 26.1. +3. **Replace the fixed ladder with the per-frame binary search** the docstring + already promises, feeding back the frame actually emitted. Cap `lam_hi` at + **800**, not 2e5 — past the FINDINGS 15 cliff a frame is not rate-controlled, + it is destroyed (26.2). Keep the leaky bucket; it works (26.4). +4. **Gate on the regression test**: `python3 tools/analysis/09_ratectl_drift.py` + exits non-zero while the desync is present and must report **zero** drifting + frames after the fix. It currently reports 111/120. Needs `tmp/fr_singe`. +5. Then re-measure the Singe window at both profiles and confirm they land on + target rather than 18%/34% over. + +Only after that is the full-disc survey worth running — otherwise it measures an +encoder nobody will ship. + +--- + ## Start here: is the tree still green? ``` @@ -112,7 +151,12 @@ rate-distortion curve, not two codecs. 00216 is the feature with a burned-in commentary PiP; 00215 is the commentary itself. **00223 (9.4 min) is the clean one.** A size-ranked survey would have encoded live action. FINDINGS 25.1. -7. **On hard content the scene palette, not the display, is the binding +7. **Rate control is unsound as written, caught before wiring it up.** The + lam-ladder in `ratectl.py` picks frames from independent temporal chains, + so SKIP blocks reference reconstructions the decoder never saw: 111 of 120 + frames drift, worst frame 43.4%, reported PSNR overstated 0.36 dB. Regression + test `tools/analysis/09_ratectl_drift.py`. FINDINGS 26. +8. **On hard content the scene palette, not the display, is the binding ceiling** — 31.33 dB on the Singe window against 39.90 dB on 00020 and 40.81 dB for the X68000 display. `scsi` is already within 0.51 dB of it. FINDINGS 25.4. @@ -218,6 +262,10 @@ notifier subscription in a global; the stack register is `SP` not `A7`; - piping MAME (or any long job) through `grep` block-buffers — write to a file. - `pkill -f ` matches your own shell and kills it (exit 144). Use `pkill -x` or kill by PID. +- **`pgrep -f | xargs kill` kills your own shell too — exit 144.** Same + root cause as the `pkill -f` trap above: the shell's own command line contains + the pattern. **Hit again in session 5**, which makes it four times across three + sessions. Kill by PID captured at launch (`$!`), or use `pkill -x`. - **`until ! pgrep -f foo.py; do sleep; done` watcher loops never exit.** The watching shell's own command line contains the string `foo.py`, so `pgrep -f` matches the watcher itself and the loop spins forever. Session 2 left 11 of @@ -323,14 +371,10 @@ SDL_VIDEODRIVER=dummy mame x68000 -bios ipl10 -video soft -window \ scene-cut frame is ~100% non-SKIP and a held frame near 0%, and the mean of those two is a number describing no actual frame. -1b. **Wire rate control into `encode.py`. NOW REQUIRED (was priority 4).** - FINDINGS 25.3: on the worst sustained window both profiles overshoot their - targets with the fixed `lam` the CLI uses — `sasi` by 18%, `scsi` by 34%. - The note that used to sit here, "no longer a blocker (FINDINGS 21)", was true - of the 1.2-1.7 s clips measured at the time and is not true of this one. - `ratectl.encode_rate_controlled()` already builds a per-frame lam ladder; it - has never been hooked up. Do this before the full-disc survey or the survey - measures an encoder nobody will ship. +1b. **Wire rate control into `encode.py`. NOW REQUIRED, and it is the agreed + next session's work** — see the **NEXT SESSION** block at the top of this + file for the ordered plan, and FINDINGS 26 for why + `encode_rate_controlled()` cannot simply be called as it stands. 2. **68000 decoder skeleton**, with the inner loop chosen by (1). Parse `DLX1`, expand codebooks, blit per block mode. The display path is verified *by 68000 diff --git a/tools/analysis/09_ratectl_drift.py b/tools/analysis/09_ratectl_drift.py new file mode 100644 index 0000000..fb23e78 --- /dev/null +++ b/tools/analysis/09_ratectl_drift.py @@ -0,0 +1,70 @@ +#!/usr/bin/env python3 +"""REGRESSION TEST for the ratectl lam-ladder desync (FINDINGS 26). + +Exits non-zero while the bug is present. After the fix it must report ZERO +drifting frames -- that is the acceptance criterion for wiring rate control +into encode.py. + +encode_rate_controlled() runs H.encode() once per lam over the WHOLE sequence, +then picks each frame from whichever rung fits the budget. But H.encode() is +temporally recursive: a frame's SKIP blocks are copied from the PREVIOUS +RECONSTRUCTION of that same rung. If frame f is taken from rung i while frame +f-1 was emitted from rung j != i, the SKIP blocks in f reference a frame the +decoder never saw. + +This replays what a real decoder does -- SKIP copies the ACTUALLY EMITTED +previous frame -- and compares it to the reconstruction ratectl recorded. + +Needs tmp/fr_singe (see docs/STATUS.md, reproducing the sustained-action +result). Takes a few minutes: it runs `steps` full-sequence encodes and +_paint is still a Python per-block loop. +""" +import sys, os +sys.path.insert(0, "tools/encoder") +import numpy as np +import vq as VQ, vq_hybrid as H, ratectl as RC + +m = H.build("tmp/fr_singe", k1=256, k4=256, iters=16) +enc = RC.encode_rate_controlled(m, target_kbps=110, steps=5, verbose=True) + +lam = enc["lam"] +sw = int((np.diff(lam) != 0).sum()) +print(f"\nframes={len(lam)} distinct lam used={len(set(lam.tolist()))} " + f"rung switches={sw}") + +pal, nbx = m["pal"], m["W"] // 4 +emitted = [] +drift_px, drift_db = [], [] +for f, (rec, mode) in enumerate(zip(enc["recon"], enc["modes"])): + out = rec.copy() + if f > 0: + prev_true = emitted[-1] + for b in np.flatnonzero(mode == 0): # SKIP blocks + by, bx = divmod(int(b), nbx) + y, x = by*4, bx*4 + out[y:y+4, x:x+4] = prev_true[y:y+4, x:x+4] + emitted.append(out) + d = (out != rec).sum() + drift_px.append(d) + drift_db.append(VQ.psnr(pal[rec], pal[out])) + +drift_px = np.array(drift_px) +print(f"pixels differing from what the encoder recorded:") +print(f" frames with ANY drift: {int((drift_px>0).sum())}/{len(drift_px)}") +print(f" max {drift_px.max()} px ({100*drift_px.max()/(m['H']*m['W']):.1f}% of frame)") +print(f" mean {drift_px.mean():.0f} px") +fin = [d for d in drift_db if np.isfinite(d)] +if fin: + print(f" encoder-vs-decoder agreement: min {min(fin):.1f} dB " + f"(inf = identical on {len(drift_db)-len(fin)} frames)") + +r = RC.summarise(m, enc, 110) +print(f"\nratectl reports PSNR {r['psnr']:.2f} dB, {r['kbps']:.1f} KB/s " + f"(target 110), {r['over']:.0f}% of frames over budget") +tp = np.mean([VQ.psnr(o, pal[e]) for o, e in zip(m["rgb"], emitted)]) +print(f"what a decoder actually reconstructs: {tp:.2f} dB " + f"-> overstated by {r['psnr']-tp:.2f} dB") + +# Acceptance criterion for the fix: a decoder replaying the emitted stream must +# reconstruct exactly what the encoder recorded. +sys.exit(1 if (drift_px > 0).any() else 0) diff --git a/tools/encoder/ratectl.py b/tools/encoder/ratectl.py index 0bf8b37..c99cb79 100644 --- a/tools/encoder/ratectl.py +++ b/tools/encoder/ratectl.py @@ -11,6 +11,12 @@ without that, quiet frames waste budget and action frames stay ugly. The ceiling is HARD: the 68000 streams at a fixed rate off the disk, and a frame that overruns is a dropped frame, not a slow frame. + +STATUS, session 5: this module is written but STILL NOT WIRED INTO encode.py, +and FINDINGS 25.3 measured both profiles overshooting their targets by 18% and +34% on the worst sustained window because of that. Before wiring it up, read +the correctness note on encode_rate_controlled() -- the lam-ladder approach it +uses is not sound against a temporally recursive encoder. """ import numpy as np import vq_hybrid as H @@ -28,9 +34,16 @@ import vq_hybrid as H # What actually bounds the high end: # - Bus: unmeasured. ~300-500 KB/s SASI / ~1 MB/s SCSI, both FOLKLORE. # This is the binding unknown and the reason the disk benchmark matters. -# - CPU: a FULL-frame blit is only 38% of the 12fps budget, and VQ decode is -# table copies (RAW, the mode that dominates at high rate, is the CHEAPEST -# to blit). So raising the bitrate is nearly free on CPU. +# - CPU: a full-frame blit is **53.6%** of the 12fps budget -- MEASURED on the +# emulated 68000, session 5, FINDINGS 24. This line previously said 38%, +# which was an estimate and was wrong by 41%. And 53.6% is a floor: MAME +# models no GVRAM wait states, so real hardware is worse. +# "Raising the bitrate is nearly free on CPU" survives but is now much +# tighter. It rests on RAW being the cheapest mode to blit, which is still +# true, but the display path alone now eats over half the frame before any +# decoding happens. The per-frame path choice of FINDINGS 25.6 (blit vs +# direct-to-GVRAM, whichever is cheaper for that frame) brings the median +# back to ~37% and caps the worst case at 53.6%. # - Entropy coding is NOT the way to buy headroom here: deflate decode is # ~216% of the frame budget on a 68000 and even LZ4 is ~54%. See FINDINGS 17. # The rates below are therefore RAW payload, no entropy coding. @@ -39,15 +52,18 @@ import vq_hybrid as H PROFILES = { "sasi": dict(kbps=110, lam=60.0, k1=256, k4=256, desc="stock 10MHz ACE/EXPERT, SASI", - quality="36.9 dB on 00020 / 29.6 dB on 00146", + quality="36.9 dB on 00020 / 29.6 dB on 00146 / 27.8 dB on the " + "Singe window, where it overshoots to 129.6 KB/s", util="~105 KB/s = 35% of the pessimistic 300 KB/s SASI figure"), "scsi": dict(kbps=280, lam=10.0, k1=256, k4=256, desc="Super/XVI, or CZ-6BS1 board in a 10MHz machine", - quality="39.4 dB on 00020 / 32.3 dB on 00146", + quality="39.4 dB on 00020 / 32.3 dB on 00146 / 30.8 dB on the " + "Singe window, where it overshoots to 373.8 KB/s", util="~275 KB/s = 28% of the 1 MB/s SCSI folklore figure"), } # lam=0 is PIXEL-EXACT against the palettised frame (0.00 dB loss) at ~450 KB/s -# of raw payload, and costs only 38% of the CPU budget. If the blocked disk +# of raw payload, and costs 53.6% of the CPU budget (not the 38% written here +# before session 5 -- FINDINGS 24). If the blocked disk # benchmark confirms SCSI sustains >=800 KB/s, the `scsi` profile should become # lam=0 and the port ships transparent video. That decision is waiting on a # measurement, not on a design choice.