-
Notifications
You must be signed in to change notification settings - Fork 1k
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 SDP negotiation for resubscriptions #1423
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! I've been reviewing/inspecting parts of this for a while so it's hard for me to find new things, just a comment for an improvement for the future.
Looking at the end result, and besides the mountain of smaller fixes/improvements, I particularly like the way promises are integrated into erizo and using conan for dependencies 💯
@@ -83,6 +83,7 @@ const Room = (altIo, altConnectionHelpers, altConnectionManager, specInput) => { | |||
if (!that.p2p) { | |||
stream.pc.addStream(stream); | |||
} | |||
stream.state = 'subscribed'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future we should think about having a more involved fsm like we're using in other places. Even maybe just having all the states set as a constant somewhere so we can see all the possible states at a glance would be an improvement.
Description
This PR aims to reapply old commits that solved cases where we want to unsubscribe and subscribe again. It is mainly focused for Single Peer Connection, but it also affects multiple Peer Connections.
The old commits introduced a set of issues like Erizo crashes, negotiation failures, etc. that I have been fixing in this PR with more commits.
Changes in Client or Server public APIs
Not needed.
[] It includes documentation for these changes in
/doc
.