Skip to content

trackDetailsFromSDP should also black list FEC ssrc #3109

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

Closed
3DRX opened this issue Apr 25, 2025 · 3 comments · Fixed by #3125
Closed

trackDetailsFromSDP should also black list FEC ssrc #3109

3DRX opened this issue Apr 25, 2025 · 3 comments · Fixed by #3125

Comments

@3DRX
Copy link
Member

3DRX commented Apr 25, 2025

The Problem

Unified plan SDP can contain FEC and RTX tracks in the same m= with video track, https://datatracker.ietf.org/doc/html/draft-roach-mmusic-unified-plan-00#section-4.5 .
But currently, pion's implementation thinks it's plan B sdp if FEC track is included.

The Cause

It seems like the problem is in function trackDetailsFromSDP of sdp.go, it only black lists rtx ssrc, but not fec ssrc.

The Fix

We should also black list fec ssrc in trackDetailsFromSDP. And also test descriptionIsPlanB against examples in https://datatracker.ietf.org/doc/html/draft-roach-mmusic-unified-plan-00#section-4 .

@Sean-Der
Copy link
Member

Should we just remove Plan-B support @3DRX?

I hope no one is still using it!

@3DRX
Copy link
Member Author

3DRX commented Apr 26, 2025

@Sean-Der I think we should, since it’s not actually in WebRTC standard and google drops the plan b support a while ago.

@3DRX
Copy link
Member Author

3DRX commented May 20, 2025

@Sean-Der I think with this pr, we can delay the decision of dropping plan b support. Since the issue here is any sdp with a FEC ssrc included will be treated as plan b by pion and caused an error after SetRemoteDescription, which shouldn't happen. Fully supporting receiving fec in pion is way harder (I'm trying, but it's harder than I initially thought), but with this PR pion doesn't return this wrong error message anymore.

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.

2 participants