Skip to content

fix(moq-mux): refuse Opus TS tracks the channel code cannot label - #4981

Merged
kixelated merged 3 commits into
mainfrom
quest/m1/ts-opus-export-refusals
Oct 7, 2026
Merged

kixelated merged 3 commits into
mainfrom
quest/m1/ts-opus-export-refusals

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

MPEG-TS export derived an Opus channel_config_code from the catalog channel count and clamped it to 1..=8. A family 255 head, an ambisonic head, a family 1 table that is not the Vorbis mapping, or more than eight channels was written with a code naming a layout the packets do not have. A track with no OpusHead and more than two channels was labeled the same way.

Approach

opus_channel_code reads the OpusHead. Family 0, and family 1 with the Vorbis mapping, keep the plain code equal to that channel count, and the count must match the catalog. A missing head is mono or stereo only. Every other layout is refused. The exporter does not write 0x81, 0x80 | channels, or 0xff.

The regression test publishes real heads, checks the PMT code, and checks the count ffprobe reads. Refused cases are the ones the old clamp mislabeled.

Impact

  • Public API: none.
  • Wire: none. MPEG-TS output is unchanged for a head the extension descriptor can name. Tracks it cannot label are refused instead of written with a guessed code.

Alternatives

Writing the explicit layout (0x81) or ffmpeg's 0x80 | channels waits on the import side. That quest has not settled which codes to parse, and ffmpeg's demuxer does not read 0x80 | channels. 0xff is itself a guessed code. Refusal is the label that does not lie.

A catalog count that disagrees with the head is refused, matching fMP4, rather than labeling from the head alone.

Follow-ups

  • When quest/m1/ts-opus-channel-codes parses 0x81, export should write that explicit layout, or pass a captured descriptor through, for a head the descriptor can then name. Recommendation: do that with the import change. Keep refusing 0x80 | channels and 0xff until a demuxer reads them.
  • .worktrees/ is untracked and not ignored, so a local just check fails biome on nested biome.jsonc roots. .gitignore already excludes /.claude/worktrees/ for this reason. Recommendation: ignore /.worktrees/ the same way, in its own change.

(Written by Grok 4.7)

kixelated and others added 2 commits October 7, 2026 00:10
Co-Authored-By: Grok 4.7 <noreply@x.ai>
@kixelated
kixelated marked this pull request as ready for review October 7, 2026 15:45
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3d1d30d3-8b88-4524-9738-b72e0001d4da
📥 Commits

Reviewing files that changed from the base of the PR and between 03529eb and 7445f04.

📒 Files selected for processing (7)
  • doc/bin/cli.md
  • doc/lib/rs/moq-mux.md
  • quest/m1/README.md
  • quest/m1/ts-opus-channel-codes.md
  • quest/m1/ts-opus-export-refusals.md
  • rs/moq-mux/src/container/ts/export.rs
  • rs/moq-mux/src/container/ts/export_test.rs
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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

@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok review of 7445f04d (first review; this head is 27ffe3d0 plus a clean merge of main)

This fixes a real problem. Before, the exporter clamped the catalog channel count into 1..=8, which mislabeled family 255, ambisonic, and non-Vorbis family 1 heads. Now opus_channel_code keys off the parsed OpusHead. I checked it against opus::Config::parse and Mapping. Family 0 with more than 2 channels is already refused by parse. Mapping's derived PartialEq compares the zero-padded table, so vorbis(n) == mapping is an exact layout match. The u8::try_from and 1..=8 guards are unreachable, but harmless, once the mapping check passes. Every in-tree Opus importer (TS, fMP4, MKV, moq-audio encode) publishes a real OpusHead as description, so none of them hit the new parse error. No blocking issues.

Non-blocking

  1. A head that arrives late now fails the export instead of waiting. audio_kind runs on every catalog snapshot until the PMT is built (export.rs ~1218), and its ? aborts the whole export, video included. A 3+ channel Opus rendition whose first snapshot has no description now errors on that snapshot, even if the next snapshot would carry a Vorbis head. The fMP4 exporter treats a missing Opus head as provisional and settles it later. I haven't confirmed an in-tree publisher that does this for multichannel, so treat it as low risk. If it matters, defer the decision until the PMT is about to be built, or drop just that track with a warning. The import-side quest already asks for per-stream refusal "without failing the rest of the program".
  2. The test now needs ffprobe on PATH. ffprobe_opus_channels (export_test.rs ~3875) is the first moq-mux unit test that spawns an external binary, and it panics with "ffprobe is in the dev shell" when the binary is missing. CI's Test job runs inside nix develop, and ffmpeg is in the flake, so CI is fine. A plain cargo test -p moq-mux outside the dev shell will fail, though. Consider skipping the ffprobe assertions, with a note, when ffprobe isn't found. The PMT channel_config_code check already covers the logic.
  3. The export follow-up now lives only in the PR body. This PR deletes quest/m1/ts-opus-export-refusals.md and the "Related" link from ts-opus-channel-codes.md, so nothing in the repo says export should write 0x81 once import parses it. Consider adding one line to ts-opus-channel-codes.md's plan so the follow-up isn't lost.

CI (Check, Test, Quest, Windows, macOS) was still pending on this head when I reviewed it.

Verdict: MERGE once CI is green.

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

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: 7445f04

No new actionable findings beyond the existing discussion. I traced OpusHead parsing, exact Mapping equality (including family/stream counts/table), catalog-count agreement, and PMT descriptor generation. The accepted mono/stereo and canonical Vorbis cases are consistent; nonrepresentable layouts now fail rather than receive a misleading code. Direction: sound fail-closed correction with useful positive/refusal coverage.

Verification limits: static review of the full change and parser/mapping context; the ffprobe integration and Rust tests were inspected but not run. Existing late-description behavior and test-environment observations are not duplicated here.

@kixelated

Copy link
Copy Markdown
Collaborator Author

On the Grok review of 7445f04d (no code change; CI is green on this head):

  1. A late head fails the export. Keeping it. No in-tree publisher sends a 3+ channel Opus rendition without its OpusHead, and refusing loudly beats guessing a code. If a publisher like that shows up, the fix is per-track refusal at PMT build time, not a wait. That goes in the follow-ups.
  2. The test needs ffprobe. Keeping it. Tests run in the Nix dev shell, which ships ffmpeg, and CI runs them there. Skipping when the binary is missing would let a broken label pass on a machine that lacks it.
  3. The export follow-up lives only in the PR body. I'm not adding it to quest/m1/ts-opus-channel-codes.md, because fix(mux): import TS Opus channel codes above 0x80 #4990 completes that quest and deletes the file. The follow-up (export writes the explicit 0x81 layout once import parses it) belongs with fix(mux): import TS Opus channel codes above 0x80 #4990's follow-ups, or a new quest once that lands. It stays listed here.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merging: the OpenAI review of 7445f04 (the head) has no new findings and CI is green. #4990 lands after this and deletes both TS Opus quest files.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 7, 2026 23:20
@kixelated
kixelated merged commit 7ba4b8f into main Oct 7, 2026
8 checks passed
@kixelated
kixelated deleted the quest/m1/ts-opus-export-refusals branch October 7, 2026 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant