Repository: pion/webrtc
Pull request #2081 (2022-01-11 18:47:27)

Use audio negotiated PTs before video builtin PTs
This avoids a bug where negotiating Opus with PT 96 in an audio-only
session results in the VP8 codec being picked for a track (because
96 is the built-in type for VP8).

#### Description
Rework `getCodecByPayload` to check negotiated audio types before
built-in video types, which avoids this issue. The code previously
would always check video types before audio types, even if audio
had already been negotiated.

#### Reference issue
Fixes https://github.com/pion/webrtc/issues/2080


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/2081?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#2081](https://codecov.io/gh/pion/webrtc/pull/2081?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (bf3fa24) into [master](https://codecov.io/gh/pion/webrtc/commit/d4b645635c44c0aac05a7f73c1fbbd7b71ecd20a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (d4b6456) will **decrease** coverage by `6.53%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/2081/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/webrtc/pull/2081?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master    #2081      +/-   ##
==========================================
- Coverage   76.75%   70.22%   -6.54%     
==========================================
  Files          85       62      -23     
  Lines        8713     3231    -5482     
==========================================
- Hits         6688     2269    -4419     
+ Misses       1614      846     -768     
+ Partials      411      116     -295     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `70.22% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/2081?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uc3RhbnRzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-81.82%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwY29kZWMuZ28=) | `13.33% <0.00%> (-80.00%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-75.00%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `9.09% <0.00%> (-63.64%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlbXV4Lmdv) | `0.00% <0.00%> (-54.55%)` | :arrow_down: |
| [atomicbool.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YXRvbWljYm9vbC5nbw==) | `57.14% <0.00%> (-42.86%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-34.49%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `24.13% <0.00%> (-34.49%)` | :arrow_down: |
| ... and [37 more](https://codecov.io/gh/pion/webrtc/pull/2081/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

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


Patch:
diff --git a/mediaengine.go b/mediaengine.go
index d932eec5..4817f764 100644
--- a/mediaengine.go
+++ b/mediaengine.go
@@ -313,27 +313,39 @@ func (m *MediaEngine) copy() *MediaEngine {
 	return cloned
 }
 
+func findCodecByPayload(codecs []RTPCodecParameters, payloadType PayloadType) *RTPCodecParameters {
+	for _, codec := range codecs {
+		if codec.PayloadType == payloadType {
+			return &codec
+		}
+	}
+	return nil
+}
+
 func (m *MediaEngine) getCodecByPayload(payloadType PayloadType) (RTPCodecParameters, RTPCodecType, error) {
 	m.mu.RLock()
 	defer m.mu.RUnlock()
 
-	codecs := m.negotiatedVideoCodecs
-	if !m.negotiatedVideo {
-		codecs = m.videoCodecs
+	// if we've negotiated audio or video, check the negotiated types before our
+	// built-in payload types, to ensure we pick the codec the other side wants.
+	if m.negotiatedVideo {
+		if codec := findCodecByPayload(m.negotiatedVideoCodecs, payloadType); codec != nil {
+			return *codec, RTPCodecTypeVideo, nil
+		}
 	}
-	for _, codec := range codecs {
-		if codec.PayloadType == payloadType {
-			return codec, RTPCodecTypeVideo, nil
+	if m.negotiatedAudio {
+		if codec := findCodecByPayload(m.negotiatedAudioCodecs, payloadType); codec != nil {
+			return *codec, RTPCodecTypeAudio, nil
 		}
 	}
-
-	codecs = m.negotiatedAudioCodecs
-	if !m.negotiatedAudio {
-		codecs = m.audioCodecs
+	if !m.negotiatedVideo {
+		if codec := findCodecByPayload(m.videoCodecs, payloadType); codec != nil {
+			return *codec, RTPCodecTypeVideo, nil
+		}
 	}
-	for _, codec := range codecs {
-		if codec.PayloadType == payloadType {
-			return codec, RTPCodecTypeAudio, nil
+	if !m.negotiatedAudio {
+		if codec := findCodecByPayload(m.audioCodecs, payloadType); codec != nil {
+			return *codec, RTPCodecTypeAudio, nil
 		}
 	}
 


Test patch:
diff --git a/mediaengine_test.go b/mediaengine_test.go
index 32f10e28..6a15363e 100644
--- a/mediaengine_test.go
+++ b/mediaengine_test.go
@@ -111,6 +111,28 @@ a=fmtp:112 minptime=10; useinbandfec=1
 		assert.Equal(t, opusCodec.MimeType, MimeTypeOpus)
 	})
 
+	t.Run("Ambiguous Payload Type", func(t *testing.T) {
+		const opusSamePayload = `v=0
+o=- 4596489990601351948 2 IN IP4 127.0.0.1
+s=-
+t=0 0
+m=audio 9 UDP/TLS/RTP/SAVPF 96
+a=rtpmap:96 opus/48000/2
+a=fmtp:96 minptime=10; useinbandfec=1
+`
+
+		m := MediaEngine{}
+		assert.NoError(t, m.RegisterDefaultCodecs())
+		assert.NoError(t, m.updateFromRemoteDescription(mustParse(opusSamePayload)))
+
+		assert.False(t, m.negotiatedVideo)
+		assert.True(t, m.negotiatedAudio)
+
+		opusCodec, _, err := m.getCodecByPayload(96)
+		assert.NoError(t, err)
+		assert.Equal(t, opusCodec.MimeType, MimeTypeOpus)
+	})
+
 	t.Run("Case Insensitive", func(t *testing.T) {
 		const opusUpcase = `v=0
 o=- 4596489990601351948 2 IN IP4 127.0.0.1