Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Guard against MediaEngine being shared between PeerConnections #1662

Closed
Sean-Der opened this issue Feb 4, 2021 · 2 comments · Fixed by #1673
Closed

Guard against MediaEngine being shared between PeerConnections #1662

Sean-Der opened this issue Feb 4, 2021 · 2 comments · Fixed by #1673

Comments

@Sean-Der
Copy link
Member

Sean-Der commented Feb 4, 2021

A MediaEngine should only be used by one PeerConnection. We need to find a way to assert that it has never been used before.
@Sean-Der
Copy link
Member Author

Sean-Der commented Feb 4, 2021 •
edited
Loading

After a little investigation it turns that we can't do this. It would be a breaking change for lots of applications. What we can do is copy. The user doesn't need a reference to the MediaEngine. Querying for Codec/HeaderExtension happens on the Transceiver level.

So a PR would look like this.

  • Add a copy and equal function to MediaEngine with a test
  • In NewPeerConnection do a copy api.mediaEngine = api.mediaEngine.copy()
  • Write a test that if you create a PeerConnection and then modify the MediaEngine that was used that DO NOT equal

@Sean-Der
Copy link
Member Author

Sean-Der commented Feb 4, 2021 •
edited
Loading

We also have the issue of interceptor's being shared. That is possible sharp edge, but it is unavoidable because we MAY want them to be shared.

Multiple PeerConnections will want to use the same congestion controller etc...

Maybe the Interceptor needs to guard against this in an init function?

@Sean-Der Sean-Der mentioned this issue Feb 7, 2021
Closed
Sean-Der added a commit that referenced this issue Feb 10, 2021
@Sean-Der
Disable MediaEngine Copy by Default
c4944cc 
If an API is shared between PeerConnections they would use the same
MediaEngine. A MediaEngine contains negotiated PayloadTypes so if the
PeerConnections were answering you would end up in invalid states.

Add DisableMediaEngineCopy to SettingEngine in case user needs old
behavior.

Resolves #1662
@Sean-Der Sean-Der mentioned this issue Feb 10, 2021
Merged
Sean-Der added a commit that referenced this issue Feb 10, 2021
@Sean-Der
Disable MediaEngine Copy by Default
c8b7aa3 
If an API is shared between PeerConnections they would use the same
MediaEngine. A MediaEngine contains negotiated PayloadTypes so if the
PeerConnections were answering you would end up in invalid states.

Add DisableMediaEngineCopy to SettingEngine in case user needs old
behavior.

Resolves #1662
Sean-Der added a commit that referenced this issue Feb 10, 2021
@Sean-Der
Improve MediaEngine Copy tests
43e10c9 
Assert that the MediaEngine used by the PeerConnection is in the state
we expect

Relates to #1662
@Sean-Der Sean-Der mentioned this issue Feb 10, 2021
Merged
Sean-Der added a commit that referenced this issue Feb 10, 2021
@Sean-Der
Fix MediaEngine Copy
1916531 
Copy the entire API. Since the MediaEngine is a pointer that would
destroy the MediaEngine that is used by other PeerConnections

Relates to #1662
Sean-Der added a commit that referenced this issue Feb 10, 2021
@Sean-Der
Fix MediaEngine Copy
8902641 
Copy the entire API. Since the MediaEngine is a pointer that would
destroy the MediaEngine that is used by other PeerConnections

Relates to #1662
@mission-liao mission-liao mentioned this issue Feb 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Disable MediaEngine Copy by Default pion/webrtc
1 participant
@Sean-Der