Skip to content

Commit

Permalink
Auto merge of #23853 - ferjm:media.update, r=<try>
Browse files Browse the repository at this point in the history
[WIP] Fix HTMLMediaElement seek race condition

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

This depends on servo/media#289 and it's blocked by the servo-media update issue mentioned [here](#23842 (comment))

<!-- 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/23853)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Aug 13, 2019
2 parents 9b24798 + 1fbb3b3 commit 72bc245
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 20 deletions.
54 changes: 41 additions & 13 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 26 additions & 7 deletions components/script/dom/htmlmediaelement.rs
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ use net_traits::{NetworkError, ResourceFetchTiming, ResourceTimingType};
use script_layout_interface::HTMLMediaData; use script_layout_interface::HTMLMediaData;
use servo_config::pref; use servo_config::pref;
use servo_media::player::frame::{Frame, FrameRenderer}; use servo_media::player::frame::{Frame, FrameRenderer};
use servo_media::player::{PlaybackState, Player, PlayerError, PlayerEvent, StreamType}; use servo_media::player::{PlaybackState, Player, PlayerError, PlayerEvent, SeekLock, StreamType};
use servo_media::{ClientContextId, ServoMedia, SupportsMediaType}; use servo_media::{ClientContextId, ServoMedia, SupportsMediaType};
use servo_url::ServoUrl; use servo_url::ServoUrl;
use std::cell::Cell; use std::cell::Cell;
Expand Down Expand Up @@ -787,7 +787,7 @@ impl HTMLMediaElement {
} }
} }


fn fetch_request(&self, offset: Option<u64>) { fn fetch_request(&self, offset: Option<u64>, seek_lock: Option<SeekLock>) {
if self.resource_url.borrow().is_none() && self.blob_url.borrow().is_none() { if self.resource_url.borrow().is_none() && self.blob_url.borrow().is_none() {
eprintln!("Missing request url"); eprintln!("Missing request url");
self.queue_dedicated_media_source_failure_steps(); self.queue_dedicated_media_source_failure_steps();
Expand Down Expand Up @@ -835,6 +835,7 @@ impl HTMLMediaElement {
self, self,
url.clone(), url.clone(),
offset.unwrap_or(0), offset.unwrap_or(0),
seek_lock,
))); )));
let (action_sender, action_receiver) = ipc::channel().unwrap(); let (action_sender, action_receiver) = ipc::channel().unwrap();
let window = window_from_node(self); let window = window_from_node(self);
Expand Down Expand Up @@ -918,7 +919,7 @@ impl HTMLMediaElement {


// Step 4.remote.2. // Step 4.remote.2.
*self.resource_url.borrow_mut() = Some(url); *self.resource_url.borrow_mut() = Some(url);
self.fetch_request(None); self.fetch_request(None, None);
}, },
Resource::Object => { Resource::Object => {
if let Some(ref src_object) = *self.src_object.borrow() { if let Some(ref src_object) = *self.src_object.borrow() {
Expand All @@ -927,7 +928,7 @@ impl HTMLMediaElement {
let blob_url = URL::CreateObjectURL(&self.global(), &*blob); let blob_url = URL::CreateObjectURL(&self.global(), &*blob);
*self.blob_url.borrow_mut() = *self.blob_url.borrow_mut() =
Some(ServoUrl::parse(&blob_url).expect("infallible")); Some(ServoUrl::parse(&blob_url).expect("infallible"));
self.fetch_request(None); self.fetch_request(None, None);
}, },
SrcObject::MediaStream(ref stream) => { SrcObject::MediaStream(ref stream) => {
let tracks = &*stream.get_tracks(); let tracks = &*stream.get_tracks();
Expand Down Expand Up @@ -1728,8 +1729,8 @@ impl HTMLMediaElement {
self.playback_position.set(position); self.playback_position.set(position);
self.time_marches_on(); self.time_marches_on();
}, },
PlayerEvent::SeekData(p) => { PlayerEvent::SeekData(p, ref seek_lock) => {
self.fetch_request(Some(p)); self.fetch_request(Some(p), Some(seek_lock.clone()));
}, },
PlayerEvent::SeekDone(_) => { PlayerEvent::SeekDone(_) => {
// Continuation of // Continuation of
Expand Down Expand Up @@ -2469,6 +2470,10 @@ struct HTMLMediaElementFetchListener {
/// EnoughData event uses this value to restart the download from /// EnoughData event uses this value to restart the download from
/// the last fetched position. /// the last fetched position.
latest_fetched_content: u64, latest_fetched_content: u64,
/// The media player discards all data pushes until the seek block
/// is released right before pushing the data from the offset requested
/// by a seek request.
seek_lock: Option<SeekLock>,
} }


// https://html.spec.whatwg.org/multipage/#media-data-processing-steps-list // https://html.spec.whatwg.org/multipage/#media-data-processing-steps-list
Expand Down Expand Up @@ -2565,6 +2570,10 @@ impl FetchResponseListener for HTMLMediaElementFetchListener {


let payload_len = payload.len() as u64; let payload_len = payload.len() as u64;


if let Some(seek_lock) = self.seek_lock.take() {
seek_lock.unlock(/* successful seek */ true);
}

// Push input data into the player. // Push input data into the player.
if let Err(e) = elem if let Err(e) = elem
.player .player
Expand Down Expand Up @@ -2609,6 +2618,10 @@ impl FetchResponseListener for HTMLMediaElementFetchListener {


// https://html.spec.whatwg.org/multipage/#media-data-processing-steps-list // https://html.spec.whatwg.org/multipage/#media-data-processing-steps-list
fn process_response_eof(&mut self, status: Result<ResourceFetchTiming, NetworkError>) { fn process_response_eof(&mut self, status: Result<ResourceFetchTiming, NetworkError>) {
if let Some(seek_lock) = self.seek_lock.take() {
seek_lock.unlock(/* successful seek */ false);
}

let elem = self.elem.root(); let elem = self.elem.root();


if elem.player.borrow().is_none() { if elem.player.borrow().is_none() {
Expand Down Expand Up @@ -2718,7 +2731,12 @@ impl PreInvoke for HTMLMediaElementFetchListener {
} }


impl HTMLMediaElementFetchListener { impl HTMLMediaElementFetchListener {
fn new(elem: &HTMLMediaElement, url: ServoUrl, offset: u64) -> Self { fn new(
elem: &HTMLMediaElement,
url: ServoUrl,
offset: u64,
seek_lock: Option<SeekLock>,
) -> Self {
Self { Self {
elem: Trusted::new(elem), elem: Trusted::new(elem),
metadata: None, metadata: None,
Expand All @@ -2728,6 +2746,7 @@ impl HTMLMediaElementFetchListener {
url, url,
expected_content_length: None, expected_content_length: None,
latest_fetched_content: offset, latest_fetched_content: offset,
seek_lock,
} }
} }
} }
3 changes: 3 additions & 0 deletions servo-tidy.toml
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -37,16 +37,19 @@ packages = [
"crossbeam-deque", "crossbeam-deque",
"euclid", # https://github.com/servo/rust-webvr/pull/89 "euclid", # https://github.com/servo/rust-webvr/pull/89
"gl_generator", # https://github.com/servo/servo/pull/23288#issuecomment-494687746 "gl_generator", # https://github.com/servo/servo/pull/23288#issuecomment-494687746
"idna", # https://github.com/servo/servo/pull/23838
"lock_api", "lock_api",
"log", "log",
"mime", "mime",
"mime_guess", "mime_guess",
"nix", # https://github.com/servo/servo/issues/23189#issuecomment-487512605 "nix", # https://github.com/servo/servo/issues/23189#issuecomment-487512605
"parking_lot", "parking_lot",
"parking_lot_core", "parking_lot_core",
"percent-encoding", # https://github.com/servo/servo/pull/23838
"rand_core", "rand_core",
"scopeguard", "scopeguard",
"unicase", "unicase",
"url", # https://github.com/servo/servo/pull/23838
] ]
# Files that are ignored for all tidy and lint checks. # Files that are ignored for all tidy and lint checks.
files = [ files = [
Expand Down

0 comments on commit 72bc245

Please sign in to comment.