Bug 77668 – [chromium] Ensure that the cc thread animation framework continues to tick when the tab is backgrounded

• Component: WebCore Misc.
• Status: RESOLVED FIXED

Currently, the framework is driven by CCLayerTreeHostImpl::animate, which does not get called when the tab is backgrounded. Will need to find a way to periodically tick the animation so that we can inform js when animations are started, possibly using a CCTimer.

[James Robinson]

This feels pretty similar to the issue of RAF-driven animations on the main thread (RAF isn't pumped in background tabs, but animation events still should fire).  What was the resolution of that, Joel?

[Joel Webber]

(In reply to comment #1)
> This feels pretty similar to the issue of RAF-driven animations on the main thread (RAF isn't pumped in background tabs, but animation events still should fire).  What was the resolution of that, Joel?

The current state of affairs is pretty hacky, but I believe necessarily so (at least for the time being). AnimationControllerPrivate::animationFrameCallbackFired() (ultimately called from FrameView::serviceScriptedAnimations()) just makes sure the page's animations get updated at the right time.

AnimationController's timer is still firing happily in the background, which is the only simple way to make sure all the animation events get fired. A more aggressive fix would change the timer to fire only when the next *event* is needed, rather than the next 40fps animation tick. I thought it best to get things working first, though, as this starts to get fairly messy, especially with the #if ENABLE(REQUEST_ANIMATION_FRAME) stuff.

Even the less-frequently-firing timer is still "wrong" in that events aren't guaranteed to fire precisely when you'd expect them -- they're practically guaranteed to fire slightly before or after the animation has been rendered in the expected state. I haven't seen this problem in the wild, but I'm pretty sure you could construct an example that used animation events for sequencing, that would have slight hitching artifacts.

I believe the best general approach would be to drive the events solely from animation-frame callbacks, and use the timer only as a fallback for background tabs.

Created attachment 130695 [details]
Patch

[James Robinson]

Comment on attachment 130695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130695&action=review

This doesn't seem too outrageous but the fact that we're adding a ticking mechanism to CCLayerTreeHostImpl when it hasn't needed one before gives me slight pause.  Should we perhaps be telling the scheduler to tick at a low rate in background pages while animations are active?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:44
> +const double lowFrequencyAnimationIntervalMs = 300;

can we make this seconds? one less thing to worry about for the ms->sec conversion

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:61
> +        m_layerTreeHostImpl->animate(WTF::monotonicallyIncreasingTime());

don't need the WTF:: here - <wtf/CurrentTime.h> pulls this into the global namespace with a using statement and there's no ambiguity here

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:94
> +    m_lowFrequencyAnimationTimeSource = CCDelayBasedTimeSource::create(lowFrequencyAnimationIntervalMs, CCProxy::hasImplThread() ? CCProxy::implThread() : CCProxy::mainThread());

hm, i wonder if we need a way to say "my current ccproxy" from CCLayerTreeHostImpl. the fact that we haven't had this in the past makes me wonder about this a bit...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:105
> +    m_lowFrequencyAnimationTimeSource->setActive(false);

is there any way that destroying the client (the time source adapter here) can deactivate the timer instead of  having to clean up ourselves?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:424
> +    m_lowFrequencyAnimationTimeSource->setActive(!visible);

i think we need to figure out a way to have this only tick when there are actually animations active that might benefit from the tick (which is the minority case). in all other cases, background tabs should be silent

[Nat Duca]

Great first pass Ian.

I do think we should get the DBTS timer moved into the proxy, eventually. But that makes the animate() call *not* pat of the scheduledActionDrawAndCommit, opting instead for a scheduledActionAnimate. But, this requires a whole new setNeedsAnimate flow, which might be a bit harder to get right. I'd really like to get to a good-enough state first, then do a followon patch to move animation scheduling into the proxy.

I do think we need a computation of *whether* to tick in the background. Thats a key building block.

James would you be comfortable with using a timer for the next week or two while we get things correct, then we go back and move things into the proxy as a follow-on? Ian, if james agrees, you need to make sure to set expectations with @backer that you're signed up for stuff even after the switch is thrown.

Created attachment 130826 [details]
Patch

Created attachment 130847 [details]
Patch

Noteworthy changes
 - Added logic in CCLayerTreeHostImpl to turn on the background ticking only when animations can benefit
 - Added a unit test that ensures that animations are ticked in the background.


(In reply to comment #4)
> (From update of attachment 130695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130695&action=review
> 
> This doesn't seem too outrageous but the fact that we're adding a ticking mechanism to CCLayerTreeHostImpl when it hasn't needed one before gives me slight pause.  Should we perhaps be telling the scheduler to tick at a low rate in background pages while animations are active?

Yes, that sounds better. As Nat was suggesting earlier, could this be an acceptable first step, and I create a bug for moving the background ticking into the proxy? I would certainly be able to see that bug through.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:44
> > +const double lowFrequencyAnimationIntervalMs = 300;
> 
> can we make this seconds? one less thing to worry about for the ms->sec conversion
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:61
> > +        m_layerTreeHostImpl->animate(WTF::monotonicallyIncreasingTime());
> 
> don't need the WTF:: here - <wtf/CurrentTime.h> pulls this into the global namespace with a using statement and there's no ambiguity here
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:94
> > +    m_lowFrequencyAnimationTimeSource = CCDelayBasedTimeSource::create(lowFrequencyAnimationIntervalMs, CCProxy::hasImplThread() ? CCProxy::implThread() : CCProxy::mainThread());
> 
> hm, i wonder if we need a way to say "my current ccproxy" from CCLayerTreeHostImpl. the fact that we haven't had this in the past makes me wonder about this a bit...
Added CCProxy::currentThread()
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:105
> > +    m_lowFrequencyAnimationTimeSource->setActive(false);
> 
> is there any way that destroying the client (the time source adapter here) can deactivate the timer instead of  having to clean up ourselves?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:424
> > +    m_lowFrequencyAnimationTimeSource->setActive(!visible);
> 
> i think we need to figure out a way to have this only tick when there are actually animations active that might benefit from the tick (which is the minority case). in all other cases, background tabs should be silent
I now only start the timer if there are active animations.

> I do think we need a computation of *whether* to tick in the background. Thats a key building block.
Agreed. I've added some logic for this in CCLayerTreeHostImpl

[James Robinson]

Comment on attachment 130847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130847&action=review

R=me w/ nits

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:44
> +const double lowFrequencyAnimationInterval = 0.3;

we tick setTimeout()s at 1Hz for background tabs, can we use the same value for this?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:50
> +class CCLayerTreeHostImplTimeSourceAdapter : public CCTimeSourceClient {
> +public:

add WTF_MAKE_NONCOPYABLE as well to hide copy/assignment c'tors

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:73
> +    explicit CCLayerTreeHostImplTimeSourceAdapter(CCLayerTreeHostImpl* layerTreeHostImpl, PassRefPtr<CCDelayBasedTimeSource> timeSource)

now you don't need explicit (since it's a 2-arg c'tor). Isn't C++ fun?

Created attachment 130867 [details]
Patch

(In reply to comment #10)
> (From update of attachment 130847 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130847&action=review
> 
> R=me w/ nits
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:44
> > +const double lowFrequencyAnimationInterval = 0.3;
> 
> we tick setTimeout()s at 1Hz for background tabs, can we use the same value for this?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:50
> > +class CCLayerTreeHostImplTimeSourceAdapter : public CCTimeSourceClient {
> > +public:
> 
> add WTF_MAKE_NONCOPYABLE as well to hide copy/assignment c'tors
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:73
> > +    explicit CCLayerTreeHostImplTimeSourceAdapter(CCLayerTreeHostImpl* layerTreeHostImpl, PassRefPtr<CCDelayBasedTimeSource> timeSource)
> 
> now you don't need explicit (since it's a 2-arg c'tor). Isn't C++ fun?
Whoops! Done.

Created attachment 130875 [details]
Patch

Created attachment 130880 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130880 [details]
Patch

Clearing flags on attachment: 130880

Committed r110252: <http://trac.webkit.org/changeset/110252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.