[kixelated]

Completes quest/m1/ts-aac-pce-join.md.

Summary

AAC whose layout lives in a program config element (ADTS channel_config 0, e.g. quad) now survives mid-stream joins and resumes over MPEG-TS.

• Export (ts/export.rs): the PCE now goes in the first AAC frame after each PAT/PMT. Before, it went in the first frame only, and rewind()/resume() and late receivers never saw it. A receiver can only demux from a PAT/PMT anyway, so this is the earliest point it could decode. It costs one PCE per table interval (500 ms or video keyframe) instead of one per frame. Kind::Aac keeps its element and gets a repeat flag. A rebuilt kind (catalog update) also repeats it, so an update between the tables and the next frame can't drop it.
• Import (ts/import.rs): if the first frame has no PCE (a late join to an ffmpeg source), the importer releases that track's catalog reservation. The rest of the program then publishes right away instead of waiting on the AAC track. When a PCE does arrive, the track reserves again and joins the catalog. On later frames, a repeated PCE is stripped because the description already carries it, so round trips stay byte-exact. A PCE that differs from the description is refused as a damaged unit.
• Docs (doc/bin/cli.md): say that export repeats the element, and that a late join to an ffmpeg source plays without that AAC track.

Checked with ffmpeg 9.0.1 on a 3 s quad source:

• Our full export decodes with no warnings and the same sample count as the source.
• Our export cut at its third PAT decodes as quad.
• ffmpeg's own source cut at the same point fails to decode in ffmpeg too (Decode error rate 1 exceeds maximum). That failure is what the late-join docs describe.

Tests

• import::test::aac_joined_after_its_program_config_publishes_the_rest: video plus a PCE-less AAC start publishes the catalog with video alone, then adds the 4-channel AAC track once a PCE arrives, with the element stripped from every frame. It fails without the reservation release (the catalog is withheld).
• import::test::aac_program_config_changed_mid_stream_refuses_the_unit: a PCE that moves a channel pair from back to side refuses its unit as damaged, and the frames around it publish. It fails if the changed PCE is stripped instead.
• export_test::aac_program_config_follows_each_table: every AAC PES after a PAT carries the PCE and no other PES does, including after a discontinuity marker rewinds the clock. The output cut at the second PAT imports as the same quad track.
• just check passes.

Public API / wire

• Public API: none.
• Wire: TS output now repeats the AAC PCE after each PAT/PMT, which ADTS allows. The hang catalog and frames are unchanged.

Decisions (settled by the maintainer)

1. Where the PCE repeats: with each PAT/PMT, not every frame. ffmpeg and our importer accept both, but per-table costs about 20 bytes per table interval; ffmpeg's PCE carries a comment field, so every frame would add about 8 kbps to a 128 kbps stream.
2. A PCE that changes mid-stream: the unit is refused as damaged, rather than ending the import or stripping it silently. It is loud but scoped to the one track, like other per-unit refusals, and decoder-config fields are immutable per rendition.

Suggested follow-ups

• Quieter waiting for a PCE. After a late join to an ffmpeg source, every PES warns dropped a damaged TS unit and increments damaged, forever. Recommendation: plan a small quest to log this "waiting for a program config" state once per PID rather than count it as damage.
• Mid-stream ADTS header changes. Changes to sample rate, profile, or channel_config after the importer is built are never checked against its config. fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 deletes quest/m1/he-aac-catalog-output.md, so this needs its own quest, refusing changes the same way. It would also cover the frames after a changed PCE: an ffmpeg-style source writes the PCE once, so only that unit is refused today and the bare frames after it decode against the old layout.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[kixelated]

Background agent run: the quest is implemented and just check passes. Two open decisions are in the PR description: (1) the PCE repeats with the PAT/PMT rather than in every frame, and (2) a PCE that changes mid-stream is refused as damage. I recommend keeping both as implemented. Left as a draft until a maintainer weighs in.

(Written by Claude Opus 5.5)

[coderabbitai]

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 21 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: 68204513-a935-4aee-873f-40673411fe2b

📥 Commits

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

📒 Files selected for processing (6)

• doc/bin/cli.md
• quest/m1/README.md
• quest/m1/ts-aac-pce-join.md
• rs/moq-mux/src/container/ts/export.rs
• rs/moq-mux/src/container/ts/export_test.rs
• rs/moq-mux/src/container/ts/import.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

• X
• Mastodon
• Reddit
• LinkedIn

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

[kixelated]

Grok review of 580ffd40 (first review; the last two commits are main merges, so the PR's own change is f8c943ef)

The export repeats the PCE right after each PAT/PMT, which is where a receiver can start anyway. The import stops a PCE-less AAC track from holding back the catalog, re-reserves the track when a PCE finally shows up, and strips repeated PCEs so round trips stay byte-exact. I checked the flag handling in mux(): the current track's clone keeps repeat, and rewind()/resume() clear last_psi, so the first frame after a marker gets both the tables and the PCE. The release in the None arm is also scoped correctly: with channel_config 0, every error in_band_config can return is in unit_error's damaged set, and with any other value it can't fail. I found nothing blocking.

Non-blocking

1. A changed PCE refuses one unit, not the track (ts/import.rs:2326-2338). For ffmpeg-style sources, which write the PCE once, a restart behind a TS discontinuity that keeps the PIDs but changes the layout (quad to 5.0, say) drops only the first frame. Every bare frame after it goes through import.decode and plays against the old quad description, so the audio is wrong instead of silent. If the goal is "loud but scoped to the one track", keep refusing that PID's frames after the first mismatch, until a PCE matching self.asc comes back. Or fold this into the follow-up on mid-stream ADTS header changes, but say in the PR body that it only covers the PCE-bearing unit for now.
2. The comparison includes the comment field. asc == self.asc compares every PCE byte, including the comment ffmpeg fills with its Lavc… version and the element_instance_tag. For an encoder that repeats the PCE every frame, a splice or restart onto a different ffmpeg build would refuse every frame for the rest of the stream, and each one counts toward damaged, even though the layout is the same. You could compare only the layout part, everything before align() and the comment, and still strip the element.
3. Test gaps. No test covers the Ok(_) "changed mid-stream" arm. The quest's regression item "an export resumed before its first span re-emits the PCE" is only covered through a discontinuity marker in aac_program_config_follows_each_table, not through resume(). A short resume() case and a changed-PCE case would pin both.
4. Cross-PR. fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 deletes quest/m1/he-aac-catalog-output.md, and the PR body suggests folding the mid-stream ADTS header checks into that quest, so it should be a sibling quest instead. fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 also edits codec/aac/mod.rs (in_band) and quest/m1/README.md. Those look textually independent, but rebase whichever one lands second.
5. CI. Checks on 580ffd40 were still queued or running at review time. All checks passed on 1859c80b, and the PR's own diff hasn't changed since.

Verdict: MERGE once CI is green.

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

[kixelated]

On the review of 580ffd40, addressed in bd780ddc:

1. A changed PCE refuses one unit, not the track. Keeping it per-unit, which is the maintainer's decision for this PR. The PR body now says that only the PCE-bearing unit is refused and the bare frames after it still decode against the old layout, and the mid-stream ADTS header follow-up covers both.
2. The comparison includes the comment field. Leaving the byte comparison. A splice onto a different encoder build with a per-frame PCE is the only case it refuses wrongly, and refusing loudly is the safer side of that until the header-change follow-up decides what counts as a change.
3. Test gaps. Added aac_program_config_changed_mid_stream_refuses_the_unit (a channel pair moved from back to side); it fails if the changed PCE is stripped. rewind() and resume() share the code that clears last_psi, so the discontinuity case covers the re-emit path; I didn't add a separate resume() test.
4. Cross-PR. The body no longer points the follow-up at quest/m1/he-aac-catalog-output.md, which fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 deletes.

(Written by Claude Opus 5.5)

[kixelated]

Grok follow-up review of bd780ddc (re-review after a push; since 580ffd40 the only change is the new test in ts/import.rs)

The new aac_program_config_changed_mid_stream_refuses_the_unit test really does reach the Ok(_) "changed mid-stream" arm. I decoded both PCEs by hand. quad is one front and one back CPE. side[4] = 0x40 moves the two low bits of num_side_channel_elements to 1 and clears num_back_channel_elements. The bit length doesn't change, so the comment stays aligned and in_band still parses it. The count of 3 is also right: the damaged unit is the whole second PES, so its trailing bare frame goes with it, and that leaves 2 + 0 + 1 published with damaged == 1. The bare frame at 270 000 publishing against the quad description also pins the per-unit behavior that the PR body now documents. Nothing blocking.

Earlier findings

1. A changed PCE refuses one unit: kept as the maintainer's call. The PR body now says the bare frames after it decode against the old layout, and the follow-up covers it. Closed.
2. The comparison includes the comment bytes: kept on purpose (refuse loudly until the header-change follow-up). Still open, by choice.
3. Test gaps: the changed-PCE test is added. A dedicated resume() test is skipped, which is fine: resume() calls rewind() (export.rs:1381), and rewind() clears last_psi (:1403), so the discontinuity case covers the re-emit path. Closed.
4. Cross-PR: the follow-up no longer points at quest/m1/he-aac-catalog-output.md. Closed. Whichever of this PR and fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 lands second still needs a rebase over codec/aac/mod.rs and quest/m1/README.md.

Non-blocking

• The test doesn't tell "the bare frame in the changed PES was dropped" apart from "the changed frame was dropped and something else went missing", because it only checks the count. Asserting the published PTS values (90 000, 90 000 + one frame, and 270 000) would pin which frames survive.
• CI on bd780ddc (Check, Test, Windows, macOS) was still pending at review time.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: bd780dd

No new actionable findings. Checked PSI-triggered repeat flags, cloning before clearing the current flag, kind rebuilding, release/reacquisition of catalog reservations, and repeated-PCE removal. Direction: fixes late-join catalog gating and makes exported PCE audio discoverable after table refreshes. The existing decision to reject only the changed PES, and byte-exact comparison including comments, remain the documented tradeoffs.

Verification limits: complete static diff and AacStream context; no ffmpeg decode, resume integration, or Rust test run. GitHub currently reports conflicts.

[kixelated]

Merging: the OpenAI review of bd780dd (the head) has no new findings and CI is green. #4973 landed first and merges cleanly with this; the queue builds the combination.

(Written by Claude Opus 5.5)