Skip to content

Commit

Permalink
Fix MediaEngine Copy
Browse files Browse the repository at this point in the history
Copy the entire API. Since the MediaEngine is a pointer that would
destroy the MediaEngine that is used by other PeerConnections

Relates to #1662
  • Loading branch information
Sean-Der committed Feb 10, 2021
1 parent 5a6f532 commit 8902641
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 7 deletions.
14 changes: 9 additions & 5 deletions peerconnection.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,6 @@ func NewPeerConnection(configuration Configuration) (*PeerConnection, error) {

// NewPeerConnection creates a new PeerConnection with the provided configuration against the received API object
func (api *API) NewPeerConnection(configuration Configuration) (*PeerConnection, error) {
if !api.settingEngine.disableMediaEngineCopy {
api.mediaEngine = api.mediaEngine.copy()
}

// https://w3c.github.io/webrtc-pc/#constructor (Step #2)
// Some variables defined explicitly despite their implicit zero values to
// allow better readability to understand what is happening.
Expand Down Expand Up @@ -137,7 +133,13 @@ func (api *API) NewPeerConnection(configuration Configuration) (*PeerConnection,
log: api.settingEngine.LoggerFactory.NewLogger("pc"),
}

pc.interceptorRTCPWriter = api.interceptor.BindRTCPWriter(interceptor.RTCPWriterFunc(pc.writeRTCP))
if !api.settingEngine.disableMediaEngineCopy {
pc.api = &API{
settingEngine: api.settingEngine,
mediaEngine: api.mediaEngine.copy(),
interceptor: api.interceptor,
}
}

var err error
if err = pc.initConfiguration(configuration); err != nil {
Expand Down Expand Up @@ -173,6 +175,8 @@ func (api *API) NewPeerConnection(configuration Configuration) (*PeerConnection,
}
})

pc.interceptorRTCPWriter = api.interceptor.BindRTCPWriter(interceptor.RTCPWriterFunc(pc.writeRTCP))

return pc, nil
}

Expand Down
38 changes: 36 additions & 2 deletions settingengine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,19 +145,40 @@ func TestSettingEngine_SetDisableMediaEngineCopy(t *testing.T) {
m := &MediaEngine{}
assert.NoError(t, m.RegisterDefaultCodecs())

offerer, answerer, err := NewAPI(WithMediaEngine(m)).newPair(Configuration{})
api := NewAPI(WithMediaEngine(m))

offerer, answerer, err := api.newPair(Configuration{})
assert.NoError(t, err)

_, err = offerer.AddTransceiverFromKind(RTPCodecTypeVideo)
assert.NoError(t, err)

assert.NoError(t, signalPair(offerer, answerer))

// Assert that the MediaEngine the user created isn't modified
assert.False(t, m.negotiatedVideo)
assert.Empty(t, m.negotiatedVideoCodecs)

// Assert that the internal MediaEngine is modified
assert.True(t, offerer.api.mediaEngine.negotiatedVideo)
assert.NotEmpty(t, offerer.api.mediaEngine.negotiatedVideoCodecs)

assert.NoError(t, offerer.Close())
assert.NoError(t, answerer.Close())

newOfferer, newAnswerer, err := api.newPair(Configuration{})
assert.NoError(t, err)

// Assert that the first internal MediaEngine hasn't been cleared
assert.True(t, offerer.api.mediaEngine.negotiatedVideo)
assert.NotEmpty(t, offerer.api.mediaEngine.negotiatedVideoCodecs)

// Assert that the new internal MediaEngine isn't modified
assert.False(t, newOfferer.api.mediaEngine.negotiatedVideo)
assert.Empty(t, newAnswerer.api.mediaEngine.negotiatedVideoCodecs)

assert.NoError(t, newOfferer.Close())
assert.NoError(t, newAnswerer.Close())
})

t.Run("No Copy", func(t *testing.T) {
Expand All @@ -167,18 +188,31 @@ func TestSettingEngine_SetDisableMediaEngineCopy(t *testing.T) {
s := SettingEngine{}
s.DisableMediaEngineCopy(true)

offerer, answerer, err := NewAPI(WithMediaEngine(m), WithSettingEngine(s)).newPair(Configuration{})
api := NewAPI(WithMediaEngine(m), WithSettingEngine(s))

offerer, answerer, err := api.newPair(Configuration{})
assert.NoError(t, err)

_, err = offerer.AddTransceiverFromKind(RTPCodecTypeVideo)
assert.NoError(t, err)

assert.NoError(t, signalPair(offerer, answerer))

// Assert that the user MediaEngine was modified, so no copy happened
assert.True(t, m.negotiatedVideo)
assert.NotEmpty(t, m.negotiatedVideoCodecs)

assert.NoError(t, offerer.Close())
assert.NoError(t, answerer.Close())

offerer, answerer, err = api.newPair(Configuration{})
assert.NoError(t, err)

// Assert that the new internal MediaEngine was modified, so no copy happened
assert.True(t, offerer.api.mediaEngine.negotiatedVideo)
assert.NotEmpty(t, offerer.api.mediaEngine.negotiatedVideoCodecs)

assert.NoError(t, offerer.Close())
assert.NoError(t, answerer.Close())
})
}

0 comments on commit 8902641

Please sign in to comment.