Closed
Bug 1455999
Opened 6 years ago
Closed 6 years ago
Enable animation tests on WebRender
Categories
(Core :: Graphics: WebRender, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
There are still disabled test cases for animations on WebRender. We should enabe them as far as possible. Just pushed a try run. Let's see what happens. https://treeherder.mozilla.org/#/jobs?repo=try&revision=166cafc3810071802cf725687fa259ef838ed94e
Assignee | ||
Comment 1•6 years ago
|
||
Though there are still failing tests, we should enable tests that pass on WebRender. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1441cce8f0aab435fa30d62d2ef6b5155a5135b8
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8970404 [details] Bug 1455999 - Drop isStyleByServo() in dom/animation/. https://reviewboard.mozilla.org/r/239178/#review244874
Attachment #8970404 -
Flags: review?(bbirtles) → review+
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8970405 [details] Bug 1455999 - Skip the visibility hidden test case on WebRender. https://reviewboard.mozilla.org/r/239180/#review244876 ::: dom/animation/test/mozilla/file_restyles.html:720 (Diff revision 1) > > await animation.ready; > + if (!isWebRender) { > - ok(!SpecialPowers.wrap(animation).isRunningOnCompositor); > + ok(!SpecialPowers.wrap(animation).isRunningOnCompositor); > + } else { > + // FIXME: Bug 1455999: Enable this check on WebRender too. This seems like the wrong bug number? Shouldn't this point to the bug where we plan to fix this?
Attachment #8970405 -
Flags: review?(bbirtles)
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #6) > Comment on attachment 8970405 [details] > Bug 1455999 - Skip the visibility hidden test case on WebRender. > > https://reviewboard.mozilla.org/r/239180/#review244876 > > ::: dom/animation/test/mozilla/file_restyles.html:720 > (Diff revision 1) > > > > await animation.ready; > > + if (!isWebRender) { > > - ok(!SpecialPowers.wrap(animation).isRunningOnCompositor); > > + ok(!SpecialPowers.wrap(animation).isRunningOnCompositor); > > + } else { > > + // FIXME: Bug 1455999: Enable this check on WebRender too. > > This seems like the wrong bug number? Shouldn't this point to the bug where > we plan to fix this? I plan to fix it in this bug at this moment, if it turns out that much more work will be needed for, I will re-number it and file a new bug when I close this bug.
Comment 8•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7) > (In reply to Brian Birtles (:birtles) from comment #6) > > This seems like the wrong bug number? Shouldn't this point to the bug where > > we plan to fix this? > > I plan to fix it in this bug at this moment, if it turns out that much more > work will be needed for, I will re-number it and file a new bug when I close > this bug. Ok. I don't understand why this patch is up for review then. Either we're fixing it here and we don't need this patch, or we aren't and this patch needs to be changed to point to where it will be fixed, and to explain the issue.
Assignee | ||
Comment 9•6 years ago
|
||
That's because I'd want to enable those tests as soon as possible. I did make a regression recently (bug 1455841), and it should have caught in some test cases.
Comment 10•6 years ago
|
||
Let's spin off a separate bug for fixing the tests still not enabled in this bug.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8970405 [details] Bug 1455999 - Skip the visibility hidden test case on WebRender. https://reviewboard.mozilla.org/r/239180/#review244892
Attachment #8970405 -
Flags: review?(bbirtles) → review+
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8970406 [details] Bug 1455999 - Enable animation tests on WebRender. https://reviewboard.mozilla.org/r/239182/#review244952 This is great, thank you!
Attachment #8970406 -
Flags: review?(bugmail) → review+
Updated•6 years ago
|
Blocks: stage-wr-trains
Priority: -- → P2
Comment 15•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/19a70418a35e Drop isStyleByServo() in dom/animation/. r=birtles https://hg.mozilla.org/integration/autoland/rev/5d08e0ab65ce Skip the visibility hidden test case on WebRender. r=birtles https://hg.mozilla.org/integration/autoland/rev/83c1e6e742fd Enable animation tests on WebRender. r=kats
Assignee | ||
Comment 16•6 years ago
|
||
I'd catch kats on IRC, but I couldn't get up early enough. :) Anyway I am wondering why we call DiscardCompositorAnimations before BuildWebRenderCommands?[1] (the animations will be discarded are enqueued through BuildWebRenderCommands). It seems to me that we can discard the dead animations in the same transaction. Also note that with the current setup, some of test cases fail because there remains some dead animations which should have already discarded. Actually moving the DiscardCompositorAnimations call after BuildWebRenderCommands make most of failures pass. Here is a try with the change. There are still 6 failures in test_animations_omta.html, but I have already a patch for that and will open a new bug for it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=80e1f2cb51ee17493f7da51e08b49ee3b1707593 [1] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderLayerManager.cpp#267-280
Flags: needinfo?(bugmail)
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/19a70418a35e https://hg.mozilla.org/mozilla-central/rev/5d08e0ab65ce https://hg.mozilla.org/mozilla-central/rev/83c1e6e742fd
Comment 18•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16) > Anyway I am wondering why we call DiscardCompositorAnimations before > BuildWebRenderCommands?[1] (the animations will be discarded are enqueued > through BuildWebRenderCommands). It seems to me that we can discard the > dead animations in the same transaction. Hm, I think part of the problem is that the transaction sends an updated display list to WR, but we may not render and composite that display list right away. We might still be showing the previous transaction and so we might need to have that animation info alive in the parent at the time. But I agree this is not clear and there's almost certainly a better way to do it. I need to revisit this behaviour anyway as part of bug 1453360 - my goal is to have each animation associated on the parent side with the transaction that deletes it. Then once that transaction is finally rendered, we will know that it is safe to delete those animations because the entire WR pipeline will be free of those animations. I'll look at that today. > Also note that with the current > setup, some of test cases fail because there remains some dead animations > which should have already discarded. Actually moving the > DiscardCompositorAnimations call after BuildWebRenderCommands make most of > failures pass. > > Here is a try with the change. There are still 6 failures in > test_animations_omta.html, but I have already a patch for that and will open > a new bug for it. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=80e1f2cb51ee17493f7da51e08b49ee3b1707593 That's interesting to see. If making this change doesn't break anything then it might be ok to do - it could be that my understanding of how this is supposed to work is wrong. At any rate, I'm going to close this bug, we can spin this off into a new bug as well.
Assignee: nobody → hikezoe
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(bugmail)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: unspecified → 61 Branch
Depends on: 1456372
Comment 19•6 years ago
|
||
This has been the source for https://bugzilla.mozilla.org/show_bug.cgi?id=1456372#c3 The above mentioned bug has 54 failures in the last 7 days, all on linux64-qr. Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=176157924&repo=autoland&lineNumber=4395 [task 2018-04-28T18:48:52.223Z] 18:48:52 INFO - TEST-START | dom/animation/test/mozilla/test_transition_finish_on_compositor.html [task 2018-04-28T18:48:52.731Z] 18:48:52 INFO - TEST-INFO | started process screentopng [task 2018-04-28T18:48:53.507Z] 18:48:53 INFO - TEST-INFO | screentopng: exit 0 [task 2018-04-28T18:48:53.509Z] 18:48:53 INFO - TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_transition_finish_on_compositor.html | Transition on the compositor keeps the final style while the main thread is busy even if the transition finished on the compositor - Transition on the compositor keeps the final style while the main thread is busy even if the transition finished on the compositor: assert_equals: The final transition style is still applied on the compositor expected "matrix(1, 0, 0, 1, 100, 0)" but got "matrix(1, 0, 0, 1, 91.5861, 0)" [task 2018-04-28T18:48:53.509Z] 18:48:53 INFO - TEST-PASS | dom/animation/test/mozilla/test_transition_finish_on_compositor.html | - : Elided 1 passes or known failures. [task 2018-04-28T18:48:53.510Z] 18:48:53 INFO - GECKO(4088) | MEMORY STAT | vsize 1625MB | residentFast 141MB | heapAllocated 26MB [task 2018-04-28T18:48:53.511Z] 18:48:53 INFO - GECKO(4088) | [GFX1-]: Unknown pipeline used for iframe IframeDisplayItem { clip_id: Clip(4, PipelineId(1, 1)), pipeline_id: PipelineId(1, 8) } [task 2018-04-28T18:48:53.513Z] 18:48:53 INFO - GECKO(4088) | ERROR 2018-04-28T18:48:52Z: webrender::display_list_flattener: Unknown pipeline used for iframe IframeDisplayItem { clip_id: Clip(4, PipelineId(1, 1)), pipeline_id: PipelineId(1, 8) } [task 2018-04-28T18:48:53.514Z] 18:48:53 INFO - TEST-OK | dom/animation/test/mozilla/test_transition_finish_on_compositor.html | took 581ms :hiro can you take a look?
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 20•6 years ago
|
||
The test case unfortunately didn't fail on some tries. I will take care of the failure test in bug 1456372.
Flags: needinfo?(hikezoe)
You need to log in
before you can comment on or make changes to this bug.
Description
•