Repository: pion/webrtc (go)

Issue:
Empty ID and StreamID when simulcasting
Browser is offerer, and offers a PC with a single sendonly transceiver with two simulcast tracks.  When Pion fires OnTrack, both track.StreamID() and track.ID() are empty.

This does not happen if either of the following is true:
  * there is an audio track in the same PC in addition to the simulcast transceiver;
  * the client is not sending a simulcast track.

The offer sent by the browser is correct as far as I can see (it contains a well-formed a=msid line in the simulcast m section).



Pull request description:
Don't consider simulcast for undeclared SSRC
If we have a media section with no SSRC we would fire an OnTrack. This
code now properly ignores a MediaSection that has a rid attribute.

Resolves #1808

Patch:
diff --git a/AUTHORS.txt b/AUTHORS.txt
index 7453e7d1..162d2d01 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -147,6 +147,7 @@ tarrencev <tarrence13@gmail.com>
 Thomas Miller <tmiv74@gmail.com>
 Tobias Fridén <tobias.friden@gmail.com>
 Tomek <tomek@pop-os.localdomain>
+Twometer <twometer@outlook.de>
 Vicken Simonian <vsimon@gmail.com>
 wattanakorn495 <wattanakorn.i@ku.th>
 Will Watson <william.a.watson@gmail.com>
diff --git a/constants.go b/constants.go
index 4a521bb9..f9d00682 100644
--- a/constants.go
+++ b/constants.go
@@ -22,9 +22,13 @@ const (
 
 	mediaSectionApplication = "application"
 
+	sdpAttributeRid = "rid"
+
 	rtpOutboundMTU = 1200
 
 	rtpPayloadTypeBitmask = 0x7F
+
+	incomingUnhandledRTPSsrc = "Incoming unhandled RTP ssrc(%d), OnTrack will not be fired. %v"
 )
 
 func defaultSrtpProtectionProfiles() []dtls.SRTPProtectionProfile {
diff --git a/peerconnection.go b/peerconnection.go
index 24619a1a..c9da9df2 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -1319,48 +1319,59 @@ func (pc *PeerConnection) startSCTP() {
 	pc.sctpTransport.lock.Unlock()
 }
 
-func (pc *PeerConnection) handleUndeclaredSSRC(rtpStream io.Reader, ssrc SSRC) error { //nolint:gocognit
-	remoteDescription := pc.RemoteDescription()
-	if remoteDescription == nil {
-		return errPeerConnRemoteDescriptionNil
+func (pc *PeerConnection) handleUndeclaredSSRC(ssrc SSRC, remoteDescription *SessionDescription) (handled bool, err error) {
+	if len(remoteDescription.parsed.MediaDescriptions) != 1 {
+		return false, nil
 	}
 
-	// If the remote SDP was only one media section the ssrc doesn't have to be explicitly declared
-	if len(remoteDescription.parsed.MediaDescriptions) == 1 {
-		onlyMediaSection := remoteDescription.parsed.MediaDescriptions[0]
-		streamID := ""
-		id := ""
-
-		for _, a := range onlyMediaSection.Attributes {
-			switch a.Key {
-			case sdp.AttrKeyMsid:
-				if split := strings.Split(a.Value, " "); len(split) == 2 {
-					streamID = split[0]
-					id = split[1]
-				}
-			case sdp.AttrKeySSRC:
-				return errPeerConnSingleMediaSectionHasExplicitSSRC
+	onlyMediaSection := remoteDescription.parsed.MediaDescriptions[0]
+	streamID := ""
+	id := ""
+
+	for _, a := range onlyMediaSection.Attributes {
+		switch a.Key {
+		case sdp.AttrKeyMsid:
+			if split := strings.Split(a.Value, " "); len(split) == 2 {
+				streamID = split[0]
+				id = split[1]
 			}
+		case sdp.AttrKeySSRC:
+			return false, errPeerConnSingleMediaSectionHasExplicitSSRC
+		case sdpAttributeRid:
+			return false, nil
 		}
+	}
 
-		incoming := trackDetails{
-			ssrc:     ssrc,
-			kind:     RTPCodecTypeVideo,
-			streamID: streamID,
-			id:       id,
-		}
-		if onlyMediaSection.MediaName.Media == RTPCodecTypeAudio.String() {
-			incoming.kind = RTPCodecTypeAudio
-		}
+	incoming := trackDetails{
+		ssrc:     ssrc,
+		kind:     RTPCodecTypeVideo,
+		streamID: streamID,
+		id:       id,
+	}
+	if onlyMediaSection.MediaName.Media == RTPCodecTypeAudio.String() {
+		incoming.kind = RTPCodecTypeAudio
+	}
 
-		t, err := pc.AddTransceiverFromKind(incoming.kind, RTPTransceiverInit{
-			Direction: RTPTransceiverDirectionSendrecv,
-		})
-		if err != nil {
-			return fmt.Errorf("%w: %d: %s", errPeerConnRemoteSSRCAddTransceiver, ssrc, err)
-		}
-		pc.startReceiver(incoming, t.Receiver())
-		return nil
+	t, err := pc.AddTransceiverFromKind(incoming.kind, RTPTransceiverInit{
+		Direction: RTPTransceiverDirectionSendrecv,
+	})
+	if err != nil {
+		return false, fmt.Errorf("%w: %d: %s", errPeerConnRemoteSSRCAddTransceiver, ssrc, err)
+	}
+
+	pc.startReceiver(incoming, t.Receiver())
+	return true, nil
+}
+
+func (pc *PeerConnection) handleIncomingSSRC(rtpStream io.Reader, ssrc SSRC) error { //nolint:gocognit
+	remoteDescription := pc.RemoteDescription()
+	if remoteDescription == nil {
+		return errPeerConnRemoteDescriptionNil
+	}
+
+	// If the remote SDP was only one media section the ssrc doesn't have to be explicitly declared
+	if handled, err := pc.handleUndeclaredSSRC(ssrc, remoteDescription); handled || err != nil {
+		return err
 	}
 
 	midExtensionID, audioSupported, videoSupported := pc.api.mediaEngine.getHeaderExtensionID(RTPHeaderExtensionCapability{sdp.SDESMidURI})
@@ -1453,8 +1464,8 @@ func (pc *PeerConnection) undeclaredMediaProcessor() {
 			go func(rtpStream io.Reader, ssrc SSRC) {
 				pc.dtlsTransport.storeSimulcastStream(stream)
 
-				if err := pc.handleUndeclaredSSRC(rtpStream, ssrc); err != nil {
-					pc.log.Errorf("Incoming unhandled RTP ssrc(%d), OnTrack will not be fired. %v", ssrc, err)
+				if err := pc.handleIncomingSSRC(rtpStream, ssrc); err != nil {
+					pc.log.Errorf(incomingUnhandledRTPSsrc, ssrc, err)
 				}
 				atomic.AddUint64(&simulcastRoutineCount, ^uint64(0))
 			}(stream, SSRC(ssrc))
diff --git a/rtpreceiver.go b/rtpreceiver.go
index 988cd175..85ab3ef9 100644
--- a/rtpreceiver.go
+++ b/rtpreceiver.go
@@ -15,7 +15,7 @@ import (
 )
 
 // trackStreams maintains a mapping of RTP/RTCP streams to a specific track
-// a RTPReceiver may contain multiple streams if we are dealing with Multicast
+// a RTPReceiver may contain multiple streams if we are dealing with Simulcast
 type trackStreams struct {
 	track *TrackRemote
 
diff --git a/sdp.go b/sdp.go
index aec31896..9eff9978 100644
--- a/sdp.go
+++ b/sdp.go
@@ -166,7 +166,7 @@ func trackDetailsFromSDP(log logging.LeveledLogger, s *sdp.SessionDescription) [
 func getRids(media *sdp.MediaDescription) map[string]string {
 	rids := map[string]string{}
 	for _, attr := range media.Attributes {
-		if attr.Key == "rid" {
+		if attr.Key == sdpAttributeRid {
 			split := strings.Split(attr.Value, " ")
 			rids[split[0]] = attr.Value
 		}
@@ -341,7 +341,7 @@ func addTransceiverSDP(d *sdp.SessionDescription, isPlanB, shouldAddCandidates b
 		recvRids := make([]string, 0, len(mediaSection.ridMap))
 
 		for rid := range mediaSection.ridMap {
-			media.WithValueAttribute("rid", rid+" recv")
+			media.WithValueAttribute(sdpAttributeRid, rid+" recv")
 			recvRids = append(recvRids, rid)
 		}
 		// Simulcast
@@ -653,7 +653,6 @@ func rtpExtensionsFromMediaDescription(m *sdp.MediaDescription) (map[string]int,
 // for subsequent calling, it updates Origin for SessionDescription from saved one
 // and increments session version by one.
 // https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-25#section-5.2.2
-// https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-25#section-5.3.2
 func updateSDPOrigin(origin *sdp.Origin, d *sdp.SessionDescription) {
 	if atomic.CompareAndSwapUint64(&origin.SessionVersion, 0, d.Origin.SessionVersion) { // store
 		atomic.StoreUint64(&origin.SessionID, d.Origin.SessionID)


Test patch:
diff --git a/peerconnection_media_test.go b/peerconnection_media_test.go
index b81474b0..a518013c 100644
--- a/peerconnection_media_test.go
+++ b/peerconnection_media_test.go
@@ -14,6 +14,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/pion/logging"
 	"github.com/pion/randutil"
 	"github.com/pion/rtcp"
 	"github.com/pion/rtp"
@@ -362,6 +363,29 @@ func TestPeerConnection_Media_Disconnected(t *testing.T) {
 	assert.NoError(t, pcOffer.Close())
 }
 
+type undeclaredSsrcLogger struct{ unhandledSimulcastError chan struct{} }
+
+func (u *undeclaredSsrcLogger) Trace(msg string)                          {}
+func (u *undeclaredSsrcLogger) Tracef(format string, args ...interface{}) {}
+func (u *undeclaredSsrcLogger) Debug(msg string)                          {}
+func (u *undeclaredSsrcLogger) Debugf(format string, args ...interface{}) {}
+func (u *undeclaredSsrcLogger) Info(msg string)                           {}
+func (u *undeclaredSsrcLogger) Infof(format string, args ...interface{})  {}
+func (u *undeclaredSsrcLogger) Warn(msg string)                           {}
+func (u *undeclaredSsrcLogger) Warnf(format string, args ...interface{})  {}
+func (u *undeclaredSsrcLogger) Error(msg string)                          {}
+func (u *undeclaredSsrcLogger) Errorf(format string, args ...interface{}) {
+	if format == incomingUnhandledRTPSsrc {
+		close(u.unhandledSimulcastError)
+	}
+}
+
+type undeclaredSsrcLoggerFactory struct{ unhandledSimulcastError chan struct{} }
+
+func (u *undeclaredSsrcLoggerFactory) NewLogger(subsystem string) logging.LeveledLogger {
+	return &undeclaredSsrcLogger{u.unhandledSimulcastError}
+}
+
 // If a SessionDescription has a single media section and no SSRC
 // assume that it is meant to handle all RTP packets
 func TestUndeclaredSSRC(t *testing.T) {
@@ -371,85 +395,100 @@ func TestUndeclaredSSRC(t *testing.T) {
 	report := test.CheckRoutines(t)
 	defer report()
 
-	pcOffer, pcAnswer, err := newPair()
-	assert.NoError(t, err)
+	// Filter SSRC lines
+	filterSsrc := func(offer *SessionDescription) (filteredSDP string) {
+		scanner := bufio.NewScanner(strings.NewReader(offer.SDP))
+		for scanner.Scan() {
+			l := scanner.Text()
+			if strings.HasPrefix(l, "a=ssrc") {
+				continue
+			}
 
-	_, err = pcAnswer.AddTransceiverFromKind(RTPCodecTypeVideo)
-	assert.NoError(t, err)
+			filteredSDP += l + "\n"
+		}
+		return
+	}
 
-	_, err = pcOffer.CreateDataChannel("test-channel", nil)
-	assert.NoError(t, err)
+	t.Run("No SSRC", func(t *testing.T) {
+		pcOffer, pcAnswer, err := newPair()
+		assert.NoError(t, err)
 
-	vp8Writer, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: "video/vp8"}, "video", "pion2")
-	assert.NoError(t, err)
+		vp8Writer, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: "video/vp8"}, "video", "pion2")
+		assert.NoError(t, err)
 
-	_, err = pcOffer.AddTrack(vp8Writer)
-	assert.NoError(t, err)
+		_, err = pcOffer.AddTrack(vp8Writer)
+		assert.NoError(t, err)
 
-	onTrackFired := make(chan *TrackRemote)
-	pcAnswer.OnTrack(func(trackRemote *TrackRemote, r *RTPReceiver) {
-		assert.Equal(t, trackRemote.StreamID(), vp8Writer.StreamID())
-		assert.Equal(t, trackRemote.ID(), vp8Writer.ID())
-		close(onTrackFired)
-	})
+		onTrackFired := make(chan struct{})
+		pcAnswer.OnTrack(func(trackRemote *TrackRemote, r *RTPReceiver) {
+			assert.Equal(t, trackRemote.StreamID(), vp8Writer.StreamID())
+			assert.Equal(t, trackRemote.ID(), vp8Writer.ID())
+			close(onTrackFired)
+		})
 
-	offer, err := pcOffer.CreateOffer(nil)
-	assert.NoError(t, err)
+		offer, err := pcOffer.CreateOffer(nil)
+		assert.NoError(t, err)
 
-	offerGatheringComplete := GatheringCompletePromise(pcOffer)
-	assert.NoError(t, pcOffer.SetLocalDescription(offer))
+		offerGatheringComplete := GatheringCompletePromise(pcOffer)
+		assert.NoError(t, pcOffer.SetLocalDescription(offer))
+		<-offerGatheringComplete
 
-	<-offerGatheringComplete
-	offer = *pcOffer.LocalDescription()
-
-	// Filter SSRC lines, and remove SCTP
-	filteredSDP := ""
-	scanner := bufio.NewScanner(strings.NewReader(offer.SDP))
-	inApplicationMedia := false
-	for scanner.Scan() {
-		l := scanner.Text()
-		if strings.HasPrefix(l, "m=application") {
-			inApplicationMedia = !inApplicationMedia
-		} else if strings.HasPrefix(l, "a=ssrc") {
-			continue
-		}
+		offer.SDP = filterSsrc(pcOffer.LocalDescription())
+		assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
 
-		if inApplicationMedia {
-			continue
-		}
+		answer, err := pcAnswer.CreateAnswer(nil)
+		assert.NoError(t, err)
 
-		filteredSDP += l + "\n"
-	}
+		answerGatheringComplete := GatheringCompletePromise(pcAnswer)
+		assert.NoError(t, pcAnswer.SetLocalDescription(answer))
+		<-answerGatheringComplete
 
-	offer.SDP = filteredSDP
+		assert.NoError(t, pcOffer.SetRemoteDescription(*pcAnswer.LocalDescription()))
 
-	assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
+		sendVideoUntilDone(onTrackFired, t, []*TrackLocalStaticSample{vp8Writer})
+		closePairNow(t, pcOffer, pcAnswer)
+	})
 
-	answer, err := pcAnswer.CreateAnswer(nil)
-	assert.NoError(t, err)
+	t.Run("Has RID", func(t *testing.T) {
+		unhandledSimulcastError := make(chan struct{})
 
-	answerGatheringComplete := GatheringCompletePromise(pcAnswer)
-	assert.NoError(t, pcAnswer.SetLocalDescription(answer))
-	<-answerGatheringComplete
+		m := &MediaEngine{}
+		assert.NoError(t, m.RegisterDefaultCodecs())
 
-	assert.NoError(t, pcOffer.SetRemoteDescription(*pcAnswer.LocalDescription()))
+		pcOffer, pcAnswer, err := NewAPI(WithSettingEngine(SettingEngine{
+			LoggerFactory: &undeclaredSsrcLoggerFactory{unhandledSimulcastError},
+		}), WithMediaEngine(m)).newPair(Configuration{})
+		assert.NoError(t, err)
 
-	go func() {
-		for {
-			assert.NoError(t, vp8Writer.WriteSample(media.Sample{Data: []byte{0x00}, Duration: time.Second}))
-			time.Sleep(time.Millisecond * 25)
+		vp8Writer, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: "video/vp8"}, "video", "pion2")
+		assert.NoError(t, err)
 
-			select {
-			case <-onTrackFired:
-				return
-			default:
-				continue
-			}
-		}
-	}()
+		_, err = pcOffer.AddTrack(vp8Writer)
+		assert.NoError(t, err)
 
-	<-onTrackFired
-	closePairNow(t, pcOffer, pcAnswer)
+		offer, err := pcOffer.CreateOffer(nil)
+		assert.NoError(t, err)
+
+		offerGatheringComplete := GatheringCompletePromise(pcOffer)
+		assert.NoError(t, pcOffer.SetLocalDescription(offer))
+		<-offerGatheringComplete
+
+		// Append RID to end of SessionDescription. Will not be considered unhandled anymore
+		offer.SDP = filterSsrc(pcOffer.LocalDescription()) + "a=" + sdpAttributeRid + "\r\n"
+		assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
+
+		answer, err := pcAnswer.CreateAnswer(nil)
+		assert.NoError(t, err)
+
+		answerGatheringComplete := GatheringCompletePromise(pcAnswer)
+		assert.NoError(t, pcAnswer.SetLocalDescription(answer))
+		<-answerGatheringComplete
+
+		assert.NoError(t, pcOffer.SetRemoteDescription(*pcAnswer.LocalDescription()))
+
+		sendVideoUntilDone(unhandledSimulcastError, t, []*TrackLocalStaticSample{vp8Writer})
+		closePairNow(t, pcOffer, pcAnswer)
+	})
 }
 
 func TestAddTransceiverFromTrackSendOnly(t *testing.T) {
diff --git a/sdp_test.go b/sdp_test.go
index 7a657bba..2cbeed43 100644
--- a/sdp_test.go
+++ b/sdp_test.go
@@ -204,7 +204,7 @@ func TestTrackDetailsFromSDP(t *testing.T) {
 					},
 					Attributes: []sdp.Attribute{
 						{Key: "sendonly"},
-						{Key: "rid", Value: "f send pt=97;max-width=1280;max-height=720"},
+						{Key: sdpAttributeRid, Value: "f send pt=97;max-width=1280;max-height=720"},
 					},
 				},
 			},
@@ -368,7 +368,7 @@ func TestMediaDescriptionFingerprints(t *testing.T) {
 }
 
 func TestPopulateSDP(t *testing.T) {
-	t.Run("Rid", func(t *testing.T) {
+	t.Run("rid", func(t *testing.T) {
 		se := SettingEngine{}
 
 		me := &MediaEngine{}
@@ -394,7 +394,7 @@ func TestPopulateSDP(t *testing.T) {
 				continue
 			}
 			for _, a := range desc.Attributes {
-				if a.Key == "rid" {
+				if a.Key == sdpAttributeRid {
 					if strings.Contains(a.Value, "ridkey") {
 						found = true
 						break
@@ -458,7 +458,7 @@ func TestGetRIDs(t *testing.T) {
 			},
 			Attributes: []sdp.Attribute{
 				{Key: "sendonly"},
-				{Key: "rid", Value: "f send pt=97;max-width=1280;max-height=720"},
+				{Key: sdpAttributeRid, Value: "f send pt=97;max-width=1280;max-height=720"},
 			},
 		},
 	}