Consider remote direction in add track

A follow on to https://github.com/pion/webrtc/pull/3200.
This removes the setting engine flag and uses knowledge
of remote direction to decide if a transceiver can be
re-used for sending.

Refactored the code a bit and moved the check into
RTPTransceiver.isSendAllowed.

Re-did the UT to check for re-use cases.

peerconnection.go

@@ -1137,6 +1137,9 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 					return err
 				}
 			}
+			if transceiver != nil {
+				transceiver.setCurrentRemoteDirection(direction)
+			}
 
 			switch {
 			case transceiver == nil:
@@ -1153,6 +1156,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 				}
 
 				transceiver = newRTPTransceiver(receiver, nil, localDirection, kind, pc.api)
+				transceiver.setCurrentRemoteDirection(direction)
 				pc.mu.Lock()
 				pc.addRTPTransceiver(transceiver)
 				pc.mu.Unlock()
@@ -2099,29 +2103,24 @@ func (pc *PeerConnection) AddTrack(track TrackLocal) (*RTPSender, error) {
 	pc.mu.Lock()
 	defer pc.mu.Unlock()
 	for _, transceiver := range pc.rtpTransceivers {
-		currentDirection := transceiver.getCurrentDirection()
-		// According to https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-addtrack, if the
-		// transceiver can be reused only if it's currentDirection never be sendrecv or sendonly.
-		// But that will cause sdp inflate. So we only check currentDirection's current value,
-		// that's worked for all browsers.
-		if transceiver.kind == track.Kind() && transceiver.Sender() == nil &&
-			currentDirection != RTPTransceiverDirectionSendrecv && currentDirection != RTPTransceiverDirectionSendonly &&
-			(!pc.api.settingEngine.disableTransceiverReuseInRecvonly || currentDirection != RTPTransceiverDirectionRecvonly) {
-			sender, err := pc.api.NewRTPSender(track, pc.dtlsTransport)
-			if err == nil {
-				err = transceiver.SetSender(sender, track)
-				if err != nil {
-					_ = sender.Stop()
-					transceiver.setSender(nil)
-				}
-			}
-			if err != nil {
-				return nil, err
-			}
-			pc.onNegotiationNeeded()
-
-			return sender, nil
+		if !transceiver.isSendAllowed(track.Kind()) {
+			continue
 		}
+
+		sender, err := pc.api.NewRTPSender(track, pc.dtlsTransport)
+		if err == nil {
+			err = transceiver.SetSender(sender, track)
+			if err != nil {
+				_ = sender.Stop()
+				transceiver.setSender(nil)
+			}
+		}
+		if err != nil {
+			return nil, err
+		}
+		pc.onNegotiationNeeded()
+
+		return sender, nil
 	}
 
 	transceiver, err := pc.newTransceiverFromTrack(RTPTransceiverDirectionSendrecv, track)

peerconnection_media_test.go

@@ -745,56 +745,141 @@ func TestAddTransceiverAddTrack_Reuse(t *testing.T) {
 		assert.NoError(t, pc.Close())
 	})
 
-	t.Run("reuse disable test", func(t *testing.T) {
-		se := SettingEngine{}
-		se.SetDisableTransceiverReuseInRecvonly(true)
-		pc, err := NewAPI(WithSettingEngine(se)).NewPeerConnection(Configuration{})
-		assert.NoError(t, err)
-
-		tr, err := pc.AddTransceiverFromKind(
-			RTPCodecTypeVideo,
-			RTPTransceiverInit{Direction: RTPTransceiverDirectionRecvonly},
-		)
-		assert.NoError(t, err)
-
-		assert.Equal(t, []*RTPTransceiver{tr}, pc.GetTransceivers())
-
-		addTrack := func() (TrackLocal, *RTPSender) {
-			track, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "foo", "bar")
-			assert.NoError(t, err)
-
-			sender, err := pc.AddTrack(track)
-			assert.NoError(t, err)
-
-			return track, sender
+	t.Run("reuse remote direction test", func(t *testing.T) {
+		testCases := []struct {
+			remoteDirection         RTPTransceiverDirection
+			remoteDirectionNoSender RTPTransceiverDirection // direction should switch to this on track removal
+			isSendAllowed           bool
+		}{
+			{
+				remoteDirection:         RTPTransceiverDirectionSendrecv,
+				remoteDirectionNoSender: RTPTransceiverDirectionRecvonly,
+				isSendAllowed:           true,
+			},
+			{
+				remoteDirection:         RTPTransceiverDirectionSendonly,
+				remoteDirectionNoSender: RTPTransceiverDirectionInactive,
+				isSendAllowed:           false,
+			},
 		}
 
-		// force direction to `recvonly` and ensure SettingEngine setting disables re-use
-		tr.setCurrentDirection(RTPTransceiverDirectionRecvonly)
-		addTrack()
-		assert.Equal(t, 2, len(pc.GetTransceivers()))
+		for _, testCase := range testCases {
+			t.Run(testCase.remoteDirection.String(), func(t *testing.T) {
+				pcOffer, pcAnswer, err := newPair()
+				require.NoError(t, err)
+				assert.NoError(t, err)
 
-		// the newly added transceiver above will have a sender, so not re-usable
-		_, sender := addTrack()
-		assert.Equal(t, 3, len(pc.GetTransceivers()))
+				remoteTrack, err := NewTrackLocalStaticSample(
+					RTPCodecCapability{
+						MimeType:    MimeTypeH264,
+						SDPFmtpLine: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f",
+					},
+					"track-id",
+					"track-label",
+				)
+				assert.NoError(t, err)
 
-		// remove last added track to make that transceiver re-usable
-		require.NoError(t, pc.RemoveTrack(sender))
+				remoteTransceiver, err := pcOffer.AddTransceiverFromTrack(remoteTrack, RTPTransceiverInit{
+					Direction: testCase.remoteDirection,
+				})
+				assert.NoError(t, err)
 
-		track, sender := addTrack()
-		assert.Equal(t, 3, len(pc.GetTransceivers()))
-		var matchedTransceiver *RTPTransceiver
-		for _, tr := range pc.GetTransceivers() {
-			if tr.Sender() == sender {
-				matchedTransceiver = tr
+				var remoteSender *RTPSender
+				for _, tr := range pcOffer.GetTransceivers() {
+					if tr == remoteTransceiver {
+						remoteSender = tr.Sender()
 
-				break
-			}
+						break
+					}
+				}
+
+				addTrack := func() (TrackLocal, *RTPSender) {
+					track, err1 := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "foo", "bar")
+					assert.NoError(t, err1)
+
+					sender, err1 := pcAnswer.AddTrack(track)
+					assert.NoError(t, err1)
+
+					return track, sender
+				}
+
+				offer, err := pcOffer.CreateOffer(nil)
+				assert.NoError(t, err)
+				assert.NoError(t, pcOffer.SetLocalDescription(offer))
+
+				assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
+				// should have created local transceiver from remote description
+				localTransceivers := pcAnswer.GetTransceivers()
+				assert.Equal(t, 1, len(localTransceivers))
+				assert.Equal(t, testCase.remoteDirection, localTransceivers[0].getCurrentRemoteDirection())
+
+				localTrack, localSender := addTrack()
+				localTransceivers = pcAnswer.GetTransceivers()
+				if testCase.isSendAllowed {
+					assert.Equal(t, 1, len(localTransceivers))
+					assert.Equal(t, localSender, localTransceivers[0].Sender())
+					assert.Equal(t, localTrack, localTransceivers[0].Sender().Track())
+				} else {
+					assert.Equal(t, 2, len(localTransceivers))
+					assert.Equal(t, localSender, localTransceivers[1].Sender())
+					assert.Equal(t, localTrack, localTransceivers[1].Sender().Track())
+				}
+
+				answer, err := pcAnswer.CreateAnswer(nil)
+				assert.NoError(t, err)
+				assert.NoError(t, pcAnswer.SetLocalDescription(answer))
+				assert.NoError(t, pcOffer.SetRemoteDescription(answer))
+
+				// even if send was not allowed and answering side created a new transcever,
+				// it would not have been added to answer because there was no media section
+				// to assign it to, so the offer side should still see only one transceiver.
+				assert.Equal(t, 1, len(pcOffer.GetTransceivers()))
+
+				// remove local track and do a negotiation to clear sender from answer
+				require.NoError(t, pcAnswer.RemoveTrack(localSender))
+
+				offer, err = pcOffer.CreateOffer(nil)
+				assert.NoError(t, err)
+				assert.NoError(t, pcOffer.SetLocalDescription(offer))
+				assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
+				assert.Equal(t, testCase.remoteDirection, localTransceivers[0].getCurrentRemoteDirection())
+
+				answer, err = pcAnswer.CreateAnswer(nil)
+				assert.NoError(t, err)
+				assert.NoError(t, pcAnswer.SetLocalDescription(answer))
+				assert.NoError(t, pcOffer.SetRemoteDescription(answer))
+
+				// remove remote track from offer to change current remote direction
+				require.NoError(t, pcOffer.RemoveTrack(remoteSender))
+
+				offer, err = pcOffer.CreateOffer(nil)
+				assert.NoError(t, err)
+				assert.NoError(t, pcOffer.SetLocalDescription(offer))
+				assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
+				assert.Equal(t, testCase.remoteDirectionNoSender, localTransceivers[0].getCurrentRemoteDirection())
+
+				// try adding again
+				localTrack, localSender = addTrack()
+				localTransceivers = pcAnswer.GetTransceivers()
+				if testCase.isSendAllowed {
+					assert.Equal(t, 1, len(localTransceivers))
+					assert.Equal(t, localSender, localTransceivers[0].Sender())
+					assert.Equal(t, localTrack, localTransceivers[0].Sender().Track())
+				} else {
+					// the unmatched local transceiver should be re-usable
+					assert.Equal(t, 2, len(localTransceivers))
+					assert.Equal(t, localSender, localTransceivers[1].Sender())
+					assert.Equal(t, localTrack, localTransceivers[1].Sender().Track())
+				}
+
+				answer, err = pcAnswer.CreateAnswer(nil)
+				assert.NoError(t, err)
+				assert.NoError(t, pcAnswer.SetLocalDescription(answer))
+				assert.NoError(t, pcOffer.SetRemoteDescription(answer))
+
+				closePairNow(t, pcOffer, pcAnswer)
+			})
 		}
-		assert.NotNil(t, matchedTransceiver)
-		assert.Equal(t, track, matchedTransceiver.Sender().Track())
-
-		assert.NoError(t, pc.Close())
 	})
 }
 

rtptransceiver.go

@@ -17,11 +17,12 @@ import (
 
 // RTPTransceiver represents a combination of an RTPSender and an RTPReceiver that share a common mid.
 type RTPTransceiver struct {
-	mid              atomic.Value // string
-	sender           atomic.Value // *RTPSender
-	receiver         atomic.Value // *RTPReceiver
-	direction        atomic.Value // RTPTransceiverDirection
-	currentDirection atomic.Value // RTPTransceiverDirection
+	mid                    atomic.Value // string
+	sender                 atomic.Value // *RTPSender
+	receiver               atomic.Value // *RTPReceiver
+	direction              atomic.Value // RTPTransceiverDirection
+	currentDirection       atomic.Value // RTPTransceiverDirection
+	currentRemoteDirection atomic.Value // RTPTransceiverDirection
 
 	codecs []RTPCodecParameters // User provided codecs via SetCodecPreferences
 
@@ -220,6 +221,18 @@ func (t *RTPTransceiver) getCurrentDirection() RTPTransceiverDirection {
 	return RTPTransceiverDirectionUnknown
 }
 
+func (t *RTPTransceiver) setCurrentRemoteDirection(d RTPTransceiverDirection) {
+	t.currentRemoteDirection.Store(d)
+}
+
+func (t *RTPTransceiver) getCurrentRemoteDirection() RTPTransceiverDirection {
+	if v, ok := t.currentRemoteDirection.Load().(RTPTransceiverDirection); ok {
+		return v
+	}
+
+	return RTPTransceiverDirectionUnknown
+}
+
 func (t *RTPTransceiver) setSendingTrack(track TrackLocal) error { //nolint:cyclop
 	if err := t.Sender().ReplaceTrack(track); err != nil {
 		return err
@@ -250,6 +263,37 @@ func (t *RTPTransceiver) setSendingTrack(track TrackLocal) error { //nolint:cycl
 	return nil
 }
 
+func (t *RTPTransceiver) isSendAllowed(kind RTPCodecType) bool {
+	if t.kind != kind || t.Sender() != nil {
+		return false
+	}
+
+	// According to https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-addtrack, if the
+	// transceiver can be reused only if its currentDirection was never sendrecv or sendonly.
+	// But that will cause sdp to inflate. So we only check currentDirection's current value,
+	// that's worked for all browsers.
+	currentDirection := t.getCurrentDirection()
+	if currentDirection == RTPTransceiverDirectionSendrecv ||
+		currentDirection == RTPTransceiverDirectionSendonly {
+		return false
+	}
+
+	// `currentRemoteDirection` should be checked before using the transceiver for send.
+	// Remote directions could be
+	//   - `sendrecv` or `recvonly` - can send, remote direction will transition from
+	//     `sendrecv` -> `recvonly` if a remote track was removed.
+	//   - `sendonly` or `inactive` - cannot send, remote direction will transitions from
+	//     `sendonly` -> `inactive` if a remote track was removed.
+	//   - `unknown` - can send - we are the offering side and remote direction is unknown
+	currentRemoteDirection := t.getCurrentRemoteDirection()
+	if currentRemoteDirection == RTPTransceiverDirectionSendonly ||
+		currentRemoteDirection == RTPTransceiverDirectionInactive {
+		return false
+	}
+
+	return true
+}
+
 func findByMid(mid string, localTransceivers []*RTPTransceiver) (*RTPTransceiver, []*RTPTransceiver) {
 	for i, t := range localTransceivers {
 		if t.Mid() == mid {

settingengine.go

@@ -110,7 +110,6 @@ type SettingEngine struct {
 	disableCloseByDTLS                        bool
 	dataChannelBlockWrite                     bool
 	handleUndeclaredSSRCWithoutAnswer         bool
-	disableTransceiverReuseInRecvonly         bool
 }
 
 func (e *SettingEngine) getSCTPMaxMessageSize() uint32 {
@@ -578,23 +577,3 @@ func (e *SettingEngine) DisableCloseByDTLS(isEnabled bool) {
 func (e *SettingEngine) SetHandleUndeclaredSSRCWithoutAnswer(handleUndeclaredSSRCWithoutAnswer bool) {
 	e.handleUndeclaredSSRCWithoutAnswer = handleUndeclaredSSRCWithoutAnswer
 }
-
-// SetDisableTransceiverReuseInRecvonly controls if a transceiver is re-used
-// when its current direction is `recvonly`.
-//
-// This is useful for the following scenario
-//   - Remote side sends `offer` with `sendonly` media section.
-//   - Local side creates transceiver in `SetRemoteDescription` and sets direction to `recvonly`.
-//   - Local side calls `AddTrack`.
-//   - As the current direction is `recvonly`, the transceiver added above will be re-used.
-//     That will set the direction to `sendrecv` and the generated `answer` will have `sendrecv`
-//     for that media section.
-//   - That answer becomes incompatible as the offerer is using `sendonly`.
-//
-// Note that local transceiver will be in `recvonly` for both `sendrecv` and `sendonly` directions
-// in the media section. If the `offer` did use `sendrecv`, it is possible to re-use that transceiver
-// for sending. So, disabling re-use will prohibit re-use in the `sendrecv` case also and hence is
-// slightly wasteful.
-func (e *SettingEngine) SetDisableTransceiverReuseInRecvonly(disableTransceiverReuseInRecvonly bool) {
-	e.disableTransceiverReuseInRecvonly = disableTransceiverReuseInRecvonly
-}