html: Track the script/UA initiated media seek request#40981
Merged
mrobinson merged 1 commit intoservo:mainfrom Dec 2, 2025
Merged
html: Track the script/UA initiated media seek request#40981mrobinson merged 1 commit intoservo:mainfrom
seek request#40981mrobinson merged 1 commit intoservo:mainfrom
Conversation
|
🔨 Triggering try run (#19822168393) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19822168393): Flaky unexpected result (35)
Stable unexpected results that are known to be intermittent (27)
|
|
✨ Try run (#19822168393) succeeded. |
servo-highfive
added
the
S-awaiting-review
mrobinson
requested changes
Dec 1, 2025
Member
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Looks good, my main concern here is that this is a tricky issue and I think we should have a bit of a rustdoc explanation of it. This GStreamer behavior is extremely non-obvious.
| volume: Cell<f64>, | ||
| /// <https://html.spec.whatwg.org/multipage/#dom-media-seeking> | ||
| seeking: Cell<bool>, | ||
| current_seek_position: Cell<f64>, |
Member
There was a problem hiding this comment.
Do you mind adding rustdoc to this field? For instance what are the units here?
Contributor
Author
There was a problem hiding this comment.
Done. Units of measurement are seconds.
Comment on lines
+2017
to
+2018
| if let Err(err) = player.lock().unwrap().seek(time) { | ||
| error!("Seek error {:?}", err); |
Member
There was a problem hiding this comment.
Nit:
Suggested change
| if let Err(err) = player.lock().unwrap().seek(time) { | |
| error!("Seek error {:?}", err); | |
| if let Err(error) = player.lock().unwrap().seek(time) { | |
| error!("Seek error {error:?}"); |
| // than the time of the end of the media resource, then the user agent must also seek to the | ||
| // time of the end of the media resource. | ||
| if self.current_playback_position.get() > duration { | ||
| self.seek(duration, /* approximate_for_speed */ false); |
Member
There was a problem hiding this comment.
Nit: Please put the name of the variable after the value.
Contributor
Author
There was a problem hiding this comment.
Other seek calls also use /* approximate_for_speed */ value ordering, so I guess need to change everywhere or do nothing or change only here... Suggestions?
Member
There was a problem hiding this comment.
I was think just this callsite,
but this is a very minor issue.
| @@ -548,6 +548,7 @@ pub(crate) struct HTMLMediaElement { | |||
| volume: Cell<f64>, | |||
| /// <https://html.spec.whatwg.org/multipage/#dom-media-seeking> | |||
Member
There was a problem hiding this comment.
Maybe clarify here what kinds of situations that seeking happens where this is also false? I understand that this is when triggered by the media engine. I think it's worth a rustdoc explanation.
Contributor
Author
There was a problem hiding this comment.
Ok. I will add rustdoc for seeking and current_seek_position as well
servo-highfive
added
S-needs-code-changes
The low-level media `seek` request could be initiated by script (DOM method call or setting of an IDL attribute), by the user agent (seeking data) or by the media engine itself (e.g. gst_play_set_rate()). And to distinguish between them we will use the latest seek position (in seconds) to be able abort processing the `seek` algorithm steps (13-17) for a `seek` request initiated by the media engine. See https://html.spec.whatwg.org/multipage/#dom-media-seek If the `seeking` is in progress, any callback which affects the current playback position (`position changed`, `end of the playback`) shouldn't be processed (event marshalling over IPC router is non-state conditional). Testing: Regression in the following test causes by gstreamer issue when the `gst_play_set_rate()` overrides the latest seek requested position unconditionally and the user agent receives the `seek completion` event with the unexpected seek position (0.5+ instead of 0.0) - html/semantics/embedded-content/media-elements/preserves-pitch.html See https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/4762 Fixes: servo#37057 Signed-off-by: Andrei Volykhin <[email protected]>
tharkum
force-pushed
the
html-media-track-seek-request
branch
from
16494f1 to
31bc214
Compare
servo-highfive
added
S-awaiting-review
tharkum
changed the title
seeking stateseek request
mrobinson
approved these changes
Dec 2, 2025
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
servo-highfive
removed
the
S-awaiting-merge
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
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.
The low-level media
seekrequest could be initiated by script (DOM method call or setting of an IDL attribute), by the user agent (seeking data) or by the media engine itself (e.g. gst_play_set_rate()).And to distinguish between them we will use the latest seek position (in seconds) to be able abort processing the
seekalgorithm steps (13-17) for aseekrequest initiated by the media engine.See https://html.spec.whatwg.org/multipage/#dom-media-seek
If the
seekingis in progress, any callback which affects the current playback position (position changed,end of the playback) shouldn't be processed (event marshalling over IPC router is non-state conditional).Testing: Regression in the following test causes by gstreamer issue when the
gst_play_set_rate()overrides the latest seek requested position unconditionally and the user agent receives theseek completionevent with the unexpected seek position (0.5+ instead of 0.0).See https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/4762
Fixes: #37057