fix(moq-gst): stop moqsrc panicking on backwards timestamps; globally unique pad ids

[kixelated]

Follow-up to #1633 (review findings #1 and #2).

1. PTS-underflow panic (the important one) 🟠

build_buffer pins its PTS reference to the first decoded frame and computes frame.timestamp - reference. Frames arrive in decode order, so a B-frame's presentation timestamp can fall before the reference. Timestamp's Sub is checked_sub(..).expect("time overflow") — it panics on underflow (moq-net/src/model/time.rs:281). That crashes the pump task and, because the panic happens before the pad-removal cleanup, leaks the pad and kills the rendition. Open-GOP / leading-B-frame content triggers it.

Fix: checked_sub and clamp to ClockTime::ZERO instead of crashing.

2. Pad-id counter was per-session 🟡

next_pad_id reset to 0 on every run_session, despite the doc claiming a "process-unique counter". A rapid Paused -> Ready -> Paused restart could mint video_0 while the previous session's cancelled pump was still removing its video_0 (two same-named pads on the element). Promote it to a process-wide AtomicU64 so it actually matches the doc and the collision window is gone.

Test

Added a throwaway publisher that sends a keyframe at ts=2000ms then frames at ts=1000ms+ (the backwards-timestamp case):

build	result
before	panicked at time.rs:281: time overflow after 1 buffer; pad leaked
after	streams all 201 buffers, clean EOS, no panic

Real H264 decode (cdn.moq.dev/demo bbb.hang) still decodes (20/20 frames). clippy + fmt --check clean.

Deliberately not included (noted in the #1633 review as lower priority): blocking pad.push() on tokio workers (situational), redundant CMAF init parsing per catalog update (minor perf), and reconcile unit tests.

(Written by Claude)

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5d0b29ae-1846-4e86-9876-ac5c162ed883

📥 Commits

Reviewing files that changed from the base of the PR and between f26e276 and 720c83d.

📒 Files selected for processing (2)

• rs/moq-gst/Cargo.toml
• rs/moq-gst/src/source/imp.rs

💤 Files with no reviewable changes (1)

• rs/moq-gst/Cargo.toml

---

Walkthrough

This PR refactors pad ID allocation to use a process-wide AtomicU64 (removing the per-session next_pad_id), updates reconcile call sites and allocation to use NEXT_PAD_ID.fetch_add(Ordering::Relaxed), introduces relative_pts(timestamp, reference) using checked_sub and clamping to gst::ClockTime::ZERO, updates build_buffer to call relative_pts, adds unit tests for relative_pts, and tweaks rs/moq-gst Cargo.toml to set doctest = false and remove test = false.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the two main changes: fixing a panic on backwards timestamps and ensuring globally unique pad IDs.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining the motivation, technical details, and testing approach for both fixes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/moqsrc-ts-underflow-pad-id

---

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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

rs/moq-gst/src/source/imp.rs (1)

533-539: ⚡ Quick win

Add an in-file regression test for the backward-PTS clamp path.

This fix looks correct, but a small unit test here would lock in the no-panic underflow behavior.

As per coding guidelines, "Add tests where they're easy to write; Rust tests are integrated within source files".

🤖 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-gst/src/source/imp.rs` around lines 533 - 539, Add a small in-file
unit test that exercises the backward-PTS clamp path so the checked_sub
underflow returns gst::ClockTime::ZERO rather than panicking: create a test in
imp.rs (#[cfg(test)] mod tests) that constructs a frame with a timestamp smaller
than the reference used in the match (exercise the code path using
frame.timestamp.checked_sub(reference) in the surrounding function), call the
function or the minimal public/private helper that performs the subtraction, and
assert the result equals gst::ClockTime::ZERO and that no panic occurs; keep the
test self-contained using dummy/frame builders present in the file or
constructing minimal types needed.

Source: Coding guidelines

🤖 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-gst/src/source/imp.rs`:
- Around line 533-539: Add a small in-file unit test that exercises the
backward-PTS clamp path so the checked_sub underflow returns
gst::ClockTime::ZERO rather than panicking: create a test in imp.rs
(#[cfg(test)] mod tests) that constructs a frame with a timestamp smaller than
the reference used in the match (exercise the code path using
frame.timestamp.checked_sub(reference) in the surrounding function), call the
function or the minimal public/private helper that performs the subtraction, and
assert the result equals gst::ClockTime::ZERO and that no panic occurs; keep the
test self-contained using dummy/frame builders present in the file or
constructing minimal types needed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e4aaeeb6-262c-41f7-a1e9-2da0ae2568b9

📥 Commits

Reviewing files that changed from the base of the PR and between 4fe88d0 and f26e276.

📒 Files selected for processing (1)

• rs/moq-gst/src/source/imp.rs

[kixelated]

Addressed the nitpick in 720c83d: extracted relative_pts and added an in-file unit test for the backward-PTS clamp (asserts a pre-reference timestamp yields ClockTime::ZERO instead of panicking, and that a forward delta is preserved). The lib had test = false, which kept #[cfg(test)] from compiling, so I dropped it; the test runs via cargo test -p moq-gst (moq-gst is excluded from default-members, so the normal cargo test is unaffected). Easy to revert that one line if you’d rather keep the crate test-free. (Written by Claude)