ref: store SSRCs as numbers (#485)

* ref: store SSRCs as a number

Converts all the places where SSRCs where stored as string to use
numbers.

* doc(RTPStatsCollector): getNonNegativeStat

Fixes invalid description about returning NaN.

* ref(JitsiConf...EventManager): simplify for..of

* ref(JitsiRemoteTrack): throw TypeError

Will throw a TypeError when 'ssrc' is not a number.

* fix(RTPStatsCollector): invalid reference

* doc(RTPStatsCollector): getNonNegativeStat private

* ref(RTPStatsCollector): simplify for..of

* fix(SSRCs): check for negative value

Will not accept negative SSRCs, since those are supposed to be unsigned.

JitsiConferenceEventManager.js

@@ -53,32 +53,24 @@ export default function JitsiConferenceEventManager(conference) {
  * @param resolutions map of resolutions by ssrc
  */
 function mapResolutionsByUserId(conference, resolutions) {
-
     const id2resolution = {};
 
     // preprocess resolutions: group by user id, skip incorrect
     // resolutions etc.
-    Object.keys(resolutions).forEach(ssrc => {
-        const resolution = resolutions[ssrc];
-
-        if (!resolution.width || !resolution.height
-            || resolution.width === -1 || resolution.height === -1) {
-            return;
-        }
-
+    for (const [ ssrc, resolution ] of resolutions) {
         const id = conference.rtc.getResourceBySSRC(ssrc);
 
-        if (!id) {
-            return;
-        }
-
-        // ssrc to resolution map for user id
-        const idResolutions = id2resolution[id] || {};
+        if (id
+            && resolution.width && resolution.height
+            && resolution.width !== -1 && resolution.height !== -1) {
+            // ssrc to resolution map for user id
+            const idResolutions = id2resolution[id] || {};
 
-        idResolutions[ssrc] = resolution;
+            idResolutions[ssrc] = resolution;
 
-        id2resolution[id] = idResolutions;
-    });
+            id2resolution[id] = idResolutions;
+        }
+    }
 
     return id2resolution;
 }
@@ -89,30 +81,21 @@ function mapResolutionsByUserId(conference, resolutions) {
  * @param framerates map of framerates by ssrc
  */
 function mapFrameratesByUserId(conference, framerates) {
-
     const id2framerate = {};
 
     // preprocess framerates: group by user id
-    Object.keys(framerates).forEach(ssrc => {
-        const framerate = framerates[ssrc];
-
-        if (framerate === 0) {
-            return;
-        }
-
+    for (const [ ssrc, framerate ] of framerates) {
         const id = conference.rtc.getResourceBySSRC(ssrc);
 
-        if (!id) {
-            return;
-        }
-
-        // ssrc to framerate map for user id
-        const id2framerates = id2framerate[id] || {};
+        if (framerate !== 0 && id) {
+            // ssrc to framerate map for user id
+            const id2framerates = id2framerate[id] || {};
 
-        id2framerates[ssrc] = framerate;
+            id2framerates[ssrc] = framerate;
 
-        id2framerate[id] = id2framerates;
-    });
+            id2framerate[id] = id2framerates;
+        }
+    }
 
     return id2framerate;
 }

modules/RTC/JitsiRemoteTrack.js

@@ -22,10 +22,11 @@ let ttfmTrackerVideoAttached = false;
  *        the new JitsiRemoteTrack
  * @param {MediaType} mediaType the type of the media
  * @param {VideoType} videoType the type of the video if applicable
- * @param {string} ssrc the SSRC number of the Media Stream
+ * @param {number} ssrc the SSRC number of the Media Stream
  * @param {boolean} muted the initial muted state
  * @param {boolean} isP2P indicates whether or not this track belongs to a P2P
  * session
+ * @throws {TypeError} if <tt>ssrc</tt> is not a number.
  * @constructor
  */
 export default function JitsiRemoteTrack(
@@ -50,6 +51,11 @@ export default function JitsiRemoteTrack(
         mediaType,
         videoType);
     this.rtc = rtc;
+
+    // Prevent from mixing up type of SSRC which should be a number
+    if (typeof ssrc !== 'number') {
+        throw new TypeError(`SSRC ${ssrc} is not a number`);
+    }
     this.ssrc = ssrc;
     this.ownerEndpointId = ownerEndpointId;
     this.muted = muted;
@@ -147,7 +153,7 @@ JitsiRemoteTrack.prototype.isLocal = function() {
 
 /**
  * Returns the synchronization source identifier (SSRC) of this remote track.
- * @returns {string} the SSRC of this remote track
+ * @returns {number} the SSRC of this remote track
  */
 JitsiRemoteTrack.prototype.getSSRC = function() {
     return this.ssrc;

modules/RTC/RTC.js

@@ -670,7 +670,7 @@ export default class RTC extends Listenable {
     /**
      * Searches in localTracks(session stores ssrc for audio and video) and
      * remoteTracks for the ssrc and returns the corresponding resource.
-     * @param ssrc the ssrc to check.
+     * @param {number} ssrc the ssrc to check.
      */
     getResourceBySSRC(ssrc) {
         const track = this._getTrackBySSRC(ssrc);
@@ -680,22 +680,16 @@ export default class RTC extends Listenable {
 
     /**
      * Finds a track (either local or remote) which runs on the given SSRC.
-     * @param {string|number} ssrc
+     * @param {number} ssrc
      * @return {JitsiTrack|undefined}
-     *
-     * FIXME figure out where SSRC is stored as a string and convert to number
      * @private
      */
     _getTrackBySSRC(ssrc) {
         let track
             = this.getLocalTracks().find(
                 localTrack =>
-
-                    // It is important that SSRC is not compared with ===,
-                    // because the code calling this method is inconsistent
-                    // about string vs number types
                     Array.from(this.peerConnections.values())
-                         .find(pc => pc.getLocalSSRC(localTrack) == ssrc) // eslint-disable-line eqeqeq, max-len
+                         .find(pc => pc.getLocalSSRC(localTrack) === ssrc)
                 );
 
         if (!track) {
@@ -708,19 +702,14 @@ export default class RTC extends Listenable {
     /**
      * Searches in remoteTracks for the ssrc and returns the corresponding
      * track.
-     * @param ssrc the ssrc to check.
+     * @param {number} ssrc the ssrc to check.
      * @return {JitsiRemoteTrack|undefined} return the first remote track that
      * matches given SSRC or <tt>undefined</tt> if no such track was found.
      * @private
      */
     _getRemoteTrackBySSRC(ssrc) {
-        /* eslint-disable eqeqeq */
-        // FIXME: Convert the SSRCs in whole project to use the same type.
-        // Now we are using number and string.
         return this.getRemoteTracks().find(
-            remoteTrack => ssrc == remoteTrack.getSSRC());
-
-        /* eslint-enable eqeqeq */
+            remoteTrack => ssrc === remoteTrack.getSSRC());
     }
 
     /**

modules/RTC/TraceablePeerConnection.js

@@ -575,10 +575,20 @@ TraceablePeerConnection.prototype._remoteTrackAdded = function(stream, track) {
 
     // FIXME the length of ssrcLines[0] not verified, but it will fail
     // with global error handler anyway
-    const trackSsrc = ssrcLines[0].substring(7).split(' ')[0];
+    const ssrcStr = ssrcLines[0].substring(7).split(' ')[0];
+    const trackSsrc = Number(ssrcStr);
     const ownerEndpointId = this.signalingLayer.getSSRCOwner(trackSsrc);
 
-    if (!ownerEndpointId) {
+    if (isNaN(trackSsrc) || trackSsrc < 0) {
+        GlobalOnErrorHandler.callErrorHandler(
+            new Error(
+                `Invalid SSRC: ${ssrcStr
+                    } for remote track, msid: ${streamId
+                    } media type: ${mediaType}`));
+
+        // Abort
+        return;
+    } else if (!ownerEndpointId) {
         GlobalOnErrorHandler.callErrorHandler(
             new Error(
                 `No SSRC owner known for: ${trackSsrc
@@ -622,7 +632,7 @@ TraceablePeerConnection.prototype._remoteTrackAdded = function(stream, track) {
  * @param {MediaStreamTrack} track the WebRTC track instance
  * @param {MediaType} mediaType the track's type of the media
  * @param {VideoType} [videoType] the track's type of the video (if applicable)
- * @param {string} ssrc the track's main SSRC number
+ * @param {number} ssrc the track's main SSRC number
  * @param {boolean} muted the initial muted status
  */
 TraceablePeerConnection.prototype._createRemoteTrack

modules/statistics/RTPStatsCollector.js

@@ -231,8 +231,11 @@ export default function StatsCollector(
     this.statsIntervalId = null;
     this.statsIntervalMilis = statsInterval;
 
-    // Map of ssrcs to SsrcStats
-    this.ssrc2stats = {};
+    /**
+     * Maps SSRC numbers to {@link SsrcStats}.
+     * @type {Map<number,SsrcStats}
+     */
+    this.ssrc2stats = new Map();
 }
 
 /* eslint-enable max-params */
@@ -401,6 +404,28 @@ StatsCollector.prototype._defineGetStatValueMethod = function(keys) {
     return (item, name) => itemStatByKey(item, keyFromName(name));
 };
 
+/**
+ * Obtains a stat value from given stat and converts it to a non-negative
+ * number. If the value is either invalid or negative then 0 will be returned.
+ * @param report
+ * @param {string} name
+ * @return {number}
+ * @private
+ */
+StatsCollector.prototype.getNonNegativeStat = function(report, name) {
+    let value = this._getStatValue(report, name);
+
+    if (typeof value !== 'number') {
+        value = Number(value);
+    }
+
+    if (isNaN(value)) {
+        return 0;
+    }
+
+    return Math.max(0, value);
+};
+
 /* eslint-disable no-continue */
 
 /**
@@ -412,25 +437,6 @@ StatsCollector.prototype.processStatsReport = function() {
     }
 
     const getStatValue = this._getStatValue;
-
-    /**
-     *
-     * @param report
-     * @param name
-     */
-    function getNonNegativeStat(report, name) {
-        let value = getStatValue(report, name);
-
-        if (typeof value !== 'number') {
-            value = Number(value);
-        }
-
-        if (isNaN(value)) {
-            return 0;
-        }
-
-        return Math.max(0, value);
-    }
     const byteSentStats = {};
 
     for (const idx in this.currentStatsReport) {
@@ -464,7 +470,7 @@ StatsCollector.prototype.processStatsReport = function() {
                 type = getStatValue(now, 'transportType');
                 localip = getStatValue(now, 'localAddress');
                 active = getStatValue(now, 'activeConnection');
-                rtt = getNonNegativeStat(now, 'currentRoundTripTime');
+                rtt = this.getNonNegativeStat(now, 'currentRoundTripTime');
             } catch (e) { /* not supported*/ }
             if (!ip || !type || !localip || active !== 'true') {
                 continue;
@@ -512,7 +518,7 @@ StatsCollector.prototype.processStatsReport = function() {
         }
 
         const before = this.previousStatsReport[idx];
-        const ssrc = getStatValue(now, 'ssrc');
+        const ssrc = this.getNonNegativeStat(now, 'ssrc');
 
         if (!before || !ssrc) {
             continue;
@@ -529,8 +535,12 @@ StatsCollector.prototype.processStatsReport = function() {
             continue;
         }
 
-        const ssrcStats
-          = this.ssrc2stats[ssrc] || (this.ssrc2stats[ssrc] = new SsrcStats());
+        let ssrcStats = this.ssrc2stats.get(ssrc);
+
+        if (!ssrcStats) {
+            ssrcStats = new SsrcStats();
+            this.ssrc2stats.set(ssrc, ssrcStats);
+        }
 
         let isDownloadStream = true;
         let key = 'packetsReceived';
@@ -550,11 +560,13 @@ StatsCollector.prototype.processStatsReport = function() {
             packetsNow = 0;
         }
 
-        const packetsBefore = getNonNegativeStat(before, key);
+        const packetsBefore = this.getNonNegativeStat(before, key);
         const packetsDiff = Math.max(0, packetsNow - packetsBefore);
 
-        const packetsLostNow = getNonNegativeStat(now, 'packetsLost');
-        const packetsLostBefore = getNonNegativeStat(before, 'packetsLost');
+        const packetsLostNow
+            = this.getNonNegativeStat(now, 'packetsLost');
+        const packetsLostBefore
+            = this.getNonNegativeStat(before, 'packetsLost');
         const packetsLostDiff = Math.max(0, packetsLostNow - packetsLostBefore);
 
         ssrcStats.setLoss({
@@ -563,8 +575,10 @@ StatsCollector.prototype.processStatsReport = function() {
             isDownloadStream
         });
 
-        const bytesReceivedNow = getNonNegativeStat(now, 'bytesReceived');
-        const bytesReceivedBefore = getNonNegativeStat(before, 'bytesReceived');
+        const bytesReceivedNow
+            = this.getNonNegativeStat(now, 'bytesReceived');
+        const bytesReceivedBefore
+            = this.getNonNegativeStat(before, 'bytesReceived');
         const bytesReceived
             = Math.max(0, bytesReceivedNow - bytesReceivedBefore);
 
@@ -628,7 +642,7 @@ StatsCollector.prototype.processStatsReport = function() {
             // let's try with another one (FF)
             try {
                 ssrcStats.setFramerate(Math.round(
-                    getNonNegativeStat(now, 'framerateMean')));
+                    this.getNonNegativeStat(now, 'framerateMean')));
             } catch (err) { /* not supported*/ }
         }
 
@@ -650,34 +664,29 @@ StatsCollector.prototype.processStatsReport = function() {
     };
     let bitrateDownload = 0;
     let bitrateUpload = 0;
-    const resolutions = {};
-    const framerates = {};
-
-    Object.keys(this.ssrc2stats).forEach(
-        function(ssrc) {
-            const ssrcStats = this.ssrc2stats[ssrc];
+    const resolutions = new Map();
+    const framerates = new Map();
 
-            // process packet loss stats
-            const loss = ssrcStats.loss;
-            const type = loss.isDownloadStream ? 'download' : 'upload';
+    for (const [ ssrc, ssrcStats ] of this.ssrc2stats) {
+        // process packet loss stats
+        const loss = ssrcStats.loss;
+        const type = loss.isDownloadStream ? 'download' : 'upload';
 
-            totalPackets[type] += loss.packetsTotal;
-            lostPackets[type] += loss.packetsLost;
+        totalPackets[type] += loss.packetsTotal;
+        lostPackets[type] += loss.packetsLost;
 
-            // process bitrate stats
-            bitrateDownload += ssrcStats.bitrate.download;
-            bitrateUpload += ssrcStats.bitrate.upload;
+        // process bitrate stats
+        bitrateDownload += ssrcStats.bitrate.download;
+        bitrateUpload += ssrcStats.bitrate.upload;
 
-            ssrcStats.resetBitrate();
+        ssrcStats.resetBitrate();
 
-            // collect resolutions
-            resolutions[ssrc] = ssrcStats.resolution;
+        // collect resolutions
+        resolutions.set(ssrc, ssrcStats.resolution);
 
-            // collect framerates
-            framerates[ssrc] = ssrcStats.framerate;
-        },
-        this
-    );
+        // collect framerates
+        framerates.set(ssrc, ssrcStats.framerate);
+    }
 
     this.eventEmitter.emit(
         StatisticsEvents.BYTE_SENT_STATS, this.peerconnection, byteSentStats);
@@ -728,7 +737,7 @@ StatsCollector.prototype.processAudioLevelReport = function() {
         }
 
         const before = this.baselineAudioLevelsReport[idx];
-        const ssrc = getStatValue(now, 'ssrc');
+        const ssrc = this.getNonNegativeStat(now, 'ssrc');
 
         if (!before) {
             logger.warn(`${ssrc} not enough data`);

modules/xmpp/JingleSessionPC.js

@@ -600,7 +600,7 @@ export default class JingleSessionPC extends JingleSession {
                         + 'source[xmlns="urn:xmpp:jingle:apps:rtp:ssma:0"]');
 
             ssrcs.each((i2, ssrcElement) => {
-                const ssrc = ssrcElement.getAttribute('ssrc');
+                const ssrc = Number(ssrcElement.getAttribute('ssrc'));
 
                 $(ssrcElement)
                     .find('>ssrc-info[xmlns="http://jitsi.org/jitmeet"]')
@@ -608,8 +608,14 @@ export default class JingleSessionPC extends JingleSession {
                         const owner = ssrcInfoElement.getAttribute('owner');
 
                         if (owner && owner.length) {
-                            this.signalingLayer.setSSRCOwner(
-                                ssrc, Strophe.getResourceFromJid(owner));
+                            if (isNaN(ssrc) || ssrc < 0) {
+                                logger.warn(
+                                    `Invalid SSRC ${ssrc} value received`
+                                        + ` for ${owner}`);
+                            } else {
+                                this.signalingLayer.setSSRCOwner(
+                                    ssrc, Strophe.getResourceFromJid(owner));
+                            }
                         }
                     }
                 );

modules/xmpp/SignalingLayerImpl.js

@@ -24,7 +24,7 @@ export default class SignalingLayerImpl extends SignalingLayer {
          * 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<string, string>} maps SSRC number to jid
+         * @type {Map<number, string>} maps SSRC number to jid
          */
         this.ssrcOwners = new Map();
 
@@ -95,10 +95,14 @@ export default class SignalingLayerImpl extends SignalingLayer {
 
     /**
      * Set an SSRC owner.
-     * @param {string} ssrc an SSRC to be owned
+     * @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
      */
     setSSRCOwner(ssrc, endpointId) {
+        if (typeof ssrc !== 'number') {
+            throw new TypeError(`SSRC(${ssrc}) must be a number`);
+        }
         this.ssrcOwners.set(ssrc, endpointId);
     }
 }

service/RTC/SignalingLayer.js

@@ -19,7 +19,7 @@ export default class SignalingLayer extends Listenable {
 
     /**
      * Obtains the endpoint ID for given SSRC.
-     * @param {string} ssrc a string representation of the SSRC number.
+     * @param {number} ssrc the SSRC number.
      * @return {string|null} the endpoint ID for given media SSRC.
      */
     getSSRCOwner(ssrc) { // eslint-disable-line no-unused-vars