Fire OnTrack before reading first RTP

[edaniels]

Prior to this, we would wait for a single RTP packet to figure out the codec which is not to spec.

@Sean-Der, as far as I can tell based on what chrome does and what the spec says, we're suppose to fire off on track, even if the codec may not be set yet. What this means is you need to read at least one RTP packet on your own within OnTrack before getting that info. This may be a breaking change though.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 79.10%. Comparing base (de5d997) to head (fd2580f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2790      +/-   ##
==========================================
- Coverage   79.18%   79.10%   -0.09%     
==========================================
  Files          89       89              
  Lines        8268     8236      -32     
==========================================
- Hits         6547     6515      -32     
  Misses       1255     1255              
  Partials      466      466              

Flag	Coverage Δ
go	80.72% <100.00%> (-0.09%)	:arrow_down:
wasm	64.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[edaniels]

@Sean-Der it seems likely users would be relying on Codec and PayloadType being accessible when OnTrack is called. With this change, you'd need to read first before calling those. I could put the peek code into those two methods instead which would preserve the old behavior. What do you think?

[cnderrauber]

Good change to make the ontrack behavior consistent with the browser! It is useful when client want to publish a muted track. Agree it is a breaking change that make Codec() and PayloadType() would block until the first packet arrives.

[edaniels]

So if we make it blocking in v3 until the first packet on those two methods, I don't think it's breaking so long as we handle the other cases where the values are already set outside of the peeking process. For v4 I think this can go in as is. What do you think?

[cnderrauber]

My concern is just for v3 user the application might hold a lock and call codec() to do something when ontrack fired, in livekit we have the code, so the new behavior will cause chaos for existing applications if the developer isn't aware of that. I prefer to keep the old behavior in v3 and only apply the change to v4.

[edaniels]

Yep that's fair. I'm looking at something else I've written for a client as well and it's too subtle to not cause problems. Let's just do this for v4

[edaniels]

@cnderrauber tagged you for review for v4 inclusion only

[cnderrauber]

Looks good for normal track. Another case is the simulcast track still relies on rtp packet to fire the event, we need consistent behavior for all tracks to avoid confusing developers.

[edaniels]

Looks good for normal track. Another case is the simulcast track still relies on rtp packet to fire the event, we need consistent behavior for all tracks to avoid confusing developers.

The simulcast code is still intercepting packets so I think we're fine there https://github.com/pion/webrtc/blob/master/peerconnection.go#L1608-L1626

[edaniels]

Oh, I see. You're saying it should also fire off the track as well. Let me take a look.

[edaniels]

It seems like the probing and what's populated into the track details before on track for simulcast is pretty tightly coupled right now. It's going to take me a bit to wrap my head around the right way to fire off the events as quickly as possible while avoiding invalid state. A naive first stab of setting up the on tracks all in the same place resulted in panics due to not having the right receiver available for RTCP.

[edaniels]

This is no longer a priority for the client I'm doing this work for, so I'll close this for now. Anyone is free to pick this work up though!