Closed Bug 741689 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Graphics, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: m1, Assigned: jgilbert)

Details

Attachments

(1 file)

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.
Severity: normal → major
OS: Mac OS X → Gonk
Priority: -- → P2
Hardware: x86 → ARM
Component: General → Graphics
gfx folks, is there any reason we need to fail when the GL_EXT* variants aren't present?
This sounds like a bug, but over to Jeff who knows the multisample stuff best.
Assignee: nobody → jgilbert
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.
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.
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
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.
/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
No worries, I still owe you a much bigger one :-)
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Also some cleanup.
Attachment #612010 - Flags: review?(bjacob)
Attachment #612010 - Flags: review?(bjacob) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: