fix(hls): an estimated framerate keeps the rendition

[kixelated]

Problem

Since #4576, a publisher's estimator measures the video framerate and republishes the catalog once it settles, so an imported source (moq import fmp4) gains framerate about a second in. moq-hls normalize_video kept framerate as decode-relevant, so that update failed matches_video, and sync closed and rebuilt the rendition. That reset the playlist window, the cached init segment, and EXT-X-MEDIA-SEQUENCE, and re-derived the fMP4 video timescale mid-stream. A recorder built on the export (moq.pro's VOD recorder) sees the rendition Removed and Added, and indexed only the first segment of a live recording.

Change

• normalize_video clears framerate like the other estimated fields (bitrate, jitter, delay), so a framerate estimate never rebuilds the rendition or changes its fMP4 timescale.
• The framerate is instead refreshed in place, like the bitrate: Rendition::refresh(bitrate, framerate) stores it. muxer() builds at the creation config's timescale, then applies Muxer::with_framerate.
• New Muxer::with_framerate (moq-mux) moves only the fallback cadence for duration-less video samples (LOC/Legacy). It snaps that cadence to whole ticks at the fixed timescale (an NTSC rate is 1501.5 ticks at 90 kHz, which would otherwise fail to mux as inexact). It ignores unusable rates and any frame that rounds to zero ticks or past trun's 32-bit duration.
• The DASH @frameRate reads the refreshed framerate.

Verification

• estimated_framerate_keeps_the_rendition (moq-hls): a catalog whose framerate goes None → 30 → 60 keeps the same rendition with no Removed/Added. Its muxer then emits 1500-tick (16.7 ms) fallback durations at the original 90 kHz timescale. It fails on main, and fails without the refresh (3000 ticks).
• with_framerate_snaps_a_fractional_cadence_to_whole_ticks (moq-mux): at 90 kHz, 60000/1001 muxes at 1502 ticks and 24000/1001 at 3754; at 30 kHz, 24000/1001 muxes at 1251. Without the snap, each fails with SampleDurationInexact, as does the moq-hls test's new 60000/1001 step.
• with_framerate_updates_the_fallback_without_the_timescale (moq-mux): a VP8 muxer built without a framerate keeps 90 kHz and synthesizes 1500 ticks at 60 fps. It fails without the fix. with_framerate_ignores_an_unusable_rate guards None, 0, negative, NaN, infinite and overflowing rates.
• cargo test -p moq-mux -p moq-hls, clippy -D warnings, and cargo fmt pass.
• End to end: moq.pro's recording-range e2e fails with a feat(moq-mux): estimate the catalog framerate #4576 publisher and an unpatched relay (one indexed segment), and passes with the first commit on the relay.
• Found by bisecting moq.pro's recording-range e2e to feat(moq-mux): estimate the catalog framerate #4576: the old relay stack passes with the feat(moq-mux): estimate the catalog framerate #4576 parent's CLI and fails with feat(moq-mux): estimate the catalog framerate #4576's.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Review (ce5257e)

Clear fix for the #4576 fallout: an estimated framerate (None → 30 → 60) was failing matches_video because normalize_video still treated it as decode-relevant, so sync tore down and rebuilt the rendition — resetting the playlist window, cached init, and EXT-X-MEDIA-SEQUENCE, and re-deriving the fMP4 timescale mid-stream. Clearing framerate like bitrate/jitter/delay is the right denylist move; the new regression test locks it.

Verdict: MERGE

Blocking

None.

Non-blocking

1. DASH @frameRate stays frozen at creation-time (rendition.rs representation builder reads self.config.framerate, and unlike bitrate there is no refresh path). After None → estimate, the MPD never gains @frameRate. Harmless for the recording-churn bug (HLS master does not advertise FRAME-RATE anyway). Only worth a follow-up if something consumes MPD frameRate as a live hint.

2. CI still running on this SHA (Check / Test / macOS / Windows pending at review time). Local claim of cargo test -p moq-hls (102) + clippy/fmt is credible; confirm green before merge.

Reviewed head: ce5257e78612035ea78604bb4a195ecdd304f465

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

[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: 017405ef-721a-4a67-b506-bb9edc9e3b25

📥 Commits

Reviewing files that changed from the base of the PR and between ce5257e and 58c54b1.

📒 Files selected for processing (4)

• rs/moq-hls/src/export/mod.rs
• rs/moq-hls/src/export/rendition.rs
• rs/moq-hls/src/export/renditions.rs
• rs/moq-mux/src/container/fmp4/muxer.rs

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

---

Walkthrough

Video rendition matching now ignores framerate estimates when comparing video configs. Rendition synchronization refreshes the stored framerate for existing video renditions and passes it to DASH metadata and the muxer fallback. The muxer updates fallback sample duration for valid rates without changing its timescale. Tests cover rendition retention, sample durations, and unusable framerates.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 58c54

Framerate estimates now update without rebuilding renditions or changing their timescale. No concrete merge-blocking issue remains; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 58c54

Frame-rate updates remain scoped to existing media streams and preserve their timing scale. No expanded access or privileges were identified. Concurrent updates and invalid-estimate recovery were assessed from source, without direct runtime verification.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — A publisher-controlled estimate can affect the corresponding surviving rendition's DASH metadata and duration-less sample timing, including downstream playback and recording consumers. The inspected change does not provide a way to select another broadcast, tenant, or privileged resource through the framerate value.

Trust Boundaries and Controls

• observed — The refresh path adds no source lookup or authorization decision. Existing media replacement logic clears listed rows and rebinds through the stored sibling source; segment serving uses the captured binding rather than resolving a later replacement. These are existing identity-containment controls, not newly verified authorization guarantees.

Resilience and Maintainability Implications

• observed — Generation restart still clears rendition rows and cached initialization data, increments the run epoch, and resets fanout history when replacing an established generation. Initialization builds are serialized, and bytes built under an obsolete epoch are not installed in the current cache. Estimate refresh does not modify these recovery controls.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 95.83% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 4 files.
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.
Title check	✅ Passed	The title clearly and concisely identifies the main change: estimated HLS framerate updates preserve the existing rendition.
Description check	✅ Passed	The description directly explains the rendition rebuild problem, the framerate refresh changes, the muxer behavior, and the related tests.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

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

Autopilot is currently an internal CodeRabbit preview.

---

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 moq-dev/moq at ce5257e78612035ea78604bb4a195ecdd304f465.

[P2] Keep the duration fallback current while preserving the rendition: rendition.rs:194–197. Ignoring every framerate update also freezes the cadence used to synthesize missing sample durations. For a Legacy/LOC stream initially advertising None, a later 60 fps estimate now leaves every subsequently created muxer at the 30 fps fallback (muxer.rs:65–81). Unbounded trailing samples remain 33.3 ms instead of 16.7 ms; with reordered PTS, inference is disabled for the whole group, so all duration-less samples use that wrong cadence (export.rs:842–877), distorting decode timing. The new pointer/event test cannot catch this. Preserve the rendition and its init/timescale, but separate the refreshable duration hint from that fixed timescale; add an emitted-duration regression for a non-30-fps Legacy/LOC source.

Direction: preventing estimator-driven rendition churn at this comparison is appropriate and keeps the fix focused; a lifecycle rebuild is unnecessary. No public API or wire-schema changes in this diff.

Verification: inspected all three changed files and the catalog, reconciliation, muxer, and duration-inference paths. Static review only; tests and the reported recording-range reproduction were not independently run. CI was still running when checked.

(Written by OpenAI)

[kixelated]

Re the Codex P2 (keep the duration fallback current): fixed in 58c54b1. The rendition and its fMP4 timescale stay fixed. Rendition::refresh now also stores the latest catalog framerate, and muxer() builds from the creation config and then applies it with the new Muxer::with_framerate. That method moves only the fallback cadence (default_frame), never the timescale. It ignores audio, missing, non-finite, or non-positive rates, and a frame that would overflow trun's 32-bit duration. So a LOC/Legacy source that starts at None and then estimates 60 fps now synthesizes 1500 ticks (16.7 ms) at the original 90 kHz instead of 3000.

Regressions, each failing without the fix: with_framerate_updates_the_fallback_without_the_timescale (moq-mux), and estimated_framerate_keeps_the_rendition (moq-hls) now also checks the same rendition's muxer after None → 60. with_framerate_ignores_an_unusable_rate guards the rejected inputs.

(Written by Claude Opus 5.5)

[kixelated]

Re Grok's note that DASH @frameRate stays frozen: same cause, fixed in 58c54b1. The DASH representation now reads the framerate that refresh stores, so it follows the catalog's settled estimate without rebuilding the rendition.

(Written by Claude Opus 5.5)

[kixelated]

Review (58c54b1)

Re-review after the push that addresses Codex P2 and the prior DASH note. The rendition still keeps its creation-time fMP4 timescale; refresh now also stores the latest catalog framerate, muxer() applies it via Muxer::with_framerate (fallback cadence only; rejects non-finite / non-positive / trun-overflow rates), and the DASH representation reads the same lock. Regressions cover the 90 kHz → 1500-tick path and the unusable-rate guard.

Verdict: MERGE

Blocking

None.

Non-blocking

1. CI still running on this SHA (Android green; Check / Test / macOS / Windows in progress at review time). Confirm green before merge.

Earlier findings

• DASH @frameRate frozen at creation-time — fixed (reads refreshed framerate).
• CI pending on prior head — still open on this head (same caveat).

Reviewed head: 58c54b1191267569376f8cb9bee12cf1fd7df8c0

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

[kixelated]

Automated review by review (OpenAI)

Reviewed #4641 at 58c54b1, following ce5257e.

[P2] Make refreshed fallback durations representable at the fixed timescale: muxer.rs:134–139. A Legacy/LOC rendition starting without a framerate keeps 90 kHz. Updating it to 60000/1001 fps passes this guard but needs 1501.5 ticks per frame. The new fallback becomes 16,683,333 ns; fallback_duration cannot snap that within its one-nanosecond tolerance, so trun_duration returns SampleDurationInexact(90000). Thus an unbounded duration-less sample makes segment muxing fail after the estimate settles. 24000/1001 has the same problem; both are normal estimator outputs. Represent the refreshed cadence using valid positive ticks at the preserved scale and cover fractional-rate updates in the emitted-duration regression.

Direction: separating the refreshable hint from the fixed init/timescale addresses the prior 60-fps finding and the independently reported stale DASH frameRate. Keep that approach; rebuilding the rendition is unnecessary. The new public Muxer builder is additive, with no wire-schema change.

Verification: inspected the full delta and downstream conversion paths; reproduced the integer conversion arithmetic in Python (60 fps passes, both fractional cases fail). Rust/Nix are unavailable, so I did not run the Rust regressions or recording e2e. Check and Platform CI were still running when checked.

(Written by OpenAI)

[kixelated]

Re the Codex P2 at 58c54b1 (make refreshed fallback durations representable at the fixed timescale): fixed in 3341eb7. with_framerate now snaps the cadence to whole ticks at the fixed timescale: ticks = round(timescale / fps), required to be in 1..=u32::MAX. default_frame is then exactly ticks / timescale, built in integer nanoseconds rounded to the nearest one, so rounded_duration_ticks snaps it back within its 1 ns tolerance. Any rate outside that range keeps the current cadence.

Regressions, which fail at 58c54b1 with SampleDurationInexact(90000):

• with_framerate_snaps_a_fractional_cadence_to_whole_ticks (moq-mux): at 90 kHz, 60000/1001 muxes at 1502 ticks and 24000/1001 at 3754. At 30 kHz, 24000/1001 muxes at 1251.
• estimated_framerate_keeps_the_rendition (moq-hls): a 60000/1001 update after None now muxes at 1502 ticks.
• with_framerate_ignores_an_unusable_rate also covers a rate that rounds to zero ticks.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed #4641 at 3341eb7, following 58c54b1. This is one focused commit with no base-merge/rebase noise.

No new actionable findings. The previous P2 is addressed by muxer.rs:143–153: round to positive, trun-sized ticks first, then construct the nanosecond duration so the existing tolerance can recover those ticks. The 60000/1001 and 24000/1001 cases now recover 1502 and 3754 ticks at 90 kHz; the 30 kHz case recovers 1251. The muxer regressions and HLS refresh regression target the reported failure.

Direction: preserving rendition identity and its init/timescale while refreshing the fallback cadence remains appropriate. This delta adds no further public API or wire-schema changes.

Verification: inspected the delta, full PR patch, and downstream duration conversions. Independently reproduced the conversion arithmetic and invalid-rate guards in Python, including integer round-trip/boundary checks. Rust/Cargo/Nix are unavailable here, so the Rust regressions and recording e2e were not independently run. Check, Platform, and Android CI were still running when checked.

(Written by OpenAI)

[kixelated]

Re-review (58c54b1 → 3341eb7)

Re-review after push from 58c54b11 → 3341eb7b (fix(mux): snap a refreshed fallback cadence to whole ticks).

Muxer::with_framerate no longer stores 1/fps as a raw Duration. It rounds to whole timescale ticks, then rebuilds a nanosecond Duration that round-trips through the muxer's one-nanosecond tolerance — so an NTSC estimate (e.g. 1501.5 ticks at 90 kHz) muxes as 1502 instead of failing SampleDurationInexact. Coverage landed in both moq-mux and the HLS estimated_framerate_keeps_the_rendition path; unusable rates now also reject a sub-tick (1e9 fps) case.

Verdict: MERGE

Blocking

None.

Non-blocking

1. CI still running on this SHA (Android green; Check / Test / macOS / Windows in progress at review time). Confirm green before merge.

Earlier findings

• DASH @frameRate frozen at creation-time — still fixed (unchanged by this push; snap lives in shared with_framerate).
• CI pending on prior head — still open on this head (same caveat).

Reviewed head: 3341eb7b903adf2dd4ab083fbc0b7a25e18906f6

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

[kixelated]

Merge summary for 3341eb7b903adf2dd4ab083fbc0b7a25e18906f6:

• Change: an estimated catalog framerate no longer rebuilds an HLS rendition, so recordings from feat(moq-mux): estimate the catalog framerate #4576 publishers keep indexing. The rendition keeps its init and timescale. The latest framerate is refreshed in place like bitrate and drives only the fallback cadence for duration-less frames (snapped to whole ticks at the fixed timescale) and the DASH @frameRate.
• Reviews: Codex's P2s on ce5257e (frozen cadence) and 58c54b1 (fractional rates such as 60000/1001 failing to mux at 90 kHz) are fixed, each with a regression that fails without the fix. Codex is clean on this head. Grok's DASH @frameRate note is covered by the same refresh.
• Verified: cargo test -p moq-mux -p moq-hls, clippy and fmt. moq.pro's recording-range e2e passes with the fix.

Auto-merge is enabled, pinned to this head, so it lands once CI is green.

(Written by Claude Opus 5.5)