1864425 - A CSS transition/animation triggered in a web extension background page will keep vsync running forever.

Status: RESOLVED FIXED

(Core :: CSS Transitions and Animations, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

This prevents me from landing bug 1863620, because we trigger the active transition on those pages, but the transition never starts, yet we keep ticking forever.

What happens is the following:

• We trigger a transition, so set it to pending here.
• That keeps requesting refresh driver ticks via this.
• But generated background pages are never painted, so we never hit this.

This is never painted because we create these in a windowLess browser here, which creates a IsNeverPainting pres shell...

So this is all kind of working as expected, except for the fact that we keep running vsync I guess and the "never painting" stuff means that we never trigger transitions and animations but are never inactive...

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1864425 - Don't keep ticking pending animations for pages that will never get painted. r=#animation

This fixes test failures with the patch in bug 1863620.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Brian, this fixes the immediate performance issue and test failures, but I wonder if we should do something else to kick the animations (or whether we shouldn't be relying on a paint happening at all).

My understanding is that this is trying to synchronize the animations with the compositor, right? But the animation ready time is basically the last tick value, it's set here once we end the transaction, from the main thread.

So basically we're using the last tick timestamp. Can we just use that, instead of relying on compositor transactions?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1864425 - Don't require a paint to trigger pending animations. r=birtles,#animation

This is an alternative (or addition) to the previous approach which is
probably more sensible, and actually doesn't keep the transitions in an
infinitely-pending state.

Use refresh driver time to sync animations between the compositor and
the main thread. That seems better anyways?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Bug 927349 had a lot more context, but I don't think most of it really applies anymore?

Comment 5

[Brian Birtles (:birtles)]

Sorry, on my phone and away from my computer for the next three days. But basically we shouldn't be setting the animation ready time to the refresh driver time. It should be the wall clock time after painting finishes and the compositor translation completes. That's really important for perceived smoothness. It's the whole reason starting animations is async.

But maybe I'm misunderstanding. I'll see if I can look at the patch in the next day or two. Sorry for the delay!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #5)

Sorry, on my phone and away from my computer for the next three days.

No need to apologize, no rush! :)

But basically we shouldn't be setting the animation ready time to the refresh driver time. It should be the wall clock time after painting finishes and the compositor translation completes. That's really important for perceived smoothness. It's the whole reason starting animations is async.

Can you elaborate on why? The current timing isn't quite that, if I'm understanding correctly. The current time, as far as I can tell, is "wall clock time once display list building has finished (and we've sent the compositor transaction)". It has no relationship with when stuff shows up on screen.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1864425 - Don't keep ticking for pending animations that will never get painted. r=hiro,boris,birtles

This is a much lower-risk fix than the other patches on this bug (since
it only affects documents where PresShell::IsNeverPainting is true), and
also unblocks bug 1863620.

So we can try to land this earlier and discuss whether
PendingAnimationTracker and the current animation ready time etc still
make sense in a separate bug.

This is covered by existing tests once bug 1863620 lands.

Comment 8

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

(In reply to Brian Birtles (:birtles) from comment #5)

Sorry, on my phone and away from my computer for the next three days.

No need to apologize, no rush! :)

Thanks. Bugzilla is not very mobile friendly unfortunately.

But basically we shouldn't be setting the animation ready time to the refresh driver time. It should be the wall clock time after painting finishes and the compositor translation completes. That's really important for perceived smoothness. It's the whole reason starting animations is async.

Can you elaborate on why? The current timing isn't quite that, if I'm understanding correctly. The current time, as far as I can tell, is "wall clock time once display list building has finished (and we've sent the compositor transaction)". It has no relationship with when stuff shows up on screen.

Yeah, so the original idea was that layer generation and upload to the compositor was really slow so if we treated the animation as starting before we did that work, the second frame of the animation would end up corresponding to several frames in to the animation (if it had run at its ideal framerate) so animations would appear to jump at the start. We tried setting the animation ready time to the next refresh driver tick but then that made the start of animations feel laggy, especially on low-end Firefox OS devices. So we tried to find a moment roughly corresponding to when to most expensive steps of preparing the animation had been finished but without having to actually synchronize with the compositor. From memory, graphics folks led me to the current setup.

That was all before WebRender however so that timing might not make sense anymore. But even if WebRender gives us zero cost animation setup time on all devices we'd still need to think about long running scripts that trigger animations and would probably want to use some moment that means we don't do the first few frames of the animation in that case.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1864425 - Remove PendingAnimationTracker. r=birtles!,#animation!

Instead of starting transitions and animations as a result of a paint,
use the refresh driver tick to do this.

This sets the transition-ready time to the current time during the next
refresh driver tick that it was started on (see mSawTickWhilePending).

This is similar to what's described in the bugs comments, and seems to
work nicely in practice.

We could easily change that (current time) by a paint-based time if
needed (when available), which would be more similar to what we were
doing. But I'd rather do the simple thing for now, and land this shortly
after the soft freeze is over so that we have time to watch out for
regressions.

There's one regression on a test that birtles wrote (using an XHR doc
and switching the timeline to a rendered doc's timeline).

We use the timeline's document rather than the target document to
determine whether to trigger animations now. That's one of the cases
where we'd keep vsync perma-running without this patch, and Chrome also
fails that test. Maybe the test should be removed / the spec should be
tweaked to allow this behavior?

This causes some progression in some CSS transitions tests too, and I
added an extra test for the vsync behavior.

Over-all this is much simpler to reason about and I think we should try
to do this.

Comment 10

[Brian Birtles (:birtles)]

I think comment 8 explains why we deliberately don't use the refresh driver tick time. It's possible that circumstances have changed (particularly introduction of WebRender) and we could get away with using the next refresh driver tick time but we'd need a way to verify that doing so doesn't re-introduce the original issue of the start of animations appearing to lag. We should also check what Blink and Webkit are currently setting the ready time to since I think at one point Chrome might have aligned to refresh driver ticks.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: startTime test

It seems Chrome matches rAF time here, see this test-case.

Animations look smooth here on all my devices, including my phone, though arguably I don't have a particularly low-end phone. I think it'd be a good idea to land this soon next week if it looks good (right after soft-freeze). I can get some wider testing during the nightly cycle and check lower-end phones.

If needed it should be feasible to change the startime to be again something based on the compositor, but I'd rather avoid that complexity if we can.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Safari also seems to match rAF time.

Comment 13

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Created attachment 9364193 [details]
startTime test

It seems Chrome matches rAF time here, see this test-case.

Looks like Chrome is using the previous rAF time. I thought we were going to use the next frame time?

Animations look smooth here on all my devices, including my phone, though arguably I don't have a particularly low-end phone. I think it'd be a good idea to land this soon next week if it looks good (right after soft-freeze). I can get some wider testing during the nightly cycle and check lower-end phones.

That sounds great assuming we're using the next frame's time.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Yes we're using the next frame time (mSawTickWhilePending is what ensures that one tick happens).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fd3bd56943d
Remove PendingAnimationTracker. r=birtles

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43254 for changes under testing/web-platform/tests

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f80a88d5dd04
Fix browser_html_list_view to wait properly.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2e9e347cf60
Fix lint failure. r=fix

Comment 19

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7fd3bd56943d
https://hg.mozilla.org/mozilla-central/rev/f80a88d5dd04
https://hg.mozilla.org/mozilla-central/rev/e2e9e347cf60

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Daniel Holbert [:dholbert]]

[ni=emilio as a reminder to request backout from beta next week, per bug 1867047 comment 3 - 4. Probably we can keep this in on trunk while we continue to investigate the regressions.]

Comment 22

[Emilio Cobos Álvarez (:emilio)]

sheriffs: Can we get this bug and bug 1863620 backed out of beta?

Comment 23

[Donal Meehan [:dmeehan]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)

sheriffs: Can we get this bug and bug 1863620 backed out of beta?

:emilio could you attach a patch that I can uplift to beta? To back everything out cleanly?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Back out of this and dependent bugs.

See commit message for details.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

The only conflicts I got were in .ini files.

Comment 26

[Donal Meehan [:dmeehan]]

Backed out of beta
https://hg.mozilla.org/releases/mozilla-beta/rev/48d2052b377a35b0f40e52b8efad5e0ea10e0ebe

Comment 27

[Daniel Holbert [:dholbert]]

Probably worth doing a backout on beta again, when 123 merges to beta in a few days, given that we don't have a fix for the regressions yet (bug 1865955, bug 1867047) and we don't want those to hit release users?

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Yes, Pascal just pinged me about it.

Comment 29

[Pascal Chevrel:pascalc]

Backed out of beta 123
https://hg.mozilla.org/releases/mozilla-beta/rev/1849dfd4ad15cc21f294ed2bb5e95364a5ba7140