Repository: pion/interceptor
Pull request #207 (2023-09-27 19:01:54)

TWCC: Fix small receive delta rounding
Prior to this change, a sequence of small receive deltas could get erroneously encoded as a series of 0s.

If the actual receive deltas were {200μs, 200μs, ..., 200μs}, what was appearing in the TWCC feedback packet (after marshaling) was {0, 0, ..., 0}. (Note: the added test, without the code fixes, would fail by returning deltas of {200, 200, ..., 200}, but marshalling the feedback packet would truncate all of those to 0).

To fix this, receive deltas are rounded to the closest "delta tick" period (250μs) as opposed to truncating. Additionally, `lastTimestampUS` is moved forward by the rounded delta instead of set to the timestamp of the last packet. This lets rounding errors accumulate so that a series of packets will have the correct sum of receive deltas (within the margin of error of a delta tick).

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/interceptor/pull/207?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
All modified lines are covered by tests :white_check_mark:

| [Files](https://app.codecov.io/gh/pion/interceptor/pull/207?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [pkg/twcc/twcc.go](https://app.codecov.io/gh/pion/interceptor/pull/207?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3R3Y2MvdHdjYy5nbw==) | `95.02% <100.00%> (+0.14%)` | :arrow_up: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/pion/interceptor/pull/207/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)


:loudspeaker: Thoughts on this report? [Let us know!](https://about.codecov.io/pull-request-comment-report/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

kcaffrey: @boks1971 Thanks for the review, I made those changes.

Patch:
diff --git a/pkg/twcc/twcc.go b/pkg/twcc/twcc.go
index 7f7b857..f403998 100644
--- a/pkg/twcc/twcc.go
+++ b/pkg/twcc/twcc.go
@@ -154,10 +154,16 @@ func (f *feedback) getRTCP() *rtcp.TransportLayerCC {
 
 func (f *feedback) addReceived(sequenceNumber uint16, timestampUS int64) bool {
 	deltaUS := timestampUS - f.lastTimestampUS
-	delta250US := deltaUS / 250
+	var delta250US int64
+	if deltaUS >= 0 {
+		delta250US = (deltaUS + rtcp.TypeTCCDeltaScaleFactor/2) / rtcp.TypeTCCDeltaScaleFactor
+	} else {
+		delta250US = (deltaUS - rtcp.TypeTCCDeltaScaleFactor/2) / rtcp.TypeTCCDeltaScaleFactor
+	}
 	if delta250US < math.MinInt16 || delta250US > math.MaxInt16 { // delta doesn't fit into 16 bit, need to create new packet
 		return false
 	}
+	deltaUSRounded := delta250US * rtcp.TypeTCCDeltaScaleFactor
 
 	for ; f.nextSequenceNumber != sequenceNumber; f.nextSequenceNumber++ {
 		if !f.lastChunk.canAdd(rtcp.TypeTCCPacketNotReceived) {
@@ -183,9 +189,9 @@ func (f *feedback) addReceived(sequenceNumber uint16, timestampUS int64) bool {
 	f.lastChunk.add(recvDelta)
 	f.deltas = append(f.deltas, &rtcp.RecvDelta{
 		Type:  recvDelta,
-		Delta: deltaUS,
+		Delta: deltaUSRounded,
 	})
-	f.lastTimestampUS = timestampUS
+	f.lastTimestampUS += deltaUSRounded
 	f.sequenceNumberCount++
 	f.nextSequenceNumber++
 	return true


Test patch:
diff --git a/pkg/twcc/twcc_test.go b/pkg/twcc/twcc_test.go
index 5ada7a0..c829159 100644
--- a/pkg/twcc/twcc_test.go
+++ b/pkg/twcc/twcc_test.go
@@ -255,6 +255,53 @@ func Test_feedback(t *testing.T) {
 		}
 	})
 
+	t.Run("add received small deltas", func(t *testing.T) {
+		f := newFeedback(0, 0, 0)
+		base := int64(320 * 1000)
+		deltaUS := int64(200)
+		f.setBase(5, base)
+
+		for i := int64(0); i < 5; i++ {
+			got := f.addReceived(5+uint16(i+1), base+deltaUS*i)
+			assert.True(t, got)
+		}
+
+		pkt := f.getRTCP()
+
+		expectedDeltas := []*rtcp.RecvDelta{
+			{
+				Type:  rtcp.TypeTCCPacketReceivedSmallDelta,
+				Delta: 0,
+			},
+			{
+				Type: rtcp.TypeTCCPacketReceivedSmallDelta,
+				// NOTE: The delta is less than the scale factor, but it should be rounded up.
+				// (rtcp.RecvDelta).Marshal() simply truncates to an interval of the scale factor,
+				// so we want to make sure that the deltas have any rounding applied when building
+				// the feedback.
+				Delta: 1 * rtcp.TypeTCCDeltaScaleFactor,
+			},
+			{
+				Type:  rtcp.TypeTCCPacketReceivedSmallDelta,
+				Delta: 1 * rtcp.TypeTCCDeltaScaleFactor,
+			},
+			{
+				Type: rtcp.TypeTCCPacketReceivedSmallDelta,
+				// NOTE: This is zero because even though the deltas are all the same, the rounding error has
+				// built up enough by this packet to cause it to be rounded down.
+				Delta: 0 * rtcp.TypeTCCDeltaScaleFactor,
+			},
+			{
+				Type:  rtcp.TypeTCCPacketReceivedSmallDelta,
+				Delta: 1 * rtcp.TypeTCCDeltaScaleFactor,
+			},
+		}
+		assert.Equal(t, len(expectedDeltas), len(pkt.RecvDeltas))
+		for i, d := range expectedDeltas {
+			assert.Equal(t, d, pkt.RecvDeltas[i])
+		}
+	})
+
 	t.Run("add received wrapped sequence number", func(t *testing.T) {
 		f := newFeedback(0, 0, 0)
 		f.setBase(65535, 320*1000)