Skip to content

Commit b8d3a7b

Browse files
j1eloSean-Der
authored andcommitted
Fix disordered RIDs in SDP
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. Fixes #2838
1 parent 9836d58 commit b8d3a7b

File tree

3 files changed

+44
-28
lines changed

3 files changed

+44
-28
lines changed

peerconnection.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2500,7 +2500,7 @@ func (pc *PeerConnection) generateMatchedSDP(transceivers []*RTPTransceiver, use
25002500
mediaTransceivers := []*RTPTransceiver{t}
25012501

25022502
extensions, _ := rtpExtensionsFromMediaDescription(media)
2503-
mediaSections = append(mediaSections, mediaSection{id: midValue, transceivers: mediaTransceivers, matchExtensions: extensions, ridMap: getRids(media)})
2503+
mediaSections = append(mediaSections, mediaSection{id: midValue, transceivers: mediaTransceivers, matchExtensions: extensions, rids: getRids(media)})
25042504
}
25052505
}
25062506

sdp.go

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,8 @@ func trackDetailsFromSDP(log logging.LeveledLogger, s *sdp.SessionDescription) (
202202
id: trackID,
203203
rids: []string{},
204204
}
205-
for rid := range rids {
206-
simulcastTrack.rids = append(simulcastTrack.rids, rid)
205+
for _, rid := range rids {
206+
simulcastTrack.rids = append(simulcastTrack.rids, rid.id)
207207
}
208208

209209
tracksInMediaSection = []trackDetails{simulcastTrack}
@@ -238,13 +238,13 @@ func trackDetailsToRTPReceiveParameters(t *trackDetails) RTPReceiveParameters {
238238
return RTPReceiveParameters{Encodings: encodings}
239239
}
240240

241-
func getRids(media *sdp.MediaDescription) map[string]*simulcastRid {
242-
rids := map[string]*simulcastRid{}
241+
func getRids(media *sdp.MediaDescription) []*simulcastRid {
242+
rids := []*simulcastRid{}
243243
var simulcastAttr string
244244
for _, attr := range media.Attributes {
245245
if attr.Key == sdpAttributeRid {
246246
split := strings.Split(attr.Value, " ")
247-
rids[split[0]] = &simulcastRid{attrValue: attr.Value}
247+
rids = append(rids, &simulcastRid{id: split[0], attrValue: attr.Value})
248248
} else if attr.Key == sdpAttributeSimulcast {
249249
simulcastAttr = attr.Value
250250
}
@@ -257,9 +257,12 @@ func getRids(media *sdp.MediaDescription) map[string]*simulcastRid {
257257
ridStates := strings.Split(simulcastAttr, ";")
258258
for _, ridState := range ridStates {
259259
if ridState[:1] == "~" {
260-
rid := ridState[1:]
261-
if r, ok := rids[rid]; ok {
262-
r.paused = true
260+
ridID := ridState[1:]
261+
for _, rid := range rids {
262+
if rid.id == ridID {
263+
rid.paused = true
264+
break
265+
}
263266
}
264267
}
265268
}
@@ -499,15 +502,16 @@ func addTransceiverSDP(
499502
media.WithExtMap(sdp.ExtMap{Value: rtpExtension.ID, URI: extURL})
500503
}
501504

502-
if len(mediaSection.ridMap) > 0 {
503-
recvRids := make([]string, 0, len(mediaSection.ridMap))
505+
if len(mediaSection.rids) > 0 {
506+
recvRids := make([]string, 0, len(mediaSection.rids))
504507

505-
for rid := range mediaSection.ridMap {
506-
media.WithValueAttribute(sdpAttributeRid, rid+" recv")
507-
if mediaSection.ridMap[rid].paused {
508-
rid = "~" + rid
508+
for _, rid := range mediaSection.rids {
509+
ridID := rid.id
510+
media.WithValueAttribute(sdpAttributeRid, ridID+" recv")
511+
if rid.paused {
512+
ridID = "~" + ridID
509513
}
510-
recvRids = append(recvRids, rid)
514+
recvRids = append(recvRids, ridID)
511515
}
512516
// Simulcast
513517
media.WithValueAttribute(sdpAttributeSimulcast, "recv "+strings.Join(recvRids, ";"))
@@ -533,6 +537,7 @@ func addTransceiverSDP(
533537
}
534538

535539
type simulcastRid struct {
540+
id string
536541
attrValue string
537542
paused bool
538543
}
@@ -542,7 +547,7 @@ type mediaSection struct {
542547
transceivers []*RTPTransceiver
543548
data bool
544549
matchExtensions map[string]int
545-
ridMap map[string]*simulcastRid
550+
rids []*simulcastRid
546551
}
547552

548553
func bundleMatchFromRemote(matchBundleGroup *string) func(mid string) bool {

sdp_test.go

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -381,16 +381,18 @@ func TestPopulateSDP(t *testing.T) {
381381

382382
tr := &RTPTransceiver{kind: RTPCodecTypeVideo, api: api, codecs: me.videoCodecs}
383383
tr.setDirection(RTPTransceiverDirectionRecvonly)
384-
ridMap := map[string]*simulcastRid{
385-
"ridkey": {
384+
rids := []*simulcastRid{
385+
{
386+
id: "ridkey",
386387
attrValue: "some",
387388
},
388-
"ridPaused": {
389+
{
390+
id: "ridPaused",
389391
attrValue: "some2",
390392
paused: true,
391393
},
392394
}
393-
mediaSections := []mediaSection{{id: "video", transceivers: []*RTPTransceiver{tr}, ridMap: ridMap}}
395+
mediaSections := []mediaSection{{id: "video", transceivers: []*RTPTransceiver{tr}, rids: rids}}
394396

395397
d := &sdp.SessionDescription{}
396398

@@ -403,12 +405,14 @@ func TestPopulateSDP(t *testing.T) {
403405
if desc.MediaName.Media != "video" {
404406
continue
405407
}
406-
ridInSDP := getRids(desc)
407-
if ridKey, ok := ridInSDP["ridkey"]; ok && !ridKey.paused {
408-
ridFound++
409-
}
410-
if ridPaused, ok := ridInSDP["ridPaused"]; ok && ridPaused.paused {
411-
ridFound++
408+
ridsInSDP := getRids(desc)
409+
for _, rid := range ridsInSDP {
410+
if rid.id == "ridkey" && !rid.paused {
411+
ridFound++
412+
}
413+
if rid.id == "ridPaused" && rid.paused {
414+
ridFound++
415+
}
412416
}
413417
}
414418
assert.Equal(t, 2, ridFound, "All rid keys should be present")
@@ -631,7 +635,14 @@ func TestGetRIDs(t *testing.T) {
631635
rids := getRids(m[0])
632636

633637
assert.NotEmpty(t, rids, "Rid mapping should be present")
634-
if _, ok := rids["f"]; !ok {
638+
found := false
639+
for _, rid := range rids {
640+
if rid.id == "f" {
641+
found = true
642+
break
643+
}
644+
}
645+
if !found {
635646
assert.Fail(t, "rid values should contain 'f'")
636647
}
637648
}

0 commit comments

Comments
 (0)