Bug 142877 – [GTK] Disable accelerated compositing on Wayland

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK] Disable accelerated compositing on Wayland

[Zan Dobersek]

Created attachment 249056 [details]
Patch

[Martin Robinson]

Comment on attachment 249056 [details]
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?

[WebKit Commit Bot]

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

[Carlos Garcia Campos]

Comment on attachment 249056 [details]
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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

(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 :-)

[Zan Dobersek]

Created attachment 249101 [details]
Patch

Doesn't yet deploy USE(REDIRECTED_XCOMPOSITE_WINDOW).

[Carlos Garcia Campos]

Comment on attachment 249101 [details]
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.

[Zan Dobersek]

Committed r181798: <http://trac.webkit.org/changeset/181798>