Enable animation tests on WebRender

RESOLVED FIXED in mozilla61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
6 months ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks 2 bugs)

61 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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
Though there are still failing tests, we should enable tests that pass on WebRender.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1441cce8f0aab435fa30d62d2ef6b5155a5135b8
Keywords: leave-open
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 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)
(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.
(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.
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.
Let's spin off a separate bug for fixing the tests still not enabled in this bug.
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 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+
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
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)
Depends on: 1456679
(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: Last year
Flags: needinfo?(bugmail)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: unspecified → 61 Branch
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)
The test case unfortunately didn't fail on some tries.  I will take care of the failure test in bug 1456372.
Flags: needinfo?(hikezoe)
See Also: → 1520705
You need to log in before you can comment on or make changes to this bug.