Repository: pion/interceptor
Pull request #70 (2021-11-01 16:44:06)

Store RTCP packet slice instead of headers
RTCP headers are never used alone, interceptors always need to parse a
slice of RTCP packets.

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/interceptor/pull/70?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#70](https://codecov.io/gh/pion/interceptor/pull/70?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (cad096e) into [master](https://codecov.io/gh/pion/interceptor/commit/31928f226889c7d1fff217f079bb1fc2e7989c45?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (31928f2) will **decrease** coverage by `0.06%`.
> The diff coverage is `86.20%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/interceptor/pull/70/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/70?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   80.97%   80.90%   -0.07%     
==========================================
  Files          26       26              
  Lines         841      838       -3     
==========================================
- Hits          681      678       -3     
  Misses        109      109              
  Partials       51       51              
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `80.90% <86.20%> (-0.07%)` | :arrow_down: |
| wasm | `78.87% <86.20%> (-0.08%)` | :arrow_down: |

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/70?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [pkg/nack/generator\_interceptor.go](https://codecov.io/gh/pion/interceptor/pull/70/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL25hY2svZ2VuZXJhdG9yX2ludGVyY2VwdG9yLmdv) | `74.62% <66.66%> (ø)` | |
| [pkg/report/receiver\_interceptor.go](https://codecov.io/gh/pion/interceptor/pull/70/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3JlcG9ydC9yZWNlaXZlcl9pbnRlcmNlcHRvci5nbw==) | `77.94% <75.00%> (ø)` | |
| [attributes.go](https://codecov.io/gh/pion/interceptor/pull/70/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YXR0cmlidXRlcy5nbw==) | `80.00% <77.77%> (-1.82%)` | :arrow_down: |
| [pkg/nack/responder\_interceptor.go](https://codecov.io/gh/pion/interceptor/pull/70/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL25hY2svcmVzcG9uZGVyX2ludGVyY2VwdG9yLmdv) | `65.90% <100.00%> (ø)` | |
| [pkg/report/receiver\_stream.go](https://codecov.io/gh/pion/interceptor/pull/70/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3JlcG9ydC9yZWNlaXZlcl9zdHJlYW0uZ28=) | `91.42% <100.00%> (ø)` | |
| [pkg/twcc/sender\_interceptor.go](https://codecov.io/gh/pion/interceptor/pull/70/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL3R3Y2Mvc2VuZGVyX2ludGVyY2VwdG9yLmdv) | `79.71% <100.00%> (-0.29%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/interceptor/pull/70?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/70?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [31928f2...cad096e](https://codecov.io/gh/pion/interceptor/pull/70?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/attributes.go b/attributes.go
index 31d49b6..603315d 100644
--- a/attributes.go
+++ b/attributes.go
@@ -7,14 +7,14 @@ import (
 	"github.com/pion/rtp"
 )
 
-type rtpHeaderKeyType int
+type unmarshaledDataKeyType int
 
 const (
-	rtpHeaderKey rtpHeaderKeyType = iota
-	rtcpHeaderKey
+	rtpHeaderKey unmarshaledDataKeyType = iota
+	rtcpPacketsKey
 )
 
-var errInvalidHeaderType = errors.New("found invalid header type in attributes map")
+var errInvalidType = errors.New("found value of invalid type in attributes map")
 
 // Attributes are a generic key/value store used by interceptors
 type Attributes map[interface{}]interface{}
@@ -29,38 +29,37 @@ func (a Attributes) Set(key interface{}, val interface{}) {
 	a[key] = val
 }
 
-// GetRTPHeader gets the RTP header if present. If the header is not present, it
-// will be unmarshaled from the raw byte slice and stored in the attributes.
+// GetRTPHeader gets the RTP header if present. If it is not present, it will be
+// unmarshalled from the raw byte slice and stored in the attribtues.
 func (a Attributes) GetRTPHeader(raw []byte) (*rtp.Header, error) {
 	if val, ok := a[rtpHeaderKey]; ok {
 		if header, ok := val.(*rtp.Header); ok {
 			return header, nil
 		}
-		return nil, errInvalidHeaderType
+		return nil, errInvalidType
 	}
 	header := &rtp.Header{}
-	_, err := header.Unmarshal(raw)
-	if err != nil {
+	if _, err := header.Unmarshal(raw); err != nil {
 		return nil, err
 	}
 	a[rtpHeaderKey] = header
 	return header, nil
 }
 
-// GetRTCPHeader gets the RTCP header if present. If the header is not present,
-// it will be unmarshaled from the raw byte slice and stored in the attributes.
-func (a Attributes) GetRTCPHeader(raw []byte) (*rtcp.Header, error) {
-	if val, ok := a[rtcpHeaderKey]; ok {
-		if header, ok := val.(*rtcp.Header); ok {
-			return header, nil
+// GetRTCPPackets gets the RTCP packets if present. If the packet slice is not
+// present, it will be unmarshaled from the raw byte slice and stored in the
+// attributes.
+func (a Attributes) GetRTCPPackets(raw []byte) ([]rtcp.Packet, error) {
+	if val, ok := a[rtcpPacketsKey]; ok {
+		if packets, ok := val.([]rtcp.Packet); ok {
+			return packets, nil
 		}
-		return nil, errInvalidHeaderType
+		return nil, errInvalidType
 	}
-	header := &rtcp.Header{}
-	err := header.Unmarshal(raw)
+	pkts, err := rtcp.Unmarshal(raw)
 	if err != nil {
 		return nil, err
 	}
-	a[rtcpHeaderKey] = header
-	return header, nil
+	a[rtcpPacketsKey] = pkts
+	return pkts, nil
 }
diff --git a/pkg/nack/generator_interceptor.go b/pkg/nack/generator_interceptor.go
index 6110309..58d720f 100644
--- a/pkg/nack/generator_interceptor.go
+++ b/pkg/nack/generator_interceptor.go
@@ -8,7 +8,6 @@ import (
 	"github.com/pion/interceptor"
 	"github.com/pion/logging"
 	"github.com/pion/rtcp"
-	"github.com/pion/rtp"
 )
 
 // GeneratorInterceptorFactory is a interceptor.Factory for a GeneratorInterceptor
@@ -96,11 +95,11 @@ func (n *GeneratorInterceptor) BindRemoteStream(info *interceptor.StreamInfo, re
 			return 0, nil, err
 		}
 
-		pkt := rtp.Packet{}
-		if err = pkt.Unmarshal(b[:i]); err != nil {
+		header, err := attr.GetRTPHeader(b[:i])
+		if err != nil {
 			return 0, nil, err
 		}
-		receiveLog.add(pkt.Header.SequenceNumber)
+		receiveLog.add(header.SequenceNumber)
 
 		return i, attr, nil
 	})
diff --git a/pkg/nack/responder_interceptor.go b/pkg/nack/responder_interceptor.go
index 0a2b850..31a2423 100644
--- a/pkg/nack/responder_interceptor.go
+++ b/pkg/nack/responder_interceptor.go
@@ -64,7 +64,7 @@ func (n *ResponderInterceptor) BindRTCPReader(reader interceptor.RTCPReader) int
 			return 0, nil, err
 		}
 
-		pkts, err := rtcp.Unmarshal(b[:i])
+		pkts, err := attr.GetRTCPPackets(b[:i])
 		if err != nil {
 			return 0, nil, err
 		}
diff --git a/pkg/report/receiver_interceptor.go b/pkg/report/receiver_interceptor.go
index 18f6206..596c5a9 100644
--- a/pkg/report/receiver_interceptor.go
+++ b/pkg/report/receiver_interceptor.go
@@ -7,7 +7,6 @@ import (
 	"github.com/pion/interceptor"
 	"github.com/pion/logging"
 	"github.com/pion/rtcp"
-	"github.com/pion/rtp"
 )
 
 // ReceiverInterceptorFactory is a interceptor.Factory for a ReceiverInterceptor
@@ -130,12 +129,12 @@ func (r *ReceiverInterceptor) BindRemoteStream(info *interceptor.StreamInfo, rea
 			return 0, nil, err
 		}
 
-		pkt := rtp.Packet{}
-		if err = pkt.Unmarshal(b[:i]); err != nil {
+		header, err := attr.GetRTPHeader(b[:i])
+		if err != nil {
 			return 0, nil, err
 		}
 
-		stream.processRTP(r.now(), &pkt)
+		stream.processRTP(r.now(), header)
 
 		return i, attr, nil
 	})
@@ -155,7 +154,7 @@ func (r *ReceiverInterceptor) BindRTCPReader(reader interceptor.RTCPReader) inte
 			return 0, nil, err
 		}
 
-		pkts, err := rtcp.Unmarshal(b[:i])
+		pkts, err := attr.GetRTCPPackets(b[:i])
 		if err != nil {
 			return 0, nil, err
 		}
diff --git a/pkg/report/receiver_stream.go b/pkg/report/receiver_stream.go
index 569715d..c572261 100644
--- a/pkg/report/receiver_stream.go
+++ b/pkg/report/receiver_stream.go
@@ -40,21 +40,21 @@ func newReceiverStream(ssrc uint32, clockRate uint32) *receiverStream {
 	}
 }
 
-func (stream *receiverStream) processRTP(now time.Time, pkt *rtp.Packet) {
+func (stream *receiverStream) processRTP(now time.Time, pktHeader *rtp.Header) {
 	stream.m.Lock()
 	defer stream.m.Unlock()
 
 	if !stream.started { // first frame
 		stream.started = true
-		stream.setReceived(pkt.SequenceNumber)
-		stream.lastSeqnum = pkt.SequenceNumber
-		stream.lastReportSeqnum = pkt.SequenceNumber - 1
-		stream.lastRTPTimeRTP = pkt.Timestamp
+		stream.setReceived(pktHeader.SequenceNumber)
+		stream.lastSeqnum = pktHeader.SequenceNumber
+		stream.lastReportSeqnum = pktHeader.SequenceNumber - 1
+		stream.lastRTPTimeRTP = pktHeader.Timestamp
 		stream.lastRTPTimeTime = now
 	} else { // following frames
-		stream.setReceived(pkt.SequenceNumber)
+		stream.setReceived(pktHeader.SequenceNumber)
 
-		diff := int32(pkt.SequenceNumber) - int32(stream.lastSeqnum)
+		diff := int32(pktHeader.SequenceNumber) - int32(stream.lastSeqnum)
 		if diff > 0 || diff < -0x0FFF {
 			// overflow
 			if diff < -0x0FFF {
@@ -62,22 +62,22 @@ func (stream *receiverStream) processRTP(now time.Time, pkt *rtp.Packet) {
 			}
 
 			// set missing packets as missing
-			for i := stream.lastSeqnum + 1; i != pkt.SequenceNumber; i++ {
+			for i := stream.lastSeqnum + 1; i != pktHeader.SequenceNumber; i++ {
 				stream.delReceived(i)
 			}
 
-			stream.lastSeqnum = pkt.SequenceNumber
+			stream.lastSeqnum = pktHeader.SequenceNumber
 		}
 
 		// compute jitter
 		// https://tools.ietf.org/html/rfc3550#page-39
 		D := now.Sub(stream.lastRTPTimeTime).Seconds()*stream.clockRate -
-			(float64(pkt.Timestamp) - float64(stream.lastRTPTimeRTP))
+			(float64(pktHeader.Timestamp) - float64(stream.lastRTPTimeRTP))
 		if D < 0 {
 			D = -D
 		}
 		stream.jitter += (D - stream.jitter) / 16
-		stream.lastRTPTimeRTP = pkt.Timestamp
+		stream.lastRTPTimeRTP = pktHeader.Timestamp
 		stream.lastRTPTimeTime = now
 	}
 }
diff --git a/pkg/twcc/sender_interceptor.go b/pkg/twcc/sender_interceptor.go
index 9189d98..9201ae9 100644
--- a/pkg/twcc/sender_interceptor.go
+++ b/pkg/twcc/sender_interceptor.go
@@ -114,20 +114,19 @@ func (s *SenderInterceptor) BindRemoteStream(info *interceptor.StreamInfo, reade
 		if err != nil {
 			return 0, nil, err
 		}
-		p := rtp.Packet{}
-		err = p.Unmarshal(buf[:i])
+		header, err := attr.GetRTPHeader(buf[:i])
 		if err != nil {
 			return 0, nil, err
 		}
 		var tccExt rtp.TransportCCExtension
-		if ext := p.GetExtension(hdrExtID); ext != nil {
+		if ext := header.GetExtension(hdrExtID); ext != nil {
 			err = tccExt.Unmarshal(ext)
 			if err != nil {
 				return 0, nil, err
 			}
 
 			s.packetChan <- packet{
-				hdr:            &p.Header,
+				hdr:            header,
 				sequenceNumber: tccExt.TransportSequence,
 				arrivalTime:    time.Since(s.startTime).Microseconds(),
 				ssrc:           info.SSRC,


Test patch:
diff --git a/attributes_test.go b/attributes_test.go
index 09f2360..a15c9c2 100644
--- a/attributes_test.go
+++ b/attributes_test.go
@@ -81,64 +81,47 @@ func TestAttributesGetRTPHeader(t *testing.T) {
 	})
 }
 
-func TestAttributesGetRTCPHeader(t *testing.T) {
-	t.Run("NilHeader", func(t *testing.T) {
+func TestAttributesGetRTCPPackets(t *testing.T) {
+	t.Run("NilPacket", func(t *testing.T) {
 		attributes := Attributes{}
-		_, err := attributes.GetRTCPHeader(nil)
+		_, err := attributes.GetRTCPPackets(nil)
 		assert.Error(t, err)
 	})
 
 	t.Run("Present", func(t *testing.T) {
 		attributes := Attributes{
-			rtcpHeaderKey: &rtcp.Header{
-				Padding: false,
-				Count:   0,
-				Type:    0,
-				Length:  0,
+			rtcpPacketsKey: []rtcp.Packet{
+				&rtcp.TransportLayerCC{
+					Header:             rtcp.Header{Padding: false, Count: 0, Type: 0, Length: 0},
+					SenderSSRC:         0,
+					MediaSSRC:          0,
+					BaseSequenceNumber: 0,
+					PacketStatusCount:  0,
+					ReferenceTime:      0,
+					FbPktCount:         0,
+					PacketChunks:       []rtcp.PacketStatusChunk{},
+					RecvDeltas:         []*rtcp.RecvDelta{},
+				},
 			},
 		}
-		header, err := attributes.GetRTCPHeader(nil)
+		packets, err := attributes.GetRTCPPackets(nil)
 		assert.NoError(t, err)
-		assert.Equal(t, attributes[rtcpHeaderKey], header)
+		assert.Equal(t, attributes[rtcpPacketsKey], packets)
 	})
 
 	t.Run("NotPresent", func(t *testing.T) {
 		attributes := Attributes{}
-		hdr := &rtcp.Header{
-			Padding: false,
-			Count:   0,
-			Type:    0,
-			Length:  0,
-		}
-		buf, err := hdr.Marshal()
-		assert.NoError(t, err)
-		header, err := attributes.GetRTCPHeader(buf)
-		assert.NoError(t, err)
-		assert.Equal(t, hdr, header)
-	})
-
-	t.Run("NotPresentFromFullRTCPPacket", func(t *testing.T) {
-		attributes := Attributes{}
-		pkt := rtcp.TransportLayerCC{
-			Header: rtcp.Header{
-				Padding: false,
-				Count:   0,
-				Type:    0,
-				Length:  0,
-			},
-			SenderSSRC:         0,
-			MediaSSRC:          0,
-			BaseSequenceNumber: 0,
-			PacketStatusCount:  0,
-			ReferenceTime:      0,
-			FbPktCount:         0,
-			PacketChunks:       []rtcp.PacketStatusChunk{},
-			RecvDeltas:         []*rtcp.RecvDelta{},
+		sr := &rtcp.SenderReport{
+			SSRC:        0,
+			NTPTime:     0,
+			RTPTime:     0,
+			PacketCount: 0,
+			OctetCount:  0,
 		}
-		buf, err := pkt.Marshal()
+		buf, err := sr.Marshal()
 		assert.NoError(t, err)
-		header, err := attributes.GetRTCPHeader(buf)
+		packets, err := attributes.GetRTCPPackets(buf)
 		assert.NoError(t, err)
-		assert.Equal(t, &pkt.Header, header)
+		assert.Equal(t, []rtcp.Packet{sr}, packets)
 	})
 }