-
Notifications
You must be signed in to change notification settings - Fork 635
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
Fix sequence number offset on packet drop #1556
Conversation
r.snOffsetsOccupancy-- | ||
if r.snOffsetsOccupancy < 0 { | ||
r.logger.Warnw("sequence number offset cache is invalid", nil, "occupancy", r.snOffsetsOccupancy) | ||
} |
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 is the main fix.
@@ -1110,7 +1108,7 @@ func (s *StreamAllocator) initProbe(probeRateBps int64) { | |||
"committed", s.committedChannelCapacity, | |||
"lastReceived", s.lastReceivedEstimate, | |||
"probeRateBps", probeRateBps, | |||
"goalBps", expectedBandwidthUsage+probeRateBps, | |||
"goalBps", s.probeGoalBps, |
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.
fixing some logging bits - drive by (actually was working on stream allocator tweaks and had a couple of useful prints for collecting more data, so including here).
All sequence numbers are stored in sequence number offset array. So, the offset write ptr should not be adjusted on packet drop. This was causing sequence numbers to get out of whack in lossy network + congestion case. And that caused PLIs to be generated.
NOTE: Still not able to reproduce the glut of PLIs, but this is one reason for PLIs.
Also made the VP8 munger under loss more resilient. Comments in code.
Enhanced RTPMunger UT to check for more sequence number offsets/write pointer.