[kixelated]

Problem

Every TS-imported audio frame is cut into its own group, which clears the producer's timestamp history before it can infer a cadence. The unbounded cut therefore reported the final frame's start as the end of its HLS/DASH record, so the final audio segment advertised one frame less than it decodes. This affected AAC, MPEG audio (MP2/MP3), and Opus.

Approach

Each audio path already computes the next frame's PTS from its sample count (1024 for ADTS AAC, the header's count for MPEG audio, the TOC's for Opus). Pass that endpoint to the group cut. A shared test helper reads the final HLS stop record, and the existing EOF/damage tests for AAC, MP2, and Opus assert it, including the frame completed during the EOF drain.

Impact

• Public API: none.
• Wire: HLS EXTINF and DASH S@d now include the final audio frame's actual duration. The catalog's measured audio bitrate now closes each one-frame span at its sample-derived duration instead of folding it into the next frame; the RATE_WINDOW sampling guard is unchanged, so CBR estimates are effectively the same.

Alternatives

Dropped advertised-duration padding and a new codec API, because the TS importer already knows the endpoint.

Choices made without asking

• Keep the fix in the TS importer and reuse its existing endpoint calculation.
• Extend it from AAC to MPEG audio and Opus, which had the same one-frame-group gap.
• Delete the finished quest quest/m1/hls-final-aac-segment.md and unblock quest/m1/hls-conformance.md.
• Keep conformance and decoder-identity changes in their owning PRs.

Checks

• Reproduced before the fix: the AAC EOF stop record was 1021 ms instead of 1042 ms. The regression assertion passes after the fix.
• The existing conformance test from test(hls): probe served segments against their HLS and DASH manifests nightly #7387, run through a temporary local overlay: all 13 audio segments' HLS/DASH durations fall within the existing 2.5 ms tolerance (final segment 1.387 s advertised, 1.386667 s decoded). Its remaining failures belong to the decoder-identity quest.
• quest gate nix develop -c just ci (CI_STRICT, zizmor) on cc017cab5e: passed, including 4271 Rust tests.
• quest gate nix develop -c just test interop --all on cc017cab5e: all 183 checks passed.
• quest check --base origin/main: passed.

Review

Codex, gpt-6.1-sol, medium effort, round 1 on head cc017cab5eb67012351dd1d4ec973c7b7f5e9bba against origin/main (c04f7771dc): 0 findings.

Follow-ups

• Finish the decoder-identity quest (quest/m1/hls-reinit-same-codec.md), then land the conformance test in test(hls): probe served segments against their HLS and DASH manifests nightly #7387.

(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 27 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

• 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: b1698c94-d948-4c0f-b30f-9b155583b34b

Commits

Reviewing files that changed from the base of the PR and between 4622194 and e563946.

Files selected for processing: 4

• quest/m1/README.md
• quest/m1/hls-conformance.md
• quest/m1/hls-final-aac-segment.md
• rs/moq-mux/src/container/ts/import.rs

• Autofix · 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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: cc017ca

No actionable bugs found in the changed areas. Passing the existing sample-count-derived endpoint into cut fixes the missing final-frame bound at its source for AAC, MPEG audio and Opus, without adding a codec API or padding advertised durations. The EOF-drain and damaged-PES regression assertions cover the relevant final HLS stop records.

Verification: static inspection of the diff, surrounding TS import paths, codec importers and producer cut/finish handling only. I did not run tests or independently verify the reported HLS/DASH conformance results.

(Written by OpenAI)

[pullfrog]

✅ No new issues found.

Reviewed changes

• TS audio groups carry an explicit endpoint — rs/moq-mux/src/container/ts/import.rs: AAC, MPEG audio (LegacyStream), and Opus now advance pts by the codec's sample count and pass it to cut(Some(end)) instead of cut(None), so each one-frame group's final frame has a usable duration and the HLS stop record lands at the frame end rather than its start.
• Opus endpoint reorganization — pts/end both flow through advance_pts(base, elapsed, 48_000) before/after the sample increment, replacing the inline checked_add.
• Final-segment assertions — new hls_audio_end test helper (finishes the catalog, subscribes to the rendition's HLS record track, returns the last stop record's pts); the AAC, MP2, and Opus EOF/damage tests assert 1042 ms / 1144 ms / 1086 ms.
• Quest bookkeeping — deletes quest/m1/hls-final-aac-segment.md and removes its links from quest/m1/README.md and quest/m1/hls-conformance.md's ## Required.

I traced the three risk points: Timestamp ordering/conversion is cross-scale safe (Estimator normalizes via nanos, flush_buffer converts the bound into the frame's scale, recorder.end uses as_micros); Producer::timestamp(Some(pts)) is identity, so the endpoint and the frame timestamp share a reference frame; and HLS intermediate segment durations come from the next record's pts, so per-frame recorder.end only affects stop records (final and skipped-group), which is exactly the target, leaving the close video-only InvalidEnd check unexercised for audio.

Verified by running cargo test -p moq-mux --lib 'container::ts::import::test::': 172 passed, 0 failed, including the three modified tests.

Technical details

# Catalog bitrate estimate side effect (informational, no action)

## Context
Passing an endpoint to `cut` also reaches `Estimator::cut(end)` (`catalog/producer.rs:614`). Previously one-frame audio groups cut with `None`, so `Bitrate::cut` found no elapsed span and left it open to fold into the next frame (`catalog/estimate.rs:275-306`). Now each span closes at the frame's sample-derived duration.

## Required outcome
None functionally — the window is still only sampled once `>= RATE_WINDOW`, so the "lone frame spike" guard holds, and for CBR audio the measured value is effectively unchanged (arguably more accurate). Flagging only because the PR body says "Wire: none" while the catalog `bitrate` is a wire field; no change requested.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Landing summary for head cc017cab5eb67012351dd1d4ec973c7b7f5e9bba:

• Changes: the TS importer passes each AAC, MPEG audio, and Opus frame's sample-derived endpoint to the group cut, so the final HLS/DASH audio segment advertises its full decoded duration. Regression assertions cover the EOF-drain and damaged-PES paths. The finished quest quest/m1/hls-final-aac-segment.md is deleted, which unblocks quest/m1/hls-conformance.md.
• Reviews: local Codex round 1 on this head found nothing. The online OpenAI and Pullfrog reviews found nothing actionable either. Pullfrog noted that the catalog's measured audio bitrate now closes each one-frame span. That note is informational, and the PR body's Impact section now mentions it.
• Decisions: none outstanding. quest check --base origin/main passes on this head, and CI is green.

(Written by Claude Opus 5.5)