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

Import APNG tests from https://philip.html5.org/tests/apng/tests.html #5546

Closed
foolip opened this issue Apr 12, 2017 · 7 comments
Closed

Import APNG tests from https://philip.html5.org/tests/apng/tests.html #5546

foolip opened this issue Apr 12, 2017 · 7 comments

Comments

@foolip
Copy link
Member

foolip commented Apr 12, 2017

https://philip.html5.org/tests/apng/tests.html

If any of these cannot be converted to reftests, then some more bugs with the "infra" label might be in order.

@LeonScroggins

@gsnedders
Copy link
Member

There's a number that test animation which inherently cannot be reftests.

@foolip
Copy link
Member Author

foolip commented Apr 13, 2017

What kinds of APIs would we need on the web platform or as part of WebDriver/something in order to be able to test animation?

@LeonScroggins
Copy link
Contributor

TL;DR: Something like Chromium's advanceImageAnimation (advance to a particular frame) would allow us to test more.

I think it would depend on exactly what we're testing:

  • Some of the images on https://philip.html5.org/tests/apng/tests.html are essentially static images, either because the APNG only has one frame, or because there is an error in the APNG so we should show the default image (i.e. ignore APNG chunks and treat it like a static PNG). We should be able to turn these into reftests without anything new.
  • I added a simple test (Add a web platform test for APNG  #5543) with an image with two frames. The first frame displays briefly, and the animation stops on the second frame. We then compare to a static image that looks like the second frame. This test relies on setTimeout to delay the comparison (the first frame is 11ms, and we wait a full second), although maybe it shouldn't, because that can be flaky? (We generally avoid using setTimeout in Chromium's LayoutTests due to flakiness. Also, Can step_timeout be used in reftests? #5547 reports that a warning states to not use setTimeout.)
  • We could expand on the above for more complex cases (e.g. the LayoutTest I added at https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/images/animated-png.html?q=animated-png+package:%5Echromium$&l=1). Since there are more frames to cycle through (as many as 12 for some of these images, and each one has to be at least 11ms long, or Chromium (at least) will increase the frame duration), we might need to increase the timeout time, and it still might be flaky. Instead, it currently relies on (what I believe are) Chromium-specific methods (advanceImageAnimation) to advance frames, so we can ensure we're showing the final frame before we compare. Something like advanceImageAnimation would be necessary for running this test reliably, although it wouldn't verify that we advance properly when not using the special methods. This kind of test is useful for verifying that we've successfully blended frames together, and that we show the proper frame based on its index (assuming we advance properly). (This test only checks the final frame, but a more complex test could be written to check earlier frames.)
  • I don't know how we can test that particular frames are shown at particular times. We have unit tests in Chromium (C++ code) that verify that we read proper values from the APNG (e.g. frame durations, number of times to loop, etc), but do not attempt to test that after e.g. 11 ms we show the second frame.

@foolip
Copy link
Member Author

foolip commented Apr 21, 2017

Would something like advanceImageAnimation be useful for web authors as well, or would it be better to pursue that as a test-only API? How does it work, does it synchronously decode up the requested frame?

@LeonScroggins
Copy link
Contributor

LeonScroggins commented Apr 21, 2017

Would something like advanceImageAnimation be useful for web authors as well

I am not a web author myself, so I am not the right person to ask, but it could be. Maybe they want to synchronize images (I might be wrong about this, but I think different browsers behave differently with respect to keeping animated images in sync when they go off screen) or change what frame they're on relative to each other, but that might be an inelegant way to do what they want to do.

does it synchronously decode up the requested frame?

It advances one frame at a time, but it could be made to advance to an index, I suppose. I think it is synchronous, although I'm not positive. It looks to change the current frame and then notify observers, and I think that will result in redrawing with the new frame.

@LeonScroggins
Copy link
Contributor

does it synchronously decode up the requested frame?

It advances one frame at a time, but it could be made to advance to an index, I suppose. I think it is
synchronous, although I'm not positive. It looks to change the current frame and then notify observers,
and I think that will result in redrawing with the new frame.

Oops - Chromium won't synchronously decode; decoding is done in another thread.

@svgeesus
Copy link
Contributor

Just came across this issue, after having done a manual conversion of these tests to WPT. Mostly reftests, but a few have to be -manual

Also I used setTimeout which is apparently forbidden but the documentation on what to use instead is sparse.

    const el = document.querySelector(".reftest-wait");
    setTimeout(() => {
        el.classList.remove('reftest-wait');
        }, "3000");

This is just to get a snapshot after the animation has finished. The reference is a static PNG of the end of the animation, usually the last frame.

I also added rel="help" links to the actual testable assertion in the spec.

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

No branches or pull requests

4 participants