webrtc
webrtc copied to clipboard
pion
Improper transceiver reuse in peerConnection.AddTrack
Your environment.
- Version: github.com/pion/webrtc/v3 v3.1.44
- Browser: Firefox 106.0.1 (but any Firefox really)
- Other Information - CollectSenders
What did you do?
-
A user1 has published a track - say audio - to the SFU by sending an offer with a sendonly track to the SFU. The SFU sets up a peerConnection with a recvonly transceiver to receive this track. All is working fine. Track and RTP is received on SFU
-
User1 gets a notification that someone else - say user2 - has published a track. User1 therefore adds a recvonly transceiver to its browser peerconnection, sends a new offer which now has its previous sendonly track and its new recvonly track info.
-
The SFU adds a sendonly transceiver to user1's SFU peerConnection (which only had a recvonly transceiver before this), adds the localtrack of user2, adds the new offer from the client and creates the answer.
-
Instead of, in AddTrack, adding an rtpSender to the newly added free sendonly transceiver, the AddTrack will find the recvonly transceiver (the published stream) and change its direction to sendrecv instead. This results in that the answer sent back will now specify sendrecv on the track where the offerer specified sendonly. This is, strictly speaking, not allowed and Firefox will, when you add the remote description (SetRemoteDescription) throw an exception: "Unhandled Rejection (InvalidAccessError): Answer tried to set send when offer did not set recv". Other browsers, like Chrome, is less formal regarding this and will not care.
What did you expect?
The expected result is that the new, free sendonly trasceiver was used when adding the localtrack rather than modifying the existing recvonly transceiver. This would have also created a proper answer with one sendonly track and one recvonly track.
Also, the code in AddTrack is broken in another way also: If you want to add a track and before add a sendonly transceiver in order to send this track on, this sendonly transceiver will actually NEVER be picked up by the loop in AddTrack. Instead it will always fall back to the base case on line 1786 https://github.com/pion/webrtc/blob/9c47fea3f2514dc6021323e66e628a3b99f6911e/peerconnection.go#L1786 where it will always add a new sendrecv transceiver. So the call to add a sendonly trasceiver is useless as that transceiver will never be used. It will however show up in the answer and cause the SDP to grow over time when you add more and more tracks to the peer connection.
What happened?
https://github.com/pion/webrtc/blob/9c47fea3f2514dc6021323e66e628a3b99f6911e/peerconnection.go#L1764-L1767 refers to https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-addtrack for why it looks the way it does, but the code does not really implement it that way. I believe that the main difference here is that the code will loop through all available transceivers and will therefore find this recvonly transceiver which also has a nil sender (obviously), but the specification on https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-addtrack will actually first do CollectSenders and then find the associated transceiver with the senders, so in the specification's sender reuse it would never even consider this recvonly transceiver as it is not associated with any sender (its sender is nil)
The snippet you quoted seems to have been added in commit 045df4c4bfd47ae0d657af98053f18e9a707ac34 from #2301. I've also noticed an issue with it Peer Calls as the tests started failing after upgrading pion/webrtc to v3.1.44. Reverting the commit fixes the issue on tip of master (66e8dfc9d824ceb80fdd75ac874d750f53747676).
@jeremija Can you post a test case of your failed case? We can check the detail of that and how to make it pass.
@cnderrauber - sure, have a look at my branch jeremija/answerer-adds-track. There are two commits on top of master:
- f8eb7dc79cce2622d1681103c269c9f233c753e3 which adds a failing test
- 1822ba3e1e40ceae65f58e8b0896c8aadceb9151 which rolls back your commit and the tests pass
You'll notice that my test case adds the track to answerer, not the offerer. This is by design because it is the only way to ensure compatibility with different browsers - I want only the server powered by pion/webrtc to generate the offers, not the browsers, because browsers sometimes have slight inconsistencies between PayloadTypes that are defined in pion/webrtc. In practice this works by adding the tracks to the peer from the browser and sending a websocket message to server to start the renegotiation from the server-side peer. After a lot of troubleshooting, experimentation, and a handful of pion patches, I was able to get much better results this way. Unfortunately this seems to have stopped working with the commit I mentioned above.
See the below log for more details:
$ git checkout f8eb7dc79cce2622d1681103c269c9f233c753e3
Note: switching to 'f8eb7dc79cce2622d1681103c269c9f233c753e3'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:
git switch -c <new-branch-name>
Or undo this operation with:
git switch -
Turn off this advice by setting config variable advice.detachedHead to false
HEAD is now at f8eb7dc Add test where answerer adds track
$ go test ./ -run AnswerAddsTrack -v
=== RUN TestPeerConnection_Regegotiation_AnswerAddsTrack
goroutine profile: total 60
8 @ 0x43c576 0x434e17 0x468049 0x4a12f2 0x4a2fa5 0x4a2f92 0x664a69 0x6849f8 0x682af1 0x682cb0 0x7e2973 0x46db21
# 0x468048 internal/poll.runtime_pollWait+0x88 /usr/lib/go/src/runtime/netpoll.go:305
# 0x4a12f1 internal/poll.(*pollDesc).wait+0x31 /usr/lib/go/src/internal/poll/fd_poll_runtime.go:84
# 0x4a2fa4 internal/poll.(*pollDesc).waitRead+0x1e4 /usr/lib/go/src/internal/poll/fd_poll_runtime.go:89
# 0x4a2f91 internal/poll.(*FD).ReadFromInet4+0x1d1 /usr/lib/go/src/internal/poll/fd_unix.go:250
# 0x664a68 net.(*netFD).readFromInet4+0x28 /usr/lib/go/src/net/fd_posix.go:66
# 0x6849f7 net.(*UDPConn).readFrom+0x1b7 /usr/lib/go/src/net/udpsock_posix.go:52
# 0x682af0 net.(*UDPConn).readFromUDP+0x30 /usr/lib/go/src/net/udpsock.go:149
# 0x682caf net.(*UDPConn).ReadFrom+0x4f /usr/lib/go/src/net/udpsock.go:158
# 0x7e2972 github.com/pion/ice/v2.(*candidateBase).recvLoop+0x152 /src/go/pkg/mod/github.com/pion/ice/[email protected]/candidate_base.go:222
6 @ 0x43c576 0x469c0c 0x469bec 0x475d2c 0x773bc5 0x77a5ba 0x874357 0x46db21
# 0x469beb sync.runtime_notifyListWait+0x12b /usr/lib/go/src/runtime/sema.go:517
# 0x475d2b sync.(*Cond).Wait+0x8b /usr/lib/go/src/sync/cond.go:70
# 0x773bc4 github.com/pion/sctp.(*Stream).ReadSCTP+0xe4 /src/go/pkg/mod/github.com/pion/[email protected]/stream.go:144
# 0x77a5b9 github.com/pion/datachannel.(*DataChannel).ReadDataChannel+0x59 /src/go/pkg/mod/github.com/pion/[email protected]/datachannel.go:193
# 0x874356 github.com/pion/webrtc/v3.(*DataChannel).readLoop+0xb6 /src/pion/webrtc/datachannel.go:325
4 @ 0x43c576 0x44c41c 0x7168c7 0x7ff905 0x78e57a 0x46db21
# 0x7168c6 github.com/pion/transport/packetio.(*Buffer).Read+0x166 /src/go/pkg/mod/github.com/pion/[email protected]/packetio/buffer.go:266
# 0x7ff904 github.com/pion/webrtc/v3/internal/mux.(*Endpoint).Read+0x24 /src/pion/webrtc/internal/mux/endpoint.go:37
# 0x78e579 github.com/pion/srtp/v2.(*session).start.func1+0xb9 /src/go/pkg/mod/github.com/pion/srtp/[email protected]/session.go:134
2 @ 0x43c576 0x4089db 0x408518 0x77a025 0x77a008 0x8a8f90 0x46db21
# 0x77a024 github.com/pion/sctp.(*Association).AcceptStream+0x44 /src/go/pkg/mod/github.com/pion/[email protected]/association.go:1359
# 0x77a007 github.com/pion/datachannel.Accept+0x27 /src/go/pkg/mod/github.com/pion/[email protected]/datachannel.go:123
# 0x8a8f8f github.com/pion/webrtc/v3.(*SCTPTransport).acceptDataChannels+0x28f /src/pion/webrtc/sctptransport.go:175
2 @ 0x43c576 0x4089db 0x408518 0x78ee6c 0x896945 0x46db21
# 0x78ee6b github.com/pion/srtp/v2.(*SessionSRTCP).AcceptStream+0x2b /src/go/pkg/mod/github.com/pion/srtp/[email protected]/session_srtcp.go:92
# 0x896944 github.com/pion/webrtc/v3.(*PeerConnection).undeclaredRTCPMediaProcessor+0x124 /src/pion/webrtc/peerconnection.go:1668
2 @ 0x43c576 0x4089db 0x408518 0x78fe4c 0x896298 0x46db21
# 0x78fe4b github.com/pion/srtp/v2.(*SessionSRTP).AcceptStream+0x2b /src/go/pkg/mod/github.com/pion/srtp/[email protected]/session_srtp.go:94
# 0x896297 github.com/pion/webrtc/v3.(*PeerConnection).undeclaredRTPMediaProcessor+0xd7 /src/pion/webrtc/peerconnection.go:1624
2 @ 0x43c576 0x4089db 0x408518 0x7d9fe8 0x46db21
# 0x7d9fe7 github.com/pion/ice/v2.(*Agent).startOnConnectionStateChangeRoutine.func1+0x47 /src/go/pkg/mod/github.com/pion/ice/[email protected]/agent.go:419
2 @ 0x43c576 0x434e17 0x468049 0x4a12f2 0x4a7ac5 0x4a7aa9 0x67c6c5 0x7a24f9 0x7ac9a8 0x7ac996 0x7b1687 0x46db21
# 0x468048 internal/poll.runtime_pollWait+0x88 /usr/lib/go/src/runtime/netpoll.go:305
# 0x4a12f1 internal/poll.(*pollDesc).wait+0x31 /usr/lib/go/src/internal/poll/fd_poll_runtime.go:84
# 0x4a7ac4 internal/poll.(*pollDesc).waitRead+0x144 /usr/lib/go/src/internal/poll/fd_poll_runtime.go:89
# 0x4a7aa8 internal/poll.(*FD).RawRead+0x128 /usr/lib/go/src/internal/poll/fd_unix.go:766
# 0x67c6c4 net.(*rawConn).Read+0x44 /usr/lib/go/src/net/rawconn.go:43
# 0x7a24f8 golang.org/x/net/internal/socket.(*Conn).recvMsg+0x178 /src/go/pkg/mod/golang.org/x/[email protected]/internal/socket/rawconn_msg.go:28
# 0x7ac9a7 golang.org/x/net/internal/socket.(*Conn).RecvMsg+0x527 /src/go/pkg/mod/golang.org/x/[email protected]/internal/socket/socket.go:247
# 0x7ac995 golang.org/x/net/ipv4.(*payloadHandler).ReadFrom+0x515 /src/go/pkg/mod/golang.org/x/[email protected]/ipv4/payload_cmsg.go:32
# 0x7b1686 github.com/pion/mdns.(*Conn).start+0x106 /src/go/pkg/mod/github.com/pion/[email protected]/conn.go:258
2 @ 0x43c576 0x44c41c 0x4fb31e 0x46db21
# 0x4fb31d context.propagateCancel.func1+0x9d /usr/lib/go/src/context/context.go:279
2 @ 0x43c576 0x44c41c 0x714489 0x46db21
# 0x714488 github.com/pion/transport/connctx.(*connCtx).ReadContext.func1+0xc8 /src/go/pkg/mod/github.com/pion/[email protected]/connctx/connctx.go:78
2 @ 0x43c576 0x44c41c 0x7168c7 0x7f82d3 0x80030b 0x46db21
# 0x7168c6 github.com/pion/transport/packetio.(*Buffer).Read+0x166 /src/go/pkg/mod/github.com/pion/[email protected]/packetio/buffer.go:266
# 0x7f82d2 github.com/pion/ice/v2.(*Conn).Read+0x72 /src/go/pkg/mod/github.com/pion/ice/[email protected]/transport.go:73
# 0x80030a github.com/pion/webrtc/v3/internal/mux.(*Mux).readLoop+0xca /src/pion/webrtc/internal/mux/mux.go:107
2 @ 0x43c576 0x44c41c 0x7168c7 0x7ff905 0x7141ce 0x73ce2e 0x73f705 0x46db21
# 0x7168c6 github.com/pion/transport/packetio.(*Buffer).Read+0x166 /src/go/pkg/mod/github.com/pion/[email protected]/packetio/buffer.go:266
# 0x7ff904 github.com/pion/webrtc/v3/internal/mux.(*Endpoint).Read+0x24 /src/pion/webrtc/internal/mux/endpoint.go:37
# 0x7141cd github.com/pion/transport/connctx.(*connCtx).ReadContext+0x22d /src/go/pkg/mod/github.com/pion/[email protected]/connctx/connctx.go:93
# 0x73ce2d github.com/pion/dtls/v2.(*Conn).readAndBuffer+0x10d /src/go/pkg/mod/github.com/pion/dtls/[email protected]/conn.go:570
# 0x73f704 github.com/pion/dtls/v2.(*Conn).handshake.func3+0x104 /src/go/pkg/mod/github.com/pion/dtls/[email protected]/conn.go:852
2 @ 0x43c576 0x44c41c 0x73ada5 0x75705e 0x46db21
# 0x73ada4 github.com/pion/dtls/v2.(*Conn).Read+0x164 /src/go/pkg/mod/github.com/pion/dtls/[email protected]/conn.go:294
# 0x75705d github.com/pion/sctp.(*Association).readLoop+0x15d /src/go/pkg/mod/github.com/pion/[email protected]/association.go:534
2 @ 0x43c576 0x44c41c 0x7513d9 0x7502fd 0x73fd05 0x46db21
# 0x7513d8 github.com/pion/dtls/v2.(*handshakeFSM).finish+0x238 /src/go/pkg/mod/github.com/pion/dtls/[email protected]/handshaker.go:312
# 0x7502fc github.com/pion/dtls/v2.(*handshakeFSM).Run+0x3fc /src/go/pkg/mod/github.com/pion/dtls/[email protected]/handshaker.go:179
# 0x73fd04 github.com/pion/dtls/v2.(*Conn).handshake.func2+0x84 /src/go/pkg/mod/github.com/pion/dtls/[email protected]/conn.go:833
2 @ 0x43c576 0x44c41c 0x757aa8 0x46db21
# 0x757aa7 github.com/pion/sctp.(*Association).writeLoop+0x207 /src/go/pkg/mod/github.com/pion/[email protected]/association.go:583
2 @ 0x43c576 0x44c41c 0x7d86c5 0x46db21
# 0x7d86c4 github.com/pion/ice/v2.(*Agent).taskLoop+0x144 /src/go/pkg/mod/github.com/pion/ice/[email protected]/agent.go:225
2 @ 0x43c576 0x44c41c 0x7d9d4d 0x46db21
# 0x7d9d4c github.com/pion/ice/v2.(*Agent).startOnConnectionStateChangeRoutine.func2+0xac /src/go/pkg/mod/github.com/pion/ice/[email protected]/agent.go:428
2 @ 0x43c576 0x44c41c 0x7da98b 0x46db21
# 0x7da98a github.com/pion/ice/v2.(*Agent).connectivityChecks+0x1aa /src/go/pkg/mod/github.com/pion/ice/[email protected]/agent.go:545
2 @ 0x43c576 0x44c41c 0x801a1a 0x46db21
# 0x801a19 github.com/pion/interceptor/pkg/nack.(*GeneratorInterceptor).loop+0x159 /src/go/pkg/mod/github.com/pion/[email protected]/pkg/nack/generator_interceptor.go:139
2 @ 0x43c576 0x44c41c 0x80543e 0x46db21
# 0x80543d github.com/pion/interceptor/pkg/report.(*ReceiverInterceptor).loop+0x19d /src/go/pkg/mod/github.com/pion/[email protected]/pkg/report/receiver_interceptor.go:97
2 @ 0x43c576 0x44c41c 0x80709e 0x46db21
# 0x80709d github.com/pion/interceptor/pkg/report.(*SenderInterceptor).loop+0x19d /src/go/pkg/mod/github.com/pion/[email protected]/pkg/report/sender_interceptor.go:98
2 @ 0x43c576 0x44c41c 0x808997 0x46db21
# 0x808996 github.com/pion/interceptor/pkg/twcc.(*SenderInterceptor).loop+0xf6 /src/go/pkg/mod/github.com/pion/[email protected]/pkg/twcc/sender_interceptor.go:169
1 @ 0x431896 0x467ba5 0x59dbf5 0x59da0d 0x59a96b 0x86f625 0x46db21
# 0x467ba4 runtime/pprof.runtime_goroutineProfileWithLabels+0x24 /usr/lib/go/src/runtime/mprof.go:846
# 0x59dbf4 runtime/pprof.writeRuntimeProfile+0xb4 /usr/lib/go/src/runtime/pprof/pprof.go:723
# 0x59da0c runtime/pprof.writeGoroutine+0x4c /usr/lib/go/src/runtime/pprof/pprof.go:683
# 0x59a96a runtime/pprof.(*Profile).WriteTo+0x14a /usr/lib/go/src/runtime/pprof/pprof.go:330
# 0x86f624 github.com/pion/transport/test.TimeOut.func1+0x44 /src/go/pkg/mod/github.com/pion/[email protected]/test/util.go:21
1 @ 0x43c576 0x4089db 0x4084d8 0x50cada 0x50ea2e 0x50bceb 0x50e8d6 0x50d3b9 0x92960a 0x43c1b2 0x46db21
# 0x50cad9 testing.(*T).Run+0x379 /usr/lib/go/src/testing/testing.go:1494
# 0x50ea2d testing.runTests.func1+0x6d /usr/lib/go/src/testing/testing.go:1846
# 0x50bcea testing.tRunner+0x10a /usr/lib/go/src/testing/testing.go:1446
# 0x50e8d5 testing.runTests+0x455 /usr/lib/go/src/testing/testing.go:1844
# 0x50d3b8 testing.(*M).Run+0x5d8 /usr/lib/go/src/testing/testing.go:1726
# 0x929609 main.main+0x1a9 _testmain.go:513
# 0x43c1b1 runtime.main+0x211 /usr/lib/go/src/runtime/proc.go:250
1 @ 0x43c576 0x4089db 0x4084d8 0x8fd4ab 0x50bceb 0x46db21
# 0x8fd4aa github.com/pion/webrtc/v3.TestPeerConnection_Regegotiation_AnswerAddsTrack+0x44a /src/pion/webrtc/peerconnection_renegotiation_test.go:1282
# 0x50bcea testing.tRunner+0x10a /usr/lib/go/src/testing/testing.go:1446
1 @ 0x43c576 0x44c41c 0x8f1c8c 0x46db21
# 0x8f1c8b github.com/pion/webrtc/v3.sendVideoUntilDone+0x8b /src/pion/webrtc/peerconnection_renegotiation_test.go:30
panic: timeout
goroutine 92 [running]:
github.com/pion/transport/test.TimeOut.func1()
/src/go/pkg/mod/github.com/pion/[email protected]/test/util.go:24 +0xa5
created by time.goFunc
/usr/lib/go/src/time/sleep.go:176 +0x32
FAIL github.com/pion/webrtc/v3 30.015s
FAIL
$ git checkout 1822ba3e1e40ceae65f58e8b0896c8aadceb9151
Previous HEAD position was f8eb7dc Add test where answerer adds track
HEAD is now at 1822ba3 Revert "Add currentDirection to RTPTransceiver"
$ go test ./ -run AnswerAddsTrack -v
=== RUN TestPeerConnection_Regegotiation_AnswerAddsTrack
on track
--- PASS: TestPeerConnection_Regegotiation_AnswerAddsTrack (0.44s)
PASS
ok github.com/pion/webrtc/v3 0.446s
P.S. I apologize for rolling back your commit on master without discussion yesterday - I reverted that change. I thought I was working on my fork, and it seems the master branch on this repo isn't protected against pushes without merge requests. Honest mistake!
@jeremija Created #2393 to fix your test-case, can you verify it works in your product? @peffis I'm not sure your case is same as @jeremija 's , can you verify the fix pr work for you too? Or you can provide a test case if it is not work.
This seems to be fixed! If you are still seeing issues @jeremija or @peffis tell me and happy to dig into this :)
