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

Migrate session-level attributes to media-level #1194

Merged
merged 1 commit into from
May 28, 2024
Merged

Migrate session-level attributes to media-level #1194

merged 1 commit into from
May 28, 2024

Conversation

wcpfeffer
Copy link
Contributor

Moves session-level attributes for a=setup, a=ice-ufrag, a=ice-pwd, a=ice-options and a=fingerprint to the media-level in Description::generateSdp(...). The goal here is to have better general compatibility as RFC 8829 recommends using the media-level attributes even if the information is identical.

Another goal of this change was to not introduce breaking API changes, and the easiest way to accomplish this was to print out the information right after the entry SDP on the Description level. The "downside" of this approach is that it will appear at the end of the media entry, but I don't believe this will impact parsing as long as it is grouped with the media.

Let me know what you think, happy to make any changes!

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

src/description.cpp Outdated Show resolved Hide resolved
src/description.cpp Outdated Show resolved Hide resolved
…e-pwd` to the media-level in `Description::generateSdp(...)`. The goal here is to have better general compatibility as RFC 8829 recommends using the media-level attributes even if the information is identical.
@wcpfeffer
Copy link
Contributor Author

Thanks for the feedback!

I removed the changes to ice-options and fingerprint in my commit. Seems reasonable to follow the lead of Firefox here as it does feel a bit cleaner.

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, thanks!

@paullouisageneau paullouisageneau merged commit 7b45b57 into paullouisageneau:master May 28, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants