Repository: pion/webrtc
Pull request #1964 (2021-09-16 20:20:44)

Add InterceptorFactory
Interceptors are being accidentally misused by users.
The issue is that an Interceptor can be re-used between
multiple PeerConnections. Interceptors were designed to only be
single PeerConnection aware, so state is being corrupted.

Instead we are now going to provide InterceptorFactories. The default
API of pion/webrtc will now be safe to use between PeerConnections.

Resolves webrtc#1956


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1964?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#1964](https://codecov.io/gh/pion/webrtc/pull/1964?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (7630fdd) into [master](https://codecov.io/gh/pion/webrtc/commit/1231f4f8acbd6d4069297d6be699111b6523072b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (1231f4f) will **decrease** coverage by `6.34%`.
> The diff coverage is `n/a`.

> :exclamation: Current head 7630fdd differs from pull request most recent head e5b1a06. Consider uploading reports for the commit e5b1a06 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/1964/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/webrtc/pull/1964?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master    #1964      +/-   ##
==========================================
- Coverage   77.06%   70.72%   -6.35%     
==========================================
  Files          85       62      -23     
  Lines        6209     2333    -3876     
==========================================
- Hits         4785     1650    -3135     
+ Misses       1022      567     -455     
+ Partials      402      116     -286     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `70.72% <ø> (ø)` | |

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/webrtc/pull/1964?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uc3RhbnRzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwY29kZWMuZ28=) | `14.28% <0.00%> (-76.20%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `8.33% <0.00%> (-58.34%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlbXV4Lmdv) | `0.00% <0.00%> (-55.56%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-37.50%)` | :arrow_down: |
| ... and [36 more](https://codecov.io/gh/pion/webrtc/pull/1964/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/1964?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/webrtc/pull/1964?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [1231f4f...e5b1a06](https://codecov.io/gh/pion/webrtc/pull/1964?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).

Sean-Der: I am really happy with this!

We have API breakage in pion/interceptor, but none in pion/webrtc and solves our re-usage problems.

Patch:
diff --git a/api.go b/api.go
index 60ac7280..baee204c 100644
--- a/api.go
+++ b/api.go
@@ -7,19 +7,22 @@ import (
 	"github.com/pion/logging"
 )
 
-// API bundles the global functions of the WebRTC and ORTC API.
-// Some of these functions are also exported globally using the
-// defaultAPI object. Note that the global version of the API
-// may be phased out in the future.
+// API allows configuration of a PeerConnection
+// with APIs that are available in the standard. This
+// lets you set custom behavior via the SettingEngine, configure
+// codecs via the MediaEngine and define custom media behaviors via
+// Interceptors.
 type API struct {
-	settingEngine *SettingEngine
-	mediaEngine   *MediaEngine
-	interceptor   interceptor.Interceptor
+	settingEngine       *SettingEngine
+	mediaEngine         *MediaEngine
+	interceptorRegistry *interceptor.Registry
+
+	interceptor interceptor.Interceptor // Generated per PeerConnection
 }
 
 // NewAPI Creates a new API object for keeping semi-global settings to WebRTC objects
 func NewAPI(options ...func(*API)) *API {
-	a := &API{}
+	a := &API{interceptor: &interceptor.NoOp{}}
 
 	for _, o := range options {
 		o(a)
@@ -37,8 +40,8 @@ func NewAPI(options ...func(*API)) *API {
 		a.mediaEngine = &MediaEngine{}
 	}
 
-	if a.interceptor == nil {
-		a.interceptor = &interceptor.NoOp{}
+	if a.interceptorRegistry == nil {
+		a.interceptorRegistry = &interceptor.Registry{}
 	}
 
 	return a
@@ -68,6 +71,6 @@ func WithSettingEngine(s SettingEngine) func(a *API) {
 // Settings should not be changed after passing the registry to an API.
 func WithInterceptorRegistry(interceptorRegistry *interceptor.Registry) func(a *API) {
 	return func(a *API) {
-		a.interceptor = interceptorRegistry.Build()
+		a.interceptorRegistry = interceptorRegistry
 	}
 }
diff --git a/go.mod b/go.mod
index 794185cd..ce7fcf76 100644
--- a/go.mod
+++ b/go.mod
@@ -8,7 +8,7 @@ require (
 	github.com/pion/datachannel v1.4.21
 	github.com/pion/dtls/v2 v2.0.9
 	github.com/pion/ice/v2 v2.1.12
-	github.com/pion/interceptor v0.0.19
+	github.com/pion/interceptor v0.1.0
 	github.com/pion/logging v0.2.2
 	github.com/pion/randutil v0.1.0
 	github.com/pion/rtcp v1.2.8
diff --git a/go.sum b/go.sum
index c9349fdc..770901c1 100644
--- a/go.sum
+++ b/go.sum
@@ -43,8 +43,8 @@ github.com/pion/dtls/v2 v2.0.9 h1:7Ow+V++YSZQMYzggI0P9vLJz/hUFcffsfGMfT/Qy+u8=
 github.com/pion/dtls/v2 v2.0.9/go.mod h1:O0Wr7si/Zj5/EBFlDzDd6UtVxx25CE1r7XM7BQKYQho=
 github.com/pion/ice/v2 v2.1.12 h1:ZDBuZz+fEI7iDifZCYFVzI4p0Foy0YhdSSZ87ZtRcRE=
 github.com/pion/ice/v2 v2.1.12/go.mod h1:ovgYHUmwYLlRvcCLI67PnQ5YGe+upXZbGgllBDG/ktU=
-github.com/pion/interceptor v0.0.19 h1:NkxrKHVH7ulrkVHTcZRJubgsF1oJeLQUvMsX1Kqm8to=
-github.com/pion/interceptor v0.0.19/go.mod h1:mv0Q0oPHxjRY8xz5v85G6aIqb1Tb0G0mxrZOaewHiVo=
+github.com/pion/interceptor v0.1.0 h1:SlXKaDlEvSl7cr4j8fJykzVz4UdH+7UDtcvx+u01wLU=
+github.com/pion/interceptor v0.1.0/go.mod h1:j5NIl3tJJPB3u8+Z2Xz8MZs/VV6rc+If9mXEKNuFmEM=
 github.com/pion/logging v0.2.2 h1:M9+AIj/+pxNsDfAT64+MAVgJO0rsyLnoJKCqf//DoeY=
 github.com/pion/logging v0.2.2/go.mod h1:k0/tDVsRCX2Mb2ZEmTqNa7CWsQPc+YYCB7Q+5pahoms=
 github.com/pion/mdns v0.0.5 h1:Q2oj/JB3NqfzY9xGZ1fPzZzK7sDSD8rZPOvcIQ10BCw=
@@ -52,7 +52,6 @@ github.com/pion/mdns v0.0.5/go.mod h1:UgssrvdD3mxpi8tMxAXbsppL3vJ4Jipw1mTCW+al01
 github.com/pion/randutil v0.1.0 h1:CFG1UdESneORglEsnimhUjf33Rwjubwj6xfiOXBa3mA=
 github.com/pion/randutil v0.1.0/go.mod h1:XcJrSMMbbMRhASFVOlj/5hQial/Y8oH/HVo7TBZq+j8=
 github.com/pion/rtcp v1.2.6/go.mod h1:52rMNPWFsjr39z9B9MhnkqhPLoeHTv1aN63o/42bWE0=
-github.com/pion/rtcp v1.2.7/go.mod h1:qVPhiCzAm4D/rxb6XzKeyZiQK69yJpbUDJSF7TgrqNo=
 github.com/pion/rtcp v1.2.8 h1:Cys8X6r0xxU65ESTmXkqr8eU1Q1Wx+lNkoZCUH4zD7E=
 github.com/pion/rtcp v1.2.8/go.mod h1:qVPhiCzAm4D/rxb6XzKeyZiQK69yJpbUDJSF7TgrqNo=
 github.com/pion/rtp v1.7.0/go.mod h1:bDb5n+BFZxXx0Ea7E5qe+klMuqiBrP+w8XSjiWtCUko=
diff --git a/peerconnection.go b/peerconnection.go
index 67f0c2a0..92db7d3b 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -134,15 +134,22 @@ func (api *API) NewPeerConnection(configuration Configuration) (*PeerConnection,
 	pc.iceConnectionState.Store(ICEConnectionStateNew)
 	pc.connectionState.Store(PeerConnectionStateNew)
 
-	if !api.settingEngine.disableMediaEngineCopy {
-		pc.api = &API{
-			settingEngine: api.settingEngine,
-			mediaEngine:   api.mediaEngine.copy(),
-			interceptor:   api.interceptor,
-		}
+	i, err := api.interceptorRegistry.Build("")
+	if err != nil {
+		return nil, err
+	}
+
+	pc.api = &API{
+		settingEngine: api.settingEngine,
+		interceptor:   i,
+	}
+
+	if api.settingEngine.disableMediaEngineCopy {
+		pc.api.mediaEngine = api.mediaEngine
+	} else {
+		pc.api.mediaEngine = api.mediaEngine.copy()
 	}
 
-	var err error
 	if err = pc.initConfiguration(configuration); err != nil {
 		return nil, err
 	}
@@ -176,7 +183,7 @@ func (api *API) NewPeerConnection(configuration Configuration) (*PeerConnection,
 		}
 	})
 
-	pc.interceptorRTCPWriter = api.interceptor.BindRTCPWriter(interceptor.RTCPWriterFunc(pc.writeRTCP))
+	pc.interceptorRTCPWriter = pc.api.interceptor.BindRTCPWriter(interceptor.RTCPWriterFunc(pc.writeRTCP))
 
 	return pc, nil
 }


Test patch:
diff --git a/interceptor_test.go b/interceptor_test.go
index 4915752c..7bca7fc1 100644
--- a/interceptor_test.go
+++ b/interceptor_test.go
@@ -33,26 +33,30 @@ func TestPeerConnection_Interceptor(t *testing.T) {
 		assert.NoError(t, m.RegisterDefaultCodecs())
 
 		ir := &interceptor.Registry{}
-		ir.Add(&mock_interceptor.Interceptor{
-			BindLocalStreamFn: func(_ *interceptor.StreamInfo, writer interceptor.RTPWriter) interceptor.RTPWriter {
-				return interceptor.RTPWriterFunc(func(header *rtp.Header, payload []byte, attributes interceptor.Attributes) (int, error) {
-					// set extension on outgoing packet
-					header.Extension = true
-					header.ExtensionProfile = 0xBEDE
-					assert.NoError(t, header.SetExtension(2, []byte("foo")))
-
-					return writer.Write(header, payload, attributes)
-				})
-			},
-			BindRemoteStreamFn: func(_ *interceptor.StreamInfo, reader interceptor.RTPReader) interceptor.RTPReader {
-				return interceptor.RTPReaderFunc(func(b []byte, a interceptor.Attributes) (int, interceptor.Attributes, error) {
-					if a == nil {
-						a = interceptor.Attributes{}
-					}
-
-					a.Set("attribute", "value")
-					return reader.Read(b, a)
-				})
+		ir.Add(&mock_interceptor.Factory{
+			NewInterceptorFn: func(_ string) (interceptor.Interceptor, error) {
+				return &mock_interceptor.Interceptor{
+					BindLocalStreamFn: func(_ *interceptor.StreamInfo, writer interceptor.RTPWriter) interceptor.RTPWriter {
+						return interceptor.RTPWriterFunc(func(header *rtp.Header, payload []byte, attributes interceptor.Attributes) (int, error) {
+							// set extension on outgoing packet
+							header.Extension = true
+							header.ExtensionProfile = 0xBEDE
+							assert.NoError(t, header.SetExtension(2, []byte("foo")))
+
+							return writer.Write(header, payload, attributes)
+						})
+					},
+					BindRemoteStreamFn: func(_ *interceptor.StreamInfo, reader interceptor.RTPReader) interceptor.RTPReader {
+						return interceptor.RTPReaderFunc(func(b []byte, a interceptor.Attributes) (int, interceptor.Attributes, error) {
+							if a == nil {
+								a = interceptor.Attributes{}
+							}
+
+							a.Set("attribute", "value")
+							return reader.Read(b, a)
+						})
+					},
+				}, nil
 			},
 		})
 
@@ -148,7 +152,9 @@ func Test_Interceptor_BindUnbind(t *testing.T) {
 		},
 	}
 	ir := &interceptor.Registry{}
-	ir.Add(mockInterceptor)
+	ir.Add(&mock_interceptor.Factory{
+		NewInterceptorFn: func(_ string) (interceptor.Interceptor, error) { return mockInterceptor, nil },
+	})
 
 	sender, receiver, err := NewAPI(WithMediaEngine(m), WithInterceptorRegistry(ir)).newPair(Configuration{})
 	assert.NoError(t, err)
@@ -209,3 +215,24 @@ func Test_Interceptor_BindUnbind(t *testing.T) {
 		t.Errorf("CloseFn is expected to be called twice, but called %d times", cnt)
 	}
 }
+
+func Test_InterceptorRegistry_Build(t *testing.T) {
+	registryBuildCount := 0
+
+	ir := &interceptor.Registry{}
+	ir.Add(&mock_interceptor.Factory{
+		NewInterceptorFn: func(_ string) (interceptor.Interceptor, error) {
+			registryBuildCount++
+			return &interceptor.NoOp{}, nil
+		},
+	})
+
+	peerConnectionA, err := NewAPI(WithInterceptorRegistry(ir)).NewPeerConnection(Configuration{})
+	assert.NoError(t, err)
+
+	peerConnectionB, err := NewAPI(WithInterceptorRegistry(ir)).NewPeerConnection(Configuration{})
+	assert.NoError(t, err)
+
+	assert.Equal(t, 2, registryBuildCount)
+	closePairNow(t, peerConnectionA, peerConnectionB)
+}