Skip to content

Commit

Permalink
Fix disordered RIDs in SDP
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.

Fixes #2838
  • Loading branch information
j1elo authored and Sean-Der committed Aug 1, 2024
1 parent 35b3ae1 commit cbbb1c2
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:]
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
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 cbbb1c2

Please sign in to comment.