Defer starting of animations until after compositing update

Defer starting of animations until after compositing update

Starting of animations depends on compositing state so that a decision
as to whether to run the animation on the main or compositor threads
can be made. Previously this happened following a style recalc where
sufficient incremental updating allowed a decision. After this patch
starting of animations may be deferred until after a compositing
update. Ideally this would happen once before the frame completes, but
the lifecycle of a frame is not yet visible to Blink.

Following the starting of animations the animation clock is unfrozen
allowing getComputedStyle and other animation API calls to advance
time when outside a frame. Previously, to maintain synchronization
in these situations the clock automatically froze on access and then
unfroze after a subsequent style recalc. This was partially inaccurate
as calls via the animation API might never trigger a recalc. Deferring the
unfreezing until after the compositing update is less feasible. Instead,
this patch allows the clock to remain unfrozen outside a frame by
throttling the frequency at which the clock is permitted to advance.

BUG=339847
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167740

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167865

[abarth-chromium, 2014-02-20 17:51:59]

Can we remove any DisableCompositingQueryAsserts as a result of this CL?

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/A...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/A...
Source/core/animation/AnimationClock.h:37: namespace {
Please don't use anonymous namespaces in headers.  That makes the linker go
crazy.

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.cpp:72:
ASSERT(document.lifecycle().state() == DocumentLifecycle::StyleClean ||
document.lifecycle().state() == DocumentLifecycle::StyleRecalcPending ||
document.lifecycle().state() == DocumentLifecycle::LayoutClean ||
document.lifecycle().state() == DocumentLifecycle::CompositingClean);
What are you trying to assert here?  As written, this line of code doesn't make
sense.  If you write out what you're trying to assert in an english sentence, we
can figure out how to translate that into code.

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.h (right):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.h:44: static void
serviceOnFrameStart(Document&, double monotonicAnimationStartTime);
Can we rename this function to indicate what it does?  It's not possible to tell
from the name what this function does.

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.h:47: class ServiceOnFrameEndScope {
This is a wacky pattern for calling a function.  Why not just call the function?

[dstockwell, 2014-02-20 18:22:01]

On 2014/02/20 17:51:59, abarth wrote:
> Can we remove any DisableCompositingQueryAsserts as a result of this CL?

Yes, this change removes one from CSSPendingAnimations.cpp

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.cpp:72:
ASSERT(document.lifecycle().state() == DocumentLifecycle::StyleClean ||
document.lifecycle().state() == DocumentLifecycle::StyleRecalcPending ||
document.lifecycle().state() == DocumentLifecycle::LayoutClean ||
document.lifecycle().state() == DocumentLifecycle::CompositingClean);
On 2014/02/20 17:52:00, abarth wrote:
> What are you trying to assert here?  As written, this line of code doesn't
make
> sense.  If you write out what you're trying to assert in an english sentence,
we
> can figure out how to translate that into code.

I was looking at what we know about the beginning of a frame. When cc calls
animate we can be in any of these states.

Between the calls to animate and layout the DocumentLifecycle can traverse the
style/layout related states several times. We don't have a good way of knowing
where we are in the process of making a single frame.

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.h (right):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.h:47: class ServiceOnFrameEndScope {
On 2014/02/20 17:52:00, abarth wrote:
> This is a wacky pattern for calling a function.  Why not just call the
function?

Isn't this the same pattern as DocumentLifecycle::Scope ?

RenderLayerCompositor::updateCompositingLayers has multiple points of return,
this needs to be called at the end.

[abarth-chromium, 2014-02-20 19:32:31]

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/c...
File Source/core/animation/css/CSSPendingAnimations.cpp (left):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/c...
Source/core/animation/css/CSSPendingAnimations.cpp:55:
DisableCompositingQueryAsserts disabler;
Ah, I missed this.  Great!

[abarth-chromium, 2014-02-20 19:40:09]

On 2014/02/20 18:22:01, dstockwell wrote:
> I was looking at what we know about the beginning of a frame. When cc calls
> animate we can be in any of these states.

We can actually go through any of these states because animate callbacks involve
running script.

> Between the calls to animate and layout the DocumentLifecycle can traverse the
> style/layout related states several times. We don't have a good way of knowing
> where we are in the process of making a single frame.

Hum....  In that case, I don't think it makes sense to assert anything about the
document lifecycle at this point.
 
>
https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
> File Source/core/animation/DocumentAnimations.h (right):
> 
>
https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
> Source/core/animation/DocumentAnimations.h:47: class ServiceOnFrameEndScope {
> On 2014/02/20 17:52:00, abarth wrote:
> > This is a wacky pattern for calling a function.  Why not just call the
> function?
> 
> Isn't this the same pattern as DocumentLifecycle::Scope ?

Yes, but that just assigns a field.  It's like TemporaryChange.

> RenderLayerCompositor::updateCompositingLayers has multiple points of return,
> this needs to be called at the end.

Maybe we should restructure the code so that an outer function calls an inner
function and all the "at the end" code can run in the outer function.  That's
more readable than a bunch of stack objects that call functions in their
destructor.

[dstockwell, 2014-02-24 06:03:24]

On 2014/02/20 19:40:09, abarth wrote:
> On 2014/02/20 18:22:01, dstockwell wrote:
> > I was looking at what we know about the beginning of a frame. When cc calls
> > animate we can be in any of these states.
> 
> We can actually go through any of these states because animate callbacks
involve
> running script.
> 
> > Between the calls to animate and layout the DocumentLifecycle can traverse
the
> > style/layout related states several times. We don't have a good way of
knowing
> > where we are in the process of making a single frame.
> 
> Hum....  In that case, I don't think it makes sense to assert anything about
the
> document lifecycle at this point.

Removed.
  
> >
>
https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
> > File Source/core/animation/DocumentAnimations.h (right):
> > 
> >
>
https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
> > Source/core/animation/DocumentAnimations.h:47: class ServiceOnFrameEndScope
{
> > On 2014/02/20 17:52:00, abarth wrote:
> > > This is a wacky pattern for calling a function.  Why not just call the
> > function?
> > 
> > Isn't this the same pattern as DocumentLifecycle::Scope ?
> 
> Yes, but that just assigns a field.  It's like TemporaryChange.
> 
> > RenderLayerCompositor::updateCompositingLayers has multiple points of
return,
> > this needs to be called at the end.
> 
> Maybe we should restructure the code so that an outer function calls an inner
> function and all the "at the end" code can run in the outer function.  That's
> more readable than a bunch of stack objects that call functions in their
> destructor.

Done.

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/A...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/A...
Source/core/animation/AnimationClock.h:37: namespace {
On 2014/02/20 17:52:00, abarth wrote:
> Please don't use anonymous namespaces in headers.  That makes the linker go
> crazy.

Done.

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.h (right):

https://codereview.chromium.org/135693003/diff/280001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.h:44: static void
serviceOnFrameStart(Document&, double monotonicAnimationStartTime);
On 2014/02/20 17:52:00, abarth wrote:
> Can we rename this function to indicate what it does?  It's not possible to
tell
> from the name what this function does.

Done. I split and renamed the functions of DocumentAnimations.

[abarth-chromium, 2014-02-24 06:14:07]

LGTM with the exception of the constant.  I'm not sure where that magic number
comes from or what the consequences of its value are.

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/A...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/A...
Source/core/animation/AnimationClock.h:44: const double
minTimeBeforeUnsynchronizedAnimationClockTick = 0.005;
Where does this constant come from?  Would this constant be different if we were
running at 120 Hz or 30 Hz?

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/A...
Source/core/animation/AnimationClock.h:60: double currentTime()
Should we move these functions out of line now that they're non-trivial?

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.h (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.h:48: static void
startPendingAnimations(Document&);
Thanks!  These are much clearer names.

https://codereview.chromium.org/135693003/diff/560001/Source/core/css/CSSComp...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/css/CSSComp...
Source/core/css/CSSComputedStyleDeclaration.cpp:1537: // an up to date value.
This comment seems slightly out of date now.

https://codereview.chromium.org/135693003/diff/560001/Source/core/page/PageAn...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:29:
DocumentAnimations::dispatchAnimationEvents(*frame->document());
Given that this function is dispatching events, don't we need to collect up the
RefPtr<Frame>s in a vector before walking them?  Scripts running below this call
stack can alter the frame tree, which can screw up the traversal.  (Feel free to
address this issue in a followup CL.)

https://codereview.chromium.org/135693003/diff/560001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:34: documents.append(frame->document());
Notice that we collect up the documents here before calling
requestAnimationFrame callbacks for that very reason.

https://codereview.chromium.org/135693003/diff/560001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerCompositor.cpp (left):

https://codereview.chromium.org/135693003/diff/560001/Source/core/rendering/R...
Source/core/rendering/RenderLayerCompositor.cpp:404: DocumentLifecycle::Scope
lifecycleScope(lifecycle(), DocumentLifecycle::CompositingClean);
Thanks for fixing this one as well.

[dstockwell, 2014-02-24 06:58:06]

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/A...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/A...
Source/core/animation/AnimationClock.h:44: const double
minTimeBeforeUnsynchronizedAnimationClockTick = 0.005;
On 2014/02/24 06:14:08, abarth wrote:
> Where does this constant come from?  Would this constant be different if we
were
> running at 120 Hz or 30 Hz?

I think whatever we choose here is going to be fairly arbitrary. If we knew the
rate as always 120 Hz or 30 Hz I would have chosen a different value. I used 200
Hz as it's greater than the rate for most users and 5ms is still a significant
amount of time.

The cases where this value has an effect are 1. getComputedStyle for compositor
animations and 2. the start times of animations created under the (experimental)
Web Animations API.

For #1, this seems an improvement over the current situation where
getComputedStyle would always advance a compositor animation to the current
time. Now we'll only advance once 5ms have elapsed since the last frame time
reported to the main thread. (we could improve here by forwarding frame times
even when not animating on the main thread, but I don't actually think this is a
very important use case).

For #2, it means two players created 5ms apart (outside a RAF callback) will not
have the same start times. The exact mechanism here is still pending some
discussion in the spec group, eg. the clock might remain frozen until the script
callback finishes.

https://codereview.chromium.org/135693003/diff/560001/Source/core/page/PageAn...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:29:
DocumentAnimations::dispatchAnimationEvents(*frame->document());
On 2014/02/24 06:14:08, abarth wrote:
> Given that this function is dispatching events, don't we need to collect up
the
> RefPtr<Frame>s in a vector before walking them?  Scripts running below this
call
> stack can alter the frame tree, which can screw up the traversal.  (Feel free
to
> address this issue in a followup CL.)

Will do. Looks like we can use the same collection as below.

[dstockwell, 2014-02-24 12:41:16]

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/A...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/animation/A...
Source/core/animation/AnimationClock.h:60: double currentTime()
On 2014/02/24 06:14:08, abarth wrote:
> Should we move these functions out of line now that they're non-trivial?

Done.

https://codereview.chromium.org/135693003/diff/560001/Source/core/css/CSSComp...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/135693003/diff/560001/Source/core/css/CSSComp...
Source/core/css/CSSComputedStyleDeclaration.cpp:1537: // an up to date value.
On 2014/02/24 06:14:08, abarth wrote:
> This comment seems slightly out of date now.

Done.

[abarth-chromium, 2014-02-24 17:48:22]

lgtm

[commit-bot: I haz the power, 2014-02-24 17:48:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/135693003/650001

[shans, 2014-02-24 22:48:55]

animation changes lgtm

[esprehn, 2014-02-24 23:14:34]

https://codereview.chromium.org/135693003/diff/650001/Source/core/page/PageAn...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:29:
DocumentAnimations::dispatchAnimationEvents(*frame->document());
This should have a FIXME that we should move the animation events logic into
ScriptedAnimationController.

Now that this is split we can trivially do that too, just move the call to
DocumentAnimations::dispatchAnimationEvents into
ScriptedAnimationController::serviceScriptedAnimations.

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
Source/core/rendering/RenderLayerCompositor.cpp:410:
DocumentAnimations::dispatchAnimationEventsAsync(m_renderView->document());
You should ASSERT() at the end that the lifecycle() is still at
CompositingClean, DocumentAnimations in here shouldn't be allowed to make the
state move backwards.

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerCompositor.h (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
Source/core/rendering/RenderLayerCompositor.h:259: void
updateCompositingLayersInternal();
This should be private.

[esprehn, 2014-02-25 00:00:47]

https://codereview.chromium.org/135693003/diff/650001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.cpp:93: if
(document.cssPendingAnimations().startPendingAnimations() && document.view())
You should ASSERT(document().view()) here, it can't be null, the place where you
call this is inside RLC which means the FrameView must exist since the
RenderView still exists.

[dstockwell, 2014-02-25 00:52:02]

https://codereview.chromium.org/135693003/diff/650001/Source/core/animation/D...
File Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/animation/D...
Source/core/animation/DocumentAnimations.cpp:93: if
(document.cssPendingAnimations().startPendingAnimations() && document.view())
On 2014/02/25 00:00:48, esprehn wrote:
> You should ASSERT(document().view()) here, it can't be null, the place where
you
> call this is inside RLC which means the FrameView must exist since the
> RenderView still exists.

Done.

https://codereview.chromium.org/135693003/diff/650001/Source/core/page/PageAn...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:29:
DocumentAnimations::dispatchAnimationEvents(*frame->document());
On 2014/02/24 23:14:35, esprehn wrote:
> This should have a FIXME that we should move the animation events logic into
> ScriptedAnimationController.
> 
> Now that this is split we can trivially do that too, just move the call to
> DocumentAnimations::dispatchAnimationEvents into
> ScriptedAnimationController::serviceScriptedAnimations.

Done.

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
Source/core/rendering/RenderLayerCompositor.cpp:410:
DocumentAnimations::dispatchAnimationEventsAsync(m_renderView->document());
On 2014/02/24 23:14:35, esprehn wrote:
> You should ASSERT() at the end that the lifecycle() is still at
> CompositingClean, DocumentAnimations in here shouldn't be allowed to make the
> state move backwards.

Done.

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerCompositor.h (right):

https://codereview.chromium.org/135693003/diff/650001/Source/core/rendering/R...
Source/core/rendering/RenderLayerCompositor.h:259: void
updateCompositingLayersInternal();
On 2014/02/24 23:14:35, esprehn wrote:
> This should be private.

It is.

[dstockwell, 2014-02-25 00:52:32]

The CQ bit was checked by dstockwell@chromium.org

[esprehn, 2014-02-25 00:53:14]

lgtm, thanks!

[commit-bot: I haz the power, 2014-02-25 01:01:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/135693003/820002

[commit-bot: I haz the power, 2014-02-25 01:01:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-25 01:02:00]

Failed to apply patch for Source/core/rendering/RenderLayerCompositor.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/rendering/RenderLayerCompositor.cpp
  |diff --git a/Source/core/rendering/RenderLayerCompositor.cpp
b/Source/core/rendering/RenderLayerCompositor.cpp
  |index
4ea971ba77e2ad55d00844322edf4f4cfe838745..0aca45d446fd5dde32bbf63710f13f9740fe92bc
100644
  |--- a/Source/core/rendering/RenderLayerCompositor.cpp
  |+++ b/Source/core/rendering/RenderLayerCompositor.cpp
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/core/rendering/RenderLayerCompositor.cpp
Index: Source/core/rendering/RenderLayerCompositor.cpp
diff --git a/Source/core/rendering/RenderLayerCompositor.cpp
b/Source/core/rendering/RenderLayerCompositor.cpp
index
4ea971ba77e2ad55d00844322edf4f4cfe838745..0aca45d446fd5dde32bbf63710f13f9740fe92bc
100644
--- a/Source/core/rendering/RenderLayerCompositor.cpp
+++ b/Source/core/rendering/RenderLayerCompositor.cpp
@@ -401,8 +401,18 @@ void RenderLayerCompositor::updateCompositingLayers()
         return;
 
     lifecycle().advanceTo(DocumentLifecycle::InCompositingUpdate);
-    DocumentLifecycle::Scope lifecycleScope(lifecycle(),
DocumentLifecycle::CompositingClean);
 
+    updateCompositingLayersInternal();
+
+    lifecycle().advanceTo(DocumentLifecycle::CompositingClean);
+
+    DocumentAnimations::startPendingAnimations(m_renderView->document());
+    DocumentAnimations::dispatchAnimationEventsAsync(m_renderView->document());
+    ASSERT(document.lifecycle().state() ==
DocumentLifecycle::CompositingClean);
+}
+
+void RenderLayerCompositor::updateCompositingLayersInternal()
+{
     if (isMainFrame() && m_renderView->frameView())
        
finishCompositingUpdateForFrameTree(&m_renderView->frameView()->frame());

[dstockwell, 2014-02-25 01:07:26]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-02-25 01:13:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/135693003/840001

[dstockwell, 2014-02-25 02:01:27]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-02-25 02:01:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/135693003/860001

[commit-bot: I haz the power, 2014-02-25 05:19:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/135693003/860001

[commit-bot: I haz the power, 2014-02-25 07:16:21]

Change committed as 167740

[dstockwell, 2014-02-26 00:12:23]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:13:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/135693003/870001

[Paweł Hajdan Jr., 2014-02-26 05:33:12]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 05:42:04]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/135693003/870001

[commit-bot: I haz the power, 2014-02-26 08:53:59]

Change committed as 167865