Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't reuse transceiver in one round negotiation #2899

Merged
merged 1 commit into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions peerconnection.go
Original file line number Diff line number Diff line change
Expand Up @@ -1118,10 +1118,18 @@
case direction == RTPTransceiverDirectionRecvonly:
if t.Direction() == RTPTransceiverDirectionSendrecv {
t.setDirection(RTPTransceiverDirectionSendonly)
} else if t.Direction() == RTPTransceiverDirectionRecvonly {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set RecvOnly transceiver to Inactive if the remote peer stop sending

t.setDirection(RTPTransceiverDirectionInactive)
}
case direction == RTPTransceiverDirectionSendrecv:
if t.Direction() == RTPTransceiverDirectionSendonly {
t.setDirection(RTPTransceiverDirectionSendrecv)
} else if t.Direction() == RTPTransceiverDirectionInactive {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transit Inactive transceiver to RecvOnly if the remote peer reuse the inactive transceiver to send track

t.setDirection(RTPTransceiverDirectionRecvonly)
}
case direction == RTPTransceiverDirectionSendonly:
if t.Direction() == RTPTransceiverDirectionInactive {
t.setDirection(RTPTransceiverDirectionRecvonly)

Check warning on line 1132 in peerconnection.go

View check run for this annotation

Codecov / codecov/patch

peerconnection.go#L1132

Added line #L1132 was not covered by tests
}
}

Expand Down Expand Up @@ -1288,7 +1296,7 @@
// If a transceiver is created by applying a remote description that has recvonly transceiver,
// it will have no sender. In this case, the transceiver's current direction is set to inactive so
// that the transceiver can be reused by next AddTrack.
if direction == RTPTransceiverDirectionSendonly && t.Sender() == nil {
if !weOffer && direction == RTPTransceiverDirectionSendonly && t.Sender() == nil {
direction = RTPTransceiverDirectionInactive
}

Expand Down Expand Up @@ -1340,28 +1348,28 @@

mid := t.Mid()
receiverNeedsStopped := false
func() {
for _, t := range tracks {
for _, track := range tracks {
func(t *TrackRemote) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrow down the scope of the lock

t.mu.Lock()
defer t.mu.Unlock()

if t.rid != "" {
if details := trackDetailsForRID(incomingTracks, mid, t.rid); details != nil {
t.id = details.id
t.streamID = details.streamID
continue
return

Check warning on line 1360 in peerconnection.go

View check run for this annotation

Codecov / codecov/patch

peerconnection.go#L1360

Added line #L1360 was not covered by tests
}
} else if t.ssrc != 0 {
if details := trackDetailsForSSRC(incomingTracks, t.ssrc); details != nil {
t.id = details.id
t.streamID = details.streamID
continue
return
}
}

receiverNeedsStopped = true
}
}()
}(track)
}

if !receiverNeedsStopped {
continue
Expand Down
44 changes: 43 additions & 1 deletion peerconnection_renegotiation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1160,12 +1160,15 @@ func TestPeerConnection_Regegotiation_ReuseTransceiver(t *testing.T) {
t.Fatal(err)
}

vp8Track, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "foo", "bar")
vp8Track, err := NewTrackLocalStaticRTP(RTPCodecCapability{MimeType: MimeTypeVP8}, "foo", "bar")
assert.NoError(t, err)
sender, err := pcOffer.AddTrack(vp8Track)
assert.NoError(t, err)
assert.NoError(t, signalPair(pcOffer, pcAnswer))

peerConnectionConnected := untilConnectionState(PeerConnectionStateConnected, pcOffer, pcAnswer)
peerConnectionConnected.Wait()

assert.Equal(t, len(pcOffer.GetTransceivers()), 1)
assert.Equal(t, pcOffer.GetTransceivers()[0].getCurrentDirection(), RTPTransceiverDirectionSendonly)
assert.NoError(t, pcOffer.RemoveTrack(sender))
Expand All @@ -1185,6 +1188,45 @@ func TestPeerConnection_Regegotiation_ReuseTransceiver(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, len(pcOffer.GetTransceivers()), 2)
assert.True(t, sender.rtpTransceiver == pcOffer.GetTransceivers()[0])
assert.NoError(t, signalPair(pcOffer, pcAnswer))

tracksCh := make(chan *TrackRemote, 2)
pcAnswer.OnTrack(func(tr *TrackRemote, _ *RTPReceiver) {
tracksCh <- tr
})

ssrcReuse := sender.GetParameters().Encodings[0].SSRC
for i := 0; i < 10; i++ {
assert.NoError(t, vp8Track.WriteRTP(&rtp.Packet{Header: rtp.Header{Version: 2}, Payload: []byte{0, 1, 2, 3, 4, 5}}))
time.Sleep(20 * time.Millisecond)
}

// shold not reuse tranceiver between two CreateOffer
offer, err := pcOffer.CreateOffer(nil)
assert.NoError(t, err)
assert.NoError(t, pcOffer.RemoveTrack(sender))
assert.NoError(t, pcOffer.SetLocalDescription(offer))
assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
answer, err := pcAnswer.CreateAnswer(nil)
assert.NoError(t, pcAnswer.SetLocalDescription(answer))
assert.NoError(t, err)
assert.NoError(t, pcOffer.SetRemoteDescription(answer))
sender3, err := pcOffer.AddTrack(vp8Track)
ssrcNotReuse := sender3.GetParameters().Encodings[0].SSRC
assert.NoError(t, err)
assert.Equal(t, len(pcOffer.GetTransceivers()), 3)
assert.NoError(t, signalPair(pcOffer, pcAnswer))
assert.True(t, sender3.rtpTransceiver == pcOffer.GetTransceivers()[2])

for i := 0; i < 10; i++ {
assert.NoError(t, vp8Track.WriteRTP(&rtp.Packet{Header: rtp.Header{Version: 2}, Payload: []byte{0, 1, 2, 3, 4, 5}}))
time.Sleep(20 * time.Millisecond)
}

tr1 := <-tracksCh
tr2 := <-tracksCh
assert.Equal(t, tr1.SSRC(), ssrcReuse)
assert.Equal(t, tr2.SSRC(), ssrcNotReuse)

closePairNow(t, pcOffer, pcAnswer)
}
Expand Down
Loading