Fix receiveForRtx usage for simulcast

[aalekseevx]

Hello! We have encountered some issues with simulcast after latest upgrade. All layers, except for one, were shutting down very quickly after the connection was established. After some investigation, we found that this issue is related to this specific commit. This change does not seem to work correctly when receiveForRtx method is called to receive a new simulcast layer. Therefore, this PR is intended to restore the previous behavior in order to ensure that simulcast continues to function properly.

[codecov]

Codecov Report

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

Project coverage is 79.91%. Comparing base (2c34a45) to head (ebb9331).
Report is 1 commits behind head on master.

Files	Patch %	Lines
peerconnection.go	40.00%	1 Missing and 2 partials ⚠️
rtpreceiver.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2751      +/-   ##
==========================================
+ Coverage   77.83%   79.91%   +2.07%     
==========================================
  Files          87       76      -11     
  Lines        8202     7398     -804     
==========================================
- Hits         6384     5912     -472     
+ Misses       1345     1061     -284     
+ Partials      473      425      -48     

Flag	Coverage Δ
go	79.91% <66.66%> (+0.56%)	⬆️
wasm	?

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.

[Sean-Der]

That’s not good, thank you @aalekseevx

Is it possible to get a failing test? If you can fix the lint would love to merge right now!

[aalekseevx]

Sure, will think of the failing test and fix the linter

[Sean-Der]

@aalekseevx If you don't have the time I am happy to fix this tonight! After work I have some time to do Pion stuff :)

[aalekseevx]

@Sean-Der, thank you! I'll fix it, just need some time ☺️

[aalekseevx]

I couldn't write a simple test to reproduce it because we need to send simulcast RTX, which is not yet supported as far as I understand. :(

It seems that the root of the problem is that SSRCs are not parsed correctly for simulcast, and HasRTX() returns false for such streams. Therefore, readRTX does not work, but receiveForRtx does work, resulting in a deadlock.

Logged RTPDecodingParameters for simulcast:

{RTPCodingParameters:{RID:hi SSRC:0 PayloadType:0 RTX:{SSRC:0}}}
{RTPCodingParameters:{RID:med SSRC:0 PayloadType:0 RTX:{SSRC:0}}}
{RTPCodingParameters:{RID:low SSRC:0 PayloadType:0 RTX:{SSRC:0}}}

Blocked goroutine:

1 @ 0x291732e 0x29283a5 0x322d570 0x294b0c1
#	0x322d56f	github.com/pion/webrtc/v3.(*RTPReceiver).receiveForRtx.func1+0x56f	/-S/vendor/github.com/pion/webrtc/v3/rtpreceiver.go:486

I think the best way to fix it is to make simulcast RTX work via the newly created readRTX. However, it may be worth returning to the old behavior for now to fix what's broken.

[aalekseevx]

@Sean-Der, updated!

[Sean-Der]

@aalekseevx Revert sounds good to me! Want to use this PR for it?

I will work with original author on relanding with tests

[Sean-Der]

Want to use this PR and just do revert commits?

[aalekseevx]

My commit is literally a revert, but only for simulcast. It fixes the issue for me. Do we need anything else?

[Sean-Der]

@aalekseevx Could we revert the commit in whole?

I would do just a normal git revert on the RTX changes. We can revisit them considering all use cases.

[aalekseevx]

No problem

[aalekseevx]

@Sean-Der, do I need to open a separate PR to v3?

[Sean-Der]

Nope! I think cherry-pick is fine when this lands.

I didn’t realize this would break public API! I need to be more careful with this

@boks1971 @cnderrauber could I get your help on this? I don’t know the RTX code well enough to fix, so I was going to revert and re-land.

I don’t think that’s a good idea because of API breakage. Do you think we could easily test/fix this issue or is it ambiguous?

X

[cnderrauber]

I remember there was a developer said they enabled rtx in prod and don't get problem, we enabled it for some users too. I will try to fix it before revert the feature if you have the steps to reproduce this issue.

[aalekseevx]

The problem reproduces with a simulcast input with rtx. Simulcast SSRCs are not present in SDP, so they do not fill TrackRemote::rtxSsrc and TrackRemote::HasRTX returns false, which results in blocking receiveForRtx

[Sean-Der]

@cnderrauber that sounds like a good plan!

We need to update those value when we get a RTX with the repair-stream-id is all?

[cnderrauber]

I think the code to update rsid is already there and it works well in livekit sfu while it is using buffer instead of track.Read to read rtp packet. I'll test the normal read method.

[Sean-Der]

@cnderrauber any progress on testing? I added d9a59a5, I will add a test for Simulcast tomorrow

[cnderrauber]

@cnderrauber any progress on testing? I added d9a59a5, I will add a test for Simulcast tomorrow

Uses examples/simulcast, I have no problem to send all 3 simulcast layers back to the browser with rtx enabled. @aalekseevx which browser are you using, can you try it with the simulcast example in the repo?

[aalekseevx]

The original issue was encountered with chromium, I'll work on reproduction based on examples/simulcast

[aalekseevx]

@cnderrauber, I have reproduced the unwanted behaviour in my branch. It may be not 100% accurate, but I hope it will be helpful.

I run it as follows:

 go test -v -run ^TestPeerConnection_Simulcast$/^E2E$ -timeout 3s
=== RUN   TestPeerConnection_Simulcast
=== RUN   TestPeerConnection_Simulcast/E2E
readRTX not reading from t.repairStreamChannel, reader.HasRTX == false
readRTX not reading from t.repairStreamChannel, reader.HasRTX == false
readRTX not reading from t.repairStreamChannel, reader.HasRTX == false
readRTX not reading from t.repairStreamChannel, reader.HasRTX == false
readRTX not reading from t.repairStreamChannel, reader.HasRTX == false
readRTX not reading from t.repairStreamChannel, reader.HasRTX == false
trying to write to track.repairStreamChannel
trying to write to track.repairStreamChannel
trying to write to track.repairStreamChannel
    peerconnection_media_test.go:1382: 
                Error Trace:    /home/alekseev-dev/webrtc/peerconnection_media_test.go:1382
                Error:          "0" is not greater than "0"
                Test:           TestPeerConnection_Simulcast/E2E
                Messages:       no rtx packet read
--- FAIL: TestPeerConnection_Simulcast (2.07s)
    --- FAIL: TestPeerConnection_Simulcast/E2E (1.67s)
FAIL
exit status 1
FAIL    github.com/pion/webrtc/v4       2.072s

[cnderrauber]

Your test case is useful, although it needs small change to match the real case. The root cause is TrackRemote relies on HasRTX(rtxssrc!=0) to determine if it needs to read the rtx packet but it dosen't have that in the simulcast case because rtx ssrc is not included in sdp. The result is TrackRemote.Read/ReadRTP method can't get any rtx packet in this case but only get media packet so examples/simucalst can pass the test.