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

Merged
merged 2 commits into from Dec 28, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Some generated files are not rendered by default. Learn more.

@@ -67,6 +67,7 @@ print
progress
radio
range
ratechange
readystatechange
reftest-wait
rejectionhandled
@@ -164,6 +164,10 @@ pub struct HTMLMediaElement {
error: MutNullableDom<MediaError>,
/// <https://html.spec.whatwg.org/multipage/#dom-media-paused>
paused: Cell<bool>,
/// <https://html.spec.whatwg.org/multipage/#dom-media-defaultplaybackrate>
defaultPlaybackRate: Cell<f64>,
/// <https://html.spec.whatwg.org/multipage/#dom-media-playbackrate>
playbackRate: Cell<f64>,
/// <https://html.spec.whatwg.org/multipage/#attr-media-autoplay>
autoplaying: Cell<bool>,
/// <https://html.spec.whatwg.org/multipage/#delaying-the-load-event-flag>
@@ -235,6 +239,8 @@ impl HTMLMediaElement {
fired_loadeddata_event: Cell::new(false),
error: Default::default(),
paused: Cell::new(true),
defaultPlaybackRate: Cell::new(1.0),
playbackRate: Cell::new(1.0),
// FIXME(nox): Why is this initialised to true?
autoplaying: Cell::new(true),
delaying_the_load_event_flag: Default::default(),
@@ -271,6 +277,15 @@ impl HTMLMediaElement {
}
}

fn play_media(&self) {
if let Err(e) = self.player.set_rate(self.playbackRate.get()) {
warn!("Could not set the playback rate {:?}", e);
}
if let Err(e) = self.player.play() {
warn!("Could not play media {:?}", e);
}
}

/// Marks that element as delaying the load event or not.
///
/// Nothing happens if the element was already delaying the load event and
@@ -358,9 +373,7 @@ impl HTMLMediaElement {
}

this.fulfill_in_flight_play_promises(|| {
if let Err(e) = this.player.play() {
eprintln!("Could not play media {:?}", e);
}
this.play_media();
});
}),
window.upcast(),
@@ -449,9 +462,7 @@ impl HTMLMediaElement {
this.fulfill_in_flight_play_promises(|| {
// Step 2.1.
this.upcast::<EventTarget>().fire_event(atom!("playing"));
if let Err(e) = this.player.play() {
eprintln!("Could not play media {:?}", e);
}
this.play_media();

// Step 2.2.
// Done after running this closure in
@@ -888,6 +899,43 @@ impl HTMLMediaElement {
);
}

fn queue_ratechange_event(&self) {
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);
}

// https://html.spec.whatwg.org/multipage/#potentially-playing
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()
}

// https://html.spec.whatwg.org/multipage/#blocked-media-element
fn is_blocked_media_element(&self) -> bool {
self.ready_state.get() <= ReadyState::HaveCurrentData ||
self.is_paused_for_user_interaction() ||
self.is_paused_for_in_band_content()
}

// https://html.spec.whatwg.org/multipage/#paused-for-user-interaction
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
}

// https://html.spec.whatwg.org/multipage/#paused-for-in-band-content
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
}

// https://html.spec.whatwg.org/multipage/#media-element-load-algorithm
fn media_element_load_algorithm(&self) {
// Reset the flag that signals whether loadeddata was ever fired for
@@ -960,7 +1008,7 @@ impl HTMLMediaElement {
}

// Step 7.
// FIXME(nox): Set playbackRate to defaultPlaybackRate.
self.playbackRate.set(self.defaultPlaybackRate.get());

// Step 8.
self.error.set(None);
@@ -1350,6 +1398,53 @@ impl HTMLMediaElementMethods for HTMLMediaElement {
self.paused.get()
}

/// https://html.spec.whatwg.org/multipage/#dom-media-defaultplaybackrate
fn GetDefaultPlaybackRate(&self) -> Fallible<Finite<f64>> {
Ok(Finite::wrap(self.defaultPlaybackRate.get()))
}

/// https://html.spec.whatwg.org/multipage/#dom-media-defaultplaybackrate
fn SetDefaultPlaybackRate(&self, value: Finite<f64>) -> ErrorResult {
let min_allowed = -64.0;
let max_allowed = 64.0;
if *value < min_allowed || *value > max_allowed {
return Err(Error::NotSupported);
}

if *value != self.defaultPlaybackRate.get() {
self.defaultPlaybackRate.set(*value);
self.queue_ratechange_event();
}

Ok(())
}

/// https://html.spec.whatwg.org/multipage/#dom-media-playbackrate
fn GetPlaybackRate(&self) -> Fallible<Finite<f64>> {
Ok(Finite::wrap(self.playbackRate.get()))
}

/// https://html.spec.whatwg.org/multipage/#dom-media-playbackrate
fn SetPlaybackRate(&self, value: Finite<f64>) -> ErrorResult {
let min_allowed = -64.0;
let max_allowed = 64.0;
if *value < min_allowed || *value > max_allowed {
return Err(Error::NotSupported);
}

if *value != self.playbackRate.get() {

This comment has been minimized.

Copy link
@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.

Copy link
@georgeroman

georgeroman Dec 17, 2018

Author 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.

Copy link
@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
}
self.playbackRate.set(*value);

This comment has been minimized.

Copy link
@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.

self.queue_ratechange_event();

This comment has been minimized.

Copy link
@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).

if self.is_potentially_playing() {
if let Err(e) = self.player.set_rate(*value) {

This comment has been minimized.

Copy link
@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.

Copy link
@georgeroman

georgeroman Dec 22, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@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.

Copy link
@georgeroman

georgeroman Dec 26, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@georgeroman

georgeroman Jan 10, 2019

Author 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.

Copy link
@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!

warn!("Could not set the playback rate {:?}", e);
}
}
}

Ok(())
}

// https://html.spec.whatwg.org/multipage/#dom-media-duration
fn Duration(&self) -> f64 {
self.duration.get()
@@ -42,8 +42,8 @@ interface HTMLMediaElement : HTMLElement {
readonly attribute unrestricted double duration;
// Date getStartDate();
readonly attribute boolean paused;
// attribute double defaultPlaybackRate;
// attribute double playbackRate;
[Throws] attribute double defaultPlaybackRate;
[Throws] attribute double playbackRate;
readonly attribute TimeRanges played;
// readonly attribute TimeRanges seekable;
// readonly attribute boolean ended;
@@ -1041,10 +1041,10 @@
expected: FAIL

[HTMLMediaElement interface: document.createElement("video") must inherit property "defaultPlaybackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("video") must inherit property "playbackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("video") must inherit property "played" with the proper type]
expected: FAIL
@@ -1113,10 +1113,10 @@
expected: FAIL

[HTMLMediaElement interface: document.createElement("audio") must inherit property "defaultPlaybackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("audio") must inherit property "playbackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("audio") must inherit property "played" with the proper type]
expected: FAIL
@@ -1248,10 +1248,10 @@
expected: FAIL

[HTMLMediaElement interface: new Audio() must inherit property "defaultPlaybackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: new Audio() must inherit property "playbackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: new Audio() must inherit property "played" with the proper type]
expected: FAIL
@@ -1401,10 +1401,10 @@
expected: FAIL

[HTMLMediaElement interface: attribute defaultPlaybackRate]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: attribute playbackRate]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: attribute played]
expected: FAIL
@@ -6775,10 +6775,10 @@
expected: FAIL

[HTMLMediaElement interface: document.createElement("video") must inherit property "defaultPlaybackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("video") must inherit property "playbackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("video") must inherit property "seekable" with the proper type]
expected: FAIL
@@ -6820,10 +6820,10 @@
expected: FAIL

[HTMLMediaElement interface: document.createElement("audio") must inherit property "defaultPlaybackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("audio") must inherit property "playbackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: document.createElement("audio") must inherit property "seekable" with the proper type]
expected: FAIL
@@ -6865,10 +6865,10 @@
expected: FAIL

[HTMLMediaElement interface: new Audio() must inherit property "defaultPlaybackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: new Audio() must inherit property "playbackRate" with the proper type]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: new Audio() must inherit property "seekable" with the proper type]
expected: FAIL
@@ -6985,10 +6985,10 @@
expected: FAIL

[HTMLMediaElement interface: attribute defaultPlaybackRate]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: attribute playbackRate]
expected: FAIL
expected: PASS

[HTMLMediaElement interface: attribute seekable]
expected: FAIL
@@ -1,5 +1,6 @@
[event_timeupdate.html]
type: testharness
disabled: Until https://github.com/servo/servo/pull/22477 is merged
expected: TIMEOUT
[setting src attribute on a sufficiently long autoplay audio should trigger timeupdate event]
expected: NOTRUN
@@ -1,5 +1,6 @@
[event_timeupdate_noautoplay.html]
type: testharness
disabled: Until https://github.com/servo/servo/pull/22477 is merged
expected: TIMEOUT
[calling play() on a sufficiently long audio should trigger timeupdate event]
expected: NOTRUN

This file was deleted.

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.