htmlmediaelement: Fix fetch request race on "seek-data" event#37002
Merged
jdm merged 1 commit intoservo:mainfrom Jun 2, 2025
Merged
Conversation
Contributor
Author
|
NOTE: This PR is partial fix - sometimes GStreamer doesn't want to continue do second "seek" or set "paused" state (due it no load event). @jdm < Could you please run linux WPT? |
|
🔨 Triggering try run (#15022924730) for Linux (WPT) |
|
Test results for linux-wpt from try job (#15022924730): Flaky unexpected result (24)
Stable unexpected results that are known to be intermittent (11)
|
|
✨ Try run (#15022924730) succeeded. |
tharkum
force-pushed
the
htmlmediaelement-fetch-request-race-on-seek-event
branch
from
205114a to
d55941a
Compare
tharkum
changed the title
Contributor
Author
|
@jdm < I found and fixed additional issue with "seek lock". |
|
🔨 Triggering try run (#15068747839) for Linux (WPT) |
Member
|
By the way, you can run try builds on your own fork with https://book.servo.org/hacking/testing.html#running-web-platform-tests-on-your-github-fork |
|
Test results for linux-wpt from try job (#15068747839): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (10)
|
|
✨ Try run (#15068747839) succeeded. |
Contributor
Author
|
cc @xiaochengh < FYI @jdm < Do you know any media expert from servo team who can review PR? |
Member
|
I'll take a look tonight. Edit: sorry, not enough brain power tonight. |
Contributor
|
cc @rayguo17 |
jdm
requested changes
May 23, 2025
Member
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
Sorry for the delay, but I really needed time to sit down and think about these changes. I'm vaguely concerned about the increase in complexity that's spreading through different methods, but I don't have any concrete suggestions on how to reduce it right now. I feel confident that these changes are addressing real issues in the current implementation, so let's get them merged!
On execution media element load algorithm https://html.spec.whatwg.org/multipage/media.html#media-element-load-algorithm we can observe race condition between parallel running fetch requests: R1: (media element) initial request (see "resource_fetch_algorithm" function) R2: (gstreamer) duration "seek-data" request (see PlayerEvent::SeekData) R3: (gstreamer) continue on last interrupted time "seek-data" request At time there are only one current fetch context for media element but because resource fetch and cancellation are async operations need to identify (by request id) and skip processing outdated fetch request (fetch listener callbacks). Async load in background sequence (stream content length = 2757913): R1 (seek-offset=0): [------------------------------X] R2 (seek-offset=2757866): [---------Y] AS-****-+++-AE R3 (seek-offset=98304): [----------------Z] BS-****-+++BE R1 (seek-offset=0): [------X] R2 (seek-offset=2757866): [--------Y] (discarded data) AS-****---------+++-AE - A*/B* performed seeks (*** seek lock, +++ flush buffer queue + reset EOS) (on Gstreamer streaming thread) - X/Y/Z "end-of-stream" events from fetch requests (on Servo script thread) All incoming input data from different requests are mixed and pushed to active media player, plus abnormal behaviour due to intermixed X/Y/Z "end-of-stream" events. To handle it properly we will unblock seek lock on "seek-data" event (on script thread) as soon as possible (GStreamer will be able to flush buffer queue and reset EOS state) and introduce buffered data source to delay pushing input data/EOS event until media player will be actually ready. Testing: Improvements to the following tests: - /html/canvas/element/manual/imagebitmap/createImageBitmap-origin.sub.html - /html/semantics/embedded-content/the-canvas-element/security.pattern.fillStyle.sub.html Fixes: servo#31931 Fixes: servo#36989 Signed-off-by: Andrei Volykhin <[email protected]>
tharkum
force-pushed
the
htmlmediaelement-fetch-request-race-on-seek-event
branch
from
d55941a to
c678f81
Compare
Contributor
Author
|
@jdm < Sorry for long delay. After your comments I slightly refactored the original code and introduced the BufferedDataSource to decouple all messy "payload data/EOS" logic from FetchContext. In future I am planning to add required seek functionlity for data:// URL (#32645) into BufferedDataSource as well (or may be convert in into Please take a look and share you feedback. |
jdm
approved these changes
Jun 2, 2025
|
|
||
| fn handle_player_event(&self, event: &PlayerEvent, can_gc: CanGc) { | ||
| fn handle_player_event(&self, player_id: usize, event: &PlayerEvent, can_gc: CanGc) { | ||
| // Ignore the asynchronous event from previous player. |
Member
There was a problem hiding this comment.
So now we have generation id, request id, and player id. Fun!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On execution media element load algorithm
https://html.spec.whatwg.org/multipage/media.html#media-element-load-algorithm
we can observe race condition between parallel running fetch requests:
R1: (media element) initial request (see "resource_fetch_algorithm" function)
R2: (gstreamer) duration "seek-data" request (see PlayerEvent::SeekData)
R3: (gstreamer) continue on last interrupted time "seek-data" request
At time there are only one current fetch context for media element but
because resource fetch and cancellation are async operations need
to identify (by request id) and skip processing outdated fetch request
(fetch listener callbacks).
Async load in background sequence (stream content length = 2757913):
All incoming input data from different requests are mixed and
pushed to active media player, plus abnormal behaviour due to
intermixed X/Y/Z "end-of-stream" events.
To handle it properly we will unblock seek lock on "seek-data" event
(on script thread) as soon as possible (GStreamer will be able
to flush buffer queue and reset EOS state) and introduce buffered data source
to delay pushing input data/EOS event until media player will be actually ready.
Testing: Improvements to the following tests:
Fixes: #31931
Fixes: #36989