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
from

Conversation

@ntrrgc
Copy link
Contributor

ntrrgc commented Jul 12, 2019

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

Copy link
Contributor

wolenetz left a comment

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:

This comment has been minimized.

Copy link
@wolenetz

wolenetz Jul 12, 2019

Contributor

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;

This comment has been minimized.

Copy link
@wolenetz

wolenetz Jul 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ntrrgc

ntrrgc Jul 14, 2019

Author Contributor

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

@wolenetz wolenetz requested a review from mounirlamouri Jul 12, 2019
@ntrrgc ntrrgc force-pushed the ntrrgc:wpt-export-for-webkit-199719 branch from b8aceeb to 4c9d0f2 Jul 16, 2019
function onTimeUpdate() {
if (Math.abs(video.currentTime - seekTime) <= 1) {
test.done();
document.body.removeChild(video);

This comment has been minimized.

Copy link
@mounirlamouri

mounirlamouri Jul 19, 2019

Contributor

What's the intent behind removing the video from the DOM? If the goal is to reset the player, why not video.src = "";?

This comment has been minimized.

Copy link
@ntrrgc

ntrrgc Aug 28, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@zcorpan

zcorpan Aug 30, 2019

Member

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.

This comment has been minimized.

Copy link
@ntrrgc

ntrrgc Aug 30, 2019

Author Contributor

But when the test fails often you are interested in what state it was at that point so you can debug the issue.

This comment has been minimized.

Copy link
@zcorpan

zcorpan Aug 30, 2019

Member

Hmm yes, good point.

@ntrrgc ntrrgc force-pushed the ntrrgc:wpt-export-for-webkit-199719 branch from 4c9d0f2 to 1c4b34b Jul 24, 2019
@ntrrgc ntrrgc force-pushed the ntrrgc:wpt-export-for-webkit-199719 branch from 1c4b34b to 3616f5b Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.