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

Receive streams in WebRTC (and MediaStreamTrack support) #23342

Merged
merged 12 commits into from May 9, 2019

Conversation

Projects
None yet
6 participants
@Manishearth
Copy link
Member

commented May 7, 2019

This adds the ontrack event handler to webrtc, and all the MediaStreamTrack stuff necessary to make it work.

WebRTC has the ability to group media tracks into streams using MSIDs, but I haven't yet figured out how to do this. For now, ontrack should work.

This should be complete, but it hasn't yet been tested (hence the WIP)

r? @ferjm or @jdm


This change is Reviewable

@Manishearth Manishearth requested review from jdm and ferjm May 7, 2019

@highfive

This comment has been minimized.

Copy link

commented May 7, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/MediaStreamTrack.webidl, components/script/dom/webidls/RTCPeerConnection.webidl, components/script/dom/mod.rs, components/script/dom/rtctrackevent.rs, components/script/dom/rtcpeerconnection.rs and 7 more
  • @KiChjang: components/script/dom/webidls/MediaStreamTrack.webidl, components/script/dom/webidls/RTCPeerConnection.webidl, components/script/dom/mod.rs, components/script/dom/rtctrackevent.rs, components/script/dom/rtcpeerconnection.rs and 7 more
@highfive

This comment has been minimized.

Copy link

commented May 7, 2019

warning Warning warning

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

@Manishearth Manishearth force-pushed the Manishearth:webrtc-streams branch from fa6c5c9 to bccac96 May 7, 2019

@jdm

jdm approved these changes May 7, 2019

Copy link
Member

left a comment

Nice!

}

impl RTCTrackEventMethods for RTCTrackEvent {
// https://html.spec.whatwg.org/multipage/#dom-RTCTrackEvent-track

This comment has been minimized.

Copy link
@jdm

jdm May 7, 2019

Member

Unlikely URL.

@jdm jdm added the S-fails-tidy label May 7, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

omg it works.

image

@Manishearth Manishearth marked this pull request as ready for review May 8, 2019

@Manishearth Manishearth changed the title [WIP] Receive streams in WebRTC (and MediaStreamTrack support) Receive streams in WebRTC (and MediaStreamTrack support) May 8, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@Manishearth Manishearth force-pushed the Manishearth:webrtc-streams branch from 189d34c to 60332d3 May 8, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@ferjm thanks for all the work in #23157, it Just Works™ here

@ferjm

ferjm approved these changes May 8, 2019

Copy link
Member

left a comment

r=me with the comments addressed. Thanks!

self.add_track(track)
}

/// https://w3c.github.io/mediacapture-main/#dom-mediastream-addtrack

This comment has been minimized.

Copy link
@ferjm

ferjm May 8, 2019

Member

This should be https://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack

/// https://w3c.github.io/mediacapture-main/#dom-mediastream-clone
fn Clone(&self) -> DomRoot<MediaStream> {
let new = MediaStream::new(&self.global());
for track in &*self.tracks.borrow_mut() {

This comment has been minimized.

Copy link
@ferjm

ferjm May 8, 2019

Member

I think this does not need to be a mutable borrow.

@Manishearth Manishearth force-pushed the Manishearth:webrtc-streams branch from 60332d3 to b4ddf68 May 8, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@bors-servo r=ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

📌 Commit b4ddf68 has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

⌛️ Testing commit b4ddf68 with merge 13973ea...

bors-servo added a commit that referenced this pull request May 9, 2019

Auto merge of #23342 - Manishearth:webrtc-streams, r=ferjm
Receive streams in WebRTC (and MediaStreamTrack support)

This adds the `ontrack` event handler to webrtc, and all the `MediaStreamTrack` stuff necessary to make it work.

WebRTC has the ability to group media tracks into streams using MSIDs, but I haven't yet figured out how to do this. For now, `ontrack` should work.

This _should_ be complete, but it hasn't yet been tested (hence the WIP)

r? @ferjm or @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/23342)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

💔 Test failed - linux-rel-css

@CYBAI

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

{
    "status": "FAIL", 
    "group": "default", 
    "message": "assert_true: If this is failing: DANGER, are you sure you want to expose the new interface RTCTrackEvent to all webpages as a property on the global? Do not make a change to this file without review from jdm or Ms2ger for that specific change! expected true got false", 
    "stack": "test_interfaces/<@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:70:7\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1587:20\ntest@http://web-platform.test:8000/resources/testharness.js:544:21\ntest_interfaces@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:2:3\n@http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:13:1\n", 
    "subtest": "Interfaces exposed on the window", 
    "test": "/_mozilla/mozilla/interfaces.html", 
    "line": 310798, 
    "action": "test_result", 
    "expected": "PASS"
}
@Manishearth

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Forgot to pref-gate

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@bors-servo r=ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

📌 Commit 7d5e493 has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

⌛️ Testing commit 7d5e493 with merge 17590fd...

bors-servo added a commit that referenced this pull request May 9, 2019

Auto merge of #23342 - Manishearth:webrtc-streams, r=ferjm
Receive streams in WebRTC (and MediaStreamTrack support)

This adds the `ontrack` event handler to webrtc, and all the `MediaStreamTrack` stuff necessary to make it work.

WebRTC has the ability to group media tracks into streams using MSIDs, but I haven't yet figured out how to do this. For now, `ontrack` should work.

This _should_ be complete, but it hasn't yet been tested (hence the WIP)

r? @ferjm or @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/23342)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@bors-servo bors-servo merged commit 7d5e493 into servo:master May 9, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@Manishearth Manishearth deleted the Manishearth:webrtc-streams branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.