TELCORE-248 / TEL-6872: Cherry-pick elastic jitter buffer fixes from SignalWire PR #2337 - #539
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
damirn
left a comment
There was a problem hiding this comment.
can't complain on fixes from upstream ;)
|
@dev-ryanc make sure you grab the second commit 60f8f34 This is preventing a race condition that can take place is someone extract the statistics in a hangup hook |
minhtuan1407-telnyx
left a comment
There was a problem hiding this comment.
🔬 Analyst review — TEL-6872 elastic JB cherry-pick
Verdict: APPROVE (with non-blocking notes)
Changes verified
MAX_DROPOUT=3000guard incheck_jb_size()— hides "old" nodes only whenudelta ∈ (1, 3000). Prevents seq-wraparound hiding valid packets. ✅decrement_seq()ordering fix —last_target_seqnow set AFTER decrementingtarget_seq. Old code left both fields equal after expand, breaking PLC seq emission. ✅expand_frame_lentracking +RESET TOO EXPANDED— increments on PLC expand, decrements on acceleration drop, resets JB ifexpand_frame_len > max_frame_len. Prevents unbounded growth. ✅MAX_CONSECUTIVE_MISS=100safety valve — falls back to non-elastic + reset after 100 consecutive misses. ✅!jb->elasticgate on BUFFERING path — elastic JB serves packets without waiting to refillframe_len. ✅consecutive_missreset on successful read +buffering_skipcounter. ✅
Non-blocking concerns
- Subset of unmerged upstream. SignalWire PR signalwire#2337 is still OPEN. It also adds:
switch_jb_export_stats(),switch_rtp_get_jitter_buffer_for_stats(),switch_core_media_export_jb_stats()(flush JB stats before hangup hooks), and a multi-tier forced/fast acceleration path (packets_in_buffer >= max_frame_len). Theexpand_frame_lencap is reactive (after PLC); upstream's proactive acceleration during reads is more robust. Recommend a follow-up ticket to track upstream. - Style:
MAX_DROPOUT/MAX_CONSECUTIVE_MISSas file-scope#definevs upstream's function-localconst int. No functional difference.
CI: Jenkins green. GH Actions cancelled (24h timeout) — not regressions. 3 existing approvals (2026-03-13).
Net: ✅ Ship it. Open follow-up for upstream re-cherry-pick or forced-acceleration port.
— 🔬 Analyst
|
@dev-ryanc i see more commits in signalwire#2337 but i also wonder why is this still not merged in upstream |
…alwire#2337 Cherry-picked fixes from signalwire#2337 (Julien Chavanton): 1. Sequence rollover bug in check_jb_size() - add MAX_DROPOUT guard to distinguish old packets from seq 65535->0 wraparound 2. decrement_seq() ordering - set last_target_seq after decrement 3. Buffering skip gate - do not block elastic mode with the complete_frames < frame_len check 4. Consecutive miss safety valve - after MAX_CONSECUTIVE_MISS (100) consecutive missed frames, disable elastic mode and reset JB
Track net buffer expansion via expand_frame_len (incremented on PLC expand, decremented on acceleration). Reset the jitter buffer when expansion exceeds max_frame_len to prevent unbounded growth during sustained packet loss. Cherry-picked from signalwire#2337 (Julien Chavanton).
…e PR signalwire#2337 Consolidate inline channel variable exports from the jitter buffer hot path into switch_jb_export_stats(), called at hangup before hangup hooks run (via switch_core_media_export_jb_stats) and as a safety net before every switch_jb_destroy() in both switch_rtp_destroy() and the KILL_JB path in read_rtp_packet(). This prevents a race condition where hangup hooks read stale/missing JB stats because the jitter buffer was already destroyed or the inline exports hadn't run recently enough. Adapted from upstream commit 0388398 — excluded fast_acceleration, forced_acceleration, and packets_in_buffer fields which don't exist in our switch_jb struct. Cherry-picked from signalwire#2337 (Julien Chavanton).
…nd boundary
[1] Reset jb->frame_len to jb->min_frame_len when disabling elastic mode
after consecutive miss safety valve triggers. Without this, the JB
stays at expanded size after elastic mode is disabled.
[2] Add SWITCH_LOG_ALERT debug log when elastic mode is disabled after
MAX_CONSECUTIVE_MISS, matching upstream.
[5] Add expand_frame_len >= max_frame_len boundary check (third else-if
branch) to catch the exact boundary case and reset via reset_error.
…signalwire#2337 1. Add switch_rtp_ready() guard in the JB poll loop to prevent polling after the session is being destroyed. 2. Add rtp_jitter_buffer_accelerate channel variable check to the tries > 20 condition, allowing JB reset to be skipped when acceleration is explicitly enabled.
Add elastic algorithm activity tracking and enhanced statistics export from upstream commit af83338: - New stats fields: grew, shrunk, grow_to_max, at_min_ms, at_max_ms, edge_since, reset_setup, reset_runtime - New jb fields: media_started, packets_total - Track frame_len edge transitions in jb_frame_inc_line() - Split reset count into setup vs runtime in switch_jb_reset() - Flush at-edge timer on set_frames() - Initialize edge_since on JB creation - Export all new metrics in switch_jb_export_stats() with a comprehensive summary log line - Set media_started and increment packets_total on each put_packet Cherry-picked from signalwire#2337 (Julien Chavanton).
@damirn Not sure why as well, but I don't see any issues with the implementation. |
Fix double-decrement in decrement_seq() — last_target_seq was over-decremented by one, shifting PLC sequence numbers an extra packet behind. Fix lock-order inversion in switch_jb_export_stats() — snapshot JB fields under mutex, unlock before channel writes to avoid holding JB mutex while acquiring channel-side locks from hangup/destroy/KILL_JB contexts. Cache rtp_jitter_buffer_accelerate channel var at function entry instead of per-iteration in the retry loop, and restore an unconditional hard retry bound (tries > 100) so the read loop cannot spin indefinitely in bad-jitter conditions.
0ed80ca
into
telnyx/telephony/deploy-development
Cherry-pick select bug fixes from signalwire#2337 (Julien Chavanton).
Fixes
hiding valid packets near seq 65535→0 wraparound
points to the correct value
complete_frames < frame_len buffering check
consecutive missed frames, disable elastic mode and reset JB
and reset JB when it exceeds max_frame_len
mutex, unlocks, then writes channel variables and logs (avoids lock-order
inversion with channel-side locks from hangup/destroy/KILL_JB contexts)
channel var at function entry, keep unconditional hard bound at tries > 100
Intentionally excluded
uninitialized variable issue with packet_vad, drops frames during active speech)
Testing