Use RIDs for SSRC resolution

boks1971 · Sean-Der · commit ef285fbe0917 · 2021-10-29T19:42:56.000-04:00
Problem: -------- Firefox (testing with Firefox 93) sends an `offer` with simulcast track which includes both RIDs and SSRCs. When track details are gathered, RIDs are fetched using `getRids` which returns a map. A `range` is done on the returned map and RIDs are appended to the `rids` array in `trackDetails`. And then SSRCs are read from SDP and set into the `ssrcs` array of `trackDetails`. As map range order is not guaranteed, some times the RID index and SSRC index in their respective arrays do not match. Due to the mismatch, services like ion-sfu which rely on RID to find the correct spatial layer get confused and end up forwarding the wrong layer. Solution(s): ------------ There are three possible solutions I could think of 1. The simplest is to not populate SSRCs in `trackDetails` if RIDs are available. Let `handleIncomingSSRC` do the SSRC resolution based on RID. According to RFC 8853 (https://www.ietf.org/rfc/rfc8853.pdf), the binding to SSRC should happen using RID. This is the change I have made in this PR. See testing below for browsers tested. 2. Look for `simulcast` attribute in SDP and take the RID ordering from that line in `getRids` if that attribute is available. If not fall back to `rid` attribute. Also, change `getRids` to return an array. But, I cannot find an RFC which defines behaviour when both RID and SSRC are used in `offer`. The question is, "Will the `ssrc` attribute ordering match the `rid` ordering?". If that is not guaranteed, this will run into trouble. This should be easy to do though if we want to go down this route. 3. The hardest option is to change the receiver SSRC based on RID. But, that makes it too complicated (for example if we have to change SSRC binding of a receiver based on received RID, there will be two receivers with the same SSRC binding unless there is some way to swap bindings of receivers) Testing: -------- Tested on Firefox, Firefox 78.15.0esr, Chrome, Ssafari and ensured that Simulcast sender and receiver are in sync for rid/ssrc/spatial layer resolution.
diff --git a/peerconnection_media_test.go b/peerconnection_media_test.go
@@ -1195,58 +1195,4 @@ func TestPeerConnection_Simulcast(t *testing.T) {
 		assertRidCorrect(t)
 		closePairNow(t, pcOffer, pcAnswer)
 	})
-
-	t.Run("SSRC Based", func(t *testing.T) {
-		pcOffer, pcAnswer, err := NewAPI(WithMediaEngine(m)).newPair(Configuration{})
-		assert.NoError(t, err)
-
-		vp8Writer, err := NewTrackLocalStaticRTP(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion2")
-		assert.NoError(t, err)
-
-		_, err = pcOffer.AddTrack(vp8Writer)
-		assert.NoError(t, err)
-
-		ridMap = map[string]int{}
-		pcAnswer.OnTrack(onTrackHandler)
-
-		assert.NoError(t, signalPairWithModification(pcOffer, pcAnswer, func(sessionDescription string) string {
-			sessionDescription = strings.Split(sessionDescription, "a=end-of-candidates\r\n")[0]
-			sessionDescription = filterSsrc(sessionDescription)
-
-			for _, rid := range rids {
-				sessionDescription += "a=" + sdpAttributeRid + ":" + rid + " send\r\n"
-			}
-			sessionDescription += "a=simulcast:send " + strings.Join(rids, ";") + "\r\n"
-
-			return sessionDescription + `a=ssrc:5000 cname:{49d59adc-fae6-407b-8850-2eb4a5e9b76e}
-a=ssrc:5001 cname:{49d59adc-fae6-407b-8850-2eb4a5e9b76e}
-a=ssrc:5002 cname:{49d59adc-fae6-407b-8850-2eb4a5e9b76e}
-a=ssrc:5003 cname:{49d59adc-fae6-407b-8850-2eb4a5e9b76e}
-a=ssrc:5004 cname:{49d59adc-fae6-407b-8850-2eb4a5e9b76e}
-a=ssrc:5005 cname:{49d59adc-fae6-407b-8850-2eb4a5e9b76e}
-a=ssrc-group:FID 5000 5001
-a=ssrc-group:FID 5002 5003
-a=ssrc-group:FID 5004 5005
-`
-		}))
-
-		for sequenceNumber := uint16(0); !ridsFullfilled(); sequenceNumber++ {
-			time.Sleep(20 * time.Millisecond)
-
-			for ssrc := 5000; ssrc <= 5004; ssrc += 2 {
-				header := &rtp.Header{
-					Version:        2,
-					SSRC:           uint32(ssrc),
-					SequenceNumber: sequenceNumber,
-					PayloadType:    96,
-				}
-
-				_, err := vp8Writer.bindings[0].writeStream.WriteRTP(header, []byte{0x00})
-				assert.NoError(t, err)
-			}
-		}
-
-		assertRidCorrect(t)
-		closePairNow(t, pcOffer, pcAnswer)
-	})
 }
diff --git a/sdp.go b/sdp.go
@@ -189,11 +189,6 @@ func trackDetailsFromSDP(log logging.LeveledLogger, s *sdp.SessionDescription) (
 			for rid := range rids {
 				simulcastTrack.rids = append(simulcastTrack.rids, rid)
 			}
-			if len(simulcastTrack.rids) == len(tracksInMediaSection) {
-				for i := range tracksInMediaSection {
-					simulcastTrack.ssrcs = append(simulcastTrack.ssrcs, tracksInMediaSection[i].ssrcs...)
-				}
-			}
 
 			tracksInMediaSection = []trackDetails{simulcastTrack}
 		}

Original file line number	Diff line number	Diff line change
`@@ -189,11 +189,6 @@ func trackDetailsFromSDP(log logging.LeveledLogger, s *sdp.SessionDescription) (`
`189`	`189`	`for rid := range rids {`
`190`	`190`	`simulcastTrack.rids = append(simulcastTrack.rids, rid)`
`191`	`191`	`}`
`192`		`- if len(simulcastTrack.rids) == len(tracksInMediaSection) {`
`193`		`- for i := range tracksInMediaSection {`
`194`		`- simulcastTrack.ssrcs = append(simulcastTrack.ssrcs, tracksInMediaSection[i].ssrcs...)`
`195`		`- }`
`196`		`- }`
`197`	`192`
`198`	`193`	`tracksInMediaSection = []trackDetails{simulcastTrack}`
`199`	`194`	`}`