Add opt to control transceiver re-use in recvonly

[boks1971]

Description

SetDisableTransceiverReuseInRecvonly controls if a transceiver is re-used when its current direction is recvonly.

This is useful for the following scenario

• Remote side sends offer with sendonly media section.
• Local side creates transceiver in SetRemoteDescription and sets direction to recvonly.
• Local side calls AddTrack.
• As the current direction is recvonly, the transceiver added above will be re-used. That will set the direction to sendrecv and the generated answer will have sendrecv for that media section.
• That answer becomes incompatible as the offerer is using sendonly.

Note that local transceiver will be in recvonly for both sendrecv and sendonly directions in the media section. If the offer did use sendrecv, it is possible to re-use that transceiver for sending. So, disabling re-use will prohibit re-use in the sendrecv case also and hence is slightly wasteful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.11%. Comparing base (3e84081) to head (b735b71).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3200      +/-   ##
==========================================
+ Coverage   77.97%   78.11%   +0.14%     
==========================================
  Files          93       93              
  Lines       11814    11817       +3     
==========================================
+ Hits         9212     9231      +19     
+ Misses       2086     2075      -11     
+ Partials      516      511       -5     

Flag	Coverage Δ
go	79.88% <100.00%> (+0.15%)	⬆️
wasm	63.33% <ø> (ø)

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.

[boks1971]

Have more details in this Discord thread - https://discord.com/channels/1352636971591274548/1359189787768258671/1408335415726837791.

And Chromium implements it the same way - https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/rtp_transmission_manager.cc;drc=43776165d1d46d532f5e0a184045fb62e959af6e;l=208.

So, added a setting in this PR to bypass the failing case. Default behaviour has not changed.

[JoeTurki]

Thank you for this fix.
I'm a bit confused: this puts the spec-correct behavior behind a flag and leaves the default behavior incorrect, is that right? Is there a reason to keep the current default - do any clients rely on it? Otherwise, keeping the fix behind a flag means only power users will benefit from this fix.

Thank you

[boks1971]

Thank you for this fix. I'm a bit confused: this puts the spec-correct behavior behind a flag and leaves the default behavior incorrect, is that right? Is there a reason to keep the current default - do any clients rely on it? Otherwise, keeping the fix behind a flag means only power users will benefit from this fix.

Thank you

Spec is unclear to me @JoeTurki . That's why I put it behind a flag.

We are already a bit off-spec (comment in code).

This really requires remote direction. This seems like a gap in the spec. Here is the scenario

• Remote sends offer. A transceiver is created from the remote description and local direction is set to recvonly here. Note that the remote could be either sendrecv or sendonly and in both cases local direction is se to recvonly.
• Later a local AddTrack call happens. If the remote was sendrecv, that media section can be used for bi-directional media. The local direction will change from recvonly to sendrecv. That happens here. That will generate an answer with sendrecv direction for that media section. All good so far.
• The problem happens if the remote direction was sendonly in Step 1 ☝️ . AddTrack will re-use it and set the local direction to sendrecv (same code lines as above). And the answer will have a direction of sendrecv which is incorrect according to spec (https://datatracker.ietf.org/doc/html/rfc3264#section-6.1).

But, the webrtc spec does not clearly handle this case as far as I could read - https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-addtrack. And Pion's implementation matches Chrome's implementation (minus the offs-spec condition I mentioned ☝️ ).

So, this feels like a gap in the spec. But, I am also finding really hard to believe that this is a spec issue given how many people would have reviewed this. And I am missing something.

But, I could not explain it after looking at it for several hours. So, I put the new behaviour behind a flag and let users enable it if they ned it. Like I mentioned the new flag if enabled also kills the case of remote direction sendrecv being re-used. That is a bit of a penalty in SDP size when using this flag, but this flag fixes the other invalid combination case.

Please let me know what you think. Have had a couple of threads with @cnderrauber who has spent a bunch of time on this also. I think he also believes that this condition is ambiguous in the spec. @cnderrauber , please chime in also if you have thoughts/ideas/suggestions.

[JoeTurki]

Okay, this makes sense.
Thank you.

[nils-ohlmeier]

Sorry for the too late comment: but the solution to the problem described is that we internally need to save the sendrecv state of how the transceiver got created.

One option is to store the sendrecv attribute of the last offer. And when looking for the next available transceiver to take these original attributes into account as well.

The other option is to use an internal state which goes beyond the 4 states of the sendrecv attribute and has extra states which represent "we are recvonly, but this can be used for sending". In this case you obviously also need one more state for the vice versa scenario.

[boks1971]

Sorry for the too late comment: but the solution to the problem described is that we internally need to save the sendrecv state of how the transceiver got created.

One option is to store the sendrecv attribute of the last offer. And when looking for the next available transceiver to take these original attributes into account as well.

The other option is to use an internal state which goes beyond the 4 states of the sendrecv attribute and has extra states which represent "we are recvonly, but this can be used for sending". In this case you obviously also need one more state for the vice versa scenario.

Thank you @nils-ohlmeier for the feedback.

Yeah, I thought about it.

But, I could not find libwebrtc doing that (but my code searching skills in libwebrtc leaves a lot to be desired :-) ) . So, I took the approach of protecting new behaviour with a setting.

I will look at adding the remote direction to transceiver state and use it.