-
Notifications
You must be signed in to change notification settings - Fork 166
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
Try to re-aquire mediastreamtrack when device has been disconnected, … #308
Conversation
…pause upstream if no device could be acquired
🦋 Changeset detectedLatest commit: 8fbb010 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
await track.restartTrack(); | ||
} catch (e) { | ||
log.warn(`could not restart track, pausing upstream instead`); | ||
await track.pauseUpstream(); |
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.
should pauseUpstream replace the MediaStreamTrack that's attached to Track
as well? I think that would ensure we are not listening to events from the old MediaStreamTrack.
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.
good question. we could do that. I assumed that ended
is the last event we ever see of a particular mediastreamtrack. The spec doesn't make it overly clear, but it sounds to me that way:
The MediaStreamTrack object's source will no longer provide any data, either because the user revoked the permissions, or because the source device has been ejected, or because the remote peer permanently stopped sending data.
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.
I think pauseUpstream
as a name might be misleading if it also replaces the original track, as we will loose the ability to resumeUpstream
. maybe it makes sense to add a stopUpstream
function which mutes on server side plus sets the actual track to undefined?
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.
right, that's a fair point. you are right it shouldn't do that. handling resume on replaceTrack covers that case I think.
track: track.sid, | ||
}); | ||
this.unpublishTrack(track); | ||
} else if (track.isUserProvided) { |
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.
should we pauseUpstream here too? if a user track is ended, I think we wouldn't know how to send data associated with it. without indicating it's muted somehow, I think we might get a bunch of PLIs from the receiver.
…pause upstream if no device could be acquired
The function is currently deeply nested. We can figure out the best course of action first and then I'll clean it up.
The basic idea to react to the 'ended' event is:
pauseUpstream
)