Repository: pion/webrtc
Pull request #1724 (2021-03-20 14:33:46)

Parse and compare FmtpLine on codec select
Exact match if fmtp parameters are not inconsistent.

e.g. default OPUS fmtp of the browsers are:
- Chrome:  `minptime=10;useinbandfec=1`
- Firefox: `maxplaybackrate=48000;stereo=1;useinbandfec=1`

They should be treated as matched.

### Reference issue
#1711


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1724?src=pr&el=h1) Report
> Merging [#1724](https://codecov.io/gh/pion/webrtc/pull/1724?src=pr&el=desc) (fc95b41) into [master](https://codecov.io/gh/pion/webrtc/commit/3db47301668ea709df4970812974d7baf124300b?el=desc) (3db4730) will **decrease** coverage by `5.01%`.
> The diff coverage is `73.33%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/1724/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2)](https://codecov.io/gh/pion/webrtc/pull/1724?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1724      +/-   ##
==========================================
- Coverage   74.97%   69.95%   -5.02%     
==========================================
  Files          79       58      -21     
  Lines        5754     2157    -3597     
==========================================
- Hits         4314     1509    -2805     
+ Misses       1065      544     -521     
+ Partials      375      104     -271     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `69.95% <73.33%> (+0.08%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/1724?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-cnRwY29kZWMuZ28=) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
| [fmtp.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-Zm10cC5nbw==) | `84.61% <84.61%> (ø)` | |
| [icetcp.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-aWNldGNwLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `0.00% <0.00%> (-47.83%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| ... and [33 more](https://codecov.io/gh/pion/webrtc/pull/1724/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/1724?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/webrtc/pull/1724?src=pr&el=footer). Last update [3db4730...fc95b41](https://codecov.io/gh/pion/webrtc/pull/1724?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

at-wat: @davidzhao what do you think? Test cases you added are satisfied as well in this version.
davidzhao: > @davidzhao what do you think? Test cases you added are satisfied as well in this version.

looks good to me. thanks for fixing it!

Patch:
diff --git a/fmtp.go b/fmtp.go
new file mode 100644
index 00000000..f82dcb82
--- /dev/null
+++ b/fmtp.go
@@ -0,0 +1,37 @@
+package webrtc
+
+import (
+	"strings"
+)
+
+type fmtp map[string]string
+
+// parseFmtp parses fmtp string.
+func parseFmtp(line string) fmtp {
+	f := fmtp{}
+	for _, p := range strings.Split(line, ";") {
+		pp := strings.SplitN(strings.TrimSpace(p), "=", 2)
+		key := strings.ToLower(pp[0])
+		var value string
+		if len(pp) > 1 {
+			value = pp[1]
+		}
+		f[key] = value
+	}
+	return f
+}
+
+// fmtpConsist checks that two FTMP parameters are not inconsistent.
+func fmtpConsist(a, b fmtp) bool {
+	for k, v := range a {
+		if vb, ok := b[k]; ok && !strings.EqualFold(vb, v) {
+			return false
+		}
+	}
+	for k, v := range b {
+		if va, ok := a[k]; ok && !strings.EqualFold(va, v) {
+			return false
+		}
+	}
+	return true
+}
diff --git a/mediaengine.go b/mediaengine.go
index bc3ed999..540f20a8 100644
--- a/mediaengine.go
+++ b/mediaengine.go
@@ -340,8 +340,9 @@ func (m *MediaEngine) matchRemoteCodec(remoteCodec RTPCodecParameters, typ RTPCo
 		codecs = m.audioCodecs
 	}
 
-	if strings.HasPrefix(remoteCodec.RTPCodecCapability.SDPFmtpLine, "apt=") {
-		payloadType, err := strconv.Atoi(strings.TrimPrefix(remoteCodec.RTPCodecCapability.SDPFmtpLine, "apt="))
+	remoteFmtp := parseFmtp(remoteCodec.RTPCodecCapability.SDPFmtpLine)
+	if apt, hasApt := remoteFmtp["apt"]; hasApt {
+		payloadType, err := strconv.Atoi(apt)
 		if err != nil {
 			return codecMatchNone, err
 		}
diff --git a/rtpcodec.go b/rtpcodec.go
index f05b98be..30a0cd90 100644
--- a/rtpcodec.go
+++ b/rtpcodec.go
@@ -97,19 +97,19 @@ const (
 // Used for lookup up a codec in an existing list to find a match
 // Returns codecMatchExact, codecMatchPartial, or codecMatchNone
 func codecParametersFuzzySearch(needle RTPCodecParameters, haystack []RTPCodecParameters) (RTPCodecParameters, codecMatchType) {
+	needleFmtp := parseFmtp(needle.RTPCodecCapability.SDPFmtpLine)
+
 	// First attempt to match on MimeType + SDPFmtpLine
-	// Exact matches means fmtp line cannot be empty
 	for _, c := range haystack {
 		if strings.EqualFold(c.RTPCodecCapability.MimeType, needle.RTPCodecCapability.MimeType) &&
-			c.RTPCodecCapability.SDPFmtpLine == needle.RTPCodecCapability.SDPFmtpLine {
+			fmtpConsist(needleFmtp, parseFmtp(c.RTPCodecCapability.SDPFmtpLine)) {
 			return c, codecMatchExact
 		}
 	}
 
-	// Fallback to just MimeType if either haystack or needle does not have fmtpline set
+	// Fallback to just MimeType
 	for _, c := range haystack {
-		if strings.EqualFold(c.RTPCodecCapability.MimeType, needle.RTPCodecCapability.MimeType) &&
-			(c.RTPCodecCapability.SDPFmtpLine == "" || needle.RTPCodecCapability.SDPFmtpLine == "") {
+		if strings.EqualFold(c.RTPCodecCapability.MimeType, needle.RTPCodecCapability.MimeType) {
 			return c, codecMatchPartial
 		}
 	}


Test patch:
diff --git a/fmtp_test.go b/fmtp_test.go
new file mode 100644
index 00000000..3f0a498e
--- /dev/null
+++ b/fmtp_test.go
@@ -0,0 +1,107 @@
+package webrtc
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestParseFmtp(t *testing.T) {
+	testCases := map[string]struct {
+		input    string
+		expected fmtp
+	}{
+		"OneParam": {
+			input: "key-name=value",
+			expected: fmtp{
+				"key-name": "value",
+			},
+		},
+		"OneParamWithWhiteSpeces": {
+			input: "\tkey-name=value ",
+			expected: fmtp{
+				"key-name": "value",
+			},
+		},
+		"TwoParams": {
+			input: "key-name=value;key2=value2",
+			expected: fmtp{
+				"key-name": "value",
+				"key2":     "value2",
+			},
+		},
+		"TwoParamsWithWhiteSpeces": {
+			input: "key-name=value;  \n\tkey2=value2 ",
+			expected: fmtp{
+				"key-name": "value",
+				"key2":     "value2",
+			},
+		},
+	}
+	for name, testCase := range testCases {
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			f := parseFmtp(testCase.input)
+			if !reflect.DeepEqual(testCase.expected, f) {
+				t.Errorf("Expected Fmtp params: %v, got: %v", testCase.expected, f)
+			}
+		})
+	}
+}
+
+func TestFmtpConsist(t *testing.T) {
+	consistString := map[bool]string{true: "consist", false: "inconsist"}
+
+	testCases := map[string]struct {
+		a, b    string
+		consist bool
+	}{
+		"Equal": {
+			a:       "key1=value1;key2=value2;key3=value3",
+			b:       "key1=value1;key2=value2;key3=value3",
+			consist: true,
+		},
+		"EqualWithWhitespaceVariants": {
+			a:       "key1=value1;key2=value2;key3=value3",
+			b:       "  key1=value1;  \nkey2=value2;\t\nkey3=value3",
+			consist: true,
+		},
+		"EqualWithCase": {
+			a:       "key1=value1;key2=value2;key3=value3",
+			b:       "key1=value1;key2=Value2;Key3=value3",
+			consist: true,
+		},
+		"OneHasExtraParam": {
+			a:       "key1=value1;key2=value2;key3=value3",
+			b:       "key1=value1;key2=value2;key3=value3;key4=value4",
+			consist: true,
+		},
+		"Inconsistent": {
+			a:       "key1=value1;key2=value2;key3=value3",
+			b:       "key1=value1;key2=different_value;key3=value3",
+			consist: false,
+		},
+		"Inconsistent_OneHasExtraParam": {
+			a:       "key1=value1;key2=value2;key3=value3;key4=value4",
+			b:       "key1=value1;key2=different_value;key3=value3",
+			consist: false,
+		},
+	}
+	for name, testCase := range testCases {
+		testCase := testCase
+		check := func(t *testing.T, a, b string) {
+			c := fmtpConsist(parseFmtp(a), parseFmtp(b))
+			if c != testCase.consist {
+				t.Errorf(
+					"'%s' and '%s' are expected to be %s, but treated as %s",
+					a, b, consistString[testCase.consist], consistString[c],
+				)
+			}
+		}
+		t.Run(name, func(t *testing.T) {
+			check(t, testCase.a, testCase.b)
+		})
+		t.Run(name+"_Reversed", func(t *testing.T) {
+			check(t, testCase.b, testCase.a)
+		})
+	}
+}