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]

Codecov Report

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

Project coverage is 78.64%. Comparing base (9b1ca73) to head (926f532).

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3157      +/-   ##
==========================================
- Coverage   78.67%   78.64%   -0.04%     
==========================================
  Files          91       91              
  Lines       11496    11529      +33     
==========================================
+ Hits         9045     9067      +22     
- Misses       1958     1966       +8     
- Partials      493      496       +3     

Flag	Coverage Δ
go	80.51% <85.40%> (-0.05%)	⬇️
wasm	63.38% <0.00%> (-0.09%)	⬇️

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.

[JoeTurki]

I'm outside today, looking at the code from my phone, this is a step in the right direction to fix that part of pion, thank you so much.
Just one thing, can we add a test because it's easy to break this change?

[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:

webrtc/rtpreceiver.go

Line 534 in b8c6f10

default:

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?