From 3d55666c0fb904f4f194e0a46ebf060448c9ac8c Mon Sep 17 00:00:00 2001 From: Hristo Terezov Date: Wed, 4 Oct 2023 16:50:23 -0500 Subject: [PATCH] fix(signaling): Merge ssrcOwners and _sourceName. Additional benefits are: - ssrc -> source name will be updated on ssrc remap message from the bridge. - the map will be correctly cleaned when member leave (this logic was not working well for _sourceName map) - Looks cleaner and simpler. --- .../proxyconnection/CustomSignalingLayer.js | 6 -- modules/xmpp/JingleSessionPC.js | 11 ++- modules/xmpp/SignalingLayerImpl.js | 72 +++++++------------ service/RTC/SignalingLayer.js | 19 ++--- 4 files changed, 35 insertions(+), 73 deletions(-) diff --git a/modules/proxyconnection/CustomSignalingLayer.js b/modules/proxyconnection/CustomSignalingLayer.js index c3f0802211..7a981c2b2c 100644 --- a/modules/proxyconnection/CustomSignalingLayer.js +++ b/modules/proxyconnection/CustomSignalingLayer.js @@ -108,12 +108,6 @@ export default class CustomSignalingLayer extends SignalingLayer { return false; } - /** - * @inheritDoc - */ - setTrackSourceName(ssrc, sourceName) { // eslint-disable-line no-unused-vars - } - /** * @inheritDoc */ diff --git a/modules/xmpp/JingleSessionPC.js b/modules/xmpp/JingleSessionPC.js index 13e1d80c04..84760970c2 100644 --- a/modules/xmpp/JingleSessionPC.js +++ b/modules/xmpp/JingleSessionPC.js @@ -926,16 +926,15 @@ export default class JingleSessionPC extends JingleSession { ssrcs.each((i, ssrcElement) => { const ssrc = Number(ssrcElement.getAttribute('ssrc')); + let sourceName; if (ssrcElement.hasAttribute('name')) { - const sourceName = ssrcElement.getAttribute('name'); - - this._signalingLayer.setTrackSourceName(ssrc, sourceName); + sourceName = ssrcElement.getAttribute('name'); } if (this.isP2P) { // In P2P all SSRCs are owner by the remote peer - this._signalingLayer.setSSRCOwner(ssrc, Strophe.getResourceFromJid(this.remoteJid)); + this._signalingLayer.setSSRCOwner(ssrc, Strophe.getResourceFromJid(this.remoteJid), sourceName); } else { $(ssrcElement) .find('>ssrc-info[xmlns="http://jitsi.org/jitmeet"]') @@ -946,7 +945,7 @@ export default class JingleSessionPC extends JingleSession { if (isNaN(ssrc) || ssrc < 0) { logger.warn(`${this} Invalid SSRC ${ssrc} value received for ${owner}`); } else { - this._signalingLayer.setSSRCOwner(ssrc, getEndpointId(owner)); + this._signalingLayer.setSSRCOwner(ssrc, getEndpointId(owner), sourceName); } } }); @@ -1804,7 +1803,7 @@ export default class JingleSessionPC extends JingleSession { logger.debug(`Existing SSRC ${ssrc}: new owner=${owner}, source-name=${source}`); // Update the SSRC owner. - this._signalingLayer.setSSRCOwner(ssrc, owner); + this._signalingLayer.setSSRCOwner(ssrc, owner, source); // Update the track with all the relevant info. track.setSourceName(source); diff --git a/modules/xmpp/SignalingLayerImpl.js b/modules/xmpp/SignalingLayerImpl.js index dd6a5e254d..d460c17d81 100644 --- a/modules/xmpp/SignalingLayerImpl.js +++ b/modules/xmpp/SignalingLayerImpl.js @@ -27,12 +27,10 @@ export default class SignalingLayerImpl extends SignalingLayer { super(); /** - * A map that stores SSRCs of remote streams. And is used only locally - * We store the mapping when jingle is received, and later is used - * onaddstream webrtc event where we have only the ssrc + * A map that stores SSRCs of remote streams and the corresponding jid and source name. * FIXME: This map got filled and never cleaned and can grow during long * conference - * @type {Map} maps SSRC number to jid + * @type {Map} maps SSRC number to jid and source name. */ this.ssrcOwners = new Map(); @@ -53,16 +51,6 @@ export default class SignalingLayerImpl extends SignalingLayer { * @private */ this._remoteSourceState = { }; - - /** - * A map that stores the source name of a track identified by it's ssrc. - * We store the mapping when jingle is received, and later is used - * onaddstream webrtc event where we have only the ssrc - * FIXME: This map got filled and never cleaned and can grow during long - * conference - * @type {Map} maps SSRC number to source name - */ - this._sourceNames = new Map(); } /** @@ -199,12 +187,6 @@ export default class SignalingLayerImpl extends SignalingLayer { const endpointId = Strophe.getResourceFromJid(jid); delete this._remoteSourceState[endpointId]; - - for (const [ key, value ] of this.ssrcOwners.entries()) { - if (value === endpointId) { - delete this._sourceNames[key]; - } - } }; room.addEventListener(XMPPEvents.MUC_MEMBER_LEFT, this._memberLeftHandler); } @@ -301,14 +283,14 @@ export default class SignalingLayerImpl extends SignalingLayer { * @inheritDoc */ getSSRCOwner(ssrc) { - return this.ssrcOwners.get(ssrc); + return this.ssrcOwners.get(ssrc)?.endpointId; } /** * @inheritDoc */ getTrackSourceName(ssrc) { - return this._sourceNames.get(ssrc); + return this.ssrcOwners.get(ssrc)?.sourceName; } /** @@ -353,7 +335,7 @@ export default class SignalingLayerImpl extends SignalingLayer { /** * @inheritDoc */ - setSSRCOwner(ssrc, endpointId) { + setSSRCOwner(ssrc, newEndpointId, newSourceName) { if (typeof ssrc !== 'number') { throw new TypeError(`SSRC(${ssrc}) must be a number`); } @@ -362,10 +344,19 @@ export default class SignalingLayerImpl extends SignalingLayer { // an SSRC conflict could potentially occur. Log a message to make debugging easier. const existingOwner = this.ssrcOwners.get(ssrc); - if (existingOwner && existingOwner !== endpointId) { - this._logOwnerChangedMessage(`SSRC owner re-assigned from ${existingOwner} to ${endpointId}`); + if (existingOwner) { + const { endpointId, sourceName } = existingOwner; + + if (endpointId !== newEndpointId || sourceName !== newSourceName) { + this._logOwnerChangedMessage( + `SSRC owner re-assigned from ${existingOwner}(source-name=${sourceName}) to ${ + newEndpointId}(source-name=${newSourceName})`); + } } - this.ssrcOwners.set(ssrc, endpointId); + this.ssrcOwners.set(ssrc, { + endpointId: newEndpointId, + sourceName: newSourceName + }); } /** @@ -386,25 +377,6 @@ export default class SignalingLayerImpl extends SignalingLayer { return false; } - /** - * @inheritDoc - */ - setTrackSourceName(ssrc, sourceName) { - if (typeof ssrc !== 'number') { - throw new TypeError(`SSRC(${ssrc}) must be a number`); - } - - // Now signaling layer instance is shared between different JingleSessionPC instances, so although very unlikely - // an SSRC conflict could potentially occur. Log a message to make debugging easier. - const existingName = this._sourceNames.get(ssrc); - - if (existingName && existingName !== sourceName) { - this._logOwnerChangedMessage(`SSRC(${ssrc}) sourceName re-assigned from ${existingName} to ${sourceName}`); - } - - this._sourceNames.set(ssrc, sourceName); - } - /** * @inheritDoc */ @@ -427,9 +399,13 @@ export default class SignalingLayerImpl extends SignalingLayer { * @inheritDoc */ updateSsrcOwnersOnLeave(id) { - const ssrcs = Array.from(this.ssrcOwners) - .filter(entry => entry[1] === id) - .map(entry => entry[0]); + const ssrcs = []; + + this.ssrcOwners.forEach(({ endpointId }, ssrc) => { + if (endpointId === id) { + ssrcs.push(ssrc); + } + }); if (!ssrcs?.length) { return; diff --git a/service/RTC/SignalingLayer.js b/service/RTC/SignalingLayer.js index fdac582ca1..fb61d5b91d 100644 --- a/service/RTC/SignalingLayer.js +++ b/service/RTC/SignalingLayer.js @@ -144,11 +144,13 @@ export default class SignalingLayer extends Listenable { /** * Set an SSRC owner. - * @param {number} ssrc an SSRC to be owned - * @param {string} endpointId owner's ID (MUC nickname) - * @throws TypeError if ssrc is not a number + * + * @param {number} ssrc - An SSRC to be owned. + * @param {string} endpointId - Owner's ID (MUC nickname). + * @param {string} sourceName - The related source name. + * @throws TypeError if ssrc is not a number. */ - setSSRCOwner(ssrc, endpointId) { // eslint-disable-line no-unused-vars + setSSRCOwner(ssrc, endpointId, sourceName) { // eslint-disable-line no-unused-vars } /** @@ -161,15 +163,6 @@ export default class SignalingLayer extends Listenable { setTrackMuteStatus(sourceName, muted) { // eslint-disable-line no-unused-vars } - /** - * Saves the source name for a track identified by it's ssrc. - * @param {number} ssrc the ssrc of the target track. - * @param {SourceName} sourceName the track's source name to save. - * @throws TypeError if ssrc is not a number - */ - setTrackSourceName(ssrc, sourceName) { // eslint-disable-line no-unused-vars - } - /** * Sets track's video type. * @param {SourceName} sourceName - the track's source name.