Fix BindRemoteStream StreamInfo

[3DRX]

Provide correct StreamInfo to receiver interceptors

Before this change, interceptor's BindRemoteStream gets called twice per track when RTX is used, with only the SSRC being different. Hence, the interceptor can't distinguish between RTX track and video track.

ReceiverInterceptor BindRemoteStream, info: &{ map[] 3181198436 0 0 0 0 0 [{http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01 4}] video/AV1 90000 0 level-idx=5;profile=0;tier=0 [{nack } {nack pli} {transport-cc }]}
ReceiverInterceptor BindRemoteStream, info: &{ map[] 1017514322 0 0 0 0 0 [{http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01 4}] video/AV1 90000 0 level-idx=5;profile=0;tier=0 [{nack } {nack pli} {transport-cc }]}

After this change, interceptor's BindRemoteStream gets called twice, the first time bind to video track, second time bind to RTX track, like this.

ReceiverInterceptor BindRemoteStream, info: &{ map[] 559414906 3287567208 0 45 46 0 [{http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01 4}] video/AV1 90000 0 level-idx=5;profile=0;tier=0 [{nack } {nack pli} {transport-cc }]}
ReceiverInterceptor BindRemoteStream, info: &{ map[] 3287567208 0 0 46 0 0 [{http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01 4}] video/AV1 90000 0 level-idx=5;profile=0;tier=0 [{nack } {nack pli} {transport-cc }]}

With this change, interceptor can implement internal logic to handle rtx track with video track. This will also make implementing BindRemoteStream to FEC track in the future easier.

This shouldn't break anything, since this PR only adds new data (that's originally always 0) to StreamInfo.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 85.40146% with 20 lines in your changes missing coverage. Please review.

Project coverage is 78.21%. Comparing base (4c1af4c) to head (e5a9e5a).

Files with missing lines	Patch %	Lines
rtpreceiver.go	84.25%	16 Missing and 4 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3157      +/-   ##
==========================================
+ Coverage   78.03%   78.21%   +0.18%     
==========================================
  Files          93       93              
  Lines       11769    11802      +33     
==========================================
+ Hits         9184     9231      +47     
+ Misses       2074     2062      -12     
+ Partials      511      509       -2     

Flag	Coverage Δ
go	79.99% <85.40%> (+0.19%)	:arrow_up:
wasm	63.08% <0.00%> (-0.09%)	:arrow_down:

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

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

:rocket: New features to boost your workflow:

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

[3DRX]

@JoeTurki Thanks for review! Sure, I will look into existing tests to see how to test this kind of things properly.

[3DRX]

@JoeTurki I refactored the code a bit for better testability and add test cases for video stream only and video + RTX, this is ready for review again. No rush :)

[arjunshajitech]

Hi @3DRX

In the following line from the Pion code: https://github.com/pion/webrtc/blob/b8c6f1036cd67f213906c79478fb55d61965c9d2/rtpreceiver.go#L534

Is the call to r.rtxPool.Put(b) necessary here?

[3DRX]

Is the call to r.rtxPool.Put(b) necessary here?

@arjunshajitech handleReceivedRTXPackets in this PR is exactly the same as the old anonymous function in receiveForRtx. When I'm doing the refactor I'm just reorganizing code into different functions and variables into different structs, not modifying any logic. Just read through handleReceivedRTXPackets and I think you are right, it not necessary.

[arjunshajitech]

Is the call to r.rtxPool.Put(b) necessary here?

@arjunshajitech handleReceivedRTXPackets in this PR is exactly the same as the old anonymous function in receiveForRtx. When I'm doing the refactor I'm just reorganizing code into different functions and variables into different structs, not modifying any logic. Just read through handleReceivedRTXPackets and I think you are right, it not necessary.

Just to clarify, I said that r.rtxPool.Put(b) is needed in the default case, but it's missing in the code.

[3DRX]

Thank you so much @arjunshajitech ! I hope this PR's refactor doesn't change any current behavior and logic, only improves testability so that reviewing is easier. Do you mind opening another PR to make the change?

[aalekseevx]

@3DRX, do you think that it would be better to feed RTX/FEC packets to the same stream as the media packets? These streams are not separate in BindLocalStream, so it looks a bit inconsistent from the interceptor's point of view. It's a breaking change, so it could be done using the SettingsEngine I guess. Of course it's not a blocker for this PR, which is definitely a step in the right direction.

[3DRX]

@3DRX, do you think that it would be better to feed RTX/FEC packets to the same stream as the media packets? These streams are not separate in BindLocalStream, so it looks a bit inconsistent from the interceptor's point of view. It's a breaking change, so it could be done using the SettingsEngine I guess. Of course it's not a blocker for this PR, which is definitely a step in the right direction.

Yes, it definately would be better imo. Current way of handling RTX is quite unintuitive to me, with BindRemoteStream being called twice. @arjunshajitech @JoeTurki @Sean-Der what do you guys think? To me, It's fine if we introduce a breaking change here, since we don't have 100% correct reciver side RTX and FEC implemented yet, so people probably not using them anyway.

[aalekseevx]

@3DRX, I'm concerned not only about breaking RTX implementation in users' interceptors, but also about breaking the existing interceptors' logic which expects packets of only one SSRC inside the stream. We even have this kind of logic in pion's interceptors, which would require fixes. Some examples: Send side. Receive side.

[3DRX]

@JoeTurki @aalekseevx have a good point here, I guess we can also mark this as a reasonable breaking change in future major version? This won’t block this PR though, it’s ready to be merged on my side. Thanks guys :)

[JoeTurki]

Maybe it's time to start making a v5 milestone? and aim for jan 2026?

[3DRX]

Maybe it's time to start making a v5 milestone? and aim for jan 2026?

@JoeTurki So sorry I missed this, sure we can make the milestone, there's a couple of things I think worth a breaking change.

[JoeTurki]

@jech Can you please look at this? Will it break any behavior for Galene?

[Sean-Der]

@3DRX I would love to hear all your ideas on how to make Pion better! In the past I used the Wiki, but w/e you prefer works for me https://github.com/pion/webrtc/wiki/Big-Ideas

My hope/goal is to try and squeeze peoples ideas in without an API break :)

[3DRX]

@Sean-Der Hi, I've updated https://github.com/pion/webrtc/wiki/Big-Ideas, please check it out, thanks :)

[jech]

The change is a net positive, it only adds more data to previously unused fields.

On the other hand, I don't think it solves any of the fundamental issues we have, which make the RTX/FEC tracks unusable in practice.

It's a difficult problem. Pion aims at being suitable both for high-level applications (streaming a video as simply as possible), and at low-level software (high-performance SFUs). The difficultly lies in designing an API that is transparent to high-level software (create a track and get RTX/FEC handling for free) and low-level software (full control on RTX/FEC, let the SFU decide whether to forward FEC or terminate FEC and regenerate it).

I don't currently see a good solution. I'd like people to join my suffering by having a look at

• https://github.com/pion/webrtc/issues/1675
• https://github.com/pion/webrtc/pull/2914
• https://github.com/pion/webrtc/issues/2994
• https://github.com/pion/webrtc/issues/2996

as well as the following patch series:

• https://github.com/pion/interceptor/pull/297
• https://github.com/pion/webrtc/pull/3002
• https://github.com/pion/interceptor/pull/298
• https://github.com/pion/webrtc/pull/3003