Bug 79389 - Hitch (due to style recalc?) when starting CSS3 animation
Summary: Hitch (due to style recalc?) when starting CSS3 animation
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: FacebookBug
Depends on:
Blocks:
 
Reported: 2012-02-23 12:20 PST by Rob Arnold
Modified: 2018-10-13 08:53 PDT (History)
16 users (show)

See Also:


Attachments
Testcase (3.34 KB, text/html)
2012-02-23 12:20 PST, Rob Arnold
no flags Details
Revised testcase which does not modify the animation rule and removes the change in background color, still exhibits hitch (2.80 KB, text/html)
2012-02-27 11:24 PST, Jonathan Kaldor
no flags Details
Test case without overlap (3.03 KB, text/html)
2012-03-15 19:46 PDT, Jonathan Kaldor
no flags Details
Patch (1.29 KB, patch)
2012-03-16 16:35 PDT, Rob Arnold
no flags Details | Formatted Diff | Diff
regression : blinking text animation (423 bytes, text/html)
2012-04-17 13:18 PDT, Loïc Yhuel
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Arnold 2012-02-23 12:20:27 PST
Created attachment 128532 [details]
Testcase

See the attached testcase for an example. In it, we begin animating a div smoothly using JS to modify the transform property but when we switch to using the CSS3 animation, there is a noticeable visual hitch. We've observed this on the latest versions of Safari, Mobile Safari and Chrome (which most clearly shows the problem). The inspector seems to attribute this hitch to a style recalc.

On mobile safari, NUMBER_OF_LAYERS can be set lower (say, 256). You may need to tune this number depending on your machine to observe the problem.
Comment 1 Simon Fraser (smfr) 2012-02-24 17:23:55 PST
Pretty sure that touching animRule will cause full style recalc.
Comment 2 Jonathan Kaldor 2012-02-27 11:24:48 PST
Created attachment 129074 [details]
Revised testcase which does not modify the animation rule and removes the change in background color, still exhibits hitch
Comment 3 Jonathan Kaldor 2012-02-27 11:26:00 PST
Regarding modifying animRule, the hitch occurs whether or not lines 70-73 and 97-101 are commented out; the hitch seems to occur because of line 102, e.g. the setting of the animation property on container.  I just uploaded a newer version of the testcase which does not modify the animation rule and which also removes the change in background color.
Comment 4 Jonathan Kaldor 2012-03-15 19:46:09 PDT
Created attachment 132179 [details]
Test case without overlap

In profiling, we noticed that there was a significant penalty to all the overlap testing.  We still see the hitch without the overlap, though, so this revised test case separates the child layers so the cost of style recalc is more apparent
Comment 5 Rob Arnold 2012-03-16 16:35:57 PDT
Created attachment 132412 [details]
Patch
Comment 6 Rob Arnold 2012-03-16 16:39:22 PDT
Comment on attachment 132412 [details]
Patch

This seems too simple to be true but I did not see any of the existing tests on the Mac port fail. I am undoing a change that was introduced when CSS3 animations were added to WebKit; it's not clear to me why the children also need to have a forced style recalc though I suspect it may be related to moving elements in and out of RenderLayers (animations are a trigger for compositing). Mostly I'm looking for a smoke check here.
Comment 7 Simon Fraser (smfr) 2012-03-16 16:52:18 PDT
Interesting. Do you have a trac link for when that was added?
Comment 8 Igor Trindade Oliveira 2012-03-16 16:56:53 PDT
In the code: 

// SyntheticStyleChange means that we need to go through the entire style change logic even though
// no style property has actually changed. It is used to restructure the tree when, for instance,
// RenderLayers are created or destroyed due to animation changes.

So would be interesting test sites using complex animations.
Comment 9 Rob Arnold 2012-03-16 17:14:49 PDT
(In reply to comment #7)
> Interesting. Do you have a trac link for when that was added?

I misread the patch - it was https://bugs.webkit.org/show_bug.cgi?id=19938 which added the webkitAnimation* event and AnimationStyleChange (later renamed for WebGL).

(In reply to comment #8)
> In the code: 
> 
> // SyntheticStyleChange means that we need to go through the entire style change logic even though
> // no style property has actually changed. It is used to restructure the tree when, for instance,
> // RenderLayers are created or destroyed due to animation changes.
> 
> So would be interesting test sites using complex animations.

I was hoping that this would be covered by existing tests but I can start looking for some. Our internal prototypes appear to be working at least though I don't think they are cover enough of the edge cases.
Comment 10 Rob Arnold 2012-03-23 16:48:03 PDT
I tried a couple of sites that use animations (some css3 filters, morphing power cubes) and wrote some test cases that (I hope) targeted the now non-recursive style recalculation (rediscovering bug 46041 in the process) but I was unable to find a test case where the behavior different from Chrome 17. Simon, do you have any ideas for test cases that might spot a bug? I noticed that you had some comments about this line of code in bug 19938 comment 5.
Comment 11 Igor Trindade Oliveira 2012-03-26 15:24:21 PDT
Comment on attachment 132412 [details]
Patch

Lets put in the commit queue and hope for the best.
Comment 12 WebKit Review Bot 2012-03-26 15:33:04 PDT
Comment on attachment 132412 [details]
Patch

Clearing flags on attachment: 132412

Committed r112155: <http://trac.webkit.org/changeset/112155>
Comment 13 WebKit Review Bot 2012-03-26 15:33:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Loïc Yhuel 2012-04-17 13:18:42 PDT
Created attachment 137588 [details]
regression : blinking text animation

This animation no longer works after this commit.

In this case, when the text color should be changed to white :
 - styleChangeType() is SyntheticStyleChange, the animated style (color: white) is set on the Element in setRenderStyle(newStyle)
 - currentStyle and newStyle are the same (color: black), so ch is NoChange
 - After r112155, change is no more set to Force, so Text::recalcTextStyle is called with NoChange as argument, the style of the text is not changed, so the text color is still black.
Comment 15 Alexey Proskuryakov 2012-04-17 13:57:34 PDT
Could you please file a new bug about the regression? It's helpful to mention the problem in a bug that introduced it, but actual discussion should happen in a new one.
Comment 16 Loïc Yhuel 2012-04-17 14:36:24 PDT
(In reply to comment #15)
> Could you please file a new bug about the regression? It's helpful to mention the problem in a bug that introduced it, but actual discussion should happen in a new one.

https://bugs.webkit.org/show_bug.cgi?id=84194
Comment 17 Alexey Proskuryakov 2012-05-22 10:55:07 PDT
This also caused bug 87146.
Comment 18 Dean Jackson 2012-05-25 12:35:19 PDT
We've received multiple reports internally and externally that this is breaking things. Sorry, I'm going to revert.
Comment 19 Dean Jackson 2012-05-25 13:04:04 PDT
Reverted in http://trac.webkit.org/changeset/118553
Comment 20 Elliott Sprehn 2013-01-17 18:53:31 PST
(In reply to comment #18)
> We've received multiple reports internally and externally that this is breaking things. Sorry, I'm going to revert.

Can you explain what was broken? It's not clear in the bug.
Comment 21 Rob Arnold 2013-01-18 10:33:34 PST
(In reply to comment #20)
> (In reply to comment #18)
> > We've received multiple reports internally and externally that this is breaking things. Sorry, I'm going to revert.
> 
> Can you explain what was broken? It's not clear in the bug.

In the blinking text example (bug 84194), the text would not blink. My guess was that any inherited values would not be animated as a side effect of my change. The use case I'm interested in is animating transforms which cannot (I think) be inherited.