-
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 unpublish and unsubscribe for Single PC #1263
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.
Looks good to me 👍 just one comment for something I think we should discuss more and potentially move to another PR in the future
@@ -116,22 +128,31 @@ const BaseStack = (specInput) => { | |||
specBase.callback({ | |||
type: localDesc.type, | |||
sdp: localDesc.sdp, | |||
config: { maxVideoBW: specBase.maxVideoBW }, | |||
}); | |||
Logger.info('Setting local description p2p', localDesc); |
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.
Consider updating the log here to show that this doesn't only apply to p2p anymore
@@ -45,7 +45,7 @@ const Room = (altIo, altConnectionHelpers, altConnectionManager, specInput) => { | |||
// Private functions | |||
const removeStream = (streamInput) => { | |||
const stream = streamInput; | |||
if (stream.stream) { | |||
if (stream.stream && !stream.local) { |
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.
Not closing the local stream when unpublishing can be risky in some cases. I'd prefer thinking about how we want to manage this in the API and update it accordingly in another PR
Description
There was some issues in Single Peer Connection when trying to unsubscribe or unpublish streams because we were not negotiating the SDP again. This PR adds such negotiation and it also introduces some minor fixes.
[] It needs and includes Unit Tests
Changes in Client or Server public APIs
Not needed.
[] It includes documentation for these changes in
/doc
.