Repository: pion/interceptor
Pull request #97 (2022-01-21 21:07:45)

Ignore unrelated RTCP in FeedbackAdapter
This ignores RTCP packets the we can't handle instead of returning
errors. The feedback might still be useful for other interceptors.


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/interceptor/pull/97?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#97](https://codecov.io/gh/pion/interceptor/pull/97?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (311919a) into [master](https://codecov.io/gh/pion/interceptor/commit/21bd73dbab13bb0af86edb00a5c0384bcd9368fe?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (21bd73d) will **increase** coverage by `0.23%`.
> The diff coverage is `72.72%`.

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

```diff
@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   78.88%   79.11%   +0.23%     
==========================================
  Files          51       51              
  Lines        2458     2452       -6     
==========================================
+ Hits         1939     1940       +1     
+ Misses        428      422       -6     
+ Partials       91       90       -1     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `79.11% <72.72%> (+0.35%)` | :arrow_up: |
| wasm | `?` | |

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/97?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [pkg/gcc/send\_side\_bwe.go](https://codecov.io/gh/pion/interceptor/pull/97/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL2djYy9zZW5kX3NpZGVfYndlLmdv) | `73.27% <57.14%> (+0.23%)` | :arrow_up: |
| [internal/cc/feedback\_adapter.go](https://codecov.io/gh/pion/interceptor/pull/97/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvY2MvZmVlZGJhY2tfYWRhcHRlci5nbw==) | `85.10% <100.00%> (+0.94%)` | :arrow_up: |
| [pkg/gcc/delay\_based\_bwe.go](https://codecov.io/gh/pion/interceptor/pull/97/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL2djYy9kZWxheV9iYXNlZF9id2UuZ28=) | `86.36% <0.00%> (+3.03%)` | :arrow_up: |
| [pkg/gcc/arrival\_group\_accumulator.go](https://codecov.io/gh/pion/interceptor/pull/97/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGtnL2djYy9hcnJpdmFsX2dyb3VwX2FjY3VtdWxhdG9yLmdv) | `100.00% <0.00%> (+9.37%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/interceptor/pull/97?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/97?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [21bd73d...311919a](https://codecov.io/gh/pion/interceptor/pull/97?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/AUTHORS.txt b/AUTHORS.txt
index 8f85a31..ea5d0e5 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -11,5 +11,6 @@ Atsushi Watanabe <atsushi.w@ieee.org>
 boks1971 <raja.gobi@tutanota.com>
 David Zhao <david@davidzhao.com>
 Jonathan Müller <jonathan@fotokite.com>
+Kevin Caffrey <kcaffrey@gmail.com>
 Mathis Engelbart <mathis.engelbart@gmail.com>
 Sean DuBois <sean@siobud.com>
diff --git a/internal/cc/feedback_adapter.go b/internal/cc/feedback_adapter.go
index 04b0b76..eb0a9a0 100644
--- a/internal/cc/feedback_adapter.go
+++ b/internal/cc/feedback_adapter.go
@@ -17,9 +17,7 @@ const TwccExtensionAttributesKey = iota
 var (
 	errMissingTWCCExtensionID = errors.New("missing transport layer cc header extension id")
 	errMissingTWCCExtension   = errors.New("missing transport layer cc header extension")
-	errUnknownFeedbackFormat  = errors.New("unknown feedback format")
-
-	errInvalidFeedback = errors.New("invalid feedback")
+	errInvalidFeedback        = errors.New("invalid feedback")
 )
 
 // FeedbackAdapter converts incoming RTCP Packets (TWCC and RFC8888) into Acknowledgments.
@@ -63,20 +61,6 @@ func (f *FeedbackAdapter) OnSent(ts time.Time, header *rtp.Header, size int, att
 	return nil
 }
 
-// OnFeedback converts incoming RTCP packet feedback to Acknowledgments.
-// Currently only TWCC is supported.
-func (f *FeedbackAdapter) OnFeedback(ts time.Time, feedback rtcp.Packet) ([]Acknowledgment, error) {
-	f.lock.Lock()
-	defer f.lock.Unlock()
-
-	switch fb := feedback.(type) {
-	case *rtcp.TransportLayerCC:
-		return f.onIncomingTransportCC(ts, fb)
-	default:
-		return nil, errUnknownFeedbackFormat
-	}
-}
-
 func (f *FeedbackAdapter) unpackRunLengthChunk(ts time.Time, start uint16, refTime time.Time, chunk *rtcp.RunLengthChunk, deltas []*rtcp.RecvDelta) (consumedDeltas int, nextRef time.Time, acks []Acknowledgment, err error) {
 	result := make([]Acknowledgment, chunk.RunLength)
 	deltaIndex := 0
@@ -124,9 +108,13 @@ func (f *FeedbackAdapter) unpackStatusVectorChunk(ts time.Time, start uint16, re
 	return deltaIndex, refTime, result, nil
 }
 
-func (f *FeedbackAdapter) onIncomingTransportCC(ts time.Time, feedback *rtcp.TransportLayerCC) ([]Acknowledgment, error) {
-	result := []Acknowledgment{}
+// OnTransportCCFeedback converts incoming TWCC RTCP packet feedback to
+// Acknowledgments.
+func (f *FeedbackAdapter) OnTransportCCFeedback(ts time.Time, feedback *rtcp.TransportLayerCC) ([]Acknowledgment, error) {
+	f.lock.Lock()
+	defer f.lock.Unlock()
 
+	result := []Acknowledgment{}
 	index := feedback.BaseSequenceNumber
 	refTime := time.Time{}.Add(time.Duration(feedback.ReferenceTime) * 64 * time.Millisecond)
 	recvDeltas := feedback.RecvDeltas
diff --git a/pkg/gcc/send_side_bwe.go b/pkg/gcc/send_side_bwe.go
index 9b7e763..949afdc 100644
--- a/pkg/gcc/send_side_bwe.go
+++ b/pkg/gcc/send_side_bwe.go
@@ -132,12 +132,14 @@ func (e *SendSideBWE) AddStream(info *interceptor.StreamInfo, writer interceptor
 // WriteRTCP adds some RTCP feedback to the bandwidth estimator
 func (e *SendSideBWE) WriteRTCP(pkts []rtcp.Packet, attributes interceptor.Attributes) error {
 	for _, pkt := range pkts {
-		acks, err := e.feedbackAdapter.OnFeedback(time.Now(), pkt)
-		if err != nil {
-			return err
+		if fb, ok := pkt.(*rtcp.TransportLayerCC); ok {
+			acks, err := e.feedbackAdapter.OnTransportCCFeedback(time.Now(), fb)
+			if err != nil {
+				return err
+			}
+			e.lossController.updateLossEstimate(acks)
+			e.delayController.updateDelayEstimate(acks)
 		}
-		e.lossController.updateLossEstimate(acks)
-		e.delayController.updateDelayEstimate(acks)
 	}
 	return nil
 }


Test patch:
diff --git a/internal/cc/feedback_adapter_test.go b/internal/cc/feedback_adapter_test.go
index a9f1861..7922caf 100644
--- a/internal/cc/feedback_adapter_test.go
+++ b/internal/cc/feedback_adapter_test.go
@@ -469,7 +469,7 @@ func getPacketWithTransportCCExt(t *testing.T, sequenceNumber uint16) *rtp.Packe
 func TestFeedbackAdapterTWCC(t *testing.T) {
 	t.Run("empty", func(t *testing.T) {
 		adapter := NewFeedbackAdapter()
-		result, err := adapter.onIncomingTransportCC(time.Time{}, &rtcp.TransportLayerCC{})
+		result, err := adapter.OnTransportCCFeedback(time.Time{}, &rtcp.TransportLayerCC{})
 		assert.NoError(t, err)
 		assert.Empty(t, result)
 	})
@@ -483,7 +483,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 			headers = append(headers, pkt.Header)
 			assert.NoError(t, adapter.OnSent(t0, &pkt.Header, 1200, interceptor.Attributes{TwccExtensionAttributesKey: hdrExtID}))
 		}
-		results, err := adapter.OnFeedback(t0, &rtcp.TransportLayerCC{
+		results, err := adapter.OnTransportCCFeedback(t0, &rtcp.TransportLayerCC{
 			Header:             rtcp.Header{},
 			SenderSSRC:         0,
 			MediaSSRC:          0,
@@ -614,7 +614,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 	t.Run("doesNotCrashOnTooManyFeedbackReports", func(*testing.T) {
 		adapter := NewFeedbackAdapter()
 		assert.NotPanics(t, func() {
-			_, err := adapter.OnFeedback(time.Time{}, &rtcp.TransportLayerCC{
+			_, err := adapter.OnTransportCCFeedback(time.Time{}, &rtcp.TransportLayerCC{
 				Header:             rtcp.Header{},
 				SenderSSRC:         0,
 				MediaSSRC:          0,
@@ -657,7 +657,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 		assert.NoError(t, adapter.OnSent(t0, &pkt65535.Header, 1200, interceptor.Attributes{TwccExtensionAttributesKey: hdrExtID}))
 		assert.NoError(t, adapter.OnSent(t0, &pkt0.Header, 1200, interceptor.Attributes{TwccExtensionAttributesKey: hdrExtID}))
 
-		results, err := adapter.OnFeedback(t0, &rtcp.TransportLayerCC{
+		results, err := adapter.OnTransportCCFeedback(t0, &rtcp.TransportLayerCC{
 			Header:             rtcp.Header{},
 			SenderSSRC:         0,
 			MediaSSRC:          0,
@@ -722,7 +722,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 			assert.NoError(t, adapter.OnSent(t0, &pkt.Header, 1200, interceptor.Attributes{TwccExtensionAttributesKey: hdrExtID}))
 		}
 
-		results, err := adapter.OnFeedback(t0, &rtcp.TransportLayerCC{
+		results, err := adapter.OnTransportCCFeedback(t0, &rtcp.TransportLayerCC{
 			Header:             rtcp.Header{},
 			SenderSSRC:         0,
 			MediaSSRC:          0,
@@ -790,7 +790,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 			pkt := getPacketWithTransportCCExt(t, i)
 			assert.NoError(t, adapter.OnSent(t0, &pkt.Header, 1200, interceptor.Attributes{TwccExtensionAttributesKey: hdrExtID}))
 		}
-		packets, err := adapter.OnFeedback(t0, &rtcp.TransportLayerCC{
+		packets, err := adapter.OnTransportCCFeedback(t0, &rtcp.TransportLayerCC{
 			Header:             rtcp.Header{},
 			SenderSSRC:         0,
 			MediaSSRC:          0,
@@ -831,7 +831,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 			pkt := getPacketWithTransportCCExt(t, i)
 			assert.NoError(t, adapter.OnSent(t0, &pkt.Header, 1200, interceptor.Attributes{TwccExtensionAttributesKey: hdrExtID}))
 		}
-		packets, err := adapter.OnFeedback(t0, &rtcp.TransportLayerCC{
+		packets, err := adapter.OnTransportCCFeedback(t0, &rtcp.TransportLayerCC{
 			Header:             rtcp.Header{},
 			SenderSSRC:         0,
 			MediaSSRC:          0,
@@ -890,7 +890,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 		}
 
 		//nolint:dupl
-		packets, err := adapter.OnFeedback(t0, &rtcp.TransportLayerCC{
+		packets, err := adapter.OnTransportCCFeedback(t0, &rtcp.TransportLayerCC{
 			Header:             rtcp.Header{},
 			SenderSSRC:         0,
 			MediaSSRC:          0,
@@ -958,7 +958,7 @@ func TestFeedbackAdapterTWCC(t *testing.T) {
 
 		//nolint:dupl
 		assert.NotPanics(t, func() {
-			packets, err := adapter.OnFeedback(t0, &rtcp.TransportLayerCC{
+			packets, err := adapter.OnTransportCCFeedback(t0, &rtcp.TransportLayerCC{
 				Header:             rtcp.Header{},
 				SenderSSRC:         0,
 				MediaSSRC:          0,