-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Test video resizing during playback #19030
Conversation
9518d88
to
e6b9d95
Compare
@zcorpan feel like reviewing this? |
Still timeout in Safari. I tried playing the file locally and the video is corrupted. Added 'ended' to the event list to fail if it's fired. |
Good, test results are what I wanted now. @eric-carlson @jernoble FYI that this could be an incoming Safari-specific failures. If it's a problem with the media file and Safari should support this sort of this, that'd be great to know. |
Instead of using static files one variation of the test is to use the code which produces the file at each browser with |
Does Safari support |
I dunno, I'd prefer to separate the two, this test is revealing a lack of interop on this specific input that using MediaRecorder might mask. |
Have not compiled a full list, though there are certainly more than one interoperability issues re |
This issue describes some of the interoperability issues re the previous comment w3c/mediacapture-fromelement#78. |
@foolip This demonstrates that the test can fail at Chromium, Chrome when a WebM file produced by Chromium, Chrome implementation of |
NOTE: this CL replaces, https://chromium-review.googlesource.com/c/chromium/src/+/1828226 which was submitted in error. This changes the behavior implicit resizing of for VP8/9 video. By implicit, we mean streams where the dimensions change without a new MSE init segment that explicitly describes the new size. Prior to this CL, we took whatever resolution was specified by the container metadata and scaled any later differing resolution into an element of that original size. Many of our decoders worked this way before https://chromium-review.googlesource.com/c/chromium/src/+/1026992/, which shifted the consensus is to allow dynamic resize. This CL makes the libVPX wrapper consistent with those. When the size changes we will update videoWidth and videoHeight and fire a 'resize' event. Bug: 992235,837337 Test: web-platform-tests/wpt#19030, with --disable-features=FFmpegDecodeOpaqueVP8 Change-Id: I7c65810c545511cfa3441c7cbb9e2771159bc88a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1832478 Commit-Queue: Chrome Cunningham <chcunningham@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Commit-Position: refs/heads/master@{#701794}
Conceptually this test LGTM. I've updated chrome to ensure dynamic framesizes are supported for all vp8 decoders. Verified this test still passes when libvpx is used. Re: media files, at a glance the webm file looks sane. I'm not savvy enough to validate the mp4. |
The test LGTM. I haven't reviewed the media files themselves. The expected events are in this order:
(I recall a Presto test for this from an eon ago, nice to see this finally come to wpt!) |
wpt-chrome-dev-stability is failing on Taskcluster like this: Unstable results
My interpretation is that the event order isn't reliably in the expected order, but I think this is a case where flaky fail is the best case. Still, I'll rebase to see if the problem has gone away with a newer version of Chrome. Not reproducing isn't evidence that it's really fixed, of course. |
Fixes #17821. New media files are from @guest271314: #19030 (comment)
03bcb6d
to
ec9cc20
Compare
@foolip |
@fooip Can you confirm the .mp4 video resizes at Safari? |
Fixes #17821. New media files are from @guest271314: #19030 (comment) #19030 (comment)
ec9cc20
to
d36965b
Compare
Fixes #17821. New media files are from @guest271314: #19030 (comment) #19030 (comment)
d36965b
to
cd341f3
Compare
@guest271314 are you entirely sure the mp4 file does correctly resize half way through? Is there a browser or a player where it does resize? I ask because it doesn't seem to resize in Firefox, but WebM does in Firefox. |
@foolip No. Am at a different OS. Erroneously just used |
@foolip Tested at Chromium 80 and Firefox 69. Was not able to build FFmpeg and apply the patches, again, yet. The procedure that used was 1) Use |
Fixes #17821. New media files are from @guest271314 in the review.
ea041ce
to
d42eea3
Compare
@guest271314 I've applied the PR you sent against this branch and rebased it, let's see what happens now. |
@foolip Do not currently have access to Safari to test. Tried both WebM and MP4 videos at Epiphany (3.28.5) to simulate Safari. Neither video resized. Tried to install Epiphany Technology Preview got error
Therefore currently do not have a means to verify Safari output, which is currently for MP4
and for WebM
Am not sure what the message
indicates for Firefox at https://wpt.fyi/results/html/semantics/embedded-content/the-video-element/resize-during-playback.html?label=pr_head&max-count=1&pr=19030 ? |
https://wpt.fyi/results/html/semantics/embedded-content/the-video-element/resize-during-playback.html?label=pr_head&max-count=1&pr=19030 looks good, the files must be OK because both mp4 and webm pass in Chrome. Looks like Safari doesn't handle resizing mp4 files. So I'll go ahead and merge this. @stephenmcgruer FYI, this won't show up as a browser-specific failure because of codec support differing. I don't have any ideas for how to fix this other than changing the query. |
To be clear, the case you are worried about is that: i. codec support is not relevant for this test (unknown to me offhand if it's spec'd at all), and I think this is probably rare enough to just leave as is, we don't want to start including precondition_failed in browser-specific failure stats. Though I am quite concerned that people are going to use it incorrectly (specifically in cases where a browser doesn't support something that is spec'd and does matter for the test), but time will tell... |
Note, Firefox does support mp4 playback. |
Oh, my bad. It seemed to be failing the precondition based on https://wpt.fyi/results/html/semantics/embedded-content/the-video-element/resize-during-playback.html (the precondition is https://github.com/web-platform-tests/wpt/pull/19030/files#diff-785bb71c11bc7b9e3c0fb147340ea8b9R15) However trying https://output.jsbin.com/vequduj/quiet (which just checks |
What does
expect the output of
and
to be? Why is the term Even Chromium outputs
|
|
Why is the precodition
test failing where Firefox 70 and Nightly 72 both output |
I think @stephenmcgruer is right in
that is, Firefox as it's run for wpt.fyi doesn't seem to support it. I'm not sure why, or if this is a known thing. |
As described at web-platform-tests/wpt.fyi#1648 (comment) there needs to be a means to set and get preferences/flags for browsers run at wpt.fyi. Instead of guessing why certain very basic or experimental features of a given browser are either defined and implemented or not (e.g., In the meantime, checking Am not certain why the What we do know is that currently Firefox does not support playback of Matroska (.mkv) container (https://bugzilla.mozilla.org/show_bug.cgi?id=1422891; https://bugzilla.mozilla.org/show_bug.cgi?id=1562862) which means we can rely on Firefox throwing a
when an Using that fact we can use the same pattern to test if the video plays by executing |
Note, Firefox will throw an error at |
Firefox 70 and Nightly 73 pass Run in your browser on w3c-test.org. What does Safari output when the test is run in the browser? |
There seems to be a lot of cross-talk here. I'm going to try and address what I consider the most pertinent points.
I do not disagree with your statement. It should be easy for people to understand what flags runs on wpt.fyi were run with. That said, tests should not be written assuming anything about wpt.fyi runs - they should be written to the specs. It is also infeasible to fully describe the environment that leads to a certain result in wpt.fyi; for example in this case I could believe that the behavior we are seeing (Firefox reporting the empty string for I have opened #20581 to look into this.
If Chromium is returning an empty string for mkv, but supports mkv in i. If browsers are widely violating the spec, and we can get agreement that the violation is reasonable, we change the spec. Otherwise we treat deviations from specs as browser bugs.
Given that
Based on a browserstack run, Safari 13 on Catalina reports "http://w3c-test.org/html/semantics/embedded-content/the-video-element/resize-during-playback.html" for mp4 and PRECONDITION_FAILED for webm. That is the same result as we get from our Azure Pipelines runs of Safari that are shown on wpt.fyi. |
Fixes #17821.
New media files are from that issue.