Re: [webkit-dev] Using ref tests for repaint bugs
I want to give everyone an update on how things are evolving. For now, I've focused most of the development on the Mac platform. For the ref-tests-as-repaint-tests path, I've added an optional parameter to LTC.display() that should control if the repaint rectangles are tracked or not. If not specified or true, the repaint rectangles are tracked. After running very basic experiments, this seems to detect if repaint artifacts are present in a test. For the repaint-rects-as-repaint-tests path, I have some questions about what should be the best testing methodology. I'm thinking about something along these lines: * I don't think a new script option or DRT parameter are necessary; the repaint rectangles for a test can be extracted together with the pixel results * The format for the rectangles data should be simple, maybe X,Y,W,H separated by new-lines? Or the smallest rectangle that includes all of the repaint rectangles? Or another suggestion? * When initially running a repaint test, both a PNG and RR (repaint-rectangles) files are generated as baselines. At subsequent executions of the test, the RR expected output is preferred over the pixel results. This means that if the test author modifies the expected PNG, the test should still pass because the RR baseline exists. However, if a pixel test in preferred, deleting the expected RR output will force the test runner to fallback on the classic image diff code path. * A port may chose to repaint everything, even for partial canvas invalidation. Do you think it's correct to evaluate a test as passed if the expected repaint rectangles are included in the actual repaint rectangles? The simplest and straightforward way to compare the actual results with the expected ones is a simple bytes diff, but I'm not convinced it's correct. Any thoughts on this? The chromium DRT project has a different strategy for masking with the repaint rectangles; could someone help me understand better what happens in the WebViewHost::paintInvalidatedRegion() method and why exactly is iterating 3 times through that code necessary? (the comment looks a little obsoleted and I'd appreciate some clarifications). Overall, I see some difficulties in keeping the repaint rectangles based testing to be platform independent. Do you have any suggestions that may help with this? Once I'm finished patching the Python scripts to support the RR tests, I'll upload a patch so people can look over the approach. Thx, Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
On Fri, May 25, 2012 at 4:05 PM, Julien Chaffraix julien.chaffr...@gmail.com wrote: I second Simon's fragility argument. His solution is more viable, platform agnostic and may even remove the need for dumping the pixel in some cases. Even if the existing harness has the subsequent paint issue, having the pixels for the repaint test is one of the few cases where they are useful IMHO (sometimes the exact area to repaint is a judgement call, see e.g. the differences between the platforms). Also there are several of techniques to make the pixel dump platform-agnostic which could minimize the number of baselines. On top of that, if both your test case and reference hit the same bug, it nullifies the value of your test - this may be a theoretical issue though as pointed out in the pixel vs ref-test thread. I didn't understand from your proposal how you would avoid this risk. Thanks, Julien In some cases I don't see how the reference and test files can hit the same bug that you are testing for if the test file is correctly designed. Imagine a simple scenario with a background repaint bug. The test document has an element that's 50px tall with a background color of red. After making the element 25px tall, the surface is not correctly cleared and the red background remains 50px tall. If the reference document contains the element directly with 25px height, it's pretty impossible WebKit will ever draw it with a 50px tall background, thus the ref-test will detect if that specific repaint bug exists. I never said ref-tests can replace all the pixel tests, but for bugs like the one I've mentioned above they seem to do quite a good job. Simon's idea sounds very good as well, but it has the disadvantage of a maintenance cost greater than the one for a ref-test. I suppose the best thing is to have both of them implemented and based on the specificity of the bug tested, use one or another. For straightforward repaint bugs, ref tests sound good. For more complex bugs, comparing repaint rectangles seems better. I'll start prototyping both of them as it doesn't look extremely difficult to extract the repaint rectangle information. Afterwards I'll try to see how both approaches can be applied to the current repaint testing battery. Thanks, Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
Andrei, On 05/25/2012 02:43 AM, Andrei Bucur wrote: Ojan, As Simon states, some repaint tests will likely not be possible to write correctly as ref tests, but some of them I think they fit very well in the happy-no-pixel-test bucket :). If people decide it's a direction worth investigating, I'll give the idea a spin. I'd be interested to hear about your progress. Recently I was facing a couple of similar issues: https://bugs.webkit.org/show_bug.cgi?id=73409 and then GTK decided to always do repaints before dumping pixel results: https://bugs.webkit.org/show_bug.cgi?id=86284 which might not be the right solution either - so if you come up with something more fine-grained - that'd be great. Dominik ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
I think this will be too fragile. It relies on the fact that a subsequent paint won't obscure the repaint bug that you're trying to detect. I'm much rather we work towards detecting repaint bugs via dumping the list of repaint rectangles. I don't think every test has to be a ref test. Don't the pixel tests face the same kind of risk, of a subsequent paint masking the repaint bug? Preserving this risk, but reducing the maintenance cost for the test is still an important improvement. I second Simon's fragility argument. His solution is more viable, platform agnostic and may even remove the need for dumping the pixel in some cases. Even if the existing harness has the subsequent paint issue, having the pixels for the repaint test is one of the few cases where they are useful IMHO (sometimes the exact area to repaint is a judgement call, see e.g. the differences between the platforms). Also there are several of techniques to make the pixel dump platform-agnostic which could minimize the number of baselines. On top of that, if both your test case and reference hit the same bug, it nullifies the value of your test - this may be a theoretical issue though as pointed out in the pixel vs ref-test thread. I didn't understand from your proposal how you would avoid this risk. Thanks, Julien ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
On Fri, May 25, 2012 at 9:57 AM, Dominik Röttsches dominik.rottsc...@intel.com wrote: Andrei, On 05/25/2012 02:43 AM, Andrei Bucur wrote: Ojan, As Simon states, some repaint tests will likely not be possible to write correctly as ref tests, but some of them I think they fit very well in the happy-no-pixel-test bucket :). If people decide it's a direction worth investigating, I'll give the idea a spin. I'd be interested to hear about your progress. Recently I was facing a couple of similar issues: https://bugs.webkit.org/show_bug.cgi?id=73409 and then GTK decided to always do repaints before dumping pixel results: https://bugs.webkit.org/show_bug.cgi?id=86284 which might not be the right solution either - so if you come up with something more fine-grained - that'd be great. Here's why a repaint is forced for the Gtk port before dumping pixel results: Gtk port uses a backing store which is updated every 0.017 seconds, i.e. 60 times per second, a reasonable interval. The problem is that many tests are structured in the way that they make an alteration that produces a layout change and then immediately finish. This leaves no time for the backing store to be updated. It would also be ridiculous to go around fixing tests by delaying the finish by any amount of time. Because of that a repaint is forced just before dumping the pixel results so the dirty regions of the backing store (only those and not the complete backing store) are updated. Regards, Zan Dominik ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
On Fri, May 25, 2012 at 1:42 PM, Žan Doberšek zandober...@gmail.com wrote: On Fri, May 25, 2012 at 9:57 AM, Dominik Röttsches dominik.rottsc...@intel.com wrote: Andrei, On 05/25/2012 02:43 AM, Andrei Bucur wrote: Ojan, As Simon states, some repaint tests will likely not be possible to write correctly as ref tests, but some of them I think they fit very well in the happy-no-pixel-test bucket :). If people decide it's a direction worth investigating, I'll give the idea a spin. I'd be interested to hear about your progress. Recently I was facing a couple of similar issues: https://bugs.webkit.org/show_bug.cgi?id=73409 and then GTK decided to always do repaints before dumping pixel results: https://bugs.webkit.org/show_bug.cgi?id=86284 which might not be the right solution either - so if you come up with something more fine-grained - that'd be great. Here's why a repaint is forced for the Gtk port before dumping pixel results: Gtk port uses a backing store which is updated every 0.017 seconds, i.e. 60 times per second, a reasonable interval. This is reasonable for an actual browser, but not reasonable for DumpRenderTree. - James The problem is that many tests are structured in the way that they make an alteration that produces a layout change and then immediately finish. This leaves no time for the backing store to be updated. It would also be ridiculous to go around fixing tests by delaying the finish by any amount of time. Because of that a repaint is forced just before dumping the pixel results so the dirty regions of the backing store (only those and not the complete backing store) are updated. Regards, Zan Dominik ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
Do you just need to force a layout at the end of repaintTest, e.g. document.body.offsetHeight;? On Thu, May 24, 2012 at 6:34 AM, Andrei Bucur andrei.bu...@gmail.comwrote: Hello WebKittens, I'm trying to simplify the patch for a certain repaint bug ( https://bugs.webkit.org/show_bug.cgi?id=59863 ) by using ref tests instead of pixel tests. The test HTML file should first render the document in state 1 and then, by modifying the DOM, render it again state 2. The bug appears when repainting from state 1 to state 2. The expected result HTML for the test is just a document directly written in state 2 (so there is no transition, no bug). This should work on all the platforms but there's problem with the time control between paints for state 1 and state 2 in the test HTML. I've tried three options: 1. Use setTimeout() to update the DOM - I really wouldn't like to use this because it slows down the test and can be flaky. 2. Use layoutTestController.display() before modifying the DOM to trigger a paint - calling this seems to make the test fail because display() starts tracking the paint rects which doesn't happen in the reference document. 3. Make use of requestAnimationFrame to time the paintings for state 1 and state 2 - doesn't work directly out-of-the-box because requestAnimationFrame schedules a full repaint after going into state 2 and the bug doesn't reproduce anymore. Is there a preferred solution to this problem or another one that I'm missing? Thanks, Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
No, I need a way to force a paint operation, similar to what layoutTestController.display() achieves, but without tracking the paint rectangles. Does anyone you find value in adding an optional parameter to display (or create another method on LTC) that disables paint rectangle tracking. The main advantage for such a change would be in extending the ref tests power to also cover repaint tests (at least a part of them), thus making pixel tests less necessary. Repaint bugs appear usually when the layout changes but the old pixels are incorrectly invalidated. By making the reference HTML directly identical to the test document after the bug triggering changes have been applied, it is pretty certain that the output pixels for the reference file will not contain the repaint artifacts (there's no repaint operation involved). When running such a test with a client that has the bug, it will fail because the test file output will have artifacts and the reference output will not. On the other hand, using a client with the repaint bug fixed, the pixels output for the test and reference files should be identical. Thoughts? Andrei. On Thu, May 24, 2012 at 11:57 PM, Ojan Vafai o...@chromium.org wrote: Do you just need to force a layout at the end of repaintTest, e.g. document.body.offsetHeight;? On Thu, May 24, 2012 at 6:34 AM, Andrei Bucur andrei.bu...@gmail.comwrote: Hello WebKittens, I'm trying to simplify the patch for a certain repaint bug ( https://bugs.webkit.org/show_bug.cgi?id=59863 ) by using ref tests instead of pixel tests. The test HTML file should first render the document in state 1 and then, by modifying the DOM, render it again state 2. The bug appears when repainting from state 1 to state 2. The expected result HTML for the test is just a document directly written in state 2 (so there is no transition, no bug). This should work on all the platforms but there's problem with the time control between paints for state 1 and state 2 in the test HTML. I've tried three options: 1. Use setTimeout() to update the DOM - I really wouldn't like to use this because it slows down the test and can be flaky. 2. Use layoutTestController.display() before modifying the DOM to trigger a paint - calling this seems to make the test fail because display() starts tracking the paint rects which doesn't happen in the reference document. 3. Make use of requestAnimationFrame to time the paintings for state 1 and state 2 - doesn't work directly out-of-the-box because requestAnimationFrame schedules a full repaint after going into state 2 and the bug doesn't reproduce anymore. Is there a preferred solution to this problem or another one that I'm missing? Thanks, Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
On Thu, May 24, 2012 at 3:59 PM, Andrei Bucur andrei.bu...@gmail.comwrote: No, I need a way to force a paint operation, similar to what layoutTestController.display() achieves, but without tracking the paint rectangles. Does anyone you find value in adding an optional parameter to display (or create another method on LTC) that disables paint rectangle tracking. I don't see any problems with this. Someone more familiar with the paint code might see issues. The main advantage for such a change would be in extending the ref tests power to also cover repaint tests (at least a part of them), thus making pixel tests less necessary. Repaint bugs appear usually when the layout changes but the old pixels are incorrectly invalidated. By making the reference HTML directly identical to the test document after the bug triggering changes have been applied, it is pretty certain that the output pixels for the reference file will not contain the repaint artifacts (there's no repaint operation involved). When running such a test with a client that has the bug, it will fail because the test file output will have artifacts and the reference output will not. On the other hand, using a client with the repaint bug fixed, the pixels output for the test and reference files should be identical. Thoughts? I'm open to experimenting with this, but different ports have very different repaint schemes (e.g. Chromium merges all the repaint rects into a large rect). I'm curious how likely a repaint reftest is to work across ports. If you're willing to investigate converting a handful of repaint tests over to reftests, that'd be very helpful. FWIW, you can easily test the reftests on the chromium port by sending a patch to the EWS via bugzilla since the Chromium-linux EWS runs the layout tests. Obviously, if we can convert repaint tests reftests (or, at least have new repaints tests be reftests), that would be a huge improvement in maintainability since repaint tests are one of the few categories of tests where we can't currently use reftests. Andrei. On Thu, May 24, 2012 at 11:57 PM, Ojan Vafai o...@chromium.org wrote: Do you just need to force a layout at the end of repaintTest, e.g. document.body.offsetHeight;? On Thu, May 24, 2012 at 6:34 AM, Andrei Bucur andrei.bu...@gmail.comwrote: Hello WebKittens, I'm trying to simplify the patch for a certain repaint bug ( https://bugs.webkit.org/show_bug.cgi?id=59863 ) by using ref tests instead of pixel tests. The test HTML file should first render the document in state 1 and then, by modifying the DOM, render it again state 2. The bug appears when repainting from state 1 to state 2. The expected result HTML for the test is just a document directly written in state 2 (so there is no transition, no bug). This should work on all the platforms but there's problem with the time control between paints for state 1 and state 2 in the test HTML. I've tried three options: 1. Use setTimeout() to update the DOM - I really wouldn't like to use this because it slows down the test and can be flaky. 2. Use layoutTestController.display() before modifying the DOM to trigger a paint - calling this seems to make the test fail because display() starts tracking the paint rects which doesn't happen in the reference document. 3. Make use of requestAnimationFrame to time the paintings for state 1 and state 2 - doesn't work directly out-of-the-box because requestAnimationFrame schedules a full repaint after going into state 2 and the bug doesn't reproduce anymore. Is there a preferred solution to this problem or another one that I'm missing? Thanks, Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
On May 24, 2012, at 3:59 PM, Andrei Bucur wrote: No, I need a way to force a paint operation, similar to what layoutTestController.display() achieves, but without tracking the paint rectangles. Does anyone you find value in adding an optional parameter to display (or create another method on LTC) that disables paint rectangle tracking. The main advantage for such a change would be in extending the ref tests power to also cover repaint tests (at least a part of them), thus making pixel tests less necessary. Repaint bugs appear usually when the layout changes but the old pixels are incorrectly invalidated. By making the reference HTML directly identical to the test document after the bug triggering changes have been applied, it is pretty certain that the output pixels for the reference file will not contain the repaint artifacts (there's no repaint operation involved). When running such a test with a client that has the bug, it will fail because the test file output will have artifacts and the reference output will not. On the other hand, using a client with the repaint bug fixed, the pixels output for the test and reference files should be identical. Thoughts? I think this will be too fragile. It relies on the fact that a subsequent paint won't obscure the repaint bug that you're trying to detect. I'm much rather we work towards detecting repaint bugs via dumping the list of repaint rectangles. I don't think every test has to be a ref test. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using ref tests for repaint bugs
Ojan, As Simon states, some repaint tests will likely not be possible to write correctly as ref tests, but some of them I think they fit very well in the happy-no-pixel-test bucket :). If people decide it's a direction worth investigating, I'll give the idea a spin. Simon, On Fri, May 25, 2012 at 2:07 AM, Simon Fraser simon.fra...@apple.comwrote: I think this will be too fragile. It relies on the fact that a subsequent paint won't obscure the repaint bug that you're trying to detect. I'm much rather we work towards detecting repaint bugs via dumping the list of repaint rectangles. I don't think every test has to be a ref test. Don't the pixel tests face the same kind of risk, of a subsequent paint masking the repaint bug? Preserving this risk, but reducing the maintenance cost for the test is still an important improvement. Thanks, Andrei. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
