Bug 87686 – [chromium] Accelerated animations should use WebTransformOperations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

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.

Created attachment 144422 [details]
Patch

Created attachment 145160 [details]
Patch

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).

[James Robinson]

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?

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.

[WebKit Review Bot]

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

Created attachment 145330 [details]
Patch

Including Vector to make the build happy.

[WebKit Review Bot]

Comment on attachment 145330 [details]
Patch

Clearing flags on attachment: 145330

Committed r119283: <http://trac.webkit.org/changeset/119283>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/25304/steps/compile/logs/stdio

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Also note that this patch had already required http://trac.webkit.org/changeset/119287/ and http://trac.webkit.org/changeset/119291/ to even compile.

[Ryosuke Niwa]

I'm sorry but I'm going to roll out this patch since nobody is responding to me on IRC or on Bugzilla.

[WebKit Review Bot]

Re-opened since this is blocked by 88159

[Ryosuke Niwa]

The patch author says he's going to investigate the issue, so I'll hold off on the roll out.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Here's chromium changeset for FYI: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=%2Ftrunk%2Fsrc&range=140118%3A140103&mode=html

Created attachment 145901 [details]
Patch

Fixes clang compilation issues and unit test failures on Mac.

[James Robinson]

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?

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.

[James Robinson]

Comment on attachment 145914 [details]
Patch

R=me

[WebKit Review Bot]

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

Created attachment 146609 [details]
patch for landing

patch for landing

[WebKit Review Bot]

Comment on attachment 146609 [details]
patch for landing

Clearing flags on attachment: 146609

Committed r119850: <http://trac.webkit.org/changeset/119850>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.