From 3b754fa219dc63a465c3098ebd96034aae3458e6 Mon Sep 17 00:00:00 2001 From: Leonard Kim Date: Tue, 4 Sep 2018 08:29:50 -0700 Subject: [PATCH 1/3] fix(tracks): mute tracks before using when created on device list change --- conference.js | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/conference.js b/conference.js index 37f0b8786..1c18868f9 100644 --- a/conference.js +++ b/conference.js @@ -2375,11 +2375,24 @@ export default { createLocalTracksF, newDevices.videoinput, newDevices.audioinput) - .then(tracks => - Promise.all(this._setLocalAudioVideoStreams(tracks))) + .then(tracks => { + // If audio or video muted before, or we unplugged current + // device and selected new one, then mute new track. + const muteSyncPromises = tracks.map(track => { + if ((track.isVideoTrack() && videoWasMuted) + || (track.isAudioTrack() && audioWasMuted)) { + return track.mute(); + } + + return Promise.resolve(); + }); + + return Promise.all(muteSyncPromises) + .then(() => Promise.all( + this._setLocalAudioVideoStreams(tracks))); + }) .then(() => { - // If audio was muted before, or we unplugged current device - // and selected new one, then mute new audio track. + // Log and sync known mute state. if (audioWasMuted) { sendAnalytics(createTrackMutedEvent( 'audio', @@ -2388,8 +2401,6 @@ export default { muteLocalAudio(true); } - // If video was muted before, or we unplugged current device - // and selected new one, then mute new video track. if (!this.isSharingScreen && videoWasMuted) { sendAnalytics(createTrackMutedEvent( 'video', From dafcde50607dc3eacc607fb0d895447d3ace130c Mon Sep 17 00:00:00 2001 From: Leonard Kim Date: Tue, 4 Sep 2018 09:26:24 -0700 Subject: [PATCH 2/3] ref(video-layout): remove instance variable for gating show/hide The instance variable is not accurate. By default isVisible is set to false but nothing sets the video container to actually not be visible. As such it is possible for the video element itself to autoplay, thereby making video visible, while the isVisible boolean is still false. The fix chosen is to remove instance variable and always respect calls to show/hide so that the video container can be set to hidden. --- modules/UI/videolayout/VideoContainer.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/modules/UI/videolayout/VideoContainer.js b/modules/UI/videolayout/VideoContainer.js index 87aef816d..b5e3b189b 100644 --- a/modules/UI/videolayout/VideoContainer.js +++ b/modules/UI/videolayout/VideoContainer.js @@ -216,8 +216,6 @@ export class VideoContainer extends LargeContainer { this.emitter = emitter; this.resizeContainer = resizeContainer; - this.isVisible = false; - /** * Whether the background should fit the height of the container * (portrait) or fit the width of the container (landscape). @@ -603,17 +601,11 @@ export class VideoContainer extends LargeContainer { * TODO: refactor this since Temasys is no longer supported. */ show() { - // its already visible - if (this.isVisible) { - return Promise.resolve(); - } - return new Promise(resolve => { this.$wrapperParent.css('visibility', 'visible').fadeTo( FADE_DURATION_MS, 1, () => { - this.isVisible = true; resolve(); } ); @@ -628,15 +620,9 @@ export class VideoContainer extends LargeContainer { // hide its avatar this.showAvatar(false); - // its already hidden - if (!this.isVisible) { - return Promise.resolve(); - } - return new Promise(resolve => { this.$wrapperParent.fadeTo(FADE_DURATION_MS, 0, () => { this.$wrapperParent.css('visibility', 'hidden'); - this.isVisible = false; resolve(); }); }); From 3927f29ba86330c5e864d56d0dfe91681058a6f7 Mon Sep 17 00:00:00 2001 From: Leonard Kim Date: Fri, 31 Aug 2018 13:02:04 -0700 Subject: [PATCH 3/3] fix(tracks): enqueue track replacement The process for doing a replaceLocalTrack is async. Is it possible to trigger replaceLocalTrack multiple times before each call is finished. This leads to situations where replaceLocalTrack is called multiple times with oldTrack being null and a new track. In this scenario, each new track will be added, causing UI issues such as the local participant's large video not displaying for remote participants. The action replaceLocalTrack is used when unmuting audio or video, when creating new tracks on device switch, and when toggling screensharing. These actions can collide with each other. One way to fix this would be to queue replaceLocalTrack. --- conference.js | 73 ++++++++++++++++++++++++++++----------- modules/util/TaskQueue.js | 63 +++++++++++++++++++++++++++++++++ modules/util/helpers.js | 11 ++++++ 3 files changed, 127 insertions(+), 20 deletions(-) create mode 100644 modules/util/TaskQueue.js diff --git a/conference.js b/conference.js index 1c18868f9..a54bc1acc 100644 --- a/conference.js +++ b/conference.js @@ -11,6 +11,7 @@ import * as RemoteControlEvents from './service/remotecontrol/RemoteControlEvents'; import UIEvents from './service/UI/UIEvents'; import UIUtil from './modules/UI/util/UIUtil'; +import { createTaskQueue } from './modules/util/helpers'; import * as JitsiMeetConferenceEvents from './ConferenceEvents'; import { @@ -274,6 +275,27 @@ function redirectToStaticPage(pathname) { windowLocation.pathname = newPathname; } +/** + * A queue for the async replaceLocalTrack action so that multiple audio + * replacements cannot happen simultaneously. This solves the issue where + * replaceLocalTrack is called multiple times with an oldTrack of null, causing + * multiple local tracks of the same type to be used. + * + * @private + * @type {Object} + */ +const _replaceLocalAudioTrackQueue = createTaskQueue(); + +/** + * A task queue for replacement local video tracks. This separate queue exists + * so video replacement is not blocked by audio replacement tasks in the queue + * {@link _replaceLocalAudioTrackQueue}. + * + * @private + * @type {Object} + */ +const _replaceLocalVideoTrackQueue = createTaskQueue(); + /** * */ @@ -856,9 +878,6 @@ export default { return; } - // FIXME it is possible to queue this task twice, but it's not causing - // any issues. Specifically this can happen when the previous - // get user media call is blocked on "ask user for permissions" dialog. if (!this.localVideo && !mute) { const maybeShowErrorDialog = error => { showUI && APP.UI.showCameraErrorNotification(error); @@ -1261,16 +1280,23 @@ export default { * @returns {Promise} */ useVideoStream(newStream) { - return APP.store.dispatch( - replaceLocalTrack(this.localVideo, newStream, room)) - .then(() => { - this.localVideo = newStream; - this._setSharingScreen(newStream); - if (newStream) { - APP.UI.addLocalStream(newStream); - } - this.setVideoMuteStatus(this.isLocalVideoMuted()); + return new Promise((resolve, reject) => { + _replaceLocalVideoTrackQueue.enqueue(onFinish => { + APP.store.dispatch( + replaceLocalTrack(this.localVideo, newStream, room)) + .then(() => { + this.localVideo = newStream; + this._setSharingScreen(newStream); + if (newStream) { + APP.UI.addLocalStream(newStream); + } + this.setVideoMuteStatus(this.isLocalVideoMuted()); + }) + .then(resolve) + .catch(reject) + .then(onFinish); }); + }); }, /** @@ -1300,15 +1326,22 @@ export default { * @returns {Promise} */ useAudioStream(newStream) { - return APP.store.dispatch( - replaceLocalTrack(this.localAudio, newStream, room)) - .then(() => { - this.localAudio = newStream; - if (newStream) { - APP.UI.addLocalStream(newStream); - } - this.setAudioMuteStatus(this.isLocalAudioMuted()); + return new Promise((resolve, reject) => { + _replaceLocalAudioTrackQueue.enqueue(onFinish => { + APP.store.dispatch( + replaceLocalTrack(this.localAudio, newStream, room)) + .then(() => { + this.localAudio = newStream; + if (newStream) { + APP.UI.addLocalStream(newStream); + } + this.setAudioMuteStatus(this.isLocalAudioMuted()); + }) + .then(resolve) + .catch(reject) + .then(onFinish); }); + }); }, /** diff --git a/modules/util/TaskQueue.js b/modules/util/TaskQueue.js new file mode 100644 index 000000000..29a4b00e5 --- /dev/null +++ b/modules/util/TaskQueue.js @@ -0,0 +1,63 @@ +const logger = require('jitsi-meet-logger').getLogger(__filename); + +/** + * Manages a queue of functions where the current function in progress will + * automatically execute the next queued function. + */ +export class TaskQueue { + /** + * Creates a new instance of {@link TaskQueue} and sets initial instance + * variable values. + */ + constructor() { + this._queue = []; + this._currentTask = null; + + this._onTaskComplete = this._onTaskComplete.bind(this); + } + + /** + * Adds a new function to the queue. It will be immediately invoked if no + * other functions are queued. + * + * @param {Function} taskFunction - The function to be queued for execution. + * @private + * @returns {void} + */ + enqueue(taskFunction) { + this._queue.push(taskFunction); + this._executeNext(); + } + + /** + * If no queued task is currently executing, invokes the first task in the + * queue if any. + * + * @private + * @returns {void} + */ + _executeNext() { + if (this._currentTask) { + logger.warn('Task queued while a task is in progress.'); + + return; + } + + this._currentTask = this._queue.shift() || null; + + if (this._currentTask) { + this._currentTask(this._onTaskComplete); + } + } + + /** + * Prepares to invoke the next function in the queue. + * + * @private + * @returns {void} + */ + _onTaskComplete() { + this._currentTask = null; + this._executeNext(); + } +} diff --git a/modules/util/helpers.js b/modules/util/helpers.js index b365ef423..6b275e223 100644 --- a/modules/util/helpers.js +++ b/modules/util/helpers.js @@ -1,3 +1,5 @@ +import { TaskQueue } from './TaskQueue'; + /** * Create deferred object. * @@ -13,3 +15,12 @@ export function createDeferred() { return deferred; } + +/** + * Returns an instance of {@link TaskQueue}. + * + * @returns {Object} + */ +export function createTaskQueue() { + return new TaskQueue(); +}