Repository: pion/interceptor
Pull request #83 (2021-12-10 16:18:13)

Fix a bug in Chunk encoding
#### Description

Always encode all of the remaining deltas in the last chunk. This did
not happen, when the chunk contained more than 7 but less than 14
deltas. This can happen, if we added 8 symbols that fit in one symbol,
but then had to start a new chunk, because the next symbol was too big.

Also happens, if we're done after more than 7, but less than 14 symbols.
In this case, it might be possible to optimize for space and only use
one symbol, but in most cases this will not make a difference and we
would have to check if the chunk only contains symbols that can be
represented by 1 bit.


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/interceptor/pull/83?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#83](https://codecov.io/gh/pion/interceptor/pull/83?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (6e3f6f3) into [master](https://codecov.io/gh/pion/interceptor/commit/6abf6b9be50a085475ab0d7d212cb856e642701c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (6abf6b9) will **increase** coverage by `0.12%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/interceptor/pull/83/graphs/tree.svg?width=650&height=150&src=pr&token=FNH9xUG6AX&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/interceptor/pull/83?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   79.53%   79.65%   +0.12%     
==========================================
  Files          31       31              
  Lines         987      983       -4     
==========================================
- Hits          785      783       -2     
+ Misses        135      134       -1     
+ Partials       67       66       -1     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `79.65% <100.00%> (+0.12%)` | :arrow_up: |
| wasm | `77.92% <100.00%> (+0.11%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/interceptor/pull/83?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://codecov.io/gh/pion/interceptor/pull/83/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3R3Y2MvdHdjYy5nbw==) | `94.53% <100.00%> (+1.34%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/interceptor/pull/83?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/interceptor/pull/83?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [6abf6b9...6e3f6f3](https://codecov.io/gh/pion/interceptor/pull/83?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/pkg/twcc/twcc.go b/pkg/twcc/twcc.go
index e0f7f0a..0e43f69 100644
--- a/pkg/twcc/twcc.go
+++ b/pkg/twcc/twcc.go
@@ -3,7 +3,6 @@ package twcc
 
 import (
 	"math"
-	"sort"
 
 	"github.com/pion/rtcp"
 )
@@ -78,9 +77,6 @@ func (r *Recorder) BuildFeedbackPacket() []rtcp.Packet {
 		return []rtcp.Packet{feedback.getRTCP()}
 	}
 
-	sort.Slice(r.receivedPackets, func(i, j int) bool {
-		return r.receivedPackets[i].sequenceNumber < r.receivedPackets[j].sequenceNumber
-	})
 	feedback.setBase(uint16(r.receivedPackets[0].sequenceNumber&0xffff), r.receivedPackets[0].arrivalTime)
 
 	var pkts []rtcp.Packet
@@ -133,10 +129,10 @@ func (f *feedback) getRTCP() *rtcp.TransportLayerCC {
 	f.rtcp.PacketStatusCount = f.sequenceNumberCount
 	f.rtcp.ReferenceTime = uint32(f.refTimestamp64MS)
 	f.rtcp.BaseSequenceNumber = f.baseSequenceNumber
-	if len(f.lastChunk.deltas) > 0 {
+	for len(f.lastChunk.deltas) > 0 {
 		f.chunks = append(f.chunks, f.lastChunk.encode())
-		f.rtcp.PacketChunks = append(f.rtcp.PacketChunks, f.chunks...)
 	}
+	f.rtcp.PacketChunks = append(f.rtcp.PacketChunks, f.chunks...)
 	f.rtcp.RecvDeltas = f.deltas
 
 	padLen := 20 + len(f.rtcp.PacketChunks)*2 + f.len // 4 bytes header + 16 bytes twcc header + 2 bytes for each chunk + length of deltas


Test patch:
diff --git a/pkg/twcc/twcc_test.go b/pkg/twcc/twcc_test.go
index 975c9ca..e9a7530 100644
--- a/pkg/twcc/twcc_test.go
+++ b/pkg/twcc/twcc_test.go
@@ -548,6 +548,197 @@ func TestDuplicatePackets(t *testing.T) {
 	marshalAll(t, rtcpPackets)
 }
 
+func TestShortDeltas(t *testing.T) {
+	t.Run("SplitsOneBitDeltas", func(t *testing.T) {
+		r := NewRecorder(5000)
+
+		arrivalTime := int64(scaleFactorReferenceTime)
+		addRun(t, r, []uint16{3, 4, 5, 7, 6, 8, 10, 11, 13, 14}, []int64{
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+		})
+
+		rtcpPackets := r.BuildFeedbackPacket()
+		assert.Equal(t, 1, len(rtcpPackets))
+
+		pkt := rtcpPackets[0].(*rtcp.TransportLayerCC)
+		bs, err := pkt.Marshal()
+		unmarshalled := &rtcp.TransportLayerCC{}
+		assert.NoError(t, err)
+		assert.NoError(t, unmarshalled.Unmarshal(bs))
+
+		assert.Equal(t, &rtcp.TransportLayerCC{
+			Header: rtcp.Header{
+				Count:   rtcp.FormatTCC,
+				Type:    rtcp.TypeTransportSpecificFeedback,
+				Padding: true,
+				Length:  8,
+			},
+			SenderSSRC:         5000,
+			MediaSSRC:          5000,
+			BaseSequenceNumber: 3,
+			ReferenceTime:      1,
+			FbPktCount:         0,
+			PacketStatusCount:  12,
+			PacketChunks: []rtcp.PacketStatusChunk{
+				&rtcp.StatusVectorChunk{
+					PacketStatusChunk: nil,
+					Type:              rtcp.BitVectorChunkType,
+					SymbolSize:        rtcp.TypeTCCSymbolSizeTwoBit,
+					SymbolList:        []uint16{1, 1, 1, 1, 1, 1, 0},
+				},
+				&rtcp.StatusVectorChunk{
+					PacketStatusChunk: nil,
+					Type:              rtcp.BitVectorChunkType,
+					SymbolSize:        rtcp.TypeTCCSymbolSizeTwoBit,
+					SymbolList:        []uint16{1, 1, 0, 1, 1, 0, 0},
+				},
+			},
+			RecvDeltas: []*rtcp.RecvDelta{
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+			},
+		}, unmarshalled)
+		marshalAll(t, rtcpPackets)
+	})
+
+	t.Run("padsTwoBitDeltas", func(t *testing.T) {
+		r := NewRecorder(5000)
+
+		arrivalTime := int64(scaleFactorReferenceTime)
+		addRun(t, r, []uint16{3, 4, 5, 7}, []int64{
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+			arrivalTime,
+		})
+
+		rtcpPackets := r.BuildFeedbackPacket()
+		assert.Equal(t, 1, len(rtcpPackets))
+
+		pkt := rtcpPackets[0].(*rtcp.TransportLayerCC)
+		bs, err := pkt.Marshal()
+		unmarshalled := &rtcp.TransportLayerCC{}
+		assert.NoError(t, err)
+		assert.NoError(t, unmarshalled.Unmarshal(bs))
+
+		assert.Equal(t, &rtcp.TransportLayerCC{
+			Header: rtcp.Header{
+				Count:   rtcp.FormatTCC,
+				Type:    rtcp.TypeTransportSpecificFeedback,
+				Padding: true,
+				Length:  6,
+			},
+			SenderSSRC:         5000,
+			MediaSSRC:          5000,
+			BaseSequenceNumber: 3,
+			ReferenceTime:      1,
+			FbPktCount:         0,
+			PacketStatusCount:  5,
+			PacketChunks: []rtcp.PacketStatusChunk{
+				&rtcp.StatusVectorChunk{
+					PacketStatusChunk: nil,
+					Type:              rtcp.BitVectorChunkType,
+					SymbolSize:        rtcp.TypeTCCSymbolSizeTwoBit,
+					SymbolList:        []uint16{1, 1, 1, 0, 1, 0, 0},
+				},
+			},
+			RecvDeltas: []*rtcp.RecvDelta{
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+				{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 0},
+			},
+		}, unmarshalled)
+		marshalAll(t, rtcpPackets)
+	})
+}
+
+func TestReorderedPackets(t *testing.T) {
+	r := NewRecorder(5000)
+
+	arrivalTime := int64(scaleFactorReferenceTime)
+	addRun(t, r, []uint16{3, 4, 5, 7, 6, 8, 10, 11, 13, 14}, []int64{
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+		increaseTime(&arrivalTime, rtcp.TypeTCCDeltaScaleFactor),
+	})
+
+	rtcpPackets := r.BuildFeedbackPacket()
+	assert.Equal(t, 1, len(rtcpPackets))
+
+	pkt := rtcpPackets[0].(*rtcp.TransportLayerCC)
+	bs, err := pkt.Marshal()
+	unmarshalled := &rtcp.TransportLayerCC{}
+	assert.NoError(t, err)
+	assert.NoError(t, unmarshalled.Unmarshal(bs))
+
+	assert.Equal(t, &rtcp.TransportLayerCC{
+		Header: rtcp.Header{
+			Count:   rtcp.FormatTCC,
+			Type:    rtcp.TypeTransportSpecificFeedback,
+			Padding: true,
+			Length:  8,
+		},
+		SenderSSRC:         5000,
+		MediaSSRC:          5000,
+		BaseSequenceNumber: 3,
+		ReferenceTime:      1,
+		FbPktCount:         0,
+		PacketStatusCount:  12,
+		PacketChunks: []rtcp.PacketStatusChunk{
+			&rtcp.StatusVectorChunk{
+				PacketStatusChunk: nil,
+				Type:              rtcp.BitVectorChunkType,
+				SymbolSize:        rtcp.TypeTCCSymbolSizeTwoBit,
+				SymbolList:        []uint16{1, 1, 1, 1, 2, 1, 0},
+			},
+			&rtcp.StatusVectorChunk{
+				PacketStatusChunk: nil,
+				Type:              rtcp.BitVectorChunkType,
+				SymbolSize:        rtcp.TypeTCCSymbolSizeTwoBit,
+				SymbolList:        []uint16{1, 1, 0, 1, 1, 0, 0},
+			},
+		},
+		RecvDeltas: []*rtcp.RecvDelta{
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 2 * rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedLargeDelta, Delta: -rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: 2 * rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: rtcp.TypeTCCDeltaScaleFactor},
+			{Type: rtcp.TypeTCCPacketReceivedSmallDelta, Delta: rtcp.TypeTCCDeltaScaleFactor},
+		},
+	}, unmarshalled)
+	marshalAll(t, rtcpPackets)
+}
+
 func TestInsertSorted(t *testing.T) {
 	cases := []struct {
 		l        []pktInfo