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

Avoid including SSRC in single media offers #2395

Merged
merged 1 commit into from
Apr 23, 2023

Conversation

streamer45
Copy link
Contributor

Description

Sending simulcast tracks from a pion sender to a pion receiver is failing unless there are other medias added:

Incoming unhandled RTP ssrc(xxxx), OnTrack will not be fired. single media section has an explicit SSRC

I admit I am not well versed in SDP semantics so please let me know if there's more to this that we should consider. The proposed changes are incredibly dumb as we are simply checking if there are multiple media sections and if not we avoid adding the ssrc attribute.

Reference issue

This is strictly related to #2299 which I guess is also a working solution although I am thinking that if we are being strict on the receiving side we should probably do the same on the sending side.

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.12 ⚠️

Comparison is base (6114c27) 77.74% compared to head (ab21ff6) 77.62%.

❗ Current head ab21ff6 differs from pull request most recent head c2e2b51. Consider uploading reports for the commit c2e2b51 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2395      +/-   ##
==========================================
- Coverage   77.74%   77.62%   -0.12%     
==========================================
  Files          87       87              
  Lines        9311     9297      -14     
==========================================
- Hits         7239     7217      -22     
- Misses       1646     1647       +1     
- Partials      426      433       +7     
Flag Coverage Δ
go 79.40% <100.00%> (-0.13%) ⬇️
wasm 70.15% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdp.go 88.07% <100.00%> (+0.37%) ⬆️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Sean-Der
Copy link
Member

Sean-Der commented Feb 9, 2023

Nice catch! Thanks for the PR @streamer45 I believe something else is broken here. For Simulcast we can't put a SSRC in the MediaSection since we will have multiple.

Are you able to write a test of Pion <-> Pion Simulcast? We might even have one already let me look for it.

@streamer45
Copy link
Contributor Author

@Sean-Der I believe we have:

func TestPeerConnection_Simulcast(t *testing.T) {

Maybe I could build on that to cover the changes in here. Let me know.

@Sean-Der
Copy link
Member

Sean-Der commented Feb 9, 2023

@streamer45 that would be fantastic! I don’t fully understand what is broken, but if you have a failing test would help a lot.

@streamer45
Copy link
Contributor Author

@Sean-Der Added a pion <-> pion test as well which would have failed before these changes, specifically in:

return false, errPeerConnSingleMediaSectionHasExplicitSSRC

Let me know if that helps.

@streamer45
Copy link
Contributor Author

@Sean-Der Let me know if there's anything else needed to move this forward. Thanks :)

@Sean-Der
Copy link
Member

Shit, sorry I missed this @streamer45!

I have some time off work and will be able to work on this now

@Sean-Der
Copy link
Member

@streamer45 Nope nothing is needed. I just got caught up in day job. With this repro I have everything I need.

I don't know exactly what is wrong/the fix. I should have a full understanding tonight and understand how to move forward.

Everytime we receieve a new SSRC we probe it and try to determine the
proper way to handle it. In most cases a Track explicitly declares a
SSRC and a OnTrack is fired. In two cases we don't know the SSRC
ahead of time
* Undeclared SSRC in a single media section (pion#880)
* Simulcast

The Undeclared SSRC processing code would run before Simulcast.
If a Simulcast Offer/Answer only contained one Media Section we
would never fire the OnTrack. We would assume it was a failed
Undeclared SSRC processing. This commit fixes the behavior.
@Sean-Der
Copy link
Member

Sean-Der commented Apr 23, 2023

Thank you so much for this PR @streamer45! That was a gnarly issue, lots of niche parts interacting :) This is the commit I am landing.

Author: streamer45 <cstcld91@gmail.com>
Date:   Tue Jan 31 17:25:12 2023 -0600

    Handle Simulcast Offer with one Media Section

    Everytime we receieve a new SSRC we probe it and try to determine the
    proper way to handle it. In most cases a Track explicitly declares a
    SSRC and a OnTrack is fired. In two cases we don't know the SSRC
    ahead of time
    * Undeclared SSRC in a single media section (pion/webrtc#880)
    * Simulcast

    The Undeclared SSRC processing code would run before Simulcast.
    If a Simulcast Offer/Answer only contained one Media Sec

I am sorry I have been so slow on Pion lately. I am going to be working on it more. I would love to have you involved more also. If you have any feedback on the project (and how I can make it better I would love to hear)

I am going to drive down our PRs/Issues and cut a new release.

@Sean-Der Sean-Der merged commit d08b3dc into pion:master Apr 23, 2023
16 of 17 checks passed
@streamer45
Copy link
Contributor Author

Thanks Sean, appreciate the explanation there and the extra work on this 🙌

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.

None yet

2 participants