quest(m1): plan #4456, #4508, #4581, #4582 #4587
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # [XS] Android logs always go to logcat | ||
|
|
||
| ## Goal | ||
|
|
||
| On dev, `moq_tokio::Log::init` sends logs to logcat on every Android build, so | ||
| the published Kotlin and Dart bindings (and libmoq) stop losing every Rust log | ||
| to stderr, which Android app processes discard. The `android-logcat` Cargo | ||
| feature is gone from `moq-tokio` and `moq-ffi`. | ||
|
|
||
| ## Plan | ||
|
|
||
| Today the logcat layer in `rs/moq-tokio/src/log.rs` is gated on | ||
| `all(target_os = "android", feature = "android-logcat")`, and no shipped build | ||
| enables the feature: libmoq has no `[features]`, and `rs/moq-ffi/build.sh`, the | ||
| Dart build hook, and the release workflows build Android without it. The | ||
| published `dev.moq:moq-ffi-android` 0.4.8 does not link `liblog`. | ||
|
|
||
| Decided (maintainer, 2026-09-30): gate the layer on `target_os = "android"` | ||
| alone and delete the feature, rather than wire it through every build, so no | ||
| Android consumer has to know to turn it on. Logging stays opt-in at runtime, | ||
| since `Log::init` runs only when the app calls it. Removing a feature from | ||
| published crates is a break, so this targets dev; no no-op feature is kept. | ||
|
|
||
| - `tracing-android` becomes a plain dependency under the existing | ||
| `cfg(target_os = "android")` target table in `rs/moq-tokio/Cargo.toml`; it | ||
| must stay target-gated because `android_log-sys` links `liblog` | ||
| unconditionally. | ||
| - Fix the `Log::init` doc comment, which already claims logcat. | ||
| - Leave the `rs/moq-native` tombstone alone. | ||
| - Regression check: the Android CI workflow builds moq-ffi for one Android | ||
| target and asserts the `.so` imports `__android_log_*`. | ||
|
|
||
| The reporter ([#4456](https://github.com/moq-dev/moq/issues/4456)) has this | ||
| working locally and offered the PR. | ||
|
|
||
| Public API: breaking on dev, the `android-logcat` feature is removed from | ||
| `moq-tokio` and `moq-ffi`. Wire: none. | ||
|
|
||
| ## Closes | ||
|
|
||
| - [#4456](https://github.com/moq-dev/moq/issues/4456) - Rust logs never reach logcat on Android |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # [M] A signalled TS restart continues as a new epoch | ||
|
|
||
| ## Goal | ||
|
|
||
| On dev, when a TS feed rewinds its time base and signals it | ||
| (`discontinuity_indicator` on the PCR PID, ISO/IEC 13818-1 2.4.3.5), | ||
| `moq import ts` and `moq-srt` finish the current broadcast cleanly and publish | ||
| the rest of the same input as a new broadcast under a fresh epoch, in the same | ||
| process and connection. This covers an encoder restart or source switch behind | ||
| a gateway that keeps its connection up, and a looping playout server, none of | ||
| which makes a new connection for [Gateways](/quest/m1/broadcast-epoch/gateways.md) | ||
| to turn into an epoch. An unsignalled rewind stays fatal, as #4543 decided. | ||
|
|
||
| ## Plan | ||
|
|
||
| Since #4543 (on dev), every rewind ends the import with `TimestampRewind` from | ||
| `Producer::write`; the importer already reads the flag in `timebase_break`, | ||
| and a flagged forward jump publishes break markers and carries on. | ||
|
|
||
| Decided (maintainer, 2026-09-30): | ||
|
|
||
| - A rewind is new content, so it is always a new broadcast at a new epoch | ||
| path, never a continuation of the old name. Viewers of the bare name follow | ||
| it through [Origin](/quest/m1/broadcast-epoch/origin.md). | ||
| - `decode` stops at the flagged rewind and reports it. The caller finishes the | ||
| old broadcast (a clean end, not an abort, so its viewers read to its end), | ||
| publishes a new broadcast at a fresh epoch path, and calls | ||
| `import.restart(broadcast)`. The importer carries over only the PAT/PMT | ||
| layout and the bytes it has not consumed, so there is no wait for the next | ||
| PSI repetition; tracks, groups, and timestamps start fresh. `ts::Programs` | ||
| does this per program. Returning unconsumed bytes to a fresh `Import` was | ||
| rejected for the PSI wait, and letting the importer publish its own | ||
| broadcast was rejected for coupling moq-mux to origin publishing. | ||
| - A flag with the next PCR behind the last one is a restart; a flag with a | ||
| forward jump stays a break marker. The 33-bit wrap is modular, not a rewind. | ||
|
Comment on lines
+34
to
+35
There was a problem hiding this comment.
When the discontinuity packet does not itself carry a PCR, the direction is unknown until a later PCR, and valid media packets may occur in between. The current importer explicitly supports a dedicated PCR PID and adaptation-only clock packets ( Useful? React with 👍 / 👎. |
||
| - Callers: `moq import ts` (`rs/moq-cli`) and `moq-srt`'s `Publisher`, single | ||
| program and `Program::All`. | ||
|
|
||
| Tests: a fixture with a flagged rewind publishes two broadcasts, the second | ||
| starting at the rewound PTS; the same rewind unflagged still errors; one SRT | ||
| connection carries both epochs. Update `doc/bin/cli.md` and `doc/bin/srt.md`. | ||
|
|
||
| Public API: breaking in moq-mux on dev, `ts::Import::decode` reports a restart | ||
| and `restart` is new. Wire: none. | ||
|
|
||
| ## Required | ||
|
|
||
| - [Origin](/quest/m1/broadcast-epoch/origin.md) - the fresh epoch path the rest of the feed publishes under | ||
|
|
||
| ## Closes | ||
|
|
||
| - [#4582](https://github.com/moq-dev/moq/issues/4582) - a signalled backward TS discontinuity ends the import | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # [M] A graceful close waits for the subscriber to read the tail | ||
|
|
||
| ## Goal | ||
|
|
||
| When `Session::close()` returns `Ok` on moq-lite-07, every subscriber has read | ||
| each finished track to its end: every frame of the final group, then a clean | ||
| end. Today close counts a group delivered once the peer's QUIC stack acks it, | ||
| then sends `CONNECTION_CLOSE`, and a real QUIC stack discards stream data the | ||
| subscriber's moq-net has not read yet. The subscriber loses the final group and | ||
| sees `Session(Cancel)`, while the publisher is told the close succeeded. | ||
|
|
||
| ## Plan | ||
|
|
||
| Root cause: a lite serve decrements the publisher's `owed` count once its | ||
| group streams and Subscribe Stream FIN are acked (`poll_close` in | ||
| `rs/moq-net/src/lite/publisher.rs`), and `Publisher::drained()` reads only that | ||
| count. `poll_drain` in `rs/moq-net/src/session.rs` then closes the session with | ||
| `Cancel`. The lost final group is unread data the subscriber's QUIC stack | ||
| discards on `CONNECTION_CLOSE`; the subscriber's abort then ends the track with | ||
| that group still open. `tests/session_close.rs` passes only because the mock | ||
| transport keeps unread data after close. | ||
|
|
||
| Decided (maintainer, 2026-09-30): | ||
|
|
||
| - The fix is a FIN handshake, per subscription. On lite-07 the subscriber FINs | ||
| its side of the Subscribe Stream once its tail accounting settles the track's | ||
| end, never earlier: every group below the end has been read to its FIN, | ||
| reset, or dropped. Today that accounting is SUBSCRIBE_END's `Stream Count`; | ||
| once [SUBSCRIBE_DROP](/quest/m1/subscribe-drop.md) lands it is a received | ||
| group or a SUBSCRIBE_DROP per sequence. The FIN rule rides on whichever is in | ||
| place, so neither quest waits on the other. The publisher's | ||
|
There was a problem hiding this comment.
When an ordinary QUIC reset loses its group header, the subscriber cannot satisfy the Stream Count until the tail grace expires. That grace is one second in Useful? React with 👍 / 👎. |
||
| drain waits until each served Subscribe Stream is closed both ways (the | ||
| subscriber's FIN or a reset), under the same `CLOSE_TIMEOUT`, before | ||
| `CONNECTION_CLOSE`. A session-level GOAWAY handshake was rejected: the peer | ||
| would need the same per-subscription knowledge to know when to close. | ||
| - lite-07 only, since it is still wip and the draft can require the | ||
| subscriber FIN there. On lite-05 and lite-06 an old subscriber never FINs, | ||
| so close keeps today's ack-based drain rather than time out every close. | ||
| #4508's repro is on lite-05, and that path stays as it is: this quest closes | ||
| the issue by fixing the close on the version that can carry the FIN rule. | ||
| IETF sessions stay with [IETF drain before close](/quest/m1/ietf-drain-before-close.md). | ||
| - Both subscribers change: moq-net and `@moq/net`. | ||
|
|
||
| Update `drafts/draft-lcurley-moq-lite.md` (the Subscribe section gains the | ||
| subscriber FIN rule, with a changelog entry) and `doc/concept/moq-lite.md` | ||
| (a subscription ends with both sides' FIN, and a graceful close waits for it). Check that a lite-07 publisher | ||
| already treats a subscriber FIN after SUBSCRIBE_END as the end of a finished | ||
| subscription, not a cancel of one still in flight | ||
| ([Request stream cancel](/quest/m1/request-stream-serve.md)). | ||
|
|
||
| Tests: the reporter's `close_tail` case (one session, paused clock, a mock | ||
| switch that acks a FIN as soon as it is sent, as a real transport does) fails | ||
| today and passes with the fix; a subscriber that never FINs makes close return | ||
| `Error::Timeout`; a final range with a skipped and a reset group still settles | ||
| and FINs. Run `just test interop --all`, since both the Rust and JS subscribers change; | ||
| [Track tail interop](/quest/m1/track-tail-interop.md) is the cross-language | ||
| proof over a real relay. | ||
|
|
||
| Public API: none. Wire: on lite-07 a subscriber FINs its Subscribe Stream | ||
| after reading the track's end, and a publisher closing gracefully waits for it. | ||
|
|
||
| ## Closes | ||
|
|
||
| - [#4508](https://github.com/moq-dev/moq/issues/4508) - `close()` returns `Ok` while the subscriber gets a cut track | ||
|
|
||
| ## Related | ||
|
|
||
| - [IETF drain before close](/quest/m1/ietf-drain-before-close.md) - the same drain for moq-transport sessions | ||
| - [SUBSCRIBE_DROP](/quest/m1/subscribe-drop.md) - replaces the lite-07 tail accounting the FIN rule waits on | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # [M] TS import drops a damaged unit instead of ending the ingest | ||
|
|
||
| ## Goal | ||
|
|
||
| `moq import ts`, and `moq-srt` through the same importer, survive a packet, | ||
| PES, or access unit they cannot parse: a malformed PES header or media-PID | ||
| adaptation field with `transport_error_indicator` clear, or a codec error such | ||
| as an H.264 NAL with `forbidden_zero_bit` set. The damaged unit is dropped and | ||
| counted, its track waits for its next keyframe, and the ingest carries on. | ||
| Today the first such error ends the import, and the gateway hangs up. | ||
|
|
||
| ## Plan | ||
|
|
||
| Today `decode` in `rs/moq-mux/src/container/ts/import.rs` runs | ||
| `while let Some(packet) = self.reader.read_ts_packet()? { self.handle_packet(packet)?; }`, | ||
| so any PES-header error from the reader and any codec error from `flush` ends | ||
| the import. The `transport_error_indicator` branch already drops a flagged | ||
| packet (clears `pending[pid]` and calls `stream.desync()`), and it is the model. | ||
|
|
||
| Decided (maintainer, 2026-09-30): | ||
|
|
||
| - Fail-loud holds at unit granularity, as [TS PSI reassembly](/quest/m1/ts-psi-reassembly.md) | ||
| holds it at section granularity: a damaged unit is refused whole, never half | ||
| published, and counted, and nothing else about the feed changes. PSI damage | ||
| stays that quest's. | ||
| - Drop the unit the way the TEI branch does: clear the PID's pending PES and | ||
| desync its stream, so a codec that needs one waits for the next keyframe. | ||
| Make sure the scratch buffer still advances past the packet on this path | ||
| (today an error skips `self.scratch.drain(..off)`). | ||
| - Count each drop in a new cumulative per-PID `damaged` counter on the stream's | ||
| stats row, beside `resyncs` and `discarded`, reported by `ts::stats::Log`. | ||
| It names what happened; no TR 101 290 check matches a codec error, and | ||
| [TS import health](/quest/m2/ts-import-health.md) keeps the ETSI names. | ||
| The rows are `#[non_exhaustive]`, so the field is additive; follow | ||
| [TS stats module](/quest/m1/ts-stats-module.md)'s names if it has landed. | ||
| - Errors that are not confined to one unit (the producer refusing a rewind, | ||
| origin or catalog failures) stay fatal. | ||
| - Built on the demux TS PSI reassembly owns, since that quest moves PES header | ||
| parsing into moq-mux. | ||
|
|
||
| Tests, from [#4581](https://github.com/moq-dev/moq/issues/4581)'s stimuli on a | ||
| fixture: a video PES header with its flag and timestamp bytes zeroed (TEI | ||
| clear), and one H.264 NAL with `forbidden_zero_bit` set. Each import carries | ||
| on, counts exactly one `damaged` on that PID and none elsewhere, and publishes | ||
| again from the next keyframe; a clean fixture counts zero. Document the counter | ||
| wherever the TS stats fields are described. | ||
|
|
||
| Public API: additive, one stats field. Wire: none. | ||
|
|
||
| ## Required | ||
|
|
||
| - [TS PSI reassembly](/quest/m1/ts-psi-reassembly.md) - moq-mux owns the demux and PES header parsing this drops through | ||
|
|
||
| ## Closes | ||
|
|
||
| - [#4581](https://github.com/moq-dev/moq/issues/4581) - one malformed packet ends the TS import | ||
|
|
||
| ## Related | ||
|
|
||
| - [TS import health](/quest/m2/ts-import-health.md) - the TR 101 290 counters for the same feed |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ts::Importis constructed with both a broadcast andcatalog::Reserved, then retains the reservation's catalog producer and binds SI capture to the original broadcast. On every restart, passing only the new broadcast leaves catalog tracks and future catalog edits attached to the old epoch, so the fresh epoch cannot publish the promised fresh catalog and tracks. Specify thatrestartalso receives a newly constructed catalog reservation, or define an explicit catalog-rebinding API and update each caller accordingly. (Written by GPT-5.6 Sol)AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.