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
WebKit export of https://bugs.webkit.org/show_bug.cgi?id=199719 #17801
base: master
Are you sure you want to change the base?
WebKit export of https://bugs.webkit.org/show_bug.cgi?id=199719 #17801
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MSE portion of this change LGTM.
I have some comments though for the non-MSE portion.
Also, @mounirlamouri might want to chime in on the non-MSE portion of this too.
return start, last | ||
|
||
def main(request, response): | ||
with open("media/movie_300.mp4", "rb") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this script or the test that uses it be made to dynamically work with the mp4 (avc1/aac) or the ogv media, based on whether or not the runtime supports it? (Not all implementations of various builds of the implementations support the codecs in the mp4 test media, for instance.)
I suggest following the pattern I used more recently in the media-source/mediasource-changetype-play.html tests, where the top level test page dynamically:
- in a distinct test within the page, checks that there is enough support in the implementation for the media (e.g., for this test, video CanPlayType(... mp4's mime vs ogv's mime vs etc) identifies at least 1 supported configuration, and
- for each supported configuration, dynamically create a test that verifies the appropriate behavior. In this change, it looks like you might need to pass some sort of query parameter to the server to identify which test media is being requested; caveat I'm less familiar with using .py in wpt to serve dynamically the bytes requested, so there might be a better way.
</video> | ||
<script> | ||
const video = document.getElementById("video"); | ||
const seekTime = 60 * 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the intent of seekTime? To be beyond currentTime? To be beyond duration? Note that the seeking spec constrains the seek target time to be in [earliest possible position, end of the media resource], then further constrains it by seekable ranges, etc. I suggest making this value defined in the async_test's scope, and calculated within videoLoaded(), with a comment describing the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent here is to seek to an unbuffered position. The test file is 5 minutes long, and here we're seeking to 4 minutes. That will make the browser make an HTTP range request asking for data somewhere in the middle of the file. The server in this case is programmed to let these requests time out on purpose. This way, the test checks that timeupdate
is fired regardless at the time of seek (which is the purpose of the test).
b8aceeb
to
4c9d0f2
Compare
html/semantics/embedded-content/the-video-element/video_timeupdate_on_seek.html
Outdated
Show resolved
Hide resolved
function onTimeUpdate() { | ||
if (Math.abs(video.currentTime - seekTime) <= 1) { | ||
test.done(); | ||
document.body.removeChild(video); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the intent behind removing the video from the DOM? If the goal is to reset the player, why not video.src = "";
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That it does not remain in the DOM after the test has run. It was created by the test, it may as well be removed by the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the test fails (doesn't reach test.done()
), then the video also won't be removed. The test.add_cleanup()
function can be used to remove the video, right after it's inserted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But when the test fails often you are interested in what state it was at that point so you can debug the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes, good point.
4c9d0f2
to
1c4b34b
Compare
1c4b34b
to
3616f5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review process for this patch is being conducted in the WebKit project.
WebKit export from bug: [MSE][GStreamer] WebKitMediaSrc rework