Repository: pion/webrtc
Pull request #2301 (2022-09-14 07:37:09)

Add currentDirection to RTPTransceiver
add currentDirection to RTPTransceiver, don't reuse transceiver if its currentDirection is sendrecv or sendonly

#### Description
If remove track from transceiver then reuse it to add track without re-negotiation to transit its direction of sdp, then safari will not fire `removetrack` and `ontrack` events for that transceiver, so the mediatrack is lost.
Step:
1. sfu offer with 2 tracks, negotiation with safari, all things work, safari fire 2 `onTrack` event
2. sfu call RemoveTrack then AddTrack, will reuse rtptransceiver, after that, re-negotiation with safari. No new mid/transceiver in safari, the reused trasceiver changed its stream attributes to the new track.
3. safari can't fire `onTrack` for the new track.

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 in sfu. So we only check currentDirection's current value, that's worked for all browsers.

> If any [RTCRtpSender](https://www.w3.org/TR/webrtc/#dom-rtcrtpsender) object in senders matches all the following criteria, let sender be that object, or null otherwise:
> 
> The sender's track is null.
> 
> The [transceiver kind](https://www.w3.org/TR/webrtc/#dfn-transceiver-kind) of the [RTCRtpTransceiver](https://www.w3.org/TR/webrtc/#dom-rtcrtptransceiver), associated with the sender, matches kind.
> 
> The [[[Stopping]]](https://www.w3.org/TR/webrtc/#dfn-stopping) slot of the [RTCRtpTransceiver](https://www.w3.org/TR/webrtc/#dom-rtcrtptransceiver) associated with the sender is false.
> 
> The sender has never been used to send. More precisely, the [[[CurrentDirection]]](https://www.w3.org/TR/webrtc/#dfn-currentdirection) slot of the [RTCRtpTransceiver](https://www.w3.org/TR/webrtc/#dom-rtcrtptransceiver) associated with the sender has never had a value of "[sendrecv](https://www.w3.org/TR/webrtc/#dom-rtcrtptransceiverdirection-sendrecv)" or "[sendonly](https://www.w3.org/TR/webrtc/#dom-rtcrtptransceiverdirection-sendonly)".

#### Reference issue
I think #1843 is related to this fix, but chrome works well on my side, only safari have issue, with this PR, safari works well

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/2301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
Base: **77.34**% // Head: **70.16**% // Decreases project coverage by **`-7.17%`** :warning:
> Coverage data is based on head [(`1ea82c1`)](https://codecov.io/gh/pion/webrtc/pull/2301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) compared to base [(`4e85358`)](https://codecov.io/gh/pion/webrtc/commit/4e853582ca30d90f2370750571a273a3413b187e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> Patch has no changes to coverable lines.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #2301      +/-   ##
==========================================
- Coverage   77.34%   70.16%   -7.18%     
==========================================
  Files          87       64      -23     
  Lines        9197     3355    -5842     
==========================================
- Hits         7113     2354    -4759     
+ Misses       1651      873     -778     
+ Partials      433      128     -305     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `70.16% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/2301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uc3RhbnRzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-82.76%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-81.82%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwY29kZWMuZ28=) | `13.33% <0.00%> (-80.00%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-66.67%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlbXV4Lmdv) | `0.00% <0.00%> (-54.55%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `13.79% <0.00%> (-44.83%)` | :arrow_down: |
| [atomicbool.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YXRvbWljYm9vbC5nbw==) | `57.14% <0.00%> (-42.86%)` | :arrow_down: |
| [dtlsrole.go](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-ZHRsc3JvbGUuZ28=) | `71.42% <0.00%> (-28.58%)` | :arrow_down: |
| ... and [37 more](https://codecov.io/gh/pion/webrtc/pull/2301/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/2301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/peerconnection.go b/peerconnection.go
index 4eeb7015..6f9e77d3 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -984,6 +984,7 @@ func (pc *PeerConnection) SetLocalDescription(desc SessionDescription) error {
 	weAnswer := desc.Type == SDPTypeAnswer
 	remoteDesc := pc.RemoteDescription()
 	if weAnswer && remoteDesc != nil {
+		_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, false)
 		if err := pc.startRTPSenders(currentTransceivers); err != nil {
 			return err
 		}
@@ -1143,6 +1144,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 
 	if isRenegotation {
 		if weOffer {
+			_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, true)
 			if err = pc.startRTPSenders(currentTransceivers); err != nil {
 				return err
 			}
@@ -1172,6 +1174,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 	// Start the networking in a new routine since it will block until
 	// the connection is actually established.
 	if weOffer {
+		_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, true)
 		if err := pc.startRTPSenders(currentTransceivers); err != nil {
 			return err
 		}
@@ -1230,6 +1233,51 @@ func (pc *PeerConnection) startReceiver(incoming trackDetails, receiver *RTPRece
 	}
 }
 
+func setRTPTransceiverCurrentDirection(answer *SessionDescription, currentTransceivers []*RTPTransceiver, weOffer bool) error {
+	currentTransceivers = append([]*RTPTransceiver{}, currentTransceivers...)
+	for _, media := range answer.parsed.MediaDescriptions {
+		midValue := getMidValue(media)
+		if midValue == "" {
+			return errPeerConnRemoteDescriptionWithoutMidValue
+		}
+
+		if media.MediaName.Media == mediaSectionApplication {
+			continue
+		}
+
+		var t *RTPTransceiver
+		t, currentTransceivers = findByMid(midValue, currentTransceivers)
+
+		if t == nil {
+			return fmt.Errorf("%w: %q", errPeerConnTranscieverMidNil, midValue)
+		}
+
+		direction := getPeerDirection(media)
+		if direction == RTPTransceiverDirection(Unknown) {
+			continue
+		}
+
+		// reverse direction if it was a remote answer
+		if weOffer {
+			switch direction {
+			case RTPTransceiverDirectionSendonly:
+				direction = RTPTransceiverDirectionRecvonly
+			case RTPTransceiverDirectionRecvonly:
+				// Pion will answer recvonly with a offer recvonly transceiver, so we should
+				// not change the direction to sendonly if we are the offerer, otherwise this
+				// tranceiver can't be reuse for AddTrack
+				if t.Direction() != RTPTransceiverDirectionRecvonly {
+					direction = RTPTransceiverDirectionSendonly
+				}
+			default:
+			}
+		}
+
+		t.setCurrentDirection(direction)
+	}
+	return nil
+}
+
 func runIfNewReceiver(
 	incomingTrack trackDetails,
 	transceivers []*RTPTransceiver,
@@ -1706,7 +1754,13 @@ func (pc *PeerConnection) AddTrack(track TrackLocal) (*RTPSender, error) {
 	pc.mu.Lock()
 	defer pc.mu.Unlock()
 	for _, t := range pc.rtpTransceivers {
-		if !t.stopped && t.kind == track.Kind() && t.Sender() == nil {
+		currentDirection := t.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 !t.stopped && t.kind == track.Kind() && t.Sender() == nil &&
+			!(currentDirection == RTPTransceiverDirectionSendrecv || currentDirection == RTPTransceiverDirectionSendonly) {
 			sender, err := pc.api.NewRTPSender(track, pc.dtlsTransport)
 			if err == nil {
 				err = t.SetSender(sender, track)
diff --git a/rtptransceiver.go b/rtptransceiver.go
index 776e0108..f94b9ed7 100644
--- a/rtptransceiver.go
+++ b/rtptransceiver.go
@@ -13,10 +13,11 @@ 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
+	mid              atomic.Value // string
+	sender           atomic.Value // *RTPSender
+	receiver         atomic.Value // *RTPReceiver
+	direction        atomic.Value // RTPTransceiverDirection
+	currentDirection atomic.Value // RTPTransceiverDirection
 
 	codecs []RTPCodecParameters // User provided codecs via SetCodecPreferences
 
@@ -38,6 +39,7 @@ func newRTPTransceiver(
 	t.setReceiver(receiver)
 	t.setSender(sender)
 	t.setDirection(direction)
+	t.setCurrentDirection(RTPTransceiverDirection(Unknown))
 	return t
 }
 
@@ -160,6 +162,7 @@ func (t *RTPTransceiver) Stop() error {
 	}
 
 	t.setDirection(RTPTransceiverDirectionInactive)
+	t.setCurrentDirection(RTPTransceiverDirectionInactive)
 	return nil
 }
 
@@ -179,6 +182,17 @@ func (t *RTPTransceiver) setDirection(d RTPTransceiverDirection) {
 	t.direction.Store(d)
 }
 
+func (t *RTPTransceiver) setCurrentDirection(d RTPTransceiverDirection) {
+	t.currentDirection.Store(d)
+}
+
+func (t *RTPTransceiver) getCurrentDirection() RTPTransceiverDirection {
+	if v, ok := t.currentDirection.Load().(RTPTransceiverDirection); ok {
+		return v
+	}
+	return RTPTransceiverDirection(Unknown)
+}
+
 func (t *RTPTransceiver) setSendingTrack(track TrackLocal) error {
 	if err := t.Sender().ReplaceTrack(track); err != nil {
 		return err


Test patch:
diff --git a/peerconnection_renegotiation_test.go b/peerconnection_renegotiation_test.go
index cc30fe1d..ef7e0e3a 100644
--- a/peerconnection_renegotiation_test.go
+++ b/peerconnection_renegotiation_test.go
@@ -128,14 +128,14 @@ func TestPeerConnection_Renegotiation_AddRecvonlyTransceiver(t *testing.T) {
 				pcOffer.OnTrack(func(track *TrackRemote, r *RTPReceiver) {
 					onTrackFiredFunc()
 				})
+				assert.NoError(t, signalPair(pcAnswer, pcOffer))
 			} else {
 				pcAnswer.OnTrack(func(track *TrackRemote, r *RTPReceiver) {
 					onTrackFiredFunc()
 				})
+				assert.NoError(t, signalPair(pcOffer, pcAnswer))
 			}
 
-			assert.NoError(t, signalPair(pcOffer, pcAnswer))
-
 			sendVideoUntilDone(onTrackFired.Done(), t, []*TrackLocalStaticSample{localTrack})
 
 			closePairNow(t, pcOffer, pcAnswer)
@@ -380,6 +380,7 @@ func TestPeerConnection_Transceiver_Mid(t *testing.T) {
 
 	offer, err = pcOffer.CreateOffer(nil)
 	assert.NoError(t, err)
+	assert.NoError(t, pcOffer.SetLocalDescription(offer))
 
 	assert.Equal(t, len(offer.parsed.MediaDescriptions), 2)
 
@@ -391,6 +392,11 @@ func TestPeerConnection_Transceiver_Mid(t *testing.T) {
 	pcOffer.ops.Done()
 	pcAnswer.ops.Done()
 
+	assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
+	answer, err = pcAnswer.CreateAnswer(nil)
+	assert.NoError(t, err)
+	assert.NoError(t, pcOffer.SetRemoteDescription(answer))
+
 	track3, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion3")
 	require.NoError(t, err)
 
@@ -468,12 +474,12 @@ func TestPeerConnection_Renegotiation_CodecChange(t *testing.T) {
 
 	require.NoError(t, pcOffer.RemoveTrack(sender1))
 
-	sender2, err := pcOffer.AddTrack(track2)
-	require.NoError(t, err)
-
 	require.NoError(t, signalPair(pcOffer, pcAnswer))
 	<-tracksClosed
 
+	sender2, err := pcOffer.AddTrack(track2)
+	require.NoError(t, err)
+	require.NoError(t, signalPair(pcOffer, pcAnswer))
 	transceivers = pcOffer.GetTransceivers()
 	require.Equal(t, 1, len(transceivers))
 	require.Equal(t, "0", transceivers[0].Mid())
@@ -1145,3 +1151,44 @@ func TestPeerConnection_Renegotiation_Simulcast(t *testing.T) {
 		closePairNow(t, pcOffer, pcAnswer)
 	})
 }
+
+func TestPeerConnection_Regegotiation_ReuseTransceiver(t *testing.T) {
+	lim := test.TimeOut(time.Second * 30)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	pcOffer, pcAnswer, err := newPair()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	vp8Track, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "foo", "bar")
+	assert.NoError(t, err)
+	sender, err := pcOffer.AddTrack(vp8Track)
+	assert.NoError(t, err)
+	assert.NoError(t, signalPair(pcOffer, pcAnswer))
+
+	assert.Equal(t, len(pcOffer.GetTransceivers()), 1)
+	assert.Equal(t, pcOffer.GetTransceivers()[0].getCurrentDirection(), RTPTransceiverDirectionSendonly)
+	assert.NoError(t, pcOffer.RemoveTrack(sender))
+	assert.Equal(t, pcOffer.GetTransceivers()[0].getCurrentDirection(), RTPTransceiverDirectionSendonly)
+
+	// should not reuse tranceiver
+	vp8Track2, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "foo", "bar")
+	assert.NoError(t, err)
+	sender2, err := pcOffer.AddTrack(vp8Track2)
+	assert.NoError(t, err)
+	assert.Equal(t, len(pcOffer.GetTransceivers()), 2)
+	assert.NoError(t, signalPair(pcOffer, pcAnswer))
+	assert.True(t, sender2.rtpTransceiver == pcOffer.GetTransceivers()[1])
+
+	// should reuse first transceiver
+	sender, err = pcOffer.AddTrack(vp8Track)
+	assert.NoError(t, err)
+	assert.Equal(t, len(pcOffer.GetTransceivers()), 2)
+	assert.True(t, sender.rtpTransceiver == pcOffer.GetTransceivers()[0])
+
+	closePairNow(t, pcOffer, pcAnswer)
+}