Set up RTP Receivers synchronously

[boks1971]

References - #2054

Do the set up of RTP receivers synchronously so that they
are ready to receive media as soon as signalling to remote
side is sent. Once signalling to remote side is sent, the
remote can send media at any time and receiver has to be ready.

Like the bug mentions, a negotiation quickly followed by
a renegotiation left the RTP receivers of the tracks in the
second offer not set up. If the tracks in the renegotiation
happen to be simulcast tracks, they are missed as browsers
send RID only in the first few packets.

The problem can be reproduced by introducing a 1 second
delay in Downstream direction in Network Link Conditioner
and using a modified version of LiveKit JS SDK sample app to
force a double negotiation spaced closely.

With this change, onTrack fires, but the unhandled warning
from RTCP for the highest layer still happens. But, the
track fires almost immediately following that warning
(less than 5 ms later). So, all the simulcast layers
are available.

Description

Reference issue

Fixes #...

[codecov]

Codecov Report

Merging #2087 (0fcb9b7) into master (f644649) will increase coverage by 0.01%.
The diff coverage is 90.44%.

@@            Coverage Diff             @@
##           master    #2087      +/-   ##
==========================================
+ Coverage   76.98%   76.99%   +0.01%     
==========================================
  Files          85       85              
  Lines        8737     8778      +41     
==========================================
+ Hits         6726     6759      +33     
- Misses       1602     1609       +7     
- Partials      409      410       +1     

Flag	Coverage Δ
go	78.82% <90.44%> (+<0.01%)	⬆️
wasm	70.22% <ø> (ø)

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

Impacted Files	Coverage Δ
peerconnection.go	77.14% <85.71%> (+0.33%)	⬆️
rtpreceiver.go	73.06% <100.00%> (+2.03%)	⬆️
sdp.go	87.87% <100.00%> (+0.37%)	⬆️
sctptransport.go	77.81% <0.00%> (-3.39%)	⬇️
dtlstransport.go	63.63% <0.00%> (-1.89%)	⬇️
rtptransceiver.go	85.71% <0.00%> (+1.29%)	⬆️
operations.go	94.11% <0.00%> (+1.96%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f644649...0fcb9b7. Read the comment docs.

[Sean-Der]

Fantastic fix, thank you @boks1971! Really appreciate you grabbing this.

Receive is ORTC/would be a breaking change to modify the behavior. Do you think there is a way we can keep the old behavior/add setupReceive as a private?

I am afk for a bit, can’t look deeper until tonight.

[boks1971]

Thank you @Sean-Der . I don't fully understand all the bits yet :-) There is also some Plan B test failing. I am still working on it. Will check on oRTC behaviour.

[boks1971]

@Sean-Der I do not know ORTC spec. So, what I tried to do here is probably not the right thing - 31f6bc2. I just created two private functions setupReceive and startReceive which are used by peerconnection.go. And a public Receive method which just wraps the two methods. I had same arguments to both setupReceive and startReceive, so wrapping them in a public method made easy. Please let me know if that satisfies the ORTC case. If not, can you please point me to the spec so that I can understand the requirements better and think about better ways to solve it.

I was thinking of a further change too, i. e. handle tracks with SSRCs also just like the RID case, i. e. do a startReceive on those also when we see an SSRC in the unhandledMediaProcessor (currently that handles the undeclared SSRC and RID based ones). I was thinking of using it for known SSRC too (although the function name undeclared would not be correct anymore though :-) ) Maybe for another PR?

[Sean-Der]

@boks1971 That sounds like a great code improvement! Simulcast is bolted on (at best).

I wasn't even aware Simulcast was a thing when we designed all of these internals. I would love to have it all re-written with that being something considered day one.

The only thing I would suggest is making a list of all the edge cases/behaviors that we support. Most things are covered by tests and I can help with that. The end of peerconnection_media_test.go and peerconnection_go_test.go have most of them.

[Sean-Der]

@boks1971 Is this a problem?

[boks1971]

Forgot to clean this up. This is used if remoteIsPlanB. I could not get PlanB test to work properly by splitting the setup and start of receivers. I will try it again today and post exactly what the issue is.

[Sean-Der]

The end of setupRTPReceivers and startRTPReceivers is the same. Could we make a helper like forEachNewReceiver? You can have a callback and have it return the unhandled tracks.

Maybe the duplication is ok. The thing that makes me uncomfortable is how many checks/conditionals we have, don't even like how it was before complexity wise.

[Sean-Der]

What do you think of configureReceiver and configureRTPReceivers or maybe setReceiverParamaters.

The more I read the code I understand the flow a bit more, but the naming throws me off. setup is a new term we are using, would be nice if we could have something in the existing WebRTC nomenclature.

[boks1971]

@Sean-Der Will change name from setup -> configure.

Will also look at consolidating common code between setup/configure and start.