Repository navigation
fix(hls): keep a reconfigured rendition's segments in its playlist - #7436
Conversation
Quest-Claim: 5162f355363d0c78
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
The interleaving fix makes the export replace the instance; the predicate already suffices. The remaining failures are in the instance handoff, so the quest needs re-scoping before implementation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A same-codec reconfigure replaced the rendition's instance and dropped its window from the playlist URI at once. The replacement now keeps listing its predecessor's segments, under their own init and URLs, until the window slides past them, and lists its own behind an EXT-X-DISCONTINUITY and a new EXT-X-MAP (RFC 8216 6.2.2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An empty retired instance passes on what it kept instead of stacking, an empty predecessor is kept until it can list nothing more, and a segment URL reaches the instance whose window holds it, even when a returning config shares its tag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Design change: this quest is re-scoped from "treat a same-codec decoder change as a new instance" (already true on
The first half is implemented in (Written by Claude Opus 5.5) |
The playlist fix lands here; the hand-over pick waits on the plan to remove the catalog's hls field (maintainer, 2026-11-02). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The merged hand-over tests slept on tokio's clock, which main's clippy config now disallows, and moq-relay re-exported Io unused without the websocket feature (as moq-cli builds it). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts first Review round 1: rows skipped across a hand-over left inherited rows ahead of a jump, so media sequence numbers shifted once they slid out. And an old playlist's init request built the replacement's init first, which can block on media. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 2: a denied predecessor refused init requests for hashes it doesn't hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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 9 minutes. View limit details
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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: cd87090
[P2] Serve an inherited-only playlist without requiring the replacement's init — rs/moq-hls/src/export/rendition.rs:853–858, 888–890. After an inline-parameter-set codec reconfigures, the retained predecessor can already have playable rows and a cached init while the replacement has no listed rows. playlist() nevertheless calls the replacement's load_init(): init_read() consults only self.live, so it returns None until a new segment is listed, and the playlist disappears despite all inherited rows already having valid maps. media_playlist() independently rejects the same case at the unconditional built_init check. Require the current init only when the snapshot actually contains current-instance rows, and serve the predecessor-only snapshot immediately. Add an inline-codec regression that builds the old init, reconfigures, withholds replacement records, and still receives the old media playlist. This is separate from the explicitly deferred first-group/config attribution race.
Direction: retaining and trimming one combined window, with per-instance maps and segment routing, is a reasonable bounded extension of the existing replacement path. The narrowed hand-over scope is explicit; no new wire marker is needed for this part.
Verification: static GitHub review of the changed code, surrounding initialization/retention paths, and regression tests only. I did not run tests, builds, or HLS playback/conformance checks.
There was a problem hiding this comment.
Important
An inline-parameter-set codec's replacement refuses to serve the predecessor's already-listed segments until it builds its own init, which it cannot do without listing a new segment. During that hand-over the media playlist goes empty, which is the gap this PR set out to close. This was raised in the prior automated review and remains unaddressed at b83bd44.
Reviewed changes
- Retains a retired instance's segments. A replacement on the same record track holds its predecessor (
Rendition::previous) and lists its rows ahead of its own, trimmed as one window, until they slide out. - Per-segment init maps.
playlist::Segmentgainsmap;render_mediaemits a newEXT-X-MAPafter the discontinuity wherever the map changes, so each predecessor segment stays under the init it was muxed for. - Ordered hand-over. The replacement's records watcher waits for the predecessor's window to close before subscribing, so rows never interleave and the new config's first row starts a discontinuity.
- Bounded.
retirepasses on a predecessor that neither lists nor waits on,inheritedreleases ancestors once no rows remain, andrestartdrops them. - Routing.
Rendition::segmentandRendition::initresolve a predecessor's URLs while listed. - Tests + build. New regression tests;
moq-relaygates itsIore-export behindwebsocket.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
An inline-parameter-set replacement builds its init only from a segment of its own, but `playlist()` and `media_playlist()` required it before rendering, so the predecessor's segments vanished for the whole hand-over. Require an instance's init only when the listed rows map it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The DASH manifest lists only the current instance's segments, but its readiness waits counted a predecessor's inherited rows, so a manifest loaded during a reconfigure dropped the replacement's video. Wait on the instance's own rows there (`represented`), keeping inherited readiness for the HLS playlist. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 22f6e32
No new actionable bugs found. The prior P2 inherited-only playlist/init finding is fixed: only instances contributing listed rows require an init, with coverage for serving predecessor rows before the replacement has media and then switching to its own map. This also addresses the identical Pullfrog finding.
Direction: separating DASH's own-row readiness from HLS's inherited-row readiness is appropriate, and the focused regression tests cover the distinction without changing the retained-window design. The deferred first-group/config attribution race remains outside this fix.
Verification: static review of the two substantive fixes, surrounding initialization/readiness paths and tests, accounting for merged base changes. No builds, tests, or HLS/DASH playback checks run.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the two commits since the prior pullfrog review at b83bd44, both fixes to the reconfigure hand-over:
- Serve inherited segments before the replacement's own init.
media_playlistnow requires the current instance's init only when a listed row maps it (an own row is present, or the snapshot is empty), andplaylistbuilds only the inits the listed segments map, re-rendering if the instance lists its first segment mid-flight. An inline-parameter-set replacement therefore serves its predecessor's segments during the hand-over instead of returningOk(None). This addresses the prior review's [IMPORTANT] thread. - Wait for the replacement's own segment before a DASH manifest. New
Rendition::represented/poll_representedcount only the instance's own rows;renditions::video_initand the manifest route now use them while the HLS playlist keeps inherited readiness, so a manifest loaded during a reconfigure does not reflect a predecessor's inherited rows. - Regression tests.
an_inherited_playlist_needs_no_init_of_its_ownanddash_readiness_waits_for_the_replacements_own_segment.
I verified render_media only consumes the init argument for rows whose map is None, so the empty placeholder passed when every listed row is inherited is never written into an EXT-X-MAP. cargo check -p moq-hls --no-default-features passes without warnings. The new tests could not be executed here (the workspace-hack dev-dependency pulls libspa/libpipewire, absent in this environment), so they were reviewed statically.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
quest land watch: the merge queue removed this pull request at 22f6e32 because it conflicts with The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs. (written by quest land watch) |

Problem
When a rendition changes resolution under the same codec string, as
video1does in the conformance test of moq-dev/moq#7387, the export already starts a new instance with a new init and segment tag. The hand-over between the two instances still loses or mislabels segments:Renditions::syncdropped the retired instance from the playlist URI at once. The replacement started with an empty window, so the segments already listed disappeared, which RFC 8216 6.2.2 forbids.This PR fixes (1) only. (2) stays in the quest, which waits on the plan to remove the catalog's
hlsfield.Decisions
hlsfield (rs/hang/src/catalog/hls.rsand the JS zod schema). That removal is planned separately via/quest-plan, not here. That makes the open marker decision moot, so it is superseded and none of its options is built:startfield naming the first record of the new config: not chosen.The quest is rewritten to the remaining hand-over race, which is designed after the removal plan.
EXT-X-MAP, behindEXT-X-DISCONTINUITYinstead of jumping (RFC 8216 6.2.2). The pick of the hand-over group must not depend on which track's data arrives first. See the design-change comment. The 2026-11-02 decision narrows what lands here to the first half.Changes
Rendition::previous). Its media playlist lists the predecessor's rows first, trimmed as one window with its own, until the window slides past them. Then it releases the predecessor. Each predecessor segment carries the hash of the init it was muxed for, andrender_mediawrites a newEXT-X-MAPwherever that changes.Rendition::segmentroutes a URL to the instance whose window holds it, matching on tag and position, since a config that returns gets its old tag back.Rendition::initalso serves a predecessor's init.playableandplaylistcount the predecessor's rows and wait for its init.set_generation) drops them.doc/bin/hls.mdsays so.hlsremoval plan. Resized [L] to [M], since only the pick remains.playlist()andmedia_playlist()required it, so the predecessor's segments vanished for the whole hand-over. Now an instance's init is required only when a listed row maps it. Found by the OpenAI and pullfrog reviews (thread).Broadcaster::playable, renamedrepresented, andvideo_init) counted inherited rows. A manifest loaded during a reconfigure answered 404, or kept audio only, and a DASH player keeps the adaptation sets of the first manifest it loads. They now wait onRendition::represented, which counts only the instance's own rows. The HLS waits keep the inherited readiness. Found in review.main(no conflicts). After the merge, main's lints failed the build: the tests slept on tokio's clock (nowkio::clock::sleep).moq-relayalso re-exportedlistener::Iounused without itswebsocketfeature, as moq-cli builds it. That fails anyjust checkthat selects moq-cli or moq-hls, and no open PR fixed it, so it is gated on the feature here.Impact
Rendition::segmentandRendition::initnow resolve a predecessor's URLs too.Choices made without asking
playlist()waits for it.media_playlist()answers "not ready" while it is missing, so a player never gets anEXT-X-MAPthat 404s.Removed, thenAdded).a_reconfigure_versions_the_init_urlnow expects the old init URL to keep resolving while its segments are listed: the hash is content-addressed, so it always names the old bytes.Requiredentry, since the removal plan has no quest yet. Its Plan says to add one once the plan lands.Suggested follow-ups (for
/quest-plan)Rendition::set(rs/moq-mux/src/catalog/tracks.rs) on an already-published rendition should floor unsupplied jitter and delay at the catalog's current values, asRendition::estimatedoes. Otherwise an H.264 or H.265 same-identity reconfigure after any raise fails withJitterDecreasedand aborts the TS import. It blocks the conformance test in moq-dev/moq#7387 once its fixture is repaired.hlsremoval is now planned as an m0 quest in moq-dev/moq#7616 (quest/m0/hls-media-grid.md), which lists this quest under Related. Once it lands, this quest shouldRequireit or fold the remaining hand-over pick into it. The retention this PR adds (each listed segment under its own init, routing, DASH listing only the current instance) still applies under the grid: only the record-track hand-over (Handoff.next, the records watcher wait) is rewritten.Checks
just check origin/main(insidenix develop, throughquest gate): pass at 22f6e32.quest check --base origin/main: pass.an_inherited_playlist_needs_no_init_of_its_own,dash_readiness_waits_for_the_replacements_own_segment(each fails without its fix),a_reconfigure_keeps_serving_its_predecessors_segments,a_predecessors_pending_first_segment_is_kept,reconfigures_listing_nothing_hold_nothing,a_returning_config_serves_both_instances_segments,playlist::tests::a_predecessor_keeps_its_own_map, andrendition::tests::a_numbering_jump_cuts_the_inherited_rows.Review
mechanical fix by quest land watch:
merged main at d5cf1154d3be75dbda9543eb3194adc1bded6d5d with the quest merge driver(head 33c7a8f)Codex (
gpt-6.1-sol, medium effort) viaquest review --base origin/main, in a new session after the pullfrog finding. Round 2 reviewed head 22f6e32 and reported no findings.video_initandBroadcaster::representedwait on the instance's own rows.Earlier sessions: the 2026-11-02 session (3 rounds, through head cd87090) fixed the numbering-jump bridge, the predecessor init blocking on the replacement's build, and the init authorization. The first session (3 rounds, through head e392601) fixed the empty-instance churn, returning-tag routing, and the closed-read race. The OpenAI review of cd87090 and pullfrog's of b83bd44 both reported the inherited-init gap fixed above.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code