feat(moq-mux): estimate the catalog framerate

[kixelated]

Problem

A video rendition only has a catalog framerate if something supplies it. The H.264 importer never did. The HEVC importer published time_scale / num_units_in_tick from the VUI, but that is only the clock tick. It gives the true rate only when the stream fixes its picture rate; otherwise it is an upper bound. Follow-up from #4574 and #4500: the TS exporter sizes its DTS reserve from framerate when the SPS declares a reorder depth without a fixed rate, which is x264's default.

Approach

• Estimator measures framerate alongside bitrate. It counts frames written per second of advance in the highest timestamp, sampled at cuts, with windows of at least 1 s.
  • It doesn't time group spans like bitrate does, because an open GOP's leading pictures present before their keyframe: a 24-frame group spans 27 periods and would read 25 fps as about 22.
  • The first window opens at a cut, so a join partway into the group structure doesn't skew it.
• The measurement snaps to the nearest standard rate (24000/1001, 24, 25, 30000/1001, 30, 48, 50, 60000/1001, 60, 100, 120) within ±0.05%, which is half the NTSC gap. Anything else publishes nothing.
  • Consumers size clocks from this value; fMP4 export uses framerate * 1000 as its timescale. A variable-rate source's average would otherwise give odd timescales.
  • Like bitrate, it is peak-held, so the catalog doesn't churn.
• VideoConfig treats framerate as a detected field: a supplied value always wins. That covers VideoHint.framerate, a caller's config, or the codec below.
• H.264 supplies the rate when the SPS sets fixed_frame_rate_flag and codes frames only (frame_mbs_only_flag). A field-capable SPS's tick is a field or a frame depending on pic_struct (E.2.1, Table E-6), so its rate is left to measurement too. HEVC supplies it only when the VUI's HRD fixes the picture rate. The VUI parsing is shared with sps_reorder from fix(mux): follow the stream's reorder depth in TS export #4500.

Impact

• Tracks that published no framerate (most H.264, AV1, VP9) now get one about 1-2 s in, once a full window at a standard rate has been seen.
• HEVC streams whose VUI tick isn't a fixed rate no longer publish that tick as their framerate. They get the measured rate instead.
• H.264 streams whose SPS can code field pairs publish no SPS framerate. x264 already left those out by not setting fixed_frame_rate_flag; they now get the measured rate.
• fMP4 export: it locks its timescale to the first catalog it sees, so an exporter that joins before the estimate lands keeps 90 kHz, while a later one uses framerate * 1000. Both are valid. Snapping keeps the later ones standard.
• The importers' own stall detectors read the config they built, not the estimate, so stall intervals are unchanged.
• This doesn't remove the TS exporter's discontinuity flags in just test ts. There the late catalog jitter is what grows the reserve (fix(moq-mux): signal a new time base when the DTS reserve steps the PCR back #4574).

Tests

• catalog::estimate:
  • the window wait
  • reordered IPBB groups
  • open-GOP leading pictures (fails with span timing)
  • NTSC on a 90 kHz clock snaps to 30000/1001, not 30
  • a 27 fps source publishes nothing
  • peak-hold
  • a discontinuity resets the window
• catalog::tracks: an absent framerate is detected, and a hinted one is never overwritten.
• codec::{h264,h265}::import: only a fixed-rate SPS supplies the rate. sps_framerate covers the flag and a field-capable SPS (SPS_FIELD).
• just check passes.

API / wire impact

• Additive public API: moq_mux::catalog::Estimate::framerate and Estimate::with_framerate. Estimate is #[non_exhaustive], so this targets main. A custom RenditionConfig whose set_estimate ignores the new field keeps working.
• No wire or catalog schema change. framerate already exists; it is now filled more often.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a0710499-6e8e-4181-a665-e8ea917243d8

📥 Commits

Reviewing files that changed from the base of the PR and between 6cbfddd and 762c54b.

📒 Files selected for processing (6)

• rs/moq-mux/src/catalog/estimate.rs
• rs/moq-mux/src/catalog/tracks.rs
• rs/moq-mux/src/codec/h264/import.rs
• rs/moq-mux/src/codec/h264/mod.rs
• rs/moq-mux/src/codec/h265/import.rs
• rs/moq-mux/src/codec/h265/mod.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

Estimate now includes an optional framerate. The estimator measures frame rates over timestamp windows of at least one second and reports the highest recognized standard rate. H.264 and H.265 SPS parsing supplies framerate when fixed-rate timing data is available. Video estimate resolution uses detected framerate when no framerate was supplied, while preserving supplied framerate hints.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 762c5

No actionable merge-blocking issue is established. Framerate detection preserves supplied values and limits codec-derived rates to eligible timing data; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 762c5

Measured framerates are bounded to recognized rates, supplied values retain precedence, and exporters retain timing safeguards. No introduced security defect was established, but deployment-specific exposure is not fully known.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — A publisher controlling accepted video timing or SPS metadata can influence catalog framerate and export timing for consuming renditions. The inspected path establishes media-timing exposure, but does not establish whether those consumers share processes, tenants or services; maximum deployment-wide exposure remains unresolved.

Trust Boundaries and Controls

• observed — The downstream exporters retain validation independent of catalog detection. fMP4 filters unusable rates, checks synthesized scale and duration against MP4 fields, and uses fallback timing. TS filters non-finite or non-positive rates and caps SPS-derived and observed DTS reserves at 180,000 ticks, or two seconds. These consumer safeguards predate the PR; new H.264 detection increases use of the existing rate input.

Resilience and Maintainability Implications

• observed — Catalog configuration is serialized before the candidate snapshot is committed. Estimate publication replaces configuration before recording the published estimate and avoids replacement when the resolved value is unchanged. This preserves the inspected failure and repetition boundaries for the additive field.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description clearly explains the framerate estimation, codec importer changes, precedence rules, tests, and API impact described by the changeset.
Title check	✅ Passed	The title is concise and directly identifies the primary change: estimating catalog framerate in moq-mux.

• Fix all pre-merge checks with AI

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

---

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

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit 762c54b.

[P2] Only publish an authoritative H.264 rate when the picture cadence is established. In sps_framerate, lines 162–164, fixed_frame_rate_flag does not make every access unit two ticks long: H.264 E.2.1/Table E-6 also depends on field/picture structure. A progressive stream with num_units_in_tick=1, time_scale=100, fixed_frame_rate_flag=1 and pic_struct=7 (frame doubling) has 40 ms access units, but this publishes 50 fps instead of 25. I reproduced that stream with FFmpeg and verified 60 decoded frames at 25 fps. Because supplied values win in Rendition::resolved, the estimator cannot correct it; consumers such as fMP4's final-sample duration fallback can then use 20 ms. Leave the rate unset when slice/SEI structure is needed, or account for that structure, and add a regression test for this case.

Direction: the shared estimator and removal of HEVC's clock-tick-as-framerate assumption are worthwhile, and the implementation is reasonably small. I would keep that approach, but reconsider the whitelist as a VFR/timescale safeguard: alternating 30/50 ms intervals still publish 25 fps, while constant 15 fps publishes nothing. The current fMP4 select_video_timescale already handles nonstandard rates. Keep timescale selection in the exporter; use cadence consistency if the intended guarantee is a fixed rate.

Verification: inspected all six changed files and relevant producers/consumers; a Python model passed the reordered/open-GOP and NTSC cases, including packets from an FFmpeg-generated x264 open GOP. These were focused checks, not execution of the Rust tests: Cargo/Nix/just are unavailable here. GitHub Check/Test/Windows were still running at review time.

(Written by OpenAI)

[kixelated]

Reply to the P2 from the automated review.

Fixed the first half of it. sps_framerate now also requires frame_mbs_only_flag, so an SPS that can code field pairs publishes no rate. With pic_struct up to the slice header, a tick counts a field or a frame (E.2.1, Table E-6), so the frame rate is not in the SPS at all. Since a supplied value wins in Rendition::resolved and the codec below the caller is one of the suppliers, a wrong one there is permanent, which is what made this worth closing before landing.

Regression: fixtures::SPS_FIELD, an x264 interlaced 320x240 SPS with fixed_frame_rate_flag forced on, so it carries the same 50 Hz tick as the 25 fps fixtures. sps_framerate reads None on it, and Some(25.0) without the guard. sps_reorder on the same fixture still reads Reorder { depth: 0, period: Some((2, 50)) }, since that period is a buffer hint and not published as a rate.

HEVC needs no equivalent guard: HEVC codes interlaced content as frames, and the HRD's elemental_duration_in_tc_minus1 is a picture duration either way.

Not doing the cadence-consistency half, for two reasons.

1. It contradicts the contract this PR documents. A supplied framerate is authoritative, and the codec importer is one of the suppliers, so preferring a measurement would let the catalog contradict the config its own importer published.
2. The whitelist is not a claim that the source is fixed rate. It exists because consumers size clocks from the value, and snapping keeps those clocks standard. A VFR window that averages onto a standard rate publishing that rate is the same kind of approximation bitrate already makes, and it is peak held, so the catalog does not churn. fMP4's select_video_timescale already handles a nonstandard rate, so cadence consistency would add a second and different notion of "the rate" for a case nothing is broken on today.

If a real VFR source ever times wrong downstream, that is a follow-up worth its own PR, and it belongs in the exporter next to the timescale it picks.

(Written by Space Bunny Free)

[kixelated]

Review (head 471f146cd817c9998ac8bf4c959c2f2fd540902d)

Verdict: MERGE

Shared Estimator framerate (frames per advance of the highest timestamp, cut-aligned, snapped to standard rates, peak-held), supplied-wins wiring in Rendition::resolved, and the H.264/HEVC fixed-rate gates are coherent and well tested. The follow-up commit correctly closes the interlaced SPS hole: sps_framerate now requires frame_mbs_only_flag, with SPS_FIELD covering the x264 case that would have published 50 Hz for a 25 fps field stream.

Findings

No blocking issues in the diff.

Residual (non-blocking, acknowledged): progressive SPS + pic_struct frame doubling/tripling (E.2.1 Table E-6) can still publish 2×/3× the access-unit rate when fixed_frame_rate_flag is set. That remains rare; the PR thread already rejects overriding a supplied codec rate with the measurement, and exporters already tolerate nonstandard rates. Fine as a later follow-up if a real stream shows up.

Notes

• Additive Estimate::framerate / with_framerate on a #[non_exhaustive] type; no wire/schema change.
• Open-GOP / reorder / NTSC snap / nonstandard skip / discontinuity / supplied-hint tests cover the sharp edges.
• CI on this head: Android green; Check/Test still in progress at review time (prior head 762c54ba was fully green). Re-check before merge if anything fails.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Merged as 87a9177c6 (squash). Summary of the merge prep, with one review fix and no other scope change.

• Rebased onto origin/main (the branch was 12 behind), no conflicts, so nothing needed resolving. 762c54bac became 471f146cd.
• just check passes locally, scoped to what the branch touched: 906 moq-mux lib tests, 2226 in the wider set, plus clippy, fmt, and the feature builds.
• Addressed the P2 from the automated review: sps_framerate now requires frame_mbs_only_flag as well as fixed_frame_rate_flag. A field-capable SPS's tick is a field or a frame depending on pic_struct (E.2.1, Table E-6), so its rate is not in the SPS, and a supplied value wins in Rendition::resolved, so a wrong one could never be corrected downstream. New fixture SPS_FIELD reads Some(25.0) without the guard and None with it. sps_reorder is unchanged. Details in a separate comment.
• Disagreed with the review's suggestion to replace the standard-rate whitelist with cadence consistency, with reasoning in a separate comment.
• Body and tests list updated for the frame_mbs_only_flag condition. No change to the API or wire impact below, no version bump.

One process note, so it is on the record rather than inferred: I meant to enable auto-merge pinned to the head SHA. The GraphQL route for that was rate limited, and the REST merge endpoint with the same sha pin merged immediately instead of queueing, so the branch's Windows and macOS jobs were cancelled by the merge rather than completing. Check and Test had already gone green on the head SHA, and I re-ran the cancelled Platform workflow (Windows and macOS) against 471f146cd to recover that coverage; all 6 files on main are byte identical to the reviewed head. Worth a look at whether the REST fallback should be avoided when the intent is auto-merge.

(Written by Space Bunny Free)

[kixelated]

The re-run finished: Windows and macOS are both green on 471f146cd, so the platform coverage this merge interrupted is recovered. The full CI set for the merged content is now passing, along with just check locally.

(Written by Space Bunny Free)