From 72e6a1f007fd78d450e3e2f5569bec5a0bb247ff Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 9 Jul 2024 02:48:44 -0400 Subject: [PATCH] Remove media element state changes triggered by network responses (#32643) * Do not change media element ready state when network response is complete. * Do not fire multiple error events for the same media content. * Inform media backend when media response is complete. * Continue delaying the load event when a complete media response is received. * Only mark a media response as complete when the response is the active one. * Update expectations for imagebitmap tests using video element. * Update fetch ORB video test expectations. * Update media CSS selector test expectation for non-implemented feature. * Update expectations for media element tests that now work. * Updat expected result for failing reftest. * Update expected failure for test that loads an audio file in a video element. * Update media test expectation for unimplemented track feature. * Do not process media element ready state changes that are unchanged. * Reset media element ready state to Current when playback finishes. * Set media element ready state to Enough when appropriate player event is received. * Update test expectations. --- components/script/dom/htmlmediaelement.rs | 76 ++++++++++------ .../mix-blend-mode-video.html.ini | 2 - .../css/css-overflow/overflow-video.html.ini | 2 + .../media/media-playback-state.html.ini | 5 +- .../tentative/known-mime-type.sub.any.js.ini | 89 ++----------------- .../orb/tentative/nosniff.sub.any.js.ini | 60 ++----------- .../fetch/orb/tentative/status.sub.any.js.ini | 24 +---- .../non-matching-range-response.html.ini | 3 + ...eImageBitmap-colorSpaceConversion.html.ini | 1 + .../createImageBitmap-drawImage.html.ini | 2 +- .../createImageBitmap-invalid-args.html.ini | 2 +- .../media-elements/audio_loop_base.html.ini | 5 -- .../audio_loop_seek_to_eos.html.ini | 3 - .../loop-from-ended.tentative.html.ini | 3 +- .../track-cue-mutable-fragment.html.ini | 3 +- ...ack-mode-not-changed-by-new-track.html.ini | 3 +- 16 files changed, 80 insertions(+), 203 deletions(-) delete mode 100644 tests/wpt/meta/css/compositing/mix-blend-mode/mix-blend-mode-video.html.ini create mode 100644 tests/wpt/meta/css/css-overflow/overflow-video.html.ini create mode 100644 tests/wpt/meta/fetch/range/non-matching-range-response.html.ini delete mode 100644 tests/wpt/meta/html/semantics/embedded-content/media-elements/audio_loop_base.html.ini delete mode 100644 tests/wpt/meta/html/semantics/embedded-content/media-elements/audio_loop_seek_to_eos.html.ini diff --git a/components/script/dom/htmlmediaelement.rs b/components/script/dom/htmlmediaelement.rs index 997fa3c849d..8a5889cae8f 100644 --- a/components/script/dom/htmlmediaelement.rs +++ b/components/script/dom/htmlmediaelement.rs @@ -615,6 +615,10 @@ impl HTMLMediaElement { return; } + if old_ready_state == ready_state { + return; + } + let window = window_from_node(self); let task_source = window.task_manager().media_element_task_source(); @@ -1064,6 +1068,10 @@ impl HTMLMediaElement { task_source.queue_simple_event(self.upcast(), atom!("ratechange"), &window); } + fn in_error_state(&self) -> bool { + self.error.get().is_some() + } + // https://html.spec.whatwg.org/multipage/#potentially-playing fn is_potentially_playing(&self) -> bool { !self.paused.get() && @@ -1538,6 +1546,9 @@ impl HTMLMediaElement { }), window.upcast(), ); + + // https://html.spec.whatwg.org/multipage/#dom-media-have_current_data + self.change_ready_state(ReadyState::HaveCurrentData); } }, @@ -1556,6 +1567,14 @@ impl HTMLMediaElement { }, PlayerEvent::Error(ref error) => { error!("Player error: {:?}", error); + + // If we have already flagged an error condition while processing + // the network response, we should silently skip any observable + // errors originating while decoding the erroneous response. + if self.in_error_state() { + return; + } + // https://html.spec.whatwg.org/multipage/#loading-the-media-resource:media-data-13 // 1. The user agent should cancel the fetching process. if let Some(ref mut current_fetch_context) = @@ -1803,6 +1822,8 @@ impl HTMLMediaElement { } }, PlayerEvent::EnoughData => { + self.change_ready_state(ReadyState::HaveEnoughData); + // The player has enough data and it is asking us to stop pushing // bytes, so we cancel the ongoing fetch request iff we are able // to restart it from where we left. Otherwise, we continue the @@ -2795,56 +2816,53 @@ impl FetchResponseListener for HTMLMediaElementFetchListener { // https://html.spec.whatwg.org/multipage/#media-data-processing-steps-list fn process_response_eof(&mut self, status: Result) { + trace!("process response eof"); if let Some(seek_lock) = self.seek_lock.take() { seek_lock.unlock(/* successful seek */ false); } let elem = self.elem.root(); - if elem.player.borrow().is_none() { + if elem.generation_id.get() != self.generation_id || elem.player.borrow().is_none() { return; } - // If an error was previously received and no new fetch request was triggered, - // we skip processing the payload and notify the media backend that we are done - // pushing data. - if elem.generation_id.get() == self.generation_id { - if let Some(ref current_fetch_context) = *elem.current_fetch_context.borrow() { - if let Some(CancelReason::Error) = current_fetch_context.cancel_reason() { - if let Err(e) = elem - .player - .borrow() - .as_ref() - .unwrap() - .lock() - .unwrap() - .end_of_stream() - { - warn!("Could not signal EOS to player {:?}", e); - } - return; - } + // There are no more chunks of the response body forthcoming, so we can + // go ahead and notify the media backend not to expect any further data. + if let Err(e) = elem + .player + .borrow() + .as_ref() + .unwrap() + .lock() + .unwrap() + .end_of_stream() + { + warn!("Could not signal EOS to player {:?}", e); + } + + // If an error was previously received we skip processing the payload. + if let Some(ref current_fetch_context) = *elem.current_fetch_context.borrow() { + if let Some(CancelReason::Error) = current_fetch_context.cancel_reason() { + return; } } if status.is_ok() && self.latest_fetched_content != 0 { - if elem.ready_state.get() == ReadyState::HaveNothing { - // Make sure that we don't skip the HaveMetadata and HaveCurrentData - // states for short streams. - elem.change_ready_state(ReadyState::HaveMetadata); - } - elem.change_ready_state(ReadyState::HaveEnoughData); - elem.upcast::().fire_event(atom!("progress")); elem.network_state.set(NetworkState::Idle); elem.upcast::().fire_event(atom!("suspend")); - - elem.delay_load_event(false); } // => "If the connection is interrupted after some media data has been received..." else if elem.ready_state.get() != ReadyState::HaveNothing { + // If the media backend has already flagged an error, skip any observable + // network-related errors. + if elem.in_error_state() { + return; + } + // Step 1 if let Some(ref mut current_fetch_context) = *elem.current_fetch_context.borrow_mut() { current_fetch_context.cancel(CancelReason::Error); diff --git a/tests/wpt/meta/css/compositing/mix-blend-mode/mix-blend-mode-video.html.ini b/tests/wpt/meta/css/compositing/mix-blend-mode/mix-blend-mode-video.html.ini deleted file mode 100644 index 26726a1734d..00000000000 --- a/tests/wpt/meta/css/compositing/mix-blend-mode/mix-blend-mode-video.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[mix-blend-mode-video.html] - expected: FAIL diff --git a/tests/wpt/meta/css/css-overflow/overflow-video.html.ini b/tests/wpt/meta/css/css-overflow/overflow-video.html.ini new file mode 100644 index 00000000000..1d0a9d754d6 --- /dev/null +++ b/tests/wpt/meta/css/css-overflow/overflow-video.html.ini @@ -0,0 +1,2 @@ +[overflow-video.html] + expected: FAIL diff --git a/tests/wpt/meta/css/selectors/media/media-playback-state.html.ini b/tests/wpt/meta/css/selectors/media/media-playback-state.html.ini index 5ece18229f8..0d01e57bd5f 100644 --- a/tests/wpt/meta/css/selectors/media/media-playback-state.html.ini +++ b/tests/wpt/meta/css/selectors/media/media-playback-state.html.ini @@ -1,4 +1,5 @@ [media-playback-state.html] + expected: TIMEOUT [Test :pseudo-class syntax is supported without throwing a SyntaxError] expected: FAIL @@ -6,7 +7,7 @@ expected: FAIL [Test :paused pseudo-classes] - expected: FAIL + expected: TIMEOUT [Test :seeking pseudo-class] - expected: FAIL + expected: NOTRUN diff --git a/tests/wpt/meta/fetch/orb/tentative/known-mime-type.sub.any.js.ini b/tests/wpt/meta/fetch/orb/tentative/known-mime-type.sub.any.js.ini index b05b6f9e2bd..cae3ce43d64 100644 --- a/tests/wpt/meta/fetch/orb/tentative/known-mime-type.sub.any.js.ini +++ b/tests/wpt/meta/fetch/orb/tentative/known-mime-type.sub.any.js.ini @@ -27,101 +27,26 @@ [ORB should block opaque text/plain: fetch(..., {mode: "no-cors"})] expected: FAIL - [ORB should block opaque text/plain: