Bug 139028 – [GTK] Enable depth 32 for the RedirectedXCompositeWindow

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Julien Isorce]

Created attachment 242160 [details]
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

On gtk/X11, the layout compositing is done in the web process. If one needs to handle alpha with the rest of the application then it is not enough to make to browser's window as RGBA.

The shared redirected window needs to be RGBA as well. (The shared X composite window between UIProcess and WebProcess).

[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

[WebKit Commit Bot]

Attachment 242160 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:439:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Isorce]

Created attachment 242161 [details]
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

[WebKit Commit Bot]

Attachment 242161 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:439:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Isorce]

Created attachment 242162 [details]
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

[Gwang Yoon Hwang]

Comment on attachment 242162 [details]
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

View in context: https://bugs.webkit.org/attachment.cgi?id=242162&action=review

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:135
> +RedirectedXCompositeWindow::RedirectedXCompositeWindow(Display* display, const IntSize& size, std::function<void()> damageNotify, int depth)

Because your patch adds Alpha support to current implementation, it would be good to use "bool supportAlpha" instead of depth.

> Tools/MiniBrowser/gtk/main.c:64
> +    // FIXME: use websettings

I don't think we need to use websettings.
It would be better to modify WebKitWebViewBase to decide whether RGBA support is needed.
ex) If parent window of webview uses RGBA, we can use RGBA support.

[Julien Isorce]

Created attachment 242197 [details]
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

I addressed the remarks ( depth -> "bool supportAlpha" and "If parent window of webview uses RGBA, we can use RGBA support" ) . But to decide if the application is RGBA or not it is up to the application to decide. So I introduced MINIBROWSER_ALPHA env var.

[Carlos Garcia Campos]

Comment on attachment 242197 [details]
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

View in context: https://bugs.webkit.org/attachment.cgi?id=242197&action=review

> Source/WebKit2/ChangeLog:3
> +        gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

Please, use the bug title here and move the bug url, see other changelog entries.

> Source/WebKit2/ChangeLog:12
> +        This allows an end-to-end RGBA solution when the application
> +        wants to interact with the alpha channel at compositing time.

Do you have a test case or is there any website that are not rendering correctly because of this?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:264
> +        GdkVisual* visual = gtk_widget_get_visual(parent);
> +        gint depth = gdk_visual_get_depth(visual);
> +        bool supportAlpha = depth == 32 && (!priv->redirectedWindow || !priv->redirectedWindow->hasAlpha());

I think here you should check if the widget visual is the screen rgba visual.

visual == gdk_screen_get_rgba_visual(gdk_display_get_default_screen(display)));

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:269
> +            if (GDK_IS_X11_DISPLAY(display)) {
> +                priv->redirectedWindow = RedirectedXCompositeWindow::create(GDK_DISPLAY_XDISPLAY(display), IntSize(1, 1), [widget] { gtk_widget_queue_draw(widget); },
> +                    supportAlpha);

Why are we creating this here again? If it's too early in constructed, we can create the redirected window when realized, but not in both places, I would say.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:271
> +                    priv->pageProxy->setAcceleratedCompositingWindowId(priv->redirectedWindow->windowID());

This is also done when the page is created

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:154
> +        XVisualInfo vi_in;

Use WebKit coding style here, visualInfo instead of vi_in

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:159
> +        int nvi = 0;

nvi -> visualInfoCount, for example

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:160
> +        XVisualInfo* xvi = XGetVisualInfo(display, VisualScreenMask | VisualDepthMask | VisualClassMask, &vi_in, &nvi);

xvi -> xVisualInfo

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:161
> +        Visual* vis = 0;

vis -> visual, use nullptr instead of 0

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:164
> +            if ((fmt->type == PictTypeDirect) && (fmt->direct.alphaMask)) {

I don't think you need the extra parentheses

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:203
> +    } else {
> +        windowAttributes.override_redirect = True;
> +        m_parentWindow = XCreateWindow(display,
> +            RootWindowOfScreen(screen),
> +            WidthOfScreen(screen) + 1, 0, 1, 1,
> +            0,
> +            CopyFromParent,
> +            InputOutput,
> +            CopyFromParent,
> +            CWOverrideRedirect,
> +            &windowAttributes);
> +        XMapWindow(display, m_parentWindow);

Do we really need two paths here? or can we set the window attrs, the mask, etc depending on whether alpha is supported or not and then create the window and call XMapWindow using the same code?

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:245
> +    if (m_hasAlpha)
> +        XFreeColormap(m_display, m_colormap);

Could we check m_colormap instead of m_hasAlpha and then we don't need to keep m_hasAlpha at all?

> Tools/ChangeLog:3
> +        gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

Same here about changelog formatting.

> Tools/MiniBrowser/gtk/main.c:69
> +    const gchar *alpha = g_getenv("MINIBROWSER_ALPHA");
> +    if (alpha && *alpha) {
> +        GdkScreen *screen = gtk_widget_get_screen(GTK_WIDGET(mainWindow));
> +        GdkVisual *visual = gdk_screen_get_rgba_visual(screen);
> +        gtk_widget_set_visual(GTK_WIDGET(mainWindow), visual);
> +    }

I don't understand why this is a user decision, doesn't it depend on whether it's supported or not?

[Julien Isorce]

Created attachment 242267 [details]
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

Thx for the review! I addressed remarks.

[Julien Isorce]

Comment on attachment 242197 [details]
gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow

View in context: https://bugs.webkit.org/attachment.cgi?id=242197&action=review

>> Source/WebKit2/ChangeLog:3
>> +        gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow
> 
> Please, use the bug title here and move the bug url, see other changelog entries.

Done

>> Source/WebKit2/ChangeLog:12
>> +        wants to interact with the alpha channel at compositing time.
> 
> Do you have a test case or is there any website that are not rendering correctly because of this?

I do not think this is useful when webkit is used for classic desktop webbrowser (though maybe for :root background=transparent, body: background=transparent). It is more for transparent UI.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:264
>> +        bool supportAlpha = depth == 32 && (!priv->redirectedWindow || !priv->redirectedWindow->hasAlpha());
> 
> I think here you should check if the widget visual is the screen rgba visual.
> 
> visual == gdk_screen_get_rgba_visual(gdk_display_get_default_screen(display)));

It now matches visual and depth from the parent winID in all cases.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:269
>> +                    supportAlpha);
> 
> Why are we creating this here again? If it's too early in constructed, we can create the redirected window when realized, but not in both places, I would say.

Exactly it was to early, i.e. before the parent was created. Ok thx, I now only left the creation when realized.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:271
>> +                    priv->pageProxy->setAcceleratedCompositingWindowId(priv->redirectedWindow->windowID());
> 
> This is also done when the page is created

Done only one time now.

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:203
>> +        XMapWindow(display, m_parentWindow);
> 
> Do we really need two paths here? or can we set the window attrs, the mask, etc depending on whether alpha is supported or not and then create the window and call XMapWindow using the same code?

You are right, I kept the code before my changes except I pass the winID application instead of the root windID. So that depth and visual matches the parent.
A little note is that once the window is created I change the parent again to be the root window. So that no need to take care of destroying the child before the parent. Anyway they do not need to depend on each other.

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:245
>> +        XFreeColormap(m_display, m_colormap);
> 
> Could we check m_colormap instead of m_hasAlpha and then we don't need to keep m_hasAlpha at all?

I removed both.

>> Tools/ChangeLog:3
>> +        gtk: support RGBA end-to-end compositing when using RedirectedXCompositeWindow
> 
> Same here about changelog formatting.

Done

>> Tools/MiniBrowser/gtk/main.c:69
>> +    }
> 
> I don't understand why this is a user decision, doesn't it depend on whether it's supported or not?

You are right, for MiniBrowser let's just set a rgba visual if available.

[Martin Robinson]

Comment on attachment 242267 [details]
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

View in context: https://bugs.webkit.org/attachment.cgi?id=242267&action=review

This looks like a good change, though I have a few questions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:260
> +    if (GtkWidget* parent = gtk_widget_get_parent(widget)) {

Why not fall back if the parent doesn't exist. If the parent always must exist, why not use an assertion instead of a conditional?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1045
> +    bool isEnable = false;
> +
>  #if USE(TEXTURE_MAPPER_GL) && PLATFORM(X11)
> -    if (priv->redirectedWindow)
> -        return;
> +    isEnable = !!priv->redirectedWindow;
>  #endif
>  
> -    priv->pageProxy->preferences().setAcceleratedCompositingEnabled(false);
> +    priv->pageProxy->preferences().setAcceleratedCompositingEnabled(isEnable);

Doesn't this do the same thing as before? It looks like the default was true? Regardless,

isEnabled should be called acceleratedCompositingEnabled and I do not believe you need the double !.

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:162
> +    // The top parent is only necessary to inherit visual and depth at creation time.
> +    if (parent)
> +        XReparentWindow(display, m_parentWindow, RootWindowOfScreen(screen), WidthOfScreen(screen) + 1, 0);

Would it be possible to pass the visual and depth from the parent instead of inheriting?

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:173
> +        CWEventMask | CWOverrideRedirect,

Why is this change necessary? You should include this information in the ChangeLog.

> Tools/MiniBrowser/gtk/main.c:69
> +    // Prefer RGBA visual if available.
> +    GdkScreen *screen = gtk_widget_get_screen(GTK_WIDGET(mainWindow));
> +    GdkVisual *visual = gdk_screen_get_rgba_visual(screen);
> +    if (visual)
> +        gtk_widget_set_visual(GTK_WIDGET(mainWindow), visual);
> +

Does this make the WebView transparent with a transparent HTML background. I'm not sure if we want that behavior in the MiniBrowser.

[Julien Isorce]

Comment on attachment 242267 [details]
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

Thx for the third review. I'll address the changes in the next patch.


View in context: https://bugs.webkit.org/attachment.cgi?id=242267&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:260
>> +    if (GtkWidget* parent = gtk_widget_get_parent(widget)) {
> 
> Why not fall back if the parent doesn't exist. If the parent always must exist, why not use an assertion instead of a conditional?

Good point. Indeed it should fallback to passing 0 to RedirectedXCompositeWindow::create. I'll address the change.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1045
>> +    priv->pageProxy->preferences().setAcceleratedCompositingEnabled(isEnable);
> 
> Doesn't this do the same thing as before? It looks like the default was true? Regardless,
> 
> isEnabled should be called acceleratedCompositingEnabled and I do not believe you need the double !.

The default is false but it gets enable if GL and redirectedWindow exists.
Without the double !, it fails to build with ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1042:14: error: cannot convert 'std::unique_ptr<WebKit::RedirectedXCompositeWindow>' to 'bool' in assignment
I'll rewrite this part more clearly.

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:162
>> +        XReparentWindow(display, m_parentWindow, RootWindowOfScreen(screen), WidthOfScreen(screen) + 1, 0);
> 
> Would it be possible to pass the visual and depth from the parent instead of inheriting?

Ok I'll have a try and let you know in the next patch.

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:173
>> +        CWEventMask | CWOverrideRedirect,
> 
> Why is this change necessary? You should include this information in the ChangeLog.

Indeed this is not necessary. This change should go in a separate patch. It is not necessary for my patch to work. Was just a consistency change.

>> Tools/MiniBrowser/gtk/main.c:69
>> +
> 
> Does this make the WebView transparent with a transparent HTML background. I'm not sure if we want that behavior in the MiniBrowser.

With only the patch, the background does not get transparent if the html page contains "body { background-color: transparent; }".
Indeed to have a transparent background it requires more changes in Minibrowser, like using gtk_widget_override_background_color(...transparent...).
So I'll let this change.

[Julien Isorce]

I add a try and now I remember why I choosed to pass the parent's Window instead of just the visual and depth retrived with gdk_x11_visual_get_xvisual/gdk_visual_get_depth. It is because then you have to deal with the colormap (see initial patch). And it is really anoying to handle it and make the code more complex and also it will probably not handle all corners cases.

So it is cleaner and safer to copy visual and depth from parent (the wording inheriting is a bit of a strech so I'll change the comment). And then reparent it to the root.

[Julien Isorce]

Created attachment 242914 [details]
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

I addressed the remark from the third review. (Also please see the 2 previous comments for answers)

[Martin Robinson]

Comment on attachment 242914 [details]
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

View in context: https://bugs.webkit.org/attachment.cgi?id=242914&action=review

Looks good to me, apart from a few nits.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:264
> +            [widget] { gtk_widget_queue_draw(widget); }

Please split this over multiple lines:
priv->redirectedWindow = RedirectedXCompositeWindow::create(
     GDK_DISPLAY_XDISPLAY(display),
     parentWindow ? GDK_WINDOW_XID(parentWindow) : 0,
     [widget] {
         gtk_widget_queue_draw(widget);
     });

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1041
> +

Extra line here.

> Tools/MiniBrowser/gtk/main.c:69
> +    // Prefer RGBA visual if available.
> +    GdkScreen *screen = gtk_widget_get_screen(GTK_WIDGET(mainWindow));
> +    GdkVisual *visual = gdk_screen_get_rgba_visual(screen);
> +    if (visual)
> +        gtk_widget_set_visual(GTK_WIDGET(mainWindow), visual);
> +

So if this doesn't change the behavior of MiniBrowser, why is it necessary?

[Julien Isorce]

Created attachment 243033 [details]
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

I addressed indentation changes, blank line and removed changes in Minibrowser as we do not want it to use RGBA by default (even if visually it does not change anything. Indeed to have a webui transparent you need more steps in the browser which I mentioned in previous comments)

[Martin Robinson]

Comment on attachment 243033 [details]
[GTK] Enable depth 32 for the RedirectedXCompositeWindow

View in context: https://bugs.webkit.org/attachment.cgi?id=243033&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:269
> +                gtk_widget_queue_draw(widget); }
> +            );

Missing a newline after the ; here, but I will fix it and land this.

[Julien Isorce]

Ah right I see :) Thx!

[Martin Robinson]

Committed r177075: <http://trac.webkit.org/changeset/177075>