Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ntrrgc
Copy link
Contributor

@ntrrgc ntrrgc commented Jul 12, 2019

WebKit export from bug: [MSE][GStreamer] WebKitMediaSrc rework

Copy link
Member

@wolenetz wolenetz left a 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:
Copy link
Member

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:

  1. 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
  2. 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;
Copy link
Member

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.

Copy link
Contributor Author

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).

@ntrrgc ntrrgc force-pushed the wpt-export-for-webkit-199719 branch from b8aceeb to 4c9d0f2 Compare July 16, 2019 20:34
function onTimeUpdate() {
if (Math.abs(video.currentTime - seekTime) <= 1) {
test.done();
document.body.removeChild(video);
Copy link
Contributor

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 = "";?

Copy link
Contributor Author

@ntrrgc ntrrgc Aug 28, 2019

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yes, good point.

@ntrrgc ntrrgc force-pushed the wpt-export-for-webkit-199719 branch from 4c9d0f2 to 1c4b34b Compare July 24, 2019 23:35
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants