760675 - Avoid using a global GL context on Android (we'll see later if we have to on some devices)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

*** 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.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: don't create a global context at all, on Android

This is emporary patch until we properly detect devices that require it for sharing because they don't support EGLImage well enough.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: update adreno blacklisting to not use the global context

!!!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.

Comment 3

[Oleg Romashin (:romaxa)]

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?

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 5

[Oleg Romashin (:romaxa)]

Hmm, interesting why on Tegra3 it is working only partially and crashes after 1 minute of rendering via EGLimage...

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

We could ask NVIDIA about that! Give me all the background info if you would like me to do that.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aba541097303
https://hg.mozilla.org/mozilla-central/rev/7f2c0baa1c93

Comment 9

[Mark Callow]

(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?

Comment 10

[Oleg Romashin (:romaxa)]

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 11

[Benoit Jacob [:bjacob] (mostly away)]

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 12

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

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 14

[Joe Drew (not getting mail)]

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 15

[Joe Drew (not getting mail)]

Comment on attachment 629349 [details] [diff] [review]
update adreno blacklisting to not use the global context

Please land on both aurora and beta.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/releases/mozilla-beta/rev/57330491c355
http://hg.mozilla.org/releases/mozilla-beta/rev/2ce4c05f17d6

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Note: this has already been uplifted to aurora.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Benoit, you said keep open in comment 6, is that still true? If so, what work is needed to close it out.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

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.