-
Notifications
You must be signed in to change notification settings - Fork 575
Turn off the camera when the video is disabled during a call #18788
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,14 @@ export default class MediaDevicesSource extends TrackSource { | |
| this._audioAllowed = true | ||
| this._videoAllowed = true | ||
|
|
||
| // Whether the video device should be actively grabbed. Unlike | ||
| // "_videoAllowed" (which reflects whether video is permitted at all, e.g. | ||
| // based on participant permissions), this reflects whether the video is | ||
| // currently enabled by the user. When the video is disabled the device | ||
| // is fully released (rather than just disabling the track) so the camera | ||
| // hardware light turns off (see spreed#4008). | ||
| this._videoActive = true | ||
|
|
||
| this._active = false | ||
| } | ||
|
|
||
|
|
@@ -57,6 +65,10 @@ export default class MediaDevicesSource extends TrackSource { | |
| return this._videoAllowed | ||
| } | ||
|
|
||
| isVideoActive() { | ||
| return this._videoActive | ||
| } | ||
|
|
||
| setAudioAllowed(audioAllowed) { | ||
| if (this._audioAllowed === audioAllowed) { | ||
| return | ||
|
|
@@ -103,6 +115,33 @@ export default class MediaDevicesSource extends TrackSource { | |
| this._setOutputTrack('video', null) | ||
| } | ||
|
|
||
| setVideoActive(videoActive) { | ||
| if (this._videoActive === videoActive) { | ||
| return | ||
| } | ||
|
|
||
| this._videoActive = videoActive | ||
|
|
||
| if (!videoActive) { | ||
| // Fully stop the video track to release the camera (and thus turn | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pipeline is the most important here, behaviour should not change (e.g. BlackVideoEnforcer should keep sending the black frame, to keep the peer connection alive). Ideally it should replicate selecting 'camera => None', but I see call flags being updated on every camera mute (might be side-effect of this PR, might be original behaviour - need more time to debug it) |
||
| // off the camera hardware light) rather than just disabling it. | ||
| if (this.getOutputTrack('video')) { | ||
| this.getOutputTrack('video').stop() | ||
| } | ||
| this._setOutputTrack('video', null) | ||
|
|
||
| return | ||
| } | ||
|
|
||
| // Re-grabbing the camera only makes sense when the source is active and | ||
| // video is allowed. | ||
| if (!this._active || !this._videoAllowed) { | ||
| return | ||
| } | ||
|
|
||
| this._handleVideoInputIdChangedBound(mediaDevicesManager, mediaDevicesManager.get('videoInputId')) | ||
| } | ||
|
|
||
| async start(retryNoVideoCallback) { | ||
| this._active = true | ||
|
|
||
|
|
@@ -116,7 +155,7 @@ export default class MediaDevicesSource extends TrackSource { | |
|
|
||
| const constraints = { | ||
| audio: this._audioAllowed, | ||
| video: this._videoAllowed, | ||
| video: this._videoAllowed && this._videoActive, | ||
| } | ||
|
|
||
| let stream | ||
|
|
@@ -321,6 +360,12 @@ export default class MediaDevicesSource extends TrackSource { | |
| return | ||
| } | ||
|
|
||
| // While the video is disabled the camera is kept released, so device | ||
| // changes should not grab it again until the video is enabled. | ||
| if (!this._videoActive) { | ||
| return | ||
| } | ||
|
|
||
| if (this._pendingVideoInputIdChangedCount) { | ||
| this._pendingVideoInputIdChangedCount++ | ||
|
|
||
|
|
||
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.
Intention is right, and there's an open issue in #15991.
If you want to continue, I'd suggest starting with it first. Maybe
useGetMessagesProvideranduseGetMessageswill be a good example to based approach on how to split them - so buttons do not subscribe at all.This might be a better approach, than to construct another layer of flags to listen