[chromium] Unknown transforms should be treated as non-axis aligned on main thread
Created attachment 134117 [details] Patch
(1) Does this patch mean that the renderSurfaceLayerList that is computed by the main thread would actually be different than the renderSurfaceLayerList computed by the impl thread? (2) Until now, we were essentially guaranteed that both sides would execute calcDrawTransforms essentially in the same way, right? At a glance I think its an unavoidable side-effect without massive impractical redesign, but I just wanted to ask, because it feels like a slippery slope.
(In reply to comment #2) > (1) Does this patch mean that the renderSurfaceLayerList that is computed by the main thread would actually be different than the renderSurfaceLayerList computed by the impl thread? Yes, accelerated animations + threaded compositor = the two sides differ in what they know about the world with certainty. > (2) Until now, we were essentially guaranteed that both sides would execute calcDrawTransforms essentially in the same way, right? > > At a glance I think its an unavoidable side-effect without massive impractical redesign, but I just wanted to ask, because it feels like a slippery slope. Yeh, they are slightly different, with the different cases well displayed in the various bool functions near the top. I think it's totally okay for main side to have more surfaces in its view of the world than impl. It shouldn't miss any that impl has though. From perspective of me looking at my screen, more render surfaces along the way won't have any impact, which is what this relies on.
Comment on attachment 134117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134117&action=review Can you explain why the test changes are needed? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:142 > +static inline bool transformToParentIsKnown(CCLayerImpl* layer, bool animatingTransformToParent) style nit: unused variables. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:149 > + return !animatingTransformToParent; I don't understand why this function needs to take animatingTransformToParent. Isn't this just "return !layer->transformIsAnimating();"?
Comment on attachment 134117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134117&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:142 >> +static inline bool transformToParentIsKnown(CCLayerImpl* layer, bool animatingTransformToParent) > > style nit: unused variables. oh thanks. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:149 >> + return !animatingTransformToParent; > > I don't understand why this function needs to take animatingTransformToParent. Isn't this just "return !layer->transformIsAnimating();"? true! I'll remove the animatingTransformToParent entirely. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:-690 > - addAnimatedTransformToController(*childOfRoot->layerAnimationController(), 10, 30, 0); Since this isn't supposed to be a rendersurface i took the animated transform off of it. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:689 > addAnimatedTransformToController(*renderSurface2->layerAnimationController(), 10, 30, 0); This verifies that an animated transform (instead of animated opacity) does make it into a rendersurface now. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:745 > - EXPECT_TRUE(renderSurface2->renderSurface()->drawOpacityIsAnimating()); > + EXPECT_FALSE(renderSurface2->renderSurface()->drawOpacityIsAnimating()); This animates a transform now instead of opacity. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:752 > + EXPECT_FALSE(childOfRoot->drawTransformIsAnimating()); No longer animating to prevent it from making a surface. This was testing that drawTransforms propagate to children, which is also tested by childOfRS2. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:766 > + EXPECT_FALSE(childOfRoot->screenSpaceTransformIsAnimating()); No longer animating to prevent it from making a surface. This was testing that drawTransforms propagate to children, which is also tested by childOfRS2.
Created attachment 134155 [details] Patch
Comment on attachment 134155 [details] Patch Ah, thanks for the explanation. R=me.
Comment on attachment 134155 [details] Patch Clearing flags on attachment: 134155 Committed r112332: <http://trac.webkit.org/changeset/112332>
All reviewed patches have been landed. Closing bug.