Port APNG tests to WPT, add test assertions. Fix #5546 #42893
Conversation
|
@ProgramMax could you review? |
Will do. Lot of new files so give me a sec. :)
In a separate pull request, should we move the existing |
Yes, my thought exactly. Firstly to do it, and secondly in a separate PR so as not to clutter up this one. |
The Animation durations were calculated by examining the images in a modified version of PNG file chunk inspector which I extended to add APNG support and is the total of all the delays. Note that delays in the test PNGs are (mostly) in 1/100 sec while the delay in setTimeout is in ms, so 10x as large. I added an additional 1000ms delay on top to be sure the animation was complete before the comparison is made. Tests on WPT should not use |
|
Actually it seems that https://www.nayuki.io/page/png-file-chunk-inspector has been updated to support APNG since I forked and updated a couple of years ago, so it can be used without modification. |
There was a problem hiding this comment.
Is the html tag now a void tag? Shouldn't it be closed?
Is that to do with the timeout that removes the 'reftest-wait' class?
png/apng/acTL-plays-one.html and png/apng/acTL-plays-two.html both do not have a closing html tag. This is unlike png/apng/apng-blue-rect-checkerboard-ref.html, for example. There are also some which don't have any html tags at all.
Also, should these get the pixel color and assert the test passes? I'm guessing there is no great way to paint a specific frame of an APNG into a canvas to read it back, is there? Does it auto-capture and compare against some sort of gold master?
In XHTML it must be closed. In HTML5 it need not be.
Unrelated, except that apparently that class should be on "the root element" which is html.
That is allowed by HTML5.
For a reftest, which most of these are, the WPT test runner auto captures and it is then compared to an auto capture of the reference file, both rendered on the same browser and platform to avoid differences in fonts or anti-aliasing. The reference is indicated by the rel="match" link. See
If they don't need the delay class on the root element then there is no need to type the html opening tag, the parser adds it automatically. |
|
@jgraham @foolip it seems that review from @ProgramMax (PNG WG chair) is not sufficient despite being listed in META.yml. Could you add whatever is needed, please? |
See the table of animation times in the original post, which are in 100ths of a second. I added a one-second padding after the end of the animation, is that too much? The original page said:
|
|
So I looked at https://philip.html5.org/tests/apng/tests.html and it seems unfortunate we're losing the ability to generate these tests in the process of adopting them here. Did you look at repurposing the script in some manner? We have a number of existing places where we also commit the script used to generate the tests, e.g., https://github.com/web-platform-tests/wpt/blob/master/mimesniff/mime-types/resources/generated-mime-types.py. |
|
Hi @annevk Yes, I had looked at the perl script, sure. It uses Cairo to do the heavy lifting, and scrapes the all-in-one html file of the original tests to get a recipe for each test file. In the original test these are preserved in a special link like Which is url-encoded from the html source in a custom element, <png>
IHDR => [W, H],
acTL => [1, 0],
fcTL => [0, W, H, 0, 0, 100, 100, DISPOSE_NONE, BLEND_OVER],
IDAT => [create_surface(W, H, 'green')],
IEND => [],
</png>Agreed, it is certainly nice to be able to make those test images from a text description like that, particularly for more complex cases like <div class="case">
<p>APNG_BLEND_OP_OVER repeatedly with nearly-transparent colours.
<p>This should be solid green.
<png>
IHDR => [W, H],
acTL => [128, 1],
IDAT => [create_surface(W, H, 'red')],
fcTL => [0, W, H, 0, 0, 10, 100, DISPOSE_NONE, BLEND_OVER],
fdAT => [1, create_surface(W, H, 'solid', 0, 1, 0, 1)],
(map {
(fcTL => [2+$_*2, W/2, H, 0, 0, 1, 100, DISPOSE_NONE, BLEND_OVER],
fdAT => [3+$_*2, create_surface(W/2, H, 'solid', 0, 1, 0, 0.01)])
} 0..126),
IEND => [],
</png>
</div>but given that the PNG files are all there, and those are what people have been using since 2007 to test APNG (in browsers, and in APNG consuming or generating tools) I wanted to have the exact same one here in WPT. For example I see the original files as part of a PR for APNG read support in libspng So I deferred any further investigation until there was a need to generate similar files which were not part of the original suite. Note that the filename is not stored in the recipe, it just uses sequential three-digit numbers as the name; so any insertion of new ones need to be at the end to avoid breaking earlier tests. I'm happy to put the perl script and the original Note that, as I said in the first message, the original suite includes a bunch of tests on broken images, which I plan to add here after is resolved because we do not have browser interop there. Chrome follows the original (Mozilla) APNG spec there while Firefox does not :) and the spec is arguably unclear on the error recovery in some cases and needs t be tightened up. That conversation would probably be easier if the original files were the subject of discussion, too. |
Done with 9e1fd74 and I added a README too |
|
@annevk does that satisfy your concern about regenerating images? |
|
Thank you! Might need to rebase and force push to make the bots happy. Failing that we'll have to ask an admin. |
|
Thanks @annevk |
|
Right, hence my recommendation to rebase (on top of latest in master) and force push this branch. |
|
|
It sounds like you didn't get do |
|
So I am not comfortable force pushing to master on a project of this size and importance. Link sometimes getting stuck is a bug, not an issue with the content of this PR and I prefer a WPT admin look into that and fix it. |
|
You most definitely shouldn't force-push to master. The lint appears to have failed legitimately: |
|
I wasn't suggesting to force push to master. I was saying that it sounded like you hadn't pulled from master as git rebase yielded no up-to-date changeset while it should have. |
This is a port of https://philip.html5.org/tests/apng/tests.html see
These tests use setTimeout, which has been added to the
lint.ignore. Most tests are reftests, and the reference is a png that captures the end state of the animation (created with apngasm disassembly).A few are manual tests, those are in the
manualsub-folder.The tests are under
/png/apngbecause APNG is now part of the PNG specification.To help in reviewing, these are the animation durations of the test files (in 1/100 sec):
I did not yet port the "error in file" tests because those need more discussion: