93536 2012-08-08 15:22:07 -0700 [CoordinatedGraphics] Blink in the end of a CSS transform "translate" 2012-10-15 00:20:13 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 webkit.review.bot rafael.lobo ap cshu dino igor.oliveira kbalazs noam rafael.lobo simon.fraser vollick oldest_to_newest 690815 0 webkit.review.bot 2012-08-08 15:22:07 -0700 [Qt] Blink in the end of a CSS transform "translate" Requested by rafaelbrandao on #webkit. 690819 1 rafael.lobo 2012-08-08 15:27:48 -0700 I will get a small test case and attach soon. But the one I'm seeing is at the end of a CSS animation that uses: webkitTransform = "translate(320px, 0)". In the very end of the animation, the elements blink. I suspect this has to do LayerTreeCoordinator finishing the animation and resetting the animation to the initial state (I can quickly see the squares going back to their initial position before they refresh). So it might be a problem when we try to synchronize things, flushing earlier than expected, and another time again when we fix the elements position to the final position. I hope to not be confusing, I'm still trying to understand how this works exactly, so these are just thoughts I want to share before I forget. :-) 692801 2 157788 rafael.lobo 2012-08-10 12:25:47 -0700 Created attachment 157788 Attached small case that shows the blink at the end of animation. Here's a public link for the html attached: http://dl.dropbox.com/u/93716615/animations.html And yes, the animation seems to go back to the initial position right before updating it on its final position. Still investigating what's causing this. 702424 3 rafael.lobo 2012-08-22 15:27:26 -0700 Ugh, I think I'm close now... still trying to understand. What I've found so far is that at the end of the animation, we call ~CoordinatedGraphicsLayer (as our animation has been removed from GraphicsLayerAnimations) and this one requests to detach its layer from CoordinatedGraphicsLayerClient. Then afterwards, LayerTreeCoordinator schedules a layer flush, but when it performs, we can see the blink (probably because we've removed that layer). When I comment detachLayer from CoordinatedGraphicsLayer destructor, things start to work, but this is obviously the wrong approach. Sharing this status in case someone else have any thoughts. 702436 4 noam 2012-08-22 15:31:06 -0700 (In reply to comment #3) > Ugh, I think I'm close now... still trying to understand. What I've found so far is that at the end of the animation, we call ~CoordinatedGraphicsLayer (as our animation has been removed from GraphicsLayerAnimations) and this one requests to detach its layer from CoordinatedGraphicsLayerClient. Then afterwards, LayerTreeCoordinator schedules a layer flush, but when it performs, we can see the blink (probably because we've removed that layer). Is the blink showing the layer at a wrong position, or not showing the layer at all while it should show it? 702466 5 rafael.lobo 2012-08-22 15:46:17 -0700 (In reply to comment #4) > (In reply to comment #3) > > Ugh, I think I'm close now... still trying to understand. What I've found so far is that at the end of the animation, we call ~CoordinatedGraphicsLayer (as our animation has been removed from GraphicsLayerAnimations) and this one requests to detach its layer from CoordinatedGraphicsLayerClient. Then afterwards, LayerTreeCoordinator schedules a layer flush, but when it performs, we can see the blink (probably because we've removed that layer). > > Is the blink showing the layer at a wrong position, or not showing the layer at all while it should show it? I'm trying to figure out more precisely yet, but when I add usleep(80000) on LayerTreeCoordinator::performScheduledLayerFlush() I can see more clearly that what I see in a blink is the element on its initial position, then it quickly comes back to its final position. If you interrupt an animation in the middle, then I see the element back to its rightmost position, which is quickly returned to the final position. Any ideas? 704056 6 rafael.lobo 2012-08-24 06:32:03 -0700 Just a quick update, the rabbit hole seems to be deeper. Commenting that line does not solve the problem completely, but it at least helps to make it blink less often. I'm still digging on this. :-) 707941 7 rafael.lobo 2012-08-29 15:32:24 -0700 I believe I've finally got what was going on. I will try to explain. When the animation ends, it triggers a Document::recalcStyle. Before that, we delete the animation and send the layer detachment event to the UIProcess. If we ask the recalcStyle to be slow enough, we will see that UIProcess will get this event, process the layer deletion before we have a chance to get the style recalculated at WebProcess. This is what leads us to see a "blink" in the end. Then the WebProcess recalc the style succesfully and do whatever it needs to update the UIProcess with the new layers / positions. As Noam guessed, we have to deal differently with layer deletion, but I'm not sure of what we should do to deal with this. I'll think more about it and try to come with something later. 707995 8 noam 2012-08-29 16:08:33 -0700 (In reply to comment #7) > When the animation ends, it triggers a Document::recalcStyle. Before that, we delete the animation and send the layer detachment event to the UIProcess. If we ask the recalcStyle to be slow enough, we will see that UIProcess will get this event, process the layer deletion before we have a chance to get the style recalculated at WebProcess. This is what leads us to see a "blink" in the end. Then the WebProcess recalc the style succesfully and do whatever it needs to update the UIProcess with the new layers / positions. > Ah! Yes, I think what needs to be done is on LayerTreeCoordinator::detachLayer don't send the message to the UI process directly, but rather keep it in a list (layersToDelete), and flush the list, sending all the DeleteCompositingLayer messages during flushPendingLayerChanges. This will assure that layer deletion occurs in sync with the new layout. 708520 9 161448 rafael.lobo 2012-08-30 05:25:31 -0700 Created attachment 161448 Patch 708602 10 161448 noam 2012-08-30 07:34:24 -0700 Comment on attachment 161448 Patch Come to think of it, this will be a noop since layer creation/deletion happens during relayout anyway. 708755 11 ap 2012-08-30 10:00:27 -0700 This bug has [Qt] in title, which means that non-Qt folks have absolutely no need to look at it. But the patch changes files that look like they are cross-platform. Or is COORDINATED_GRAPHICS a purely Qt thing that's not of interest to anyone else? 709521 12 rafael.lobo 2012-08-31 05:43:20 -0700 (In reply to comment #11) > This bug has [Qt] in title, which means that non-Qt folks have absolutely no need to look at it. But the patch changes files that look like they are cross-platform. > > Or is COORDINATED_GRAPHICS a purely Qt thing that's not of interest to anyone else? It seems that efl also uses it. But in this bug I'm starting to think that it's the mix of COORDINATED_GRAPHICS and ACCELERATED_COMPOSITING that causes the blink. Is there anything I should add to the title to make this more clear? I will remove [Qt] and replace by [WK2] for now. Please feel free to CC folks you know that have worked with any of those subjects. 709700 13 simon.fraser 2012-08-31 09:12:22 -0700 Mac doesn't use CoordinatedGraphics. I have no idea what it means. 709707 14 rafael.lobo 2012-08-31 09:17:17 -0700 Ok. I've changed to [CoordinatedGraphics] instead, I believe Qt is the only port which uses that and accelerated compositing which I believe is what leads to this bug. 709791 15 noam 2012-08-31 10:44:57 -0700 Qt and EFL use (In reply to comment #13) > Mac doesn't use CoordinatedGraphics. I have no idea what it means. CoordinatedGraphics is used by Qt and EFL, but does not have Qt/EFL specific code. 722962 16 rafael.lobo 2012-09-18 12:03:12 -0700 As the patch on bug #96919 is quite similar to this one, I believe we will no longer be able to see this bug happening when it lands. 741802 17 kbalazs 2012-10-15 00:20:13 -0700 The test case seems to work properly now. 157788 2012-08-10 12:25:47 -0700 2012-08-10 12:25:47 -0700 Attached small case that shows the blink at the end of animation. animations.html text/html 312 rafael.lobo PGh0bWw+CjxoZWFkPgo8L2hlYWQ+CjxzdHlsZSB0eXBlPSJ0ZXh0L2NzcyI+CiNib3ggewogIHBv c2l0aW9uOiBhYnNvbHV0ZTsKICB3aWR0aDogMjAwcHg7CiAgaGVpZ2h0OiAyMDBweDsKICB4OiAx MHB4OwogIHk6IDEwcHg7CiAgYmFja2dyb3VuZC1jb2xvcjogIzExMTsKICAtd2Via2l0LXRyYW5z aXRpb246IGFsbCAxLjBzIGVhc2UtaW4tb3V0Owp9CiNib3g6aG92ZXIgewogIC13ZWJraXQtdHJh bnNmb3JtOiB0cmFuc2xhdGUoNDAwcHgsIDApOwp9Cjwvc3R5bGU+Cjxib2R5Pgo8ZGl2IGlkPSJi b3giPjwvZGl2Pgo8L2JvZHk+CjwvaHRtbD4K 161448 2012-08-30 05:25:31 -0700 2012-08-30 07:34:24 -0700 Patch bug-93536-20120830092524.patch text/plain 3899 rafael.lobo U3VidmVyc2lvbiBSZXZpc2lvbjogMTI3MTI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggYjljNTNkN2Y4NDFkNjlh YzI1NTFmOTVlNDI1ZmQwOTcyOGM3ODM3OC4uOWJhNzA4OTI1YTA3ZjQ1YTNjNmZkY2Q3ZjU0NmRm NjA1MzNhYjgwMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDEyLTA4LTMwICBSYWZh ZWwgQnJhbmRhbyAgPHJhZmFlbC5sb2JvQG9wZW5ib3NzYS5vcmc+CisKKyAgICAgICAgW1F0XSBC bGluayBpbiB0aGUgZW5kIG9mIGEgQ1NTIHRyYW5zZm9ybSAidHJhbnNsYXRlIgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTM1MzYKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBPbmNlIGFuIGFuaW1hdGlvbiBl bmRzLCBzb21lIG5vZGVzIHdpbGwgYmUgY2hhbmdlZCBhbmQgdGhlbiB3ZSB3aWxsIGhhdmUgdG8g dHJpZ2dlciBhIG5ldworICAgICAgICBzdHlsZSByZWNhbGN1bGF0aW9uLCBidXQgdGhpcyBkb2Vz bid0IGhhcHBlbiByaWdodCBhd2F5LiBUaW1lIHRvIHRpbWUgaXQgaXMgdHJpZ2dlcmVkIGJ5Cisg ICAgICAgIEFuaW1hdGlvbkNvbnRyb2xsZXJQcml2YXRlOjpmaXJlRXZlbnRzQW5kVXBkYXRlU3R5 bGUuIEJ1dCB0aGUgYW5pbWF0aW9uIGl0c2VsZiB3YXMgYmVpbmcgZGVsZXRlZAorICAgICAgICBp bW1lZGlhdGVseSBhbmQgdGhpcyByZXN1bHRlZCBpbiBhIGxheWVyIGRldGFjaG1lbnQgZXZlbnQg dG8gYmUgc2VudCB0byBVSVByb2Nlc3MgYXQgdGhhdCB0aW1lLiBCZWZvcmUKKyAgICAgICAgV2Vi UHJvY2VzcyBnZXQgYSBjaGFuY2UgdG8gYWN0dWFsbHkgdHJpZ2dlciBEb2N1bWVudDo6cmVjYWxj U3R5bGUsIFVJUHJvY2VzcyB3YXMgcmVjZWl2aW5nIHRoaXMgZXZlbnQgYW5kCisgICAgICAgIGRl bGV0aW5nIHRoZSBsYXllciwgc28gaXQgZW5kZWQgdXAgc2hvd2luZyBhbiBlbXB0eSBwYWdlIHVu dGlsIHdlIGdldCB0aGUgc3R5bGUgcmVjYWxjdWxhdGVkIGFuZCB0aGUgc3Vic2VxdWVudAorICAg ICAgICBsYXllciBtb2RpZmljYXRpb25zIHdlcmUgc2VudCB0byBVSVByb2Nlc3MuCisgICAgICAg IAorICAgICAgICBUaGUgc29sdXRpb24gaGVyZSBpcyB0byBrZWVwIHRoZXNlIGxheWVyIGRldGFj aG1lbnRzIG9uIGhvbGQgdW50aWwgd2UgZ2V0IHRoZSBzdHlsZSByZWNhbGN1bGF0ZWQuCisgICAg ICAgIE9uY2UgaXQgaXMgZmluaXNoZWQsIHdlIGNhbiBzZW5kIHRoZXNlIHRvIFVJUHJvY2VzcyBh bmQgZmluYWxseSBkZWxldGUgdGhlbS4KKworICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9D b29yZGluYXRlZEdyYXBoaWNzL0xheWVyVHJlZUNvb3JkaW5hdG9yLmNwcDogRGlzcGF0Y2ggbGF5 ZXIgZGVsZXRpb24gZXZlbnRzIHRvIFVJUHJvY2VzcworICAgICAgICBvbiBmbHVzaFBlbmRpbmdM YXllckNoYW5nZXMsIHdoZXJlIHdlIGd1YXJhbnRlZSB0aGUgcmVsYXlvdXQgaXMgYWxyZWFkeSBk b25lLgorICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9Db29yZGluYXRlZEdyYXBoaWNzL0xh eWVyVHJlZUNvb3JkaW5hdG9yLmg6CisKIDIwMTItMDgtMzAgIE1pa2UgU2llcnJhICA8bWlrZS5z aWVycmFAbm9raWEuY29tPgogCiAgICAgICAgIFtRVF1bV0syXSB3ZWJ2aWV3IEFQSSBkb2MKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9Db29yZGluYXRlZEdy YXBoaWNzL0xheWVyVHJlZUNvb3JkaW5hdG9yLmNwcCBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nl c3MvV2ViUGFnZS9Db29yZGluYXRlZEdyYXBoaWNzL0xheWVyVHJlZUNvb3JkaW5hdG9yLmNwcApp bmRleCBhM2E1NzI5MDczYmRlOWZmNzMzNWU3YzJhN2E4NGNlYTNlZDU4M2M1Li42OWEyYzJmMzky NGMwOWJjYTQzMjVkNTRlNmQwZjJiYjQ3NjYxMDJjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvQ29vcmRpbmF0ZWRHcmFwaGljcy9MYXllclRyZWVDb29yZGlu YXRvci5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL0Nvb3JkaW5h dGVkR3JhcGhpY3MvTGF5ZXJUcmVlQ29vcmRpbmF0b3IuY3BwCkBAIC0yNjYsNiArMjY2LDEyIEBA IGJvb2wgTGF5ZXJUcmVlQ29vcmRpbmF0b3I6OmZsdXNoUGVuZGluZ0xheWVyQ2hhbmdlcygpCiAg ICAgICAgIG1fc2hvdWxkU3luY1Jvb3RMYXllciA9IGZhbHNlOwogICAgIH0KIAorICAgIGlmICht X2xheWVyc1RvRGVsZXRlLnNpemUoKSkgeworICAgICAgICBmb3IgKFZlY3RvcjxXZWJLaXQ6Oldl YkxheWVySUQ+OjppdGVyYXRvciBpdCA9IG1fbGF5ZXJzVG9EZWxldGUuYmVnaW4oKTsgaXQgIT0g bV9sYXllcnNUb0RlbGV0ZS5lbmQoKTsgKytpdCkKKyAgICAgICAgICAgIG1fd2ViUGFnZS0+c2Vu ZChNZXNzYWdlczo6TGF5ZXJUcmVlQ29vcmRpbmF0b3JQcm94eTo6RGVsZXRlQ29tcG9zaXRpbmdM YXllcigoKml0KSkpOworICAgICAgICBtX2xheWVyc1RvRGVsZXRlLmNsZWFyKCk7CisgICAgfQor CiAgICAgaWYgKG1fc2hvdWxkU3luY0ZyYW1lKSB7CiAgICAgICAgIGRpZFN5bmMgPSB0cnVlOwog ICAgICAgICBtX3dlYlBhZ2UtPnNlbmQoTWVzc2FnZXM6OkxheWVyVHJlZUNvb3JkaW5hdG9yUHJv eHk6OkRpZFJlbmRlckZyYW1lKCkpOwpAQCAtMzI1LDcgKzMzMSw3IEBAIHZvaWQgTGF5ZXJUcmVl Q29vcmRpbmF0b3I6OmRldGFjaExheWVyKENvb3JkaW5hdGVkR3JhcGhpY3NMYXllciogbGF5ZXIp CiB7CiAgICAgbV9yZWdpc3RlcmVkTGF5ZXJzLnJlbW92ZShsYXllcik7CiAgICAgbV9zaG91bGRT eW5jRnJhbWUgPSB0cnVlOwotICAgIG1fd2ViUGFnZS0+c2VuZChNZXNzYWdlczo6TGF5ZXJUcmVl Q29vcmRpbmF0b3JQcm94eTo6RGVsZXRlQ29tcG9zaXRpbmdMYXllcihsYXllci0+aWQoKSkpOwor ICAgIG1fbGF5ZXJzVG9EZWxldGUuYXBwZW5kKGxheWVyLT5pZCgpKTsKIH0KIAogc3RhdGljIHZv aWQgdXBkYXRlT2Zmc2V0RnJvbVZpZXdwb3J0Rm9yU2VsZihSZW5kZXJMYXllciogcmVuZGVyTGF5 ZXIpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvQ29vcmRp bmF0ZWRHcmFwaGljcy9MYXllclRyZWVDb29yZGluYXRvci5oIGIvU291cmNlL1dlYktpdDIvV2Vi UHJvY2Vzcy9XZWJQYWdlL0Nvb3JkaW5hdGVkR3JhcGhpY3MvTGF5ZXJUcmVlQ29vcmRpbmF0b3Iu aAppbmRleCBjOGVmNTRhNjg2YWM4YWUwZjA4M2VjMjFhYWZjMDQzOGI0ZmMzMDA1Li5jYmMyN2Uz Yjc2NzUwYmUyNDMzNTg4ZTI2NTk5YWE4YzJiMDMwZDYzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi S2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvQ29vcmRpbmF0ZWRHcmFwaGljcy9MYXllclRyZWVDb29y ZGluYXRvci5oCisrKyBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9Db29yZGlu YXRlZEdyYXBoaWNzL0xheWVyVHJlZUNvb3JkaW5hdG9yLmgKQEAgLTEyNCw2ICsxMjQsNyBAQCBw cml2YXRlOgogICAgIEhhc2hTZXQ8V2ViQ29yZTo6Q29vcmRpbmF0ZWRHcmFwaGljc0xheWVyKj4g bV9yZWdpc3RlcmVkTGF5ZXJzOwogICAgIEhhc2hNYXA8aW50NjRfdCwgaW50PiBtX2RpcmVjdGx5 Q29tcG9zaXRlZEltYWdlUmVmQ291bnRzOwogICAgIFZlY3RvcjxPd25QdHI8VXBkYXRlQXRsYXM+ ID4gbV91cGRhdGVBdGxhc2VzOworICAgIFZlY3RvcjxXZWJLaXQ6OldlYkxheWVySUQ+IG1fbGF5 ZXJzVG9EZWxldGU7CiAKICAgICBib29sIG1fbm90aWZ5QWZ0ZXJTY2hlZHVsZWRMYXllckZsdXNo OwogICAgIGJvb2wgbV9pc1ZhbGlkOwo=