Skip to content

Commit

Permalink
RID order guaranteed by avoiding iteration over a map
Browse files Browse the repository at this point in the history
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
  • Loading branch information
j1elo committed Aug 1, 2024
1 parent 35b3ae1 commit 3ba0bf9
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 28 deletions.
2 changes: 1 addition & 1 deletion peerconnection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)})
}
}

Expand Down
37 changes: 21 additions & 16 deletions sdp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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
}
Expand All @@ -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:]

Check warning on line 260 in sdp.go

View workflow job for this annotation

GitHub Actions / lint / Go

var-naming: var ridId should be ridID (revive)
for _, rid := range rids {
if rid.id == ridId {
rid.paused = true
break
}
}
}
}
Expand Down Expand Up @@ -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

Check warning on line 509 in sdp.go

View workflow job for this annotation

GitHub Actions / lint / Go

var-naming: var ridId should be ridID (revive)
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, ";"))
Expand All @@ -533,6 +537,7 @@ func addTransceiverSDP(
}

type simulcastRid struct {
id string
attrValue string
paused bool
}
Expand All @@ -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 {
Expand Down
33 changes: 22 additions & 11 deletions sdp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}

Expand All @@ -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")
Expand Down Expand Up @@ -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'")
}
}
Expand Down

0 comments on commit 3ba0bf9

Please sign in to comment.