Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

watchRTC as a feature for Jitsi meet #2311

Merged
merged 4 commits into from
Jul 19, 2023
Merged

Conversation

subhamcyara
Copy link
Contributor

Following PR contains changes required to add watchRTC as a module within lib jitsi meet

This involves adding

  • watchRTC SDK package (npm i @testrtc/watchrtc-sdk).
  • watchRTC SDK is initialized within Statistics.init to proxy WebRTC functions such as GUM and RTCPeerConnection.
  • watchRTC SDK then starts collecting required stats when the conference begins. Here we check if the user has not configured room name or user/peer name, if not, then use relevant data available from Jitsi.
  • watchRTC SDK does not require to function for react native.
  • checks implemented to run watchRTC SDK when
    -- watchRTC is enabled
    -- rtcstats must be disabled
    -- analytics should be enabled
    -- third party requests are not disabled

@jitsi-jenkins
Copy link

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 :(.

return;
}

// watchRTC "proxies" WebRTC functions such as GUM and RTCPeerConnection by rewriting the global
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer the case. We used to store window.PeerConnection but we no longer do. Or did we miss a spot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand. Is this about the verbose comment written and behaviour of the lib jitsi meet written within the comment ?!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's about the comment.

package.json Outdated
@@ -20,6 +20,7 @@
"@jitsi/logger": "2.0.0",
"@jitsi/sdp-interop": "git+https://github.com/jitsi/sdp-interop#3d49eb4aa26863a3f8d32d7581cdb4321244266b",
"@jitsi/sdp-simulcast": "0.4.0",
"@testrtc/watchrtc-sdk": "^1.36.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the exact version, aka, leave ^ out.

@saghul
Copy link
Member

saghul commented Jul 11, 2023

Please see the test failure, we are going to need to bump the library size.

@subhamcyara
Copy link
Contributor Author

@saghul , I have tried to address all review points. Please let me know your feedback.
Thanks

@saghul
Copy link
Member

saghul commented Jul 11, 2023

The test failures seem legit. I think we should gate starting of watchRTC if it's not enabled or no room name was provided.

@subhamcyara
Copy link
Contributor Author

I believe the failure should resolve now. For context, there are checks to see if watchRTC is enabled or not. The idea is to use the roomName and/or peerName from the watchRTC config and if not supplied, we can use what jitsi provides. If we get nothing from jitsi too, watchRTC SDK would handle it.

saghul
saghul previously approved these changes Jul 12, 2023
Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @andrei-gavrilescu can you PTAL too?

@andrei-gavrilescu
Copy link
Contributor

Looks good 👍

@saghul
Copy link
Member

saghul commented Jul 18, 2023

@subhamcyara Can you please rebase? Looks like there is some small conflict. Once solved it's good to merge.

@subhamcyara
Copy link
Contributor Author

Hi, thanks for the review here. I believe we might also need to take a look at jitsi/jitsi-meet#13527 for changes related to JM.

@saghul
Copy link
Member

saghul commented Jul 19, 2023

Jenkins please test this please.

@saghul saghul merged commit 7c0a112 into jitsi:master Jul 19, 2023
1 check passed
@subhamcyara subhamcyara deleted the add-watchrtc branch July 21, 2023 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants