TEL-6738: Add BUNDLE RTP receive demux - #575
Closed
baloeng wants to merge 79 commits into
Closed
Conversation
…ibility
Phase 1+2 - PLI/FIR/NACK/TMMBR feedback SSRC fix:
* Track a separate video local/remote SSRC pair on the shared audio RTP session
when BUNDLE multiplexes video on the audio transport. Plumb in
switch_rtp_set_bundle_has_video() / switch_rtp_set_bundle_video_ssrcs(),
rtp_has_video_feedback(), rtp_feedback_local_ssrc/remote_ssrc helpers, and
rtcp_fb_local_ssrc / rtcp_fb_remote_ssrc fields on switch_rtp_t.
* Wire the video SSRCs into the shared session from
switch_core_media_activate_rtp() BUNDLE reuse path and from check_ice()
SDP ssrc-learn so they are non-zero before any PLI/FIR is queued.
* In check_rtcp_and_ice() PLI / FIR / NACK / TMMBR construction, use the
feedback SSRCs instead of the audio rtp_session->ssrc / remote_ssrc when
bundle_has_video && !SWITCH_RTP_FLAG_VIDEO. Prior behavior caused the
receiver (Safari) to ignore PLI/FIR because the FB items targeted the
audio SSRC instead of the video SSRC.
* Bump TEL6738 RTCP queue/send/recv PLI/FIR/NACK log lines from
DEBUG1/DEBUG2 to DEBUG so they actually appear at the configured level.
* Add gate-trace SWITCH_LOG_DEBUG lines for queue FIR/PLI DROPPED cases
(rtp_write_ready==0 path and queue branch-not-entered path) and a
BUNDLE refresh_req trace + a post-sendto TEL6738 RTCP BUNDLE fb sent
summary, all guarded on bundle_has_video.
Phase 3 - BUNDLE stats hygiene:
* rtcp_stats(): early-return when bundle_has_video and the packet SSRC
matches the known BUNDLE video remote SSRC. Prevents rtcp_stats_init from
flipping audio/video stats state on every SSRC switch (~2000+ reinits per
call) and corrupting audio seq/jitter/loss accounting.
* rtp_common_read() do_cng path: skip rtp_session->stats.inbound.skip_packet_count++
when io_flags & SWITCH_IO_FLAG_BUNDLE_DRAIN. The BUNDLE drain thread polls
the socket at ~1ms and harmlessly returns CNG on every empty poll; counting
these inflated the CDR skip_packet_count to ~56k and dropped quality % to 42.
All changes are guarded behind bundle_has_video && !SWITCH_RTP_FLAG_VIDEO so
non-BUNDLE / pure-video / pure-audio sessions are unaffected.
Verified on tel-at1-prox-dev-222 b2bua-rtc-canary: Safari (BUNDLE) -> SIP
-> Chrome (non-BUNDLE) callee sample shows 0 video skip, 100 percent video quality
on every CDR-bearing leg, real keyframes from Safari every ~1s after PLI/FIR,
no rtcp_stats_init thrashing, and the receiver renders clear video.
Guard 1 — BUNDLE caller audio bypass: Skip session-level MID stamping when bundle_has_video && !VIDEO && !force_video. The video engine sets ext_mid.ext_id on the shared audio session, but audio packets must not carry the video MID. Previously every outbound audio packet entered rtp_add_mid_extension() with the video MID ext id, which failed on inbound BEDE extensions and hard-dropped ~3400 audio packets per call. Guard 2 — rtp_add_mid_extension() element strip fix: The existing element strip loop removed ALL multi-byte extension elements (elem_payload_len > 1), not just the MID element. This destroyed legitimate extensions like abs-send-time (3 bytes) and transport-cc (2 bytes), leaving malformed remnants that failed the bounds check. Now only elements matching mid_ext_id are stripped, preserving other extensions intact. Guard 3 — soft-fail instead of hard-drop: When session-level (non-override) MID stamping fails, strip the extension header entirely and send the packet without it, rather than dropping. A packet without MID is better than no packet. This makes both BUNDLE and bridge legs self-healing when rtp_add_mid_extension encounters an edge case. All guards conditioned on bundle_has_video or the session-level (non-override) MID path. Non-BUNDLE sessions with explicit MID overrides are unaffected. Verified: BAD sample showed 3418 audio drops on BUNDLE caller + 20 on bridge (correlating with broken callee video). GOOD sample had 6 drops. With these guards the drops should be eliminated.
When Guard 1 skips session-level MID stamping for audio on a BUNDLE shared session, the inbound packet may still carry a BEDE extension header from the bridge side. Passing this raw extension to libsrtp causes srtp_err_status_bad_param (code=2) because the extension bytes are from the inbound SRTP context, not the outbound one. The protect failure tears down the bridge (NORMAL_CLEARING). Fix: strip the extension header (memmove payload, clear header.x/ext/ebody) in the Guard 1 skip path, same pattern as Guard 3.
Guard 1 incorrectly skipped MID stamping and stripped all RTP extensions from audio on the BUNDLE shared session. The session ext_mid actually holds the audio MID, not video. Stripping it broke Safari BUNDLE demux (RFC 9143) and removed audio-level/abs-send-time extensions, degrading video bitrate. Audio now flows through rtp_add_mid_extension() which stamps the correct audio MID and preserves other extensions. Guard 2 (parser) and Guard 3 (soft-fail) retained.
…gation Phase 5: Change process_rtcp_report() RTPFB/PSFB gate from SWITCH_RTP_FLAG_VIDEO to rtp_has_video_feedback(rtp_session) so the BUNDLE shared audio session processes inbound FIR/PLI/NACK from Safari. Phase 5b: When the BUNDLE shared session receives FIR/PLI, propagate the refresh request directly to the bridged PARTNER session (SIP caller) via switch_core_session_force_request_video_refresh(partner). This bypasses a race condition where the wrong bridge thread consumed CF_VIDEO_REFRESH_REQ before it could propagate to the video source (callee). Without 5b, Safari PLI is received (Phase 5) but source_refresh only reaches ~21 via periodic timer. With 5b, each PLI directly triggers a keyframe request on the callee through the partner session.
damirn
requested changes
Jun 23, 2026
damirn
left a comment
damirn
left a comment
There was a problem hiding this comment.
Evaluation of the BUNDLE RTP demux PR. The pure switch_bundle.c module is well-designed and well-tested, and the switch_utils.c extension-aware frame-clone fix is correct. Main concerns before merge: (1) leftover TEL6738 debug scaffolding (~82 new log calls, several at NOTICE) on a deploy-branch target; (2) a drain-thread start/stop race and loss of audio media-timeout detection when the drain thread owns the socket; (3) a video-refresh direction change that affects non-BUNDLE bridges; (4) the riskiest code (outbound MID rewrite, drain/buffer threads) is essentially untested. Inline notes below. Note: the recv_rtp_exts_size widening and the two #if HAVE_MID_EXT blocks are pre-existing, not new regressions.
| fct_req(rtp != NULL); | ||
|
|
||
| fct_chk(switch_rtp_enable_mid(rtp, TEST_MID_EXT_ID, "0") == SWITCH_STATUS_SUCCESS); | ||
| fct_chk(switch_rtp_get_received_mid(rtp) == NULL); |
There was a problem hiding this comment.
This only asserts has_ext==FALSE after one write. The outbound MID strip/rewrite memmove in switch_rtp.c (the most corruption-prone code in the PR) is otherwise untested — add a case that stamps MID over a packet already carrying a foreign extension (e.g. abs-send-time) and verifies the resulting bytes/offsets and that the other extension is preserved.
damirn
reviewed
Jun 23, 2026
damirn
left a comment
damirn
left a comment
There was a problem hiding this comment.
A few more inline notes that I folded out of the previous summary.
WebRTC callers demand keyframes via FIR/PLI, but a plain RTP/AVP SIP callee that negotiated no a=rtcp-fb is never asked for a fresh IDR: RTCP FIR/PLI toward it is gated on negotiated feedback, and the existing SIP INFO picture_fast_update fallback required the sofia_send_info_vid_refresh channel var to be set. A WebRTC caller that loses its bootstrap keyframe then stalls with no recovery path. Fire the SIP INFO (RFC 5168) picture_fast_update fallback automatically when the video callee negotiated no keyframe-capable RTCP feedback (no fir/pli), keeping the explicit sofia_send_info_vid_refresh override, the 500ms throttle, and the CF_VIDEO/!CF_AVPF/!CF_MANUAL_VID_REFRESH gates. Add switch_core_media_has_video_refresh_rtcp_fb() to report whether the video engine negotiated fir/pli. Parse rtcp-fb into locals during SDP negotiation and commit fir/pli/nack/tmmbr to the engine only on codec acceptance, snapshotting and restoring on set_video_codec/check_ice failure so a rejected re-INVITE preserves prior feedback state and any synchronous VIDEO_REFRESH_REQ reads the newly accepted state.
test_received_mid_clears_per_packet requested two ports from the shared RTP port allocator, but the unit-test config exposes a single-port range (rtp-start-port=rtp-end-port=1234). The allocator has exactly one slot, so the second switch_rtp_request_port() returned 0 and the test failed at fst_requires(remote_port > 0) on every CI run since the test was added. Keep local_port from the allocator for the RTP session listen port, but derive the sender's remote_port from an OS-assigned ephemeral bind: bind send_sock to rx_host:0 and read back the kernel-assigned port via switch_socket_addr_get(SWITCH_FALSE) + switch_sockaddr_get_port(). Drop the now-invalid switch_rtp_release_port() for remote_port since it is no longer allocator-owned. The MID parsing/per-packet-clearing assertions are unchanged.
…callees" This reverts commit 7f473b3.
The edge-case packetlen test built synthetic 0xbede one-byte RTP header-extension bodies with arbitrary non-zero bytes. For ext_words=2 those bytes were parsed as multiple extension elements, and the final 0x46 element header declared a payload length larger than the remaining extension block. The RTP extension parser correctly rejected the packet, so the test timed out with got=0 before checking packetlen. Build each extension block as one valid id=1/len=0 element with a single payload byte, followed by zero padding. This keeps the 0/1/2/3-word wire length coverage while making every synthetic packet valid.
Add deterministic switch_bundle unit tests covering group reset generation/policy semantics, mline lookup by mid and index (incl. NULL and empty inputs), remote-SSRC learn idempotency and rebind/collision guards, and demux rejection of an unoffered MID. Also fix the pre-existing test_missing_rtcp_mux_rejects: the MID RTP-extension gate in switch_bundle_group_validate() rejects the audio BUNDLE-tag m-line before validation reaches the video m-line's missing rtcp-mux check, so set a valid remote MID extension on the audio m-line so validation advances to the condition under test.
baloeng
force-pushed
the
jira-tel-6738-bundle-v2
branch
from
July 3, 2026 07:20
0ca6e43 to
50bf4aa
Compare
dev-ryanc
reviewed
Jul 4, 2026
| } | ||
|
|
||
| if (ssrc && switch_bundle_group_learn_remote_ssrc(group, mline, ssrc) != SWITCH_STATUS_SUCCESS) { | ||
| return NULL; |
Collaborator
There was a problem hiding this comment.
[MEDIUM] SSRC change mid-session permanently drops BUNDLE media despite valid MID
// switch_bundle.c:226 — MID is authoritative (RFC 9143), don't drop on SSRC learning failure:
- if (ssrc && switch_bundle_group_learn_remote_ssrc(group, mline, ssrc) != SWITCH_STATUS_SUCCESS) {
- return NULL;
- }
+ if (ssrc && mline->remote_ssrc != ssrc) mline->remote_ssrc = ssrc;learn_remote_ssrc (L194) rejects SSRC updates — a legitimate SSRC change (RFC 3550 collision/codec switch) permanently drops media until re-negotiation.
dev-ryanc
reviewed
Jul 4, 2026
dev-ryanc
left a comment
dev-ryanc
left a comment
Collaborator
There was a problem hiding this comment.
1 inline comment posted. BUNDLE MID demux drops media on legitimate SSRC changes because learn_remote_ssrc never updates existing bindings. See inline comment for specifics.
dev-ryanc
reviewed
Jul 6, 2026
| /* Optional Unified Plan BUNDLE grouping. Only reference MIDs that will be | ||
| * emitted below; SDP BUNDLE group values must exactly match a=mid lines. */ | ||
| if (switch_core_media_bundle_should_offer(smh) && !bundle_no_attr_mid && !bundle_no_audio_mid) { | ||
| bundle_audio_mid = !zstr(audio_mid) ? audio_mid : "audio"; |
Collaborator
There was a problem hiding this comment.
[LOW] Same !zstr fix needed at L15264 and L15761
// switch_core_media.c:15264 and :15761 still use pointer check:
- "a=mid:%s\r\n", audio_mid ? audio_mid : "audio");
+ "a=mid:%s\r\n", !zstr(audio_mid) ? audio_mid : "audio");This commit fixed the BUNDLE group (L15051) but two a=mid emission sites still use audio_mid ? — if rtp_audio_mid="" (non-NULL), BUNDLE group says "audio" but m-line says "" — SDP BUNDLE must match.
dev-ryanc
reviewed
Jul 6, 2026
dev-ryanc
left a comment
dev-ryanc
left a comment
Collaborator
There was a problem hiding this comment.
1 inline comment posted. New commit fixes the BUNDLE group !zstr check (L15051) but the same pattern remains unfixed at two a=mid emission sites (L15264, L15761) — BUNDLE group and m-line can disagree on empty rtp_audio_mid. See inline comment for specifics.
Queued BUNDLE audio drained from the shared socket was raw-forwarded with the peer packet's RTP header extensions preserved (SFF_RAW_RTP). On a negotiated callee leg this leaked stale/foreign MID extension ids onto the egress audio, causing the browser (Safari) to reject/mis-demux it as no inbound audio. Copy only the payload, clear packet/packetlen and the raw RTP flags so queued audio re-enters the normal audio write path and FS emits clean, locally-negotiated RTP/PT/SSRC/MID. Also allow same-mline SSRC rebind when MID is present (preserving cross-mline collision rejection), default empty audio/video MID vars to audio/video for BUNDLE group and a=mid emission, and add unit coverage.
Author
|
Superseded by #620, which carries the same Unified Plan / BUNDLE changes squashed into a single |
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
3 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
No
mod_telnyx_rtcfiles are changed in this PR; signaling-side enablement is handled separately.