142877 2015-03-19 13:16:49 -0700 [GTK] Disable accelerated compositing on Wayland 2019-02-23 05:42:16 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 zan zan bakalaka675 berto cgarcia commit-queue gustavo mrobinson oldest_to_newest 1078604 0 zan 2015-03-19 13:16:49 -0700 [GTK] Disable accelerated compositing on Wayland 1078607 1 249056 zan 2015-03-19 13:19:35 -0700 Created attachment 249056 Patch 1078610 2 249056 mrobinson 2015-03-19 13:21:50 -0700 Comment on attachment 249056 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249056&action=review Looks good. One tiny question... > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:66 > -#if defined(GDK_WINDOWING_X11) > +#if PLATFORM(X11) > #include <gdk/gdkx.h> > #endif > +#if PLATFORM(WAYLAND) > +#include <gdk/gdkwayland.h> > +#endif Can these move past the end of the first include block? 1078611 3 commit-queue 2015-03-19 13:22:40 -0700 Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API 1078772 4 249056 cgarcia 2015-03-20 00:27:46 -0700 Comment on attachment 249056 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249056&action=review I think my patch in bug #142865 already does this, by only enabling the redirected xcomposite window for X11 target. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:63 > -#if defined(GDK_WINDOWING_X11) > +#if PLATFORM(X11) > #include <gdk/gdkx.h> > #endif I think this is only needed by the redireted xcomposite window, so we can make it more obvious by including it only when redirected window is enabled (assuming my patch lands at some point), since it's possible to disable it even for X11 platform. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:66 >> +#if PLATFORM(WAYLAND) >> +#include <gdk/gdkwayland.h> >> +#endif > > Can these move past the end of the first include block? What's gdkwayland.h needed for? > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:342 > +#if PLATFORM(WAYLAND) > + if (GDK_IS_WAYLAND_DISPLAY(display)) > + priv->pageProxy->preferences().setAcceleratedCompositingEnabled(false); > +#endif > +#endif Ah, I see now why gdkwayland was needed. I think we should make this more generic, since it should be possible to have AC disabled for X11 as well, if opengl is not found, for example, no? Would it be possible to do this unconditionally when TEXTURE_MAPPER_GL is disabled? And then we would disable it at configure time for wayland. If we have to keep this here, I would add a comment explaining that we disable AC in wayland because it's not yet supported, with a FIXME and a bug url. 1078808 5 249056 zan 2015-03-20 02:18:05 -0700 Comment on attachment 249056 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249056&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:342 >> +#endif > > Ah, I see now why gdkwayland was needed. I think we should make this more generic, since it should be possible to have AC disabled for X11 as well, if opengl is not found, for example, no? Would it be possible to do this unconditionally when TEXTURE_MAPPER_GL is disabled? And then we would disable it at configure time for wayland. If we have to keep this here, I would add a comment explaining that we disable AC in wayland because it's not yet supported, with a FIXME and a bug url. It's possible to end up with a configuration on X11 that disables the texture mapper if OpenGL isn't found. I think it shouldn't be possible as it's about time we make hardware acceleration a hard dependency. And I don't think anybody is hitting this problem at all, since AC is enabled by default and not dependent on TextureMapper, meaning we would enter AC mode with nothing to actually render the AC output, which would result in a blank screen (much like what's happening under Wayland at the moment -- see bug #142334). Disabling TextureMapper might not work (I don't know of anyone that is covering this), and is just one more configuration option that should be removed ASAP, in parallel with switching to hardware acceleration as a hard dep. On top of that AC doesn't depend on TextureMapper being enabled. That's why I prefer disabling AC under Wayland at runtime. It would be technically correct to do this outside of the USE(TEXTURE_MAPPER_GL) guards, but like I said, these are pretty much always enabled. 1078810 6 cgarcia 2015-03-20 02:34:50 -0700 (In reply to comment #5) > Comment on attachment 249056 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249056&action=review > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:342 > >> +#endif > > > > Ah, I see now why gdkwayland was needed. I think we should make this more generic, since it should be possible to have AC disabled for X11 as well, if opengl is not found, for example, no? Would it be possible to do this unconditionally when TEXTURE_MAPPER_GL is disabled? And then we would disable it at configure time for wayland. If we have to keep this here, I would add a comment explaining that we disable AC in wayland because it's not yet supported, with a FIXME and a bug url. > > It's possible to end up with a configuration on X11 that disables the > texture mapper if OpenGL isn't found. I think it shouldn't be possible as > it's about time we make hardware acceleration a hard dependency. And I don't > think anybody is hitting this problem at all, since AC is enabled by default > and not dependent on TextureMapper, meaning we would enter AC mode with > nothing to actually render the AC output, which would result in a blank > screen (much like what's happening under Wayland at the moment -- see bug > #142334). > > Disabling TextureMapper might not work (I don't know of anyone that is > covering this), and is just one more configuration option that should be > removed ASAP, in parallel with switching to hardware acceleration as a hard > dep. On top of that AC doesn't depend on TextureMapper being enabled. That's > why I prefer disabling AC under Wayland at runtime. It would be technically > correct to do this outside of the USE(TEXTURE_MAPPER_GL) guards, but like I > said, these are pretty much always enabled. Ok, let's add the comment then :-) 1078816 7 249101 zan 2015-03-20 02:50:32 -0700 Created attachment 249101 Patch Doesn't yet deploy USE(REDIRECTED_XCOMPOSITE_WINDOW). 1078821 8 249101 cgarcia 2015-03-20 03:13:08 -0700 Comment on attachment 249101 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249101&action=review Sure, I'll update my patch to use USE and apply on top of this one. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:345 > +#if PLATFORM(WAYLAND) > + // FIXME: Accelerated compositing under Wayland is not yet supported. > + // https://bugs.webkit.org/show_bug.cgi?id=115803 > + if (GDK_IS_WAYLAND_DISPLAY(display)) > + priv->pageProxy->preferences().setAcceleratedCompositingEnabled(false); > +#endif > +#endif We don't need to wait until the web view is realized to disable AC since we only need the display. In fact this can be problematic, because at this point the web process can be in AC mode already. Please move this to webkitWebViewBaseCreateWebPage, setting this to false in the initial settings we pass to the web process when it's created. That way we also avoid sending a message only for this. I'm sorry that I didn't notice this in my previous review. 1078823 9 zan 2015-03-20 03:38:58 -0700 Committed r181798: <http://trac.webkit.org/changeset/181798> 249056 2015-03-19 13:19:35 -0700 2015-03-20 02:50:24 -0700 Patch bug-142877-20150319132211.patch text/plain 2172 zan SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4MTc1NCkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE1LTAzLTE5ICBaYW4gRG9i ZXJzZWsgIDx6ZG9iZXJzZWtAaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXSBEaXNhYmxlIGFj Y2VsZXJhdGVkIGNvbXBvc2l0aW5nIG9uIFdheWxhbmQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0Mjg3NworCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgICogVUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0V2ViVmll d0Jhc2UuY3BwOgorICAgICAgICAod2Via2l0V2ViVmlld0Jhc2VSZWFsaXplKTogQXMgd2FzIGRv bmUgaW4gdGhlIHBhc3QsIHdlIHNob3VsZCBkaXNhYmxlCisgICAgICAgIGFjY2VsZXJhdGVkIGNv bXBvc2l0aW5nIG9uIFdheWxhbmQgdW50aWwgdGhlIHByb3BlciBzdXBwb3J0IGZvciBpdCBsYW5k cy4KKwogMjAxNS0wMy0xOSAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgogCiAgICAg ICAgIENsZWFuIHVwIERPTVRpbWVyIHJlbGF0ZWQgc2V0dGluZ3MKSW5kZXg6IFNvdXJjZS9XZWJL aXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRXZWJWaWV3QmFzZS5jcHAJ KHJldmlzaW9uIDE4MTc1NCkKKysrIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dl YktpdFdlYlZpZXdCYXNlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTgsOSArNTgsMTIgQEAKICNp bmNsdWRlIDxXZWJDb3JlL1JlZ2lvbi5oPgogI2luY2x1ZGUgPGdkay9nZGsuaD4KICNpbmNsdWRl IDxnZGsvZ2Rra2V5c3ltcy5oPgotI2lmIGRlZmluZWQoR0RLX1dJTkRPV0lOR19YMTEpCisjaWYg UExBVEZPUk0oWDExKQogI2luY2x1ZGUgPGdkay9nZGt4Lmg+CiAjZW5kaWYKKyNpZiBQTEFURk9S TShXQVlMQU5EKQorI2luY2x1ZGUgPGdkay9nZGt3YXlsYW5kLmg+CisjZW5kaWYKICNpbmNsdWRl IDxtZW1vcnk+CiAjaW5jbHVkZSA8d3RmL0hhc2hNYXAuaD4KICNpbmNsdWRlIDx3dGYvZ29iamVj dC9HUmVmUHRyLmg+CkBAIC0zMTQsOCArMzE3LDkgQEAgc3RhdGljIHZvaWQgd2Via2l0V2ViVmll d0Jhc2VSZWFsaXplKEd0awogICAgIFdlYktpdFdlYlZpZXdCYXNlKiB3ZWJWaWV3ID0gV0VCS0lU X1dFQl9WSUVXX0JBU0Uod2lkZ2V0KTsKICAgICBXZWJLaXRXZWJWaWV3QmFzZVByaXZhdGUqIHBy aXYgPSB3ZWJWaWV3LT5wcml2OwogCi0jaWYgVVNFKFRFWFRVUkVfTUFQUEVSX0dMKSAmJiBQTEFU Rk9STShYMTEpCisjaWYgVVNFKFRFWFRVUkVfTUFQUEVSX0dMKQogICAgIEdka0Rpc3BsYXkqIGRp c3BsYXkgPSBnZGtfZGlzcGxheV9tYW5hZ2VyX2dldF9kZWZhdWx0X2Rpc3BsYXkoZ2RrX2Rpc3Bs YXlfbWFuYWdlcl9nZXQoKSk7CisjaWYgUExBVEZPUk0oWDExKQogICAgIGlmIChHREtfSVNfWDEx X0RJU1BMQVkoZGlzcGxheSkpIHsKICAgICAgICAgcHJpdi0+cmVkaXJlY3RlZFdpbmRvdyA9IFJl ZGlyZWN0ZWRYQ29tcG9zaXRlV2luZG93OjpjcmVhdGUoCiAgICAgICAgICAgICBndGtfd2lkZ2V0 X2dldF9wYXJlbnRfd2luZG93KHdpZGdldCksCkBAIC0zMzEsNiArMzM1LDEyIEBAIHN0YXRpYyB2 b2lkIHdlYmtpdFdlYlZpZXdCYXNlUmVhbGl6ZShHdGsKICAgICB9CiAjZW5kaWYKIAorI2lmIFBM QVRGT1JNKFdBWUxBTkQpCisgICAgaWYgKEdES19JU19XQVlMQU5EX0RJU1BMQVkoZGlzcGxheSkp CisgICAgICAgIHByaXYtPnBhZ2VQcm94eS0+cHJlZmVyZW5jZXMoKS5zZXRBY2NlbGVyYXRlZENv bXBvc2l0aW5nRW5hYmxlZChmYWxzZSk7CisjZW5kaWYKKyNlbmRpZgorCiAgICAgZ3RrX3dpZGdl dF9zZXRfcmVhbGl6ZWQod2lkZ2V0LCBUUlVFKTsKIAogICAgIEd0a0FsbG9jYXRpb24gYWxsb2Nh dGlvbjsK 249101 2015-03-20 02:50:32 -0700 2015-03-20 03:13:08 -0700 Patch bug-142877-20150320025308.patch text/plain 2673 zan SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4MTc5NSkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE1LTAzLTIwICBaYW4gRG9i ZXJzZWsgIDx6ZG9iZXJzZWtAaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXSBEaXNhYmxlIGFj Y2VsZXJhdGVkIGNvbXBvc2l0aW5nIG9uIFdheWxhbmQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0Mjg3NworCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgICogVUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0V2ViVmll d0Jhc2UuY3BwOgorICAgICAgICAod2Via2l0V2ViVmlld0Jhc2VSZWFsaXplKTogQXMgd2FzIGRv bmUgaW4gdGhlIHBhc3QsIHdlIHNob3VsZCBkaXNhYmxlCisgICAgICAgIGFjY2VsZXJhdGVkIGNv bXBvc2l0aW5nIG9uIFdheWxhbmQgdW50aWwgdGhlIHByb3BlciBzdXBwb3J0IGZvciBpdCBsYW5k cy4KKwogMjAxNS0wMy0xOSAgQW5keSBFc3RlcyAgPGFlc3Rlc0BhcHBsZS5jb20+CiAKICAgICAg ICAgW0NvbnRlbnQgRmlsdGVyaW5nXSBBZGQgdGVzdHMgZm9yIHVuYmxvY2sgcmVxdWVzdHMKSW5k ZXg6IFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRX ZWJWaWV3QmFzZS5jcHAJKHJldmlzaW9uIDE4MTc5NSkKKysrIFNvdXJjZS9XZWJLaXQyL1VJUHJv Y2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXdCYXNlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTgs OSArNTgsNiBAQAogI2luY2x1ZGUgPFdlYkNvcmUvUmVnaW9uLmg+CiAjaW5jbHVkZSA8Z2RrL2dk ay5oPgogI2luY2x1ZGUgPGdkay9nZGtrZXlzeW1zLmg+Ci0jaWYgZGVmaW5lZChHREtfV0lORE9X SU5HX1gxMSkKLSNpbmNsdWRlIDxnZGsvZ2RreC5oPgotI2VuZGlmCiAjaW5jbHVkZSA8bWVtb3J5 PgogI2luY2x1ZGUgPHd0Zi9IYXNoTWFwLmg+CiAjaW5jbHVkZSA8d3RmL2dvYmplY3QvR1JlZlB0 ci5oPgpAQCAtNzAsNiArNjcsMTMgQEAKICNpbmNsdWRlICJXZWJGdWxsU2NyZWVuTWFuYWdlclBy b3h5LmgiCiAjZW5kaWYKIAorI2lmIFBMQVRGT1JNKFgxMSkKKyNpbmNsdWRlIDxnZGsvZ2RreC5o PgorI2VuZGlmCisjaWYgUExBVEZPUk0oV0FZTEFORCkKKyNpbmNsdWRlIDxnZGsvZ2Rrd2F5bGFu ZC5oPgorI2VuZGlmCisKIC8vIGd0a193aWRnZXRfZ2V0X3NjYWxlX2ZhY3RvcigpIGFwcGVhcmVk IGluIEdUSyAzLjEwLCBidXQgd2UgYWxzbyBuZWVkCiAvLyB0byBtYWtlIHN1cmUgd2UgaGF2ZSBj YWlybyBuZXcgZW5vdWdoIHRvIHN1cHBvcnQgY2Fpcm9fc3VyZmFjZV9zZXRfZGV2aWNlX3NjYWxl CiAjZGVmaW5lIEhBVkVfR1RLX1NDQUxFX0ZBQ1RPUiBIQVZFX0NBSVJPX1NVUkZBQ0VfU0VUX0RF VklDRV9TQ0FMRSAmJiBHVEtfQ0hFQ0tfVkVSU0lPTigzLCAxMCwgMCkKQEAgLTMxNCw4ICszMTgs OSBAQCBzdGF0aWMgdm9pZCB3ZWJraXRXZWJWaWV3QmFzZVJlYWxpemUoR3RrCiAgICAgV2ViS2l0 V2ViVmlld0Jhc2UqIHdlYlZpZXcgPSBXRUJLSVRfV0VCX1ZJRVdfQkFTRSh3aWRnZXQpOwogICAg IFdlYktpdFdlYlZpZXdCYXNlUHJpdmF0ZSogcHJpdiA9IHdlYlZpZXctPnByaXY7CiAKLSNpZiBV U0UoVEVYVFVSRV9NQVBQRVJfR0wpICYmIFBMQVRGT1JNKFgxMSkKKyNpZiBVU0UoVEVYVFVSRV9N QVBQRVJfR0wpCiAgICAgR2RrRGlzcGxheSogZGlzcGxheSA9IGdka19kaXNwbGF5X21hbmFnZXJf Z2V0X2RlZmF1bHRfZGlzcGxheShnZGtfZGlzcGxheV9tYW5hZ2VyX2dldCgpKTsKKyNpZiBQTEFU Rk9STShYMTEpCiAgICAgaWYgKEdES19JU19YMTFfRElTUExBWShkaXNwbGF5KSkgewogICAgICAg ICBwcml2LT5yZWRpcmVjdGVkV2luZG93ID0gUmVkaXJlY3RlZFhDb21wb3NpdGVXaW5kb3c6OmNy ZWF0ZSgKICAgICAgICAgICAgIGd0a193aWRnZXRfZ2V0X3BhcmVudF93aW5kb3cod2lkZ2V0KSwK QEAgLTMzMSw2ICszMzYsMTQgQEAgc3RhdGljIHZvaWQgd2Via2l0V2ViVmlld0Jhc2VSZWFsaXpl KEd0awogICAgIH0KICNlbmRpZgogCisjaWYgUExBVEZPUk0oV0FZTEFORCkKKyAgICAvLyBGSVhN RTogQWNjZWxlcmF0ZWQgY29tcG9zaXRpbmcgdW5kZXIgV2F5bGFuZCBpcyBub3QgeWV0IHN1cHBv cnRlZC4KKyAgICAvLyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE1 ODAzCisgICAgaWYgKEdES19JU19XQVlMQU5EX0RJU1BMQVkoZGlzcGxheSkpCisgICAgICAgIHBy aXYtPnBhZ2VQcm94eS0+cHJlZmVyZW5jZXMoKS5zZXRBY2NlbGVyYXRlZENvbXBvc2l0aW5nRW5h YmxlZChmYWxzZSk7CisjZW5kaWYKKyNlbmRpZgorCiAgICAgZ3RrX3dpZGdldF9zZXRfcmVhbGl6 ZWQod2lkZ2V0LCBUUlVFKTsKIAogICAgIEd0a0FsbG9jYXRpb24gYWxsb2NhdGlvbjsK