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.

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

Pion spuriously negotiates transport-cc #2943

Open
jech opened this issue Oct 29, 2024 · 5 comments · May be fixed by #2949
Open

Pion spuriously negotiates transport-cc #2943

jech opened this issue Oct 29, 2024 · 5 comments · May be fixed by #2949

Comments

@jech
Copy link
Member

jech commented Oct 29, 2024

In Galene, I'm explicitly constructing a MediaEngine with just the right parameters. However, Pion is still negotiating transport-cc, even though I didn't enable it in the MediaEngine. More precisely:

  • in v3, transport-cc is negotiated in answers but not in offers [edit: this is not split into 2944];
  • in v4, transport-cc is negotiated in both offers and answers.

Full test at https://go.dev/play/p/IBH55knGgkb

@jech jech changed the title v4 spuriously negotiates transport-cc Pion spuriously negotiates transport-cc Oct 29, 2024
@kcaffrey
Copy link

I think this might be the culprit:

webrtc/api.go

Line 57 in e53cbc4

err := RegisterDefaultInterceptors(a.mediaEngine, a.interceptorRegistry)

If you don't provide an interceptor registry, the default interceptors are used (and transport-cc is included in the default interceptors). It appears like this may have been intentional: 21c5a71

One fix would be to supply an empty interceptor registry if you don't want any interceptors. It is documented on NewAPI(), so I suspect it probably was an intentional behavior change in v4, but @Sean-Der can comment on that.

@Sean-Der
Copy link
Member

Thank you so much @kcaffrey that's some great debugging. Completely slipped my mind.

Users complained about the inverse. When they created a SettingEngine they were frustrated that interceptor/mediaengine was empty. So the behavior was changed.

I documented it here but I think it buries the impact. What should I say/would be better do you think @jech ?

@jech
Copy link
Member Author

jech commented Oct 29, 2024

That would explain the behaviour with v4. But what about the behaviour with v3, where transport-cc is negotiated in the answer but not in the offer? [Edit: this part was split into #2944.]

If you don't provide an interceptor registry, the default interceptors are used (and transport-cc is included in the default interceptors).

[...]

What should I say/would be better do you think @jech ?

My understanding was that simple applications (that want a default set of interceptors) should be using webrtc.NewPeerConnection, but that applications that use api.NewPeerConnection get exactly what they ask for (and should call RegisterDefaultWhatevers if they want reasonable defaults). That's a behaviour that's easy to understand and easy to explain.

If the other behaviour is desired, I can live with it, but it needs to be documented here: https://pkg.go.dev/github.com/pion/webrtc/v4#API.NewPeerConnection

@jech
Copy link
Member Author

jech commented Oct 30, 2024

I've split the second part of this into #2944, since it appears there is another related bug.

@jech
Copy link
Member Author

jech commented Nov 6, 2024

Thinking about it some more, I don't dislike the new behaviour. Having webrtc.NewPeerConnection have different semantics from api.NewPeerConnection is uselessly confusing, and applications that want the old behaviour can easily add an empty InterceptorRegistry.

@jech jech linked a pull request Nov 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants