Turning on layers.offmainthreadcomposition.enabled and layers.acceleration.force-enabled together cause such firefox crash: firefox: ../../src/xcb_io.c:140: dequeue_pending_request: Проверочное утверждение «req == dpy->xcb->pending_requests» не выполнено. Аварийный останов on my system (NVIDIA driver 290.10, X.Org X Server 1.9.2.901).

The Linux-specific bits we need for off-main-thread-compositing haven't yet been implemented. Essentially, we need the Linux equivalent of the widget/cocoa/* stuff from Attachment 589583 [details] [diff].

This is blocking getting layers acceleration on linux.

Fixes crash when using layers.acceleration.force-enabled and layers.offmainthreadcompositing.enabled on Linux.

Comment on attachment 613729 [details] [diff] [review] Calls XInitThreads before the first call to XOpenDisplay to allow using X11 from multiple threads. Should not we call this XInitThreads only in case if omtc is enabled? otherwise X will enable Multithread support/checks for normal builds too, and that could hit performance for non OMTC case

(In reply to Oleg Romashin (:romaxa) from comment #4) > Comment on attachment 613729 [details] [diff] [review] > Calls XInitThreads before the first call to XOpenDisplay to allow using X11 > from multiple threads. > > Should not we call this XInitThreads only in case if omtc is enabled? > otherwise X will enable Multithread support/checks for normal builds too, > and that could hit performance for non OMTC case It only works if XInitThread is called before the first call to XOpenDisplay. If we check for the pref before, then we also need to make sure omtc is done only if the pref is set AND we know that XInitThread was called when firefox started (else, we have a bad crash in X11 as soon as omtc is used). So changing layers.offmainthreadcomposition.enabled would take effect only after a restart of firefox (which makes using a pref less interesting). Can we benchmark it efficiently to check if it is worth disabling thread safety on non-omtc configs? I don't know much about firefox initialization so I would need some experienced people's advice about this: - Is it a problem to depend from mozilla::Preferences in nsAppRunner.cpp? Would it even work actually (well, that can be checked easily)? - If it is a problem, can we defer X/gdk related calls to later in the initialization process? If using libpref so early is a problem, I see two potential solutions: - using #ifdef guards to activate/deactivate omtc at build time (so that platforms that are using X and are not likely to use omtc may not worry bout XInitThread related overhead)? - or, as bjacob just suggested to me, use an environment variable instead of a pref to deal with omtc. This would remove potential problems with omtc changing state throughout execution.

v2: Added a call to XInitThreads before the first call to XOpenDisplay if the environment variable MOX_X_THREADSAFE is set. If MOZ_X_THREADSAFE is not set, disable omtc to prevent crash. XInitThreads is required in order to use X11 from multiple threads and fixes a crash with omtc on Linux. I use an environment variable because I think xpcom is not initialized before the first call to gdk_open_display which means I can't read the pref this early in the initialization process.

Comment on attachment 614179 [details] [diff] [review] v2: Enables thread-safe X11 if the environment variable MOZ_X_THREADSAFE is set. This is a little bit messy... can we move display open functionality after Profile service creation. so we can request pref and don't bother with env options. > + if(PR_GetEnv("MOZ_X_THREADSAFE")){ also please make sure to follow style guide lines. if[space](blabla)[space]{ probably karl could suggest something useful here

(In reply to Oleg Romashin (:romaxa) from comment #7) > Comment on attachment 614179 [details] [diff] [review] > v2: Enables thread-safe X11 if the environment variable MOZ_X_THREADSAFE is > set. > > This is a little bit messy... can we move display open functionality after > Profile service creation. so we can request pref and don't bother with env > options. I'm afraid I won't be able to do that. There are too many interdependencies in the initialization of firefox. Prefs get accessible quite late in the process, relying on things that rely on other things [..] that rely on X11 stuff. I spent this morning trying to isolate things and move them around. It breaks very quickly. I agree, though, that having both a pref and an environment variable is not very beautiful. Since there is motivation in keeping the pref, I can have it automatically set to false if MOZ_X_THREADSAFE is not set (for consistency in the point of view of the user). > > + if(PR_GetEnv("MOZ_X_THREADSAFE")){ > > also please make sure to follow style guide lines. if[space](blabla)[space]{ Sure, sorry about this.

Now X11 platforms don't have the pref layers.offmainthreadcomposition.enabled anymore. Instead they enable omtc using an environment variable because it is not currently possible to access prefs before the first call to XOpenDisplay and we don't want to have the overhead of XInitThreads when OMTC is not used. On every platforms, whether or not we use OMTC is decided only once per execution (as opposed to every time we create a nsBaseWidget) and other ports of the code should access this state using nsBseWidget::UseOffMainThreadCompositing instead of accessing the pref directly.

Comment on attachment 614552 [details] [diff] [review] Initialize X11 in thread-safe mode if the environment variable MOZ_USE_OMTC is set. Yes, I agree the environment variable seems the practical way to test OMTC on X11. (I don't see a way to find out whether XInitThreads() has been called without looking at the "internals" of a Display struct.) I'll let Jeff decide whether the check-once approach is appropriate/suitable for the other platforms. Please include function names and 8 lines of context in future patches. The [diff] section of the hgrc here will do that by default: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F >+/* static */ void >+InitOnlyOnce() Uncomment "static" here to make it clear that this function has a file-internal linkage. We use /* static */ on class methods (to distinguish them from object methods) because C++ doesn't allow the static keyword on their definition.

Comment on attachment 616131 [details] [diff] [review] Fixes style problems >+#ifndef MOZ_X11 > pref("layers.offmainthreadcomposition.enabled", false); >+#endif What is the point of this change? I think we should not touch it >+ // interdependencies in the initialization. >+ if (PR_GetEnv("MOZ_USE_OMTC")) { >+ XInitThreads(); ^ white space > // nsBaseWidget >@@ -127,16 +130,17 @@ nsBaseWidget::nsBaseWidget() > #ifdef NOISY_WIDGET_LEAKS > gNumWidgets++; > printf("WIDGETS+ = %d\n", gNumWidgets); > #endif > > #ifdef DEBUG > debug_RegisterPrefCallbacks(); > #endif >+ InitOnlyOnce(); ^ better to keep 2 spaces here, because most of the file is 2 space indent > >+static void InitOnlyOnce() function already declared as static, no need to static here. >+ >+#ifdef MOZ_X11 >+ // On X11 platforms only use OMTC if firefox was initalized with thread-safe X11 (else it would crash). >+ sUseOffMainThreadCompositing = (PR_GetEnv("MOZ_USE_OMTC")!=NULL); >+#else >+ sUseOffMainThreadCompositing = mozilla::Preferences::GetBool("layers.offmainthreadcomposition.enabled", false); >+#endif I think it make sense, to do here: + sUseOffMainThreadCompositing = PR_GetEnv("MOZ_USE_OMTC") != NULL; + sUseOffMainThreadCompositing = + mozilla::Preferences::GetBool("layers.offmainthreadcomposition.enabled", sUseOffMainThreadCompositing); > > nsWindowType GetWindowType() { return mWindowType; } >- >+ static bool UseOffMainThreadCompositing(); don't kill empty lines

from other side, have you tried to insert XInitThreads somewhere in gfxPlatform::Init... does it work for you?

Could you check this patch if it works for you?

(In reply to Oleg Romashin (:romaxa) from comment #14) > Created attachment 618198 [details] [diff] [review] > Simple initialization. seems to work for me > > Could you check this patch if it works for you? I just tried attachment 618198 [details] [diff] [review] and it crashes on my machine (Fedora 16, x86_64). On my laptop it systematically doesn't work as soon as a put XInitThreads after the first call to XOpenDisplay (which is called within gdk_display_open here: http://dxr.lanedo.com/mozilla-central/toolkit/xre/nsAppRunner.cpp.html?string=nsAppRunner.cpp#l3298). X11 doc says "This function must be the first Xlib function a multi-threaded program calls, and it must complete before any other Xlib call is made." Thanks for pointing the styling problems, I'll fix them right away. About the change in libpref/init/all.js >+#ifndef MOZ_X11 > pref("layers.offmainthreadcomposition.enabled", false); >+#endif I removed the pref on X11 platforms since my patch ignores it and uses an environment variable instead. If we want to keep a pref that has no effect I can remove the #ifndef/#endif pair. It just felt more logical to me, should I remove it?

Just to make sure we test the same thing here: this crash happens systematically when I use _both_ omtc and layer acceleration (unless, of course, XInitThreads is invoked soon enough in which case I don't have the crash at all).

New version with White spaces fixed. romaxa: I think I see the misunderstanding with the pref/env variable: The way this patch works, you either are on an X11 platform and use the environment variable, or you are on a non-X11 platform in which case you use the pref. I mean that on Linux, for instance, you don't use both the pref and the env variable, you only use the env variable (and the pref does not exist since it would have no effect at all).

(In reply to Oleg Romashin (:romaxa) from comment #12) > >+static void InitOnlyOnce() > > function already declared as static, no need to static here. I asked Nicolas to use an explicit static here in comment 10. That saves reader from having to look to see if there is a forward declaration that may have additional keywords. It also provides some safety should the forward declaration become unnecessary and get removed some day. > I think it make sense, to do here: > + sUseOffMainThreadCompositing = PR_GetEnv("MOZ_USE_OMTC") != NULL; > + sUseOffMainThreadCompositing = > + mozilla::Preferences::GetBool("layers.offmainthreadcomposition.enabled", > sUseOffMainThreadCompositing); That would enable people to test OMTC even when XInitThreads() has not been called, but I don't think we should allow that possibility as it will only lead to problems.

I have compiled Firefox with attachment 616131 [details] [diff] [review] (sorry for the obsolete version, but it was the current one when I was building Firefox and the new one is only a styling fix, as I understand) and it is still crashy for me with layers acceleration and omtc enabled together (though they don't cause crashes separately). Startup runs normally but if I go, for example, to "Help" -> "About Firefox", after closing the window with information Firefox segfaults. Other windows created from Firefox window (including some gtk elements like selection from list) behave the same way. Extensions like Speed Dial also crash (they create windows internally as I can guess). Firefox crashes also when I close the main window, after restart I see a session restore dialog. Another issue is that about:support says layers acceleration isn't enabled even though layers.acceleration.force-enabled is set to true, and the only thing can tell me that I have layers accelerated is those crashes which aren't present otherwise (by the way, I don't see usual bugs of hardware acceleration itself like black areas around the downloads panel and some other UI elements with omtc enabled). Does anyone have these issues?

(In reply to konstartyom from comment #19) > I have compiled Firefox with attachment 616131 [details] [diff] [review] > (sorry for the obsolete version, but it was the current one when I was > building Firefox and the new one is only a styling fix, as I understand) and > it is still crashy for me with layers acceleration and omtc enabled together > (though they don't cause crashes separately). Startup runs normally but if I > go, for example, to "Help" -> "About Firefox", after closing the window with > information Firefox segfaults. Other windows created from Firefox window > (including some gtk elements like selection from list) behave the same way. > Extensions like Speed Dial also crash (they create windows internally as I > can guess). Firefox crashes also when I close the main window, after restart > I see a session restore dialog. > Another issue is that about:support says layers acceleration isn't enabled > even though layers.acceleration.force-enabled is set to true, and the only > thing can tell me that I have layers accelerated is those crashes which > aren't present otherwise (by the way, I don't see usual bugs of hardware > acceleration itself like black areas around the downloads panel and some > other UI elements with omtc enabled). > Does anyone have these issues? Ouch that's not good. I tried everything you said and it does not crash for me. What's your config? Also, If you enable layers.acceleration.drawfps does it display the pink rectangle with fps counter on the top left of each window? If so it means that layers acceleration is used (whatever about:support might say). On my machine about:support is coherent with about:config. You did set the environment variable MOZ_USE_OMTC right? The only difference we can have is that I have another patch (Bug 743830) that allows me to disable the use of TextureFromPixmap, but I tried with and without it and I don't have a crash either way. Did you change any other pref?

(In reply to Nicolas Silva [:nical] from comment #20) > Ouch that's not good. I tried everything you said and it does not crash for > me. What's your config? Ubuntu 10.10, X.Org X Server 1.9.2.901 (1.9.3 RC 1), NVIDIA driver 295.40. > Also, If you enable layers.acceleration.drawfps does > it display the pink rectangle with fps counter on the top left of each > window? Yes. > You did set the environment variable MOZ_USE_OMTC right? I understand from your comments that I need simply to set environment variable MOZ_USE_OMTC, am I right? So I add MOZ_USE_OMTC=1 to the environment. Correct me if I need a different value.

I forgot to add that I saw double ###!!! ABORT: X_GLXMakeCurrent: GLXBadDrawable: file /home/src/firefox/toolkit/xre/nsX11ErrorHandler.cpp, line 190 in the terminal when Firefox crashed. It's reproducible on the clean profile with only layers.acceleration.force-enabled=true. Ubuntu is x86_64.

(In reply to konstartyom from comment #21) > (In reply to Nicolas Silva [:nical] from comment #20) > > > Ouch that's not good. I tried everything you said and it does not crash for > > me. What's your config? > > Ubuntu 10.10, X.Org X Server 1.9.2.901 (1.9.3 RC 1), NVIDIA driver 295.40. I have a newer version of Xorg (1.11.4) and intel integrated gpu instead of nvidia. I'm downloading a live cd of ubuntu 10.10 to try to reproduce (just hope it's not on the nvdia side since I don't have an nvidia gpu with me) > I understand from your comments that I need simply to set environment > variable MOZ_USE_OMTC, am I right? So I add MOZ_USE_OMTC=1 to the > environment. Correct me if I need a different value. Yes you are right (sorry for this kind of questions, English being a second language for me, I am never sure I explain things clearly enough). > It's reproducible on the clean profile with only layers.acceleration.force- > enabled=true. Ubuntu is x86_64. So you also crash without off-main-thread compositing? In this case I think it is a different bug (worth investigating though).

(In reply to Nicolas Silva [:nical] from comment #23) > > It's reproducible on the clean profile with only layers.acceleration.force- > > enabled=true. Ubuntu is x86_64. > > So you also crash without off-main-thread compositing? In this case I think > it is a different bug (worth investigating though). No. I mean layers.acceleration.force-enabled is set to true and MOZ_USE_OMTC is set.

(In reply to konstartyom from comment #24) > No. I mean layers.acceleration.force-enabled is set to true and MOZ_USE_OMTC > is set. Oh, yes, sorry. I'll look into it. Also, if you can provide me with A stack trace of when the error shows up, it could help me a lot in case I don't manage to reproduce the bug.

(In reply to konstartyom from comment #22) > I forgot to add that I saw double > ###!!! ABORT: X_GLXMakeCurrent: GLXBadDrawable: file > /home/src/firefox/toolkit/xre/nsX11ErrorHandler.cpp, line 190 Could glXMakeCurrent be getting called after the window has been destroyed? Nicolas, you may like to try putting MOZ_X_SYNC=1 in the environment to see whether that helps you reproduce.

Comment on attachment 618255 [details] [diff] [review] Fixes crah when using OMTC and layer acceleration on linux. Review of attachment 618255 [details] [diff] [review]: ----------------------------------------------------------------- Karl, Can you take this review for me?

Sorry for my delayed response, I was a bit busy. Here is the backtrace (made with gdb): #0 TouchBadMemory () at /home/src/firefox/memory/mozalloc/mozalloc_abort.cpp:68 #1 0x00007ffff74b512c in mozalloc_abort (msg=<value optimized out>) at /home/src/firefox/memory/mozalloc/mozalloc_abort.cpp:89 #2 0x00007ffff5166016 in Abort (aSeverity=<value optimized out>, aStr=<value optimized out>, aExpr=0x0, aFile=0x7ffff5769f70 "/home/src/firefox/toolkit/xre/nsX11ErrorHandler.cpp", aLine=190) at /home/src/firefox/xpcom/base/nsDebugImpl.cpp:417 #3 NS_DebugBreak_P (aSeverity=<value optimized out>, aStr=<value optimized out>, aExpr=0x0, aFile=0x7ffff5769f70 "/home/src/firefox/toolkit/xre/nsX11ErrorHandler.cpp", aLine=190) at /home/src/firefox/xpcom/base/nsDebugImpl.cpp:404 #4 0x00007ffff4366ce7 in X11Error (display=0x7ffff7d8f000, event=0x7fffe8117550) at /home/src/firefox/toolkit/xre/nsX11ErrorHandler.cpp:190 #5 0x0000003ea30432d4 in _XError () from /usr/lib/libX11.so.6 #6 0x0000003ea303ffa7 in ?? () from /usr/lib/libX11.so.6 #7 0x0000003ea30406b7 in _XReply () from /usr/lib/libX11.so.6 #8 0x000000341f07d5c8 in ?? () from /usr/lib/nvidia-current/libGL.so.1 #9 0x000000341f07f01b in ?? () from /usr/lib/nvidia-current/libGL.so.1 #10 0x00007ffff520273a in mozilla::gl::GLXLibrary::xMakeCurrent ( this=0x7ffff652db30, display=0x7ffff7d8f000, drawable=48241858, context=0x7fffd90ca138) at /home/src/firefox/gfx/gl/GLContextProviderGLX.cpp:432 #11 0x00007ffff52027c0 in MakeCurrentImpl (this=0x7fffe2f1f800, aForce=<value optimized out>) at /home/src/firefox/gfx/gl/GLContextProviderGLX.cpp:798 #12 mozilla::gl::GLContextGLX::MakeCurrentImpl (this=0x7fffe2f1f800, aForce=<value optimized out>) at /home/src/firefox/gfx/gl/GLContextProviderGLX.cpp:787 #13 0x00007ffff52007ae in mozilla::gl::GLContext::MarkDestroyed ( this=0x7fffe2f1f800) at /home/src/firefox/gfx/gl/GLContext.cpp:1746 #14 0x00007ffff5204946 in mozilla::gl::GLContextGLX::~GLContextGLX ( this=0x7fffe2f1f800, __in_chrg=<value optimized out>) at /home/src/firefox/gfx/gl/GLContextProviderGLX.cpp:750 #15 0x00007ffff52049ea in mozilla::gl::GLContextGLX::~GLContextGLX ( this=0x7fffe2f1f800, __in_chrg=<value optimized out>) at /home/src/firefox/gfx/gl/GLContextProviderGLX.cpp:765 #16 0x00007ffff480786a in Release (this=0x7fffe2f1f800) at ../../../dist/include/GLContext.h:515 #17 mozilla::gl::GLContext::Release (this=0x7fffe2f1f800) at ../../../dist/include/GLContext.h:515 #18 0x00007ffff51eddf4 in operator= (this=0x7fffd90df540) at ../../dist/include/nsAutoPtr.h:964 #19 mozilla::layers::LayerManagerOGL::CleanupResources (this=0x7fffd90df540) at /home/src/firefox/gfx/layers/opengl/LayerManagerOGL.cpp:160 #20 0x00007ffff51ede41 in Destroy (this=0x7fffd90df540) at /home/src/firefox/gfx/layers/opengl/LayerManagerOGL.cpp:116 #21 mozilla::layers::LayerManagerOGL::Destroy (this=0x7fffd90df540) at /home/src/firefox/gfx/layers/opengl/LayerManagerOGL.cpp:108 #22 0x00007ffff51f66de in mozilla::layers::CompositorParent::RecvWillStop ( this=<value optimized out>) at /home/src/firefox/gfx/layers/ipc/CompositorParent.cpp:104 #23 0x00007ffff5090331 in mozilla::layers::PCompositorParent::OnMessageReceived (this=0x7fffd90cd000, __msg=<value optimized out>, __reply=@0x7fffe8117a28) at /home/src/firefox/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PCompositorParent.cpp:353 #24 0x00007ffff505b521 in mozilla::ipc::SyncChannel::OnDispatchMessage ( this=0x7fffd90cd010, msg=...) at /home/src/firefox/ipc/glue/SyncChannel.cpp:175 #25 0x00007ffff5058e3d in mozilla::ipc::RPCChannel::OnMaybeDequeueOne ( this=0x7fffd90cd010) at /home/src/firefox/ipc/glue/RPCChannel.cpp:432 #26 0x00007ffff5185c91 in MessageLoop::RunTask (this=0x7fffe8117c90, task=0x7fffdf288060) at /home/src/firefox/ipc/chromium/src/base/message_loop.cc:318 #27 0x00007ffff5187380 in MessageLoop::DeferOrRunPendingTask ( this=<value optimized out>, pending_task=<value optimized out>) at /home/src/firefox/ipc/chromium/src/base/message_loop.cc:326 #28 0x00007ffff51874aa in DoWork (this=0x7fffe8117c90) at /home/src/firefox/ipc/chromium/src/base/message_loop.cc:426 #29 MessageLoop::DoWork (this=0x7fffe8117c90) at /home/src/firefox/ipc/chromium/src/base/message_loop.cc:405 #30 0x00007ffff51876c1 in base::MessagePumpDefault::Run (this=0x7fffdf2b1f00, delegate=0x7fffe8117c90) at /home/src/firefox/ipc/chromium/src/base/message_pump_default.cc:23 #31 0x00007ffff5185e54 in MessageLoop::RunInternal (this=0x7fffe8117c90) at /home/src/firefox/ipc/chromium/src/base/message_loop.cc:208 #32 0x00007ffff5185e7c in RunHandler (this=0x7fffe8117c90) at /home/src/firefox/ipc/chromium/src/base/message_loop.cc:201 #33 MessageLoop::Run (this=0x7fffe8117c90) at /home/src/firefox/ipc/chromium/src/base/message_loop.cc:175 #34 0x00007ffff518b3d2 in base::Thread::ThreadMain (this=0x7fffe1bf0cd0) at /home/src/firefox/ipc/chromium/src/base/thread.cc:156 #35 0x00007ffff5194f08 in ThreadFunc (closure=<value optimized out>) at /home/src/firefox/ipc/chromium/src/base/platform_thread_posix.cc:26 #36 0x0000003ea1807971 in start_thread () from /lib/libpthread.so.0 #37 0x0000003ea0ce6f3d in clone () from /lib/libc.so.6 #38 0x0000000000000000 in ?? ()

(In reply to Karl Tomlinson (:karlt) from comment #26) > (In reply to konstartyom from comment #22) > > I forgot to add that I saw double > > ###!!! ABORT: X_GLXMakeCurrent: GLXBadDrawable: file > > /home/src/firefox/toolkit/xre/nsX11ErrorHandler.cpp, line 190 > > Could glXMakeCurrent be getting called after the window has been destroyed? You are right, Thank you! I'll make a separate patch for this one since it is a different bug.

This patch should be applied on top of the previous one since it (should) fix a different bug. I managed to reproduce the bugs sometimes using MOZ_X_SYNC (thanks karlt) though it is not systematic. As karlt guessed the GLContext was destroyed after the gtk window. So this patch fixes this wrong ordering.

I've tested attachment 619621 [details] [diff] [review], it fixes the crashes for me. Now it is quite usable except these things: 1. WebGL doesn't paint. For example, http://www.paulbrunt.co.uk/steamcube/index.php counts moves when I play the game but I cannot see the cube. http://madebyevan.com/webgl-water/ also doesn't work. These are the webpages I have tested. With omtc or layers acceleration separately the demos work as expected. 2. html5 video and canvas work but blink. Canvas (tested on pdf viewer and http://canvasrider.com/) blinks regardless of gfx.canvas.azure.enabled value. Video is only tested with gstreamer backend on youtube but I think these problems may be related. 3. about:support still shows me that I have no accelerated windows though I have.

(In reply to konstartyom from comment #31) > I've tested attachment 619621 [details] [diff] [review], it fixes the > crashes for me. Awesome. > Now it is quite usable except these things: > 1. WebGL doesn't paint. For example, > http://www.paulbrunt.co.uk/steamcube/index.php counts moves when I play the > game but I cannot see the cube. http://madebyevan.com/webgl-water/ also > doesn't work. These are the webpages I have tested. With omtc or layers > acceleration separately the demos work as expected. > 2. html5 video and canvas work but blink. Yes, I have the same issues on my machine, canvas, webgl and video blink with omtc + layers acceleration. omtc Has not really been implemented for Linux yet. It has been implemented on Mac, and since Linux and Mac share a lot of the same code, it turns out that it kinda works on Linux as a side effect. The two patches I made fix specific crashes but there are plenty of other issues I guess. For example I have sometimes a crash when closing Firefox, because the IPC stuff times out in the tear-down part of the protocol (i'll file a new bug). I think we should open new bugzilla entries for the next bugs instead of putting it all in this one. It would be nice to review and (hopefully) land these two patches in the mean time :)

I agree we can go ahead with the patches here and fix the other issues in other bug reports. I'm keen to review these, and they look like small enough patches, but I also have plenty of other patches to review. As this is not yet a usable feature, it is not as high priority as some of those, but I do intend to get to them.

Minor modification to the gtk widget destruction patch. Makes it less likely to touch a gl context after its gtk window has been deleted.

Comment on attachment 618255 [details] [diff] [review] Fixes crah when using OMTC and layer acceleration on linux. Is there a reason why changes to layers.offmainthreadcomposition.enabled should no longer take effect while the process is running for ports that use that pref? > #include <gtk/gtk.h> > #ifdef MOZ_X11 >+#include <X11/Xlib.h> > #include <gdk/gdkx.h> > #endif /* MOZ_X11 */ I'm a bit puzzled as to why and the way in which this include is added. It is only added for the GTK toolkit but the XInitThreads() call is also for other X11 toolkits. I guess one of the Qt includes already includes this file? But gdkx.h also includes X11/Xlib, so I see no need for the include. >+ // On X11 platforms only use OMTC if firefox was initalized with thread-safe X11 (else it would crash). >+ sUseOffMainThreadCompositing = (PR_GetEnv("MOZ_USE_OMTC")!=NULL); Gecko style is spaces around binary operators. Wrap the comment within 80 columns. >+#else >+ sUseOffMainThreadCompositing = mozilla::Preferences::GetBool("layers.offmainthreadcomposition.enabled", false); Stay within 80 columns.

(In reply to Karl Tomlinson (:karlt) from comment #35) > Comment on attachment 618255 [details] [diff] [review] > Fixes crah when using OMTC and layer acceleration on linux. > > Is there a reason why changes to layers.offmainthreadcomposition.enabled > should no longer take effect while the process is running for ports that use > that pref? Mostly for the sake of simplicity. Jeff told we don't especially need to support being able to switch between omtc and non-omtc without restarting. I think there will be some cases where supporting this will force us to deal with annoying corner cases. For example, I'm thinking about omtc-video: omtc and non-omtc will infer slightly different code paths and setups. Now what happens when you grab a tab containing a video from a omtc window to another (a non-omtc one as we just flipped the pref)? I mean this is not a huge deal but as we don't really need to support switching between omtc and non-omtc, I'd rather get rid of this kind of things already and not worry about corner cases. Especially when these cases may not be obvious to every one since not all platforms can have several compositor threads. > > > #include <gtk/gtk.h> > > #ifdef MOZ_X11 > >+#include <X11/Xlib.h> > > #include <gdk/gdkx.h> > > #endif /* MOZ_X11 */ > > I'm a bit puzzled as to why and the way in which this include is added. > > It is only added for the GTK toolkit but the XInitThreads() call is also for > other X11 toolkits. I guess one of the Qt includes already includes this > file? But gdkx.h also includes X11/Xlib, so I see no need for the include. Right, I removed it. Also pushed to to make sure it doesn't break somewhere. > > >+ // On X11 platforms only use OMTC if firefox was initalized with thread-safe X11 (else it would crash). > >+ sUseOffMainThreadCompositing = (PR_GetEnv("MOZ_USE_OMTC")!=NULL); > > Gecko style is spaces around binary operators. > > Wrap the comment within 80 columns. > > >+#else > >+ sUseOffMainThreadCompositing = mozilla::Preferences::GetBool("layers.offmainthreadcomposition.enabled", false); > > Stay within 80 columns. I fixed these style issues.

(In reply to Nicolas Silva [:nical] from comment #36) > Created attachment 625100 [details] [diff] [review] > Fixes crah when using OMTC and layer acceleration on linux. > > (In reply to Karl Tomlinson (:karlt) from comment #35) > > Comment on attachment 618255 [details] [diff] [review] > > Fixes crah when using OMTC and layer acceleration on linux. > > > > Is there a reason why changes to layers.offmainthreadcomposition.enabled > > should no longer take effect while the process is running for ports that use > > that pref? > > Mostly for the sake of simplicity. Jeff told we don't especially need to > support being able to switch between omtc and non-omtc without restarting. Agreed, we never intended to support switching between the two at runtime.

Comment on attachment 625100 [details] [diff] [review] Fixes crah when using OMTC and layer acceleration on linux. >+ // (called inside gdk_display_open). This is a requirement for off main tread compositing. tread -> thread -+#ifndef MOZ_X11 - pref("layers.offmainthreadcomposition.enabled", false); -+#endif Please restore this #ifndef. It is only confusing to expose a pref that doesn't do anything.

Comment on attachment 621377 [details] [diff] [review] Fixes crash when deleting a Gtk widget with OMTC. > Bug 722012 - Fixes crash on Gtk widget destruction when using OMTC on linux. TheGLContext attached to the ShadodwLayerManager was destroyed after the window was destroyed. The fix concists in invoking the tear-down part of the IPC protocol sooner in the destruction of the widget (only for gtk widgets). Commit messages usually try to include a brief one line summary of the change that the patch makes. If more details are helpful, they can be added in a paragraph after a blank line. TheGLContext - missing space ShadodwLayerManager - extra d concists - consists >+// Defined in nsBaseWidget.cpp >+void DestroyCompositor(layers::CompositorParent* aCompositorParent, >+ layers::CompositorChild* aCompositorChild, >+ base::Thread* aCompositorThread); Functions called across files should be properly exposed in interfaces. The nsBaseWidget class is appropriate here. However, rather than exposing this small part of the destruction process, it would be better to expose a method that performed the whole process, including SendWillStop and .forget(). That way this awkward code is shared instead of duplicated.

karlt, thank you for your reviews. I fixed the first patch following your remarks, I suppose it is ready to land now. I'll modify the second patch to make the destruction sequence cleaner as you said.

Comment on attachment 626010 [details] [diff] [review] Fixes crah when using OMTC and layer acceleration on linux. Review of attachment 626010 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/src/init/all.js @@ +3470,1 @@ > pref("layers.offmainthreadcomposition.enabled", false); i don't think there's any reason to remove this pref. We want it off by default on X11 and having this here will make it show up in about:config as false by default. Otherwise the user will need to type it in.

The pref is ignored on X11 platforms, so it should not appear there. It was only documented in nsAppRunner.cpp, I added a comment in init/all.js to make it clearer.

New version of the second patch, following karlt's remarks. Now the compositor destruction sequence is in nsBaseWidget::DestroyCompositor and the scheduled second phase of the destruction is in DeferredDestroyCompositor (scheduled from DestroyCompositor). nsWindow now only needs to call DestroyCompositor without extra declarations or includes). It is much cleaner now.

Comment on attachment 626032 [details] [diff] [review] Fixes crah when using OMTC and layer acceleration on linux. https://hg.mozilla.org/integration/mozilla-inbound/rev/b2673bce4ba1

Comment on attachment 626089 [details] [diff] [review] Fixes crash when deleting a Gtk widget with OMTC. Yes, much cleaner, thanks. >-static void DestroyCompositor(CompositorParent* aCompositorParent, >+void DeferredDestroyCompositor(CompositorParent* aCompositorParent, Leave "static" for internal linkage here.

Added missing 'static' keyword for DeferredDestroyCompositor.

Comment on attachment 626426 [details] [diff] [review] Fixes crash when deleting a Gtk widget with OMTC. https://hg.mozilla.org/integration/mozilla-inbound/rev/328f316179bd

Re-open until https://hg.mozilla.org/integration/mozilla-inbound/rev/328f316179bd is merged.

We should probably keep it open actually, or create a separate tracking bug for OMTC Linux because there's still some work to do in this area.

Please don't add any further patches to this bug. Using other bugs for additional patches will aid tracking changes and effects.

try push with OMTC, layers acceleration, async-video, async-animation, and async-transparency on Linux to get an idea of the current state of OMTC linux: https://tbpl.mozilla.org/?tree=Try&rev=e83499ca0dca I did not disable xrender, so we may want to try with xrender disabled if this does not show good results.

Do you want help testing that try-build? What should I look for and how can I check that omtc and layers accel is actually enabled?

Ernst Sjostrand, thank you for proposing to help! you can definitely help by testing OMTC on your computer, giving us feedback and filing bugs if you run into any. I am writing a blog post to explain the steps to make sure layers acceleration is working and test OMTC, I will get back to you as soon as it's published.

Sorry for the delay, here is the blog post explaining how to help testing OMTC: http://mozillagfx.wordpress.com/2012/10/06/how-to-help-testing-off-main-thread-compositing/

Following the blog post (comment 56) : i've tested my intel Ivy Bridge HD4000 (Core i3-3225) : Netvibes, Gmail, web pages scrolling, HTML5 video, WebGL : everything works perfectly fine :-) Nightly 19.0a1 (2012-10-09) Debian GNU/Linux Sid 64 bits (GNOME 3.4) with linux-image-3.5.0-5.dmz.1-liquorix-amd64 Mesa 8.0.4-2 xserver-xorg-video-intel 2:2.19.0-6 Thanks !

Work fine too. Intel® Core™2 CPU T5600 Nightly 19.0a1 (2012-10-09) Ubuntu 12.04 64 bits (GNOME 3.4) with linux kernel 3.2.0-31 Mesa DRI Intel(R) 945GM Mesa 8.0.4

Also, someone has reported on linuxfr.org that he did the tests (including WebGL, gmail, youtube, HTML5, some benchmarks…) with success : Intel Sandy Bridge (Intel(R) Core(TM) i5-2410M CPU @ 2.30GHz) Linux 3.5.6-1-ck (Arch Testing) mesa 9.0 xf86-video-intel 2.20.9-2 https://linuxfr.org/nodes/95943/comments/1397284

Well, in fact, it turns out I forgot (I'm the user antistress' comment mentions) to export the MOZ_USE_OMTC variable, so I was basically testing nothing... So, I did notice some changes: flickering when scrolling on gmail (the whole gmail, not just the scrollable part), flickering when clicking opening a mail on gmail (the screen blink between the inbox page and the mail), a really bad case of flickering when watching an html5 video (fullscreen or not). Here is what I noticed: - it flickers more when fullscreen, - it flickers a lot more when the control are shown, - it flickers on scroll (albeit a lot lot less with the second video), - it flickers while paused and I scroll the page. I tried with two videos http://www.mozilla.org/en-US/contribute/ and http://www.mozilla.org/projects/firefox/prerelease.html The second one seems to flicker a lot more than the first (for instance, the second flickers when paused, the first does not, except if I scroll). Flash videos on youtube play smoothly. HTML5 videos on youtube do not.

(In reply to dieppe from comment #60) > [...] > - it flickers on scroll (albeit a lot lot less with the second video), A lot less with the *first* video.

(In reply to antistress from comment #59) > Also, someone has reported on linuxfr.org that he did the tests (including > WebGL, gmail, youtube, HTML5, some benchmarks…) with success : > > Intel Sandy Bridge (Intel(R) Core(TM) i5-2410M CPU @ 2.30GHz) > Linux 3.5.6-1-ck (Arch Testing) > mesa 9.0 > xf86-video-intel 2.20.9-2 > > https://linuxfr.org/nodes/95943/comments/1397284 Same here, sorry i forgot to switch the 3 final options (layers.offmainthreadcomposition.animate-transform, layers.offmainthreadcomposition.animate-opacity & layers.async-video.enabled) - i thought it was for Mac. Lots of flickering for me - both within the web page and for the GUI (tab buttons) Text dispappear and re-appear while i'm writing this comment (and selecting the whole text makes it dispappear) and there is a huge lag between what i type and what i see on the screen Concerning flickering, see for instance the top of the page here http://taskumuro.com/artikkelit/the-story-of-nokia-meego (everything that is above the "The story of Nokia MeeGo" title) Scrolling is not bug-free either and when the page shows some photos,i see their ghost while i scroll (at the top of the page if i scroll toward the top, at the bottom of the page if i scroll toward the bottom) i tried the 2 videos given in comment 60 and i don't have any of the reported bugs on the 1st one (except *), whereas i get web page and video controls flickering with the 2nd one while moving around the mouse pointer (but not for the video itself which remains rock solid). * For both videos i get controls flickering while i mouse over "sound" or "(un)maximize" button, no matter if the video is maximized or not. Flickering on Gmail. What is strange is that on the main view, its not the wole page which os flickering but only part of it as if it was on whole block : i have painted it in red on the attached screenshot Flickering on Netvibes while i mouse over blocks On the new home page, thumbnails are also flickering if i mouse over them WebGL is fine, fullscreen or not (http://media.tojicode.com/q3bsp/) except the beginning of Flight of the Navigator : flickering on the text that shows credits I hope it helps

It's best not to report success or failure in this bug right here, but to report new bug blocking this one when you encounter issues with OMTC that you don't see without it. Before that, of course, first check the existing dependencies here if your issue is already filed.

Will this benefit Qt based XUL Fennec with EGL rendering by default, or some extra work is required to share this effort?

I am reducing the amount of bugs I am assigned to, to better reflect what I am actively working on. These days I can't allocate time on Linux OMTC. I'm still very happy to help with reviews in this area.

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

With Manjaro (Arch based) Linux, I also have horrible flickering, in chrome as well as various web sites. With the settings enabled for OMTC the browser becomes unusable. System: Kernel: 3.15.8-1-MANJARO x86_64 (64 bit) Desktop: Xfce 4.11.6git-UNKNOWN Distro: ManjaroLinux 0.8.10 Ascella Machine: System: Gigabyte product: N/A Mobo: Gigabyte model: 990XA-UD3 Bios: American Megatrends v: FB date: 12/07/2012 CPU: Octa core AMD FX-8320 Eight-Core (-MCP-) cache: 16384 KB Clock Speeds: 1: 1400 MHz 2: 1400 MHz 3: 1400 MHz 4: 1400 MHz 5: 1400 MHz 6: 1400 MHz 7: 1400 MHz 8: 1400 MHz Graphics: Card: Advanced Micro Devices [AMD/ATI] Pitcairn XT [Radeon HD 7870 GHz Edition] Display Server: X.Org 1.15.2 driver: fglrx Resolution: 1920x1080@60.00hz GLX Renderer: AMD Radeon HD 7800 Series GLX Version: 4.4.12874 - CPC 14.10.1006.1001

Any news on this? Linux is the only platform where this hasn't shipped.

Actively working on it, don't have the ETA to give you.

I have layers.offmainthreadcomposition.enabled and layers.acceleration.force-enabled for several weeks already and putting aside small glitches like occasional repaint area blackening (rarely) its works fine. No crashes. Ubuntu 14.10 Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0 GeForce GTX 750 Ti/PCIe/SSE2 NVIDIA Driver 331.113

OMTC is enabled on Linux since Fireofx 40.

Alright! Thank you very much. I don't understand what this means, only that it is a step towards Firefox not being terribly slow in my favorite OS.

Hi. I just realised that default settings in my Firefox 43 (linux) for both layers.offmainthreadcomposition.enabled and layers.acceleration.force-enabled were false. I turn them true and I had serious CPU usage reduction in youtube videos. Are they suppose to be false in stable releases and enabled only in Nightly / Dev? Thanks.

layers.offmainthreadcomposition.enabled is on by default, on all trains. layers.acceleration.force-enabled is false on all trains. But, yes, if it works for you, forcing the acceleration would drop the CPU usage. It will be on by default on Linux once bug 594876 is cleared.