Fix null track temporary set when disabling the virtual background

When an output track was stopped it was automatically removed. When the
virtual background is disabled the input track is bypassed to the
output, but first the effect is stopped. This caused a null track to be
temporary set as the output track before setting the input track which,
in turn, caused different issues in other parts of the code (like
causing the video to hang in the device checker due to the stream being
removed, disabling the video during calls when disabling the background
blur due to the null stream being handled as "no video", or reconnecting
when disabling the video during calls if background blur was enabled due
to a loop in the event handling which caused the events to be handled in
the wrong order).

Even if the robustness of that other code could be improved conceptually
it makes more sense that disabling the virtual background immediately
bypasses the input track, without setting a null track first. Therefore
now the automatic removal of the output track can be disabled if needed,
and it is used when stopping the virtual background effect.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

3 files changed, 102 insertions, 2 deletions

diff --git a/src/utils/media/pipeline/TrackSourceMixin.js b/src/utils/media/pipeline/TrackSourceMixin.js
index 6a472b3ee..b5487f097 100644
--- a/src/utils/media/pipeline/TrackSourceMixin.js
+++ b/src/utils/media/pipeline/TrackSourceMixin.js
@@ -58,8 +58,10 @@
  * sinks will be automatically updated when those methods are called.
  *
  * Note that ended tracks are automatically removed; the output track does not
- * need to be automatically set to null in that case. However, removing a track
- * does not automatically stop it; that needs to be explicitly done if needed.
+ * need to be automatically set to null in that case. If needed, this can be
+ * prevented by calling "_disableRemoveTrackWhenEnded(track)". However, removing
+ * a track does not automatically stop it; that needs to be explicitly done if
+ * needed.
  */
 export default (function() {
 
@@ -174,6 +176,20 @@ export default (function() {
 	}
 
 	/**
+	 * @param {MediaStreamTrack} track the track to prevent being automatically
+	 *        removed when ended
+	 */
+	function _disableRemoveTrackWhenEnded(track) {
+		const trackIds = Object.keys(this._outputTracks)
+
+		trackIds.forEach(trackId => {
+			if (this._outputTracks[trackId] === track) {
+				this._outputTracks[trackId].removeEventListener('ended', this._removeTrackWhenEndedHandlers[this._outputTracks[trackId].id])
+			}
+		})
+	}
+
+	/**
 	 * @param {MediaStreamTrack} track the ended track to remove
 	 */
 	function _removeTrackWhenEnded(track) {
@@ -225,6 +241,7 @@ export default (function() {
 		this._addOutputTrackSlot = this._addOutputTrackSlot || _addOutputTrackSlot
 		this._removeOutputTrackSlot = this._removeOutputTrackSlot || _removeOutputTrackSlot
 		this._setOutputTrack = this._setOutputTrack || _setOutputTrack
+		this._disableRemoveTrackWhenEnded = this._disableRemoveTrackWhenEnded || _disableRemoveTrackWhenEnded
 		this._removeTrackWhenEnded = this._removeTrackWhenEnded || _removeTrackWhenEnded
 		this._setOutputTrackEnabled = this._setOutputTrackEnabled || _setOutputTrackEnabled
 	}
diff --git a/src/utils/media/pipeline/VirtualBackground.js b/src/utils/media/pipeline/VirtualBackground.js
index 4842eaa20..012641c61 100644
--- a/src/utils/media/pipeline/VirtualBackground.js
+++ b/src/utils/media/pipeline/VirtualBackground.js
@@ -280,6 +280,8 @@ export default class VirtualBackground extends TrackSinkSource {
 
 		this._jitsiStreamBackgroundEffect.stopEffect()
 		this._outputStream.getTracks().forEach(track => {
+			this._disableRemoveTrackWhenEnded(track)
+
 			track.stop()
 		})
 
diff --git a/src/utils/media/pipeline/VirtualBackground.spec.js b/src/utils/media/pipeline/VirtualBackground.spec.js
index 96a466074..10767f82d 100644
--- a/src/utils/media/pipeline/VirtualBackground.spec.js
+++ b/src/utils/media/pipeline/VirtualBackground.spec.js
@@ -204,6 +204,21 @@ describe('VirtualBackground', () => {
 	})
 
 	describe('enable/disable virtual background after setting input track', () => {
+		test('sets input track as its output track if disabled', () => {
+			const inputTrack = newMediaStreamTrackMock('input')
+
+			virtualBackground._setInputTrack('default', inputTrack)
+			virtualBackground.setEnabled(false)
+
+			expect(virtualBackground._setOutputTrack).toHaveBeenCalledTimes(2)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(1, 'default', effectOutputTrack)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(2, 'default', inputTrack)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.startEffect).toHaveBeenCalledTimes(1)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.updateInputStream).toHaveBeenCalledTimes(0)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.stopEffect).toHaveBeenCalledTimes(1)
+			expect(effectOutputTrack.stop).toHaveBeenCalledTimes(1)
+		})
+
 		test('sets effect output track as its output track if enabled', () => {
 			const inputTrack = newMediaStreamTrackMock('input')
 
@@ -222,6 +237,21 @@ describe('VirtualBackground', () => {
 	})
 
 	describe('enable/disable input track', () => {
+		test('sets input track as its output track if input track is disabled', () => {
+			const inputTrack = newMediaStreamTrackMock('input')
+
+			virtualBackground._setInputTrack('default', inputTrack)
+			virtualBackground._setInputTrackEnabled('default', false)
+
+			expect(virtualBackground._setOutputTrack).toHaveBeenCalledTimes(2)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(1, 'default', effectOutputTrack)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(2, 'default', inputTrack)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.startEffect).toHaveBeenCalledTimes(1)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.updateInputStream).toHaveBeenCalledTimes(0)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.stopEffect).toHaveBeenCalledTimes(1)
+			expect(effectOutputTrack.stop).toHaveBeenCalledTimes(1)
+		})
+
 		test('sets effect output track as its output track if input track is enabled', () => {
 			const inputTrack = newMediaStreamTrackMock('input')
 
@@ -240,6 +270,21 @@ describe('VirtualBackground', () => {
 	})
 
 	describe('remove input track', () => {
+		test('removes output track when removing input track', () => {
+			const inputTrack = newMediaStreamTrackMock('input')
+
+			virtualBackground._setInputTrack('default', inputTrack)
+			virtualBackground._setInputTrack('default', null)
+
+			expect(virtualBackground._setOutputTrack).toHaveBeenCalledTimes(2)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(1, 'default', effectOutputTrack)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(2, 'default', null)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.startEffect).toHaveBeenCalledTimes(1)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.updateInputStream).toHaveBeenCalledTimes(0)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.stopEffect).toHaveBeenCalledTimes(1)
+			expect(effectOutputTrack.stop).toHaveBeenCalledTimes(1)
+		})
+
 		test('removes output track when removing disabled input track', () => {
 			const inputTrack = newMediaStreamTrackMock('input')
 
@@ -286,6 +331,42 @@ describe('VirtualBackground', () => {
 			expect(virtualBackground._jitsiStreamBackgroundEffect.stopEffect).toHaveBeenCalledTimes(0)
 		})
 
+		test('sets new effect output track as its output track when setting another input track', () => {
+			const inputTrack = newMediaStreamTrackMock('input')
+			const inputTrack2 = newMediaStreamTrackMock('input2')
+
+			virtualBackground._setInputTrack('default', inputTrack)
+			const originalEffectOutputTrack = effectOutputTrack
+			virtualBackground._setInputTrack('default', inputTrack2)
+
+			expect(virtualBackground._setOutputTrack).toHaveBeenCalledTimes(2)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(1, 'default', originalEffectOutputTrack)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(2, 'default', effectOutputTrack)
+			expect(effectOutputTrack).not.toBe(originalEffectOutputTrack)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.startEffect).toHaveBeenCalledTimes(2)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.updateInputStream).toHaveBeenCalledTimes(0)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.stopEffect).toHaveBeenCalledTimes(1)
+			expect(originalEffectOutputTrack.stop).toHaveBeenCalledTimes(1)
+			expect(effectOutputTrack.stop).toHaveBeenCalledTimes(0)
+		})
+
+		test('sets input track as its output track when setting another now disabled input track', () => {
+			const inputTrack = newMediaStreamTrackMock('input')
+			const inputTrack2 = newMediaStreamTrackMock('input2')
+
+			virtualBackground._setInputTrack('default', inputTrack)
+			inputTrack2.enabled = false
+			virtualBackground._setInputTrack('default', inputTrack2)
+
+			expect(virtualBackground._setOutputTrack).toHaveBeenCalledTimes(2)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(1, 'default', effectOutputTrack)
+			expect(virtualBackground._setOutputTrack).toHaveBeenNthCalledWith(2, 'default', inputTrack2)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.startEffect).toHaveBeenCalledTimes(1)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.updateInputStream).toHaveBeenCalledTimes(0)
+			expect(virtualBackground._jitsiStreamBackgroundEffect.stopEffect).toHaveBeenCalledTimes(1)
+			expect(effectOutputTrack.stop).toHaveBeenCalledTimes(1)
+		})
+
 		test('sets effect output track as its output track when setting another now enabled input track', () => {
 			const inputTrack = newMediaStreamTrackMock('input')
 			const inputTrack2 = newMediaStreamTrackMock('input2')