Bug 82571 - [chromium] Flip transition painting delayed with threaded animations
Summary: [chromium] Flip transition painting delayed with threaded animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-28 21:29 PDT by Eric Penner
Modified: 2012-05-16 10:43 PDT (History)
10 users (show)

See Also:


Attachments
Patch (20.03 KB, patch)
2012-03-30 21:03 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (19.15 KB, patch)
2012-04-03 10:34 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (18.14 KB, patch)
2012-04-06 18:04 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Penner 2012-03-28 21:29:11 PDT
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.
Comment 1 Dana Jansens 2012-03-28 22:10:26 PDT
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).
Comment 2 Dana Jansens 2012-03-30 21:03:46 PDT
Created attachment 134939 [details]
Patch
Comment 3 Dana Jansens 2012-03-30 21:06:48 PDT
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?
Comment 4 Eric Penner 2012-04-02 12:53:56 PDT
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?
Comment 5 Dana Jansens 2012-04-02 14:03:08 PDT
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..
Comment 6 Adrienne Walker 2012-04-02 19:29:30 PDT
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.
Comment 7 Dana Jansens 2012-04-03 10:11:52 PDT
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.
Comment 8 Dana Jansens 2012-04-03 10:34:05 PDT
Created attachment 135358 [details]
Patch
Comment 9 Eric Penner 2012-04-03 11:17:13 PDT
> 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.
Comment 10 Adrienne Walker 2012-04-06 17:30:02 PDT
Comment on attachment 135358 [details]
Patch

R=me.  Sorry for not getting back to this sooner.
Comment 11 WebKit Review Bot 2012-04-06 17:42:07 PDT
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
Comment 12 Dana Jansens 2012-04-06 18:04:38 PDT
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)
Comment 13 WebKit Review Bot 2012-04-09 11:58:23 PDT
Comment on attachment 136112 [details]
Patch

Clearing flags on attachment: 136112

Committed r113602: <http://trac.webkit.org/changeset/113602>
Comment 14 WebKit Review Bot 2012-04-09 11:58:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Eric Penner 2012-05-10 19:29:35 PDT
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?
Comment 16 Dana Jansens 2012-05-10 19:41:39 PDT
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.
Comment 17 Eric Penner 2012-05-10 20:00:42 PDT
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.
Comment 18 Dana Jansens 2012-05-15 17:37:20 PDT
(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..
Comment 19 Eric Penner 2012-05-15 17:52:10 PDT
> 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.
Comment 20 Dana Jansens 2012-05-15 18:00:45 PDT
Ah, yes... you're right! Ok let's do this.
Comment 21 vollick 2012-05-16 05:35:56 PDT
I think that the backface detection code has a small bug -- it assumes orthographic projection.
Comment 22 vollick 2012-05-16 05:36:37 PDT
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.
Comment 23 Shawn Singh 2012-05-16 10:43:17 PDT
(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.