From de2ec3aef2c45fb5a0e22e5fa4053b3fc43722e3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 3 Aug 2026 07:36:30 +0000 Subject: [PATCH 1/2] Fix AMR answer fmtp mismatch for octet-align offers (#747) media-sdk answers AMR/AMR-WB with rtpmap only, so an octet-aligned offer was accepted as bandwidth-efficient and produced garbled audio. Before answering, strip AMR formats we cannot support (e.g. octet-align=1); when accepting bandwidth-efficient AMR, echo RFC 4867 required fmtp params. Co-authored-by: li xuanqun <793005378@qq.com> --- pkg/sip/media_port.go | 9 +- pkg/sip/sdp_amr.go | 274 ++++++++++++++++++++++++++++++++++++++++ pkg/sip/sdp_amr_test.go | 168 ++++++++++++++++++++++++ 3 files changed, 450 insertions(+), 1 deletion(-) create mode 100644 pkg/sip/sdp_amr.go create mode 100644 pkg/sip/sdp_amr_test.go diff --git a/pkg/sip/media_port.go b/pkg/sip/media_port.go index 12f3131f0..204224b6a 100644 --- a/pkg/sip/media_port.go +++ b/pkg/sip/media_port.go @@ -785,7 +785,13 @@ func (p *mediaPort) GenerateAnswer(offerData []byte) ([]byte, error) { return p.GetLocalSDP() } - offer, err := parseOfferWith(p.log, p.mon, p.codecs, offerData) + // Strip AMR/AMR-WB formats we cannot answer per RFC 4867 (e.g. octet-align=1) + // before negotiation, then echo required fmtp on the answer (livekit/sip#747). + filteredOffer, err := filterAMROfferSDP(offerData) + if err != nil { + return nil, SDPError{Err: err} + } + offer, err := parseOfferWith(p.log, p.mon, p.codecs, filteredOffer) if err != nil { return nil, SDPError{Err: err} } @@ -797,6 +803,7 @@ func (p *mediaPort) GenerateAnswer(offerData []byte) ([]byte, error) { if err != nil { return nil, SDPError{Err: err} } + appendAMRFmtpToAnswer(&answer.SDP, filteredOffer) answerData, err := answer.SDP.Marshal() if err != nil { diff --git a/pkg/sip/sdp_amr.go b/pkg/sip/sdp_amr.go new file mode 100644 index 000000000..cb62a3164 --- /dev/null +++ b/pkg/sip/sdp_amr.go @@ -0,0 +1,274 @@ +package sip + +import ( + "strings" + + "github.com/pion/sdp/v3" +) + +// AMR/AMR-WB SDP fmtp handling for answers (livekit/sip#747). +// +// media-sdk registers AMR/AMR-WB as bandwidth-efficient only (octet-align=0), +// and AnswerMedia emits rtpmap without echoing offer fmtp. Accepting an +// octet-aligned offer (or other unsupported modes) therefore produces a +// mismatched answer and garbled audio. RFC 4867 §8.3.1 requires the answerer +// to echo certain parameters for the accepted payload type (or reject it). +// +// Until media-sdk can negotiate octet-aligned RTP we: +// 1. strip unsupported AMR formats from the offer before answering, and +// 2. echo the required fmtp attributes for any AMR format we accept. + +var amrAnswerUnsupported = map[string]func(string) bool{ + // Default is 0 (bandwidth-efficient). octet-align=1 needs a different + // RTP payload format that media-sdk does not implement yet. + "octet-align": func(v string) bool { return v == "1" }, + // CRC-protected frames are not implemented. + "crc": func(v string) bool { return v == "1" }, + // Robust sorting is not implemented. + "robust-sorting": func(v string) bool { return v == "1" }, + // Interleaving requires a different de-interleave path. + "interleaving": func(v string) bool { return v != "" && v != "0" }, +} + +func isAMRName(name string) bool { + n := strings.ToUpper(name) + return n == "AMR" || n == "AMR-WB" +} + +func parseFmtpParams(fmtp string) map[string]string { + out := make(map[string]string) + for _, part := range strings.Split(fmtp, ";") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + k, v, ok := strings.Cut(part, "=") + if !ok { + out[strings.ToLower(strings.TrimSpace(part))] = "" + continue + } + out[strings.ToLower(strings.TrimSpace(k))] = strings.TrimSpace(v) + } + return out +} + +func amrFmtpUnsupported(fmtp string) bool { + if fmtp == "" { + // No fmtp → octet-align defaults to 0 (bandwidth-efficient). + return false + } + params := parseFmtpParams(fmtp) + for key, bad := range amrAnswerUnsupported { + if v, ok := params[key]; ok && bad(v) { + return true + } + } + return false +} + +func amrFmtpForAnswer(offerFmtp string) string { + if offerFmtp == "" || amrFmtpUnsupported(offerFmtp) { + return "" + } + params := parseFmtpParams(offerFmtp) + var parts []string + // RFC 4867 §8.3.1: answerer MUST include these if present in the offer + // for the accepted payload type (omit defaults). + for _, key := range []string{ + "octet-align", + "mode-change-capability", + "max-red", + } { + v, ok := params[key] + if !ok { + continue + } + // Skip octet-align=0 (default). + if key == "octet-align" && (v == "" || v == "0") { + continue + } + parts = append(parts, key+"="+v) + } + // mode-set / mode-change-period / mode-change-neighbor may be answered + // with a subset; echo offer values when present so both sides agree. + for _, key := range []string{ + "mode-set", + "mode-change-period", + "mode-change-neighbor", + } { + if v, ok := params[key]; ok { + parts = append(parts, key+"="+v) + } + } + return strings.Join(parts, ";") +} + +func attrPayloadType(value string) string { + pt, _, ok := strings.Cut(value, " ") + if !ok { + return value + } + return pt +} + +func rtpmapCodecName(value string) string { + _, rest, ok := strings.Cut(value, " ") + if !ok { + return "" + } + name, _, _ := strings.Cut(rest, "/") + return name +} + +// filterAMROfferSDP removes AMR/AMR-WB dynamic formats whose fmtp we cannot +// answer correctly, returning a rewritten offer suitable for media-sdk Answer. +// Remaining formats (including bandwidth-efficient AMR) are left intact. +func filterAMROfferSDP(offerData []byte) ([]byte, error) { + var offer sdp.SessionDescription + if err := offer.Unmarshal(offerData); err != nil { + return offerData, err + } + changed := filterOfferAMRFormats(&offer) + if !changed { + return offerData, nil + } + out, err := offer.Marshal() + if err != nil { + return offerData, err + } + return out, nil +} + +// filterOfferAMRFormats removes unsupported AMR formats in-place. +// Returns true if the description was modified. +func filterOfferAMRFormats(offer *sdp.SessionDescription) bool { + if offer == nil { + return false + } + changed := false + for _, md := range offer.MediaDescriptions { + if md.MediaName.Media != "audio" { + continue + } + dropPT := make(map[string]struct{}) + rtpmapNameByPT := make(map[string]string) + fmtpByPT := make(map[string]string) + for _, a := range md.Attributes { + switch a.Key { + case "rtpmap": + pt := attrPayloadType(a.Value) + rtpmapNameByPT[pt] = rtpmapCodecName(a.Value) + case "fmtp": + pt, rest, ok := strings.Cut(a.Value, " ") + if ok { + fmtpByPT[pt] = rest + } + } + } + for pt, name := range rtpmapNameByPT { + if !isAMRName(name) { + continue + } + if amrFmtpUnsupported(fmtpByPT[pt]) { + dropPT[pt] = struct{}{} + } + } + if len(dropPT) == 0 { + continue + } + changed = true + filtered := make([]string, 0, len(md.MediaName.Formats)) + for _, f := range md.MediaName.Formats { + if _, drop := dropPT[f]; !drop { + filtered = append(filtered, f) + } + } + md.MediaName.Formats = filtered + attrs := make([]sdp.Attribute, 0, len(md.Attributes)) + for _, a := range md.Attributes { + switch a.Key { + case "rtpmap", "fmtp": + if _, drop := dropPT[attrPayloadType(a.Value)]; drop { + continue + } + } + attrs = append(attrs, a) + } + md.Attributes = attrs + } + return changed +} + +// appendAMRFmtpToAnswer mutates answer SDP: for each accepted AMR payload type, +// copy the offer's answerable fmtp onto the answer (RFC 4867 §8.3.1). +func appendAMRFmtpToAnswer(answer *sdp.SessionDescription, offerData []byte) { + if answer == nil { + return + } + var offer sdp.SessionDescription + if err := offer.Unmarshal(offerData); err != nil { + return + } + + offerFmtpByPT := map[string]string{} + for _, md := range offer.MediaDescriptions { + if md.MediaName.Media != "audio" { + continue + } + rtpNames := map[string]string{} + for _, a := range md.Attributes { + if a.Key == "rtpmap" { + rtpNames[attrPayloadType(a.Value)] = rtpmapCodecName(a.Value) + } + } + for _, a := range md.Attributes { + if a.Key != "fmtp" { + continue + } + pt, rest, ok := strings.Cut(a.Value, " ") + if !ok || !isAMRName(rtpNames[pt]) { + continue + } + offerFmtpByPT[pt] = rest + } + } + if len(offerFmtpByPT) == 0 { + return + } + + for _, md := range answer.MediaDescriptions { + if md.MediaName.Media != "audio" { + continue + } + haveFmtp := map[string]struct{}{} + rtpNames := map[string]string{} + for _, a := range md.Attributes { + switch a.Key { + case "fmtp": + haveFmtp[attrPayloadType(a.Value)] = struct{}{} + case "rtpmap": + rtpNames[attrPayloadType(a.Value)] = rtpmapCodecName(a.Value) + } + } + for pt, name := range rtpNames { + if !isAMRName(name) { + continue + } + if _, ok := haveFmtp[pt]; ok { + continue + } + offerFmtp, ok := offerFmtpByPT[pt] + if !ok { + continue + } + echo := amrFmtpForAnswer(offerFmtp) + if echo == "" { + continue + } + md.Attributes = append(md.Attributes, sdp.Attribute{ + Key: "fmtp", + Value: pt + " " + echo, + }) + } + } +} diff --git a/pkg/sip/sdp_amr_test.go b/pkg/sip/sdp_amr_test.go new file mode 100644 index 000000000..3a1b2f896 --- /dev/null +++ b/pkg/sip/sdp_amr_test.go @@ -0,0 +1,168 @@ +package sip + +import ( + "strings" + "testing" + + "github.com/pion/sdp/v3" + "github.com/stretchr/testify/require" + + msdk "github.com/livekit/media-sdk" + "github.com/livekit/media-sdk/amrwb" + "github.com/livekit/media-sdk/dtmf" + "github.com/livekit/media-sdk/g711" + "github.com/livekit/media-sdk/g722" + "github.com/livekit/mediatransportutil/pkg/rtcconfig" + "github.com/livekit/protocol/logger" +) + +func amrEnabledCodecs() *msdk.CodecSet { + s := defaultCodecs.NewSet() + s.SetEnabled(amrwb.SDPNameAndRate, true) + return s +} + +func TestFilterAMROfferSDP_OctetAlign(t *testing.T) { + // Offer from livekit/sip#747 style: PT 96 is octet-aligned (unsupported), + // PT 98 is bandwidth-efficient with mode-change-capability (supported). + offer := []byte(`v=0 +o=- 0 0 IN IP4 1.2.3.4 +s=- +c=IN IP4 1.2.3.4 +t=0 0 +m=audio 10000 RTP/AVP 96 98 97 +a=rtpmap:96 AMR-WB/16000 +a=fmtp:96 octet-align=1;mode-change-capability=2 +a=rtpmap:98 AMR-WB/16000 +a=fmtp:98 mode-change-capability=2 +a=rtpmap:97 telephone-event/8000 +a=fmtp:97 0-15 +`) + filtered, err := filterAMROfferSDP(offer) + require.NoError(t, err) + s := string(filtered) + require.NotContains(t, s, "rtpmap:96") + require.NotContains(t, s, "fmtp:96") + require.Contains(t, s, "rtpmap:98 AMR-WB/16000") + require.Contains(t, s, "fmtp:98 mode-change-capability=2") + require.Contains(t, s, "rtpmap:97 telephone-event/8000") + require.Contains(t, s, "m=audio 10000 RTP/AVP 98 97") +} + +func TestFilterAMROfferSDP_OnlyOctetAlign(t *testing.T) { + offer := []byte(`v=0 +o=- 0 0 IN IP4 1.2.3.4 +s=- +c=IN IP4 1.2.3.4 +t=0 0 +m=audio 10000 RTP/AVP 96 97 +a=rtpmap:96 AMR-WB/16000 +a=fmtp:96 octet-align=1;mode-change-capability=2 +a=rtpmap:97 telephone-event/8000 +a=fmtp:97 0-15 +`) + filtered, err := filterAMROfferSDP(offer) + require.NoError(t, err) + s := string(filtered) + require.NotContains(t, s, "AMR-WB") + require.Contains(t, s, "telephone-event") + require.Contains(t, s, "m=audio 10000 RTP/AVP 97") +} + +func TestAppendAMRFmtpToAnswer_EchoesModeChangeCapability(t *testing.T) { + offer := []byte(`v=0 +o=- 0 0 IN IP4 1.2.3.4 +s=- +c=IN IP4 1.2.3.4 +t=0 0 +m=audio 10000 RTP/AVP 98 +a=rtpmap:98 AMR-WB/16000 +a=fmtp:98 mode-change-capability=2 +`) + var answer sdp.SessionDescription + require.NoError(t, answer.Unmarshal([]byte(`v=0 +o=- 0 0 IN IP4 5.6.7.8 +s=LiveKit +c=IN IP4 5.6.7.8 +t=0 0 +m=audio 20000 RTP/AVP 98 +a=rtpmap:98 AMR-WB/16000 +a=ptime:20 +a=sendrecv +`))) + appendAMRFmtpToAnswer(&answer, offer) + out, err := answer.Marshal() + require.NoError(t, err) + require.Contains(t, string(out), "a=fmtp:98 mode-change-capability=2") +} + +func TestAMRFmtpForAnswer(t *testing.T) { + require.Equal(t, "", amrFmtpForAnswer("octet-align=1;mode-change-capability=2")) + require.Equal(t, "mode-change-capability=2", amrFmtpForAnswer("mode-change-capability=2")) + require.Equal(t, "mode-change-capability=2;mode-set=0,1,2", amrFmtpForAnswer("mode-change-capability=2;mode-set=0,1,2")) + require.Equal(t, "", amrFmtpForAnswer("octet-align=0")) // default omitted + require.Equal(t, "", amrFmtpForAnswer("")) +} + +func TestGenerateAnswer_AMRRejectsOctetAlignOnly(t *testing.T) { + // Exact mismatch from livekit/sip#747: offer only has octet-aligned AMR-WB. + // We must reject rather than answer without fmtp (which implies octet-align=0). + codecs := amrEnabledCodecs() + c1, _ := newUDPPipe() + port := newTestPort(t, logger.NewTestLogger(t), c1, &MediaOptions{ + IP: newIP("1.1.1.1"), + Ports: rtcconfig.PortRange{Start: 10000}, + Codecs: codecs, + }, 16000) + + offer := []byte(`v=0 +o=- 0 0 IN IP4 2.2.2.2 +s=- +c=IN IP4 2.2.2.2 +t=0 0 +m=audio 20000 RTP/AVP 96 +a=rtpmap:96 AMR-WB/16000 +a=fmtp:96 octet-align=1;mode-change-capability=2 +`) + _, err := port.GenerateAnswer(offer) + require.Error(t, err) +} + +func TestGenerateAnswer_AMRAcceptsBandwidthEfficientAndEchoesFmtp(t *testing.T) { + codecs := amrEnabledCodecs() + // Disable G.711/G.722 so negotiation must pick AMR-WB. + codecs.SetEnabled(g711.ALawSDPNameAndRate, false) + codecs.SetEnabled(g711.ULawSDPNameAndRate, false) + codecs.SetEnabled(g722.SDPNameAndRate, false) + codecs.SetEnabled(dtmf.SDPNameAndRate, true) + + c1, _ := newUDPPipe() + port := newTestPort(t, logger.NewTestLogger(t), c1, &MediaOptions{ + IP: newIP("1.1.1.1"), + Ports: rtcconfig.PortRange{Start: 10000}, + Codecs: codecs, + }, 16000) + + // PT 96 unsupported, PT 98 supported — answer must select 98 and echo fmtp. + offer := []byte(`v=0 +o=- 0 0 IN IP4 2.2.2.2 +s=- +c=IN IP4 2.2.2.2 +t=0 0 +m=audio 20000 RTP/AVP 96 98 97 +a=rtpmap:96 AMR-WB/16000 +a=fmtp:96 octet-align=1;mode-change-capability=2 +a=rtpmap:98 AMR-WB/16000 +a=fmtp:98 mode-change-capability=2 +a=rtpmap:97 telephone-event/8000 +a=fmtp:97 0-15 +`) + answerData, err := port.GenerateAnswer(offer) + require.NoError(t, err) + s := string(answerData) + require.Contains(t, s, "a=rtpmap:98 AMR-WB/16000") + require.Contains(t, s, "a=fmtp:98 mode-change-capability=2") + require.NotContains(t, s, "rtpmap:96") + // Ensure we did not invent octet-align=1 in the answer. + require.False(t, strings.Contains(s, "octet-align=1")) +} From ea9eb25ca01ff6290734f11191a49b85aa8345b6 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 3 Aug 2026 07:55:56 +0000 Subject: [PATCH 2/2] Treat AMR offer filter failures as non-fatal If filterAMROfferSDP cannot parse or re-serialize the offer, fall back to the original SDP and continue negotiation instead of aborting the call. Co-authored-by: li xuanqun <793005378@qq.com> --- pkg/sip/media_port.go | 4 ++- pkg/sip/sdp_amr.go | 60 ++++++++++++++++++++++++++++++++--------- pkg/sip/sdp_amr_test.go | 36 ++++++++++++++++++++++++- 3 files changed, 86 insertions(+), 14 deletions(-) diff --git a/pkg/sip/media_port.go b/pkg/sip/media_port.go index 204224b6a..db9d8f0b4 100644 --- a/pkg/sip/media_port.go +++ b/pkg/sip/media_port.go @@ -789,7 +789,9 @@ func (p *mediaPort) GenerateAnswer(offerData []byte) ([]byte, error) { // before negotiation, then echo required fmtp on the answer (livekit/sip#747). filteredOffer, err := filterAMROfferSDP(offerData) if err != nil { - return nil, SDPError{Err: err} + // Non-fatal: fall back to the original offer and let media-sdk parse it. + p.log.Debugw("cannot filter AMR formats from offer", "error", err) + filteredOffer = offerData } offer, err := parseOfferWith(p.log, p.mon, p.codecs, filteredOffer) if err != nil { diff --git a/pkg/sip/sdp_amr.go b/pkg/sip/sdp_amr.go index cb62a3164..d0e22c926 100644 --- a/pkg/sip/sdp_amr.go +++ b/pkg/sip/sdp_amr.go @@ -123,6 +123,8 @@ func rtpmapCodecName(value string) string { // filterAMROfferSDP removes AMR/AMR-WB dynamic formats whose fmtp we cannot // answer correctly, returning a rewritten offer suitable for media-sdk Answer. // Remaining formats (including bandwidth-efficient AMR) are left intact. +// On parse/marshal failure it returns the original offerData with an error; +// callers should treat the error as non-fatal and continue with the original. func filterAMROfferSDP(offerData []byte) ([]byte, error) { var offer sdp.SessionDescription if err := offer.Unmarshal(offerData); err != nil { @@ -240,13 +242,9 @@ func appendAMRFmtpToAnswer(answer *sdp.SessionDescription, offerData []byte) { if md.MediaName.Media != "audio" { continue } - haveFmtp := map[string]struct{}{} rtpNames := map[string]string{} for _, a := range md.Attributes { - switch a.Key { - case "fmtp": - haveFmtp[attrPayloadType(a.Value)] = struct{}{} - case "rtpmap": + if a.Key == "rtpmap" { rtpNames[attrPayloadType(a.Value)] = rtpmapCodecName(a.Value) } } @@ -254,9 +252,6 @@ func appendAMRFmtpToAnswer(answer *sdp.SessionDescription, offerData []byte) { if !isAMRName(name) { continue } - if _, ok := haveFmtp[pt]; ok { - continue - } offerFmtp, ok := offerFmtpByPT[pt] if !ok { continue @@ -265,10 +260,51 @@ func appendAMRFmtpToAnswer(answer *sdp.SessionDescription, offerData []byte) { if echo == "" { continue } - md.Attributes = append(md.Attributes, sdp.Attribute{ - Key: "fmtp", - Value: pt + " " + echo, - }) + // media-sdk already emits its own fmtp for AMR (e.g. octet-align=0 + // since livekit/sip#781); merge the echoed offer params into it + // instead of appending a duplicate attribute. + merged := false + for i := range md.Attributes { + a := &md.Attributes[i] + if a.Key != "fmtp" || attrPayloadType(a.Value) != pt { + continue + } + _, existing, _ := strings.Cut(a.Value, " ") + a.Value = pt + " " + mergeFmtp(existing, echo) + merged = true + } + if !merged { + md.Attributes = append(md.Attributes, sdp.Attribute{ + Key: "fmtp", + Value: pt + " " + echo, + }) + } + } + } +} + +// mergeFmtp appends the params from echo to existing without duplicating keys +// that existing already declares. +func mergeFmtp(existing, echo string) string { + if echo == "" { + return existing + } + have := parseFmtpParams(existing) + var b strings.Builder + b.WriteString(existing) + for _, part := range strings.Split(echo, ";") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + k, _, _ := strings.Cut(part, "=") + if _, ok := have[strings.ToLower(strings.TrimSpace(k))]; ok { + continue + } + if b.Len() > 0 { + b.WriteString(";") } + b.WriteString(part) } + return b.String() } diff --git a/pkg/sip/sdp_amr_test.go b/pkg/sip/sdp_amr_test.go index 3a1b2f896..1f512c528 100644 --- a/pkg/sip/sdp_amr_test.go +++ b/pkg/sip/sdp_amr_test.go @@ -96,6 +96,38 @@ a=sendrecv require.Contains(t, string(out), "a=fmtp:98 mode-change-capability=2") } +func TestAppendAMRFmtpToAnswer_MergesWithExistingFmtp(t *testing.T) { + // Since livekit/sip#781 media-sdk emits its own AMR fmtp (octet-align=0). + // The echoed offer params must be merged into it, not appended twice. + offer := []byte(`v=0 +o=- 0 0 IN IP4 1.2.3.4 +s=- +c=IN IP4 1.2.3.4 +t=0 0 +m=audio 10000 RTP/AVP 98 +a=rtpmap:98 AMR-WB/16000 +a=fmtp:98 mode-change-capability=2 +`) + var answer sdp.SessionDescription + require.NoError(t, answer.Unmarshal([]byte(`v=0 +o=- 0 0 IN IP4 5.6.7.8 +s=LiveKit +c=IN IP4 5.6.7.8 +t=0 0 +m=audio 20000 RTP/AVP 98 +a=rtpmap:98 AMR-WB/16000 +a=fmtp:98 octet-align=0 +a=ptime:20 +a=sendrecv +`))) + appendAMRFmtpToAnswer(&answer, offer) + out, err := answer.Marshal() + require.NoError(t, err) + s := string(out) + require.Contains(t, s, "a=fmtp:98 octet-align=0;mode-change-capability=2") + require.NotContains(t, s, "octet-align=1") +} + func TestAMRFmtpForAnswer(t *testing.T) { require.Equal(t, "", amrFmtpForAnswer("octet-align=1;mode-change-capability=2")) require.Equal(t, "mode-change-capability=2", amrFmtpForAnswer("mode-change-capability=2")) @@ -161,7 +193,9 @@ a=fmtp:97 0-15 require.NoError(t, err) s := string(answerData) require.Contains(t, s, "a=rtpmap:98 AMR-WB/16000") - require.Contains(t, s, "a=fmtp:98 mode-change-capability=2") + // media-sdk emits octet-align=0 (livekit/sip#781); the echoed offer fmtp is + // merged into it per RFC 4867 §8.3.1. + require.Contains(t, s, "a=fmtp:98 octet-align=0;mode-change-capability=2") require.NotContains(t, s, "rtpmap:96") // Ensure we did not invent octet-align=1 in the answer. require.False(t, strings.Contains(s, "octet-align=1"))