161683 2016-09-07 02:11:48 -0700 [GTK] Clarify frame callbacks behaviour in Wayland compositor 2016-09-07 08:56:41 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Local Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 emanuele.aina webkit-unassigned bugs-noreply cgarcia commit-queue mcatanzaro oldest_to_newest 1227117 0 emanuele.aina 2016-09-07 02:11:48 -0700 The way we fire frame callbacks in the nested Wayland compositor can be puzzling to developers expecting Wayland semantics, but since we have our own mechanism to handle synchronization we don't care much about them. Adding a comment would avoid surprised Wayland developers. 1227118 1 288124 emanuele.aina 2016-09-07 02:12:50 -0700 Created attachment 288124 Patch 1227124 2 288124 cgarcia 2016-09-07 02:33:03 -0700 Comment on attachment 288124 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288124&action=review > w/Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:237 > + // From a Wayland point-of-view firing frame callbacks here is very weird, > + // but our WebProcess clients don't rely on them for synchronization and > + // set the EGL swap interval to zero, relying on the RenderNextFrame events > + // in the WebKit IPC instead The comment adds more confusion to me. It says that the firing frame callbacks here is weird, but it doesn't say why or where they should be fired. The only reason why I didn't remove all frame callbacks handling from the nested compositor is because I was not sure we can rely that eglSwapInterval is always going to work for all drivers. But if it works, frame callbacks are never emitted, so this list is always empty. The reason why we do this, is not because we rely on RenderNextFrame events, adn there isn't any WebKit IPC involved. The reason is that we are rendering to an offscreen context always in the web process, and when we have to render a new frame, we schedule a redraw in the web view, and then we render in the screen. So syncing to vblank in the web process doesn't help, because we will not render at that time to the screen, we will render into the offscreen context, and the schedule a redraw on the widget. It's at that point where we want to vblank sync, and that's already done by GTK+. This avoids unnecessary throttling in the web process. When there's a new frame rendered in the web process, eglSwapBuffers is what triggers the repaint in the nested compositor using the wayland protocol, not the WebKit IPC. 1227129 3 emanuele.aina 2016-09-07 02:48:03 -0700 Frame callbacks should be fired where you know the compositor has used the committed content. E.g. in Weston it the place where it has queued the GL commands to update the screen and so the next screen update contents have been locked in place. To be specific, it should not just be any screen update, it should be the particular update that first uses the content from the same commit as where the frame callback was filed. Frame callbacks should also not be fired if the surface is not shown. What I was trying to say is that despite frame callbacks being the main throttling mechanism in normal Wayland usage, we don't need them for that purpose as throttling is already handled elsewhere by WebKit. 1227148 4 288131 emanuele.aina 2016-09-07 06:20:13 -0700 Created attachment 288131 Patch 1227151 5 288131 cgarcia 2016-09-07 06:37:33 -0700 Comment on attachment 288131 Patch Much clearer, thanks! 1227181 6 288131 commit-queue 2016-09-07 08:56:38 -0700 Comment on attachment 288131 Patch Clearing flags on attachment: 288131 Committed r205547: <http://trac.webkit.org/changeset/205547> 1227182 7 commit-queue 2016-09-07 08:56:41 -0700 All reviewed patches have been landed. Closing bug. 288124 2016-09-07 02:12:50 -0700 2016-09-07 06:20:04 -0700 Patch bug-161683-20160907111025.patch text/plain 1929 emanuele.aina U3VidmVyc2lvbiBSZXZpc2lvbjogMjA1NDcyCmRpZmYgLS1naXQgYy9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgdy9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggYTM4ODhmZTg4NWYzNmI2 MDg5YzAzZmRiMTNlNWYzOTQwZjdmODA3NC4uZWMwOTFhYmY4MTI0NDBlZDBhYWFhOThmOWIxZDQ5 Zjc0OTE1YTFlZCAxMDA2NDQKLS0tIGMvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyB3L1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE2LTA5LTA2ICBFbWFu dWVsZSBBaW5hICA8ZW1hbnVlbGUuYWluYUBjb2xsYWJvcmEuY29tPgorCisgICAgICAgIFtHVEtd IENsYXJpZnkgZnJhbWUgY2FsbGJhY2tzIGJlaGF2aW91ciBpbiBXYXlsYW5kIGNvbXBvc2l0b3IK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2MTY4Mwor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSB3YXkg d2UgZmlyZSBmcmFtZSBjYWxsYmFja3MgaW4gdGhlIG5lc3RlZCBXYXlsYW5kIGNvbXBvc2l0b3Ig Y2FuCisgICAgICAgIGJlIHB1enpsaW5nIHRvIGRldmVsb3BlcnMgZXhwZWN0aW5nIFdheWxhbmQg c2VtYW50aWNzLCBidXQgc2luY2Ugd2UKKyAgICAgICAgaGF2ZSBvdXIgb3duIG1lY2hhbmlzbSB0 byBoYW5kbGUgc3luY2hyb25pemF0aW9uIHdlIGRvbid0IGNhcmUgbXVjaAorICAgICAgICBhYm91 dCB0aGVtLiBBZGQgYSBjb21tZW50IHRvIGF2b2lkIHN1cnByaXNlZCBXYXlsYW5kIGRldmVsb3Bl cnMuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvZ3RrL1dheWxhbmRDb21wb3NpdG9yLmNwcDoKKyAg ICAgICAgKFdlYktpdDo6V2F5bGFuZENvbXBvc2l0b3I6OlN1cmZhY2U6OmNvbW1pdCk6CisKIDIw MTYtMDktMDUgIFphbiBEb2JlcnNlayAgPHpkb2JlcnNla0BpZ2FsaWEuY29tPgogCiAgICAgICAg IEZpeCBFTkFCTEUoR0FNRVBBRCkgYnVpbGQgZXJyb3JzIG9uIG5vbi1Db2NvYSBwbGF0Zm9ybXMK ZGlmZiAtLWdpdCBjL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9ndGsvV2F5bGFuZENvbXBvc2l0 b3IuY3BwIHcvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2d0ay9XYXlsYW5kQ29tcG9zaXRvci5j cHAKaW5kZXggNGFhZmMxMWVhYWQ5ZmViZjQxZmJmODAwYjk3NTJkN2EzYzRlN2U0MC4uNTM0ZjIx MWUzNjUxNDI5MzNhNTk5ZGU1MDVlNmQxMDQxNmQ3YTI0NCAxMDA2NDQKLS0tIGMvU291cmNlL1dl YktpdDIvVUlQcm9jZXNzL2d0ay9XYXlsYW5kQ29tcG9zaXRvci5jcHAKKysrIHcvU291cmNlL1dl YktpdDIvVUlQcm9jZXNzL2d0ay9XYXlsYW5kQ29tcG9zaXRvci5jcHAKQEAgLTIzMSw2ICsyMzEs MTAgQEAgYm9vbCBXYXlsYW5kQ29tcG9zaXRvcjo6U3VyZmFjZTo6Y29tbWl0KCkKICAgICBpZiAo bV93ZWJQYWdlKQogICAgICAgICBtX3dlYlBhZ2UtPnNldFZpZXdOZWVkc0Rpc3BsYXkoSW50UmVj dChJbnRQb2ludDo6emVybygpLCBtX3dlYlBhZ2UtPnZpZXdTaXplKCkpKTsKIAorICAgIC8vIEZy b20gYSBXYXlsYW5kIHBvaW50LW9mLXZpZXcgZmlyaW5nIGZyYW1lIGNhbGxiYWNrcyBoZXJlIGlz IHZlcnkgd2VpcmQsCisgICAgLy8gYnV0IG91ciBXZWJQcm9jZXNzIGNsaWVudHMgZG9uJ3QgcmVs eSBvbiB0aGVtIGZvciBzeW5jaHJvbml6YXRpb24gYW5kCisgICAgLy8gc2V0IHRoZSBFR0wgc3dh cCBpbnRlcnZhbCB0byB6ZXJvLCByZWx5aW5nIG9uIHRoZSBSZW5kZXJOZXh0RnJhbWUgZXZlbnRz CisgICAgLy8gaW4gdGhlIFdlYktpdCBJUEMgaW5zdGVhZAogICAgIGF1dG8gbGlzdCA9IFdURk1v dmUobV9mcmFtZUNhbGxiYWNrTGlzdCk7CiAgICAgZm9yIChhdXRvKiByZXNvdXJjZSA6IGxpc3Qp IHsKICAgICAgICAgd2xfY2FsbGJhY2tfc2VuZF9kb25lKHJlc291cmNlLCAwKTsK 288131 2016-09-07 06:20:13 -0700 2016-09-07 08:56:38 -0700 Patch bug-161683-20160907151748.patch text/plain 1964 emanuele.aina U3VidmVyc2lvbiBSZXZpc2lvbjogMjA1NTM5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMGViMTc4YjMzNjc0YjBi YzUwZmVmNmNlMzQ2MWMxOWJlYjA2MjQ3Ni4uM2MwZWJjNzlhNmMwODk3MTRmZGNmZDBjOTZlOGRi ZTI4MjU3ZjhhOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE2LTA5LTA2ICBFbWFu dWVsZSBBaW5hICA8ZW1hbnVlbGUuYWluYUBjb2xsYWJvcmEuY29tPgorCisgICAgICAgIFtHVEtd IENsYXJpZnkgZnJhbWUgY2FsbGJhY2tzIGJlaGF2aW91ciBpbiBXYXlsYW5kIGNvbXBvc2l0b3IK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2MTY4Mwor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSB3YXkg d2UgZmlyZSBmcmFtZSBjYWxsYmFja3MgaW4gdGhlIG5lc3RlZCBXYXlsYW5kIGNvbXBvc2l0b3Ig Y2FuCisgICAgICAgIGJlIHB1enpsaW5nIHRvIGRldmVsb3BlcnMgZXhwZWN0aW5nIFdheWxhbmQg c2VtYW50aWNzLCBidXQgc2luY2Ugd2UKKyAgICAgICAgaGF2ZSBvdXIgb3duIG1lY2hhbmlzbSB0 byBoYW5kbGUgc3luY2hyb25pemF0aW9uIHdlIGRvbid0IGNhcmUgbXVjaAorICAgICAgICBhYm91 dCB0aGVtLiBBZGQgYSBjb21tZW50IHRvIGF2b2lkIHN1cnByaXNlZCBXYXlsYW5kIGRldmVsb3Bl cnMuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvZ3RrL1dheWxhbmRDb21wb3NpdG9yLmNwcDoKKyAg ICAgICAgKFdlYktpdDo6V2F5bGFuZENvbXBvc2l0b3I6OlN1cmZhY2U6OmNvbW1pdCk6CisKIDIw MTYtMDktMDYgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAg ICAgICAgIFtHVEtdW1RocmVhZGVkIENvbXBvc2l0b3JdIFNldmVyYWwgZmxha3kgdGVzdHMKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9ndGsvV2F5bGFuZENvbXBvc2l0b3Iu Y3BwIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2d0ay9XYXlsYW5kQ29tcG9zaXRvci5jcHAK aW5kZXggMjdlNjAyNjBjNGY4NTc5ZDM4YjQ3NWRkNzQ1NWNkMDgzMjljNWIzNS4uNGQ1OTgwNTVk ZDIzNzFmMjk2MTkwM2VhOWZmMmU5ZTAyZDE3OGEzOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktp dDIvVUlQcm9jZXNzL2d0ay9XYXlsYW5kQ29tcG9zaXRvci5jcHAKKysrIGIvU291cmNlL1dlYktp dDIvVUlQcm9jZXNzL2d0ay9XYXlsYW5kQ29tcG9zaXRvci5jcHAKQEAgLTIzMSw2ICsyMzEsMTAg QEAgYm9vbCBXYXlsYW5kQ29tcG9zaXRvcjo6U3VyZmFjZTo6Y29tbWl0KCkKICAgICBpZiAobV93 ZWJQYWdlKQogICAgICAgICBtX3dlYlBhZ2UtPnNldFZpZXdOZWVkc0Rpc3BsYXkoSW50UmVjdChJ bnRQb2ludDo6emVybygpLCBtX3dlYlBhZ2UtPnZpZXdTaXplKCkpKTsKIAorICAgIC8vIEZyb20g YSBXYXlsYW5kIHBvaW50LW9mLXZpZXcgZnJhbWUgY2FsbGJhY2tzIHNob3VsZCBiZSBmaXJlZCB3 aGVyZSB0aGUKKyAgICAvLyBjb21wb3NpdG9yIGtub3dzIGl0IGhhcyAqdXNlZCogdGhlIGNvbW1p dHRlZCBjb250ZW50cywgc28gZmlyaW5nIHRoZW0gaGVyZQorICAgIC8vIGNhbiBiZSBzdXJwcmlz aW5nIGJ1dCB3ZSBkb24ndCBuZWVkIHRoZW0gYXMgYSB0aHJvdHRsaW5nIG1lY2hhbmlzbSBiZWNh dXNlCisgICAgLy8gcmVuZGVyaW5nIHN5bmNocm9uaXphdGlvbiBpcyBoYW5kbGVkIGVsc2V3aGVy ZSBieSBXZWJLaXQuCiAgICAgYXV0byBsaXN0ID0gV1RGTW92ZShtX2ZyYW1lQ2FsbGJhY2tMaXN0 KTsKICAgICBmb3IgKGF1dG8qIHJlc291cmNlIDogbGlzdCkgewogICAgICAgICB3bF9jYWxsYmFj a19zZW5kX2RvbmUocmVzb3VyY2UsIDApOwo=