From 39d4b3133ffe531eb675507350c9c5da105551eb Mon Sep 17 00:00:00 2001 From: Pawel Domas <2965063+paweldomas@users.noreply.github.com> Date: Fri, 10 Sep 2021 10:41:21 -0500 Subject: [PATCH 1/6] do not use this.local video --- conference.js | 52 +++++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/conference.js b/conference.js index e9ca00ee6fe62..f6e70031b6d5c 100644 --- a/conference.js +++ b/conference.js @@ -961,17 +961,18 @@ export default { const maybeShowErrorDialog = error => { showUI && APP.store.dispatch(notifyCameraError(error)); }; + const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); if (mute) { try { - await this.localVideo.setEffect(undefined); + await localVideo.setEffect(undefined); } catch (err) { logger.error('Failed to remove the presenter effect', err); maybeShowErrorDialog(err); } } else { try { - await this.localVideo.setEffect(await this._createPresenterStreamEffect()); + await localVideo.setEffect(await this._createPresenterStreamEffect()); } catch (err) { logger.error('Failed to apply the presenter effect', err); maybeShowErrorDialog(err); @@ -1013,7 +1014,9 @@ export default { return; } - if (!this.localVideo && !mute) { + const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); + + if (!localVideo && !mute) { const maybeShowErrorDialog = error => { showUI && APP.store.dispatch(notifyCameraError(error)); }; @@ -1384,12 +1387,11 @@ export default { return new Promise((resolve, reject) => { _replaceLocalVideoTrackQueue.enqueue(onFinish => { const state = APP.store.getState(); + const oldTrack = getLocalJitsiVideoTrack(state); // When the prejoin page is displayed localVideo is not set // so just replace the video track from the store with the new one. if (isPrejoinPageVisible(state)) { - const oldTrack = getLocalJitsiVideoTrack(state); - logger.debug(`useVideoStream on the prejoin screen: Replacing ${oldTrack} with ${newTrack}`); return APP.store.dispatch(replaceLocalTrack(oldTrack, newTrack)) @@ -1401,11 +1403,10 @@ export default { .then(onFinish); } - logger.debug(`useVideoStream: Replacing ${this.localVideo} with ${newTrack}`); + logger.debug(`useVideoStream: Replacing ${oldTrack} with ${newTrack}`); APP.store.dispatch( - replaceLocalTrack(this.localVideo, newTrack, room)) + replaceLocalTrack(oldTrack, newTrack, room)) .then(() => { - this.localVideo = newTrack; this._setSharingScreen(newTrack); this.setVideoMuteStatus(); }) @@ -1772,7 +1773,8 @@ export default { // Create a new presenter track and apply the presenter effect. if (!this.localPresenterVideo && !mute) { - const { height, width } = this.localVideo.track.getSettings() ?? this.localVideo.track.getConstraints(); + const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); + const { height, width } = localVideo.track.getSettings() ?? localVideo.track.getConstraints(); const isPortrait = height >= width; const DESKTOP_STREAM_CAP = 720; @@ -1801,7 +1803,7 @@ export default { // Apply the constraints on the desktop track. try { - await this.localVideo.track.applyConstraints(desktopResizeConstraints); + await localVideo.track.applyConstraints(desktopResizeConstraints); } catch (err) { logger.error('Failed to apply constraints on the desktop stream for presenter mode', err); @@ -1809,7 +1811,7 @@ export default { } } const trackHeight = resizeDesktopStream - ? this.localVideo.track.getSettings().height ?? DESKTOP_STREAM_CAP + ? localVideo.track.getSettings().height ?? DESKTOP_STREAM_CAP : height; let effect; @@ -1824,7 +1826,7 @@ export default { // Replace the desktop track on the peerconnection. try { - await this.localVideo.setEffect(effect); + await localVideo.setEffect(effect); APP.store.dispatch(setVideoMuted(mute, MEDIA_TYPE.PRESENTER)); this.setVideoMuteStatus(); } catch (err) { @@ -2311,6 +2313,7 @@ export default { APP.UI.addListener( UIEvents.VIDEO_DEVICE_CHANGED, cameraDeviceId => { + const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); const videoWasMuted = this.isLocalVideoMuted(); sendAnalytics(createDeviceChangedEvent('video', 'input')); @@ -2318,7 +2321,7 @@ export default { // If both screenshare and video are in progress, restart the // presenter mode with the new camera device. if (this.isSharingScreen && !videoWasMuted) { - const { height } = this.localVideo.track.getSettings(); + const { height } = localVideo.track.getSettings(); // dispose the existing presenter track and create a new // camera track. @@ -2327,7 +2330,7 @@ export default { this.localPresenterVideo = null; return this._createPresenterStreamEffect(height, cameraDeviceId) - .then(effect => this.localVideo.setEffect(effect)) + .then(effect => localVideo.setEffect(effect)) .then(() => { this.setVideoMuteStatus(); logger.log('Switched local video device while screen sharing and the video is unmuted'); @@ -2340,7 +2343,7 @@ export default { // that can be applied on un-mute. } else if (this.isSharingScreen && videoWasMuted) { logger.log('Switched local video device: while screen sharing and the video is muted'); - const { height } = this.localVideo.track.getSettings(); + const { height } = localVideo.track.getSettings(); this._updateVideoDeviceId(); @@ -2499,7 +2502,6 @@ export default { this.deviceChangeListener); } - this.localVideo = null; this.localAudio = null; }, @@ -2563,10 +2565,11 @@ export default { * @private */ _updateVideoDeviceId() { - if (this.localVideo - && this.localVideo.videoType === 'camera') { + const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); + + if (localVideo && localVideo.videoType === 'camera') { APP.store.dispatch(updateSettings({ - cameraDeviceId: this.localVideo.getDeviceId() + cameraDeviceId: localVideo.getDeviceId() })); } @@ -2600,6 +2603,7 @@ export default { */ _onDeviceListChanged(devices) { const oldDevices = APP.store.getState()['features/base/devices'].availableDevices; + const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); APP.store.dispatch(updateDeviceList(devices)); @@ -2607,7 +2611,7 @@ export default { = mediaDeviceHelper.getNewMediaDevicesAfterDeviceListChanged( devices, this.isSharingScreen, - this.localVideo, + localVideo, this.localAudio); const promises = []; const audioWasMuted = this.isLocalAudioMuted(); @@ -2635,8 +2639,8 @@ export default { this.localAudio.stopStream(); } - if (requestedInput.video && this.localVideo) { - this.localVideo.stopStream(); + if (requestedInput.video && localVideo) { + localVideo.stopStream(); } // Let's handle unknown/non-preferred devices @@ -2785,13 +2789,14 @@ export default { = APP.store.getState()['features/base/devices'].availableDevices.videoInput; const videoDeviceCount = videoMediaDevices ? videoMediaDevices.length : 0; + const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); // The video functionality is considered available if there are any // video devices detected or if there is local video stream already // active which could be either screensharing stream or a video track // created before the permissions were rejected (through browser // config). - const available = videoDeviceCount > 0 || Boolean(this.localVideo); + const available = videoDeviceCount > 0 || Boolean(localVideo); APP.store.dispatch(setVideoAvailable(available)); APP.API.notifyVideoAvailabilityChanged(available); @@ -2809,7 +2814,6 @@ export default { APP.store.dispatch(destroyLocalTracks()); this._localTracksInitialized = false; - this.localVideo = null; this.localAudio = null; // Remove unnecessary event listeners from firing callbacks. From 170710fba5cb22de0e7d9c950c3fdcfd6fc6eee4 Mon Sep 17 00:00:00 2001 From: Pawel Domas <2965063+paweldomas@users.noreply.github.com> Date: Fri, 10 Sep 2021 13:07:48 -0500 Subject: [PATCH 2/6] move tracks initialized flag around --- conference.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/conference.js b/conference.js index f6e70031b6d5c..5fa52e3ef3c84 100644 --- a/conference.js +++ b/conference.js @@ -728,9 +728,7 @@ export default { track.mute(); } }); - logger.log(`Initialized with ${tracks.length} local tracks`); - this._localTracksInitialized = true; con.addEventListener(JitsiConnectionEvents.CONNECTION_FAILED, _connectionFailedHandler); APP.connection = connection = con; @@ -1350,7 +1348,7 @@ export default { * @private */ _setLocalAudioVideoStreams(tracks = []) { - return tracks.map(track => { + const promise = tracks.map(track => { if (track.isAudioTrack()) { return this.useAudioStream(track); } else if (track.isVideoTrack()) { @@ -1359,12 +1357,16 @@ export default { return this.useVideoStream(track); } - logger.error( - 'Ignored not an audio nor a video track: ', track); + logger.error('Ignored not an audio nor a video track: ', track); return Promise.resolve(); }); + + return promise.then(() => { + this._localTracksInitialized = true; + logger.log(`Initialized with ${tracks.length} local tracks`); + }); }, _getConferenceOptions() { From a0a3cb2e90020c5833ec27afbeabaeb48b1398d6 Mon Sep 17 00:00:00 2001 From: Pawel Domas <2965063+paweldomas@users.noreply.github.com> Date: Fri, 10 Sep 2021 13:59:20 -0500 Subject: [PATCH 3/6] do not use this.localAudio --- conference.js | 80 +++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 44 deletions(-) diff --git a/conference.js b/conference.js index 5fa52e3ef3c84..57ce8cd11f8f1 100644 --- a/conference.js +++ b/conference.js @@ -454,27 +454,12 @@ export default { isSharingScreen: false, - /** - * The local audio track (if any). - * FIXME tracks from redux store should be the single source of truth - * @type {JitsiLocalTrack|null} - */ - localAudio: null, - /** * The local presenter video track (if any). * @type {JitsiLocalTrack|null} */ localPresenterVideo: null, - /** - * The local video track (if any). - * FIXME tracks from redux store should be the single source of truth, but - * more refactoring is required around screen sharing ('localVideo' usages). - * @type {JitsiLocalTrack|null} - */ - localVideo: null, - /** * Returns an object containing a promise which resolves with the created tracks & * the errors resulting from that process. @@ -905,7 +890,9 @@ export default { return; } - if (!this.localAudio && !mute) { + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); + + if (!localAudio && !mute) { const maybeShowErrorDialog = error => { showUI && APP.store.dispatch(notifyMicError(error)); }; @@ -1459,12 +1446,11 @@ export default { return new Promise((resolve, reject) => { _replaceLocalAudioTrackQueue.enqueue(onFinish => { const state = APP.store.getState(); + const oldTrack = getLocalJitsiAudioTrack(state); // When the prejoin page is displayed localAudio is not set // so just replace the audio track from the store with the new one. if (isPrejoinPageVisible(state)) { - const oldTrack = getLocalJitsiAudioTrack(state); - return APP.store.dispatch(replaceLocalTrack(oldTrack, newTrack)) .then(resolve) .catch(reject) @@ -1472,9 +1458,9 @@ export default { } APP.store.dispatch( - replaceLocalTrack(this.localAudio, newTrack, room)) + replaceLocalTrack(oldTrack, newTrack, room)) .then(() => { - this.localAudio = newTrack; + // TODO do it isPrejoinPageVisible conditionally although it would not hurt to run always this.setAudioMuteStatus(this.isLocalAudioMuted()); }) .then(resolve) @@ -1549,7 +1535,9 @@ export default { // If system audio was also shared stop the AudioMixerEffect and dispose of the desktop audio track. if (this._mixerEffect) { - await this.localAudio.setEffect(undefined); + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); + + await localAudio.setEffect(undefined); await this._desktopAudioStream.dispose(); this._mixerEffect = undefined; this._desktopAudioStream = undefined; @@ -1884,12 +1872,14 @@ export default { } if (this._desktopAudioStream) { + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); + // If there is a localAudio stream, mix in the desktop audio stream captured by the screen sharing // api. - if (this.localAudio) { + if (localAudio) { this._mixerEffect = new AudioMixerEffect(this._desktopAudioStream); - await this.localAudio.setEffect(this._mixerEffect); + await localAudio.setEffect(this._mixerEffect); } else { // If no local stream is present ( i.e. no input audio devices) we use the screen share audio // stream as we would use a regular stream. @@ -2070,10 +2060,10 @@ export default { }); room.on(JitsiConferenceEvents.TRACK_AUDIO_LEVEL_CHANGED, (id, lvl) => { + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); let newLvl = lvl; - if (this.isLocalId(id) - && this.localAudio && this.localAudio.isMuted()) { + if (this.isLocalId(id) && localAudio?.isMuted()) { newLvl = 0; } @@ -2431,13 +2421,15 @@ export default { return this.useAudioStream(stream); }) .then(() => { - if (this.localAudio && hasDefaultMicChanged) { + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); + + if (localAudio && hasDefaultMicChanged) { // workaround for the default device to be shown as selected in the // settings even when the real device id was passed to gUM because of the // above mentioned chrome bug. - this.localAudio._realDeviceId = this.localAudio.deviceId = 'default'; + localAudio._realDeviceId = localAudio.deviceId = 'default'; } - logger.log(`switched local audio device: ${this.localAudio?.getDeviceId()}`); + logger.log(`switched local audio device: ${localAudio?.getDeviceId()}`); this._updateAudioDeviceId(); }) @@ -2503,8 +2495,6 @@ export default { JitsiMediaDevicesEvents.DEVICE_LIST_CHANGED, this.deviceChangeListener); } - - this.localAudio = null; }, /** @@ -2589,9 +2579,11 @@ export default { * @private */ _updateAudioDeviceId() { - if (this.localAudio) { + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); + + if (localAudio) { APP.store.dispatch(updateSettings({ - micDeviceId: this.localAudio.getDeviceId() + micDeviceId: localAudio.getDeviceId() })); } }, @@ -2605,6 +2597,7 @@ export default { */ _onDeviceListChanged(devices) { const oldDevices = APP.store.getState()['features/base/devices'].availableDevices; + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); const localVideo = getLocalJitsiVideoTrack(APP.store.getState()); APP.store.dispatch(updateDeviceList(devices)); @@ -2614,7 +2607,7 @@ export default { devices, this.isSharingScreen, localVideo, - this.localAudio); + localAudio); const promises = []; const audioWasMuted = this.isLocalAudioMuted(); const videoWasMuted = this.isLocalVideoMuted(); @@ -2637,8 +2630,8 @@ export default { // simpler): // If the default device is changed we need to first stop the local streams and then call GUM. Otherwise GUM // will return a stream using the old default device. - if (requestedInput.audio && this.localAudio) { - this.localAudio.stopStream(); + if (requestedInput.audio && localAudio) { + localAudio.stopStream(); } if (requestedInput.video && localVideo) { @@ -2722,15 +2715,16 @@ export default { = mediaType === 'audio' ? this.useAudioStream.bind(this) : this.useVideoStream.bind(this); + const track = tracks.find(t => t.getType() === mediaType) || null; // Use the new stream or null if we failed to obtain it. - return useStream(tracks.find(track => track.getType() === mediaType) || null) + return useStream(track) .then(() => { - if (this.localAudio && hasDefaultMicChanged) { + if (track?.isAudioTrack() && hasDefaultMicChanged) { // workaround for the default device to be shown as selected in the // settings even when the real device id was passed to gUM because of // the above mentioned chrome bug. - this.localAudio._realDeviceId = this.localAudio.deviceId = 'default'; + track._realDeviceId = track.deviceId = 'default'; } mediaType === 'audio' ? this._updateAudioDeviceId() @@ -2770,14 +2764,13 @@ export default { * Determines whether or not the audio button should be enabled. */ updateAudioIconEnabled() { - const audioMediaDevices - = APP.store.getState()['features/base/devices'].availableDevices.audioInput; - const audioDeviceCount - = audioMediaDevices ? audioMediaDevices.length : 0; + const localAudio = getLocalJitsiAudioTrack(APP.store.getState()); + const audioMediaDevices = APP.store.getState()['features/base/devices'].availableDevices.audioInput; + const audioDeviceCount = audioMediaDevices ? audioMediaDevices.length : 0; // The audio functionality is considered available if there are any // audio devices detected or if the local audio stream already exists. - const available = audioDeviceCount > 0 || Boolean(this.localAudio); + const available = audioDeviceCount > 0 || Boolean(localAudio); APP.store.dispatch(setAudioAvailable(available)); APP.API.notifyAudioAvailabilityChanged(available); @@ -2816,7 +2809,6 @@ export default { APP.store.dispatch(destroyLocalTracks()); this._localTracksInitialized = false; - this.localAudio = null; // Remove unnecessary event listeners from firing callbacks. if (this.deviceChangeListener) { From 532fbd3fbe49ae42ccbb8421ef5e0f1d58ebd873 Mon Sep 17 00:00:00 2001 From: Pawel Domas <2965063+paweldomas@users.noreply.github.com> Date: Fri, 10 Sep 2021 14:20:30 -0500 Subject: [PATCH 4/6] untangle use audio/video stream methods It should be safe to call setVideoMuteStatus and setAudioMuteStatus regardless of the prejoin page visibility state. --- conference.js | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/conference.js b/conference.js index 57ce8cd11f8f1..99d64321ec3e6 100644 --- a/conference.js +++ b/conference.js @@ -1375,24 +1375,10 @@ export default { return new Promise((resolve, reject) => { _replaceLocalVideoTrackQueue.enqueue(onFinish => { - const state = APP.store.getState(); - const oldTrack = getLocalJitsiVideoTrack(state); - - // When the prejoin page is displayed localVideo is not set - // so just replace the video track from the store with the new one. - if (isPrejoinPageVisible(state)) { - logger.debug(`useVideoStream on the prejoin screen: Replacing ${oldTrack} with ${newTrack}`); - - return APP.store.dispatch(replaceLocalTrack(oldTrack, newTrack)) - .then(resolve) - .catch(error => { - logger.error(`useVideoStream failed on the prejoin screen: ${error}`); - reject(error); - }) - .then(onFinish); - } + const oldTrack = getLocalJitsiVideoTrack(APP.store.getState()); logger.debug(`useVideoStream: Replacing ${oldTrack} with ${newTrack}`); + APP.store.dispatch( replaceLocalTrack(oldTrack, newTrack, room)) .then(() => { @@ -1445,22 +1431,11 @@ export default { useAudioStream(newTrack) { return new Promise((resolve, reject) => { _replaceLocalAudioTrackQueue.enqueue(onFinish => { - const state = APP.store.getState(); - const oldTrack = getLocalJitsiAudioTrack(state); - - // When the prejoin page is displayed localAudio is not set - // so just replace the audio track from the store with the new one. - if (isPrejoinPageVisible(state)) { - return APP.store.dispatch(replaceLocalTrack(oldTrack, newTrack)) - .then(resolve) - .catch(reject) - .then(onFinish); - } + const oldTrack = getLocalJitsiAudioTrack(APP.store.getState()); APP.store.dispatch( replaceLocalTrack(oldTrack, newTrack, room)) .then(() => { - // TODO do it isPrejoinPageVisible conditionally although it would not hurt to run always this.setAudioMuteStatus(this.isLocalAudioMuted()); }) .then(resolve) From 9683c2beb4e14ea82bd334d07439d7d1f9b44d7e Mon Sep 17 00:00:00 2001 From: Pawel Domas <2965063+paweldomas@users.noreply.github.com> Date: Fri, 10 Sep 2021 14:50:38 -0500 Subject: [PATCH 5/6] add NO-OP to use track methods and fix crash in _setLocalAudioVideoStreams on not a promise --- conference.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/conference.js b/conference.js index 99d64321ec3e6..54a4abc6faf20 100644 --- a/conference.js +++ b/conference.js @@ -1335,7 +1335,7 @@ export default { * @private */ _setLocalAudioVideoStreams(tracks = []) { - const promise = tracks.map(track => { + const promises = tracks.map(track => { if (track.isAudioTrack()) { return this.useAudioStream(track); } else if (track.isVideoTrack()) { @@ -1350,7 +1350,7 @@ export default { }); - return promise.then(() => { + return Promise.all(promises).then(() => { this._localTracksInitialized = true; logger.log(`Initialized with ${tracks.length} local tracks`); }); @@ -1379,6 +1379,13 @@ export default { logger.debug(`useVideoStream: Replacing ${oldTrack} with ${newTrack}`); + if (oldTrack === newTrack) { + resolve(); + onFinish(); + + return; + } + APP.store.dispatch( replaceLocalTrack(oldTrack, newTrack, room)) .then(() => { @@ -1433,6 +1440,13 @@ export default { _replaceLocalAudioTrackQueue.enqueue(onFinish => { const oldTrack = getLocalJitsiAudioTrack(APP.store.getState()); + if (oldTrack === newTrack) { + resolve(); + onFinish(); + + return; + } + APP.store.dispatch( replaceLocalTrack(oldTrack, newTrack, room)) .then(() => { From 2b778f834530d3ca800fde059d96bbb9b1c7c0f0 Mon Sep 17 00:00:00 2001 From: Pawel Domas <2965063+paweldomas@users.noreply.github.com> Date: Mon, 13 Sep 2021 12:07:28 -0500 Subject: [PATCH 6/6] use allSettled --- conference.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conference.js b/conference.js index 54a4abc6faf20..a4cdb546018b0 100644 --- a/conference.js +++ b/conference.js @@ -1350,7 +1350,7 @@ export default { }); - return Promise.all(promises).then(() => { + return Promise.allSettled(promises).then(() => { this._localTracksInitialized = true; logger.log(`Initialized with ${tracks.length} local tracks`); });