[kixelated]

Quest: quest/m2/intra-refresh/dual-decoder.md

Summary

@moq/watch now decodes a video rendition with a catalog warmup (intra refresh) twice, as Rust's moq_video::decode::Consumer does (moq-dev/moq#7376). It also opens an H.264 intra-refresh join past Chromium's key check.

• Min-delay decoder. Holds the app's latency. It alone drives Sync (received, and reset on a timeline restart), counts stats, and shows the first-picture poster. It may show refresh stripes after a join or a skip.
• Max-delay decoder. Its container consumer gets delay: { min: delay + warmup, max: maxDelay + warmup }, and its subscription gets maxDelay + warmup. It starts one group earlier and tolerates one more sweep of lateness. That follows the Delay { min, max } decision from moq-dev/moq#7311: it passes no warmup prop. js/net merges the two subscriptions upstream. Pictures already past due present at once without moving the clock.
• Selector. A new Rendition class wraps the pair inside decoder.ts. The existing pending/active switch promotes a whole pair. The rule:
  • Show the max-delay picture when its timestamp is at or past the min-delay one's, so it wins ties. Otherwise show the min-delay picture.
  • Never show a timestamp below the last one shown, until the min-delay decoder restarts its timeline.
  • A max-delay picture from before that restart never wins.
  • It does not use caughtUp.
• One decoder without warmup. A missing or zero warmup keeps one decoder, as before. A catalog change opens or closes the max-delay decoder. A changed value updates its budgets without reopening it.
• Chromium join (new). On the chunk that opens a codec (after configure or reset), when the rendition has warmup and the chunk is H.264 with no IDR, insertRecoveryPoint inserts an SEI recovery point with recovery_frame_cnt = 0 right before the first slice. It handles both length-prefixed payloads (the avcC description gives the length size) and Annex B. An IDR, a payload with no slice, or one that does not parse is left alone. Both decoders get it, on every open and every reset. The code comment says why: Chromium's WebCodecs only opens a codec on an IDR or a zero-count recovery point (AnalyzeAnnexB in media/formats/mp4/avc.cc), and an intra-refresh recovery point counts a whole sweep.
• Docs. doc/lib/js/watch.md explains the two decoders and the inserted recovery point.

Tests in js/watch/src/video/decoder.test.ts cover the cases the Plan lists:

• Cold join (fed per consumer, and live through real consumers, where the max-delay decoder starts one group earlier).
• Live stall shorter than a sweep, where only the min-delay decoder skips.
• Live stall longer than a sweep, where both skip.
• No warmup: one subscription and one decoder.
• Only the min-delay decoder moves Sync, including across a timeline restart.
• The 100 ms trailing-gap regression.
• Hand-back when the max-delay decoder stops producing.
• A leading max-delay picture survives that decoder closing.
• insertRecoveryPoint: length-prefixed (4-byte and 2-byte lengths) and Annex B (3-byte and 4-byte start codes). IDRs, payloads with no slice, and malformed payloads come back unchanged.
• Both containers (legacy and CMAF): only the chunk that opens each codec is rewritten, in both decoders and again after a reset. An opening IDR and a rendition without warmup are untouched. Disabling the warmup gate, or the opening flag, fails these tests.

The quest file now describes only what remains: checking Firefox and Safari.

Measurements (Chromium 156 headless, WebCodecs only)

1080p30 H.264 x264 intra refresh (1 s sweep, 6 Mb/s, 20 s clip). Each access unit is fed live at 30 fps to one or two VideoDecoders. Three rounds, alternating order. The host was shared with other jobs, so CPU is noisy.

decoders	CPU s per 20 s (browser process tree)	peak PSS	late (>100 ms) / frames	decode latency p50 / p95
1	5.38, 6.25, 4.64 (23-31 %)	352-354 MiB	0 / 600	3.6-5.2 / 7.4-8.3 ms
2	9.28, 5.60, 16.23 (28-81 %)	378-380 MiB	0 / 1200	2.6-7.6 / 5.8-8.8 ms

The second decoder costs about 26 MiB and up to about twice the decode CPU, with no late frames. This measures the decoders, not the whole player. Headless Chromium's decoder implementation (software or hardware) was not checked.

Engine checks (H.264 recovery point sent as key)

• Chromium 156: works with the inserted recovery point. I encoded a 320x240 x264 clip with intra-refresh=1:keyint=30, so it has one IDR and recovery points at frames 30, 60, and 90. I joined at frame 60 and fed 60 access units to Chrome 156.0.8078.79 headless, sending each recovery point as key. Without the rewrite, decode() throws "An EncodedVideoChunk was marked as type key but wasn't a key frame" (avcC) or "A key frame is required" (Annex B). After insertRecoveryPoint ran on the opening chunk, it decoded 60/60 pictures in both formats. Chromium checks only the chunk that opens a codec (verify_key_frame in third_party/blink/renderer/modules/webcodecs/video_decoder.cc), so later recovery points sent as key pass unchanged.
• Firefox 157 (Playwright build): untested. It reports H.264 unsupported.
• WebKit (Playwright): untested. It would not launch on this host.

Decisions

1. ✅ A (2026-11-01). How should js/watch get an H.264 intra-refresh join past Chromium's key check?

   • ✅ A. On the chunk that opens a codec (after configure or reset), when the rendition has warmup and the chunk is H.264 with no IDR, insert an SEI recovery point with recovery_frame_cnt = 0 before the first slice. Keep the change local to decoder.ts.
   • B. Treat H.264 warmup renditions as unsupported in Chromium.
   • C. Ask Chromium to accept recovery points with a nonzero count as key chunks.

   Answer A is implemented. The quest stays open for Firefox and Safari, because neither can be checked on this host.

2. ✅ B (2026-11-01). Should the per-picture group map have a hard cap for a publisher that never outputs a picture?

   • A. Keep the output-side sweep and add a generous hard cap for hostile streams only, such as entries more than 10 s of timestamps behind the newest one submitted.
   • ✅ B. Keep the output-side sweep alone, with no hard cap.
   • C. Replace per-chunk bookkeeping with per-group boundaries.

   DecoderTrack maps each submitted chunk's timestamp to its group, and each output retires every entry at or before its timestamp. WebCodecs outputs pictures in presentation order, so the map stays bounded for any stream that shows pictures. A publisher that never outputs a picture (for example, only hidden VP9 frames) grows it by one entry per chunk. The maintainer accepted that growth as an explicit exception to the root AGENTS.md bounded-state rule for this map. WebCodecs sets no bound on how many chunks a codec may hold before output, so any cap is a guess, and round 4 showed that a cap can evict the group of a picture the codec still owes. The comment on #groups in js/watch/src/video/decoder.ts records the exception. A review finding that raises this again is refuted by this decision.

Choices made without asking

• Only the min-delay decoder shows the startup poster. A max-delay poster is a sweep behind on a cold join, and would set the no-rewind floor early.
• An error from either decoder fails the rendition, as in Rust.
• Each picture carries the group it was read from. The selector only takes pictures from the min-delay decoder's current timeline start on, so a picture held from before a publisher restart neither shows nor sets the no-rewind floor. Within each decoder, a picture decoded before its own restart never presents after it.
• Stats (frameCount, bytesReceived) come from the min-delay decoder only, since the bytes arrive once.
• The inserted SEI sets exact_match_flag = 0 and broken_link_flag = 0, and goes after any SEI the stream already carries, right before the first slice.
• Both decoders rewrite their opening chunk, since each opens its own codec.

Follow-ups

• One-frame stripes between paints. On every frame, the two decoders' clock waits resolve in separate timer tasks, min-delay first. A requestAnimationFrame landing between them would paint the min-delay picture once (striped while it is unrecovered). Rust holds a newer min-delay picture up to one frame interval for the max-delay one. Consider porting that hold if it shows up in practice.
• Player-level cost on real hardware. Measure the dual-decoder cost on the bench page (bench/browser), with hardware decode and a warmup catalog, once an importer publishes one (h264-import).
• HEVC intra-refresh join. Chromium also rejects a non-IRAP H.265 frame sent as key. HEVC has no recovery-point SEI that Chromium honors, so it needs its own approach once h265-import publishes a warmup.
• Pictures a codec outputs after reset(). If an engine delivers a picture decoded before a reset (the Pullfrog review asks which do), it still passes the late check and can move DecoderTrack.timestamp past the new timeline. That predates this PR. Dropping any output with no #groups entry would fix it, but the synthetic-output tests would need entries first.

Review

Head 5620fc283d: local review skipped under the simple-review rule (moq-dev/quest#364). It is a small fix (about 30 lines with its test) in decoder.ts internals, with no public API, wire format, security code, or unsafe. It answers Pullfrog's two findings on 22cc2aef0d:

• [Pullfrog] #groups retirement runs for an output decoded before a reset, and can delete the new timeline's entries. Fixed: only an output with an entry in the map retires entries. Every chunk gets its entry right before decode(), after any reset clears the map, so an output without one is from a codec life the reset ended. Moving the loop below the #generation check, as suggested, would not catch it, since generation is read when the frame comes out, after the bump.
• [Pullfrog] Rendition.timestamp keeps the old timeline's playhead after a restart, so a pending rendition waits to catch up to it. Fixed: the selector clears it when the min-delay decoder's timeline start changes, as DecoderTrack.timestamp clears on a break. The new test promotes a pending rendition once the outgoing one restarts its timeline fails without the fix.

Head 22cc2aef0d: local review skipped under the same rule. Since the last reviewed tree, it only added a comment on #groups that records decision 2, and a conflict-free merge of origin/main that touches no js/ files.

Earlier rounds: Codex gpt-6.1-sol, medium effort, via quest review --base origin/main, session 01a1990f. Head 5a3d950494 has the same tree as round 2's head 6b9f0d54ba, plus the round-3 cap and its revert.

• Round 1 (head a003ac15b1): [medium] #groups grows for chunks that produce no picture (hidden VP9 frames). Fixed in 6b9f0d54ba: an output retires every entry at or before its timestamp.
• Rounds 2 and 3 (head 6b9f0d54ba): [medium] still unbounded when no picture is ever output. Refuted by maintainer decision 2 (✅ B, 2026-11-01). The round-3 cap (05add87419) was reverted.
• Round 4 (head 05add87419): [medium] that cap can evict the group of a picture the codec still owes. Fixed by the revert in 5a3d950494. This was the design checkpoint, and the maintainer chose to continue with B. A future local review starts with quest review --new.

Earlier rounds on the dual decoder (heads 38d55dc47a and 7775e5a3e9): two findings, both fixed, then no findings.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: f9abe90f-1836-4a3d-9258-04e7de27db4c

Commits

Reviewing files that changed from the base of the PR and between 59e12a5 and 5620fc2.

Files selected for processing: 6

• doc/lib/js/watch.md
• js/watch/src/video/decoder.test.ts
• js/watch/src/video/decoder.ts
• quest/m2/intra-refresh/README.md
• quest/m2/intra-refresh/consumer-warmup.md
• quest/m2/intra-refresh/dual-decoder.md

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 22cc2aef0d00433508d8aafd6f1151ec87379885.

No actionable bugs found in the changed paths. Checked the dual-decoder lifecycle, min-only clock/stat ownership, restart filtering, tie/no-rewind selection, opening-chunk rewrite, and relevant tests and surrounding code, including an independent selector/Sync cross-check.

Overall direction is sound: the pair preserves live latency while retaining refresh history, using the existing consumer and shared clock rather than another scheduler. A single decoder would sacrifice either continuity or latency; keeping the pair internal and retaining one decoder without warmup limits the complexity. The documented local-review dispositions are not new findings here.

Verification limits: static review only; I did not run tests, browser playback, or benchmarks. Firefox/Safari recovery-point behavior and real-hardware player-level cost remain unverified; the PR's Chromium measurements were not independently reproduced.

(Written by OpenAI)

[pullfrog]

Important

The dual-decoder plumbing is sound and insertRecoveryPoint checks out against the H.264 spec and Chromium's key-frame gate. Two bookkeeping edges in the new selector can drop pictures the old single-decoder path showed: the #groups retirement loop runs for outputs that the #generation guard later discards, and Rendition.timestamp no longer resets on a timeline break, so #runPending's caughtUp compares against a stale playhead.

Reviewed changes

• Dual decoder. Rendition wraps a min-delay DecoderTrack that drives Sync, counts stats, and posts the poster, plus a max-delay one gated on catalog warmup; the selector shows the max picture when its timestamp is at or past the min one and never steps back.
• H.264 join. insertRecoveryPoint inserts a zero-count recovery-point SEI before the first slice of the chunk that opens a codec, handling length-prefixed and Annex B payloads; IDR, no-slice, and malformed input pass through.
• Wiring. Decoder.#active is now a Rendition; the min decoder alone reports to Sync; the max decoder adds warmup to its consumer delay bounds and subscription maxDelay, merged upstream by js/net.
• Tests and docs. New decoder.test.ts cases for cold join, short/long stalls, restarts, budgets, hand-back, and the SEI rewrite; doc/lib/js/watch.md and the quest updated.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 5620fc283d0e81fc6d3ebdeefbdb34b968fc1ca3, against my previous 22cc2aef0d00433508d8aafd6f1151ec87379885 review; base unchanged.

• Fixed: the stale rendition timestamp. Clearing it on a timeline change restores pending promotion, and the new test targets that case.
• Fixed in scope: unmapped output deleting new group entries. The retirement guard prevents that deletion. The broader late-output caveat remains, as already acknowledged in Follow-ups, not a new finding: if an engine emits an old 1000 ms picture after a restart to 0 ms, decoder.ts:570–574 skips retirement but :581/:604 can still update first/playhead; in instant mode subsequent lower-PTS pictures are dropped at :575–578. Discarding retired-lifetime output before timeline-state updates, with a regression test, would close that remaining case.

No new actionable bugs found in this delta. These targeted state fixes fit the existing design without another scheduler or timeout.

Static review plus independent cross-check only; no tests or browsers run. Actual engine delivery of post-reset stale output remains unverified.

(Written by OpenAI)

[pullfrog]

✅ No new issues found.

Reviewed changes

• #groups retirement guarded by a known output. The retirement loop now runs only when the output frame has its own entry in #groups, so a stale picture delivered after a reset() (the map is cleared before any new entry is written) can no longer delete the new timeline's entries. A frame with no entry is from the codec life the reset ended.
• Selection timestamp clears on a restart. Rendition.#select now sets this.timestamp.set(undefined) when the min-delay decoder's start changes, mirroring DecoderTrack.#onBreak, so #runPending's caughtUp no longer compares a pending rendition against the outgoing timeline's stale playhead.
• Regression test. promotes a pending rendition once the outgoing one restarts its timeline exercises exactly that promotion path and fails without the #select change.

Both prior review findings are fixed; the residual "pictures a codec outputs after reset()" case remains the pre-existing Follow-up it was already acknowledged as, not a new issue.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}