Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AddTrack transceiver reuse per W3C specs #1861

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

EdoaLive
Copy link

According to W3C specifications, the transceiver can be reused if "The sender has never been used to send. More precisely, the [[CurrentDirection]] slot of the RTCRtpTransceiver associated with the sender has never had a value of "sendrecv" or "sendonly"".
Current implementation does not have CurrentDirection slot, so the flag usedToSend is set on setDirection change.
This fixes #1843 and maybe other issues (for us it fixes other two issues when reusing transceiver, eg clients going and coming back to a conference).
This has been done on the v2 branch because we currently don't have the resources to switch our software to v3 and test it there. Looking at the source (master) seems this kind of issue is still there.

According to W3C specifications, the transceiver can be reused if "The sender has never been used to send. More precisely, the [[CurrentDirection]] slot of the RTCRtpTransceiver associated with the sender has never had a value of "sendrecv" or "sendonly"".
Current implementation does not have CurrentDirection slot, so the flag usedToSend is set on setDirection change.
This fixes pion#1843 and maybe other issues.
@Sean-Der
Copy link
Member

Sean-Der commented Jul 2, 2021

Thank you so much for the fix @EdoaLive !

I will write a test for it and merge to master and v2 in the next couple of days.

@Sean-Der Sean-Der added this to the 3.1.0 milestone Aug 2, 2021
@Sean-Der Sean-Der modified the milestones: 3.1.0, 4.0.0 Sep 14, 2021
@Sean-Der Sean-Der removed this from the 4.0.0 milestone May 22, 2022
@edaniels
Copy link
Member

hey @EdoaLive , do you still want to work together to get this in?

@EdoaLive
Copy link
Author

Hi @edaniels, what do you mean by "work together"?
I'm currently re-implementing our software almost from scratch, using pion/webrtc v3.
I honestly forgot about this issue but I see it's not still merged so maybe the problems it solved are still there.
What work is to be done, except merging the PR?
I don't think I have the means to recreate the context and test the behavior right now, I should also re-read the issue and re-learn what all this was about. Maybe in a couple of weeks when I'm in a consistent testing phase.

Could you be more clear about what has to be done?

@edaniels
Copy link
Member

other than merging the PR, this needs some tests to validate the fix since we're dealing with a state machine. If you get those in, I think we'd be good! No rush on that.

@EdoaLive
Copy link
Author

Do you mean automatic (unit?) testing or real-world testing? In the second case, we used the patched version for years with many users, so there's that.
In the first case, we should recreate the exact conditions (those explained in issue #1843 I guess), but if the tests use pion for both sides of the negotiation, it might works anyway because both sides don't comply with W3C specifications, the sides "understand" each other in both cases, but with a different protocol or machine state than the one used in browsers.

Anyway I'll get back tho this when (and if) this issue happens with our new version and with pion v3 implementation.

@edaniels
Copy link
Member

I do mean unit testing to validate the new code doesn't break any old code. I get what you're saying with needing something other than just two pion peers. It would be nice if we had higher level tests for that, but that's definitely too much for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants