TEL-6982: Emit /channels in AMR-WB rtpmap for strict IMS peers - #569
TEL-6982: Emit /channels in AMR-WB rtpmap for strict IMS peers#569damirn wants to merge 1 commit into
Conversation
FreeSWITCH has never emitted the channels suffix in rtpmap for mono codecs (since commit 05dd02d in 2013), relying on RFC 4566's implicit default of 1. Some IMS/VoLTE peers (e.g. Mavenir) reject such answers with "no audio payload can be selected" and immediately BYE the call, requiring the explicit "/1". Add a per-codec-impl flag always_emit_channels_in_rtpmap, plumbed into payload_map_t during SDP negotiation, and consulted at all three rtpmap emission sites in switch_core_media.c. Set the flag on both AMR-WB implementations (octet-aligned and bandwidth-efficient). Both new struct fields are added at the end of their respective structs to preserve binary layout of existing fields.
minhtuan1407-telnyx
left a comment
There was a problem hiding this comment.
Dev-FreeSWITCH CoC Review — freeswitch#569 (TEL-6982)
PR: #569
Title: TEL-6982: Emit /channels in AMR-WB rtpmap for strict IMS peers
Base → Head: telnyx/telephony/deploy-development ← jira-tel-6982 @ f29ece36
Scope: 4 files / +10 / −3. Two new struct fields, two mod_amrwb assignments, four negotiation/emission wire-ups.
Summary
Adds a per-codec-impl boolean always_emit_channels_in_rtpmap to switch_codec_implementation and payload_map_t. mod_amrwb sets SWITCH_TRUE on both implementations (OA pt=100 and BE pt=110) immediately after switch_core_codec_add_implementation(). Three generic a=rtpmap:%d %s/%d emission sites in switch_core_media.c now OR the flag with the existing channels > 1 check; one SDP-negotiation site copies the flag from mimp into the fresh pmap. Fix is minimally invasive and solves the concrete Mavenir-style IMS rejection ("no audio payload can be selected") on mono AMR-WB answers.
Correctness / Memory / ABI
- Zero-init guaranteed.
switch_core_perform_alloc()alwaysmemset(ptr, 0, memory)(src/switch_core_memory.c:543). Every allocator for these structs —switch_codec_implementationinswitch_core_codec_add_implementation(src/include/switch_loadable_module.h:528), pmap seeds atsrc/switch_core_media.c:2698/2702/2708, and the pmap grow path (~L1032) — goes throughswitch_core_alloc. New field defaults toSWITCH_FALSEon every code path, including recovery. No uninitialized-read risk. - ABI/layout preserved. Both fields are appended at the end of their respective structs. Existing binary consumers won't shift offsets. Matches the PR description.
- Flag propagation. At
src/switch_core_media.c:7988the negotiation path copiesmimp->always_emit_channels_in_rtpmapintopmapafterswitch_core_media_add_payload_map().switch_core_media_add_payload_map()itself doesn't read or set this field — correct: it has no access to theswitch_codec_implementationfor generic callers (proxy, t140/red, early-m-video, etc.). All non-AMR pmaps therefore stay FALSE. Desired behavior. - All 3 real emission sites covered: generate_m @ L12777, cur_payload_map branch @ L13665, full-codec enumeration @ L14191. The other
a=rtpmapprinters are for DTMF (telephone-event), CNG, t140/red, MULTI_ANSWER video, and recovery — none of those need/1. - No locks, no allocs, no error paths, no session locate. Nothing to leak, nothing to deadlock, nothing to crash. C90 clean (fields declared at struct top, no mid-block decls).
Minor gap (non-blocking)
- MULTI_ANSWER_AUDIO alt-rtpmap @ L13681 still emits
a=rtpmap:%d %s/%ldwith rate only, bypassing the flag. If a channel hasSCMF_MULTI_ANSWER_AUDIOenabled and AMR-WB is offered in both OA and BE, the non-primary pmap will lack/1. In practice B2BUA production profiles don't enable multi-answer on the same leg that talks to strict IMS peers, so this is a narrow-edge coverage gap, not a functional bug for the stated fix. Worth a follow-up if anyone ever stacks multi-answer with IMS. - Recovery path in
switch_core_media_recover_session()(~L17255) rebuildscur_payload_mapfrom channel variables (rtp_use_codec_name,rtp_use_codec_fmtp, etc.) but does not restorealways_emit_channels_in_rtpmap. The pmap is pool-zeroed so the flag will be FALSE on the recovered leg. If a recovered AMR-WB call has to re-offer SDP to a strict IMS peer, the re-INVITE will omit/1. This is a narrow scenario (recovery + IMS + AMR-WB re-offer), and the usual recovery flow rebuilds the impl viaswitch_loadable_module_get_codecs_sorted()which does re-readsmh->codecs[i]that still has the flag set — so subsequent full-enumeration re-offers via L14191 will be correct. Flagging for awareness; not a blocker.
Dependency / gating vs #567 and #568
- Branch
jira-tel-6982has exactly one commitf29ece36on top oftelnyx/telephony/deploy-development. It does NOT contain #567's26f81b6f(NUA bridge API) or #568's392a8f4(BerkeleyDb erase). - No file overlap: #568 touches mod_call_recovery (BerkeleyDb); #567 touches mod_telnyx/sofia profile loading. #569 touches codec interface struct, payload_map, mod_amrwb, and switch_core_media SDP emission.
- Conclusion: #569 is independent. #567's CHANGES_REQUESTED state does not gate #569. #568's APPROVED state is irrelevant. No merge-order constraint.
CI
reviewjob FAILED at##[error]Unable to resolve action 'team-telnyx/reviewpr-internal', not found→ pre-existing workflow/infra issue, unrelated to this PR. Observed on other recent PRs in the same repo.trigger telnyx_b2bua_builderCANCELLED after 24h default timeout; not a real failure.- No blocking CI signal. Build impact on this patch is mechanical (two trivial struct fields + one bool assignment + four OR conditions).
23-Rules sanity check
1/8 smallest-necessary fix ✓ 2/3 code read 3× ✓ 4 prefix incr N/A (no loops added) ✓ 6 no wheel reinvention (reuses existing rtpmap gate) ✓ 7 C90 ✓ 9 all functions exist ✓ 15 no allocs, no frees needed ✓ 16 no mutexes ✓ 17 no session locate ✓ 18/22 no error paths / no ptr derefs ✓ 20/21 leak/deadlock analysis ✓ 23 impact = SDP emission for AMR-WB only; does NOT affect any other codec because flag defaults FALSE everywhere else ✓
Verdict: APPROVE
Clean, targeted, RFC-compliant (RFC 4566 §5.14 allows explicit /channels and some strict peers require it). No safety concerns. Minor recovery/multi-answer coverage gaps noted for follow-up; neither blocks merge. 2nd CoC approval (after @tajamulTelnyx's LGTM).
— Dev-FreeSWITCH ⚙️
FreeSWITCH has never emitted the channels suffix in rtpmap for mono codecs (since commit 05dd02d in 2013), relying on RFC 4566's implicit default of 1. Some IMS/VoLTE peers (e.g. Mavenir) reject such answers with "no audio payload can be selected" and immediately BYE the call, requiring the explicit "/1".
Add a per-codec-impl flag always_emit_channels_in_rtpmap, plumbed into payload_map_t during SDP negotiation, and consulted at all three rtpmap emission sites in switch_core_media.c. Set the flag on both AMR-WB implementations (octet-aligned and bandwidth-efficient).
Both new struct fields are added at the end of their respective structs to preserve binary layout of existing fields.