More fine grained filtering NACKs after a key frame. #2159
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are applications with periodic key frame. So, a packet lost before a key frame will not be retransmitted. But, decoder could wait (jitter buffer, play out time) and cause a stutter. Idea behind disabling NACKs after key frame was another knob to throttle retransmission bit rate. But, with spaced out retransmissions and max retransmissions per sequence number, there are throttles. This would provide more throttling, but affects some applications. So, disabling filtering NACKs after a key frame. Introducing another flag to disallow layers. This would still be quite useful, i. e. under congestion the stream allocator would move the target lower. But, because of congestion, higher layer would have lost a bunch of packets. Client would NACK those. Retransmitting those higher layer packets would congest the channel more. The new flag (default enabled) would disallow higher layers retransmission. This was happening before this change also, just splitting out the flag for more control.
boks1971
commented
Oct 19, 2023
| @@ -315,7 +315,9 @@ func (r *RTPStatsReceiver) SetRtcpSenderReportData(srData *RTCPSenderReportData) | |||
| timeSinceFirst := srData.NTPTimestamp.Time().Sub(r.srFirst.NTPTimestamp.Time()).Seconds() | |||
| rtpDiffSinceFirst := srDataCopy.RTPTimestampExt - r.srFirst.RTPTimestampExt | |||
| calculatedClockRateFromFirst := float64(rtpDiffSinceFirst) / timeSinceFirst | |||
| if math.Abs(float64(r.params.ClockRate)-calculatedClockRateFromLast) > 0.2*float64(r.params.ClockRate) || math.Abs(float64(r.params.ClockRate)-calculatedClockRateFromFirst) > 0.2*float64(r.params.ClockRate) { | |||
|
|
|||
| if (timeSinceLast > 0.2 && math.Abs(float64(r.params.ClockRate)-calculatedClockRateFromLast) > 0.2*float64(r.params.ClockRate)) || | |||
There was a problem hiding this comment.
This was logging on very small time differences like 4 ms. Setting a min of 200 ms.
boks1971
commented
Oct 19, 2023
| @@ -235,6 +232,8 @@ func NewWebRTCReceiver( | |||
| }) | |||
| w.connectionStats.Start(w.trackInfo) | |||
|
|
|||
| w.streamTrackerManager = NewStreamTrackerManager(logger, trackInfo, w.isSVC, w.codec.ClockRate, trackersConfig) | |||
There was a problem hiding this comment.
drive-by, related code was split, just putting them together.
paulwe
approved these changes
Oct 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are applications with periodic key frame.
So, a packet lost before a key frame will not be retransmitted. But, decoder could wait (jitter buffer, play out time) and cause a stutter.
Idea behind disabling NACKs after key frame was another knob to throttle retransmission bit rate. But, with spaced out retransmissions and max retransmissions per sequence number, there are throttles. This would provide more throttling, but affects some applications. So, disabling filtering NACKs after a key frame.
Introducing another flag to disallow layers. This would still be quite useful, i. e. under congestion the stream allocator would move the target lower. But, because of congestion, higher layer would have lost a bunch of packets. Client would NACK those. Retransmitting those higher layer packets would congest the channel more. The new flag (default enabled) would disallow higher layers retransmission. This was happening before this change also, just splitting out the flag for more control.