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

Implement HTMLMediaElement poster attribute #22399

Merged
merged 6 commits into from Jan 15, 2019

Conversation

Projects
None yet
6 participants
@ferjm
Copy link
Member

commented Dec 10, 2018

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22288
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Dec 10, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmlvideoelement.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/webidls/HTMLVideoElement.webidl
  • @KiChjang: components/script/dom/htmlvideoelement.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/webidls/HTMLVideoElement.webidl
@highfive

This comment has been minimized.

Copy link

commented Dec 10, 2018

warning Warning warning

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

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

⌛️ Trying commit 74b9d9e with merge 8de190f...

bors-servo added a commit that referenced this pull request Dec 11, 2018

Auto merge of #22399 - ferjm:poster.frame, r=<try>
WIP Implement HTMLMediaElement poster attribute

- [X] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #22288
- [ ] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

💔 Test failed - linux-rel-wpt

@ferjm ferjm force-pushed the ferjm:poster.frame branch from 74b9d9e to 909979b Dec 11, 2018

@ferjm ferjm force-pushed the ferjm:poster.frame branch from 909979b to e07be5c Dec 11, 2018

@ferjm ferjm changed the title WIP Implement HTMLMediaElement poster attribute Implement HTMLMediaElement poster attribute Dec 11, 2018

@ferjm ferjm force-pushed the ferjm:poster.frame branch from e07be5c to 026a5b9 Dec 11, 2018

@ferjm

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

r? @jdm

@highfive highfive assigned jdm and unassigned SimonSapin Dec 11, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

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

@jdm
Copy link
Member

left a comment

Can we add a reftest that compares a paused video with a poster attribute against a page with an image element?

Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlvideoelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
@jdm

jdm approved these changes Dec 21, 2018

Show resolved Hide resolved components/script/dom/htmlvideoelement.rs Outdated
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

⌛️ Testing commit 4e286ff with merge f759959...

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

Auto merge of #22399 - ferjm:poster.frame, r=jdm
Implement HTMLMediaElement poster attribute

- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22288
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

  ▶ TIMEOUT [expected OK] /resource-timing/resource_initiator_types.html
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 18.3.0-devel
  │ 0:00:00.054561787 124407 0x7f43a802e230 WARN                typefind gsttypefindelement.c:856:gst_type_find_get_extension:<typefind> could not find uri extension in appsrc://
  │ 0:00:00.074380640 124407 0x7f439c0030a0 WARN                typefind gsttypefindelement.c:856:gst_type_find_get_extension:<typefind> could not find uri extension in appsrc://
  │ 0:00:00.083655392 124407 0x7f43a802e230 WARN                 qtdemux qtdemux.c:3154:qtdemux_parse_trex:<qtdemux0> failed to find fragment defaults for stream 1
  │ 0:00:00.083719707 124407 0x7f439c0030a0 WARN                 qtdemux qtdemux.c:3154:qtdemux_parse_trex:<qtdemux1> failed to find fragment defaults for stream 1
  │ 0:00:00.083758015 124407 0x7f43a802e230 WARN                 qtdemux qtdemux.c:3154:qtdemux_parse_trex:<qtdemux0> failed to find fragment defaults for stream 2
  │ 0:00:00.083783753 124407 0x7f439c0030a0 WARN                 qtdemux qtdemux.c:3154:qtdemux_parse_trex:<qtdemux1> failed to find fragment defaults for stream 2
  │ 0:00:00.087888372 124407 0x7f43a802e230 WARN                 default oss4-property-probe.c:303:gst_oss4_property_probe_get_values:<oss4sink0> Can't open file descriptor to probe available devices: No such file or directory
  │ 0:00:00.088150220 124407 0x7f43a802e230 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<oss4sink0> error: Could not open audio device for playback.
  │ 0:00:00.087894828 124407 0x7f439c0030a0 WARN                 default oss4-property-probe.c:303:gst_oss4_property_probe_get_values:<oss4sink1> Can't open file descriptor to probe available devices: No such file or directory
  │ 0:00:00.088646959 124407 0x7f439c0030a0 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<oss4sink1> error: Could not open audio device for playback.
  │ 0:00:00.088755785 124407 0x7f439c0030a0 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<oss4sink1> error: system error: No such file or directory
  │ 0:00:00.088853397 124407 0x7f439c0030a0 WARN                 playbin gstplaybin2.c:4663:autoplug_select_cb:<playbin> Could not activate sink oss4sink
  │ 0:00:00.089216375 124407 0x7f43a802e230 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<oss4sink0> error: system error: No such file or directory
  │ 0:00:00.089400439 124407 0x7f43a802e230 WARN                 playbin gstplaybin2.c:4663:autoplug_select_cb:<playbin> Could not activate sink oss4sink
  │ 0:00:00.089864409 124407 0x7f43a802e230 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<osssink1> error: Could not open audio device for playback.
  │ 0:00:00.089974637 124407 0x7f43a802e230 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<osssink1> error: system error: No such file or directory
  │ 0:00:00.090166638 124407 0x7f43a802e230 WARN                 playbin gstplaybin2.c:4663:autoplug_select_cb:<playbin> Could not activate sink osssink
  │ 0:00:00.090508757 124407 0x7f439c0030a0 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<osssink0> error: Could not open audio device for playback.
  │ 0:00:00.090527999 124407 0x7f439c0030a0 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<osssink0> error: system error: No such file or directory
  │ 0:00:00.090583580 124407 0x7f439c0030a0 WARN                 playbin gstplaybin2.c:4663:autoplug_select_cb:<playbin> Could not activate sink osssink
  │ 0:00:00.116693011 124407 0x7f44641405e0 FIXME                    bin gstbin.c:4337:gst_bin_query: implement duration caching in GstBin again
  │ 0:00:00.168474247 124407 0x7f43b814c590 WARN                 default oss4-property-probe.c:303:gst_oss4_property_probe_get_values:<audiosink-actual-sink-oss4> Can't open file descriptor to probe available devices: No such file or directory
  │ 0:00:00.168546586 124407 0x7f43b814c590 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<audiosink-actual-sink-oss4> error: Could not open audio device for playback.
  │ 0:00:00.168555585 124407 0x7f43b814c590 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<audiosink-actual-sink-oss4> error: system error: No such file or directory
  │ 0:00:00.168652628 124407 0x7f43b814c590 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<audiosink-actual-sink-oss> error: Could not open audio device for playback.
  │ 0:00:00.168661041 124407 0x7f43b814c590 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<audiosink-actual-sink-oss> error: system error: No such file or directory
  │ 0:00:00.174282737 124407 0x7f446412f8f0 WARN                 default oss4-property-probe.c:303:gst_oss4_property_probe_get_values:<audiosink-actual-sink-oss4> Can't open file descriptor to probe available devices: No such file or directory
  │ 0:00:00.174468943 124407 0x7f446412f8f0 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<audiosink-actual-sink-oss4> error: Could not open audio device for playback.
  │ 0:00:00.174573609 124407 0x7f446412f8f0 WARN                oss4sink oss4-sink.c:514:gst_oss4_sink_open:<audiosink-actual-sink-oss4> error: system error: No such file or directory
  │ 0:00:00.174797488 124407 0x7f446412f8f0 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<audiosink-actual-sink-oss> error: Could not open audio device for playback.
  │ 0:00:00.174909927 124407 0x7f446412f8f0 WARN                     oss gstosssink.c:399:gst_oss_sink_open:<audiosink-actual-sink-oss> error: system error: No such file or directory
  │ 0:00:05.192981034 124407 0x7f43b814c590 WARN                   libav gstavauddec.c:630:gst_ffmpegauddec_drain:<avdec_aac0> send packet failed, could not drain decoder
  └ 0:00:05.197381213 124407 0x7f43840039e0 WARN                   libav gstavauddec.c:630:gst_ffmpegauddec_drain:<avdec_aac1> send packet failed, could not drain decoder
@ferjm

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

The last commit fixes the timeout. We need independent load blockers. r? @jdm

// <video poster="poster.png"></video>
// (which triggers no media load algorithm unless a explicit call to .load() is done)
// will block the document's load event forever.
let mut blocker = self.load_blocker.borrow_mut();

This comment has been minimized.

Copy link
@jdm

jdm Jan 14, 2019

Member

Does this still work correctly if we switch the poster attribute value while the poster is loading? I think we probably need to call LoadBlocker::terminate.

This comment has been minimized.

Copy link
@ferjm

ferjm Jan 14, 2019

Author Member

Good point. Done.

@jdm

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

📌 Commit 7633cab has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

⌛️ Testing commit 7633cab with merge 2cf9a00...

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

Auto merge of #22399 - ferjm:poster.frame, r=jdm
Implement HTMLMediaElement poster attribute

- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22288
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

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

Merged

Implement formdata event #22660

4 of 4 tasks complete

@bors-servo bors-servo merged commit 7633cab into servo:master Jan 15, 2019

4 checks passed

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

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

Merged

Fix brotli decoding #22616

4 of 4 tasks complete
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.