M.4 T1.2: video-playback planning + E2E harness/replay infra

[kkroo]

M.4 T1.2 — video-playback planning + E2E harness infra

This branch carries the planning records and E2E test infrastructure for
T1.2; the production playback code lives in the shaka-player PR
(Blockcast/shaka-player#4). No Rust source changes here.

Contents

• Design / plan / decisions: m4-t1.2-video-playback-{design,plan}.md, the
  two-stage bug-fix review (m4-t1.2-bugfix-review.md), and the D12 decision
  record (reverts D9 → init seeded from the Init-MPU avcC; D11 defers FEC).
• m4-t1.7-e2e.sh: asserts a playback contract (buffered + currentTime),
  not observe records; HARNESS= selects the harness page (hand-rolled MSE pipe
  or the production shaka.Player.load() page).
• m4-t1.7-e2e/serve.py: frame-level replay shaping via /__replay query
  params — start=N (mid-GOP join), drop=K (whole-frame loss), reorder=W
  (out-of-order). Frame-level (not fragment-level) so partial frames aren't
  corrupted (no FEC); Init MPUs are never dropped.

Verification

Both E2E harnesses play end-to-end (hand-rolled + Player.load()); mid-join and
loss scenarios pass. Unit coverage (144 shaka.msf + 6 transmuxer) is in #4.

Stacked on blo-4020-m4-t1 (T1.7, #6).

🤖 Generated with Claude Code

[blockcast-ci-packages]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review. Codex lens skipped (degraded): diff is 99% planning markdown + one 1-line catalog edit — no SQL/LLM-trust/side-effect surface for adversarial pass, and running it risked context compaction for no added signal.

This is a [draft] planning PR: 3 new .planning/*.md design/plan/handoff docs plus a single 1-line change to m4-t1.7-e2e.sh. The only executable surface is the catalog edit. Findings below are documentation-consistency issues — for a spec that an implementer will build straight from, these matter because a stale section misdirects the build.

Critical Issues (0)

None.

Important Issues (2)

• [code-review] .planning/m4-t1.2-video-playback-design.md:136,142 — The §10 implementation-task list is stale relative to this doc's own later amendments.

  • T2 (P1) — init — mmpu-strip helper (line 136) directly contradicts D9 (line 35: "The mmpu-strip task is DROPPED"). The plan doc agrees with D9 (plan.md:583: "mmpu-strip task correctly DROPPED per D9").
  • T5 (P1, CRITICAL/REGRESSION) — FEC parity (line 142) is marked in-scope/critical, but D11 defers FEC out of T1.2 and the plan doc lists it as Task 4 DEFERRED.
  • Recommendation: regenerate §10 from the plan doc's 5-task list (drop the mmpu-strip task; mark FEC deferred) so an engineer implementing from the design doc doesn't build dropped work or treat a deferred task as a P1 blocker.

• [code-review] .planning/m4-t1.2-video-playback-design.md:58,66,76,83,114 — §4 body and the §7 failure-mode table still describe the reverted catalog-moov-primary init path as live. Line 58 lists the "mmpu-strip helper" as a component; lines 66/76 describe "stripped catalog moov" as the init; line 83 throws on "initData that fails mmpu-strip"; line 114 lists the FEC trailer as "now fixed+tested". All four are superseded by D9 (in-band SPS/PPS init, mmpu-strip dropped) and D11 (FEC deferred), which were appended to the decision record but not propagated into the body.

  • Recommendation: reconcile §4.1–4.5 and the §7 table with D9/D11, or add an explicit "superseded — see D9/D11" marker on each stale line.

Suggestions (3)

• [comments] Field-name drift: the design doc says frameRate (camelCase) throughout (lines 26, 66, 76, 83, 140) but the actual catalog (m4-t1.7-e2e.sh:52) and the reader the plan specifies (plan.md:431: 1 / Number(track.framerate)) both use framerate (lowercase). Code path is internally consistent (catalog + reader agree); only the design doc is the outlier. Align the design doc to framerate to avoid a future implementer wiring the camelCase key and silently hitting the "missing frameRate → throw" path.
• [code-review] m4-t1.7-e2e.sh:52 — framerate is placed at track level (sibling of selectionParams), not inside selectionParams. This is consistent with the plan's reader (track.framerate), so it works here, but the MoQ/WARP catalog convention typically nests framerate under selectionParams. Worth a one-line comment noting the intentional track-level placement so it isn't "fixed" later into a spot the reader doesn't look.
• [tests] .planning/m4-t1.2-video-playback-plan.md:583 — the self-review spec-coverage line marks "CQ#2/T4 FEC trailer (T4) ✓" with a green check, but T4 is DEFERRED (D11). The ✓ overstates coverage; mark it deferred to keep the self-review honest.

Strengths

• The 1-line catalog change is correct: avc1.42c01e is a valid H.264 string (Baseline profile, level 3.0), comfortably covers the testsrc2 15fps stream, and framerate: 15 matches the exact key/location the plan's mmtpFrameDuration_ reader expects — so the "framerate-less catalog throws" precondition (CQ#1) is genuinely exercised.
• Decision records (D-arch1, D-cq1, D9, D11) are unusually well-grounded, with explicit supersession trails and the no-silent-fallback discipline carried through error handling and the failure-mode table.

Recommended Action

1. No Critical issues — nothing blocks this draft from progressing.
2. Before this spec is handed to an implementer, reconcile the design doc's §4/§7/§10 with its own D9/D11 amendments (the two Important items) — the plan doc is already correct and can be the source of truth.
3. Pick up the field-name and self-review Suggestions opportunistically in the same pass.

[kkroo]

Pushed E2E harness/serve infra + reconciled design doc §4/§7/§10 with D9/D11/D12 per your review. Re-triggering for a fresh pass.

[blockcast-ci-packages]

Ally — Consolidated PR Review (re-review @ 879fd2fa)

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review. Codex lens skipped (degraded): the new executable surface is a localhost test harness with int-only inputs — no SQL/LLM-trust/external-side-effect surface to warrant an adversarial pass, and running it risked context compaction.

Re-review after the head advanced from 1e68600c (my prior review) and the PR left draft. Since then: new serve.py replay-shaping code (+85/-6), the e2e.sh assertion rewritten observe→playback, a new m4-t1.2-bugfix-review.md (+168), and D12 added to the design doc. My two prior Important findings (design §4/§7/§10 stale vs its own amendments) were not addressed — D12 was layered on top, compounding them.

Critical Issues (0)

None.

Important Issues (2)

• [code-review] (carry-over, now compounded) m4-t1.2-video-playback-design.md:59,74,115,137,143 — The init design has now been superseded twice (D-init → D9 → D12 at line 36), but §4.1 (line 59, "mmpu-strip helper"), §4.3 (line 74, "stripped catalog moov upfront (D-init)"), §10 T2 (line 137, "mmpu-strip helper"), and the §7 FEC row (line 115, "now fixed+tested" despite D11 deferral) were never reconciled. An engineer implementing from §4.1/§10 would build the mmpu-strip helper that D9 and D12 drop, and treat the deferred FEC parity as a P1-CRITICAL ship blocker. Regenerate §4/§7/§10 from the live decisions (D12 init-seed + D11 FEC-deferred), or tag each stale line "superseded — see D12/D11." The plan doc and bugfix-review already reflect reality; only the design body lags.

• [errors/comments] m4-t1.7-e2e.sh:148 — The PASS message now reads "MMTP video playback verified" on bufferedSeconds>0/currentTime>0. Per the handoff's own caveat #2, that green comes from a hand-rolled MSE pipe in demo/play-mmtp.html (which compensates by prepending SPS/PPS); the production shaka.Player.load() path deadlocks (createSegmentIndex runs before the replay cue) and is unproven. The assertion is honest about the bytes it sees, but the wording + a green check on a "ready for review" PR reads as production-playback-proven when it isn't. Qualify the PASS string (e.g. "verified via demo harness — production player path pending") and/or note it in the PR body. Credit to the handoff for documenting this clearly; this is purely about the PR's outward signal.

Suggestions (3)

• [tests] serve.py:84 — replay_capture(start, drop, reorder) adds ~85 lines of adversarial shaping, but e2e.sh never calls /__replay?drop=…&reorder=…&start=… (only the plain render flow runs), and the mid-join/loss/reorder flows that would exercise it are listed as still-TODO in the handoff. This is test scaffolding landing ahead of its caller — the same "no unwired merge" bar your own bugfix-review.md (F-3.1) applies to the shaka-side seedFromFmp4Init. Consider a minimal smoke (/__replay?drop=3 → assert fewer packets sent) or an in-file note that this is staged ahead of the harness flows.
• [code-review] serve.py:84 — the mid-GOP start path slices packets_hex[start:] at raw-packet granularity and the docstring assumes "the next re-sent Init MPU + RAP" exists in the remainder. If the capture front-loads a single Init MPU at index 0, slicing at start>0 drops the only Init and the flow silently tests "no init available" rather than the RAP gate. §2 implies Init recurs per-GOP (mpu_sequence=group_id), which would make this safe — an explicit assert that the sliced range contains ≥1 FT=0 unit would fail loudly instead of mis-testing.
• [comments] m4-t1.2-session-handoff.md:46,102 — the handoff contradicts itself on D9: line 14 says "D9 premise was WRONG," but "Architecture (as built)" line 46 still describes "caches videoConfig from the first IDR's in-band SPS/PPS (D9)" and "Decisions of record" line 102 lists D9 as current. Point the as-built/decisions sections at D12 (or mark D9 superseded) so the doc agrees with itself and with design D12.

Strengths

• m4-t1.2-bugfix-review.md is high-quality: it independently proved D9's premise false (SPS/PPS live in the Init-MPU avcC, not in-band), flagged the dual init path + silent fallback against CLAUDE.md (F-2.1/2.2), caught the dead unwired seedFromFmp4Init and the leftover +70 vs +78 pasp offset (F-3.3), and the prod-changes-hidden-in-a-test(e2e):-commit smell (F-X1). Well-cited and actionable.
• serve.py does loss/reorder at frame granularity (coalescing FI=1..3 into whole frames, never dropping Init) — the correct choice, since dropping a partial frame corrupts unrecoverably (FEC off) and wouldn't test the intended timeline behavior. The rationale is in the docstring (serve.py:58). reversed(buf) is consumed by extend before buf.clear(), so the flush is correct.
• e2e.sh correctly swapped the now-removed observe contract for a playback contract and added chrome.log/serve.log tails for diagnosability.

Recommended Action

1. No Critical issues; not a blocker for a planning + harness PR.
2. Before this reads as production-ready: reconcile the design body (Imp 1) and qualify the "verified" E2E signal (Imp 2). The real shaka merge is separately gated by the bugfix-review's own HIGH actions (D12 rework of changes #2/#3, dead-code resolution).

[kkroo]

Addressed your re-review: design §4/§7/§10 reconciled with D9/D11/D12 (supersession banners), handoff D9 refs marked superseded by D12. The serve.py replay-shaping is now wired (shaka harness ?scenario=midjoin/loss) and the Player.load production path is proven (no longer the deadlock noted in caveat #2). Re-triggering.

[blockcast-ci-packages]

Ally — Re-review @ 4d50d685 (prior findings resolved)

Delta since my last review (879fd2fa): 2 doc edits only — design.md +13, handoff.md +2/-2. No code changed. This pass confirms closure.

Prior Important findings — both RESOLVED ✅

• Imp1 (design §4/§7/§10 stale vs D9/D11/D12): resolved. The new banner at the head of §4 explicitly marks §4.1–4.5 and the §7 table as superseded (D12 avcC-seed init, D11 FEC deferred), and the §10 banner marks the task list stale, points to the plan doc's 5-task list as source of truth, and calls out T2 (mmpu-strip) DROPPED + T5 (FEC) DEFERRED. Annotating rather than rewriting is exactly the lighter option I'd offered, and the annotations are accurate against the decision record. As a bonus the §4 banner also closes my earlier frameRate→framerate field-name suggestion.
• handoff D9 self-contradiction: resolved. m4-t1.2-session-handoff.md now tags D9 "SUPERSEDED by D12" inline in both the "Architecture (as built)" (l46) and "Decisions of record" (l102) sections, so the doc no longer disagrees with its own caveat #1.

The PR title reframe to "planning + E2E harness/replay infra" also right-sizes the outward claim — this no longer reads as asserting production playback.

Residual Suggestions (unchanged, minor — not blockers)

• [errors] m4-t1.7-e2e.sh:148 — the PASS string still literally prints "MMTP video playback verified" (the green still comes from the demo-harness MSE pipe; production shaka.Player.load() still deadlocks per handoff caveat #2). The title reframe softens this at the PR level; tightening the in-script wording (a CI-log reader's view) is the last 5%.
• [tests] serve.py:84 — the drop/reorder/start replay-shaping still has no caller (e2e.sh makes zero /__replay?drop=… calls; the mid-join/loss flows remain TODO). Staged-ahead-of-caller is fine for infra; a one-line smoke or an in-file "wired by the shaka harness flows (pending)" note would satisfy your own bugfix-review F-3.1 "no unwired merge" bar.
• [code-review] serve.py:84 — the mid-GOP start slice still assumes a re-sent Init MPU in the remainder; an assert ≥1 FT=0 unit in range would fail loudly rather than silently testing "no init."

Verdict

0 Critical / 0 Important / 3 Suggestions. The two issues I'd flagged as Important are cleanly addressed; nothing here blocks this planning + harness-infra PR. The real shaka-side merge remains gated by your own bugfix-review.md HIGH actions (D12 rework of changes #2/#3, dead-code resolution) — out of scope for this repo's PR.

[kkroo]

Addressed the 3 residual suggestions: serve.py now asserts the mid-GOP start-slice contains an Init MPU (fails loud, not silent mis-test) + docstrings the shaka-harness caller; e2e.sh PASS string now states the real mechanism + HARNESS selection. Midjoin e2e still PASS. (Note: caveat #2's 'Player.load deadlock' is stale — the shaka-side fix proved Player.load plays; visible in shaka #4.) Re-triggering.

[kkroo]

Re-review request: since the last Ally pass (06-01T01:28), the base blo-4020-m4-t1 has merged the M.4 scoping + decision PRs (#8 docs, #9 T1.6 correction, #10 B/Opus, #12 OD-1 + T1.FEC API w/ libmmt sign-off). Requesting a fresh Ally review of #7 against the updated base. @ally / cc blockcast-ci-packages.

[blockcast-ci-packages]

Ally — Base-aware re-review @ 971eec78 (requested by kkroo)

Checked: (a) #7 head delta since my last pass (4d50d685 → 971eec78), and (b) #7's content against the updated base blo-4020-m4-t1, which has merged #8/#9/#10 (scoping), #11 (re-vendor libmmt FEC trailer), #12 (MmtpHeaderExt API sign-off) + a FEC-on replay-routing commit. Codex lens skipped (degraded): docs + localhost-harness surface only.

My 3 prior residual Suggestions — all ADDRESSED ✅

• serve.py now raises on a mid-GOP start slice that drops every Init MPU (if start>0 and not any(_ft_fi(h)[0]==0 …): raise ValueError), exactly the fail-loud guard I'd suggested; and the docstring now states it's wired by the demo/play-mmtp-load.html?scenario=midjoin|loss harness — closing the "unwired/no caller" note.
• m4-t1.7-e2e.sh:145 PASS message reworded to name the harness ("hand-rolled MSE pipe or production Player.load"), and the new m4-t1.2-DONE-handoff.md reports the production shaka.Player.load() path now works (the "createSegmentIndex deadlock" was a misdiagnosis — undefined originalMimeType, fixed). That retires my Imp2 from the prior pass: production playback is no longer unproven.

Important — base drift on the FEC-deferral rationale (the reason for this re-review)

• m4-t1.2-video-playback-design.md:28 (D11), :98 (§4.6), and m4-t1.2-DONE-handoff.md:47-49 all still describe the libmmt FEC trailer fix as a future/un-landed thing: "Blockcast/libmmt is already mid-fix on branch codex/mmtp-fec-trailer-timeout with ~430 lines of uncommitted FEC work … the SS_ID trailer fix converges with that libmmt branch under T1.FEC."
• That is now stale against this PR's own base. Base blo-4020-m4-t1 merged #11 (03cfd7b9, "re-vendor libmmt fec trailer contract"), which re-vendored exactly the architectural change D11 deferred: the vendored mmt-core now writes SS_ID as a trailer (write_source_fec_payload_id_trailer / split_payload_and_trailer / read_trailer, with a fec_source_id_is_packet_trailer_not_prefix test), write_to is base-only, and #12 (53dc6f77) records the libmmt-owner sign-off (APPROVED) on that API. The same commit also fixed the Rust example vector generator the plan doc says "still encodes the prefix bug."
• The deferral conclusion is still correct — T1.2's stream is FEC-off, so nothing here is blocked, and a JS-parser slice legitimately remains for T1.FEC. Only the rationale narrative is OBE. Recommend: update D11/§4.6/DONE-handoff to say the libmmt trailer contract has landed in-base (#11/#12, owner-signed-off), so remaining T1.FEC scope narrows to the shaka mmtp_parser.js trailer read + regenerated vectors (no longer "wait for / converge with an uncommitted branch"). Otherwise a reader takes T1.FEC to be blocked on work that has already merged underneath this PR.

Suggestion

• m4-t1.2-DONE-handoff.md:59 — "Next session targets" lists "T1.6 — @blockcast/transport WebTransport factory," but merged #9 corrected T1.6 scoping to a second package @blockcast/mmt-transport. Reflect both packages so the next-session pointer matches the merged correction.

Merge state

mergeable was still UNKNOWN (GitHub recomputing post-base-merge). No obvious file-level overlap: #7 touches only .planning/m4-t1.2-* + .planning/m4-t1.7-e2e*, while the merged base PRs added differently-named scoping docs and Rust (moq-pub-mmtp/…). Worth a glance once GitHub resolves it, but I don't expect a conflict.

Verdict

0 Critical / 1 Important / 1 Suggestion. The Important is documentation drift, not a code defect — the FEC-off scoping that makes this PR safe is unchanged. With D11/§4.6/DONE-handoff updated to reflect the merged libmmt trailer landing, this planning + harness-infra PR is clean against the new base.

[kkroo]

Review resolved (see prior comment for full findings):

• F1 (header rot) fixed in 7a2ffb7 — m4-t1.7-e2e.sh docblock now describes the playback flow (play-mmtp.html / MSE bufferedSeconds+currentTime) instead of the stale T1.7 observe-dump description.
• F2 (observe→playback supersession) accepted as intended: t1.2's playback e2e supersedes the T1.7 observe gate in this shared script.
• F3 (serve.py:_ft_fi byte-14/repair-packet edge) noted, non-blocking for this source-only capture.

serve.py frame-level loss/reorder shaping reviewed — correct granularity, fails-loud on missing Init. Rebased onto main (planning + e2e harness only, no product code). Merging via rebase.