Fire OnTrack before reading first RTP #2790
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
edaniels
force-pushed
the
alwaysfireontrack
branch
from
61e945f to
7a3880b
Compare
Prior to this, we would wait for a single RTP packet to figure out the codec which is not to spec.
edaniels
force-pushed
the
alwaysfireontrack
branch
from
7a3880b to
169440e
Compare
| @@ -26,3 +26,4 @@ cover.out | |||
| examples/sfu-ws/cert.pem | |||
| examples/sfu-ws/key.pem | |||
| wasm_exec.js | |||
| *.DS_Store | |||
There was a problem hiding this comment.
Might as well never store these :)
| @@ -187,25 +167,6 @@ func (t *TrackRemote) ReadRTP() (*rtp.Packet, interceptor.Attributes, error) { | |||
| return r, attributes, nil | |||
| } | |||
|
|
|||
| // peek is like Read, but it doesn't discard the packet read | |||
| func (t *TrackRemote) peek(b []byte) (n int, a interceptor.Attributes, err error) { | |||
There was a problem hiding this comment.
This now becomes unused, is it okay to remove?
|
@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? |
|
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. |
|
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? |
|
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. |
|
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 |
|
@cnderrauber tagged you for review for v4 inclusion only |
|
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 |
|
Oh, I see. You're saying it should also fire off the track as well. Let me take a look. |
|
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. |
|
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! |
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.