From e97645e20807563c71053cd17d763bf9757eafb6 Mon Sep 17 00:00:00 2001 From: Pawel Domas <2965063+paweldomas@users.noreply.github.com> Date: Wed, 7 Feb 2024 12:45:23 -0600 Subject: [PATCH] fix(JitsiConference): sync up local tracks It can happen that a new session is started while JingleSessionPC operation is in progress. Queue up the track operation on the new session to make sure it catches up with the correct tracks state. Example: 1. JitsiConference.replaceTrack(camera1, camera2); 2. At the time there's only JVB session, it's replaceTrack will take 100ms. 3. 50 ms later - a P2P session is started, it will use camera1 as "local tracks" to start the session, because the replacement has not finished yet and this.rtc.localTracks still contains camera1. 4. Another 50ms later, JVB session finishes the replaceTrack and here we see that there's a new P2P session. Schedule a catch-up replaceTrack operation and make it part of the original addTrack operation chain. --- JitsiConference.js | 115 ++++++++++--- JitsiConference.spec.ts | 330 ++++++++++++++++++++++++++++++++++++ modules/RTC/MockClasses.js | 45 ++++- modules/xmpp/MockClasses.js | 30 ++++ 4 files changed, 492 insertions(+), 28 deletions(-) create mode 100644 JitsiConference.spec.ts diff --git a/JitsiConference.js b/JitsiConference.js index 17faaa9ac6..131f0fd4c1 100644 --- a/JitsiConference.js +++ b/JitsiConference.js @@ -1075,6 +1075,8 @@ JitsiConference.prototype.getTranscriptionStatus = function() { * another video track in the conference. */ JitsiConference.prototype.addTrack = function(track) { + logger.info(`addTrack: ${track}`); + if (!track) { throw new Error('addTrack - a track is required'); } @@ -1099,24 +1101,23 @@ JitsiConference.prototype.addTrack = function(track) { this.getLocalTracks(mediaType)?.length); track.setSourceName(sourceName); - const addTrackPromises = []; - this.p2pJingleSession && addTrackPromises.push(this.p2pJingleSession.addTracks([ track ])); - this.jvbJingleSession && addTrackPromises.push(this.jvbJingleSession.addTracks([ track ])); + const addTracksToJingleSessions = this._addTracksToSessions([ track ]); - return Promise.all(addTrackPromises) - .then(() => { - this._setupNewTrack(track); - this._sendBridgeVideoTypeMessage(track); - this._updateRoomPresence(this.getActiveMediaSession()); + return addTracksToJingleSessions.then(() => { + this._setupNewTrack(track); + this._sendBridgeVideoTypeMessage(track); + this._updateRoomPresence(this.getActiveMediaSession()); - if (this.isMutedByFocus || this.isVideoMutedByFocus) { - this._fireMuteChangeEvent(track); - } - }); + if (this.isMutedByFocus || this.isVideoMutedByFocus) { + this._fireMuteChangeEvent(track); + } + + logger.info(`addTrack: ${track} - done`); + }); } - return Promise.reject(new Error(`Cannot add second ${mediaType} track to the conference`)); + return Promise.reject(new Error(`Cannot add second "${track.getVideoType()}" video track`)); } return this.replaceTrack(null, track) @@ -1127,6 +1128,45 @@ JitsiConference.prototype.addTrack = function(track) { if (track.getVideoType() === VideoType.DESKTOP) { this._updateRoomPresence(this.getActiveMediaSession()); } + + logger.info(`addTrack: ${track} - done`); + }); +}; + +/** + * Adds tracks to the media sessions. + * + * @param {JitsiLocalTrack[]} tracks - the tracks to add. + * @returns {Promise} + * @private + */ +JitsiConference.prototype._addTracksToSessions = function(tracks) { + const addTrackPromises = []; + const p2pAddTrack = (this.p2pJingleSession && addTrackPromises.push(this.p2pJingleSession.addTracks(tracks))) + || logger.info(`addTracks tracks=${tracks} - no P2P session`); + const jvbAddTrack = (this.jvbJingleSession && addTrackPromises.push(this.jvbJingleSession.addTracks(tracks))) + || logger.info(`addTracks tracks=${tracks} - no JVB session`); + + return Promise.all(addTrackPromises) + .then(() => { + // It can happen that a new session is started while JingleSessionPC operation is in progress. + // Queue up the track operation on the new session to make sure it catches up with the correct + // tracks state. + // Example: + // 1. JitsiConference.replaceTrack(camera1, camera2); + // 2. At the time there's only JVB session, it's replaceTrack will take 100ms. + // 3. 50 ms later - a P2P session is started, it will use camera1 as "local tracks" to start the session, + // because the replacement has not finished and this.rtc.localTracks still contains camera1. + // 4. Another 50ms later, JVB session finishes the replaceTrack and here we see that there's a new P2P + // session. Schedule a catch-up replaceTrack operation and make it part of the original operation chain. + const catchUpPromises = []; + + !p2pAddTrack && this.p2pJingleSession && catchUpPromises.push(this.p2pJingleSession.addTracks(tracks)) + && logger.info(`addTracks tracks=${tracks} catch up operation scheduled for P2P session`); + !jvbAddTrack && this.jvbJingleSession && catchUpPromises.push(this.jvbJingleSession.addTracks(tracks)) + && logger.info(`addTracks tracks=${tracks} catch up operation scheduled for JVB session`); + + return Promise.all(catchUpPromises); }); }; @@ -1261,6 +1301,8 @@ JitsiConference.prototype.removeTrack = function(track) { * @returns {Promise} resolves when the replacement is finished */ JitsiConference.prototype.replaceTrack = function(oldTrack, newTrack) { + logger.info(`replaceTrack old=${oldTrack} new=${newTrack}`); + const oldVideoType = oldTrack?.getVideoType(); const mediaType = oldTrack?.getType() || newTrack?.getType(); const newVideoType = newTrack?.getVideoType(); @@ -1311,10 +1353,12 @@ JitsiConference.prototype.replaceTrack = function(oldTrack, newTrack) { this._fireMuteChangeEvent(newTrack); } + logger.info(`replaceTrack old=${oldTrack} new=${newTrack} - done`); + return Promise.resolve(); }) .catch(error => { - logger.error(`replaceTrack failed: ${error?.stack}`); + logger.error(`replaceTrack old=${oldTrack} new=${newTrack} - failed: ${error?.message} ${error?.stack}`); return Promise.reject(error); }); @@ -1333,21 +1377,38 @@ JitsiConference.prototype.replaceTrack = function(oldTrack, newTrack) { * @private */ JitsiConference.prototype._doReplaceTrack = function(oldTrack, newTrack) { - const replaceTrackPromises = []; + const replaceTracks = []; + const jvbReplaceTrack + = (this.jvbJingleSession && replaceTracks.push(this.jvbJingleSession.replaceTrack(oldTrack, newTrack))) + || logger.info('_doReplaceTrack - no JVB JingleSession'); - if (this.jvbJingleSession) { - replaceTrackPromises.push(this.jvbJingleSession.replaceTrack(oldTrack, newTrack)); - } else { - logger.info('_doReplaceTrack - no JVB JingleSession'); - } + const p2pReplaceTrack + = (this.p2pJingleSession && replaceTracks.push(this.p2pJingleSession.replaceTrack(oldTrack, newTrack))) + || logger.info('_doReplaceTrack - no P2P JingleSession'); - if (this.p2pJingleSession) { - replaceTrackPromises.push(this.p2pJingleSession.replaceTrack(oldTrack, newTrack)); - } else { - logger.info('_doReplaceTrack - no P2P JingleSession'); - } - - return Promise.all(replaceTrackPromises); + return Promise.all(replaceTracks) + .then(() => { + // It can happen that a new session is started while JingleSessionPC operation is in progress. + // Queue up the track operation on the new session to make sure it catches up with the correct + // tracks state. + // Example: + // 1. JitsiConference.replaceTrack(camera1, camera2); + // 2. At the time there's only JVB session, it's replaceTrack will take 100ms. + // 3. 50 ms later - a P2P session is started, it will use camera1 as "local tracks" to start the session, + // because the replacement has not finished and this.rtc.localTracks still contains camera1. + // 4. Another 50ms later, JVB session finishes the replaceTrack and here we see that there's a new P2P + // session. Schedule a catch-up replaceTrack operation and make it part of the original operation chain. + const catchUpPromises = []; + + !p2pReplaceTrack && this.p2pJingleSession + && catchUpPromises.push(this.p2pJingleSession.replaceTrack(oldTrack, newTrack)) + && logger.info('_doReplaceTrack - scheduled catch-up replaceTrack for the P2P session'); + !jvbReplaceTrack && this.jvbJingleSession + && catchUpPromises.push(this.jvbJingleSession.replaceTrack(oldTrack, newTrack)) + && logger.info('_doReplaceTrack - scheduled catch-up replaceTrack for the JVB session'); + + return Promise.all(catchUpPromises); + }); }; /** diff --git a/JitsiConference.spec.ts b/JitsiConference.spec.ts new file mode 100644 index 0000000000..687c0c2a4d --- /dev/null +++ b/JitsiConference.spec.ts @@ -0,0 +1,330 @@ +import FeatureFlags from './modules/flags/FeatureFlags'; +import { nextTick } from './modules/util/TestUtils'; + +import JitsiConference from './JitsiConference'; +import { MockJitsiLocalTrack } from './modules/RTC/MockClasses'; +import Listenable from './modules/util/Listenable'; +import { MockChatRoom } from './modules/xmpp/MockClasses'; +import { MediaType } from './service/RTC/MediaType'; +import { VideoType } from './service/RTC/VideoType'; +import browser from './modules/browser'; + +class MockXmpp extends Listenable { + connection: { + options: { hosts: {}; videoQuality: {} }; + xmpp: MockXmpp; + jingle: { newP2PJingleSession: () => undefined } + }; + constructor() { + super(); + } + + isRoomCreated() { + return false; + } + + createRoom() { + return new MockChatRoom(); + } + + newP2PJingleSession() { + return undefined; + } +} + +class MockOffer { + find() { + return { + attr: () => { + // This is not written in a generic way and is intended for this condition to return true (to make p2p work): + // https://github.com/jitsi/lib-jitsi-meet/blob/a519f18b9ae33f34968b60be3ab123c81288f602/JitsiConference.js#L2092 + return '0'; + } + } + } +} + +function createMockConfig() { + return { + hosts: { + }, + videoQuality: { + } + }; +} + +function createMockConnection() { + const xmpp = new MockXmpp(); + const connection = { + xmpp, + options: createMockConfig(), + jingle: { + newP2PJingleSession: () => undefined, + } + }; + xmpp.connection = connection; + + return connection; +} + +class MockJingleSessionPC { + private isP2P: boolean; + private peerconnection: { + getLocalTracks: () => MockJitsiLocalTrack[], + getStats: () => Promise<[]>, + getAudioLevels: () => [], + + }; + private _delayMs: number; + constructor({ isP2P }: { isP2P: boolean }) { + this.isP2P = isP2P; + this._delayMs = 0; + this.peerconnection = { + getLocalTracks: () => [], + getStats: () => Promise.resolve([]), + getAudioLevels: () => [], + } + } + + isReady() { + return true; + } + + initialize() { } + + acceptOffer(offer, success, failure, localTracks) { } + + invite(localTracks: MockJitsiLocalTrack[]) { } + + _executeWithDelay(workload: () => void) { + return this._delayMs > 0 ? setTimeout(workload, this._delayMs) : workload(); + } + + async replaceTrack(oldTrack: MockJitsiLocalTrack, newTrack: MockJitsiLocalTrack) { + return new Promise(resolve => this._executeWithDelay(resolve)); + } + + async addTracks(tracks: MockJitsiLocalTrack[]) { + return new Promise(resolve => this._executeWithDelay(resolve)); + } + + addDelayToTrackOperations(delayMs) { + this._delayMs = delayMs; + } + + close() { + + } +} + +function startJvbSession(conference) { + const jvbSession = new MockJingleSessionPC({ isP2P: false }); + + conference.onIncomingCall(jvbSession); + + return jvbSession; +} + +function startP2PSession(conference) { + const p2pJingleSession = new MockJingleSessionPC({ isP2P: true }); + + // Need to inject the mock class here: + spyOn(conference.xmpp.connection.jingle, 'newP2PJingleSession').and.returnValue(p2pJingleSession); + + // This executes the code-path that sends an invite to the 'usr@server.com/conn1' JID + conference._startP2PSession('user@server.com/conn1'); + + return p2pJingleSession; +} + +describe('JitsiConference', () => { + let conference; + + beforeEach(() => { + conference = new JitsiConference({ + name: 'test-conf-1', + connection: createMockConnection(), + config: createMockConfig() + }); + jasmine.clock().install(); + }); + + afterEach(() => { + jasmine.clock().uninstall(); + }); + + describe('addTrack', () => { + it('should throw an error when a falsy value is passed a the track argument', async () => { + try { + conference.addTrack(undefined); + fail('addTrack should throw'); + } catch (error) { + expect(error.message).toBe('addTrack - a track is required'); + } + }); + it('should throw if 2nd video track of the same video kind is added', async () => { + const cameraTrack1 = new MockJitsiLocalTrack(360, MediaType.VIDEO, VideoType.CAMERA); + const cameraTrack2 = new MockJitsiLocalTrack(720, MediaType.VIDEO, VideoType.CAMERA); + const screenTrack1 = new MockJitsiLocalTrack(1080, MediaType.VIDEO, VideoType.DESKTOP); + + await conference.addTrack(cameraTrack1); + await conference.addTrack(screenTrack1); + await conference.addTrack(cameraTrack2) + .then( + () => fail('did not throw'), + error => expect(error.message).toBe('Cannot add second "camera" video track') + ); + }); + it('should be a NOOP if the track is in the conference already', async () => { + const jvbSession = startJvbSession(conference); + const cameraTrack1 = new MockJitsiLocalTrack(360, MediaType.VIDEO, VideoType.CAMERA); + + // FIXME JingleSessionPC.replaceTrack is used to add primary video track instead of addTrack + // const addTracksSpy = spyOn(jvbSession, 'addTracks'); + const replaceTracksSpy = spyOn(jvbSession, 'replaceTrack'); + + await conference.addTrack(cameraTrack1); + await conference.addTrack(cameraTrack1); + await conference.addTrack(cameraTrack1); + + expect(replaceTracksSpy) + .withContext('add track on the JingleSession should have been called once with the camera track') + .toHaveBeenCalledOnceWith(null, cameraTrack1); + }); + }); + describe('JVB JingleSession should pickup the local tracks', () => { + it('when created while track operation is in-progress on the P2P session', async () => { + const p2pJingleSession = startP2PSession(conference); + + p2pJingleSession.addDelayToTrackOperations(1000); + + const cameraTrack1 = new MockJitsiLocalTrack(360, MediaType.VIDEO, VideoType.CAMERA); + + conference.addTrack(cameraTrack1); + + await nextTick(500); + + const jvbSession = new MockJingleSessionPC({ isP2P: false }); + // FIXME adjust to addTrack once JitsiConference will start using addTrack instead of replace for the primary tracks + const replaceTrackSpy = spyOn(jvbSession, 'replaceTrack'); + + conference.onIncomingCall(jvbSession); + + await nextTick(1000); + expect(replaceTrackSpy) + .withContext('replaceTrack should have been called with a track at the 1500ms mark') + .toHaveBeenCalledOnceWith(null, cameraTrack1); + }); + }) + describe('Peer-to-peer JingleSession should pickup the local tracks', () => { + it('when offer created while track operation is in-progress on the JVB session', async () => { + const jvbSession = startJvbSession(conference); + const cameraTrack1 = new MockJitsiLocalTrack(360, MediaType.VIDEO, VideoType.CAMERA); + + // It will take 1000ms to add local track to the conference + jvbSession.addDelayToTrackOperations(1000); + conference.addTrack(cameraTrack1); + + // After 500ms start the P2P session + await nextTick(500); + const p2pJingleSession = new MockJingleSessionPC({ isP2P: true }); + // FIXME adjust to addTrack once JitsiConference will start using addTrack instead of replace for the primary tracks + const replaceTrackSpy = spyOn(p2pJingleSession, 'replaceTrack'); + spyOn(conference.xmpp.connection.jingle, 'newP2PJingleSession').and.returnValue(p2pJingleSession); + conference._startP2PSession('jid'); + + await nextTick(1000); + expect(replaceTrackSpy) + .withContext('invite should have been called with the camera track by the 1500ms mark') + .toHaveBeenCalledOnceWith(null, cameraTrack1); + }); + it('when offer accepted while track operation is in-progress on the JVB session', async () => { + const jvbSession = startJvbSession(conference); + const cameraTrack1 = new MockJitsiLocalTrack(360, MediaType.VIDEO, VideoType.CAMERA); + + // It will take 1000ms to add local track to the conference + jvbSession.addDelayToTrackOperations(1000); + conference.addTrack(cameraTrack1); + + // After 500ms start the P2P session. Note that addTrack is still in progress and the conference needs to + // wait with calling 'accept offer', so that the track is included correctly (there's no flow that would + // pick it up later). + await nextTick(500); + // Different mocks to satisfy the P2P flow starting at conference.onIncomingCall: + spyOn(conference, 'isP2PEnabled').and.returnValue(true); + spyOn(conference, '_shouldBeInP2PMode').and.returnValue(true); + const p2pSession = new MockJingleSessionPC({ isP2P: true }); + // FIXME adjust to addTrack once JitsiConference will start using addTrack instead of replace for the primary tracks + const replaceTrackSpy = spyOn(p2pSession, 'replaceTrack'); + const mockOffer = new MockOffer(); + conference.onIncomingCall(p2pSession, mockOffer); + + await nextTick(1000); + expect(replaceTrackSpy) + .withContext('replaceTrack should have been called with a track on the P2P session by the 1500ms mark') + .toHaveBeenCalledOnceWith(null, cameraTrack1); + }); + }) + describe('replaceTrack', () => { + it('replaces tracks correctly when waiting for past promises to resolve', async () => { + const cameraTrack1 = new MockJitsiLocalTrack(360, MediaType.VIDEO, VideoType.CAMERA); + const cameraTrack2 = new MockJitsiLocalTrack(361, MediaType.VIDEO, VideoType.CAMERA); + const cameraTrack3 = new MockJitsiLocalTrack(362, MediaType.VIDEO, VideoType.CAMERA); + const cameraTrack4 = new MockJitsiLocalTrack(363, MediaType.VIDEO, VideoType.CAMERA); + + const jvbSession = startJvbSession(conference); + + const addTracksSpy = spyOn(jvbSession, 'addTracks'); + const replaceTrackSpy = spyOn(jvbSession, 'replaceTrack'); + + await conference.addTrack(cameraTrack1); + await conference.replaceTrack(cameraTrack1, cameraTrack2); + await conference.replaceTrack(cameraTrack2, cameraTrack3); + await conference.replaceTrack(cameraTrack3, cameraTrack4); + expect(conference.getLocalVideoTracks()[0]) + .withContext('track 3 should have been replaced with track 4') + .toBe(cameraTrack4); + + // FIXME replaceTrack is used on JingleSessionPC instead of the addTrack method + // expect(addTracksSpy) + // .toHaveBeenCalledOnceWith([cameraTrack1]); + // FIXME +1 accounts for replace track used to add the first track of same video type + expect(replaceTrackSpy) + .toHaveBeenCalledTimes(3 + 1); + + // Verify that the tracks were actually replaced in the expected sequence on the JingleSession level: + + // FIXME addTrack - called as replaceTrack(null, cameraTrack1) + expect(replaceTrackSpy.calls.all()[0].args[0]).toEqual(null); + expect(replaceTrackSpy.calls.all()[0].args[1]).toEqual(cameraTrack1); + + // replaceTrack(cameraTrack1, cameraTrack2) + expect(replaceTrackSpy.calls.all()[1].args[0]).toEqual(cameraTrack1); + expect(replaceTrackSpy.calls.all()[1].args[1]).toEqual(cameraTrack2); + + // replaceTrack(cameraTrack2, cameraTrack3) + expect(replaceTrackSpy.calls.all()[2].args[0]).toEqual(cameraTrack2); + expect(replaceTrackSpy.calls.all()[2].args[1]).toEqual(cameraTrack3); + + // replaceTrack(cameraTrack3, cameraTrack4) + expect(replaceTrackSpy.calls.all()[3].args[0]).toEqual(cameraTrack3); + expect(replaceTrackSpy.calls.all()[3].args[1]).toEqual(cameraTrack4); + }); + it('should not allow to replace tracks of different video types', async () => { + const cameraTrack = new MockJitsiLocalTrack(360, MediaType.VIDEO, VideoType.CAMERA); + const screenTrack = new MockJitsiLocalTrack(1080, MediaType.VIDEO, VideoType.DESKTOP); + + await conference.addTrack(cameraTrack); + + try { + conference.replaceTrack(cameraTrack, screenTrack); + fail('Should throw an error'); + } catch(error) { + expect(error.message) + .toBe( + 'Replacing a track of videoType=camera with a track of videoType=desktop' + + ' is not supported in this mode.'); + } + }); + }) +}); diff --git a/modules/RTC/MockClasses.js b/modules/RTC/MockClasses.js index b69ef1c9e4..4cd72c9c11 100644 --- a/modules/RTC/MockClasses.js +++ b/modules/RTC/MockClasses.js @@ -1,7 +1,11 @@ import transform from 'sdp-transform'; +import { MediaType } from '../../service/RTC/MediaType'; +import Listenable from '../util/Listenable'; + /* eslint-disable no-empty-function */ /* eslint-disable max-len */ +/* eslint-disable require-jsdoc */ /** * MockRTCPeerConnection that return the local description sdp. @@ -307,18 +311,47 @@ export class MockTrack { } } +let trackId = 1; + /** * MockJitsiLocalTrack */ -export class MockJitsiLocalTrack { +export class MockJitsiLocalTrack extends Listenable { /** * A constructor */ constructor(height, mediaType, videoType) { + super(); this.resolution = height; this.track = new MockTrack(height); this.type = mediaType; this.videoType = videoType; + this._id = trackId; + trackId += 1; // The track id is useful to distinguish between instances (see toString) + } + + setSourceName(sourceName) { + this.sourceName = sourceName; + } + + getSourceName() { + return this.sourceName; + } + + setConference(conference) { + this.conference = conference; + } + + isAudioTrack() { + return this.getType() === MediaType.AUDIO; + } + + isVideoTrack() { + return this.getType() === MediaType.VIDEO; + } + + isMuted() { + return false; } /** @@ -360,4 +393,14 @@ export class MockJitsiLocalTrack { getVideoType() { return this.videoType; } + + _sendMuteStatus() { } + + toString() { + return `JitsiLocalTrack[id=${this._id},sourceName=${this.sourceName},type=${this.type}]`; + } } + +/* eslint-enable no-empty-function */ +/* eslint-enable max-len */ +/* eslint-enable require-jsdoc */ diff --git a/modules/xmpp/MockClasses.js b/modules/xmpp/MockClasses.js index 701bdaf49a..dfae5741d0 100644 --- a/modules/xmpp/MockClasses.js +++ b/modules/xmpp/MockClasses.js @@ -3,16 +3,45 @@ import { Strophe } from 'strophe.js'; import Listenable from '../util/Listenable'; /* eslint-disable no-empty-function */ +/* eslint-disable require-jsdoc */ /** * Mock {@link ChatRoom}. */ export class MockChatRoom extends Listenable { + constructor() { + super(); + this.connectionTimes = {}; + } + /** * {@link ChatRoom.addPresenceListener}. */ addPresenceListener() { } + + removePresenceListener() { + + } + + addOrReplaceInPresence() { + } + + setParticipantPropertyListener() { + + } + + isFocus() { + return true; // Used to verify incoming JVB jingle session + } + + getMeetingId() { + return undefined; // Doesn't matter in the tests we have so far. + } + + leave() { + + } } /** @@ -80,3 +109,4 @@ export class MockStropheConnection extends Listenable { } } /* eslint-enable no-empty-function */ +/* eslint-enable require-jsdoc */