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

Media Capabilities: make framerate a double instead of a DOMString. #18440

Merged
merged 2 commits into from Nov 20, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Aug 15, 2019

Spec change: w3c/media-capabilities#128

Bug: 994017
Change-Id: Iab036264fe19a6676c97bb12648321408d91f283
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755046
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716773}

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.

Already reviewed downstream.

Spec change: w3c/media-capabilities#128

Bug: 994017
Change-Id: Iab036264fe19a6676c97bb12648321408d91f283
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755046
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716773}
@stephenmcgruer
Copy link
Contributor

Looks like a slow test (on Chrome dev, at least):

 1:07.56 INFO ## Slow tests ##

 1:07.56 INFO |                     Test                    | Result | Longest duration (ms) | Timeout (ms) |
 1:07.56 INFO |---------------------------------------------|--------|-----------------------|--------------|
 1:07.56 INFO | `/media-capabilities/decodingInfo.any.html` | `OK`   | `10109`               | `10000`      |
 1:07.56 INFO 
 1:07.56 INFO ::: Running tests in a loop 10 times : PASS
 1:07.56 INFO ::: Running tests in a loop with restarts 5 times : FAIL
 1:07.56 INFO :::
 1:07.56 ERROR ::: Test verification FAIL

I suspect the test was already slow based on the changes in content, and we had just been scraping the edge for a while. Need to check if its marked slow already - if not, then let's do that. In either case, will warn the author of the CL after we mark the test as slow. @KyleJu do you have bandwidth available to do that?

@KyleJu
Copy link
Contributor

KyleJu commented Nov 20, 2019

Looks like a slow test (on Chrome dev, at least):

 1:07.56 INFO ## Slow tests ##

 1:07.56 INFO |                     Test                    | Result | Longest duration (ms) | Timeout (ms) |
 1:07.56 INFO |---------------------------------------------|--------|-----------------------|--------------|
 1:07.56 INFO | `/media-capabilities/decodingInfo.any.html` | `OK`   | `10109`               | `10000`      |
 1:07.56 INFO 
 1:07.56 INFO ::: Running tests in a loop 10 times : PASS
 1:07.56 INFO ::: Running tests in a loop with restarts 5 times : FAIL
 1:07.56 INFO :::
 1:07.56 ERROR ::: Test verification FAIL

I suspect the test was already slow based on the changes in content, and we had just been scraping the edge for a while. Need to check if its marked slow already - if not, then let's do that. In either case, will warn the author of the CL after we mark the test as slow. @KyleJu do you have bandwidth available to do that?

Marked as a slow test and left a comment on the CL

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 2142380 into master Nov 20, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1755046 branch November 20, 2019 15:40
@mounirlamouri
Copy link
Contributor

@KyleJu @stephenmcgruer how does "Running tests in a loop with restarts 5 times" translates in Chrome's web_tests runner? I can't reproduce this and I wonder if I don't use the right configs or if it's because WPT is using //chrome while we do not internally in Blink. @Rob--W @foolip FYI

@chcunningham I wonder if the issue could be the loading of the DB assuming the above is correct and it can't be repro in Blink.

@stephenmcgruer
Copy link
Contributor

Hey, sorry for the delay @mounirlamouri . "Running tests in a loop with restarts 5 times" would be approximately equivalent to passing --repeat-each=5 and --restart-shell-between-tests to run_web_tests.py (cc @LukeZielinski). However what we're actually looking for here is that any single run of the test took long enough that it threatened to timeout, so a single run should reproduce it.

Locally running this a few times, I got one result of 9.8 seconds which is close to the timeout, and one result of 10.6 that is over it:

...
Ran 1 tests finished in 9.8 seconds.
$ ./wpt run --no-pause-after-test --binary ~/chromium/src/out/Release/chrome chrome media-capabilities/decodingInfo.any.html
...
Ran 1 tests finished in 10.6 seconds.

Now I'm using a local build so not as fast as a release one (which is what wpt.fyi uses), but still a quite slow test!

Also if you look at https://wpt.fyi/results/media-capabilities/decodingInfo.any.html?label=experimental&label=master&aligned, Chrome took 2.927 seconds to run this test file (on the run I looked at) vs 0.42 seconds for Firefox.

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

6 participants