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

Properly handle non-media probes #2816

Merged
merged 1 commit into from
Jul 21, 2024
Merged

Properly handle non-media probes #2816

merged 1 commit into from
Jul 21, 2024

Conversation

Sean-Der
Copy link
Member

libwebrtc has started sending media probes on an unannounced SSRC(0). This adds a special case to accept SSRC 0 and Read the RTP packets. This causes the TWCC reports to properly be generated.

Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.03%. Comparing base (17d3e97) to head (ca717ff).

Files Patch % Lines
peerconnection.go 71.42% 4 Missing and 2 partials ⚠️
rtpreceiver.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2816      +/-   ##
==========================================
- Coverage   79.14%   79.03%   -0.11%     
==========================================
  Files          89       89              
  Lines        8270     8290      +20     
==========================================
+ Hits         6545     6552       +7     
- Misses       1257     1265       +8     
- Partials      468      473       +5     
Flag Coverage Δ
go 80.64% <69.23%> (-0.13%) ⬇️
wasm 64.98% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adriancable
Copy link
Contributor

Yay! For more info on when this can happen, it requires RTX to be enabled and it requires SetAllowProbeWithoutMediaPacket to be enabled (modules/pacing/bitrate_prober.cc in libwebrtc). In this case BWE probes get sent as soon as the ICE connection is established, before tracks start. The probes have SSRC 0.

This change was made recently in the Chromium repo, I am not sure if it is in production yet (or just in canary) but if it isn't happening now, it soon will so this PR is super important. Right now Pion ignores this 'probe track' which means TWCC/RTCP RRs do not get sent for it. This makes the sender think there is 100% packet loss on the probes, which causes it to adjust sender bandwidth to hit the defined bitrate floor - not very nice!

Copy link
Member

@cnderrauber cnderrauber left a comment

Choose a reason for hiding this comment

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

Looks good!

@Sean-Der Sean-Der force-pushed the non-media-probes branch 2 times, most recently from 3a6298f to 2cd7263 Compare July 20, 2024 01:54
libwebrtc has started sending media probes on an unannounced SSRC(0).
Currently Pion will ignore this as the SSRC hasn't been declared
explicitly and no RID/MID RTP Headers.

This adds a special case to accept SSRC 0 and Read the RTP packets. This
allows the TWCC reports to properly be generated.
Copy link
Contributor

@boks1971 boks1971 left a comment

Choose a reason for hiding this comment

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

lgtm!

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

Successfully merging this pull request may close these issues.

4 participants