Skip to content

Commit

Permalink
TWCC: Fix small receive delta rounding
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
kcaffrey committed Sep 27, 2023
1 parent ca7a624 commit cbbf263
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 3 deletions.
11 changes: 8 additions & 3 deletions pkg/twcc/twcc.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,12 @@ 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 + 125) / 250
} else {
delta250US = (deltaUS - 125) / 250
}
if delta250US < math.MinInt16 || delta250US > math.MaxInt16 { // delta doesn't fit into 16 bit, need to create new packet
return false
}
Expand Down Expand Up @@ -183,9 +188,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: delta250US * 250,
})
f.lastTimestampUS = timestampUS
f.lastTimestampUS += delta250US * 250
f.sequenceNumberCount++
f.nextSequenceNumber++
return true
Expand Down
47 changes: 47 additions & 0 deletions pkg/twcc/twcc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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), 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)
Expand Down

0 comments on commit cbbf263

Please sign in to comment.