-
Notifications
You must be signed in to change notification settings - Fork 384
chore: add unpublishStream on video stop in sample app #4526
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
base: next
Are you sure you want to change the base?
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
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'm not sure if this is the most correct way of handling this. I think we probably need additional buttons like Unpublish Camera Stream
and Unpublish Audio Stream
. I don't think that we should unpublish the camera/video stream every time we mute the camera. I presume that this would mean that we unrequest/request the stream every time we mute the audio/video? Or am I missing something? Maybe @antsukanova has a better notion of the SDK sample app, so I would be interested in her comments.
if (localMedia.cameraStream) { | ||
await meeting.unpublishStreams([localMedia.cameraStream]); | ||
localMedia.cameraStream.stop(); |
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.
Shouldn't this be after we stop the camera stream?
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.
Probably yes, because have error with localMedia.cameraStream if was another order
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 believe the order here is right, we won't be able to `stop` cameraStream and then `unpublishStreams`, but should we also check that meeting exists? What will happen if the meeting does not exist? And I beliebe you need to do `const meeting = getCurrentMeeting();` after `if (localMedia.cameraStream) {`
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.
"deploy:npm": "yarn npm publish" | ||
}, | ||
"dependencies": { | ||
"@webex/internal-media-core": "2.18.5", |
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 should be as separate PR change.
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.
lgtm
COMPLETES WEBEX-447557
This pull request addresses
When user stop vide, source state is live every time
by making the following changes
When user stop vide, source state changed from live to no source
Change Type
The following scenarios were tested
Test it manually, on sample app click stop video button and check source state
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.