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

Initial webrtc and getUserMedia DOM support #22780

Merged
merged 26 commits into from Jan 31, 2019

Conversation

Projects
None yet
7 participants
@Manishearth
Copy link
Member

Manishearth commented Jan 29, 2019

This is able to reach the point where connections are properly negotiated and ready to exchange streams.

The toJSON() stuff doesn't work yet, so most example code will need to be tweaked to manually construct JSON first before sending SDP and ICE messages over websockets. I'll add support for this soon. (This may need webidl tweaks to support [Default] and toJSON())

For some reason I haven't yet figured out, connections are one-way, Servo is able to receive streams but the other end doesn't see the streams Servo sends. I don't think this is due to servo/media#191, but that bug is making it harder to test.

This implementation simply drops streams that it receives, without connecting them up to any output elements, since servo-media-player doesn't yet have mediastream support.

Since servo can neither effectively send nor receive streams this implementation isn't useful yet, however it is getting large and I figured I'd get it reviewed and landed early as a base.

r? @jdm


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 29, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/rtcsessiondescription.rs, components/script/dom/webidls/RTCPeerConnection.webidl, components/script/dom/navigator.rs, components/script/dom/mod.rs, components/script/dom/rtcpeerconnectioniceevent.rs and 10 more
  • @KiChjang: components/script/dom/rtcsessiondescription.rs, components/script/dom/webidls/RTCPeerConnection.webidl, components/script/dom/navigator.rs, components/script/dom/mod.rs, components/script/dom/rtcpeerconnectioniceevent.rs and 10 more
@highfive

This comment has been minimized.

Copy link

highfive commented Jan 29, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 29, 2019

added toJSON support. Ideally [Default] would automatically work and give it to us for free for RTCSessionDescription, but writing it manually here isn't hard since we already support RTCSessionDescriptionInit.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 29, 2019

./components/script/dom/webidls/RTCPeerConnection.webidl:91: no newline at EOF

@Manishearth Manishearth force-pushed the Manishearth:webrtc branch from 70ed3f4 to 2007463 Jan 29, 2019


partial interface Navigator {
// [SameObject, SecureContext]
readonly attribute MediaDevices mediaDevices;

This comment has been minimized.

@jdm

jdm Jan 29, 2019

Member

Pref-gate this.


partial interface MediaDevices {
// MediaTrackSupportedConstraints getSupportedConstraints();
Promise<MediaStream> getUserMedia(optional MediaStreamConstraints constraints);

This comment has been minimized.

@jdm

jdm Jan 29, 2019

Member

Pref.

This comment has been minimized.

@Manishearth

Manishearth Jan 29, 2019

Author Member

no need, MediaDevices is pref-gated already


[Constructor(DOMString type, optional RTCPeerConnectionIceEventInit eventInitDict),
Exposed=Window]
interface RTCPeerConnectionIceEvent : Event {

This comment has been minimized.

@jdm

jdm Jan 29, 2019

Member

Pref.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 29, 2019

This all looks fine for the initial code dump, otherwise.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 30, 2019

⌛️ Testing commit 0280829 with merge bee8969...

bors-servo added a commit that referenced this pull request Jan 30, 2019

Auto merge of #22780 - Manishearth:webrtc, r=jdm
Initial webrtc and getUserMedia DOM support

This is able to reach the point where connections are properly negotiated and ready to exchange streams.

<s>The `toJSON()` stuff doesn't work yet, so most example code will need to be tweaked to manually construct JSON first before sending SDP and ICE messages over websockets. I'll add support for this soon. (This may need webidl tweaks to support `[Default]` and `toJSON()`)</s>

For some reason I haven't yet figured out, connections are one-way, Servo is able to receive streams but the other end doesn't see the streams Servo sends. I don't think this is due to servo/media#191, but that bug is making it harder to test.

This implementation simply drops streams that it receives, without connecting them up to any output elements, since servo-media-player doesn't yet have mediastream support.

Since servo can neither effectively send nor receive streams this implementation isn't useful yet, however it is getting large and I figured I'd get it reviewed and landed early as a base.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22780)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 30, 2019

💔 Test failed - status-taskcluster

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 30, 2019

Looks like our android gst libraries don't have any gst_webrtc or sdp symbols.

But the pkg-config file says 1.14.3 so I'm not sure how this went wrong. @sdroege any idea what could be going on here?

The library being used is at https://servo-deps.s3.amazonaws.com/gstreamer/gstreamer-armeabi-v7a-1.14.3-20181105-103937.zip

@sdroege

This comment has been minimized.

Copy link

sdroege commented Jan 31, 2019

But the pkg-config file says 1.14.3 so I'm not sure how this went wrong. @sdroege any idea what could be going on here?

I'm not sure if you're still using @ferjm's libgstreamer_android_gen thingy, but if you do the problem is here: https://github.com/servo/libgstreamer_android_gen/blob/536676168014b60d59ce8a0d1ef4003632d9c941/jni/Android.mk#L31

You have to add gstreamer-sdp-1.0 and gstreamer-webrtc-1.0 to that line to include all of the two libraries in the generated libgstreamer_android.so.

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 31, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 31, 2019

@ferjm could you have a look and regenerate binaries if we need to?

@sdroege

This comment has been minimized.

Copy link

sdroege commented Jan 31, 2019

@sdroege i think we use https://github.com/servo/servo/wiki/How-to-generate-GStreamer-binaries-for-CI now ? which uses meson?

I believe that's only for the CI, not for the Android builds?

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Jan 31, 2019

@ferjm could you have a look and regenerate binaries if we need to?

Sure.

I believe that's only for the CI, not for the Android builds?

That's right.

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Jan 31, 2019

I built this branch locally with the new binaries and I was able to launch Servo (I wasn't able before updating the binaries as I was getting a dlopen failed: cannot locate symbol "gst_webrtc_session_description_free" error)

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 31, 2019

📌 Commit 351723e has been approved by jdm

@@ -441,7 +441,7 @@ def build(self, target=None, release=False, dev=False, jobs=None,
# Build the name of the package containing all GStreamer dependencies
# according to the build target.
gst_lib = "gst-build-{}".format(self.config["android"]["lib"])
gst_lib_zip = "gstreamer-{}-1.14.3-20181105-103937.zip".format(self.config["android"]["lib"])
gst_lib_zip = "gstreamer-{}-1.14.3-20190131-153818.zip".format(self.config["android"]["lib"])

This comment has been minimized.

@sdroege

sdroege Jan 31, 2019

Not sure if you did that, but you also should ensure that (in addition to the libraries themselves) the relevant plugins you're going to use are also included. For WebRTC that are going to be quite a few.

This comment has been minimized.

@ferjm

ferjm Jan 31, 2019

Member

Hmm, I only added gstreamer-sdp-1.0 and gstreamer-webrtc-1.0 to the list of extra deps, as you suggested before. I'm not sure which plugins we need for WebRTC.

This comment has been minimized.

@sdroege

sdroege Jan 31, 2019

You added those libraries to the list of libraries, nothing with plugins. The plugins are in a different variable.

I'm not sure what you all need, would have to check that too. At least webrtc, rtp, rtpmanager, vpx, opus and nice.

This comment has been minimized.

@nirbheek

nirbheek Jan 31, 2019

You need the following plugins: opus vpx nice webrtc dtls srtp rtpmanager. sctp too if/when you use data channels. I would recommend adding a runtime check that queries the gstreamer registry for the plugins that are available, something like this: https://github.com/centricular/gstwebrtc-demos/blob/master/sendrecv/gst-rust/src/main.rs#L113

This comment has been minimized.

@ferjm

ferjm Jan 31, 2019

Member

Ok, thanks!

We already have opus and vpx.

IIUC adding GSTREAMER_PLUGINS_NET to https://github.com/servo/libgstreamer_android_gen/blob/536676168014b60d59ce8a0d1ef4003632d9c941/jni/Android.mk#L30 should give us webrtc dtls srtp and rtpmanager.

I don't see nice and sctp listed in share/gst-android/ndk-build/plugins.mk for 1.14.3.

This comment has been minimized.

@nirbheek

nirbheek Jan 31, 2019

sctp is not available in 1.14.x (was added in 1.15.x and will be available in 1.16.0), but nice being missing is a bug. I'll fix it our build aggregator, but in the meantime you can add it to the GSTREAMER_PLUGINS_NET list in plugins.mk by hand for now. The plugin should already be packaged correctly.

This comment has been minimized.

@nirbheek

nirbheek Feb 1, 2019

The fix for nice has been merged into 1.14 and will be available in 1.14.5, which should be out in the next few weeks.

This comment has been minimized.

@ferjm

ferjm Feb 1, 2019

Member

Great. Thank you! :)

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 31, 2019

⌛️ Testing commit 351723e with merge 6b64842...

bors-servo added a commit that referenced this pull request Jan 31, 2019

Auto merge of #22780 - Manishearth:webrtc, r=jdm
Initial webrtc and getUserMedia DOM support

This is able to reach the point where connections are properly negotiated and ready to exchange streams.

<s>The `toJSON()` stuff doesn't work yet, so most example code will need to be tweaked to manually construct JSON first before sending SDP and ICE messages over websockets. I'll add support for this soon. (This may need webidl tweaks to support `[Default]` and `toJSON()`)</s>

For some reason I haven't yet figured out, connections are one-way, Servo is able to receive streams but the other end doesn't see the streams Servo sends. I don't think this is due to servo/media#191, but that bug is making it harder to test.

This implementation simply drops streams that it receives, without connecting them up to any output elements, since servo-media-player doesn't yet have mediastream support.

Since servo can neither effectively send nor receive streams this implementation isn't useful yet, however it is getting large and I figured I'd get it reviewed and landed early as a base.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22780)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 31, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: jdm
Pushing 6b64842 to master...

@bors-servo bors-servo merged commit 351723e into servo:master Jan 31, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@bors-servo bors-servo referenced this pull request Jan 31, 2019

Merged

Make the signal handler exit immediately #22259

4 of 4 tasks complete
@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 31, 2019

Thanks everyone!

@Manishearth Manishearth deleted the Manishearth:webrtc branch Jan 31, 2019

bors-servo added a commit that referenced this pull request Feb 4, 2019

Auto merge of #22806 - ferjm:webrtc.gst.plugins, r=jdm
Update gstreamer binaries for Android with webrtc related plugins

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

I updated the binaries for Android with the required plugins for WebRTC as suggested [here](#22780 (comment))

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22806)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Feb 5, 2019

Auto merge of #22806 - ferjm:webrtc.gst.plugins, r=jdm
Update gstreamer binaries for Android with webrtc related plugins

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

I updated the binaries for Android with the required plugins for WebRTC as suggested [here](#22780 (comment))

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22806)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment