DataChannel negotiated peer connection's server.AddTrack doesn't always trigger client.OnTrack

[nicksanford]

NOTE: This is my first issue/contribution to Pion. I've done my best to follow the contribution guidelines but if I've missed the mark please let me know.

Your environment.

• 2 webrtc peers using pion/webrtc/v3.2.40

What did you do?

1. Established a PeerConnection negotiated over a data channel between a client (recvonly) and a server (sendonly)
2. Once pc.OnOpen is called on both the client & server, the server creates a NewTrackLocalStaticSample sending h264, passes it to pc.AddTrack, and starts calling trackLocal.WriteSample
3. Client waits for pc.OnTrack to be invoked so it can start receiving the RTP packets written by server

Repro test case: #2775

What did you expect?

The Client's pc.OnTrack callback function is always invoked

What happened?

Sometimes the client's pc.OnTrack callback function is not invoked

Additional Notes:

I think the problem might be in peerconnection.go 's onNegotiationNeeded state machine which changes states even if the onNegotiationNeededHandler is not installed. If I uncomment these lines the repro test case doesn't fail.

[streamer45]

@nicksanford @Sean-Der These changes (pulled from v3.2.42) are causing a regression on our end. A pion-based client is, at times, triggering step 2.2 and ending up in an endless loop.

webrtc/peerconnection.go

Lines 330 to 334 in 43a6a69

	// non-canon step 2.2
	if !pc.ops.IsEmpty() {
	pc.ops.Enqueue(pc.negotiationNeededOp)
	return
	}

I am trying to assemble a simple reproducer but let me know if you have any thoughts.

[edaniels]

@nicksanford @Sean-Der These changes (pulled from v3.2.42) are causing a regression on our end. A pion-based client is, at times, triggering step 2.2 and ending up in an endless loop.

webrtc/peerconnection.go

Lines 330 to 334 in 43a6a69

	// non-canon step 2.2
	if !pc.ops.IsEmpty() {
	pc.ops.Enqueue(pc.negotiationNeededOp)
	return
	}

I am trying to assemble a simple reproducer but let me know if you have any thoughts.

Taking a look now

[edaniels]

So it turns out my patch was partially wrong and this issue is a non-issue. Firing the event handler again after negotiation is already needed is not allowed. This means @nicksanford that if you want to see a negotiation needed event, you must set the handler prior to any background operations happening that could cause the event to fire.

[edaniels]

@streamer45 published https://github.com/pion/webrtc/releases/tag/v3.2.43. let me know if you see the same issue.

[streamer45]

Thanks @edaniels for the incredibly quick turnaround on this! The new changes are working as expected.

[edaniels]

Great! No problem, sorry for the regression!

[streamer45]

No worries. This simply made a bunch of e2e tests fail on our CI, so it was caught pretty early in the process.