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.
jitsi · Oct 6, 2023 · 3d55666 · 3d55666
1 parent 6d46c58
commit 3d55666
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 73 deletions.
diff --git 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
@@ -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
@@ -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<number, string>} maps SSRC number to jid
+         * @type {Map<number, { endpointId: string, sourceName: string }>} 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<number, string>} 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
@@ -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 <tt>ssrc</tt> 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 <tt>ssrc</tt> 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 <tt>ssrc</tt> 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.