Closed Bug 1401455 Opened 7 years ago Closed 7 years ago

WebRender wrong rendering on Linux with Nvidia and proprietary drivers

Categories

(Core :: Graphics: WebRender, defect, P3)

Product:
Core
Component:
Graphics: WebRender
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox61 --- fixed

People

(Reporter: noxphasma, Assigned: snorp)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 1 obsolete file)

Rendering Example
608.10 KB, image/png
Details
Bug 1401455 - Expose glxChooseVisual() in GLXLibrary
59 bytes, text/x-review-board-request
Details
Bug 1401455 - Use correct GLX visual when rendering with WebRender
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1401455 - Add GLContextGLX::FindVisual()
59 bytes, text/x-review-board-request
lsalzman
: review+
Details
Stack from asan build
13.59 KB, text/plain
Details
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170919220202 Steps to reproduce: I've enabled WebRender with these about:config options: turn off layers.async-pan-zoom.enabled turn on gfx.webrender.enabled turn on gfx.webrendest.enabled turn on gfx.webrender.layers-free add and turn on gfx.webrender.blob-images if you are on Linux, turn on layers.acceleration.force-enabled Linux Mint 18.2 Nvidia GTX 970 Nvidia proprietary drivers 384.69 Actual results: Webpages don't draw background colors, background images. When I change the Firefox theme or interact with the Firefox UI (open Burger Menu, or Library) while WebRender is enabled, WebRender crashes and about:support reports this: WEBRENDER opt-in by default: WebRender is an opt-in feature available by user: Enabled by pref unavailable by runtime: WebRender initialization failed Fehlerprotokoll (#0) Error Failed GL context creation for WebRender: 0 (#1) Error Compositors might be mixed (5,2) Expected results: Rendering should working correctly, WebRender shouldn't crash.
Component: Untriaged → Graphics: WebRender
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64
Priority: -- → P3
Whiteboard: [gfx-noted]
I can duplicate this on Arch Linux w/ 58a1 Build ID 20171104100412 on OpenGL 4.6.0 Nvidia 387.22 drivers.
status-firefox56: --- → unaffected
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
status-firefox-esr52: --- → unaffected
Version: 57 Branch → Trunk
Can you post a link to a crash report?
Firefox itself doesn't crash, only WebRender crashes and falls back to the OpenGL renderer. No crash report is created. This is the raw data of about:support of my current profile, but the same happens with a fresh profile: https://gist.github.com/NoXPhasma/3d4ad123a406b542265adb2abde43794
How do you know that WebRender is crashing?
When I start Firefox with WebRender enabled, I can see on about:support that WebRender is enabled without any Error(https://gist.github.com/NoXPhasma/c875882ad49aebe21d03eb965a0106ac). Also I get the wrong rendering on websites (including about: pages) as described in the first post. The moment I open the hamburger menu or the library, the renderer falls back to the default and I get the output in about:support as posted above. Then the rendering on all pages is correct, as when WebRender wouldn't be enabled in the first place. Not sure if you would call it a crash, but for me it seems that WebRender crashes.
I've done a record on a fresh profile, so you can see what happens: https://www.youtube.com/watch?v=dmrN6cP4Ep4
See Also: → 1371210
Confirmed on Ubuntu 17.10 with WebGL 1 Driver Renderer: NVIDIA Corporation -- GeForce GTX 750 Ti/PCIe/SSE2 WebGL 1 Driver Version: 4.5.0 NVIDIA 384.90 and the test-case: <html> <body style="background-color: blue;"> <div style="background-color: red; height: 5000px; width: 100px;">hello</div> </body> </html> I only see "Hello" and no blue and red backgrounds. Missing background-color is just one of the issues. For example, there's no cursor caret in the location bar. However, the graphical issues are fixed by following these steps: 1) Enable general.autoScroll. 2) Press the middle mouse button to start autoscroll. At this point all tabs render background-color correctly. I started firefox from a shell, so I see this in stdout/stderr: [GFX1-]: Failed GL context creation for WebRender: 0 [GFX1-]: Compositors might be mixed (5,2) If I then quit Firefox, it crashes. I don't have debug symbols, but this is the backtrace I see: #0 0x00007f0df5975d87 in ThreadInfo::~ThreadInfo() [clone .cold.455] () at /media/new_drive/downloads/firefox/libxul.so #1 0x00007f0df6a45863 in profiler_shutdown() () at /media/new_drive/downloads/firefox/libxul.so #2 0x00007f0df6a996f1 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /media/new_drive/downloads/firefox/libxul.so #3 0x00007f0df6a99286 in XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /media/new_drive/downloads/firefox/libxul.so #4 0x0000000000421dc9 in do_main(int, char**, char**) () #5 0x00000000004159ef in main ()
(In reply to Ori Avtalion (salty-horse) from comment #7) > Confirmed on Ubuntu 17.10 with > WebGL 1 Driver Renderer: NVIDIA Corporation -- GeForce GTX 750 Ti/PCIe/SSE2 > WebGL 1 Driver Version: 4.5.0 NVIDIA 384.90 > > and the test-case: > <html> > <body style="background-color: blue;"> > <div style="background-color: red; height: 5000px; width: > 100px;">hello</div> > </body> > </html> > > I only see "Hello" and no blue and red backgrounds. > > Missing background-color is just one of the issues. For example, there's no > cursor caret in the location bar. > > However, the graphical issues are fixed by following these steps: > 1) Enable general.autoScroll. > 2) Press the middle mouse button to start autoscroll. > > At this point all tabs render background-color correctly. > I started firefox from a shell, so I see this in stdout/stderr: > [GFX1-]: Failed GL context creation for WebRender: 0 > [GFX1-]: Compositors might be mixed (5,2) > > If I then quit Firefox, it crashes. I don't have debug symbols, but this is > the backtrace I see: > #0 0x00007f0df5975d87 in ThreadInfo::~ThreadInfo() [clone .cold.455] () > at /media/new_drive/downloads/firefox/libxul.so > #1 0x00007f0df6a45863 in profiler_shutdown() () at > /media/new_drive/downloads/firefox/libxul.so > #2 0x00007f0df6a996f1 in XREMain::XRE_main(int, char**, > mozilla::BootstrapConfig const&) () at > /media/new_drive/downloads/firefox/libxul.so > #3 0x00007f0df6a99286 in XRE_main(int, char**, mozilla::BootstrapConfig > const&) () at /media/new_drive/downloads/firefox/libxul.so > #4 0x0000000000421dc9 in do_main(int, char**, char**) () > #5 0x00000000004159ef in main () After you scroll and the colors are fixed, check about:support. I'd bet you that you that scrolling triggered something that failed you back to OpenGL and it won't be using Webrender anymore.
(In reply to Vash63 from comment #8) > After you scroll and the colors are fixed, check about:support. I'd bet you > that you that scrolling triggered something that failed you back to OpenGL > and it won't be using Webrender anymore. You're right :( So I have nothing to add to what's in noxphasma's original report.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Some additional info, I'm able to use WR with most images missing and no crash up until the point I try to use autoscrolling. It's still not rendering properly as the initial bug report shows, so the crash may be unrelated as it can be triggered by hitting middle mouse button (with autoscrolling enabled) and doesn't happen if I don't try to scroll.
I have a machine that shows this problem (1080Ti on Fedora). I captured a recording in Firefox and played it back in 'wrench' and it displays fine there, so we evidently have some problem with how WR integrates into Gecko that's causing this.
Attached image Rendering Example —
Today's nightly build 2018-01-16 seems to fix the stability issues on my machine, I can't reproduce any of the crashing or error messages that were coming up before. The missing graphics are still evident. Attached a screenshot as an example if that helps.
I'm suffering from this as well (it seems be caused by opaque primitives not getting rendered or being completely transparent, see bug 1434735) but Wrench seems to draw everything properly, so I'm a bit confused.
I've spent the weekend attempting to track the issue down, unsuccessfully. - A WR dump from Gecko is drawn correctly by Wrench, not reproducing the problem. - qapitrace replays the frames from an apitrace correctly, not reproducing the problem. - Disabling the clear scissor has no effect. - Stubbing out some optimizations that elide depth clears has no effect.
The gpu drivers are only being used from parent process.
Ah, right; MOZ_SANDBOX_LOGGING didn't show any violations. I also tried reducing the sandbox level to 0.
The issue is that the visual GDK (GTK) picks for Firefox's window doesn't have a depth buffer. GDK chooses the visuals 43 (0x02b, 24-bit RGB) and 130 (0x082, 32-bit RGBA) for its windows and caches this selection on the X root window: `xprop -root` shows "GDK_VISUALS(INTEGER) = 43, 130". Overwriting this value using `xprop -root -format GDK_VISUALS 32ii -set GDK_VISUALS "39, 126"` to make GDK pick visuals with a depth buffer allows WR to draw properly. Obviously, that's not a tenable workaround. How to do get Firefox to tell GDK it wants different visuals?
(In reply to Jan Alexander Steffens [:heftig] from comment #20) > Overwriting this value using `xprop -root -format GDK_VISUALS 32ii -set > GDK_VISUALS "39, 126"` to make GDK pick visuals with a depth buffer allows > WR to draw properly. This works perfectly fine for me, the rendering is correct now. However, I do get some error messages, which are probably not related to this issue, I just want to point it out anyway: > WR: ERROR: Invalid window dimensions! Please call api.set_window_size() These messages occur a lot in about:support.
(In reply to Jan Alexander Steffens [:heftig] from comment #20) > The issue is that the visual GDK (GTK) picks for Firefox's window doesn't > have a depth buffer. What a catch! It looks like GDK does not consider GLX visuals at all[0]. This is bad. Things happened to work before because we never used a GLX context that needed fancy things like a depth buffer. I think we'll probably need to create the X11 window ourselves with a GLX visual, then create a GdkWindow from that via gdk_x11_window_foreign_new_for_display(). [0] https://github.com/GNOME/gtk/blob/master/gdk/x11/gdkvisual-x11.c#L52 [1] https://developer.gnome.org/gdk3/stable/gdk3-X-Window-System-Interaction.html#gdk-x11-window-foreign-new-for-display
Actually it does look like GDK tries to pick a good GL visual if able. I guess that machinery isn't working for some reason. https://github.com/GNOME/gtk/blob/1a9a0c25771d124a54b8bf7d81e36033ba429970/gdk/x11/gdkglcontext-x11.c#L1117
Yeah, but it specifically looks for visuals with depth_size == 0 because a depth buffer is "unnecessary stuff": https://github.com/GNOME/gtk/blob/1a9a0c25771d124a54b8bf7d81e36033ba429970/gdk/x11/gdkglcontext-x11.c#L1002 GTK's own GLArea widget draws to a texture or renderbuffer which is blitted to the widget, so the missing depth buffer is not an issue.
(In reply to Jan Alexander Steffens [:heftig] from comment #24) Indeed. In that case, I think my original comment #22 might be the way to go.
Just from a glance at the widget/gtk/nsWindow code I think that's going to be difficult as the window is a GtkWindow; I believe you can't wrap an arbitrary GdkWindow in a GtkWindow; the latter creates the former. Maybe the current mContainer solution used for CSD needs to be extended so that the WR render target is always a child of the toplevel GtkWindow that we can select a visual for? The MozContainer widget (used for mContainer?) is already capable of creating its own GdkWindow. Perhaps this bug should be moved to "Widget: Gtk"?
Martin, do you have any ideas on this?
Flags: needinfo?(stransky)
(In reply to Markus Stange [:mstange] from comment #27) > Martin, do you have any ideas on this? Sure, the custom visual can be set when we call moz_container_realize(): https://dxr.mozilla.org/mozilla-central/rev/efe1f380d0279d7786b0c6340a60aaf6c76eabbe/widget/gtk/mozcontainer.cpp#367 or when main nsWindow is created (this is without CSD): https://dxr.mozilla.org/mozilla-central/rev/efe1f380d0279d7786b0c6340a60aaf6c76eabbe/widget/gtk/nsWindow.cpp#3639 toolkit code just need to be notified about it. How to get the depth buffer request?
Flags: needinfo?(stransky)
(In reply to Martin Stránský [:stransky] from comment #28) > (In reply to Markus Stange [:mstange] from comment #27) > > Martin, do you have any ideas on this? > > Sure, the custom visual can be set when we call moz_container_realize(): > > https://dxr.mozilla.org/mozilla-central/rev/ > efe1f380d0279d7786b0c6340a60aaf6c76eabbe/widget/gtk/mozcontainer.cpp#367 > I hacked this to use visual 0x21 (which has depth) and it didn't seem to help. Does WebRender use this window or the toplevel one when CSD is enabled? > or when main nsWindow is created (this is without CSD): > > https://dxr.mozilla.org/mozilla-central/rev/ > efe1f380d0279d7786b0c6340a60aaf6c76eabbe/widget/gtk/nsWindow.cpp#3639 AFAICT this won't work because it appears GDK always uses gdk_x11_display_get_window_visual() for the window visual, and this function discards visuals with depth buffers as comment #24 indicates. > > toolkit code just need to be notified about it. How to get the depth buffer > request? We'd probably just query if WR is enabled and request a visual with depth (16bit?) in that case.
Flags: needinfo?(stransky)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #29) > (In reply to Martin Stránský [:stransky] from comment #28) > > (In reply to Markus Stange [:mstange] from comment #27) > > > Martin, do you have any ideas on this? > > > > Sure, the custom visual can be set when we call moz_container_realize(): > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > efe1f380d0279d7786b0c6340a60aaf6c76eabbe/widget/gtk/mozcontainer.cpp#367 > > > > I hacked this to use visual 0x21 (which has depth) and it didn't seem to > help. Does WebRender use this window or the toplevel one when CSD is enabled? When CSD is enabled the mozcontainer window is always used. You can also force firefox to run in CSD mode by: $MOZ_GTK_TITLEBAR_DECORATION="client" ./firefox Basically it's controlled by "drawToContainer" variable: https://dxr.mozilla.org/mozilla-central/rev/a859a4b2257e6c61f7c2aab456cf551df856bd95/widget/gtk/nsWindow.cpp#3766 You can set it to true to always use the container window. > > or when main nsWindow is created (this is without CSD): > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > efe1f380d0279d7786b0c6340a60aaf6c76eabbe/widget/gtk/nsWindow.cpp#3639 > > AFAICT this won't work because it appears GDK always uses > gdk_x11_display_get_window_visual() for the window visual, and this function > discards visuals with depth buffers as comment #24 indicates. Hm, maybe we need to create the GDK window with GL support? I can work on that with gnome folks. > > > > toolkit code just need to be notified about it. How to get the depth buffer > > request? > > We'd probably just query if WR is enabled and request a visual with depth > (16bit?) in that case. Can you please provide me that check which can be used from nsWindow::Create()? That will speed it up as I'm not familiar with the WR code - I see only "nsNativeThemeGTK::CreateWebRenderCommandsForWidget()" there which does not tell me much.
Flags: needinfo?(stransky)
(In reply to Martin Stránský [:stransky] from comment #30) > > > > We'd probably just query if WR is enabled and request a visual with depth > > (16bit?) in that case. > > Can you please provide me that check which can be used from > nsWindow::Create()? That will speed it up as I'm not familiar with the WR > code - I see only "nsNativeThemeGTK::CreateWebRenderCommandsForWidget()" > there which does not tell me much. The WR folks said they expect 24 bit, so the patches I put up just request that.
To me it sounds a bit like bug 1406230 could be related. bp-96d51f1e-b8dc-413c-a53d-357520180203 Would your patches also fix that?
Comment on attachment 8957554 [details] Bug 1401455 - Expose glxChooseVisual() in GLXLibrary https://reviewboard.mozilla.org/r/226426/#review232444 ::: gfx/gl/GLXLibrary.h:15 (Diff revision 1) > #include "prlink.h" > typedef realGLboolean GLboolean; > > // stuff from glx.h > #include "X11/Xlib.h" > +#include "X11/Xutil.h" // for XVisualInfo Forward-declare it rather than including the header.
Attachment #8957554 - Flags: review?(jgilbert) → review+
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review232738 ::: widget/gtk/mozcontainer.cpp:352 (Diff revision 2) > } > > +static GdkVisual* > +choose_glx_visual(GtkWidget *widget) > +{ > +#ifdef MOZ_X11 Please use X11 display run-time check here (see GDK_IS_X11_DISPLAY(display) at moz_container_map_surface(). gdk_x11_screen_lookup_visual() will crash on Wayland display.
Comment on attachment 8957554 [details] Bug 1401455 - Expose glxChooseVisual() in GLXLibrary https://reviewboard.mozilla.org/r/226426/#review232444 > Forward-declare it rather than including the header. It's a typedefed C struct, so it's not so easy to forward declare, AFAICT.
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review232980 ::: widget/gtk/mozcontainer.cpp:353 (Diff revision 3) > > +static GdkVisual* > +choose_glx_visual(GtkWidget *widget) > +{ > +#ifdef MOZ_WAYLAND > + if (GDK_IS_WAYLAND_DISPLAY (gtk_widget_get_display (widget))) { Man, the style in this file is insane. I'll normalize it separately.
Attachment #8957555 - Flags: review+
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender Ah, could have sworn this was r?me. Oops!
Attachment #8957555 - Flags: review+
(In reply to Jeff Gilbert [:jgilbert] from comment #40) > Comment on attachment 8957555 [details] > Bug 1401455 - Use correct GLX visual when rendering with WebRender > > https://reviewboard.mozilla.org/r/226428/#review232980 > > ::: widget/gtk/mozcontainer.cpp:353 > (Diff revision 3) > > > > +static GdkVisual* > > +choose_glx_visual(GtkWidget *widget) > > +{ > > +#ifdef MOZ_WAYLAND > > + if (GDK_IS_WAYLAND_DISPLAY (gtk_widget_get_display (widget))) { > > Man, the style in this file is insane. I'll normalize it separately. The space before parens is gtk+ style, and yes it's terrible. https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en
Just because we use their library doesn't mean we should adopt their code style! I can understand whoever contributed this being used to it, though.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #29) > (In reply to Martin Stránský [:stransky] from comment #28) > > or when main nsWindow is created (this is without CSD): > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > efe1f380d0279d7786b0c6340a60aaf6c76eabbe/widget/gtk/nsWindow.cpp#3639 > > AFAICT this won't work because it appears GDK always uses > gdk_x11_display_get_window_visual() for the window visual, and this function > discards visuals with depth buffers as comment #24 indicates. I can't find gdk_x11_display_get_window_visual in GDK 3.22.19. Be careful not to look at GDK 4 code. I assume GDK 4 will continue to change before Gecko tries to use it, and assume Gecko will need to do something quite different and so I wouldn't plan ahead for that now. (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #42) > The space before parens is gtk+ style, and yes it's terrible. Almost as bad as putting a space between "if" and the parens ;)
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review232650 ::: commit-message-5eedc:5 (Diff revision 2) > +visual when creating the window. This should be fine even when we're > +using the container with traditional layers. dlopening libGL is not free. Perhaps it is OK now (if fewer libraries are using text relocations for example), but it is not clear that it should be fine. If the depth buffer is actually a property of the window, rather than just the fbconfig, then isn't that requesting unnecessary resources when the depth buffer is not required? Is a pref check to reduce risk an option? ::: widget/gtk/mozcontainer.cpp:353 (Diff revisions 2 - 3) > > static GdkVisual* > choose_glx_visual(GtkWidget *widget) > { > +#ifdef MOZ_WAYLAND > + if (GDK_IS_WAYLAND_DISPLAY (gtk_widget_get_display (widget))) { Use GDK_IS_X11_DISPLAY(display) because there are other displays that are not Wayland, even if not supported in Gecko at this stage. ::: widget/gtk/mozcontainer.cpp:359 (Diff revision 2) > + LOCAL_GLX_RGBA, > + LOCAL_GLX_DOUBLEBUFFER, > + LOCAL_GLX_RED_SIZE, 8, > + LOCAL_GLX_GREEN_SIZE, 8, > + LOCAL_GLX_BLUE_SIZE, 8, > + LOCAL_GLX_ALPHA_SIZE, 0, This is unlikely to work well for translucent windows. https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/widget/gtk/nsWindow.cpp#3643 ::: widget/gtk/mozcontainer.cpp:360 (Diff revision 2) > + LOCAL_GLX_DOUBLEBUFFER, > + LOCAL_GLX_RED_SIZE, 8, > + LOCAL_GLX_GREEN_SIZE, 8, > + LOCAL_GLX_BLUE_SIZE, 8, > + LOCAL_GLX_ALPHA_SIZE, 0, > + LOCAL_GLX_DEPTH_SIZE, 24, This code is all a bit foreign to widget/GTK. I guess this number is matching a similar number here, for example? https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/gl/GLContextProviderGLX.cpp#895 There are a number of similar constructions in GLContextProviderGLX/GLContextGLX. I assume the |aWebRender| part of this test is no longer necessary with this change? https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/gl/GLContextProviderGLX.cpp#936 Would it be practical to implemented in GLContextProviderGLX.cpp a method to return the visual (Xlib or id) based on the translucency requirement? Please request review from :lsalzman for this part. I don't know much about depth buffers and Lee reviewed the original addition of depth buffers. ::: widget/gtk/mozcontainer.cpp:365 (Diff revision 2) > + LOCAL_GLX_DEPTH_SIZE, 24, > + LOCAL_GL_NONE > + }; > + > + GdkScreen* screen = gtk_widget_get_screen(widget); > + g_return_val_if_fail(screen, NULL); gtk_widget_get_screen() won't return null (except on programmer errors) and so no need to return null here. ::: widget/gtk/mozcontainer.cpp:367 (Diff revision 2) > + g_return_val_if_fail(mozilla::gl::sGLXLibrary.EnsureInitialized(), NULL); > + XVisualInfo* visuals = mozilla::gl::sGLXLibrary.fChooseVisual(GDK_SCREEN_XDISPLAY(screen), > + 0, attribs); > + g_return_val_if_fail(visuals, NULL); g_return_val_if_fail() is for programmer errors only, but don't use it in new Gecko code. If G_DISABLE_CHECKS is set, then it is a no-op and EnsureInitialized() will not be called. When they are compiled, I assume these can fail (for reasons other than programming errors), in which case a null visual would not be appropriate. ::: widget/gtk/mozcontainer.cpp:397 (Diff revision 2) > attributes.x = allocation.x; > attributes.y = allocation.y; > attributes.width = allocation.width; > attributes.height = allocation.height; > attributes.wclass = GDK_INPUT_OUTPUT; > - attributes.visual = gtk_widget_get_visual (widget); > + attributes.visual = choose_glx_visual(widget); The GTK convention is that the visual of the widget is what is used as the visual of the window. I don't know whether there are any problems if that convention is not followed, but it is not necessary to stray from this convention. The visual of the widget can be set as desired with gtk_widget_set_visual() before the widget is realized. i.e. in nsWindow::Create(). ::: widget/gtk/nsWindow.cpp:3754 (Diff revision 2) > /* There are two cases here: > * > * 1) We're running on Gtk+ without client side decorations. > * Content is rendered to mShell window and we listen > * to the Gtk+ events on mShell > * 2) We're running on Gtk+ and client side decorations > * are drawn by Gtk+ to mShell. Content is rendered to mContainer > * and we listen to the Gtk+ events on mContainer. > * 3) We're running on Wayland. All gecko content is rendered > * to mContainer and we listen to the Gtk+ events on mContainer. > + * 4) We're rendering this window with WebRender Why do you want to render to a child window with WebRender? Would changing the visual of mShell (when rendering to mShell) make this unnecessary? If this is necessary, then please explain in a comment. This comment also needs tidying up, and adding this line is making it worse. It is not clear what the intended widget for rendering is with WebRender. I assume it is the same as for 2 and 3, but the comment is not clear. It seems that 1 should be replaced with an otherwise case, because the current description is not mutually exclusive from 4.
Attachment #8957555 - Flags: review?(karlt) → review-
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review233246
Comment on attachment 8957554 [details] Bug 1401455 - Expose glxChooseVisual() in GLXLibrary https://reviewboard.mozilla.org/r/226426/#review233270 ::: gfx/gl/GLContextProviderGLX.cpp:896 (Diff revision 2) > { > ScopedXFree<GLXFBConfig>& cfgs = *out_scopedConfigArr; > int numConfigs; > - const int webrenderAttribs[] = { > - LOCAL_GLX_DEPTH_SIZE, 24, > + const int attribs[] = { > + LOCAL_GLX_ALPHA_SIZE, aUseAlpha ? 8 : 0, > + LOCAL_GLX_DEPTH_SIZE, aWebRender ? 0 : 24, This appears to be backwards? Did you mean "aWebRender ? 24 : 0"?
Comment on attachment 8957554 [details] Bug 1401455 - Expose glxChooseVisual() in GLXLibrary https://reviewboard.mozilla.org/r/226426/#review233356 ::: commit-message-9219c:1 (Diff revision 3) > +Bug 1401455 - Make FindFBConfigForWindow() optionally look for configs with alpha r=lsalzman Note that this is already usually implicitly happening through the AreCompatibleVisuals() or VisualID checks. The exception is if there is a visual/fbconfig with depth 32, but only 24 color bits and zero alpha bits (NVIDIA). The window visual already has information about whether there is an alpha channel, and so the extra aUseAlpha parameter is more or less redundant. XGetVisualInfo(,VisualIDMask,,) and glXGetConfig(,,GLX_ALPHA_SIZE,) could be used instead of the parameter. That does cost an extra allocation, but that is not the only one here - I'd guess another would not make much difference.
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review233358 ::: commit-message-5eedc:3 (Diff revision 5) > +WebRender requires an OpenGL depth buffer, which on X11 requires a > +window with a visual that also has a depth buffer. GtkWindow chooses > +its own visual instead of the one supplied via gtk_widget_set_visual() > +and helpfully filters out any visuals that have a depth > +buffer. To work around this, render WebRender content into > +the container widget, which sets up the visual as we requested > +via gtk_widget_set_visual(). The only time I see GtkWindow overriding the one from gtk_widget_set_visual() is with CSD, but, in that case, Gecko is already rendering to a window for the container. Translucent windows are already depending on gtk_widget_set_visual(mShell). ::: gfx/gl/GLContextGLX.h:13 (Diff revision 5) > #define GLCONTEXTGLX_H_ > > #include "GLContext.h" > #include "GLXLibrary.h" > #include "mozilla/X11Util.h" > +#include "gfxXlibSurface.h" Unrelated? ::: widget/gtk/mozcontainer.cpp (Diff revision 5) > -#ifdef MOZ_WAYLAND > #include <gdk/gdkx.h> > +#ifdef MOZ_WAYLAND Unrelated? ::: widget/gtk/nsWindow.cpp:3652 (Diff revision 5) > GdkVisual *visual = gdk_screen_get_rgba_visual(screen); > gtk_widget_set_visual(mShell, visual); > } > } > > + No extra whitespace please. ::: widget/gtk/nsWindow.cpp:3770 (Diff revision 5) > + * 4) We're using WebRender on X11. WebRender requires a Visual with a depth buffer, > + * and GtkWindow does not allow us to specify this. The container > + * will create a new window with the requested visual, which is set as the > + * widget visual on the container. Gecko will be rendered into > + * mContainer, and we listen to Gtk+ events on mContainer. Wrap to 80 columns. 1 and 4 are overlapping and inconsistent. Use otherwise. ::: widget/gtk/nsWindow.cpp:3786 (Diff revision 5) > + (mIsX11Display && useWebRender); > eventWidget = (drawToContainer) ? container : mShell; > > + if (mIsX11Display && useWebRender) { > + GtkWidget* widget = GTK_WIDGET(mContainer); > + Display* display = GDK_DISPLAY_XDISPLAY(gtk_widget_get_display(widget)); 80 columns. ::: widget/gtk/nsWindow.cpp:3792 (Diff revision 5) > + int screen = GDK_SCREEN_XNUMBER(gtk_widget_get_screen(widget)); > + > + mozilla::ScopedXFree<GLXFBConfig> configArr; > + GLXFBConfig config; > + int visualId; > + if (GLContextGLX::FindFBConfigForWindow(display, screen, FindFBConfigForWindow() is really FindFBConfigForVisual() with an XGetWindowAttributes() to find the visual for the window. That XGetWindowAttributes() blocks on the Xserver, a pattern which is avoided where possible, and avoiding is possible with the callers of FindFBConfigForWindow() because the visual is already known by the client. The window parameter is meaningless here really as it is the visual that is being matched. FindFBConfigForWindow() is OK to use as an interim solution though, if this is not intended for production. ::: widget/gtk/nsWindow.cpp:3792 (Diff revision 5) > + if (GLContextGLX::FindFBConfigForWindow(display, screen, > + GDK_ROOT_WINDOW(), This doesn't solve the visual-with-alpha situation, except that the FindFBConfigForWindow will return null. The root window will (usually) not have alpha bits and so AreCompatibleVisuals() will not match. A quick fix option would be to only use web render when !useAlphaVisual. The changes to FindFBConfigForWindow() would then not be required. It seems that what this approach is aiming at is FindPreferredFBConfigLikeVisual() for some definition of "like", where the visual is that currently set on the widget. ::: widget/gtk/nsWindow.cpp:3794 (Diff revision 5) > + useWebRender, useAlphaVisual, > + &configArr, &config, &visualId)) { 80 columns. ::: widget/gtk/nsWindow.cpp:3797 (Diff revision 5) > + if (GLContextGLX::FindFBConfigForWindow(display, screen, > + GDK_ROOT_WINDOW(), > + useWebRender, useAlphaVisual, > + &configArr, &config, &visualId)) { > + gtk_widget_set_visual(widget, > + gdk_x11_screen_lookup_visual(gtk_widget_get_screen(widget), Call gtk_widget_get_screen only once and store the result in an automatic variable.
Attachment #8957555 - Flags: review?(karlt) → review-
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review233448 ::: commit-message-5eedc:3 (Diff revision 5) > +WebRender requires an OpenGL depth buffer, which on X11 requires a > +window with a visual that also has a depth buffer. GtkWindow chooses > +its own visual instead of the one supplied via gtk_widget_set_visual() > +and helpfully filters out any visuals that have a depth > +buffer. To work around this, render WebRender content into > +the container widget, which sets up the visual as we requested > +via gtk_widget_set_visual(). See comments #23 and #24. Also https://github.com/GNOME/gtk/blob/1a9a0c25771d124a54b8bf7d81e36033ba429970/gdk/x11/gdkwindow-x11.c#L894 ::: gfx/gl/GLContextGLX.h:13 (Diff revision 5) > #define GLCONTEXTGLX_H_ > > #include "GLContext.h" > #include "GLXLibrary.h" > #include "mozilla/X11Util.h" > +#include "gfxXlibSurface.h" It doesn't build without this because GLContextGLX.h uses gfxXlibSurface. Probably works everywhere else by accident. ::: widget/gtk/mozcontainer.cpp (Diff revision 5) > -#ifdef MOZ_WAYLAND > #include <gdk/gdkx.h> > +#ifdef MOZ_WAYLAND Whoops, that's from a prior approach doing X stuff in the container. ::: widget/gtk/nsWindow.cpp:3792 (Diff revision 5) > + int screen = GDK_SCREEN_XNUMBER(gtk_widget_get_screen(widget)); > + > + mozilla::ScopedXFree<GLXFBConfig> configArr; > + GLXFBConfig config; > + int visualId; > + if (GLContextGLX::FindFBConfigForWindow(display, screen, Hmm, indeed. I think maybe I'll go back to putting a glxChooseVisuals() thing in GLContextGLX and leaving FindFBConfigForWindow() alone.
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review232650 > This code is all a bit foreign to widget/GTK. > I guess this number is matching a similar number here, for example? > > https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/gl/GLContextProviderGLX.cpp#895 > > There are a number of similar constructions in > GLContextProviderGLX/GLContextGLX. > > I assume the |aWebRender| part of this test is no longer necessary with this change? > https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/gl/GLContextProviderGLX.cpp#936 > > Would it be practical to implemented in > GLContextProviderGLX.cpp a method to return the visual (Xlib or id) based on the translucency requirement? > > Please request review from :lsalzman for this part. > I don't know much about depth buffers and Lee reviewed the original addition of depth buffers. It looks like we can actually just use GLContextGLX::FindFBConfigForWindow(), as it returns a visual ID there. This will also ensure the GLContext and Window agree on the attributes used.
Comment on attachment 8958867 [details] Bug 1401455 - Add GLContextGLX::FindVisual() https://reviewboard.mozilla.org/r/227740/#review233586
Attachment #8958867 - Flags: review?(lsalzman) → review+
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review232650 > It looks like we can actually just use GLContextGLX::FindFBConfigForWindow(), as it returns a visual ID there. This will also ensure the GLContext and Window agree on the attributes used. Is the |aWebRender| test still required?
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review233448 > See comments #23 and #24. Also https://github.com/GNOME/gtk/blob/1a9a0c25771d124a54b8bf7d81e36033ba429970/gdk/x11/gdkwindow-x11.c#L894 https://bugzilla.mozilla.org/show_bug.cgi?id=1401455#c44 That looks like GDK 4 code, but Gecko runs against GDK 3. It dates back to this commit https://gitlab.gnome.org/GNOME/gtk/commit/f420dc74566ef1c284cd22314017e12624d51a15#9356f34f94bf1eb7b3f420efa0500ed7892f2561_469_469 but gdk_window_get_visual() is part of the GDK 3 ABI, and so it can't be removed. Don't plan for GDK 4 now because it will change. At the moment, for example, it looks like GDK 4 has no means to specify the visual of a GdkWindow (excluding perhaps tricks to set root window properties), and so the same problem will prevent creating a GdkWindow for the MozContainer with the correct visual. The visual on mShell would need to be set before gtk_widget_realize(), and it would need to be set again on the container when |drawToContainer| for the sake of the CSD case. That may mean that the code is simpler if instead the container is always created for the WebRender case. If the code is simpler then I'm fine with that but please explain that in the comments.
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review233738 ::: gfx/gl/GLContextGLX.h:13 (Diff revision 6) > #define GLCONTEXTGLX_H_ > > #include "GLContext.h" > #include "GLXLibrary.h" > #include "mozilla/X11Util.h" > +#include "gfxXlibSurface.h" Does "class gfxXlibSurface;" work here? GLContextGLX constructor and destructor are not implemented in the header. ::: widget/gtk/nsWindow.cpp:53 (Diff revision 6) > #include <gdk/gdkx.h> > #include <X11/Xatom.h> > #include <X11/extensions/XShm.h> > #include <X11/extensions/shape.h> > #include <gdk/gdkkeysyms-compat.h> > +#include "GLContextGLX.h" // for GLContextGLX::FindVisual() Can you move this from among system includes to among Gecko includes, please? Before GtkCompositorWidget.h, perhaps. ::: widget/gtk/nsWindow.cpp:3637 (Diff revision 6) > // mozilla.widget.use-argb-visuals is a hidden pref defaulting to false > // to allow experimentation > if (Preferences::GetBool("mozilla.widget.use-argb-visuals", false)) > useAlphaVisual = true; > > + bool useWebRender = gfx::gfxVars::UseWebRender() && AllowWebRenderForThisWindow(); 80 cols.
Attachment #8957555 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #61) > Comment on attachment 8957555 [details] > Bug 1401455 - Use correct GLX visual when rendering with WebRender > > https://reviewboard.mozilla.org/r/226428/#review233448 > > > See comments #23 and #24. Also https://github.com/GNOME/gtk/blob/1a9a0c25771d124a54b8bf7d81e36033ba429970/gdk/x11/gdkwindow-x11.c#L894 > > https://bugzilla.mozilla.org/show_bug.cgi?id=1401455#c44 > > That looks like GDK 4 code, but Gecko runs against GDK 3. > It dates back to this commit > https://gitlab.gnome.org/GNOME/gtk/commit/ > f420dc74566ef1c284cd22314017e12624d51a15#9356f34f94bf1eb7b3f420efa0500ed7892f > 2561_469_469 > but gdk_window_get_visual() is part of the GDK 3 ABI, and so it can't be > removed. > > Don't plan for GDK 4 now because it will change. > At the moment, for example, it looks like GDK 4 has no means to specify the > visual of a GdkWindow (excluding perhaps tricks to set root window > properties), and so the same problem will prevent creating a GdkWindow for > the MozContainer with the correct visual. The very first thing I tried was simply setting the visual directly on the mShell, and it didn't seem to help. I tried again just now and it does work, so I must've done something wrong earlier. > > The visual on mShell would need to be set before gtk_widget_realize(), and > it would need to be set again on the container when |drawToContainer| for > the sake of the CSD case. > That may mean that the code is simpler if instead the container is always > created for the WebRender case. If the code is simpler then I'm fine with > that but please explain that in the comments. It looks like mContainer returns the parent visual[1] in the case where one isn't explicitly set, so no additional effort is required for mContainer. [1] https://github.com/GNOME/gtk/blob/gtk-3-22/gtk/gtkwidget.c#L11628
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review232650 > Is the |aWebRender| test still required? The assumption in FindFBConfigForWindow() has been that any fbconfig can be chosen provided the color buffer format matches, according to the depth and AreCompatibleVisuals() checks. This bug seems to indicate that the assumption is not valid. With WebRender now setting the visual on the window, it can use an fbconfig with the visual id match, and there is no need to make the assumption in the WebRender case. Perhaps comparing the visual ids may also provide more robust fallback to another render method in the case of failing to find suitable visuals (I'm not sure).
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #63) > (In reply to Karl Tomlinson (:karlt) from comment #61) > > The visual on mShell would need to be set before gtk_widget_realize(), and > > it would need to be set again on the container when |drawToContainer| for > > the sake of the CSD case. > > That may mean that the code is simpler if instead the container is always > > created for the WebRender case. If the code is simpler then I'm fine with > > that but please explain that in the comments. > > It looks like mContainer returns the parent visual[1] in the case where one > isn't explicitly set, so no additional effort is required for mContainer. > > [1] https://github.com/GNOME/gtk/blob/gtk-3-22/gtk/gtkwidget.c#L11628 Usually, yes, but with CSD the GtkWindow does override its visual https://github.com/GNOME/gtk/blob/v3.22.20/gtk/gtkwindow.c#L4134 With the container realized after the shell, the container fetches the CSD visual from the parent. It is difficult to determine whether CSD is being used without realizing the shell first.
Comment on attachment 8957555 [details] Bug 1401455 - Use correct GLX visual when rendering with WebRender https://reviewboard.mozilla.org/r/226428/#review234048 Everything here is good, thanks. There are a couple of things that would finish this off, but this part is good to land as is, and addresses the bug in the common case. https://reviewboard.mozilla.org/r/226428/#comment295918 https://bugzilla.mozilla.org/show_bug.cgi?id=1401455#c66
Attachment #8957555 - Flags: review?(karlt) → review+
Pushed by jwillcox@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/23f2179c84b7 Expose glxChooseVisual() in GLXLibrary r=jgilbert https://hg.mozilla.org/integration/mozilla-inbound/rev/d63e97b61878 Add GLContextGLX::FindVisual() r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c821125d8d Use correct GLX visual when rendering with WebRender r=karlt
Backout by jwillcox@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/62d1727dd81a Back out due to Marionette failures on a CLOSED TREE
This was backed out due to a crash in Marionette tests. The test in question is test_refresh_firefox.py. The crash is caused by gfxVars::UseWebRender() being called after gfxVars has been shut down. This means we're creating a new window during (after?) shutdown, which is certainly unexpected. Gijs, it appears you wrote this test, can you take a look at why this would be happening?
Flags: needinfo?(gijskruitbosch+bugs)
Attached file Stack from asan build —
Looks like you're crashing trying to create a window. Based on the stack (from https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f7c821125d8d17bcaf19d97263d67a8257db857f&selectedJob=168534019 ) I expect this is trying to create the migration dialog (migration.xul) to indicate progress of Firefox refresh. At this point the profile won't exist yet, so perhaps if you're sourcing things from there (explicitly or otherwise), that might explain what's going on? It's possible you can reproduce this yourself either by running with the `-migration` commandline handler, by manually running a throwaway profile (created through the profile manager) and then using Firefox refresh, or by running the test in question yourself: ./mach marionette-test browser/components/migration/tests/marionette/test_refresh_firefox.py From a *very* quick look at the code, looks like this: bool useWebRender = gfx::gfxVars::UseWebRender() && AllowWebRenderForThisWindow(); is throwing a nullptr exception. Maybe gfxVars isn't initialized at this point or something? You'd probably know better than me...
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by jwillcox@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa26cb6f9d6 Expose glxChooseVisual() in GLXLibrary r=jgilbert https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0394eb1c57 Add GLContextGLX::FindVisual() r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/8d73f18bc1a2 Use correct GLX visual when rendering with WebRender r=karlt
I changed that line to: bool useWebRender = gfxPlatform::Initialized() && gfx::gfxVars::UseWebRender() && AllowWebRenderForThisWindow(); For whatever reason, gfxPlatform is not initialized when we're running the migration dialog. I'll file a followup on that one.
I filed a followup for the gfxVars issue in bug 1446553
Assignee: nobody → snorp
The wayland build is broken ATM since this landed, see bug 1447270.
This bug appears to be back in the Nightly build dated 2018-03-22 after working for a few days after it was marked Resolved Fixed.
(In reply to Vash63 from comment #77) > This bug appears to be back in the Nightly build dated 2018-03-22 after > working for a few days after it was marked Resolved Fixed. Yeah, due to bug 1446553.
Depends on: 1447270
Depends on: 1446553
Now that the VRService thread is enabled by default, we can remove these old files. The OpenVR 3rd party code has been moved from gfx/vr/openvr to gfx/vr/service/openvr to be closer to the OpenVRSession implementation. The Oculus header (ovr_capi_dynamic.h) has been moved from gfx/vr/ovr_capi_dynamic.h to gfx/vr/service/oculus to be closer to the OculusSession implementation.
(In reply to :kip (Kearwood Gilbert) from comment #80) > Created attachment 9019525 [details] > Bug 1501455 - Remove gfxVROculus and gfxVROpenVR > gfx/vr/gfxVROculus.cpp/h and gfx/vr/gfxVROpenVR.cpp/h have > been replaced with the VRService thread implementations in > gfx/vr/service > > Now that the VRService thread is enabled by default, we > can remove these old files. > > The OpenVR 3rd party code has been moved from gfx/vr/openvr > to gfx/vr/service/openvr to be closer to the OpenVRSession > implementation. > > The Oculus header (ovr_capi_dynamic.h) has been moved from > gfx/vr/ovr_capi_dynamic.h to gfx/vr/service/oculus to be > closer to the OculusSession implementation. Is this really the bug you wanted to attach this patch to? It appears to me that you actually wanted the bug with a 5 as second digit.
Flags: needinfo?(kgilbert)
Attachment #9019525 - Attachment is obsolete: true
(In reply to Martin Giger [:freaktechnik] from comment #81) > (In reply to :kip (Kearwood Gilbert) from comment #80) > > Created attachment 9019525 [details] > > Bug 1501455 - Remove gfxVROculus and gfxVROpenVR > > gfx/vr/gfxVROculus.cpp/h and gfx/vr/gfxVROpenVR.cpp/h have > > been replaced with the VRService thread implementations in > > gfx/vr/service > > > > Now that the VRService thread is enabled by default, we > > can remove these old files. > > > > The OpenVR 3rd party code has been moved from gfx/vr/openvr > > to gfx/vr/service/openvr to be closer to the OpenVRSession > > implementation. > > > > The Oculus header (ovr_capi_dynamic.h) has been moved from > > gfx/vr/ovr_capi_dynamic.h to gfx/vr/service/oculus to be > > closer to the OculusSession implementation. > > Is this really the bug you wanted to attach this patch to? It appears to me > that you actually wanted the bug with a 5 as second digit. Indeed, this was attached to the wrong bug. Please disregard this patch.
Flags: needinfo?(kgilbert)
See Also: → 1525892
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: