-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fire OnTrack before reading first RTP #2790
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2790 +/- ##
==========================================
+ Coverage 78.99% 79.06% +0.06%
==========================================
Files 87 87
Lines 8203 8171 -32
==========================================
- Hits 6480 6460 -20
+ Misses 1256 1246 -10
+ Partials 467 465 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.