From 25451f3e6a6ea3e79cff7753c32ce5badd3189f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Wed, 22 Mar 2023 01:33:06 +0100 Subject: [PATCH] Fix RemoteVideoBlocker still active after removing its associated model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once a CallParticipantModel is removed its associated RemoteVideoBlocker should no longer block the remote video. Otherwise a deferred blocking would be rejected by the HPB, as the remote peer is no longer available or, even worse, it could be accepted and block the video of the remote peer if it joined again since the original model was removed. To solve that now the RemoteVideoBlocker is destroyed once its associated model is removed, which causes the RemoteVideoBlocker to ignore any further call on its methods. Note that the RemoteVideoBlocker reference can not be nullified instead on the shared datas, as other components, like the VideoView, expect a RemoteVideoBlocker to be always available in its shared data. Internally ignoring further calls also makes the code more robust than just externally guarding any possible call. Signed-off-by: Daniel Calviño Sánchez --- src/components/CallView/CallView.vue | 2 + src/utils/webrtc/RemoteVideoBlocker.js | 23 +++++++++ src/utils/webrtc/RemoteVideoBlocker.spec.js | 55 +++++++++++++++++++++ 3 files changed, 80 insertions(+) diff --git a/src/components/CallView/CallView.vue b/src/components/CallView/CallView.vue index 427c6731e3a..c803321eb45 100644 --- a/src/components/CallView/CallView.vue +++ b/src/components/CallView/CallView.vue @@ -440,6 +440,8 @@ export default { const removedModelIds = Object.keys(this.sharedDatas).filter(sharedDataId => models.find(model => model.attributes.peerId === sharedDataId) === undefined) removedModelIds.forEach(removedModelId => { + this.sharedDatas[removedModelId].remoteVideoBlocker.destroy() + this.$delete(this.sharedDatas, removedModelId) this.speakingUnwatchers[removedModelId]() diff --git a/src/utils/webrtc/RemoteVideoBlocker.js b/src/utils/webrtc/RemoteVideoBlocker.js index 2a8a65cbd6a..5945ba96a50 100644 --- a/src/utils/webrtc/RemoteVideoBlocker.js +++ b/src/utils/webrtc/RemoteVideoBlocker.js @@ -42,6 +42,11 @@ * have video permissions). In that case the CallParticipantModel will block the * video if needed if it becomes available. * + * Once the CallParticipantModel is no longer used the associated + * RemoteVideoBlocker must be destroyed to ensure that the video will not be + * blocked or unblocked; calling any (modifier) method on the RemoteVideoBlocker + * once destroyed has no effect. + * * @param {object} callParticipantModel the model to block/unblock the video on. */ export default function RemoteVideoBlocker(callParticipantModel) { @@ -64,11 +69,21 @@ export default function RemoteVideoBlocker(callParticipantModel) { RemoteVideoBlocker.prototype = { + destroy() { + this._destroyed = true + + clearTimeout(this._blockVideoTimeout) + }, + isVideoEnabled() { return this._enabled }, setVideoEnabled(enabled) { + if (this._destroyed) { + return + } + this._enabled = enabled const hadBlockVideoTimeout = this._blockVideoTimeout @@ -84,6 +99,10 @@ RemoteVideoBlocker.prototype = { }, increaseVisibleCounter() { + if (this._destroyed) { + return + } + this._visibleCounter++ clearTimeout(this._blockVideoTimeout) @@ -97,6 +116,10 @@ RemoteVideoBlocker.prototype = { }, decreaseVisibleCounter() { + if (this._destroyed) { + return + } + if (this._visibleCounter <= 0) { console.error('Visible counter decreased when not visible') diff --git a/src/utils/webrtc/RemoteVideoBlocker.spec.js b/src/utils/webrtc/RemoteVideoBlocker.spec.js index ff2da1d5927..29efa6b04c2 100644 --- a/src/utils/webrtc/RemoteVideoBlocker.spec.js +++ b/src/utils/webrtc/RemoteVideoBlocker.spec.js @@ -333,4 +333,59 @@ describe('RemoteVideoBlocker', () => { expect(remoteVideoBlocker.isVideoEnabled()).toBe(true) }) }) + + describe('destroy', () => { + test('prevents the video from being blocked by default if not shown in some seconds', () => { + jest.advanceTimersByTime(4000) + + expect(callParticipantModel.setVideoBlocked).toHaveBeenCalledTimes(0) + + remoteVideoBlocker.destroy() + + jest.advanceTimersByTime(1000) + + expect(callParticipantModel.setVideoBlocked).toHaveBeenCalledTimes(0) + }) + + test('prevents the video from being blocked or unblocked if enabled or disabled', () => { + remoteVideoBlocker.increaseVisibleCounter() + + remoteVideoBlocker.destroy() + + remoteVideoBlocker.setVideoEnabled(false) + remoteVideoBlocker.setVideoEnabled(true) + + expect(callParticipantModel.setVideoBlocked).toHaveBeenCalledTimes(0) + }) + + test('prevents the video from being blocked after some seconds if hidden before destroying', () => { + remoteVideoBlocker.increaseVisibleCounter() + remoteVideoBlocker.decreaseVisibleCounter() + + jest.advanceTimersByTime(4000) + + expect(callParticipantModel.setVideoBlocked).toHaveBeenCalledTimes(0) + + remoteVideoBlocker.destroy() + + jest.advanceTimersByTime(1000) + + expect(callParticipantModel.setVideoBlocked).toHaveBeenCalledTimes(0) + }) + + test('prevents the video from being blocked after some seconds if hidden after destroying', () => { + remoteVideoBlocker.destroy() + + remoteVideoBlocker.increaseVisibleCounter() + remoteVideoBlocker.decreaseVisibleCounter() + + jest.advanceTimersByTime(4000) + + expect(callParticipantModel.setVideoBlocked).toHaveBeenCalledTimes(0) + + jest.advanceTimersByTime(1000) + + expect(callParticipantModel.setVideoBlocked).toHaveBeenCalledTimes(0) + }) + }) })