Bug 82571 – [chromium] Flip transition painting delayed with threaded animations

• Component: CSS
• Status: RESOLVED FIXED

[Eric Penner]

On the "flip" transition here:
http://jquerymobile.com/demos/1.0.1/docs/pages/page-transitions.html

Since this layer is completely visible I figure it should be our best case and be painted during the first webkit commit. However without the CLs to block until the texture is ready, the layer doesn't paint until well after it is flipped... I think there might be some remaining back-face culling logic that prevents it from getting painted until it has flipped over.

[Dana Jansens]

Oh interesting, I didnt think of backface stuff. Yeh for sure it will get dropped from the render surface tree on webkit side until it is flipped. Then suddenly we have this enormous layer to paint. Nice find! Should be easy to fix (if animating dont backface cull).

[Dana Jansens]

Created attachment 134939 [details]
Patch

[Dana Jansens]

To experiment, I made prepainting paint entire animating layers, and stopped dropping checkerboarded animation.

With a really big window, and without this patch, you can see the red tile borders of missing tiles as the layer flips over. With this patch, they prepaint and you get no checkerboard (or very short depending on how fast it can prepaint when the animation starts).

An interesting note however is... without this patch: I am able to see the checkerboard the black "behind" layer when it flips over. But I never saw it once on the "front" layer when it animates and flips over into view. Eric, did you notice this too?

[Eric Penner]

I just see the delay just as the layer is flipping over, but didn't see the missing tiles. 

This is great. It would be great if this were more easily cherry-pick-able. Do the methods need to change places in this CL?

[Dana Jansens]

In order to share code for deciding if a layer's back face is visible, that function needs to move to the .h or the calcVisibleLayerRect function needs to move to the .cpp. I think the removal from the class definition is a small change after that.. so basically I think its going to look something like this with moving code unless we duplicated that code..

[Adrienne Walker]

Comment on attachment 134939 [details]
Patch

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

epenner: Is there some reason this patch isn't cherry-pickable? Are there other patches that need to be cherry-picked first?

> Source/WebCore/ChangeLog:24
> +        The calculateVisibleLayerRect needs to use the backFaceIsVisible()
> +        helper, so we move it into the .cpp file where it belongs, and make
> +        it static to the file along with calculateVisibleRect. These methods
> +        were not used outside of the CCLTHCommon.cpp at all.

Yay, that's really where it should be.  If it ever does need to be used outside (again) we should just leave it in the cpp and expose some explicit functions like we do for everything else.  Thank you!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:154
> +static inline bool transformToScreenIsKnown(LayerChromium*, bool animatingTransformToScreen)
> +{
> +    return !animatingTransformToScreen;
> +}

No need to pass animatingTransformToScreen here.  You can just ask the layer.

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1033
> +    // FIXME: when we fix this "root-layer special case" behavior in CCLayerTreeHost, we will have to fix it here, too.

I see this copied around in so many tests.  I don't even know what this means anymore.

[Dana Jansens]

Comment on attachment 134939 [details]
Patch

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

>> Source/WebCore/ChangeLog:24
>> +        were not used outside of the CCLTHCommon.cpp at all.
> 
> Yay, that's really where it should be.  If it ever does need to be used outside (again) we should just leave it in the cpp and expose some explicit functions like we do for everything else.  Thank you!

Yep :)

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:154
>> +}
> 
> No need to pass animatingTransformToScreen here.  You can just ask the layer.

Yes, this happens later in the function, thanks.

>> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1033
>> +    // FIXME: when we fix this "root-layer special case" behavior in CCLayerTreeHost, we will have to fix it here, too.
> 
> I see this copied around in so many tests.  I don't even know what this means anymore.

That's true, I'll just remove it here.

[Dana Jansens]

Created attachment 135358 [details]
Patch

[Eric Penner]

> epenner: Is there some reason this patch isn't cherry-pickable? Are there other patches that need to be cherry-picked first?
> 

I'm sure it'll be fine. Sounds like the movement had good reason to go in this CL but just wanted to be sure.

[Adrienne Walker]

Comment on attachment 135358 [details]
Patch

R=me.  Sorry for not getting back to this sooner.

[WebKit Review Bot]

Comment on attachment 135358 [details]
Patch

Rejecting attachment 135358 [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:
n'
Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp: In member function 'virtual void<unnamed>::CCLayerTreeHostCommonTest_verifyVisibleRectForPerspectiveUnprojection_Test::TestBody()':
Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1237: error: 'calculateVisibleRect' is not a member of 'WebCore::CCLayerTreeHostCommon'
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12361149

[Dana Jansens]

Created attachment 136112 [details]
Patch

rebased

a bunch of tests were added that use CCLTHC::calculateVisibleRect() so I left it exposed as a static method of the class (calculateVisibleLayerRect could still move into the .cpp)

[WebKit Review Bot]

Comment on attachment 136112 [details]
Patch

Clearing flags on attachment: 136112

Committed r113602: <http://trac.webkit.org/changeset/113602>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Penner]

Comment on attachment 136112 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:88
> +    // and have their back face showing. In this case, their visible rect should be empty.

I didn't notice this before, but doesn't this line cause the same problem?  If this ends up being used for the paint rect, we won't paint anything in advance, but only when it becomes flipped?

[Dana Jansens]

Comment on attachment 136112 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:88
>> +    // and have their back face showing. In this case, their visible rect should be empty.
> 
> I didn't notice this before, but doesn't this line cause the same problem?  If this ends up being used for the paint rect, we won't paint anything in advance, but only when it becomes flipped?

No, we have special code in idlePaintRect() for animating layers with no visible area.

[Eric Penner]

Comment on attachment 136112 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:88
>>> +    // and have their back face showing. In this case, their visible rect should be empty.
>> 
>> I didn't notice this before, but doesn't this line cause the same problem?  If this ends up being used for the paint rect, we won't paint anything in advance, but only when it becomes flipped?
> 
> No, we have special code in idlePaintRect() for animating layers with no visible area.

I think that has a size constraint right (rightfully so as otherwise it would use up all our memory)? The link demonstrating this bug is full page transition:
http://jquerymobile.com/demos/1.0.1/docs/pages/page-transitions.html (flip)

For back-face visibility, what would be wrong with just ignoring whether it is back-facing for animated layers? Then it would be visible on the first animated frame and get painted before the animation-started event is triggered.

[Dana Jansens]

(In reply to comment #17)
> I think that has a size constraint right (rightfully so as otherwise it would use up all our memory)? The link demonstrating this bug is full page transition:
> http://jquerymobile.com/demos/1.0.1/docs/pages/page-transitions.html (flip)
> 
> For back-face visibility, what would be wrong with just ignoring whether it is back-facing for animated layers? Then it would be visible on the first animated frame and get painted before the animation-started event is triggered.

I guess here's my concern. What if you have a whole lot of backfacing layers. They are not drawn so they don't occlude for paint culling. But they'd all be painted with visible priority then.

I'd like the visibleLayerRect to be non-empty (use that rect for prepainting) but not consider the layer as actually visible for painting..

[Eric Penner]

> I guess here's my concern. What if you have a whole lot of backfacing layers. They are not drawn so they don't occlude for paint culling. But they'd all be painted with visible priority then.

I was thinking this could be only for animated layers though, in which case they should already not occlude for paint culling?

> I'd like the visibleLayerRect to be non-empty (use that rect for prepainting) but not consider the layer as actually visible for painting..

It's valid that this could take memory away from truly visible stuff, but it seems unlikely there would be back-facing animated content that isn't going to become visible during the animation. However, with priorities there could indeed be an extra priority level (just below visible) for this type of thing.

[Dana Jansens]

Ah, yes... you're right! Ok let's do this.

I think that the backface detection code has a small bug -- it assumes orthographic projection.

Comment on attachment 136112 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:56
> +    return zAxis.z() < 0;

This code assumes orthographic projection. I think you want to check the sign of zAxis.dot(somePointOnTheRect - eyePosition) instead.

[Shawn Singh]

(In reply to comment #22)
> (From update of attachment 136112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136112&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:56
> > +    return zAxis.z() < 0;
> 
> This code assumes orthographic projection. I think you want to check the sign of zAxis.dot(somePointOnTheRect - eyePosition) instead.


Please check CCMathUtilTest - there is a test that covers doing back-face visibility with a perspective projection.  As far as I know, it seems to work correctly.   Definitely let me know if something seems further wrong, though - a test case as to how/why it fails would be extremely helpful.


Also, just a heads up to those involved with CCLayerTreeHostCommonTest::verifyBackFaceCulling -- I might possibly be re-naming this to verifyBackFaceCullingWithAnimations, and adding another test to verifyBackFaceCulling without animations.