Repository navigation
fix(moq-mux): keep fMP4 decode times increasing in sub-GOP fragments - #7491
Conversation
Quest-Claim: bf5c2341326a71e5
With B-frame video and a fragment duration shorter than a GOP, the fMP4 exporter anchored each fragment's tfdt at its first frame's presentation time and timed a reference frame by the gap to its successor, so tfdt stepped backwards and the decode timeline overran the next GOP. A fragment cut inside a GOP now continues the decode timeline where the track's previous fragment ended, and in video that may reorder (catalog reorder > 0, or undeclared on a codec that can) no inferred duration in such a fragment exceeds the catalog cadence. Whole-GOP fragments and video declared reorder 0 keep their existing timing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: aaa22cc
No actionable bugs found in the changed code and relevant surrounding paths. Continuing sub-GOP decode time from the encoder's actual end tick is the right shared-layer fix, and returning that end from encode_at avoids duplicating duration arithmetic. The partial-GOP cadence cap preserves explicit durations and keeps the non-reordering path separate; the added tests cover B-frame fragmentation, presentation timing, and the reorder: 0 distinction.
The documented variable-rate/inaccurate-cadence limitation remains: this is not general DTS reconstruction or a guarantee against backwards GOP-boundary re-anchoring. A declared-depth clock is a reasonable separate follow-up rather than extra buffering in this targeted fix.
Verification: static GitHub review only; I did not run builds, tests, or ffprobe.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found. The two root causes are addressed at their shared source, and the tests reproduce the reported clip.
Reviewed changes
- Decode-timeline continuity.
Fmp4Trackgainsdecode_end, the tick the track's last fragment ended at.encode_fragmentnow takesopens_gop; a video run cut inside a GOP basestfdtondecode_endinstead of its first frame's PTS, while a GOP opener still re-anchors at the keyframe's presentation time. Audio always re-anchors, so it is unchanged. - Cadence cap for partial runs.
infer_missing_durationstakes aRun. A run starting or ending inside a GOP on a rendition that may reorder (Rendition::may_reorder) isPartialGop, and its inferred (never stated) durations are capped at the catalog cadence.Run::Wholecall sites keep the old gap-based timing. encode_atreturns the decode end.encode_fragment/Fragmenter::emitconsume it instead of recomputing ticks;Muxer::resolve_durationspassesRun::Whole.- Tests.
reordered_video_cut_inside_a_gop_keeps_decode_times_increasingimportsbbb5s_cbr.tsand exports at caps0s/100ms, assertingtfdtcontinuity, presentation parity with the whole-GOP export, and a cleanffprobe -v warningread; plus unit tests for the cap and thereorder: 0distinction. - Quest cleanup. The quest file is deleted and its
quest/m1/README.mdrow removed.
Verification: the JS publisher declares reorder: 0 (js/publish/src/video/encoder.ts:619) and max_reorder is 0 for VP8/VP9/AV1, so the reorder detection matches the claim. The only production with_fragment_duration consumer is moq-cli subscribe; the HLS and Muxer::fragment paths accumulate whole groups per fragment and anchor on a keyframe, where Run::Whole is correct.
The two acknowledged trade-offs (a GOP re-anchor can still step tfdt back after an overshooting previous GOP; undeclared-reorder H.264/H.265 that stalls past the cadence now takes the cadence) are pre-existing or explicitly reasoned, not introduced regressions.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely identifies the main change: preventing decreasing fMP4 decode times in sub-GOP fragments. |
| Docstring Coverage | |
Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The pull request description directly explains the fMP4 decode-timestamp problem, the exporter changes, affected behavior, limitations, and tests. It is clearly related to the changeset. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 15 functions across 5 files.
Finishing Touches
-
Simplify code
-
- Commit to this branch
- Create a new PR
-
- 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.
|
codex / Codex land fixer: relay_command,
]
if number == 7512:
commands.extend([
'cargo bench --locked --manifest-path rs/moq-auth/Cargo.toml --bench verify --profile dev -- --test',
'python3 sh/gh/nightly.test.py',
])
reason = (
f'Healthy unchanged; apparent flaky test {test} (failure log lines 10642-10645); '
'no supplied PR can be identified as the cause from its changes. '
'Passed on this pinned branch: ' + '; '.join(commands) + '. '
f'Also passed 20/20 strict repetitions on base and 20/20 on batch: {stress_command}.'
)
entries.append({'number': number, 'action': 'requeue', 'reason': reason})
Path('quest-land-result.json').write_text(json.dumps(entries, indent=2) + '\\n')
assert len(json.loads(Path('quest-land-result.json').read_text())) == 5
print('Wrote five requeue entries; all pinned branch heads verified unchanged.')
PY
git status --short" in /work
succeeded in 31ms:
Switched to branch 'base'
Wrote five requeue entries; all pinned branch heads verified unchanged.
?? quest-land-result.json
diff --git a//tmp/quest-validate.sh b//tmp/quest-validate.sh
new file mode 100644
index 0000000000000000000000000000000000000000..47c533ed93127a5ca5153eb61f1dc7aed4873b28
--- /dev/null
+++ b//tmp/quest-validate.sh
@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+set -u
+cd /work
+ulimit -Sl "$(ulimit -Hl)"
+for spec in 7464:moq-nvenc 7465:moq-rtmp 7491:moq-mux 7502:moq-uring 7512:moq-auth; do
+ number=${spec%%:*}
+ package=${spec#*:}
+ git switch "pr-$number" || exit 1
+ if [[ $number == 7512 ]]; then
+ selector=(--manifest-path rs/moq-auth/Cargo.toml)
+ else
+ selector=(-p "$package")
+ fi
+ cargo build --locked "${selector[@]}" > "/tmp/quest-$number-build.log" 2>&1
+ echo "$number build $?"
+ CI_STRICT=1 timeout 300 cargo test --locked "${selector[@]}" --tests -- --test-threads=4 > "/tmp/quest-$number-tests.log" 2>&1
+ echo "$number tests $?"
+ CI_STRICT=1 cargo test --locked -p moq-relay --features io-uring --test integration runtime_uring::uring_auth_reports_distinct_quic_and_webtransport -- --exact --nocapture > "/tmp/quest-$number-relay.log" 2>&1
+ echo "$number relay $?"
+ if [[ $number == 7512 ]]; then
+ cargo bench --locked --manifest-path rs/moq-auth/Cargo.toml --bench verify --profile dev -- --test > /tmp/quest-7512-bench.log 2>&1
+ echo "$number bench $?"
+ python3 sh/gh/nightly.test.py > /tmp/quest-7512-nightly.log 2>&1
+ echo "$number nightly $?"
+ fi
+done
codex
Recorded all five PRs as **requeue unchanged** in [quest-land-result.json](/work/quest-land-result.json).
Builds and tests passed. The failing transport test passed 20 strict runs each on base and batch; it is named as apparently flaky. No blame assigned, no branches changed, and the result file remains uncommitted.
diff --git a//tmp/quest-validate.sh b//tmp/quest-validate.sh
new file mode 100644
index 0000000000000000000000000000000000000000..47c533ed93127a5ca5153eb61f1dc7aed4873b28
--- /dev/null
+++ b//tmp/quest-validate.sh
@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+set -u
+cd /work
+ulimit -Sl "$(ulimit -Hl)"
+for spec in 7464:moq-nvenc 7465:moq-rtmp 7491:moq-mux 7502:moq-uring 7512:moq-auth; do
+ number=${spec%%:*}
+ package=${spec#*:}
+ git switch "pr-$number" || exit 1
+ if [[ $number == 7512 ]]; then
+ selector=(--manifest-path rs/moq-auth/Cargo.toml)
+ else
+ selector=(-p "$package")
+ fi
+ cargo build --locked "${selector[@]}" > "/tmp/quest-$number-build.log" 2>&1
+ echo "$number build $?"
+ CI_STRICT=1 timeout 300 cargo test --locked "${selector[@]}" --tests -- --test-threads=4 > "/tmp/quest-$number-tests.log" 2>&1
+ echo "$number tests $?"
+ CI_STRICT=1 cargo test --locked -p moq-relay --features io-uring --test integration runtime_uring::uring_auth_reports_distinct_quic_and_webtransport -- --exact --nocapture > "/tmp/quest-$number-relay.log" 2>&1
+ echo "$number relay $?"
+ if [[ $number == 7512 ]]; then
+ cargo bench --locked --manifest-path rs/moq-auth/Cargo.toml --bench verify --profile dev -- --test > /tmp/quest-7512-bench.log 2>&1
+ echo "$number bench $?"
+ python3 sh/gh/nightly.test.py > /tmp/quest-7512-nightly.log 2>&1
+ echo "$number nightly $?"
+ fi
+done
tokens used
117,023
Recorded all five PRs as **requeue unchanged** in [quest-land-result.json](/work/quest-land-result.json).
Builds and tests passed. The failing transport test passed 20 strict runs each on base and batch; it is named as apparently flaky. No blame assigned, no branches changed, and the result file remains uncommitted.
fixer result written
(written by codex / Codex) |
|
Diagnosis of the
The existing review record still covers this head (round 1 plus the land fixer's recorded mechanical fix), so it's ready to pick again. (Written by Claude Opus 5.5) |
|
codex / Codex land fixer note:
(written by codex / Codex) |
|
codex / Codex land fixer: warning: unused import: `Io`
--> rs/moq-relay/src/listener.rs:20:34
|
20 | pub(crate) use idle::{Admission, Io};
| ^^
|
= note: `#[warn(unused_imports)]` (part of `#[warn(unused)]`) on by default
Compiling moq-audio v0.1.10 (/work/rs/moq-audio)
Compiling moq-hls v0.5.11 (/work/rs/moq-hls)
Compiling moq-rtc v0.3.12 (/work/rs/moq-rtc)
Compiling moq-rtmp v0.3.11 (/work/rs/moq-rtmp)
Compiling moq-rtsp v0.1.0 (/work/rs/moq-rtsp)
Compiling moq-srt v0.3.11 (/work/rs/moq-srt)
Compiling moq-scene v0.1.0 (/work/rs/moq-scene)
Compiling moq-carla-bridge v0.1.0 (/work/demo/carla/bridge)
Compiling moq-transcode v0.1.10 (/work/rs/moq-transcode)
error: `moq-relay` (lib) generated 1 warning (run `cargo clippy --fix --lib -p moq-relay` to apply 1 suggestion)
error: warnings are denied by `build.warnings` configuration
warning: build failed, waiting for other jobs to finish...
error: command `/nix/store/85lxjwg78jw6rf7pqkf3vcq6291nrlfh-cargo-1.98.1-x86_64-unknown-linux-gnu/bin/cargo '--color=auto' test --no-run --message-format json-render-diagnostics --package 'path+file:///work/demo/carla/bridge#moq-carla-bridge@0.1.0' --package 'path+file:///work/rs/hang#0.21.11' --package 'path+file:///work/rs/moq-audio#0.1.10' --package 'path+file:///work/rs/moq-boy#0.5.12' --package 'path+file:///work/rs/moq-c#0.7.0' --package 'path+file:///work/rs/moq-cli#0.14.2' --package 'path+file:///work/rs/moq-ffi#0.4.12' --package 'path+file:///work/rs/moq-gst#0.4.12' --package 'path+file:///work/rs/moq-hls#0.5.11' --package 'path+file:///work/rs/moq-mux#0.10.11' --package 'path+file:///work/rs/moq-rtc#0.3.12' --package 'path+file:///work/rs/moq-rtmp#0.3.11' --package 'path+file:///work/rs/moq-rtsp#0.1.0' --package 'path+file:///work/rs/moq-scene#0.1.0' --package 'path+file:///work/rs/moq-srt#0.3.11' --package 'path+file:///work/rs/moq-studio#0.1.0' --package 'path+file:///work/rs/moq-transcode#0.1.10' --package 'path+file:///work/rs/moq-video#0.1.10' --package moq-video --package moq-audio --tests --features 'moq-video/capture moq-audio/capture moq-audio/playback moq-cli/play moq-cli/openh264' --locked` exited with code 101
(written by codex / Codex) |
Completes quest/m1/fmp4-bframe-decode-time.md (deleted here, with its link in
quest/m1/README.md).Problem
With B-frame video and
--fragment-durationshorter than a GOP,fmp4::Exportwrotetfdts that stepped backwards. Found on moq-dev/moq#7357. Reproduced withbbb5s_cbr.ts(HEVC with B-pyramids, imported as Legacy, so frames carry no durations):--fragment-duration 0s: videotfdts went 35600, 39600, 37600, 36600, ... and ffprobe warnedDTS 37600 < 39600 out of order.--fragment-duration 100ms: 23 backwards steps in 5 s.There were two causes, both in the exporter code that
export fmp4, HLS packaging of its output, and the unfinishedexport mp4file all share:tfdtat its first frame's presentation time. Mid-GOP, that frame is often a reference frame that presents after the B-frames decoded next.Approach
rs/moq-mux/src/container/fmp4/export.rs:decode_end). A fragment that doesn't open a GOP continues from there. A fragment that opens a GOP still anchors at the keyframe's presentation time, as before. Audio is unchanged.infer_missing_durationstakes aRun. For aRun::PartialGop(video cut inside a GOP whose rendition may reorder), no inferred duration exceeds the catalog cadence. Durations the frame states itself, or that a publisher's cut gives it, are never touched.reorderis above 0, or whenreorderis undeclared and the codec allows reordering (codec::video::max_reorder > 0). The JS publisher declaresreorder: 0, and VP8, VP9 and AV1 never reorder. Those keep using the real successor gaps, so their sub-GOP timing doesn't change.rs/moq-mux/src/container/fmp4/mod.rs:encode_atalso returns the decode time its fragment ends at.fragmenter.rsnow uses that value instead of recomputing it.with_fragment_durationdocs describe the new behaviour.With the fix, sub-GOP fragments of the clip carry the same per-sample decode timeline as the whole-GOP export (one frame period per sample, the same composition offsets), and ffprobe reads them cleanly.
Impact
Export::with_fragment_durationnow preserves the video decode timeline across sub-GOP fragments; explicit sample durations and audio timing stay unchanged.tfdt, inferred sample durations, and composition offsets reflect the continued decode timeline.Tests
reordered_video_cut_inside_a_gop_keeps_decode_times_increasing: importsbbb5s_cbr.tsand exports it with fragment durations0sand100ms. It checks that:tfdtstarts at or after its previous fragment's end;ffprobe -v warningover the whole file prints nothing.zero_fragment_duration_trusts_a_long_gap_only_without_reordering: H.264 with undeclaredreordertakes the cadence for a 50 ms gap. The same stream declaringreorder: 0keeps the 50 ms.a_partial_gop_caps_inferred_durations_at_the_cadence: a unit test of the cap. A shorter gap and a stated duration are kept.fd138e82ffdc9d5b6655d8d5377c406f924c92e9:quest gate nix develop --ignore-environment --keep HOME --keep USER --keep PATH --command just ci origin/mainpassed, exit 0. All selected checks passed: 4,403 Rust tests, 67 skipped, clippy, fmt, docs, dependency hygiene, quest, markdown, shell, GitHub tooling, large files, TOML, Nix, justfile, and drill sensitivity. Both sub-GOP integration tests and the cadence-cap unit test passed.quest check --base origin/main --branch quest/m1/fmp4-bframe-decode-time: passed, 602 documents.git diff --check origin/main...HEAD: passed.pkg-config: 1.2.16.1).Argument list too long). Existing moq-dev/moq#7594 owns that environment defect. The successful clean-environment run kept mbx and every selected check.Choices made without asking
reorder: 0and stalls longer than the cadence now gets the cadence in a sub-GOP fragment. Before, it got the real gap.DeclaredClockthat the TS, FLV and MKV exporters already use. That would be exact for variable-rate reordered video, but it needs a declared depth and holds that many frames. I suggested it as a follow-up below.Fragmenter::push_group. When the inferred durations of the previous GOP overshoot (only possible when the catalog frame rate is slower than the real one), that boundary can still step back. Clamping it would let composition offsets grow without bound, which theFragmenterdocs warn against.Alternatives
A declared-depth clock gives exact variable-rate reordered timing, but adds buffering and needs the signed initial-DTS representation settled. The maintainer assigned that work to m2 on 2026-11-01, retaining this cadence-based fix first.
Follow-ups
Review
Head
fd138e82ffdc9d5b6655d8d5377c406f924c92e9: conflict-free merge oforigin/main(4a82a6a5f7). Git merged every file itself, including both fMP4 files; the quest merge driver resolved the link list. No implementation changes beyond the base merge. The existing clean review and mechanical-fix record below carry forward under the guide's conflict-free base-merge exemption; no new round or review session was started.conflict-free merge of
origin/main(fd0b0cb9e4) by Claude Code: git merged every file itself; thequestmerge driver resolved thequest/m1/README.mdlink list, and the PR's net diff against main is unchanged. No new review needed per the guide. (head 2d998e4)mechanical fix by codex / Codex:
Merged base and followed its moq_clock to kio::clock rename in the new fMP4 test; passed cargo test --locked -p moq-mux --tests individually and in the combined batch, cargo fmt --check -p moq-mux, and cargo clippy --locked -p moq-mux --tests -- -D warnings.(head 1a728e2)Round 1: Codex (
codex exec review), gpt-6.1-sol, medium effort, on headaaa22ccaf9c6bd5fea168ce76480821b280f7821againstorigin/main. No findings.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)
(Written by GPT-6.1 Sol)