Bug 82370 – [chromium] Unknown transforms should be treated as non-axis aligned on main thread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Unknown transforms should be treated as non-axis aligned on main thread

[Dana Jansens]

Created attachment 134117 [details]
Patch

[Shawn Singh]

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

[Dana Jansens]

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

[Adrienne Walker]

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();"?

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 134155 [details]
Patch

[Adrienne Walker]

Comment on attachment 134155 [details]
Patch

Ah, thanks for the explanation.  R=me.

[WebKit Review Bot]

Comment on attachment 134155 [details]
Patch

Clearing flags on attachment: 134155

Committed r112332: <http://trac.webkit.org/changeset/112332>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.