Skip to content

TELCORE-303: keep AMR-WB codec selection and fmtp stable across re-INVITEs - #640

Open
dev-ryanc wants to merge 6 commits into
telnyx/telephony/deploy-developmentfrom
linear-telcore-303
Open

TELCORE-303: keep AMR-WB codec selection and fmtp stable across re-INVITEs#640
dev-ryanc wants to merge 6 commits into
telnyx/telephony/deploy-developmentfrom
linear-telcore-303

Conversation

@dev-ryanc

Copy link
Copy Markdown
Collaborator

Problem

A PSTN caller on AT&T VoLTE presses hold on an inbound call to a Call Control DID. The inbound leg is AMR-WB, transcoded to OPUS toward the customer SBC. Hold is accepted, then reverted by the originating IMS core 719 ms later (hold_accum_ms = 719). Android shows "Can't hold call"; iPhone/VoLTE tears the call down. AT&T eventually BYEs.

Two distinct defects, both verified in the customer pcap:

CSeq Caller offers We answered
1 initial PT 104 (octet-align=0) + PT 110 (octet-align=1) PT 104, mode-set=2;octet-align=0;max-red=0;mode-change-capability=2
3 hold PT 104 only PT 104, no a=fmtp:104 at all
4 resume PT 104 + PT 110 PT 110, octet-align=1
5 PT 104 only PT 104, no a=fmtp:104, while the codec stayed octet aligned internally

The carrier ACKs the hold answer at t=26.158 and re-INVITEs 9 ms later, so the fmtp omission is the trigger; the payload type flip is the failed recovery.

Root cause

mod_amrwb registers two implementations under a single iananame (mod_amrwb.c:926-951): octet aligned (ianacode 100) and bandwidth efficient (ianacode 110). They differ only in ianacode and fmtp — rate, ptime and channels are identical. AMR, G7221, SILK and iLBC register the same way.

1. Payload type flip. The "prioritize previously negotiated codec" loop compared iananame only, with no break, so every implementation sharing the name matched and the last one won. On the initial INVITE read_impl.iananame is unset, so the first match survived — which is why CSeq 1 and CSeq 4 answer differently to the same offer.

2. Codec kept when it should change. The loop deciding whether to reset asked whether any match was name/rate/ptime compatible, not whether the implementation actually selected was. A re-INVITE offering only the other implementation kept the running codec while the answer advertised the offered one.

3. Dropped fmtp. The a=fmtp line comes from cur_payload_map->fmtp_out, and switch_core_media_set_codec() is the only thing that populates it. A re-offer whose fmtp differs from the stored one fails fmtp_check_match() (field-count comparison, :928) and allocates a fresh payload map, so when the codec is kept and set_codec() is not called, the answer goes out with no fmtp at all.

An offer that omits a=ptime also matches every implementation a codec registers, because the ptime filter at :7903 is guarded on ptime being present — so this is not limited to AMR-WB.

Changes

All behaviour is gated behind the telnyx-strict-codec-match channel variable, off by default, modelled on the existing telnyx-strict-ptime in the same function. With it unset, every condition short-circuits and the original code paths run unchanged.

  • same_codec_impl() — one definition of "same implementation" (ianacode, iananame, ptime, rate), used by both decision sites so they cannot drift.
  • Prioritize loop — requires a full implementation match and latches the first exact one, so a later same-name match cannot overwrite it. Also skips re-asserting this leg's codec while the partner leg drives the renegotiation (CF_AWAITING_STREAM_CHANGE), so the partner's answer decides.
  • Reset decision — compares the implementation actually selected. When it changed, resets inline rather than deferring: the answer SDP is generated before the deferred reset runs on the media thread (:3891), so deferring would drop the fmtp from the answer.
  • fmtp carry-over — when the codec is kept but the payload map was rebuilt, copies the fmtp of the codec in use onto the new map, guarded on the implementation so a map cannot inherit another's fmtp.
  • strict-codec-match sofia profile parameter — profiles cannot carry channel variables, so this stamps the variable on inbound channels. Stored as a string rather than a profile flag so an absent parameter stays distinguishable from an explicit false and lets the global variable decide.

Scoping, lowest precedence to highest: global variable → sofia profile parameter → channel variable from the dialplan or Call Control. The whole fix acts only on re-INVITEs, which arrive long after the dialplan has run, so a per-call variable is effective.

Verification

Unittests/unit/switch_sdp.c, two sessions driving switch_core_media_negotiate_sdp() and asserting on the generated answer.

Flag Result
on PASSED 5/5
off (negative control) FAILED 3/5

With the flag off the log reproduces the defect verbatim: Sticking with previously negotiated codec AMR-WB but different pt 104 --> 110. Each of the three code changes has an assertion that fails without it. The second session exists because the first one changes implementation before reaching the re-offer, and that change forces a reset which repopulates the fmtp and hides the defect.

SIPp — new uac-amrwb-hold-resume.xml plus four re-INVITE variants against a local FS built from this branch.

Re-INVITE offers Flag on Baseline
same full list PT 104 octet-align=0 PT 110 octet-align=1
AMR-WB BE only, different fmtp PT 104 + fmtp PT 104, no fmtp
AMR-WB OA only PT 110 + fmtp PT 110 + fmtp
PCMU/PCMA/G722 only PT 0 PT 0

Behaviour changes in exactly the two broken cases; the other two are byte-identical. The baseline column reproduces the customer pcap.

Build — clean rebuild in freeswitch-docker-base:1.14.3, exit 0, zero warnings referencing any changed file.

Reviewer notes

  • This needs a clean build. struct sofia_profile gained a field, and this tree disables dependency tracking, so an incremental rebuild mixing old and new objects corrupts silently rather than failing.
  • Inline reset and locking. switch_core_media_set_codec() takes the session codec read/write locks and codec_init_mutex, with a single exit releasing all three. Calling it from the partner's thread during a partner-driven pass is a pre-existing ordering — the changed_pt branch already does exactly this — but this change exercises it more often. The deferred alternative is what drops the fmtp, so correctness rules it out.
  • First-match-wins. Where a remote offers the same implementation at two payload types, the latch now selects the first rather than falling through to the last. Both were arbitrary; this is a deliberate change under the flag.
  • Blast radius. The implementation comparison is a no-op for every codec registering one ianacode per name. Only AMR, AMR-WB, G7221, SILK and iLBC are affected.

Rollback

Unset telnyx-strict-codec-match (or set it false, or drop the profile parameter). Behaviour returns to current, byte for byte, without redeploying anything else.

Known gaps, filed separately

  • TELCORE-368 — with rtp_pass_codecs_on_stream_change, a B-leg renegotiation that never produces an SDP answer (488, timeout, partner teardown) leaves CF_AWAITING_STREAM_CHANGE set, and sofia.c:9397 then suppresses the A-leg 200 OK forever. Pre-existing.
  • TELCORE-371 — the B-leg answer decides the codec but not which implementation, so it cannot steer the AMR-WB octet-align variant. inherit_codec=true builds the caller string without fmtp; passthru carries fmtp but still names both implementations. Measured, pre-existing, unaffected by this change.

Neither is caused or fixed here, and neither affects the customer defect.

…tation

Codecs such as AMR and AMR-WB register several implementations under a
single iananame, and an offer that omits a=ptime matches every
implementation a codec has registered. The loop that re-selects a
previously negotiated codec compared iananame only, so every one of them
matched and the last won. On an AMR-WB re-INVITE offering both the
bandwidth efficient and octet aligned payload types, this flipped the
answer to the octet aligned variant mid-dialog.

Add same_codec_impl() and require a full implementation match, latching the
first exact one so a later match on the same name cannot overwrite it.
Gated behind the telnyx-strict-codec-match channel variable, off by
default; when unset the condition reduces to the original expression.
The loop deciding whether to keep the current codec asked whether any
match was interchangeable with the running one, not whether the
implementation we actually selected was. With several implementations
registered under one iananame, a re-INVITE offering only the other one
kept the running codec while the answer advertised the offered one.

Track the implementation behind the selected payload map and compare
against that. When it changed, reset inline rather than leaving it to the
deferred reset in the read loop: the answer SDP is generated before that
runs and takes a=fmtp from the payload map set_codec() fills in, so
deferring would drop the fmtp from the answer.

Also skip re-asserting this leg's previously negotiated codec while the
partner leg drives the renegotiation, so its answer decides the codec.

All gated behind telnyx-strict-codec-match; the original loop is kept
verbatim for the flag-off path.
The a=fmtp line in a generated answer comes from the payload map's
fmtp_out, and switch_core_media_set_codec() is the only thing that fills
it in. A re-offer carrying a different fmtp than the one we stored fails
fmtp_check_match() and allocates a fresh payload map, so when the codec is
kept and set_codec() is not called, the answer advertised the codec with no
a=fmtp at all.

Carry the fmtp of the codec being kept over to the new payload map, guarded
on the implementation so a map cannot inherit the fmtp of a different one.
Sofia profiles cannot carry channel variables, so telnyx-strict-codec-match
could only be set globally or per channel from the dialplan. Add a profile
parameter that stamps the variable on inbound channels, giving per-profile
scoping between the two.

Stored as a string rather than a profile flag so that an absent parameter
stays distinguishable from an explicit false and lets the global variable
decide. The stamp happens before the dialplan runs, so anything set later
on the channel still overrides it.
Exercises a single session through an initial offer carrying both AMR-WB
implementations and three re-offers: the same list, the same implementation
with a different fmtp, and only the other implementation. Asserts the
implementation stays put where it should, changes where it must, and that
the generated answer keeps advertising an fmtp in every case.
The existing sequence changes implementation before it reaches the re-offer,
and that change forces a codec reset which repopulates the fmtp, so the
assertion could not fail. Add a session that goes straight from the initial
offer to a re-offer of the implementation already in use carrying a
different fmtp, which is the hold case seen in the field.
@dev-ryanc
dev-ryanc requested a review from a team August 12, 2026 18:07
@dev-ryanc
dev-ryanc marked this pull request as ready for review August 12, 2026 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants