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 negotiates transport-cc in the answer even with an empty interceptor registry #2944

Open
jech opened this issue Oct 30, 2024 · 3 comments

Comments

@jech
Copy link
Member

jech commented Oct 30, 2024

In #2943, I reported that Pion spuriously negotiates transport-cc. @kcaffrey identified the issue in v4, indicating that I should include an empty interceptor registry.

It turns out that if I do that, then the behaviour in Pion v3 and v4 is identical:

  • transport-cc is not negotiated in the offer;
  • however, if the peer attempts to negotiate transport-cc in the offer, then Pion (both v3 and v4) negotiates transport-cc in the answer.

Full test here: https://go.dev/play/p/i4vhBRmtpxb

@kcaffrey
Copy link

I dug in a bit here, and I'm not exactly an expert so someone like @Sean-Der may correct something I get wrong, but it does indeed seem to be the case that if Pion is the answerer side that it copies any rtcp-fb entries that is in the offer. When calling pc.SetRemoteDescription(desc), internally this calls (*MediaEngine).updateFromRemoteDescription(). This finds codecs in the remote description that match what the media engine supports and copies those into the answer. The matching logic matches on MIME type and the Fmtp line, so it doesn't consider supported RTCP feedback types. The codec that we copy, however, does include RTCP feedback types.

This does seem problematic, but it may not actually break any spec. Like I said, I'm not an expert, but it seems like the RFC that governs this behavior is RFC 4585, and it says:

The "rtcp-fb" attribute SHOULD be used to indicate which RTCP FB
messages MAY be used in this media session for the indicated payload
type.

That is, while we SHOULD only include "rtcp-fb" for what's supported, it doesn't seem to technically violate the RFC to include other feedback.

That said, it's my opinion that this should be changed to match your expectations.

@kcaffrey
Copy link

For comparison, it appears that libwebrtc intersects the rtcp-fb entries.

@Sean-Der
Copy link
Member

👍 lets match that behavior

I will put a PR that implements intersection soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants