Move TraceablePeerConnection to RTC #373

paweldomas · 2017-02-27T20:23:06Z

The purpose of this PR is to move TraceablePeerConnection to the RTC module where it belongs.

bbaldino · 2017-02-28T01:07:08Z

JitsiConferenceEventManager.js

-        conference.getLocalTracks().forEach(function (track) {
-            var ssrc = track.getSSRC();
-            if(!track.isAudioTrack() || !ssrc || !stats.hasOwnProperty(ssrc))
+        conference.getLocalTracks(MediaType.AUDIO).forEach(function (track) {


why is this only audio now?

the previous code seemed to return early if the track wasn't an audio track, AFAIS.

Yes, the previous code was returning early for audio tracks just like Saul wrote.

bbaldino · 2017-02-28T01:07:59Z

modules/RTC/JitsiRemoteTrack.js

 * @constructor
 */
-function JitsiRemoteTrack(rtc, conference, ownerJid, stream, track, mediaType, videoType,
+function JitsiRemoteTrack(rtc, conference, owner, stream, track, mediaType, videoType,


maybe a personal preference, i think it'd be clearer to rename 'owner' to 'ownerEndpointId'...that way in the code it's self-documenting so you don't have to check the jsdoc to see if it's an object or whatever

I know you've mentioned that already at least one, but I must have missed that fix and it stayed somewhere on top of the old peer-to-peer branch... sorry about that

bbaldino · 2017-02-28T01:09:02Z

modules/xmpp/ChatRoom.js

+            // No presence available
+            return null;
+        }
+        let data = {


looks like this could be const?

bbaldino · 2017-02-28T01:10:03Z

modules/xmpp/JingleSession.js

+
+    /**
+     * The RTC service instance
+     * @type {RTC|null}


i think in general the convention has been to only document its intended type, and not denote it could be null (i don't think we do that anywhere else, and i don't think we have any type that can't be null, so maybe the |null is redundant?)

It depends whether or not it is expected to handle the null case. Here RTC will be initialised later (not in the constructor), but I would say that the instance should not be used prior to that, so I agree that "|null" can be removed (it is not expected to handle null).

bbaldino · 2017-02-28T01:12:37Z

service/RTC/SignallingLayer.js

@@ -0,0 +1,40 @@
+


nit: in the states we'd spell this "SignalingLayer" (one "L" in signaling). apparently "signalling" is the UK way which probably explain why makes my brain feel weird when i read it.

@saghul which one do you prefer ? I don't have strong opinion, but would leave it as is only, because I'm too lazy to change it...

Nevermind I saw your comment below. Will remove "l"

saghul · 2017-02-28T09:59:20Z

JitsiConference.js

@@ -3,7 +3,7 @@
 var logger = require("jitsi-meet-logger").getLogger(__filename);
 import RTC from "./modules/RTC/RTC";
 import * as MediaType from "./service/RTC/MediaType";
-var XMPPEvents = require("./service/xmpp/XMPPEvents");
+var RTCEvents = require("./service/RTC/RTCEvents");


can you refactor the imports since you are here? pretty please? 🙏

saghul · 2017-02-28T10:00:19Z

JitsiConference.js

+ * @return {JitsiLocalTrack|undefined}
+ */
+JitsiConference.prototype.getLocalAudioTrack = function () {
+    return this.rtc ? this.rtc.getLocalAudioTrack() : undefined;


what did we say about undefined vs null on the other PR? /me doesn't remember!

in case we settle on undefined this can be simplified as return this.rtc && this.rtc.getLocalAudioTrack().

I'd say we did not settle at all I just changed to null that particular case. In my opinion it's fine to use whichever as long as it's stated in JSDoc. Otherwise we would have to ask the whole team and come up with a strict rule.

saghul · 2017-02-28T10:00:51Z

JitsiConference.js

-                this.transcriber.addTrack(localTrack);
-            }
-        }.bind(this));
+        this.getLocalTracks(MediaType.AUDIO).forEach(


saghul · 2017-02-28T10:02:32Z

JitsiConference.js

+ * @return {JitsiLocalTrack|undefined}
+ */
+JitsiConference.prototype.getLocalVideoTrack = function () {
+    return this.rtc ? this.rtc.getLocalVideoTrack() : undefined;


saghul · 2017-02-28T10:02:56Z

JitsiConference.js

-        this.rtc.remoteTracks.forEach(function (remoteTrack){
-            if (remoteTrack.isAudioTrack()){
+        this.rtc.getRemoteTracks(MediaType.AUDIO).forEach(
+            function (remoteTrack){


saghul · 2017-02-28T10:39:22Z

modules/RTC/TraceablePeerConnection.js

+        return;
+    }
+
+    let muted = peerMediaInfo.muted;


saghul · 2017-02-28T10:39:34Z

modules/RTC/TraceablePeerConnection.js

+ * PeerConnection
+ */
+TraceablePeerConnection.prototype._remoteStreamRemoved = function (stream) {
+    const self = this;


saghul · 2017-02-28T10:40:27Z

modules/RTC/TraceablePeerConnection.js

+        // behave unexpectedly (the "user left" event would come before "track
+        // removed" events).
+        logger.warn(
+            "Removed track not found for msid: " + streamId


use template strings

saghul · 2017-02-28T10:41:31Z

modules/RTC/TraceablePeerConnection.js

@@ -259,7 +492,8 @@ var normalizePlanB = function(desc) {
                });
            }

-            if (typeof mLine.ssrcs !== 'undefined' && Array.isArray(mLine.ssrcs)) {
+            if (typeof mLine.ssrcs !== 'undefined'


Can't the first part of the test be deleted? Array.isArray(undefined) will be false anyway.

saghul · 2017-02-28T10:44:54Z

modules/xmpp/SDP.js

+                    let msid = null;
+                    // FIXME what is this ? global APP.RTC in SDP ?
+                    const localTrack = APP.RTC.getLocalTracks(mline.media);
+                    if(localTrack) {


style: space after if

There's no reason to involve the ChatRoom directly into the generation of remote stream added event. It merely provides some info carried in the presence about muted state etc.

also fixes lint errors

Moves part of the XMPP events which are clearly related to the PeerConnection to RTCEvents.

bbaldino

test failures are lip sync, so re-running them.

paweldomas · 2017-02-28T16:59:15Z

Yes, lip-sync and start muted. I'm looking into that

paweldomas · 2017-02-28T17:21:03Z

Audio levels were broken. Should be ok now - I'll let you know if the tests pass.

bbaldino reviewed Feb 28, 2017

View reviewed changes

saghul requested changes Feb 28, 2017

View reviewed changes

paweldomas added 12 commits February 28, 2017 10:27

ref(ChatRoom): move emit remote stream added

67cc2f3

There's no reason to involve the ChatRoom directly into the generation of remote stream added event. It merely provides some info carried in the presence about muted state etc.

ref: move TraceablePeerConnection to RTC

1cbc4d7

also fixes lint errors

ref(XMPPEvents): move some PC related events to RTC

98df9a5

Moves part of the XMPP events which are clearly related to the PeerConnection to RTCEvents.

ref(JingleSessionPC): emit track added/removed in RTC

ff932f3

feat(RTC): store peer connections and assign IDs

a31a50e

ref(RTC): add getRemoteTracks method

0ddd4e8

ref(RTC): cleanup local track access

3b59cdb

doc(TraceablePeerConnection): comment on warning msg

d30451a

ref(RTC): rename "owner" to "ownerEndpointId"

af26c86

ref: remove double "l" from SignallingLayer

359e525

ref(JitsiConference): cleanup imports

7a7d697

ref(TraceablePeerConnection): cleanup imports

f2e6d38

paweldomas force-pushed the move_TPC branch from 7c9519a to f2e6d38 Compare February 28, 2017 16:27

bbaldino approved these changes Feb 28, 2017

View reviewed changes

fix(RTC): broken "getRemoteTracks" method

c843889

bbaldino merged commit 12a91dc into master Feb 28, 2017

paweldomas deleted the move_TPC branch March 2, 2017 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move TraceablePeerConnection to RTC #373

Move TraceablePeerConnection to RTC #373

paweldomas commented Feb 27, 2017 •

edited

bbaldino Feb 28, 2017

saghul Feb 28, 2017

paweldomas Feb 28, 2017

bbaldino Feb 28, 2017

bbaldino Feb 28, 2017

saghul Feb 28, 2017

paweldomas Feb 28, 2017

bbaldino Feb 28, 2017

bbaldino Feb 28, 2017

bbaldino Feb 28, 2017

paweldomas Feb 28, 2017

bbaldino Feb 28, 2017

paweldomas Feb 28, 2017

paweldomas Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

paweldomas Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

saghul Feb 28, 2017

bbaldino left a comment

paweldomas commented Feb 28, 2017

paweldomas commented Feb 28, 2017

Move TraceablePeerConnection to RTC #373

Move TraceablePeerConnection to RTC #373

Conversation

paweldomas commented Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbaldino left a comment

Choose a reason for hiding this comment

paweldomas commented Feb 28, 2017

paweldomas commented Feb 28, 2017

paweldomas commented Feb 27, 2017 •

edited