Repository: pion/webrtc (go)

Issue:
Undefined iteration order when parsing SDP RID ids
Hi,

I've been observing a strange behavior in my WebRTC application, due to the fact that some times the simulcast layers are provided by Pion in a different order than the one in SDP messages.

After quite a bunch of debugging and printfs, ended up finding this line:

https://github.com/pion/webrtc/blob/2364173cedda65030a8a5f2110416387deb0083e/sdp.go#L197-L207

`getRids` returned a `map[string]*simulcastRid`, and then it gets traversed so the RIDs get appended into `simulcastTrack.rids`.

However, a big problem that I think is what I'm observing, is that Go does not guarantee any ordering on map traversal:

> 3. The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next.

[Source](https://go.dev/ref/spec#For_range)

**Concrete example**

This is the relevant part of an SDP Offer that is received:

```
a=rid:0 send
a=rid:1 send
a=rid:2 send
a=simulcast:send 0;1;2
a=sendonly
```

If then we add a printf of `incomingTrack.rids` right before this line: https://github.com/pion/webrtc/blob/v3.2.50/peerconnection.go#L1414
What *some* times we get is this: `[1 2 0]`.

So I believe that to respect the same ordering as given in the SDP's `a=simulcast` line, a different method of construction should be used that avoids unspecified orderings.

What do you think?

Pull request:
RID order guaranteed by avoiding iteration over a map
Map iteration order is not guaranteed by Go, so it's an error to iterate over a map in places where maintaining the same ordering is important.

This change replaces the map of simulcastRid{} with an array of the same type. The simulcastRid{} type is extended to hold the rid-id which previously was used as the key in the map.

Accesses to the map are replaced with range loops to find the desired rid-id for each case.

Default code format applied.
Unit tests passed.

Fixes #2838 


Patch:
diff --git a/peerconnection.go b/peerconnection.go
index 2b8a9038..a764f989 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -2520,7 +2520,7 @@ func (pc *PeerConnection) generateMatchedSDP(transceivers []*RTPTransceiver, use
 			mediaTransceivers := []*RTPTransceiver{t}
 
 			extensions, _ := rtpExtensionsFromMediaDescription(media)
-			mediaSections = append(mediaSections, mediaSection{id: midValue, transceivers: mediaTransceivers, matchExtensions: extensions, ridMap: getRids(media)})
+			mediaSections = append(mediaSections, mediaSection{id: midValue, transceivers: mediaTransceivers, matchExtensions: extensions, rids: getRids(media)})
 		}
 	}
 
diff --git a/sdp.go b/sdp.go
index f1b59940..cdeda226 100644
--- a/sdp.go
+++ b/sdp.go
@@ -202,8 +202,8 @@ func trackDetailsFromSDP(log logging.LeveledLogger, s *sdp.SessionDescription) (
 				id:       trackID,
 				rids:     []string{},
 			}
-			for rid := range rids {
-				simulcastTrack.rids = append(simulcastTrack.rids, rid)
+			for _, rid := range rids {
+				simulcastTrack.rids = append(simulcastTrack.rids, rid.id)
 			}
 
 			tracksInMediaSection = []trackDetails{simulcastTrack}
@@ -238,13 +238,13 @@ func trackDetailsToRTPReceiveParameters(t *trackDetails) RTPReceiveParameters {
 	return RTPReceiveParameters{Encodings: encodings}
 }
 
-func getRids(media *sdp.MediaDescription) map[string]*simulcastRid {
-	rids := map[string]*simulcastRid{}
+func getRids(media *sdp.MediaDescription) []*simulcastRid {
+	rids := []*simulcastRid{}
 	var simulcastAttr string
 	for _, attr := range media.Attributes {
 		if attr.Key == sdpAttributeRid {
 			split := strings.Split(attr.Value, " ")
-			rids[split[0]] = &simulcastRid{attrValue: attr.Value}
+			rids = append(rids, &simulcastRid{id: split[0], attrValue: attr.Value})
 		} else if attr.Key == sdpAttributeSimulcast {
 			simulcastAttr = attr.Value
 		}
@@ -257,9 +257,12 @@ func getRids(media *sdp.MediaDescription) map[string]*simulcastRid {
 		ridStates := strings.Split(simulcastAttr, ";")
 		for _, ridState := range ridStates {
 			if ridState[:1] == "~" {
-				rid := ridState[1:]
-				if r, ok := rids[rid]; ok {
-					r.paused = true
+				ridID := ridState[1:]
+				for _, rid := range rids {
+					if rid.id == ridID {
+						rid.paused = true
+						break
+					}
 				}
 			}
 		}
@@ -499,15 +502,16 @@ func addTransceiverSDP(
 		media.WithExtMap(sdp.ExtMap{Value: rtpExtension.ID, URI: extURL})
 	}
 
-	if len(mediaSection.ridMap) > 0 {
-		recvRids := make([]string, 0, len(mediaSection.ridMap))
+	if len(mediaSection.rids) > 0 {
+		recvRids := make([]string, 0, len(mediaSection.rids))
 
-		for rid := range mediaSection.ridMap {
-			media.WithValueAttribute(sdpAttributeRid, rid+" recv")
-			if mediaSection.ridMap[rid].paused {
-				rid = "~" + rid
+		for _, rid := range mediaSection.rids {
+			ridID := rid.id
+			media.WithValueAttribute(sdpAttributeRid, ridID+" recv")
+			if rid.paused {
+				ridID = "~" + ridID
 			}
-			recvRids = append(recvRids, rid)
+			recvRids = append(recvRids, ridID)
 		}
 		// Simulcast
 		media.WithValueAttribute(sdpAttributeSimulcast, "recv "+strings.Join(recvRids, ";"))
@@ -533,6 +537,7 @@ func addTransceiverSDP(
 }
 
 type simulcastRid struct {
+	id        string
 	attrValue string
 	paused    bool
 }
@@ -542,7 +547,7 @@ type mediaSection struct {
 	transceivers    []*RTPTransceiver
 	data            bool
 	matchExtensions map[string]int
-	ridMap          map[string]*simulcastRid
+	rids            []*simulcastRid
 }
 
 func bundleMatchFromRemote(matchBundleGroup *string) func(mid string) bool {


Test patch:
diff --git a/sdp_test.go b/sdp_test.go
index bda54fa6..c448ac6e 100644
--- a/sdp_test.go
+++ b/sdp_test.go
@@ -417,16 +417,18 @@ func TestPopulateSDP(t *testing.T) {
 
 		tr := &RTPTransceiver{kind: RTPCodecTypeVideo, api: api, codecs: me.videoCodecs}
 		tr.setDirection(RTPTransceiverDirectionRecvonly)
-		ridMap := map[string]*simulcastRid{
-			"ridkey": {
+		rids := []*simulcastRid{
+			{
+				id:        "ridkey",
 				attrValue: "some",
 			},
-			"ridPaused": {
+			{
+				id:        "ridPaused",
 				attrValue: "some2",
 				paused:    true,
 			},
 		}
-		mediaSections := []mediaSection{{id: "video", transceivers: []*RTPTransceiver{tr}, ridMap: ridMap}}
+		mediaSections := []mediaSection{{id: "video", transceivers: []*RTPTransceiver{tr}, rids: rids}}
 
 		d := &sdp.SessionDescription{}
 
@@ -439,12 +441,14 @@ func TestPopulateSDP(t *testing.T) {
 			if desc.MediaName.Media != "video" {
 				continue
 			}
-			ridInSDP := getRids(desc)
-			if ridKey, ok := ridInSDP["ridkey"]; ok && !ridKey.paused {
-				ridFound++
-			}
-			if ridPaused, ok := ridInSDP["ridPaused"]; ok && ridPaused.paused {
-				ridFound++
+			ridsInSDP := getRids(desc)
+			for _, rid := range ridsInSDP {
+				if rid.id == "ridkey" && !rid.paused {
+					ridFound++
+				}
+				if rid.id == "ridPaused" && rid.paused {
+					ridFound++
+				}
 			}
 		}
 		assert.Equal(t, 2, ridFound, "All rid keys should be present")
@@ -667,7 +671,14 @@ func TestGetRIDs(t *testing.T) {
 	rids := getRids(m[0])
 
 	assert.NotEmpty(t, rids, "Rid mapping should be present")
-	if _, ok := rids["f"]; !ok {
+	found := false
+	for _, rid := range rids {
+		if rid.id == "f" {
+			found = true
+			break
+		}
+	}
+	if !found {
 		assert.Fail(t, "rid values should contain 'f'")
 	}
 }