diff --git a/pkg/sip/media_port.go b/pkg/sip/media_port.go index 068ad9fa..43795a14 100644 --- a/pkg/sip/media_port.go +++ b/pkg/sip/media_port.go @@ -739,7 +739,15 @@ func (p *MediaPort) SetAnswer(offer *sdp.Offer, answerData []byte, codecs *msdk. // SetOffer decodes the offer from another party and returns encoded answer. To accept the offer, call SetConfig. func (p *MediaPort) SetOffer(offerData []byte, codecs *msdk.CodecSet, enc sdp.Encryption) (*sdp.Answer, *MediaConf, error) { - offer, err := sdp.ParseOfferWith(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 { + // 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 := sdp.ParseOfferWith(codecs, filteredOffer) if err != nil { return nil, nil, SDPError{Err: err} } @@ -747,6 +755,7 @@ func (p *MediaPort) SetOffer(offerData []byte, codecs *msdk.CodecSet, enc sdp.En if err != nil { return nil, nil, SDPError{Err: err} } + appendAMRFmtpToAnswer(&answer.SDP, filteredOffer) return answer, &MediaConf{MediaConfig: *mc}, nil } diff --git a/pkg/sip/sdp_amr.go b/pkg/sip/sdp_amr.go new file mode 100644 index 00000000..3e4467c2 --- /dev/null +++ b/pkg/sip/sdp_amr.go @@ -0,0 +1,276 @@ +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. +// 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 { + 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 00000000..ba2c78e4 --- /dev/null +++ b/pkg/sip/sdp_amr_test.go @@ -0,0 +1,177 @@ +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" + sdpsdk "github.com/livekit/media-sdk/sdp" + "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 TestSetOffer_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, err := NewMediaPortWith(1, logger.GetLogger(), newTestCallMonitor(t), c1, &MediaOptions{ + IP: newIP("1.1.1.1"), + Ports: rtcconfig.PortRange{Start: 10000}, + }, 16000) + require.NoError(t, err) + t.Cleanup(port.Close) + + 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.SetOffer(offer, codecs, sdpsdk.EncryptionNone) + require.Error(t, err) +} + +func TestSetOffer_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, err := NewMediaPortWith(1, logger.GetLogger(), newTestCallMonitor(t), c1, &MediaOptions{ + IP: newIP("1.1.1.1"), + Ports: rtcconfig.PortRange{Start: 10000}, + }, 16000) + require.NoError(t, err) + t.Cleanup(port.Close) + + // 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 +`) + answer, mc, err := port.SetOffer(offer, codecs, sdpsdk.EncryptionNone) + require.NoError(t, err) + require.NotNil(t, mc) + require.Equal(t, amrwb.SDPNameAndRate, mc.Audio.Codec.Info().SDPName) + require.Equal(t, byte(98), mc.Audio.Type) + + answerData, err := answer.SDP.Marshal() + 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")) +}