741689 - GLContext init failure when ANGLE and !EXT fb extensions are present

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Michael Vines [:m1] [:evilmachines]]

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
conditionals:

https://github.com/cgjones/mozilla-central/blob/master/gfx/gl/GLContext.cpp#L399
https://github.com/cgjones/mozilla-central/blob/master/gfx/gl/GLContext.cpp#L411

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

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

gfx folks, is there any reason we need to fail when the GL_EXT* variants aren't present?

Comment 2

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

This sounds like a bug, but over to Jeff who knows the multisample stuff best.

Comment 3

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

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

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

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

[Michael Vines [:m1] [:evilmachines]]

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

Thanks,
Michael

Comment 6

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

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

[Michael Vines [:m1] [:evilmachines]]

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


[1] https://www.codeaurora.org/gitweb/quic/la/?p=platform/frameworks/base.git;a=tree;f=opengl/libs/GLES2;hb=ics

Comment 8

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

No worries, I still owe you a much bigger one :-)

Comment 9

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: Disable extensions when their functions are not properly exposed

Also some cleanup.

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a06c3ef2c1b

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3a06c3ef2c1b