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 defaultPlaybackRate and playbackRate #22449

Merged
merged 2 commits into from Dec 28, 2018

Conversation

Projects
None yet
5 participants
@georgeroman
Copy link
Contributor

georgeroman commented Dec 13, 2018


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

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 13, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/HTMLMediaElement.webidl, components/script/dom/htmlmediaelement.rs
  • @KiChjang: components/script/dom/webidls/HTMLMediaElement.webidl, components/script/dom/htmlmediaelement.rs
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 13, 2018

r? @ferjm

@highfive highfive assigned ferjm and unassigned jdm Dec 13, 2018

/// https://html.spec.whatwg.org/multipage/#dom-media-playbackrate
fn SetPlaybackRate(&self, value: Finite<f64>) {
if *value != self.playbackRate.get() {
self.playbackRate.set(*value);

This comment has been minimized.

@ferjm

ferjm Dec 14, 2018

Member

Right now we are only storing and retrieving the playback rate value from this local member. We still need to pass this value to the actual player, using the API that you introduced in servo/media#168. You need to update servo-media (./mach cargo-update -p servo-media) and make use of the new API here.

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 14, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2018

⌛️ Trying commit e7e89ad with merge 34a45c2...

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

Auto merge of #22449 - georgeroman:implement_htmlmediaelement_playbac…
…k_rates, r=<try>

Implement HTMLMediaElement defaultPlaybackRate and playbackRate

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22293

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2018

💔 Test failed - status-taskcluster


let window = window_from_node(self);
let task_source = window.task_manager().media_element_task_source();
task_source.queue_simple_event(self.upcast(), atom!("ratechange"), &window);

This comment has been minimized.

@ferjm

ferjm Dec 14, 2018

Member

We could add a new queue_ratechange_event method, so we don't duplicate these lines.

This comment has been minimized.

@ferjm

ferjm Dec 14, 2018

Member

Or even better, since this is a pattern that is repeated all over this file, we could have a fn queue_media_event(&self, event: &str) method. But it's ok to do this in a follow-up.

This comment has been minimized.

@georgeroman

georgeroman Dec 14, 2018

Contributor

I chose the first method so as to keep the changes as self-contained as possible. Once this PR is done I'll refactor the whole file using the second method.

@georgeroman georgeroman force-pushed the georgeroman:implement_htmlmediaelement_playback_rates branch from e7e89ad to af829ee Dec 14, 2018

@ferjm
Copy link
Member

ferjm left a comment

Almost there :)

if *value != self.playbackRate.get() {
self.playbackRate.set(*value);
if let Err(e) = self.player.set_rate(*value) {
eprintln!("Could not set the playback rate {:?}", e);

This comment has been minimized.

@ferjm

ferjm Dec 16, 2018

Member

Use warn! instead of eprintln! here, please. (I know that there are other uses of eprintln! in this file, but we should switch all of them to warn! or error! at some point)

if self.playbackRate.get() != r {
self.playbackRate.set(r);
self.queue_ratechange_event();
}

This comment has been minimized.

@ferjm

ferjm Dec 16, 2018

Member

Let's do this slightly different to avoid duplicated ratechange events:

if self.playbackRate.get() == r {
    self.queue_ratechange_event();
} else {
    self.playbackRate.set(r);
}

This comment has been minimized.

@georgeroman

georgeroman Dec 16, 2018

Contributor

Wouldn't we miss a ratechange event here? If the new rate is different than the previous one, we simply set the rate to its new value but no event is triggered.

This comment has been minimized.

@ferjm

ferjm Dec 16, 2018

Member

Oh, right!! Sorry, I misread the code before, I don't know why I thought there was a SetPlaybackRate call there ^_^. Your original code works fine (no duplicated event), but I still think that it may be slightly better to trigger the event once the media backend notifies about the actual rate change. So, just taking the call to queue the event out of your original code should work if you address this comment as well:

if self.playbackRate.get() != r {
    self.playbackRate.set(r);
}
self.queue_ratechange_event();

This comment has been minimized.

@georgeroman

georgeroman Dec 17, 2018

Contributor

I modified the code according to this and the comment you specified and I was getting TIMEOUT on some of the tests. Is this happening because we miss triggering the event when the playback rate is changed by script?

When the defaultPlaybackRate or playbackRate attributes change value (either by being set by script or by being changed directly by the user agent, e.g. in response to user control) the user agent must queue a task to fire an event named ratechange at the media element.

This comment has been minimized.

@ferjm

ferjm Dec 17, 2018

Member

Which tests are timing out? Could you update the PR with your latest changes, please?

If it is timing out it may be because we are not getting the Player::RateChanged in some cases or because there is something else wrong.

This comment has been minimized.

@georgeroman

georgeroman Dec 17, 2018

Contributor

Here are the tests that time out:
image

This comment has been minimized.

@ferjm

ferjm Dec 17, 2018

Member

It seems that the problem is in servo-media. Setting the playback rate here completely breaks a/v playback. It even failed on Travis. Sorry, I should have catched that on the review.

I still don't know why that fails. I will investigate more tomorrow.

This comment has been minimized.

@ferjm

ferjm Dec 19, 2018

Member

I found the reason why this was not working. We were creating a deadlock while trying to send the RateChanged event. The good news is that we don't need this event at all. I mistakenly thought that the rate change signal was triggered when the actual rate change was effective and that the player could change its playback rate for other reasons other than the client asking for that change, but apparently none of my assumptions were correct. The signal is triggered only when the rate property is changed and the playback rate will only change if the client instructs the player to change it and so we don't need to notify the client about that change.

I am removing the signal handler and the Player::RateChanged event in servo/media#175.

This comment has been minimized.

@ferjm

ferjm Dec 20, 2018

Member

@georgeroman servo/media#175 has already landed, so we can update this PR to remove the Player::RateChanged logic. Thanks for your pacience.

This comment has been minimized.

@georgeroman

georgeroman Dec 20, 2018

Contributor

Great! I updated the code accordingly and now the tests pass.

}

/// https://html.spec.whatwg.org/multipage/#dom-media-playbackrate
fn SetPlaybackRate(&self, value: Finite<f64>) {

This comment has been minimized.

@ferjm

ferjm Dec 16, 2018

Member

According to the gst-player docs the allowed range for the rate property is [-64,64] and the spec expects us to throw a NotSupportedError exception if we try setting a value out of the allowed range.

if let Err(e) = self.player.set_rate(*value) {
eprintln!("Could not set the playback rate {:?}", e);
}
self.queue_ratechange_event();

This comment has been minimized.

@ferjm

ferjm Dec 16, 2018

Member

We receive the Player::RateChanged event from the media backend once the new rate has been actually set and we are already queueing the ratechange event in the Player::RateChanged handler, so there's no need to queue it here (we would get duplicated events).


/// https://html.spec.whatwg.org/multipage/#dom-media-playbackrate
fn SetPlaybackRate(&self, value: Finite<f64>) {
if *value != self.playbackRate.get() {

This comment has been minimized.

@ferjm

ferjm Dec 16, 2018

Member

Also according to the spec, we should only change the rate if we are potentially playing

This comment has been minimized.

@georgeroman

georgeroman Dec 17, 2018

Contributor

I looked at how Gecko does this check and noticed that it misses some steps in the process. I am not really sure how to go about implementing it, could you, please, give me some guidelines?

This comment has been minimized.

@ferjm

ferjm Dec 17, 2018

Member

Yeah, we don't have all the required pieces to completely implement all the checks, but we can at least leave the corresponding placeholders:

  • We depend on #22348 to get the Ended method.

  • I suspect we depend on the MediaSession API to be able to implement the is_paused_for_user_interaction method.

  • We certainly depend on #22314 to implement is_paused_for_in_band_content. Let's just leave the placeholders for now.

I would do something like:

fn is_potentially_playing(&self) -> bool {
    !self.paused.get() &&
    // FIXME: We need https://github.com/servo/servo/pull/22348
    //              to know whether playback has ended or not
    // !self.Ended() &&
    self.error.get().is_none() &&
    !self.is_blocked_media_element()
}

fn is_blocked_media_element(&self) -> bool {
    self.readyState.get() <= ReadyState::HaveCurrentData ||
    self.is_paused_for_user_interaction() ||
    self.is_paused_for_in_band_content()
}

fn is_paused_for_user_interaction(&self) -> bool {
    // FIXME: we will likely be able to fill this placeholder once (if) we
    //        implement the MediaSession API.
    false
}

fn is_paused_for_in_band_content(&self) -> bool {
     // FIXME: we will likely be able to fill this placeholder once (if) we
     //        implement https://github.com/servo/servo/issues/22314
    false
}

@georgeroman georgeroman force-pushed the georgeroman:implement_htmlmediaelement_playback_rates branch 2 times, most recently from 6182b0a to f3a1240 Dec 17, 2018

@ferjm

ferjm approved these changes Dec 20, 2018

Copy link
Member

ferjm left a comment

Looks great! r=me with the last comment addressed. Thanks!

(Our CI env is on fire now, so we need to wait for #22493 to land before being able to merge this one)

self.playbackRate.set(*value);
self.queue_ratechange_event();
if self.is_potentially_playing() {
if let Err(e) = self.player.set_rate(*value) {

This comment has been minimized.

@ferjm

ferjm Dec 20, 2018

Member

One last change! Let's set the property and queue the ratechange event only if setting the player rate succeeds.

This comment has been minimized.

@georgeroman

georgeroman Dec 22, 2018

Contributor

With this change in place, it looks like the tests timeouts are back.

This comment has been minimized.

@ferjm

ferjm Dec 26, 2018

Member

Ok. I think this may be a misinterpretation (or actually a lack of clarity) of the spec.

So the spec says here:

Set playbackRate to the new value, and if the element is potentially playing, change the playback speed.

Changing the playback speed (self.player.set_rate()) depends on the element being potentially playing.

But then it says as well:

When the defaultPlaybackRate or playbackRate attributes change value (either by being set by script or by being changed directly by the user agent, e.g. in response to user control) the user agent must queue a task to fire an event named ratechange at the media element.

It does not say anything about firing the ratechange event only if the element is potentially playing. So we will need to change the attribute value (self.playbackRate.set(*value)) and fire the event (self.queue_ratechange_event()) even if the element is not potentially playing.

The problem with allowing the playbackRate attribute change when the media element is not playing is that we will be missing the actual rate change when the media starts playing. So to fix that issue we will need to apply the current playbackRate attribute value to the player (self.player.set_rate(self.playbackRage.get())) right before the media starts playing.

It may be good to open an issue in https://github.com/whatwg/html about this.

This comment has been minimized.

@georgeroman

georgeroman Dec 26, 2018

Contributor

Thank you for the explanations! I opened an issue here: whatwg/html#4256

This comment has been minimized.

@georgeroman

georgeroman Jan 10, 2019

Contributor

@ferjm It seems that updating the playback rate of the media element was implicitly mentioned in another paragraph of the API: whatwg/html#4256 (comment)

This comment has been minimized.

@ferjm

ferjm Jan 10, 2019

Member

Ah, I see! Thanks for following up on that. The code fully reflects what the spec says then. Thanks!

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 20, 2018

Oh, you will likely need to update the test expectations for https://github.com/servo/servo/blob/master/tests/wpt/mozilla/tests/mozilla/interfaces.html as well.

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 20, 2018

⌛️ Trying commit f3a1240 with merge a38814b...

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

Auto merge of #22449 - georgeroman:implement_htmlmediaelement_playbac…
…k_rates, r=<try>

Implement HTMLMediaElement defaultPlaybackRate and playbackRate

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22293

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

@georgeroman georgeroman force-pushed the georgeroman:implement_htmlmediaelement_playback_rates branch from f3a1240 to 58907d7 Dec 20, 2018

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 22, 2018

@bors-servo r=ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

📌 Commit 58907d7 has been approved by ferjm

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 27, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 27, 2018

⌛️ Testing commit deb02ab with merge 7f662de...

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

Auto merge of #22449 - georgeroman:implement_htmlmediaelement_playbac…
…k_rates, r=ferjm

Implement HTMLMediaElement defaultPlaybackRate and playbackRate

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22293

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 27, 2018

💔 Test failed - linux-rel-css

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 27, 2018

Hmm ... /html/semantics/embedded-content/media-elements/event_timeupdate.html intermittently passes because gstreamer performs a couple of initial seek requests with ogg files (this does not happen with other containers) and we are triggering the timeupdate event when a seek is completed.
The problem is that ogg playback is not very reliable on gstreamer, and I am forcing the usage of mp4 for the WPT suite in #22477.
Since the test is mostly testing the time marches on algorithm rather than timeupdate events fired from other places and we are not implementing this algorithm yet, I think we should disable this test and /html/semantics/embedded-content/media-elements/event_timeupdate_noautoplay.html for now until I figure out how to merge #22477.

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 27, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 27, 2018

📌 Commit 9aea7fb has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 27, 2018

⌛️ Testing commit 9aea7fb with merge 9a04f3c...

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

Auto merge of #22449 - georgeroman:implement_htmlmediaelement_playbac…
…k_rates, r=ferjm

Implement HTMLMediaElement defaultPlaybackRate and playbackRate

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22293

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 28, 2018

💥 Test timed out

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 28, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 28, 2018

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: #22428
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 28, 2018

📌 Commit 9aea7fb has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 28, 2018

⌛️ Testing commit 9aea7fb with merge ec846e3...

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

Auto merge of #22449 - georgeroman:implement_htmlmediaelement_playbac…
…k_rates, r=ferjm

Implement HTMLMediaElement defaultPlaybackRate and playbackRate

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22293

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 28, 2018

@bors-servo bors-servo merged commit 9aea7fb into servo:master Dec 28, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment