909 bytes, patch
|Details | Diff | Splinter Review|
3.74 KB, patch
|Details | Diff | Splinter Review|
*** Current state of things *** Currently we always create a global context on all platforms for texture sharing. We don't currently make use of it on Android, as we don't currently do texture sharing, but we plan to start doing that soon in bug 728524. *** Problems with the global context *** Using a global context has multiple disadvantages: 1. it's by itself another context and some devices have a very low global limit on the number of live contexts overall (like 8). So it can make us fail to use OpenGL. 2. On Tegra 2, context share groups are very poorly supported and avoiding using a global context and share groups is a requirement to get WebGL to work (bug 759225). 3. (Hypothetical) We have had strange context creation crashes in bug 736123 which I can't reproduce in my builds that disable the global context. So it seems that one of the Adreno bugs we hit there had to do with share groups. We still won't be able to enable WebGL on these Adreno drivers though due to the texSubImage2D bug discussed in bug 736123. *** Why we can't always avoid the global context *** Without a global context and share groups, to share textures (required for fast WebGL), we will need EGLImage. Unfortunately, as bug 728524 comment 31 shows, EGLImage is too poorly supported to be usable on certain devices. *** The plan *** So the plan is to use either EGLImage or context sharing depending on the driver. We will have to detect the driver before making this choice, as context sharing must be decided at context creation time. This is nontrivial as the driver can only be known by already having a GL context and calling glGetString(GL_RENDERER) etc. To solve this problem, we should edit the Java code to, spawn a small thread that will create a GL context and query GL strings, and post them as a message to the Gecko thread. This should be done at about the same time as when we spawn the Gecko thread itself. So this is in effect similar to the glxtest process we're using on X11, except it's just a thread instead of a process, as AFAIK we don't have context creation crashes to dodge here. *** The very-short-term plan *** The above is already a short-term plan as it shouldn't be hard at all. Say next week. It's a blocker for bug 728524. But even before that, as bug 728524 hasn't landed yet, I would like to go ahead with the part that will be enough to allow running WebGL on Tegra 2. That's in particular a blocker for bug 759221. This means: temporarily, until we have the mechanism to obtain the GL strings and decide when to use either approach, just disable the global context and share groups.
This is emporary patch until we properly detect devices that require it for sharing because they don't support EGLImage well enough.
Attachment #629347 - Flags: review?(jmuizelaar)
!!!Note: this patch is required otherwise you'll get crashes!!! The current Adreno blacklists hard-asserts (even in release builds) that there is a global context, as it needs it to get renderer info. That was done as, at the time (bug 736123) there were GL context creation crashes there. However, without share groups, I can't reproduce the context creation crashes anymore, so this approach doesn't seem to be needed anymore. If we were unlucky and there still were context creation crashes, the solution would be the java thread thing mentioned in comment 0, anyway.
Attachment #629349 - Flags: review?(jmuizelaar)
is there are any android device/ drivers/chip configuration where EGLImage works properly as expected? which I can use for reference implementation of shared webgl textures?
For example, NVIDIA explicitly recommended us to use EGLImage for sharing the texture used to back a WebGL context, on the Tegra 2 drivers where we can't use share groups.
Hmm, interesting why on Tegra3 it is working only partially and crashes after 1 minute of rendering via EGLimage...
We could ask NVIDIA about that! Give me all the background info if you would like me to do that.
Attachment #629349 - Flags: review?(jmuizelaar) → review+
Attachment #629347 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba541097303 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2c0baa1c93 Keeping open until the Java-thread-querying-GL-strings patch is done. For the same reason, marking target milestone = 16. However, I wanted to land these 2 patches ASAP as they make WebGL work on Tegra 2 devices.
Assignee: nobody → bjacob
Whiteboard: Keep open!
Target Milestone: --- → mozilla16
(In reply to Oleg Romashin (:romaxa) from comment #5) > Hmm, interesting why on Tegra3 it is working only partially and crashes > after 1 minute of rendering via EGLimage... Maybe this is the reason: "The downside of EGLImage is that the app is fully responsible for synchronization if the EGLImage is accessed from 2 contexts concurrently. If you do need to access the EGLImage concurrently from 2 threads the EGL_KHR_fence_sync extension may be useful." Do you have concurrent accesses? If so, are you synchronizing them?
I found reason of crash, I created another version with glEGLImageTargetRenderbufferStorageOES, and it does not crash any more. but I cannot get it working without flickering... even after adding fence sync + hard mutex protected access to GLContext (After/Before Read/Draw Bind/Unbind) I still see flickering... less often but it still visible.
Comment on attachment 629347 [details] [diff] [review] don't create a global context at all, on Android [Approval Request Comment] Bug caused by (feature/regressing bug #): since OMTC landed (the Maple branch merge, a couple months ago) User impact if declined: no WebGL on many Android devices (the Tegra and Tegra2-based devices) Testing completed (on m-c, etc.): m-c since yesterday, tested in Nightly today Risk to taking this patch (and alternatives if risky): very little risk. This is a deep change that makes us use a different path in the driver, but this change goes in the direction of doing simpler, more standard stuff, hence it makes us run into fewer driver bugs (The bug at hand here, in particular, is a driver bug that we no longer hit with this fix). String or UUID changes made by this patch: none
Comment on attachment 629349 [details] [diff] [review] update adreno blacklisting to not use the global context [Approval Request Comment] Bug caused by (feature/regressing bug #): this patch needs to be landed together with the other one here User impact if declined: can't land the other patch without landing this one, Fennec would just abort Testing completed (on m-c, etc.): m-c since yesterday, Nightly today Risk to taking this patch (and alternatives if risky): very low risk. Worst case, we would run again into Adreno crashes as in bug 736123 but I've tested on a HTC Desire and it was not crashing. String or UUID changes made by this patch: none.
Here is a new reply from NVIDIA, OK'd to share publicly here, about the merits of using EGLImage as opposed to a share group: The EGLImage solution may be your best bet even on versions that support context sharing. When two sharing contexts are simultaneously bound to 2 different threads the GL driver needs to do extra work to ensure correctness (this is because GL context sharing is very course granularity and potentially any object could be used from both contexts/threads concurrently). In contrast, the EGLImage is fine grained and optimized to avoid overhead. So avoiding context sharing with multithreading (and using EGLImage instead) may be better for performance. Note that this overhead can exist if the context has *ever* been current at the same time as a sharing context was current in a different thread (even if only 1 context is current in only 1 thread now). The complication with using EGLImage is that the app is fully responsible for synchronization if the EGLImage is accessed from 2 contexts concurrently. If you do need to access the EGLImage concurrently from 2 threads the EGL_KHR_fence_sync extension may be useful.
Comment on attachment 629347 [details] [diff] [review] don't create a global context at all, on Android Please land on both aurora and beta.
Comment on attachment 629349 [details] [diff] [review] update adreno blacklisting to not use the global context Please land on both aurora and beta.
Note: this has already been uplifted to aurora.
Benoit, you said keep open in comment 6, is that still true? If so, what work is needed to close it out.
I've filed bug 766251 for the remaining work, which consists in implementing proper GfxInfo data sources on Android, allowing to decide between EGLImage and share groups as needed. Closing this bug now.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Summary: Avoid using a global GL context on Android, except where really necessary (where EGLImage can't be used to share textures) → Avoid using a global GL context on Android (we'll see later if we have to on some devices)
You need to log in before you can comment on or make changes to this bug.