Bug 87686 - [chromium] Accelerated animations should use WebTransformOperations
Summary: [chromium] Accelerated animations should use WebTransformOperations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on: 87510 88159
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-28 18:39 PDT by vollick
Modified: 2012-06-08 11:42 PDT (History)
5 users (show)

See Also:


Attachments
Patch (86.77 KB, patch)
2012-05-28 18:45 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (89.85 KB, patch)
2012-05-31 14:47 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (98.11 KB, patch)
2012-05-31 19:17 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (98.21 KB, patch)
2012-06-01 08:04 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (98.26 KB, patch)
2012-06-01 09:53 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (103.26 KB, patch)
2012-06-05 17:22 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (103.41 KB, patch)
2012-06-05 19:04 PDT, vollick
no flags Details | Formatted Diff | Diff
patch for landing (103.39 KB, patch)
2012-06-08 11:03 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-05-28 18:39:07 PDT
This will require the following
 - Change CCTransformKeyframe to own a WebTransformOperations rather than a TransformOperations.
 - Change LayerChromium's API. LayerChromium::addAnimation should take only a CCActiveAnimation.
 - GraphicsLayerChromium is responsible for translating to WebTransformOperations and creating CCActiveAnimations.
 - Tests that use the public API (that is, they call addAnimation with KeyframeValueList and Animation arguments) must be moved to GraphicsLayerChromiumTest.cpp.
Comment 1 vollick 2012-05-28 18:45:58 PDT
Created attachment 144422 [details]
Patch
Comment 2 vollick 2012-05-31 14:47:28 PDT
Created attachment 145160 [details]
Patch
Comment 3 vollick 2012-05-31 19:17:41 PDT
Created attachment 145194 [details]
Patch

Moved the animation translation code into AnimationTranslationUtil.

Also added logic to bail on accelerated anim when there are rotations of >= 180
degrees and when we have singular matrices (as well as some unit tests).
Comment 4 James Robinson 2012-05-31 19:59:09 PDT
Comment on attachment 145194 [details]
Patch

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

Nice! R=me, but I'm pretty sure that http://trac.webkit.org/changeset/119178 has given you the gift of many merge conflicts.

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:75
> +            ScaleTransformOperation* transform = static_cast<ScaleTransformOperation*>(transformOperations.operations()[j].get());

I think using temporaries like this is fine, but can we const it up a bit?
Comment 5 vollick 2012-06-01 08:04:10 PDT
Created attachment 145309 [details]
Patch

Patch for landing.

(In reply to comment #4)
> (From update of attachment 145194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145194&action=review
>
> Nice! R=me, but I'm pretty sure that http://trac.webkit.org/changeset/119178 has given you the gift of many merge conflicts.
>
> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:75
> > +            ScaleTransformOperation* transform = static_cast<ScaleTransformOperation*>(transformOperations.operations()[j].get());
>
> I think using temporaries like this is fine, but can we const it up a bit?

Done.
Comment 6 WebKit Review Bot 2012-06-01 09:13:42 PDT
Comment on attachment 145309 [details]
Patch

Rejecting attachment 145309 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
cs/chromium/cc/CCKeyframedAnimationCurve.h:103: error: expected ';' before '<' token
Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:127: error: ISO C++ forbids declaration of 'Vector' with no type
Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.h:127: error: expected ';' before '<' token
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/CCKeyframedAnimationCurveTest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12866565
Comment 7 vollick 2012-06-01 09:53:21 PDT
Created attachment 145330 [details]
Patch

Including Vector to make the build happy.
Comment 8 WebKit Review Bot 2012-06-01 14:46:58 PDT
Comment on attachment 145330 [details]
Patch

Clearing flags on attachment: 145330

Committed r119283: <http://trac.webkit.org/changeset/119283>
Comment 9 WebKit Review Bot 2012-06-01 14:47:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Ryosuke Niwa 2012-06-01 14:59:53 PDT
This patch broke Chromium Mac:
../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/LayerChromium.h:83:28: error: 'bounds' marked 'override' but does not override any member functions
    virtual const IntSize& bounds() const OVERRIDE { return m_bounds; }
                           ^
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:42:
In file included from ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.h:29:
../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:66:28: error: 'bounds' marked 'override' but does not override any member functions
    virtual const IntSize& bounds() const OVERRIDE { return m_bounds; }
                           ^
2 errors generated.
Comment 12 Ryosuke Niwa 2012-06-01 18:26:18 PDT
It appears that this caused basicCreateAndDestroy not to complete:
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.5%20%28dbg%29%281%29/builds/7810

Blame list: http://trac.webkit.org/log/?verbose=on&rev=119292&stop_rev=119282
Comment 13 Ryosuke Niwa 2012-06-01 18:29:19 PDT
Same issue on 10.6:
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28dbg%29/builds/10733

LayerChromiumTest.basicCreateAndDestroy: 
Did not complete.
Comment 14 Ryosuke Niwa 2012-06-01 18:30:15 PDT
Also note that this patch had already required http://trac.webkit.org/changeset/119287/ and http://trac.webkit.org/changeset/119291/ to even compile.
Comment 15 Ryosuke Niwa 2012-06-01 18:32:02 PDT
I'm sorry but I'm going to roll out this patch since nobody is responding to me on IRC or on Bugzilla.
Comment 16 WebKit Review Bot 2012-06-01 18:33:01 PDT
Re-opened since this is blocked by 88159
Comment 17 Ryosuke Niwa 2012-06-01 18:35:21 PDT
The patch author says he's going to investigate the issue, so I'll hold off on the roll out.
Comment 18 Ryosuke Niwa 2012-06-01 19:39:49 PDT
Ian and I both investigated this problem (I used the exact revision for both WebKit and Chromium) but neither of us could reproduce the failure locally. It's not clear why this assertion failure occurs:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28dbg%29/builds/10734/steps/webkit_unit_tests/logs/stdio

[----------] 15 tests from LayerChromiumTest
[ RUN      ] LayerChromiumTest.basicCreateAndDestroy
ASSERTION FAILED: CCProxy::isMainThread()
/Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/chromium/cc/CCLayerTreeHost.cpp(91) : WebCore::CCLayerTreeHost::CCLayerTreeHost(WebCore::CCLayerTreeHostClient *, const WebCore::CCSettings &)
1   0x49a2af92 WebCore::CCLayerTreeHost::CCLayerTreeHost(WebCore::CCLayerTreeHostClient*, WebCore::CCSettings const&)
2   0x4751151a (anonymous namespace)::MockCCLayerTreeHost::MockCCLayerTreeHost()
3   0x475113eb (anonymous namespace)::MockCCLayerTreeHost::MockCCLayerTreeHost()
4   0x4750f8b5 (anonymous namespace)::LayerChromiumTest::SetUp()
5   0x4797a3a1 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
6   0x4796f9de void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
7   0x479649b8 testing::Test::Run()
8   0x47965077 testing::TestInfo::Run()
9   0x479655f9 testing::TestCase::Run()
10  0x4796a925 testing::internal::UnitTestImpl::RunAllTests()
11  0x47978041 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
12  0x47971b8e bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
13  0x4796a5b4 testing::UnitTest::Run()
14  0x47b22df0 base::TestSuite::Run()
15  0x46f13349 main
16  0x46f132a6 start
17  0x2
[1040:-1602484928:0601/171831:203491476840:ERROR:process_util_posix.cc(143)] Received signal 11
	0   webkit_unit_tests                   0x479d1fff base::debug::StackTrace::StackTrace() + 63
	1   webkit_unit_tests                   0x479d1f9b base::debug::StackTrace::StackTrace() + 43
	2   webkit_unit_tests                   0x47a8b667 base::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, __darwin_ucontext*) + 295
	3   libSystem.B.dylib                   0x9539405b _sigtramp + 43
	4   ???                                 0xffffffff 0x0 + 4294967295
	5   webkit_unit_tests                   0x4751151a (anonymous namespace)::MockCCLayerTreeHost::MockCCLayerTreeHost() + 106
	6   webkit_unit_tests                   0x475113eb (anonymous namespace)::MockCCLayerTreeHost::MockCCLayerTreeHost() + 43
	7   webkit_unit_tests                   0x4750f8b5 (anonymous namespace)::LayerChromiumTest::SetUp() + 101
	8   webkit_unit_tests                   0x4797a3a1 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 145
	9   webkit_unit_tests                   0x4796f9de void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 126
	10  webkit_unit_tests                   0x479649b8 testing::Test::Run() + 152
	11  webkit_unit_tests                   0x47965077 testing::TestInfo::Run() + 263
	12  webkit_unit_tests                   0x479655f9 testing::TestCase::Run() + 265
	13  webkit_unit_tests                   0x4796a925 testing::internal::UnitTestImpl::RunAllTests() + 821
	14  webkit_unit_tests                   0x47978041 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 145
	15  webkit_unit_tests                   0x47971b8e bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 126
	16  webkit_unit_tests                   0x4796a5b4 testing::UnitTest::Run() + 148
	17  webkit_unit_tests                   0x47b22df0 base::TestSuite::Run() + 256
	18  webkit_unit_tests                   0x46f13349 main + 105
	19  webkit_unit_tests                   0x46f132a6 start + 54
	20  ???                                 0x00000002 0x0 + 2
ax: bbadbeef, bx: 4ccd2868, cx: 9f542ff7, dx: 9f542ff7
di: 4b873216, si: 4bad9f4e, bp: bffff178, sp: bffff130, ss: 23, flags: 10286
ip: 49a2af9c, cs: 1b, ds: 23, es: 23, fs: 0, gs: f
Comment 19 Ryosuke Niwa 2012-06-01 19:47:50 PDT
At this point, I'm going to roll out his change per Ian's request. If that doesn't fix the problem, I'll roll it back in.
Comment 21 vollick 2012-06-05 17:22:32 PDT
Created attachment 145901 [details]
Patch

Fixes clang compilation issues and unit test failures on Mac.
Comment 22 James Robinson 2012-06-05 17:31:30 PDT
Comment on attachment 145901 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

for this and other new files - it's 2012 now, and we prefer 2-clause for new files (although that's not a big deal)

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.h:43
> +PassOwnPtr<CCActiveAnimation> createActiveAnimation(const KeyframeValueList&, const Animation*, size_t animationId, size_t groupId, double timeOffset, const FloatSize& boxSize);

Would you mind documenting the behavior of this function with non-supported input (like non-invertable transforms or whatnot?)  Looks like it just returns nullptr, but it'd be good to know that without having to read through the impl

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:506
> +    // Bail early if we have a large rotation.

Hm - we bail here for some things, but it looks like createActiveAnimation() returns nullptr for other "bad" values.  What's the rationale for the split?
Comment 23 vollick 2012-06-05 19:04:02 PDT
Created attachment 145914 [details]
Patch

(In reply to comment #22)
> (From update of attachment 145901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145901&action=review
>
> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.h:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
>
> for this and other new files - it's 2012 now, and we prefer 2-clause for new files (although that's not a big deal)
Fixed.
>
> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.h:43
> > +PassOwnPtr<CCActiveAnimation> createActiveAnimation(const KeyframeValueList&, const Animation*, size_t animationId, size_t groupId, double timeOffset, const FloatSize& boxSize);
>
> Would you mind documenting the behavior of this function with non-supported input (like non-invertable transforms or whatnot?)  Looks like it just returns nullptr, but it'd be good to know that without having to read through the impl
Done.
>
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:506
> > +    // Bail early if we have a large rotation.
>
> Hm - we bail here for some things, but it looks like createActiveAnimation() returns nullptr for other "bad" values.  What's the rationale for the split?
I do the big angle check in GraphicsLayerChromium, because I use the protected GraphicsLayer::validateTransformOperations. I can do everything else in the AnimationTranslationUtil.
Comment 24 James Robinson 2012-06-07 14:59:14 PDT
Comment on attachment 145914 [details]
Patch

R=me
Comment 25 WebKit Review Bot 2012-06-07 18:35:33 PDT
Comment on attachment 145914 [details]
Patch

Rejecting attachment 145914 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/chromium/tests/CCKeyframedAnimationCurveTest.cpp
patching file Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp
patching file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
patching file Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp
patching file Source/WebKit/chromium/tests/LayerChromiumTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12923212
Comment 26 vollick 2012-06-08 11:03:45 PDT
Created attachment 146609 [details]
patch for landing

patch for landing
Comment 27 WebKit Review Bot 2012-06-08 11:42:02 PDT
Comment on attachment 146609 [details]
patch for landing

Clearing flags on attachment: 146609

Committed r119850: <http://trac.webkit.org/changeset/119850>
Comment 28 WebKit Review Bot 2012-06-08 11:42:08 PDT
All reviewed patches have been landed.  Closing bug.