feat(multi-stream-support) Add track streaming status #1855

WillLiang918 · 2022-01-19T23:59:13Z

This change allow us handle the streaming status of each individual track when the feature flag is enabled. The emitted streaming status is mostly used for the connection indicator in the UI.

Related jitsi-meet PR: jitsi/jitsi-meet#10934

Todo/Follow up item:

Look into moving some of the TrackStreamingStatus#onForwardedSourcesChanged logic out into RTC#_onForwardedSourcesChanged. The reason being is that since TrackStreamingStatus is only initialized when a track is rendered, there is a corner case where we are not capturing the timestamp of when it entered forwarded sources.
Add specs, this can be part of another PR.

jitsi-jenkins · 2022-01-20T00:01:17Z

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

modules/RTC/BridgeChannel.js

modules/RTC/TrackStreamingStatus.js

WillLiang918 · 2022-01-25T18:50:09Z

modules/RTC/TrackStreamingStatus.js

+                RTCEvents.REMOTE_TRACK_UNMUTE,
+                this._onTrackRtcUnmuted);
+
+            this.track.off(


The logic to handle the JitsiConferenceEvents.TRACK_REMOVED event has been moved into the dispose method of this class.

And when a track is removed this instance will be disposed anyway, right?

I will double check to confirm if that is true. If not, I can dispose it where we are emitting JitsiConferenceEvents.TRACK_REMOVED

I don't think I saw any logic in LJM that called dispose when a tracked was removed. I wasn't sure if this is intended so I added logic to clear _trackStreamingStatusImpl and _trackStreamingStatus where the event was emitted

modules/RTC/TrackStreamingStatus.js

WillLiang918 · 2022-01-25T23:32:48Z

modules/RTC/TrackStreamingStatus.js

+     * @param {Array<string>} enteringForwardedSources - The array of sourceName entering forwarded sources.
+     * @private
+     */
+    _onForwardedSourcesChanged(leavingForwardedSources = [], enteringForwardedSources = []) {


I don't think this will work as I expect it to. I'll need to move this logic somewhere else, perhaps JitsiConferenceEventManager, and have it iterate through all the leaving and entering forwarded sources.

Ah because the timestamps will not be accurate since this instance may disappear. Lets try to think about it... Maybe we need to store the timestamps somewhere (the place which emits the forwarded sources changed event) and then expose that via a getter? Or better have a static part/helper class of TrackStreamingStatus which would keep track of the timestamps (mostly because they are not used by any other logic) ?

I was looking into using a static method/helper class and it was starting to get a bit messy since it would still need to call figureOutStreamingStatus for each instances that is leaving/entering or when !browser.supportsVideoMuteOnConnInterrupted(). If our only concern is having inaccurate timestamps and performance isn't an issue, can we just emit the timestamp with the forwarded sources? Never mind, we still have the issue of losing this timestamp when the instance disappears

We can actually make it so that the JitsiRemoteTrack holds the timestamps when it entered/left the forwarded sources and this can happen any time the RTC module emits the forwarded sources event?

modules/RTC/JitsiRemoteTrack.js

modules/RTC/RTC.js

service/statistics/AnalyticsEvents.js

modules/RTC/TrackStreamingStatus.js

paweldomas · 2022-01-26T02:05:53Z

modules/RTC/TrackStreamingStatus.js

+                RTCEvents.REMOTE_TRACK_UNMUTE,
+                this._onTrackRtcUnmuted);
+
+            this.track.off(


And when a track is removed this instance will be disposed anyway, right?

modules/RTC/TrackStreamingStatus.js

paweldomas · 2022-01-26T02:12:27Z

modules/RTC/TrackStreamingStatus.js

+     * @param {Array<string>} enteringForwardedSources - The array of sourceName entering forwarded sources.
+     * @private
+     */
+    _onForwardedSourcesChanged(leavingForwardedSources = [], enteringForwardedSources = []) {


Ah because the timestamps will not be accurate since this instance may disappear. Lets try to think about it... Maybe we need to store the timestamps somewhere (the place which emits the forwarded sources changed event) and then expose that via a getter? Or better have a static part/helper class of TrackStreamingStatus which would keep track of the timestamps (mostly because they are not used by any other logic) ?

JitsiTrackEvents.js

jallamsetty1

Great work @WillLiang918. Left few minor comments. One complaint that I have is there is a lot of code duplication.

JitsiConferenceEvents.js

modules/RTC/TrackStreamingStatus.js

modules/connectivity/TrackStreamingStatus.ts

types/hand-crafted/JitsiConference.d.ts

tsconfig.json

modules/connectivity/TrackStreamingStatus.ts

JitsiConference.js

JitsiTrackEvents.js

modules/connectivity/TrackStreamingStatus.ts

paweldomas · 2022-02-04T03:00:34Z

modules/RTC/TrackStreamingStatus.js

+     * @param {Array<string>} enteringForwardedSources - The array of sourceName entering forwarded sources.
+     * @private
+     */
+    _onForwardedSourcesChanged(leavingForwardedSources = [], enteringForwardedSources = []) {


We can actually make it so that the JitsiRemoteTrack holds the timestamps when it entered/left the forwarded sources and this can happen any time the RTC module emits the forwarded sources event?

JitsiConference.js

modules/RTC/JitsiRemoteTrack.js

jallamsetty1 · 2022-02-10T15:14:22Z

Jenkins please add to whitelist

jallamsetty1 · 2022-02-10T15:17:20Z

Jenkins test this please.

paweldomas · 2022-02-18T02:29:26Z

JitsiConferenceEvents.js

+ * @param {Array<string>|null} enteringForwardedSources the sourceNames of all the tracks which are entering forwarded
+ * sources
+ */
+export const FORWARDED_SOURCES_CHANGED = 'conference.forwardedSourcesChanged';


Sorry that I wasn't paying attention to this earlier, but we need to add a getter to JitsiConference.getForwardedSources(), so that a LJM user can get the current state without having to register the even listener from the start.

I see, it does make sense to allow us to access the current forwarded sources state without having to register a listener. Is this more of a best practice to expose this on the conference ~~do you see an optimization we can make~~?

paweldomas · 2022-02-18T02:37:15Z

JitsiTrackEvents.js

@@ -43,3 +43,16 @@ export const NO_DATA_FROM_SOURCE = 'track.no_data_from_source';
 * the microphone that is currently selected.
 */
 export const NO_AUDIO_INPUT = 'track.no_audio_input';
+
+/**
+ * Event fired when we detect local problem with the video track.


Lets reword it to Event fired whenever video track's streaming changes

paweldomas · 2022-02-18T02:43:38Z

modules/connectivity/TrackStreamingStatus.ts

+import Statistics from '../statistics/statistics';
+
+/** Track streaming statuses. */
+type TrackStreamingStatus = 'active' | 'inactive' | 'interrupted' | 'restoring';


Is it possible to make this an enum and not have the TrackStreamingStatusMapType?

paweldomas · 2022-02-18T02:46:13Z

modules/RTC/JitsiRemoteTrack.js

+    /**
+     * @returns {Boolean} Whether this is a muted video track.
+     */
+    isVideoMuted() {


Can we remove this method? In place where it's used it should be enough to check isMuted()

paweldomas · 2022-02-18T02:47:56Z

modules/RTC/JitsiRemoteTrack.js

+    /**
+     * Disposes trackStreamingStatusImpl and clears trackStreamingStatus
+     */
+    disposeTrackStreamingStatus() {


can we add underscore to this method's name? JitsiRemoteTrack is exported by lib-jitsi-meet and this would look like a public method

paweldomas · 2022-02-18T02:48:35Z

modules/RTC/JitsiRemoteTrack.js

+    /**
+     * Clears the timestamp of when the track entered forwarded sources.
+     */
+    clearEnteredForwardedSourcesTimestamp() {


please add _

paweldomas · 2022-02-18T02:48:53Z

modules/RTC/JitsiRemoteTrack.js

+     *
+     * @param {number} timestamp the time in millis
+     */
+    setEnteredForwardedSourcesTimestamp(timestamp) {


please add _

paweldomas · 2022-02-18T02:49:02Z

modules/RTC/JitsiRemoteTrack.js

+     *
+     * @returns {number} the time in millis
+     */
+    getEnteredForwardedSourcesTimestamp() {


please add _

paweldomas · 2022-02-18T02:52:39Z

modules/connectivity/TrackStreamingStatus.ts

+    [key: string]: TrackStreamingStatus
+}
+
+type VideoType = 'camera' | 'desktop';


is there a place outside of TrackStreamingStatus where this type can be defined? Maybe create a dedicated .ts file? If that's too much work please mark this with a TODO comment that it should be removed from here once service/RTC/VideoType.js is converted to type script.

Why can't we import VideoType from service/RTC/VideoType.js here directly ?
ETA: What I meant was that we already have auto-generated type for VideoType. Can we use that instead of defining it here again ?

We definitely can! I found that type after Pawel pointed this out. I'll push another set of changes out shortly after testing it locally

Wait do you mean the VideoType enum in types/hand-crafted/services/RTC/VideoType.d.ts or the VideoType object in VideoType.js?

I'm going to take a quick look to see if we can do something similar to the recent changes to /JitsiConferenceEvents.ts

I'm going to revert my changes and adding a TODO to use the VideoType enum in https://github.com/jitsi/lib-jitsi-meet/pull/1916/files

Sorry had to take half day off yesterday and was away. Yes, I was talking about VideoType from hand-crafted dir. It makes sense to use the VideoType enum from the newly created TS files after they get merged.

paweldomas · 2022-02-18T02:53:57Z

modules/connectivity/TrackStreamingStatus.ts

+ */
+const DEFAULT_RESTORING_TIMEOUT = 10000;
+
+export const TrackStreamingStatusMap: TrackStreamingStatusMapType = {


as per the other comment, if TrackStreamingStatusMapType was replace with TrackStreamingStatus enum, will it be possible to get rid of this map?

I'll take another look into this. I think I was having issues when I first tried it because TrackStreamingStatusMap was being imported into non .ts files where enums aren't supported

update JitsiRemoteTrack to init and dispose TrackStreamingStatus stop emitting LASTN_ENDPOINT_CHANGED event when source name signaling is enabled convert TrackStreamingStatus class to typescript

paweldomas

It's looking good! Left just one comment about JS docs

JitsiConferenceEvents.ts

WillLiang918 commented Jan 25, 2022

View reviewed changes

modules/RTC/BridgeChannel.js Show resolved Hide resolved

WillLiang918 commented Jan 25, 2022

View reviewed changes

modules/RTC/TrackStreamingStatus.js Outdated Show resolved Hide resolved