Last Comment Bug 741689 - GLContext init failure when ANGLE and !EXT fb extensions are present
: GLContext init failure when ANGLE and !EXT fb extensions are present
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
P2 major (vote)
: mozilla14
Assigned To: Jeff Gilbert [:jgilbert]
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2012-04-02 22:07 PDT by Michael Vines [:m1] [:evilmachines]
Modified: 2012-04-05 11:24 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Disable extensions when their functions are not properly exposed (8.71 KB, patch)
2012-04-03 16:27 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description User image Michael Vines [:m1] [:evilmachines] 2012-04-02 22:07:15 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.109 Safari/535.19

Steps to reproduce:

Tried to run B2G on a device with a particular OpenGL implementation

Actual results:

GLContext init fell down because this OpenGL implementation provides GL_ANGLE_framebuffer_multisample and
GL_ANGLE_framebuffer_blit, but NOT
GL_EXT_framebuffer_multisample and GL_EXT_framebuffer_blit.

There are a couple places in gecko GL initialization there the code
blocks that assume that ANGLE *and* EXT are either both present or
not, even though those blocks are wrapped by ANGLE *or* EXT

This causes GL initialization to fail as the glBlitFramebufferEXT and
glRenderbufferStorageMultisampleEXT symbols are not found.

Expected results:

GLContext init should not have failed.
Comment 1 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-03 10:06:15 PDT
gfx folks, is there any reason we need to fail when the GL_EXT* variants aren't present?
Comment 2 User image Benoit Jacob [:bjacob] (mostly away) 2012-04-03 10:10:28 PDT
This sounds like a bug, but over to Jeff who knows the multisample stuff best.
Comment 3 User image Benoit Jacob [:bjacob] (mostly away) 2012-04-03 10:11:43 PDT
To be clear, these extensions are only used for WebGL anti-aliasing, so you could make a quick and dirty patch ignoring them and not trying to do WebGL anti-aliasing.
Comment 4 User image Benoit Jacob [:bjacob] (mostly away) 2012-04-03 10:34:46 PDT
Michael, this code:

if (IsExtensionSupported(GLContext::ANGLE_framebuffer_blit) ||
            IsExtensionSupported(GLContext::EXT_framebuffer_blit)) {
            SymLoadStruct auxSymbols[] = {
                    { (PRFuncPtr*) &mSymbols.fBlitFramebuffer, { "BlitFramebuffer", "BlitFramebufferEXT", "BlitFramebufferANGLE", NULL } },
                    { NULL, { NULL } },
            if (!LoadSymbols(&auxSymbols[0], trygl, prefix)) {
                NS_RUNTIMEABORT("GL supports framebuffer_blit without supplying glBlitFramebuffer");
                mInitialized = false;

Is not meant to require both BlitFramebufferEXT and BlitFramebufferANGLE symbols. The symbol name list passed to LoadSymbols() is supposed to mean "try these symbols, use the first one that works". It's not supposed to mean "require all these symbols to exist".

This LoadSymbols function is implemented in GLLibraryLoaded.cpp, and looking at the code, it seems to be indeed doing that.

Can you give more details about why you think it's requiring both symbols to exist? Maybe running this code in GDB, stepping into LoadSymbols, will allow to understand what's happening.
Comment 5 User image Michael Vines [:m1] [:evilmachines] 2012-04-03 13:11:16 PDT
Hi Benoit,

I didn't step through this in GDB like you requested but I think it's pretty clear what's happening by code inspection:

1) LoadSymbols() fails to resolve the *EXT symbols.  failCount++.
2) The last line of LoadSymbols():
    return failCount == 0 ? true : false;
3) Upon return we then hit the NS_RUNTIMEABORT()

Comment 6 User image Benoit Jacob [:bjacob] (mostly away) 2012-04-03 13:51:04 PDT
It seems to me that failCount++; only happens when none of the symbols is found. Indeed it comes after the end of the loop,

   for (int i = 0; i < MAX_SYMBOL_NAMES; i++)

and is only reached if (*ss->symPointer == 0) after then end of that loop, which means that none of the symbols is found.
Comment 7 User image Michael Vines [:m1] [:evilmachines] 2012-04-03 14:45:37 PDT
/me shamed for not looking closer

Okay, I think I know what's *actually* going on now.   While the glBlitFramebufferANGLE symbol exists in the adreno200 GLES2 library and it reports support for it, the Android libEGL wrapper library does not currently contain a gl2.cpp shim for glBlitFramebufferANGLE [1].  This is a little bit disturbing, in that it seems like we cannot fully trust what glGetString(GL_EXTENSIONS) tells us.

Comment 8 User image Benoit Jacob [:bjacob] (mostly away) 2012-04-03 15:01:32 PDT
No worries, I still owe you a much bigger one :-)
Comment 9 User image Jeff Gilbert [:jgilbert] 2012-04-03 15:55:44 PDT
According to comment 7, if we cannot trust the extension string, we can, instead of ABORTing when we don't find the symbols, we can mark the extension as unsupported at runtime.

I can take this.
Comment 10 User image Jeff Gilbert [:jgilbert] 2012-04-03 16:27:39 PDT
Created attachment 612010 [details] [diff] [review]
Disable extensions when their functions are not properly exposed

Also some cleanup.

Note You need to log in before you can comment on or make changes to this bug.