-
Notifications
You must be signed in to change notification settings - Fork 158
sync engine: bound timeline drift, preserve OWD across NTP rebuilds #948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,9 @@ const ( | |
|
|
||
| // deadbandThreshold is the minimum |correction| before slew smoothing kicks in. | ||
| deadbandThreshold = 5 * time.Millisecond | ||
|
|
||
| // wallSlewAlpha bounds publisher sample-clock skew: steady-state |drift| = R·δ·(1-α)/α ≈ 10ms for R=20ms, δ=±500ppm, α=0.01. | ||
| wallSlewAlpha = 0.01 | ||
| ) | ||
|
|
||
| // syncEngineTrack implements TrackSync for a single track within a SyncEngine. | ||
|
|
@@ -425,6 +428,8 @@ func (st *syncEngineTrack) GetPTS(pkt jitter.ExtPacket) (time.Duration, error) { | |
| "deadline", deadline, | ||
| "behindBy", limit-oldPTS, | ||
| ) | ||
| // Reset timeliness clock so a lingering transient deficit doesn't fire force-correction on every subsequent packet, collapsing them into the same newPTS at the mixer. | ||
| st.lastTimelyPacket = time.Now() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is one scenario worth zooming in - what happens on the packets after a force-correction, when the track is still behind.. which is the case we kept seeing in cases where carriers don't signal loss correctly (sip) - i.e when the deficit is structural rather than transient. The jump raises lastPTSAdjusted but the raw baselines (lastWallPTS/lastSlewPTS/sessionOffset) still reflect the behind timeline, so the next packet computes a behind PTS again. With the timer now reset, the monotonicity floor becomes the only thing advancing output - +1ms per 20ms frame, until the timer expires and a multi-second jump fires again. |
||
| } | ||
| } else { | ||
| st.lastTimelyPacket = time.Now() | ||
|
|
@@ -489,43 +494,33 @@ func (st *syncEngineTrack) wallClockPTSForRTP(ts uint32, receivedAt time.Time) ( | |
| // wallClockPTSForRTPLocked is the lock-held implementation of wall-clock PTS | ||
| // computation. Caller must hold st.mu. | ||
| func (st *syncEngineTrack) wallClockPTSForRTPLocked(ts uint32, receivedAt time.Time) time.Duration { | ||
| // Defense in depth: if receivedAt is the zero time.Time, wallElapsed | ||
| // below would be a ~63-billion-second negative duration, the sanity | ||
| // threshold would reject rtpDerived, and the function would return 0 | ||
| // regardless of sessionOffset — locking the trust-threshold clamp onto | ||
| // a bogus baseline. initializeLocked already substitutes time.Now() for | ||
| // zero ReceivedAt on the first packet; do the same here in case a later | ||
| // caller (e.g., OnRTCP using sr.RTPTime + an unset receivedAt) misses it. | ||
| // A zero receivedAt would make wallElapsed hugely negative and lock every downstream clamp onto a bogus baseline. | ||
| if receivedAt.IsZero() { | ||
| receivedAt = time.Now() | ||
| } | ||
|
|
||
| // Wall-clock elapsed since this track started, plus session offset | ||
| wallElapsed := receivedAt.Sub(st.startTime) + st.sessionOffset | ||
| if wallElapsed < 0 { | ||
| wallElapsed = 0 | ||
| } | ||
|
|
||
| if !st.initialized { | ||
| return wallElapsed | ||
| } | ||
|
|
||
| // If we have processed at least one packet, use RTP delta from the previous | ||
| // pure-wall-clock baseline for more precision (immune to receivedAt jitter). | ||
| // uint32 subtraction wraps for backward RTP timestamps; the sanity check | ||
| // below catches the resulting huge positive delta and falls back to wall clock. | ||
| if st.initialized { | ||
| rtpDelta := ts - st.lastTS | ||
| rtpDerived := st.lastWallPTS + st.converter.ToDuration(rtpDelta) | ||
|
|
||
| // Sanity check: if RTP-derived PTS diverges from wall-clock by > 5s, use wall clock. | ||
| diff := rtpDerived - wallElapsed | ||
| if diff < 0 { | ||
| diff = -diff | ||
| } | ||
| if diff <= wallClockSanityThreshold { | ||
| return rtpDerived | ||
| } | ||
| } | ||
| rtpDelta := ts - st.lastTS | ||
| rtpDerived := st.lastWallPTS + st.converter.ToDuration(rtpDelta) | ||
| diff := rtpDerived - wallElapsed | ||
|
|
||
| // Use wall-clock elapsed, ensuring non-negative. | ||
| if wallElapsed < 0 { | ||
| wallElapsed = 0 | ||
| if diff > wallClockSanityThreshold || diff < -wallClockSanityThreshold { | ||
| return wallElapsed | ||
| } | ||
| return wallElapsed | ||
|
|
||
| // Slew rtpDerived toward wallElapsed each packet so publisher sample-clock skew stops accumulating across the sanity band; see wallSlewAlpha for the steady-state math. | ||
| absorbed := time.Duration(float64(diff) * wallSlewAlpha) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm - I have a feeling this creates a closed loop with the tempo controller. Let me check it.. The SR drift path reads the slewed baseline, so a delayed packet now enters the tempo loop as phantom drift: tempo books a permanent correction (corrected), the slew then drains the same disturbance back out of the sensor, effective flips sign, and the controller fires counter-corrections undoing its own work, audible ±5% warble from one late packet. Need to check more
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. asked claude to check it - I think the graph it generated might be helpful: |
||
| return rtpDerived - absorbed | ||
| } | ||
|
|
||
| // OnSenderReport implements TrackSync. It stores a callback invoked on sender reports. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.