pion / sdp

A Go implementation of the SDP
https://pion.ly/
MIT License
148 stars 56 forks source link

Add support for rtcp-fb:* (Chrome PSA) #152

Closed thebongy closed 6 months ago

thebongy commented 1 year ago

Description

According to Chrome PSA: https://groups.google.com/g/discuss-webrtc/c/Y_h2B-NOzW0 rtcp-fb:* might start getting sent by chrome in M112. For more information, refer to "wildcard" defined in (https://www.rfc-editor.org/rfc/rfc4585.html#section-4.2)

This was rolled back in M112 as it broke a few projects, but may land again in M114 according to maintainers.

Also, according to https://webrtc.googlesource.com/src.git/+/815522782a92e168b80edc760b2e53e4d0e4ea0d%5E!/#F0 chrome may plan on sending both rtcp-fb:* and rtcp-fb:<int> variants for some time to allow migration in downstream projects. This change correctly parses the wildcard case to add the feedback to all payload types found in the SDP, to maintain compatibility with pion project.

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 80.64516% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 64.96%. Comparing base (84cf380) to head (b7d0ec1).

Files Patch % Lines
util.go 80.64% 5 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #152 +/- ## ========================================== + Coverage 64.67% 64.96% +0.29% ========================================== Files 12 12 Lines 1571 1587 +16 ========================================== + Hits 1016 1031 +15 - Misses 454 455 +1 Partials 101 101 ``` | [Flag](https://app.codecov.io/gh/pion/sdp/pull/152/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | | |---|---|---| | [go](https://app.codecov.io/gh/pion/sdp/pull/152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `64.96% <80.64%> (+0.29%)` | :arrow_up: | | [wasm](https://app.codecov.io/gh/pion/sdp/pull/152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `64.96% <80.64%> (+0.29%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stv0g commented 1 year ago

Lets a wait a bit more until its merged into Chrome. Currently, it has not made it into 112:

https://groups.google.com/g/discuss-webrtc/c/Y_h2B-NOzW0/m/26SqldfuAgAJ

artofey commented 1 year ago

And why wait for Chrome if it is already described in the RFC?

Sean-Der commented 6 months ago

Sorry I didn't merge this sooner! LGTM merging now, will also update pion/webrtc to respect these values

Sean-Der commented 6 months ago

Amazing work @thebongy

I am sorry this took so long.