Fix missing RTX codec when using SetCodecPreferences

[stephanrotolante]

Description

When you specify the codecs that you want to use, we need to add the RTX codec related to the specified codecs

Reference issue

Fixes #2996

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.60%. Comparing base (e4ff415) to head (2d892b5).
⚠️ Report is 255 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3051      +/-   ##
==========================================
+ Coverage   78.38%   78.60%   +0.22%     
==========================================
  Files          91       91              
  Lines       11300    11313      +13     
==========================================
+ Hits         8857     8893      +36     
+ Misses       1950     1932      -18     
+ Partials      493      488       -5     

Flag	Coverage Δ
go	80.50% <100.00%> (+0.24%)	⬆️
wasm	63.61% <0.00%> (-0.11%)	⬇️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[stephanrotolante]

broken test cases.... I am looking into that

[JoTurk]

We match codecs with different rate or channels now, I know RTX is never used for audio, but can we use the fmtp helpers that Alessandro's made in his PR instead, for the sake of behavioral consistency (Like we use strings.EqualFold there to match the MimeType), so one behavior and less code to test?

webrtc/internal/fmtp/fmtp.go

Line 169 in e4ff415

func (g *genericFMTP) Match(b FMTP) bool {

[stephanrotolante]

sure, let me see what I can do

[stephanrotolante]

now that I reviewed the code, I wonder if I should just use codecParametersFuzzySearch

[JoTurk]

Yeah if you can use codecParametersFuzzySearch that would be better!

[JoTurk]

Thank you

[itzmanish]

Hi, Please correct me if my understanding of the issue is wrong.

This PR fixes the scenario where someone sets CodecPreferences with one codec, and it disables the RTX for that codec. Right?

According to MDN, the parameter for setCodecPreferences is a list of codecs. Pion's API is the same as well. So my question is, why should pion try to find the RTX codec from mediaengine instead of the user providing the list of codecs with the RTX codec in setCodecPreferences?

[stephanrotolante]

@itzmanish that is a great question and I thought about that as well while making this PR

If we want the RTX codec when using setCodecPreferences, why not just specify it?

why should pion try to find the RTX codec from mediaengine instead of the user providing the list of codecs with the RTX codec > in setCodecPreferences?

I guess you could say this is a pion specific feature, that will handle this for you.....

Maybe I should ask the author of the issue why they cannot just pass in the RTX codec 🤔

[JoTurk]

@jech what is your thoughts on @itzmanish comment?

[jech]

Sure, no problem. However, since that would be a divergence from the JavaScript API, it would need to be clearly documented.

If you choose to do that, please add a test to make sure that a codec with an associated RTX track can be specified, since I'm afraid that's code that might rot and since things will still sort of work if the RTX track is incorrect, we might not notice that it's broken.

[stephanrotolante]

@joeturki @itzmanish

What do you think? Should we stay aligned with the javascript api or do our own thing??

I am thinking we stay aligned

[jech]

Should we stay aligned with the javascript api or do our own thing?

The JavaScript API is fairly high level. Pion is useful for low-level software, such as SFUs, so it might make sense to give more control to the programmer. But perhaps it's not worth the extra complexity in this particular case.

In other words, I have no clue.

[itzmanish]

Pion is useful for low-level software, such as SFUs, so it might make sense to give more control to the programmer. But perhaps it's not worth the extra complexity in this particular case.

This ^

In my view, the reward for introducing extra complexity and logic is unfulfilling. The more pion/webrtc operates behind the scene logic, the more unpredictable the API will be, and it will be more challenging for users using the library.

[JoTurk]

I too think we should stay aligned!

[jech]

It's a little bit more complicated than that.

When using SetCodecPreferences in Pion, we set the ptype, and in particular we can set a ptype that is not defined in the mediaengine. If we do that, there is no way for Pion to choose the right ptype for the RTX track.

This is not the case in the JavaScript case, where we do not set the ptype. See here: https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpTransceiver/setCodecPreferences

So the API is already not aligned with the JavaScript case, and not being explicit will cause the API to fail for mysterious reasons.

[stephanrotolante]

@Sean-Der Do you have any suggestions?

[stephanrotolante]

@jech

let peerConnection = new RTCPeerConnection();

  const videoCodecs = RTCRtpSender.getCapabilities("video").codecs;

  const transceiver = peerConnection.addTransceiver('video', { direction: "recvonly" });

  const codecs = RTCRtpSender.getCapabilities("video").codecs;

  const preferredCodecs = codecs.filter(c => c.mimeType === "video/VP8");

  transceiver.setCodecPreferences(preferredCodecs);

  peerConnection.createOffer().then(offer => {
    console.log(offer)
    return peerConnection.setLocalDescription(offer);
  });

v=0
o=- 219046925391310381 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0
a=extmap-allow-mixed
a=msid-semantic: WMS
m=video 9 UDP/TLS/RTP/SAVPF 96
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:uvYX
a=ice-pwd:Jm/Z2PV2vCIuKg+XG7owMHj9
a=ice-options:trickle
a=fingerprint:sha-256 A5:BA:59:DE:81:84:71:C9:D9:CE:1E:EE:5E:7A:B9:8E:9F:A3:D5:CD:E3:85:EF:DA:D8:94:A6:2C:8B:99:ED:C4
a=setup:actpass
a=mid:0
a=extmap:1 urn:ietf:params:rtp-hdrext:toffset
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=extmap:3 urn:3gpp:video-orientation
a=extmap:4 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=extmap:5 http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
a=extmap:6 http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
a=extmap:7 http://www.webrtc.org/experiments/rtp-hdrext/video-timing
a=extmap:8 http://www.webrtc.org/experiments/rtp-hdrext/color-space
a=extmap:9 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:10 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
a=extmap:11 urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id
a=recvonly
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:96 VP8/90000
a=rtcp-fb:96 goog-remb
a=rtcp-fb:96 transport-cc
a=rtcp-fb:96 ccm fir
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli