fix(moq-mux): confirm TS sync lock before trusting a candidate sync byte#1697
Merged
Conversation
The TS demuxer scanned forward one byte at a time to find the next 0x47 sync byte after a misalignment. Replace the byte-by-byte loop with memchr::memchr, which is SIMD-accelerated and already a dependency. The end state is identical (jump to the next sync byte, or drop the buffer if none remains), so resync semantics are unchanged.
Match repo Rust convention preferring if let over an unwrapping match.
The MPEG-TS demuxer trusted any single 0x47 at the expected offset as a packet boundary. But 0x47 also occurs freely in packet payloads (TS has no byte stuffing), so a stray payload 0x47 during sync acquisition would be mistaken for a boundary and route one bogus 188-byte packet before the next packet revealed the misalignment. Add a sync lock: while unaligned, confirm a candidate 0x47 against the next packet's sync byte (offset + 188) before trusting it. A non-sync byte there means the candidate was payload, so keep scanning. If the buffer doesn't yet reach offset + 188, retain the candidate and re-check on the next decode call. Once locked we stride 188 at a time and drop the lock the instant a stride misses a sync byte, so the per-packet check stays the tripwire and the final packet of a buffer still emits without a follower to confirm against.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughImport::decode now keeps a persistent 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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. Comment |
Drop the Import::aligned field. Confirmation only matters while re-acquiring sync, so do it inline in the resync branch: scan for a candidate 0x47 whose next packet also starts with 0x47, skipping payload 0x47s, before trusting it. A trusted stride keeps the cheap single-byte check, so no per-call lock state is needed and the original aligned-stream behavior (and the final packet of a buffer) is preserved. Use a let-else break when no sync byte remains. Update resyncs_past_false_sync_byte to enter via a resync so it exercises the confirmation path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-mux/src/container/ts/import.rs`:
- Around line 140-167: The code currently trusts any 0x47 at self.scratch[off]
without persisting a locked/unlocked state, which lets unconfirmed candidates be
parsed; add a boolean lock field on the decoder (e.g., self.locked) and use it
so that when self.locked is false you DO NOT immediately accept a sync at
self.scratch[off] but instead run the existing memchr::memchr confirmation logic
(checking self.scratch.get(off + TsPacket::SIZE) == Some(&0x47)) and only set
self.locked = true when the +188 confirmation succeeds; when confirmation cannot
be performed because the candidate is near the buffer tail, retain the candidate
(break/return without parsing) so it will be rechecked on the next decode call,
and when you detect a missed sync (current code path where self.scratch[off] !=
0x47 and you stride incorrectly) ensure you set self.locked = false so
subsequent bytes are re-scanned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77f5aa37-f429-4364-beb3-59aea00dbd6a
📒 Files selected for processing (2)
rs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/container/ts/import_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-mux/src/container/ts/import_test.rs
…onfirm # Conflicts: # rs/moq-mux/src/container/ts/import.rs
Gate the resync confirmation on a persisted Import::synced flag. Without it, a candidate found during resync that lands within 188 bytes of a buffer tail (so its next-packet sync byte isn't buffered yet) was retained and then trusted on the next decode call without ever being confirmed, reintroducing the bogus-packet routing this change set out to prevent. Now an unconfirmed candidate keeps synced=false, so the next call re-runs the confirmation against the trailing bytes instead of trusting it. Once a candidate is confirmed we stride 188 at a time, dropping the lock the instant a stride misses a sync byte. Add resyncs_across_chunk_boundaries: a misaligned stream fed in small chunks, exercising tail candidates carried across decode calls.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-mux/src/container/ts/import_test.rs (1)
93-94: ⚡ Quick winAdd descriptive assertion messages for consistency.
The assertions lack failure messages, unlike similar assertions throughout this test file (e.g., lines 71-72, 57-58). Descriptive messages help diagnose which resync scenario failed.
📝 Suggested assertion messages
- assert_eq!(snapshot.video.renditions.len(), 1); - assert_eq!(snapshot.audio.renditions.len(), 1); + assert_eq!(snapshot.video.renditions.len(), 1, "chunked resync failed: no video track"); + assert_eq!(snapshot.audio.renditions.len(), 1, "chunked resync failed: no audio track");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/container/ts/import_test.rs` around lines 93 - 94, The two assertions assert_eq!(snapshot.video.renditions.len(), 1) and assert_eq!(snapshot.audio.renditions.len(), 1) need descriptive failure messages like the other assertions in this test; update each assert_eq! to include a third argument string describing the expectation (e.g., "expected exactly 1 video rendition after resync" and "expected exactly 1 audio rendition after resync") so failures indicate which resync scenario failed and mirror the style used in the nearby assertions (lines with similar checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rs/moq-mux/src/container/ts/import_test.rs`:
- Around line 93-94: The two assertions
assert_eq!(snapshot.video.renditions.len(), 1) and
assert_eq!(snapshot.audio.renditions.len(), 1) need descriptive failure messages
like the other assertions in this test; update each assert_eq! to include a
third argument string describing the expectation (e.g., "expected exactly 1
video rendition after resync" and "expected exactly 1 audio rendition after
resync") so failures indicate which resync scenario failed and mirror the style
used in the nearby assertions (lines with similar checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c1b3d79-0276-4e37-853e-a6b63e706019
📒 Files selected for processing (2)
rs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/container/ts/import_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-mux/src/container/ts/import.rs
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Follow-up to #1695. While reviewing the MPEG-TS sync logic we noticed a latent correctness gap: the demuxer trusted any single
0x47at the expected offset as a packet boundary.But
0x47is only the sync byte at byte 0 of each 188-byte packet. The other 187 bytes (header continuation, adaptation field, payload, raw codec bitstream) are arbitrary, and TS has no byte stuffing or escaping to keep0x47out of them (unlike HDLC byte-stuffing or H.264 NAL emulation-prevention). So0x47appears in payload roughly once per packet on average. The format relies on the fixed 188-byte periodicity, not on the sync byte being unique.Consequence: during sync acquisition (a misaligned start, mid-stream join, or post-corruption resync), a stray payload
0x47could be mistaken for a boundary and route one bogus 188-byte "packet" before the next packet revealed the misalignment.Fix
Add a sync lock, a persisted
Import::syncedflag:memchr) for a candidate0x47whose next packet also begins with0x47, skipping payload0x47s, before locking onto it. If no sync byte remains, drop the buffer. If the candidate sits within 188 bytes of the buffer tail (its+188neighbor isn't buffered yet), stay unsynced and retain it, so it's re-confirmed on the nextdecodecall with the trailing bytes rather than trusted blindly.0x47sync byte and stride 188 at a time; the per-packet check stays the tripwire that drops the lock the instant a stride misses.The flag persists across
decodecalls, which closes the cross-call hole: a candidate pending confirmation at a buffer tail is confirmed next call, never trusted on faith.Test plan
resyncs_past_false_sync_byte: enters resync, then a stray0x47whose+188byte isn't a sync byte, proving the confirmation rejects it and scans on.resyncs_across_chunk_boundaries: a misaligned stream fed in 100-byte chunks, exercising resync candidates carried (pending confirmation) acrossdecodecalls.cargo test -p moq-mux(all 38container::tstests pass, includingimport_resyncs_after_byte_misalignment,survives_midstream_join,import_handles_unaligned_chunks,kyrion_dirtystart_extracts_real_cues)cargo clippy -p moq-muxclean(Written by Claude)