watchRTC as a feature for Jitsi meet #13527
Conversation
|
Hi, thanks for your contribution! |
|
Can you PTAL @andrei-gavrilescu ? |
|
Looks good to me. One thing to consider is that lib-jitsi-meet users won't have this functionality, that's why we're currently moving rtcstats to lib-jitsi-meet as well see: jitsi/lib-jitsi-meet#2305 and #13508 |
| * Class that controls the watchRTC flow, because it overwrites and proxies global function it should only be | ||
| * initialized once. | ||
| */ | ||
| class watchRTCHandler { |
There was a problem hiding this comment.
nit. use PascalCase for class name. Also I think there are a couple of linting errors around, consider running the linter.
change file name to follow pascal casing
| switch (action.type) { | ||
| case LIB_WILL_INIT: { | ||
| if (isRtcstatsEnabled(state)) { | ||
| logger.error("Cannot initialize WatchRTC when RTCStats is enabled."); |
There was a problem hiding this comment.
This will be printed for all users using rtcstats, isn't it?
If this is the case, please drop it.
Agreed. It should also be less code I think. @subhamcyara can you pl move it to LJM, following the lead of rtcstats? |
@saghul |
|
While that is technically correct we've seen lib-Jitsi-meet customers ask for a builtin stats system since they want to leverage it. Since we went in that direction it makes sense that all stats implementations follow suit. |
|
Yes totally agree. I would follow the lead of how it is done with rtcstats and modify. |
|
@saghul @andrei-gavrilescu |
|
Thank you! |
|
LGTM! |
|
@saghul Can we remove the error log we keep printing after this PR: I was commenting about it earlier but seems this was not fixed. |
|
Actually the print is in ljm PR |
|
jitsi/lib-jitsi-meet#2325 |
Following PR contains changes required to add watchRTC as a feature for jitsi meet.
This involves adding