Repository: pion/webrtc
Pull request #2220 (2022-05-13 01:17:39)

Fix incorrectly generated (i.e. conflicting or misassigned) extmap IDs when re-offering
#### Thanks

First big thanks to @k0nserv who originally reported this and put together an initial test showing the failure.

#### Background

This PR addresses an issue whereby under a specific (but not unusual) circumstance, pion fails in two different ways to generate the correct SDP extmap IDs for a track. Some browsers are more picky about this than others, but Firefox is known to be picky and this issue may be responsible for some reported incompatibilities.

Sequence to trigger the issue:

1. Pion side registers audio & video codecs and audio & video header extensions.
2. Pion side gets an audio-only offer from the remote, and responds with an answer.
3. At some later point in time, Pion side wants to add a video track and prepares a new offer.
4. Extmap IDs in the new offer SDP are usually incorrect (conflicting or misassigned).

#### Minimal example code

```
func main() {
  // Create a MediaEngine object to configure the supported codecs
  m := &webrtc.MediaEngine{}

  // Setup video codec
  m.RegisterCodec(webrtc.RTPCodecParameters{
    RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: webrtc.MimeTypeH264, ClockRate: 90000, Channels: 0, SDPFmtpLine: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=640c1f", RTCPFeedback: nil},
    PayloadType:        96,
  }, webrtc.RTPCodecTypeVideo)

  // Setup audio codec
  m.RegisterCodec(webrtc.RTPCodecParameters{
    RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: webrtc.MimeTypeOpus, ClockRate: 48000, Channels: 2, SDPFmtpLine: "minptime=10;useinbandfec=1", RTCPFeedback: nil},
    PayloadType:        111,
  }, webrtc.RTPCodecTypeAudio)

  m.RegisterHeaderExtension(webrtc.RTPHeaderExtensionCapability{URI: "urn:ietf:params:rtp-hdrext:sdes:mid"}, webrtc.RTPCodecTypeVideo)
  m.RegisterHeaderExtension(webrtc.RTPHeaderExtensionCapability{URI: "urn:3gpp:video-orientation"}, webrtc.RTPCodecTypeVideo)

  m.RegisterHeaderExtension(webrtc.RTPHeaderExtensionCapability{URI: "urn:ietf:params:rtp-hdrext:sdes:mid"}, webrtc.RTPCodecTypeAudio)
  m.RegisterHeaderExtension(webrtc.RTPHeaderExtensionCapability{URI: "urn:ietf:params:rtp-hdrext:ssrc-audio-level"}, webrtc.RTPCodecTypeAudio)

  // Create the API object with the MediaEngine
  api := webrtc.NewAPI(webrtc.WithMediaEngine(m))

  // Create a new RTCPeerConnection
  peerConnection, _ := api.NewPeerConnection(webrtc.Configuration{})
  peerConnection.AddTransceiverFromKind(webrtc.RTPCodecTypeAudio, webrtc.RTPTransceiverInit{ Direction: webrtc.RTPTransceiverDirectionSendrecv })

  sdp_snippet := `v=0
o=- 4596489990601351948 2 IN IP4 127.0.0.1
s=-
t=0 0
m=audio 9 UDP/TLS/RTP/SAVPF 111
a=mid:0
a=ice-ufrag:LgQ1
a=ice-pwd:2jUslCr2ub4FRFMtR3jRAPG+
a=ice-options:trickle renomination
a=fingerprint:sha-256 12:C7:4A:71:4E:87:8E:54:FF:ED:FB:D8:E2:E3:BA:A4:D3:24:E0:BD:D3:AB:B4:6B:FE:83:0D:C3:CB:56:76:2B
a=extmap:2 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=extmap:5 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
a=rtpmap:111 opus/48000/2
a=sendrecv
`

  // Remote side offers sdp_snippet
  peerConnection.SetRemoteDescription(webrtc.SessionDescription{ Type: webrtc.NewSDPType("offer"), SDP: sdp_snippet })

  // Pion side answers
  answer, _ := peerConnection.CreateAnswer(nil)
  peerConnection.SetLocalDescription(answer)

  // Pion side now wants to add a video track, so it makes an updated offer (which would get sent out-of-band in a real example)
  peerConnection.AddTransceiverFromKind(webrtc.RTPCodecTypeVideo, webrtc.RTPTransceiverInit{ Direction: webrtc.RTPTransceiverDirectionSendrecv })
  offer, _ := peerConnection.CreateOffer(nil)

  peerConnection.SetLocalDescription(offer)

  // Oops! The video section in the new offer contains two incorrectly assigned extmap IDs.
  fmt.Println(peerConnection.LocalDescription().SDP)
}
```

Output (SDP offer created by pion):

```
...
m=audio 9 UDP/TLS/RTP/SAVPF 111
a=extmap:2 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
...
m=video 9 UDP/TLS/RTP/SAVPF 96
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:2 urn:3gpp:video-orientation
...
```

Unfortunately both of the video extmap assignments are illegal.

The `urn:ietf:params:rtp-hdrext:sdes:mid` is assigned ID 1, but must actually be ID 2 (see: https://www.rfc-editor.org/rfc/rfc8843.html#section-12). This is the *misassignment* failure mode.

The `urn:3gpp:video-orientation` is assigned ID 2, but this conflicts with `urn:ietf:params:rtp-hdrext:ssrc-audio-level` in the `m=audio` section, see: https://www.rfc-editor.org/rfc/rfc8285.html#page-14). This is the *conflict* failure mode.

#### What this PR does

This PR fixes the logic in `getRTPParametersByKind` to avoid these two failure modes. Also, a new test is added.

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

```diff
@@            Coverage Diff             @@
##           master    #2220      +/-   ##
==========================================
- Coverage   77.28%   70.16%   -7.13%     
==========================================
  Files          87       64      -23     
  Lines        9157     3355    -5802     
==========================================
- Hits         7077     2354    -4723     
+ Misses       1649      873     -776     
+ Partials      431      128     -303     
```

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

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/2220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/2220/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%> (-100.00%)` | :arrow_down: |
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/2220/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: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/2220/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%> (-82.76%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/2220/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/2220/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/2220/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%> (-66.67%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/2220/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: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/2220/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `13.79% <0.00%> (-44.83%)` | :arrow_down: |
| [atomicbool.go](https://codecov.io/gh/pion/webrtc/pull/2220/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: |
| [dtlsrole.go](https://codecov.io/gh/pion/webrtc/pull/2220/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-ZHRsc3JvbGUuZ28=) | `71.42% <0.00%> (-28.58%)` | :arrow_down: |
| ... and [37 more](https://codecov.io/gh/pion/webrtc/pull/2220/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/2220?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/2220?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [5c737e1...fcf2592](https://codecov.io/gh/pion/webrtc/pull/2220?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).

adriancable: Hmm - wonder why the Simulcast tests are now failing. I'll need to look into that.
Sean-Der: No worries I have it @adriancable  :) 

Sorry I should have approved+commented. Thank you so much for grabbing it. Stuff like this helps so much, I bet this has bit a few people its a gnarly one and assumes lots of SDP/RTP knowledge 
Sean-Der: @adriancable The bug is some kind of race. If you add debug logging it goes away :( 
adriancable: @Sean-Der - hmm, I am questioning my change now. Give me a few moments to double check something - I'll get back to you.
adriancable: @Sean-Der - just pushed a new commit https://github.com/pion/webrtc/pull/2220/commits/09bcafafde64e6e9abe966c64fa8ff8a8a94f3ad which fixes something that may have been responsible for the data race.
Sean-Der: 🔥 nice! Mind squashing into one commit? If it’s green I am all for merging 
adriancable: @Sean-Der - squashed!

Patch:
diff --git a/mediaengine.go b/mediaengine.go
index 902c9030..ad2ee7f4 100644
--- a/mediaengine.go
+++ b/mediaengine.go
@@ -553,7 +553,7 @@ func (m *MediaEngine) getCodecsByKind(typ RTPCodecType) []RTPCodecParameters {
 	return nil
 }
 
-func (m *MediaEngine) getRTPParametersByKind(typ RTPCodecType, directions []RTPTransceiverDirection) RTPParameters {
+func (m *MediaEngine) getRTPParametersByKind(typ RTPCodecType, directions []RTPTransceiverDirection) RTPParameters { //nolint:gocognit
 	headerExtensions := make([]RTPHeaderExtensionParameter, 0)
 
 	// perform before locking to prevent recursive RLocks
@@ -569,9 +569,31 @@ func (m *MediaEngine) getRTPParametersByKind(typ RTPCodecType, directions []RTPT
 			}
 		}
 	} else {
-		for id, e := range m.headerExtensions {
+		for _, e := range m.headerExtensions {
 			if haveRTPTransceiverDirectionIntersection(e.allowedDirections, directions) && (e.isAudio && typ == RTPCodecTypeAudio || e.isVideo && typ == RTPCodecTypeVideo) {
-				headerExtensions = append(headerExtensions, RTPHeaderExtensionParameter{ID: id + 1, URI: e.uri})
+				usingNegotiatedID := false
+				for id := range m.negotiatedHeaderExtensions {
+					if m.negotiatedHeaderExtensions[id].uri == e.uri {
+						usingNegotiatedID = true
+						headerExtensions = append(headerExtensions, RTPHeaderExtensionParameter{ID: id, URI: e.uri})
+						break
+					}
+				}
+				if !usingNegotiatedID {
+					for id := 1; id < 15; id++ {
+						idAvailable := true
+						for checkID := range headerExtensions {
+							if headerExtensions[checkID].ID == id {
+								idAvailable = false
+								break
+							}
+						}
+						if _, taken := m.negotiatedHeaderExtensions[id]; idAvailable && !taken {
+							headerExtensions = append(headerExtensions, RTPHeaderExtensionParameter{ID: id, URI: e.uri})
+							break
+						}
+					}
+				}
 			}
 		}
 	}


Test patch:
diff --git a/mediaengine_test.go b/mediaengine_test.go
index 1acf0247..58e81ca6 100644
--- a/mediaengine_test.go
+++ b/mediaengine_test.go
@@ -460,3 +460,61 @@ func TestUpdateHeaderExtenstionToClonedMediaEngine(t *testing.T) {
 	validate(&src)
 	validate(src.copy())
 }
+
+func TestExtensionIdCollision(t *testing.T) {
+	mustParse := func(raw string) sdp.SessionDescription {
+		s := sdp.SessionDescription{}
+		assert.NoError(t, s.Unmarshal([]byte(raw)))
+		return s
+	}
+	sdpSnippet := `v=0
+o=- 4596489990601351948 2 IN IP4 127.0.0.1
+s=-
+t=0 0
+m=audio 9 UDP/TLS/RTP/SAVPF 111
+a=extmap:2 urn:ietf:params:rtp-hdrext:sdes:mid
+a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
+a=extmap:5 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
+a=rtpmap:111 opus/48000/2
+`
+
+	m := MediaEngine{}
+	assert.NoError(t, m.RegisterDefaultCodecs())
+
+	assert.NoError(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{sdp.SDESMidURI}, RTPCodecTypeVideo))
+	assert.NoError(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{"urn:3gpp:video-orientation"}, RTPCodecTypeVideo))
+
+	assert.NoError(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{sdp.SDESMidURI}, RTPCodecTypeAudio))
+	assert.NoError(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{sdp.AudioLevelURI}, RTPCodecTypeAudio))
+
+	assert.NoError(t, m.updateFromRemoteDescription(mustParse(sdpSnippet)))
+
+	assert.True(t, m.negotiatedAudio)
+	assert.False(t, m.negotiatedVideo)
+
+	id, audioNegotiated, videoNegotiated := m.getHeaderExtensionID(RTPHeaderExtensionCapability{sdp.ABSSendTimeURI})
+	assert.Equal(t, id, 0)
+	assert.False(t, audioNegotiated)
+	assert.False(t, videoNegotiated)
+
+	id, audioNegotiated, videoNegotiated = m.getHeaderExtensionID(RTPHeaderExtensionCapability{sdp.SDESMidURI})
+	assert.Equal(t, id, 2)
+	assert.True(t, audioNegotiated)
+	assert.False(t, videoNegotiated)
+
+	id, audioNegotiated, videoNegotiated = m.getHeaderExtensionID(RTPHeaderExtensionCapability{sdp.AudioLevelURI})
+	assert.Equal(t, id, 1)
+	assert.True(t, audioNegotiated)
+	assert.False(t, videoNegotiated)
+
+	params := m.getRTPParametersByKind(RTPCodecTypeVideo, []RTPTransceiverDirection{RTPTransceiverDirectionSendonly})
+	extensions := params.HeaderExtensions
+
+	assert.Equal(t, 2, len(extensions))
+	assert.Equal(t, sdp.SDESMidURI, extensions[0].URI)
+	assert.Equal(t, 2, extensions[0].ID)
+	assert.Equal(t, "urn:3gpp:video-orientation", extensions[1].URI)
+	assert.NotEqual(t, 1, extensions[1].ID)
+	assert.NotEqual(t, 2, extensions[1].ID)
+	assert.NotEqual(t, 5, extensions[1].ID)
+}