You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(moq-mux): an HLS viewer starts on-demand capture - #7054
The HLS export reads a rendition's records and FETCHes media groups per segment request, so it never subscribes to the media. On-demand publishers (moq publish capture via moq-video / moq-audio) gate on track.demand().wanted() of the media track. So when an HLS export was the only viewer, the camera never opened and no segments appeared.
Change
rs/moq-mux/src/hls.rs: each Recorder now lends its record track's demand to its media track. While the record track has any subscriber, a local subscription keeps the media wanted. The demand ends when the record track's last subscriber leaves, the record track ends, or the recorder drops.
Runtime-free, like the rest of hls.rs: a Waker built on a Weak to the lend re-polls the record track's Demand::poll_wanted_changed and opens or drops the media subscription inline. kio fires wakers only outside its own locks, and the lend's own work touches only the media track.
Both track handles are weak. The media side is a track::Consumer from Producer::consume(), and the lend holds an unresolved track::Subscribing, which is demand while held. So the lend never keeps a track open. My first version held a strong track::Producer clone. That kept the media track open past its owner's handle and changed teardown order: moq-hls's record_cursors_yield_renditions_and_segments saw CatalogFailed before RecordsFailed.
rs/moq-net/src/model/track.rs: TrackState::advance woke the demand waiters while holding the track-state write guard. It was the only track path doing so, against kio's "release before waking" rule that join also follows. Because the lend polls inline from its waker, a join over the record track's refresh ratio deadlocked the next record publish (found by review). advance now defers its wakes into the state, and its five guarded callers fire them after dropping the guard.
Wired where a recorder meets its media track: container::Producer::with_recorder, and the fMP4 passthrough's enrollment. Every catalog-enrolled video/audio track gets it, not only the CLI.
No public API change and no wire change. The lend's inline poll depends on every wake of a track's waiters happening after its lock is released, which kio and moq-net now both uphold. The Lend docs record that requirement.
Tests
moq-muxhls::test::record_readers_want_the_media: record readers make the media wanted, the last reader leaving releases it, and dropping the recorder releases it.
moq-muxhls::test::a_refreshing_join_keeps_lending: puts a reader on the records, passes the refresh ratio, joins late, and publishes the record that answers the join with a fresh group. It hangs (nextest's 60s timeout) with the moq-net change reverted, and passes with it.
moq-videoencode::producer::tests::control::an_hls_only_viewer_starts_capture: runs the real capture driver moq publish uses, against a synthetic camera. The only subscriber is a moq_mux::hls::Consumer on the advertised records. The camera opens and the first segment record arrives.
moq-video and moq-audiohls_records_keep_the_live_timestamps held a record reader across an idle restart. Under the new semantics that reader keeps capture running, so both now read the records after the runs. What they cover (idle gap left out of the segments) is unchanged.
Choices made without asking
Lend wired at with_recorder + fMP4 enrollment, not by changing Tracks::track / catalog::Producer::enroll to take the media track. Binding at enrollment would be marginally harder to miss, but it would change ~20 name-based test call sites for the same two production paths.
The lend subscribes with Subscription::default() (priority 0, zero delay budget), the same as subscribe(None) elsewhere. On a publisher-local track the aggregate isn't forwarded anywhere.
Not covered: audio records aren't advertised until audio has run
moq-audio capture enrolls its rendition without an HLS hint, so its hls catalog field appears only after its first segment. An HLS-only viewer of an A/V moq publish therefore finds only the video records. Video starts now, but audio's records stay unadvertised, so audio never starts and the export lists no audio rendition. This has a separate, pre-existing cause; once audio records are advertised, the demand path in this PR starts audio too. Audio groups have a fixed cadence (Options::group_duration), which is exactly what a hint declares. Two catches:
Passing one via catalog.reserve().with_hls_hint(..) would make the rendition withhold the catalog until layout discovery, which audio capture deliberately avoids.
A non-withholding per-track hint is new public moq-mux API.
So it's suggested as a follow-up below rather than done here.
Suggested follow-ups
Audio capture advertises its HLS records up front.moq-audio's capture (rs/moq-audio/src/encode/producer.rs, Reserved::new → catalog.audio(..)) should pass its fixed group cadence as an HLS hint, so the catalog names its record track before audio runs. That needs a way to hint a rendition that doesn't withhold the catalog, e.g. a hint on catalog::Producer::audio/track, or a Reserved variant that doesn't gate. Then extend an_hls_only_viewer_starts_capture (or an audio twin) to an HLS-only viewer of audio capture.
Stale private intra-doc links in moq-mux.cargo doc -p moq-mux --document-private-items -D warnings fails on pre-existing links (catalog/consumer.rsmsf, catalog/producer.rsRenditionConfig::insert, catalog/tracks.rsInit::label, container/producer.rsReserved). CI doesn't build private docs, so they don't fail it.
Checks run locally (all through quest gate)
just rs test -p moq-net -p moq-mux -p moq-hls -p moq-relay: 5532 passed
just rs test -p moq-video -p moq-audio -p moq-scene -p moq-studio -p moq-cli --features moq-video/capture,moq-video/openh264,moq-audio/capture,moq-audio/playback,moq-scene/capture,moq-studio/capture: 841 passed
An HLS export reads a rendition's records and fetches media per segment, so it
never subscribes to the media. An on-demand publisher such as `moq publish`
capture therefore never started for an HLS-only viewer.
Each recorder now lends its record track's demand to its media track: while the
records have a subscriber, a local subscription keeps the media wanted. It is
driven by the record track's wakeups (no runtime) and holds only weak track
handles. The capture HLS tests that idle between runs now read the records
afterward, since a record reader keeps capture running.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No actionable comments were generated in the recent review.
Recent review info
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: 5219506c-d98a-4f41-aef1-8153fd641fe9
Commits
Reviewing files that changed from the base of the PR and between d82743f and 6c03cf7.
Files selected for processing: 8
quest/m1/README.md
quest/m1/eager-encode/README.md
rs/moq-audio/src/encode/capture.rs
rs/moq-mux/src/container/producer.rs
rs/moq-mux/src/hls.rs
rs/moq-net/src/model/track.rs
rs/moq-net/src/model/track/fill.rs
rs/moq-video/src/encode/producer.rs
Files with no reviewable changes: 1
quest/m1/README.md
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
Walkthrough
HLS record subscribers now create demand for the corresponding media track through a local subscription. The recorder releases that subscription when record demand ends or the recorder is dropped. Track demand wakeups are deferred until after the track-state guard is released. Tests cover HLS-only capture, subscriber changes, and record consumption after capture runs finish.
Priority: Normal
Merge Risk: Minimal · up to 6c03c
HLS record readers can now create media demand. No identified issue needs to be resolved before merging, subject to normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title clearly and concisely describes the main change: enabling an HLS viewer to start on-demand capture.
Description check
The description directly explains the HLS demand-lending implementation, related deadlock fix, tests, limitations, and follow-ups.
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.
Full details: Docstring Coverage
Explanation
Docstring coverage is 70.00% which is insufficient. 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 10 functions across 6 files. (2 skipped: 1 unsupported, 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
✨ Simplify code
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
Clippy (1.99.0)
Clippy execution timed out
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.
P1 — record refresh can deadlock the publisher (rs/moq-mux/src/hls.rs:737–739). I verified the identical existing finding, rather than adding a duplicate inline comment: TrackState::advance wakes subscription waiters while its write guard remains held; this synchronous callback re-enters Demand::subscriptions, which reads that same state. A later viewer triggering the record track's refresh can therefore stall publication. Defer re-polling, or move those notifications outside the track guard, and add a regression covering a join after the refresh threshold.
Direction: lending record-reader demand at the shared recorder boundary is appropriate, and weak handles avoid extending producer lifetime. The runtime-free callback adds a lock-order requirement that the current implementation does not satisfy. The documented audio-discovery limitation is pre-existing, not a new finding here.
Verification: static inspection of the diff, enrollment paths, demand/subscription lifecycle and refresh locking; no tests run.
`TrackState::advance` woke the subscription waiters while the track-state write
guard was held, unlike every other track wake (`join`, preferences, kio's
consumer and guard paths). The HLS record lend polls the record track's demand
inline from its waker, so a join over the record track's refresh ratio
deadlocked the next record publish. Defer `advance`'s wakes until each caller
drops its guard, and cover the refreshing join in moq-mux.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Automated review by review (OpenAI)
Reviewed commit: d82743f, compared with f9d5db6 (unchanged base).
The previous P1 refresh deadlock is addressed: TrackState::advance now defers demand notifications, and the group, datagram and fill callers release the state guard before firing them. The added a_refreshing_join_keeps_lending regression exercises the problematic late join and verifies that a fresh group is produced.
No new actionable bugs found in this update. Direction is sound: fixing the notification boundary preserves the runtime-free demand bridge without introducing a task or extending producer lifetime. The explicit deferred-wake drain is justified here; future callers of advance must preserve it.
Verification: static review of the incremental changes and all production callers of the affected group/sequence paths. I did not run tests; the author's reported results were not independently reproduced.
Merge summary for head d82743f7b0119314a7b1a4a44d81b40b541a27b2:
Change: each HLS Recorder lends its record track's demand to its media track, so an HLS-only viewer now starts on-demand capture. The lend holds weak handles and is runtime-free. It also fixes TrackState::advance in moq-net, which woke demand waiters while still holding the track-state guard. With the inline lend, that deadlocked a late join on a refreshing record track.
Reviews: Codex's P1 on f9d5db6809 (synchronous re-poll deadlock) was fixed in d82743f7b0 with the a_refreshing_join_keeps_lending regression, and its thread is resolved. Codex reviewed d82743f7b0 with no findings (👍). CodeRabbit reported nothing actionable.
Checks: CI 11/11 passed. quest check --base origin/main is clean; the PR deletes quest/m1/hls-record-demand.md and its m1 link.
Decisions: the "choices made without asking" in the PR body stand (lend wired at with_recorder and fMP4 enrollment; Subscription::default()).
Follow-ups for /quest-plan: audio capture should advertise its HLS records up front with a non-withholding hint, and the stale private intra-doc links in moq-mux need fixing (both are described in the PR body).
Re-landing after the merge queue dropped this with merge_conflict.
Merged main into the branch (21804bc). The one conflict was in rs/moq-mux/src/hls.rs: this PR's Recorder::lend and main's new Recorder::declared were added at the same spot; both are kept, unchanged.
just check origin/main passed locally (9015 Rust tests, plus lint and quest checks); quest check passes.
Re-merged main after the queue dropped this with merge_conflict:
rs/moq-mux/src/hls.rs: the module doc conflicted with #7046, which made record tracks compressed. Kept main's "compressed moq_json::window" sentence and this PR's paragraph on record readers lending demand to the media.
rs/moq-net/src/model/track.rs merged automatically. I checked that every advance caller still drains the deferred demand wakes after it releases the guard.
quest check caught a new link from quest/m1/eager-encode/README.md (added in #7124) to this PR's deleted quest. Its note said the quest "narrows to the opt-in mode". The demand lending only matters for on-demand publishers, so the quest is finished and I dropped the sentence.
cargo clippy, cargo test, and cargo fmt --check pass for moq-mux and moq-net, and quest check passes against origin/main. Codex 👍'd the new head 9dd28c0.
Heads-up from moq-dev/moq#7164, which makes moq_video::encode::publish_capture encode always and adds Capture::on_demand as the opt-in. Its capture also keeps the probe's camera instead of opening it again.
an_hls_only_viewer_starts_capture depends on capture being demand-gated, so once both PRs are in, it needs on_demand: true in its Capture. Without it, opens stays at 1. The module-doc line here also conflicts with the rewrite there. I trial-merged this head (9dd28c0) into #7164 and it passes with that one change. Whichever PR lands second applies it.
The only textual conflicts were in quest/m1/README.md and quest/m1/eager-encode/README.md. The quest merge driver resolved them, keeping main's side and this PR's removal of the finished quest's links.
cargo test and clippy -D warnings pass for moq-audio, moq-mux, moq-net, and moq-video. CI passes, and Codex gave this head a thumbs-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
kixelated
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
kixelated
left a comment
Completes quest/m1/hls-record-demand.md (deleted here, along with its link in the m1 README).
Problem
The HLS export reads a rendition's records and FETCHes media groups per segment request, so it never subscribes to the media. On-demand publishers (
moq publishcapture viamoq-video/moq-audio) gate ontrack.demand().wanted()of the media track. So when an HLS export was the only viewer, the camera never opened and no segments appeared.Change
rs/moq-mux/src/hls.rs: eachRecordernow lends its record track's demand to its media track. While the record track has any subscriber, a local subscription keeps the media wanted. The demand ends when the record track's last subscriber leaves, the record track ends, or the recorder drops.hls.rs: aWakerbuilt on aWeakto the lend re-polls the record track'sDemand::poll_wanted_changedand opens or drops the media subscription inline. kio fires wakers only outside its own locks, and the lend's own work touches only the media track.track::ConsumerfromProducer::consume(), and the lend holds an unresolvedtrack::Subscribing, which is demand while held. So the lend never keeps a track open. My first version held a strongtrack::Producerclone. That kept the media track open past its owner's handle and changed teardown order:moq-hls'srecord_cursors_yield_renditions_and_segmentssawCatalogFailedbeforeRecordsFailed.rs/moq-net/src/model/track.rs:TrackState::advancewoke the demand waiters while holding the track-state write guard. It was the only track path doing so, against kio's "release before waking" rule thatjoinalso follows. Because the lend polls inline from its waker, a join over the record track's refresh ratio deadlocked the next record publish (found by review).advancenow defers its wakes into the state, and its five guarded callers fire them after dropping the guard.container::Producer::with_recorder, and the fMP4 passthrough's enrollment. Every catalog-enrolled video/audio track gets it, not only the CLI.No public API change and no wire change. The lend's inline poll depends on every wake of a track's waiters happening after its lock is released, which kio and moq-net now both uphold. The
Lenddocs record that requirement.Tests
moq-muxhls::test::record_readers_want_the_media: record readers make the media wanted, the last reader leaving releases it, and dropping the recorder releases it.moq-muxhls::test::a_refreshing_join_keeps_lending: puts a reader on the records, passes the refresh ratio, joins late, and publishes the record that answers the join with a fresh group. It hangs (nextest's 60s timeout) with the moq-net change reverted, and passes with it.moq-videoencode::producer::tests::control::an_hls_only_viewer_starts_capture: runs the real capture drivermoq publishuses, against a synthetic camera. The only subscriber is amoq_mux::hls::Consumeron the advertised records. The camera opens and the first segment record arrives.moq-videoandmoq-audiohls_records_keep_the_live_timestampsheld a record reader across an idle restart. Under the new semantics that reader keeps capture running, so both now read the records after the runs. What they cover (idle gap left out of the segments) is unchanged.Choices made without asking
with_recorder+ fMP4 enrollment, not by changingTracks::track/catalog::Producer::enrollto take the media track. Binding at enrollment would be marginally harder to miss, but it would change ~20 name-based test call sites for the same two production paths.Subscription::default()(priority 0, zero delay budget), the same assubscribe(None)elsewhere. On a publisher-local track the aggregate isn't forwarded anywhere.Not covered: audio records aren't advertised until audio has run
moq-audiocapture enrolls its rendition without an HLS hint, so itshlscatalog field appears only after its first segment. An HLS-only viewer of an A/Vmoq publishtherefore finds only the video records. Video starts now, but audio's records stay unadvertised, so audio never starts and the export lists no audio rendition. This has a separate, pre-existing cause; once audio records are advertised, the demand path in this PR starts audio too. Audio groups have a fixed cadence (Options::group_duration), which is exactly what a hint declares. Two catches:catalog.reserve().with_hls_hint(..)would make the rendition withhold the catalog until layout discovery, which audio capture deliberately avoids.moq-muxAPI.So it's suggested as a follow-up below rather than done here.
Suggested follow-ups
moq-audio's capture (rs/moq-audio/src/encode/producer.rs,Reserved::new→catalog.audio(..)) should pass its fixed group cadence as an HLS hint, so the catalog names its record track before audio runs. That needs a way to hint a rendition that doesn't withhold the catalog, e.g. a hint oncatalog::Producer::audio/track, or aReservedvariant that doesn't gate. Then extendan_hls_only_viewer_starts_capture(or an audio twin) to an HLS-only viewer of audio capture.moq-mux.cargo doc -p moq-mux --document-private-items -D warningsfails on pre-existing links (catalog/consumer.rsmsf,catalog/producer.rsRenditionConfig::insert,catalog/tracks.rsInit::label,container/producer.rsReserved). CI doesn't build private docs, so they don't fail it.Checks run locally (all through
quest gate)just rs test -p moq-net -p moq-mux -p moq-hls -p moq-relay: 5532 passedjust rs test -p moq-video -p moq-audio -p moq-scene -p moq-studio -p moq-cli --features moq-video/capture,moq-video/openh264,moq-audio/capture,moq-audio/playback,moq-scene/capture,moq-studio/capture: 841 passedcargo fmt --all -- --check,RUSTDOCFLAGS="-D warnings" cargo doc -p moq-mux --no-deps,quest check: clean(
just rs testenforces clippy throughclippy-driver.)🤖 Generated with Claude Code
(Written by Claude Opus 5.5)