Drive SVG Animations via requestAnimationFrame

Drive SVG Animations via requestAnimationFrame

Start using FrameView::scheduleAnimation instead of a Timer for driving
the animations in a SMILTimeContainer.
The timer however remains to use when the next animation event is further
into the future than within the (~) next frame interval. Hence
SMILTimeContainer::m_timer is renamed to m_wakeupTimer.

The animation time from the compositor is plumbed through the layers, but
is not yet used as the "global" clock. That will have to wait for a later
CL.

For the SVG-in-<img> (and similar) case(s),
SVGImageChromeClient::scheduleAnimation is modified to use a fixed frame
interval.
In SVGImage, the initialization of m_page is moved later, to disable any
methods that depend on accessing the SVG root element (prompted by the
need to query if the image has any animations) until the actual document
is loaded/being loaded.

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

[fs, 2014-02-28 16:58:23]

This tries to hook up SVG animations to the rAF "framework".

There's a non-pleasant quirk wrt to testing (pixel tests) and
what the current time is when the first frame is observed.
The issue is that when the timeline is started (on load),
time 0 is defined on the timeline, and animations are updated
and a new frame schedule (if needed). The compositor is then
given control and schedule/begin a new frame - which runs
the frame callbacks, and hence updates values again. This
has the result that the frame that ends up being grabbed is
(usually) at a time ever so slightly above 0 - but enough
yield a difference large enough to be observable by the image
diff.

Because of this patch set 2 contains an ugly hack to freeze
the clock between timeline begin and the first frame callback.

If you have any ideas about how to handle that in a better way, then please let
me know.

[pdr., 2014-03-03 17:47:32]

I really like this. I would prefer if one of the web animations folks would take
a look as well (dstockwell?)

Some minor comments below. We're still failing a couple tests
(animation-currentColor.svg, reinserting-svg-into-document.html).

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:40: static const double
minimumDelay = 0.04;
Can you use DocumentTimeline::s_minimumDelay directly instead of copying it?

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:50: , m_clockFrozen(false)
I'm wary of adding more clock-related state to this already complex state
machine. Unfortunately I tried to combine m_clockFrozen into another variable
and I don't see an easy way to do it.

Can you think of a way to simplify this? I'm happy if we need it but I'd like to
avoid more state if possible.

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:192: if (m_beginTime)
How did this if statement get here? :P

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:222: bool
SMILTimeContainer::isTimelineRunning() const
Should this have m_clockFrozen too?

[dstockwell, 2014-03-04 01:19:03]

On 2014/02/28 16:58:23, fs wrote:
> There's a non-pleasant quirk wrt to testing (pixel tests) and
> what the current time is when the first frame is observed.
> The issue is that when the timeline is started (on load),
> time 0 is defined on the timeline, and animations are updated
> and a new frame schedule (if needed). The compositor is then
> given control and schedule/begin a new frame - which runs
> the frame callbacks, and hence updates values again. This
> has the result that the frame that ends up being grabbed is
> (usually) at a time ever so slightly above 0 - but enough
> yield a difference large enough to be observable by the image
> diff.

Can you link to a test that shows this problem?


Is time 0 required to be defined at load? Or could it be delayed until the first
frame?

https://codereview.chromium.org/179293004/diff/40001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/179293004/diff/40001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:34:
SVGDocumentExtensions::serviceOnAnimationFrame(*frame->document(),
monotonicAnimationStartTime);
We updated the naming in DocumentAnimations recently as for those outside the
animations systems it's not clear at all what 'service' is doing.

Dispatching of CSS Animation events then moved to the regular scripted animation
controller (see https://codereview.chromium.org/177203009/)

Are SVG Animation events ever fired synchronously?

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:40: static const double
minimumDelay = 0.04;
I'm OK with this duplication for now, but this logic (schedule-frame-after-time)
should likely be pushed up to PageAnimator at some point.

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:305: void
SMILTimeContainer::serviceAnimations(double monotonicAnimationStartTime)
This patch just moves updates to be driven by the frame callback, but does not
use the frame times yet? Is that planned?

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/graphics...
File Source/core/svg/graphics/SVGImageChromeClient.cpp (right):

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/graphics...
Source/core/svg/graphics/SVGImageChromeClient.cpp:38: static const double
animationFrameDelay = 0.025;
Is this 40hz, why?

[fs, 2014-03-04 10:28:19]

Thanks for having a look!

On 2014/03/03 17:47:32, pdr wrote:
> Some minor comments below. We're still failing a couple tests
> (animation-currentColor.svg, reinserting-svg-into-document.html).

Haven't seen these fail myself, but I'll put them to scrutiny.

On 2014/03/04 01:19:03, dstockwell wrote:
> On 2014/02/28 16:58:23, fs wrote:
> > There's a non-pleasant quirk wrt to testing (pixel tests) and
> > what the current time is when the first frame is observed.
> > The issue is that when the timeline is started (on load),
> > time 0 is defined on the timeline, and animations are updated
> > and a new frame schedule (if needed). The compositor is then
> > given control and schedule/begin a new frame - which runs
> > the frame callbacks, and hence updates values again. This
> > has the result that the frame that ends up being grabbed is
> > (usually) at a time ever so slightly above 0 - but enough
> > yield a difference large enough to be observable by the image
> > diff.
> 
> Can you link to a test that shows this problem?

svg/W3C-SVG-1.1/animate-elem-10-t.svg would be one such example - I expect that
all SVGs with animations starting at 0s to be affected to some degree though.
It'll naturally boil down to the exact time between the two "events" and to what
degree you're able to exact a (visually, if you've pixel-perfect eyes) noticable
change in that time.
 
> Is time 0 required to be defined at load? Or could it be delayed until the
first
> frame?

Yes, at least ATM. Some relevant spec-text:

"SMIL Animation requires that the host language define the meaning for document
begin .... Since an ‘svg’ is sometimes the root of the XML document tree and
other times can be a component of a parent XML grammar, the document begin for a
given SVG document fragment is defined to be the exact time at which the ‘svg’
element's load event is triggered."

(https://svgwg.org/svg2-draft/animate.html#RelationshipToSMILAnimation fifth
paragraph)

This could of course be subject to change with additional harmonization with Web
Animations.

https://codereview.chromium.org/179293004/diff/40001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/179293004/diff/40001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:34:
SVGDocumentExtensions::serviceOnAnimationFrame(*frame->document(),
monotonicAnimationStartTime);
On 2014/03/04 01:19:04, dstockwell wrote:
> We updated the naming in DocumentAnimations recently as for those outside the
> animations systems it's not clear at all what 'service' is doing.

I noticed this, and for a while considered following suite. I seemed better to
keep the old naming until the new one made sense though (i.e. until we have both
components as CSS Animations do etc.) Hence, yet another thing I hope to get
aligned soon(TM).

> Dispatching of CSS Animation events then moved to the regular scripted
animation
> controller (see https://codereview.chromium.org/177203009/)

Haven't looked into that much. YATTA ("Yet Another Thing To Align") I suppose
then.

> Are SVG Animation events ever fired synchronously?

AFAICR, no.

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:40: static const double
minimumDelay = 0.04;
On 2014/03/04 01:19:04, dstockwell wrote:
> I'm OK with this duplication for now, but this logic
(schedule-frame-after-time)
> should likely be pushed up to PageAnimator at some point.

I suppose we can trade this two line hack for a one-line hack
(friend-declaration in DocumentTimeline). I do hope we'll be able to consolidate
and converge reasonably quickly here though. Unifying this sounds reasonable.

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:50: , m_clockFrozen(false)
On 2014/03/03 17:47:33, pdr wrote:
> I'm wary of adding more clock-related state to this already complex state
> machine. Unfortunately I tried to combine m_clockFrozen into another variable
> and I don't see an easy way to do it.
> 
> Can you think of a way to simplify this? I'm happy if we need it but I'd like
to
> avoid more state if possible.

I will try to formalize this somewhat - probably by migrating to AnimationClock,
and either extend it or inherit from it (preferably the former). I'd really like
to come up with a long-term solution for this though...

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:192: if (m_beginTime)
On 2014/03/03 17:47:33, pdr wrote:
> How did this if statement get here? :P

Magic? =P

Maybe it should've been some other condition... I'll just drop it for now.

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:222: bool
SMILTimeContainer::isTimelineRunning() const
On 2014/03/03 17:47:33, pdr wrote:
> Should this have m_clockFrozen too?

I don't think so. The clock being frozen does not imply that the timeline isn't
running.

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:305: void
SMILTimeContainer::serviceAnimations(double monotonicAnimationStartTime)
On 2014/03/04 01:19:04, dstockwell wrote:
> This patch just moves updates to be driven by the frame callback, but does not
> use the frame times yet? Is that planned?

Yes, I plan to that in follow up CLs. Had originally thought that I might be
able to make use of it here directly (in this CL), but figured it might just be
"too much" at the same time. If it ends up being unused I'll de-plumb it and
move it to a/the later CL.

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/graphics...
File Source/core/svg/graphics/SVGImageChromeClient.cpp (right):

https://codereview.chromium.org/179293004/diff/40001/Source/core/svg/graphics...
Source/core/svg/graphics/SVGImageChromeClient.cpp:38: static const double
animationFrameDelay = 0.025;
On 2014/03/04 01:19:04, dstockwell wrote:
> Is this 40hz, why?

I used the old value from the SVG animations code here. (To try and avoid
changing too much in one go.) I don't feel strongly about the exact value though
(and would of course rather sync this with some clock, but that's not how
animated Images work in general, and we don't really have anything to key off in
this context.)

[fs, 2014-03-04 16:22:01]

Updated to use an AnimationClock, and use its 'frozen' state instead of making
loose such in SMILTimeContainer.

animation-currentColor.svg is still failing, but it looks like it's a
pre-existing issue, which is just made visible by this change. I'm trying to
come up with (another) TC that illustrates the problem, and will probably try to
land a fix for that separately.

[fs, 2014-03-04 18:16:45]

On 2014/03/04 16:22:01, fs wrote:
> animation-currentColor.svg is still failing, but it looks like it's a
> pre-existing issue, which is just made visible by this change. I'm trying to
> come up with (another) TC that illustrates the problem, and will probably try
to
> land a fix for that separately.

...and it looks like that was my doing - Doh!

Fix here: https://codereview.chromium.org/183973023/ - please review and commit
if it looks ok.

[pdr., 2014-03-04 23:36:48]

LGTM for core/svg.

If you don't mind, please wait for an LGTM from dstockwell.

[fs, 2014-03-06 09:37:19]

On 2014/03/04 23:36:48, pdr wrote:
> LGTM for core/svg.
> 
> If you don't mind, please wait for an LGTM from dstockwell.

@dstockwell: Ping!

[dstockwell, 2014-03-06 11:03:23]

lgtm

[fs, 2014-03-06 12:09:04]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-03-06 12:09:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/179293004/60001

[commit-bot: I haz the power, 2014-03-06 12:33:06]

Change committed as 168631

[cbiesinger, 2014-03-06 20:40:53]

On 2014/03/06 12:33:06, I haz the power (commit-bot) wrote:
> Change committed as 168631

I've reverted this in https://codereview.chromium.org/188903003/ because I
suspect it caused various layout test failures, see the link in that commit
description.