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

Add <audio> and <video> player backends #21543

Merged
merged 38 commits into from Oct 9, 2018

Conversation

9 participants
@ceyusa
Contributor

ceyusa commented Aug 30, 2018

These patches enables audio and video playing inside Servo.

It is bit hackish way to enable it, thus the purpose of this pull request is for an early request for comments.

It is tested with the current servo-media GStreamer backend in Linux.

The produced layout is not correct, since the elements after the video seems to be stacked behind, and the same with the scrolling bars.

There is no JavaScript interface yet, neither controls.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #6711

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Aug 30, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

@highfive

This comment has been minimized.

highfive commented Aug 30, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/lib.rs, components/script/Cargo.toml, components/constellation/pipeline.rs, components/script/dom/node.rs, components/script/dom/mediaframerenderer.rs and 4 more
  • @cbrewster: components/constellation/pipeline.rs
  • @KiChjang: components/script/lib.rs, components/script/Cargo.toml, components/script/dom/node.rs, components/script/dom/mediaframerenderer.rs, components/script/dom/htmlmediaelement.rs and 4 more
  • @paulrouget: components/constellation/pipeline.rs
  • @emilio: components/layout/fragment.rs, components/layout/display_list/builder.rs, components/layout/construct.rs
@highfive

This comment has been minimized.

highfive commented Aug 30, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout and script code, but no tests are modified. Please consider adding a test!

@ceyusa ceyusa changed the title from RFC: add video player to [WIP] add video player Aug 30, 2018

@ferjm ferjm force-pushed the ceyusa:wip-player branch from f1dd8ed to ba0c37f Aug 31, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 1, 2018

☔️ The latest upstream changes (presumably #21509) made this pull request unmergeable. Please resolve the merge conflicts.

@ferjm ferjm force-pushed the ceyusa:wip-player branch 2 times, most recently from 9a4d088 to 3d3a86d Sep 3, 2018

@Manishearth

This comment has been minimized.

Member

Manishearth commented Sep 5, 2018

I'm unable to get this to render at all -- can you share the testcase you're using? I'm just using the one from W3schools since it's very simple.

@ferjm

This comment has been minimized.

Member

ferjm commented Sep 5, 2018

r? @nox could you take a look at the DOM changes, please?

r? @Manishearth @mrobinson could you review the layout changes, please?

Thanks!

@highfive highfive assigned nox and unassigned mbrubeck Sep 5, 2018

@nox

This comment has been minimized.

Member

nox commented Sep 5, 2018

@highfive highfive assigned Manishearth and unassigned nox Sep 5, 2018

@ferjm

This comment has been minimized.

Member

ferjm commented Sep 5, 2018

I'm unable to get this to render at all -- can you share the testcase you're using? I'm just using the one from W3schools since it's very simple.

I already replied on IRC, but for reference, in case anyone else wants to try this, this example should work.

Cargo.toml Outdated
@@ -27,3 +27,5 @@ opt-level = 3
#
# [patch."https://github.com/servo/<repository>"]
# <crate> = { path = "/path/to/local/checkout" }
[patch."https://github.com/servo/media"]
servo-media = { git = "https://github.com/ferjm/media", branch = "framerenderer" }

This comment has been minimized.

@ferjm

ferjm Sep 5, 2018

Member

This should be gone once #21325 is merged

@@ -570,6 +570,9 @@ pub struct ScriptThread {
/// The Webrender Document ID associated with this thread.
webrender_document: DocumentId,
/// FIXME(victor):

This comment has been minimized.

@ferjm

ferjm Sep 5, 2018

Member

@ceyusa could you elaborate more on what needs to be fixed here exactly, please?

This comment has been minimized.

@ceyusa

ceyusa Sep 5, 2018

Contributor

I used the wrong tag. It is not a fixme but a xxx, perhaps. What I mean is it would be nice to rethink this design. Perhaps it is better to create a proxy object to keep the RenderApiSender for the servo-media, rather than pass it all along from constellation to document to html element.

This comment has been minimized.

@Manishearth

Manishearth Sep 17, 2018

Member

I think process isolation wise script should not have access to this, cc @asajeffrey

A proxy approach would be nice. Ideally the network task would directly talk to WR, maybe via constellation

@@ -559,6 +561,8 @@ pub struct InitialScriptState {
pub webvr_chan: Option<IpcSender<WebVRMsg>>,
/// The Webrender document ID associated with this thread.
pub webrender_document: DocumentId,
/// FIXME(victor): The Webrender API sender in this constellation's pipeline

This comment has been minimized.

@ferjm

ferjm Sep 5, 2018

Member

@ceyusa what needs to be fixed here?

This comment has been minimized.

@ceyusa

ceyusa Sep 5, 2018

Contributor

The same I commented above, not a fixme but a request to rethink this approach,

@@ -602,6 +690,7 @@ impl HTMLMediaElement {
.. RequestInit::default()
};
self.setup_media_player();

This comment has been minimized.

@ferjm

ferjm Sep 5, 2018

Member

I suspect that this is not the best place to setup the media player. I need to properly read the spec, but I would say that we should be setting it up a bit earlier, and ideally handling any potential setup failure.

@@ -269,6 +352,9 @@ impl HTMLMediaElement {
// Step 2.3.2.
this.upcast::<EventTarget>().fire_event(atom!("pause"));
//FIXME(victor)
//this.player.pause();

This comment has been minimized.

@ferjm

ferjm Sep 5, 2018

Member

This can be uncommented once we update servo-media to its latest revision (after #21325 lands)

@Manishearth

This comment has been minimized.

Member

Manishearth commented Sep 5, 2018

Hm, so it's not that they're stacked behind, it's that they seem to get laid out as 0x0 boxes at (0,0). Curious.

@ferjm ferjm force-pushed the ceyusa:wip-player branch from b246941 to 79035f4 Sep 13, 2018

@ferjm ferjm removed the S-needs-rebase label Sep 13, 2018

@Manishearth

The DOM side approach looks okay, though we might want to rethink the sender stuff.

Any progress on figuring out what's going on with layout? cc @mbrubeck @mrobinson

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 18, 2018

☔️ The latest upstream changes (presumably #21723) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive removed the S-tests-failed label Oct 9, 2018

@ferjm

This comment has been minimized.

Member

ferjm commented Oct 9, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 9, 2018

⌛️ Trying commit 7e6661d with merge 3b153af...

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21543 - ceyusa:wip-player, r=<try>
Add <audio> and <video> player backends

These patches enables audio and video playing inside Servo.

It is bit hackish way to enable it, thus the purpose of this pull request is for an early request for comments.

It is tested with the current servo-media GStreamer backend in Linux.

~~The produced layout is not correct, since the elements after the video seems to be stacked behind, and the same with the scrolling bars.~~

~~There is no JavaScript interface yet~~, neither controls.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [X] These changes fix #6711

<!-- 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/21543)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 9, 2018

💔 Test failed - mac-rel-wpt3

@ferjm

This comment has been minimized.

Member

ferjm commented Oct 9, 2018

  ▶ CRASH [expected OK] /html/dom/elements/the-innertext-idl-attribute/getter.html
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 18.1.0-devel
  │ called `Result::unwrap()` on an `Err` value: ChannelClosedError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(2), index: PipelineIndex(7) }, at libcore/result.rs:1009)
  │ 2018-10-09 05:04:32.098 servo[85591:104129169] Metadata.framework [Error]: couldn't get the client port
  │ stack backtrace:
  │    0:        0x1069d117e - __ZN9backtrace9backtrace5trace17h7e39d6bd7c678f2eE
  │    1:        0x1069d1e8c - __ZN72_$LT$backtrace..capture..Backtrace$u20$as$u20$core..default..Default$GT$7default17ha5434a657e6f38c1E
  │    2:        0x1069d1f0d - __ZN9backtrace7capture9Backtrace3new17h027ac9deb1d86dcfE
  │    3:        0x103ebc5f2 - __ZN5servo4main28_$u7b$$u7b$closure$u7d$$u7d$17he67dbfce30c0cf9fE
  │    4:        0x1069f6838 - __ZN3std9panicking20rust_panic_with_hook17h14490a1041ee4235E
  │    5:        0x1069f634c - __ZN3std9panicking18continue_panic_fmt17h7beddad0ba45f4eaE
  │    6:        0x1069f6238 - _rust_begin_unwind
  │    7:        0x106a13d01 - __ZN4core9panicking9panic_fmt17hc2d37e7312d32a42E
  │    8:        0x104a5de6b - __ZN4core6result13unwrap_failed17hde0251e98d282c5cE
  │    9:        0x1047f520b - __ZN6script3dom8document8Document15set_quirks_mode17hb0e4b28a660531a2E
  │   10:        0x104aa72bd - __ZN71_$LT$html5ever..tree_builder..TreeBuilder$LT$Handle$C$$u20$Sink$GT$$GT$4step17h5b5c27613eac3c68E
  │   11:        0x104a17fb3 - __ZN125_$LT$html5ever..tree_builder..TreeBuilder$LT$Handle$C$$u20$Sink$GT$$u20$as$u20$html5ever..tokenizer..interface..TokenSink$GT$13process_token17heb4ee21f941c8e22E
  │   12:        0x104d536b2 - __ZN52_$LT$html5ever..tokenizer..Tokenizer$LT$Sink$GT$$GT$13process_token17he4a887dc1ccb4b8eE.llvm.7703268766650612569
  │   13:        0x104d5f142 - __ZN52_$LT$html5ever..tokenizer..Tokenizer$LT$Sink$GT$$GT$4step17h3f8979140582a039E
  │   14:        0x104d5afca - __ZN52_$LT$html5ever..tokenizer..Tokenizer$LT$Sink$GT$$GT$3run17hd00c861be46bd011E.llvm.7703268766650612569
  │   15:        0x104e82eeb - __ZN6script3dom11servoparser4html9Tokenizer4feed17h112165b7aed7ca0eE
  │   16:        0x10479d31c - __ZN6script3dom11servoparser11ServoParser13do_parse_sync17h0abe13de0b4e0d20E
  │   17:        0x104a307dd - __ZN14profile_traits4time7profile17h56bd06f49e294cf3E
  │   18:        0x10479d0d6 - __ZN6script3dom11servoparser11ServoParser10parse_sync17h40ec5210842cd6d2E.llvm.15457495438227833469
  │   19:        0x10479fee8 - __ZN93_$LT$script..dom..servoparser..ParserContext$u20$as$u20$net_traits..FetchResponseListener$GT$22process_response_chunk17h97bc4cee11ab4773E
  │   20:        0x104904ce5 - __ZN6script13script_thread12ScriptThread29handle_msg_from_constellation17hf41fce0ec6ee0376E
  │   21:        0x1048fe194 - __ZN6script13script_thread12ScriptThread11handle_msgs17hf1278d1c475732c2E.llvm.5337641549442363273
  │   22:        0x104a3ccf7 - __ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17h5cbb6512ca403ff6E
  │   23:        0x10429cfad - __ZN3std9panicking3try7do_call17h63359dbf63ea5718E.llvm.7079729101609438744
  │   24:        0x106a02dae - ___rust_maybe_catch_panic
  │   25:        0x104d52bf6 - __ZN50_$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$8call_box17hcc1a52b90f7d7a98E
  │   26:        0x1069da3eb - __ZN3std3sys4unix6thread6Thread3new12thread_start17hba53abd905cebc29E
  │   27:     0x7fffa187799c - __pthread_body
  │   28:     0x7fffa1877919 - __pthread_start
  │ ERROR 2018-10-09T09:04:32Z: servo: called `Result::unwrap()` on an `Err` value: ChannelClosedError
  └ Pipeline failed in hard-fail mode.  Crashing!

The crash looks unrelated to this PR.

@ferjm

This comment has been minimized.

Member

ferjm commented Oct 9, 2018

@bors-servo r=Manishearth

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 9, 2018

📌 Commit 7e6661d has been approved by Manishearth

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 9, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 9, 2018

@bors-servo bors-servo merged commit 7e6661d into servo:master Oct 9, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@bors-servo bors-servo referenced this pull request Oct 9, 2018

Open

Synchronize WebGL contexts with page rendering #21841

4 of 4 tasks complete

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=<try>
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=<try>
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=<try>
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=<try>
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=<try>
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=<try>
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Oct 9, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=<try>
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Oct 10, 2018

Auto merge of #21895 - Manishearth:gst-mac, r=jdm
Revert "Disable event_timeupdate_noautoplay test on mac"

Hopefully the builders now have the right gstreamer things.

From #21543 , needs servo/saltfs#898

<!-- 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/21895)
<!-- Reviewable:end -->

@ferjm ferjm referenced this pull request Oct 24, 2018

Merged

Update servo-media to fix intermittent timeouts #22014

3 of 3 tasks complete

@ferjm ferjm added this to Done in Media playback Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment