Bug 55022 - REGRESSION: Accelerated transitions are jumpy
Summary: REGRESSION: Accelerated transitions are jumpy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Chris Marrin
URL: http://www.eleqtriq.com/2010/05/css-3...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-02-22 21:50 PST by Simon Fraser (smfr)
Modified: 2011-03-03 15:16 PST (History)
5 users (show)

See Also:


Attachments
Patch (24.59 KB, patch)
2011-02-23 23:25 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch botched by Windows (821 bytes, patch)
2011-03-01 11:28 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Windows Patch (1.53 KB, patch)
2011-03-01 11:37 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2011-03-02 16:28 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch changing TimingFunction per Adam's suggestion (1.33 KB, patch)
2011-03-03 14:07 PST, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-02-22 21:50:25 PST
Go to http://www.eleqtriq.com/2010/05/css-3d-matrix-transformations/, and scroll down to the "Example: Coverflow in CSS" section, and click Open Example. In the example, alternately hover over two adjacent cards. Note how they jump backwards and forwards, rather than animating smoothly. This is a regression from Safari 5.
Comment 1 Simon Fraser (smfr) 2011-02-22 21:51:10 PST
<rdar://problem/9040742>
Comment 2 Simon Fraser (smfr) 2011-02-23 22:46:32 PST
We're getting the timing function wrong.
Comment 3 Simon Fraser (smfr) 2011-02-23 23:25:06 PST
Created attachment 83611 [details]
Patch
Comment 4 Eric Seidel (no email) 2011-02-24 02:27:49 PST
Comment on attachment 83611 [details]
Patch

rs=me.
Comment 5 Simon Fraser (smfr) 2011-02-24 08:13:49 PST
http://trac.webkit.org/changeset/79568
Comment 6 mitz 2011-02-24 08:18:46 PST
Comment on attachment 83611 [details]
Patch

Consider getting rid of the text in the test in the success case and landing the results in a cross-platform location.
Comment 7 Simon Fraser (smfr) 2011-02-24 09:28:47 PST
I plan to do a pass over the transition/animation tests and make them dump as text.

However, the pixel results aren't cross platform; the CALayer can paint non pixel-aligned.
Comment 8 Dean Jackson 2011-02-24 10:12:09 PST
sorry! my mistake!
Comment 9 Dean Jackson 2011-02-24 10:35:45 PST
I take it back. I'm not sorry. This was Chris :)
Comment 10 Chris Marrin 2011-02-28 15:07:28 PST
It's still wrong on the Win version. I'll add a patch for that presently.
Comment 11 Chris Marrin 2011-03-01 11:28:55 PST
Created attachment 84257 [details]
Patch botched by Windows
Comment 12 Chris Marrin 2011-03-01 11:37:51 PST
Created attachment 84258 [details]
Windows Patch
Comment 13 Simon Fraser (smfr) 2011-03-01 11:41:50 PST
Comment on attachment 84258 [details]
Windows Patch

I think we should do the null check higher up and always pass in a TimingFunction here, because now these magic numbers are in 3 places.
Comment 14 Chris Marrin 2011-03-02 16:28:14 PST
Created attachment 84482 [details]
Patch
Comment 15 Simon Fraser (smfr) 2011-03-02 16:51:47 PST
Comment on attachment 84482 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:128
> +    ASSERT(timingFunction);
> +    

The Mac version has no blank line after the assert.
Comment 16 Chris Marrin 2011-03-03 11:33:33 PST
Committed r80266: <http://trac.webkit.org/changeset/80266>
Comment 17 Adam Roben (:aroben) 2011-03-03 11:51:58 PST
Comment on attachment 84482 [details]
Patch

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

> Source/WebCore/platform/animation/TimingFunction.h:109
> +    static const CubicBezierTimingFunction* defaultTimingFunction()
> +    {
> +        DEFINE_STATIC_LOCAL(const CubicBezierTimingFunction, dtf, ());
> +        return &dtf;
> +    }

This is wacky, and could lead to assertion failures due to adoptRef never being called. I'd suggest writing it like this:

static const CubicBezierTimingFunction* function = create().leakRef();
return function;
Comment 18 Chris Marrin 2011-03-03 14:07:52 PST
Created attachment 84623 [details]
Patch changing TimingFunction per Adam's suggestion
Comment 19 Chris Marrin 2011-03-03 15:16:01 PST
Committed r80289: <http://trac.webkit.org/changeset/80289>