The Intel X driver on Ubuntu 10.04 only claims support for GLX 1.2, but it also claims support for extensions (in particular, GLX_ARB_get_proc_address and GLX_SGIX_fbconfig) that are equivalent to the features we use in 1.3 and 1.4. I have a series of patches in my patch queue to do the necessary function pointer lookup to get the pointers to the extension function names instead of the standard ones. With these patches plus the patches in my patch queue for bug 578877, the demos at http://www.khronos.org/webgl/wiki/Demo_Repository are working on my laptop. (Just dropping the version check doesn't work; it complains that the driver doesn't support the necessary version.) My current work-in-progress is here: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/gl-version-checks http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/duplicate-xfree http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/destroy-context-pointer http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/remove-createcontext http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/remove-glxlibrary-unused http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/add-glxlibrary-extensions http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/glxlibrary-no-repeat http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ee900fab0516/glx-extensions but a bunch of these need a bit of cleanup. I probably won't get to doing so today since I've already spent 3 hours more on this than I intended to.

I noticed this, which seems wrong since cfgs is a ScopedXFree<GLXFBConfig>.

This inconsistency was confusing when I was trying to sort through lists of functions.

I don't think this code is used since we bail much earlier if the GLX version is < 1.3, but my later patch will allow us to run sometimes in that case, and I don't think we want this code for that.

Remove unused functions, so there was less to do when categorizing functions by their GLX version.

It seems bad to repeatedly try initializing if we've failed before.

This is the "main" patch in this bug. That said, it turns out considerably less of this was actually needed to get running on my Intel card, though that minimum doesn't seem safe for dealing with a variety of drivers. All I actually needed to change to get running, it turns out (though I only discovered this after writing the whole patch series), was: @@ GLXLibrary::EnsureInitialized() gGLXVersion = PR_MIN(clientVersion, serverVersion); - if (gGLXVersion < 0x0103) + if (gGLXVersion < 0x0102) return PR_FALSE; gIsATI = vendor && DoesVendorStringMatch(vendor, "ATI"); @@ GLContextProviderGLX::CreateForWindow(ns int numConfigs; ScopedXFree<GLXFBConfig> cfgs; - if (gIsATI) { + if (1) { const int attribs[] = { GLX_DOUBLEBUFFER, False, 0 }; cfgs = sGLXLibrary.xChooseFBConfig(display, though that led to getting the following warning when I ran WebGL demos: WARNING: Application calling GLX 1.3 function "glXCreatePixmap" when GLX 1.3 is not supported! This is an application bug! So I suspect the GLX library has some handling for extensions, or the Intel X driver actually supports some of the real GLX 1.3 functions, or maybe there's binary-compatibility in the protocol plus no version checking in the library, or something. If I figure out what was actually going on to make things work with fewer changes, I could perhaps shrink this patch a bit. That said, I think the approach here is reasonable, although somewhat painful -- just switch to using extension functions when the functions we want are not available according to the version check, but the extension functions are available.

This is (like patch 1) independent of the main point of the bug. The version checking code scared me since it clearly wouldn't work with a version "1.10" (which seems permitted according to the GLX spec). This simplifies the version checking code by using glXQueryVersion and makes it a bit more robust.

Looks good from a quick glance, but would rather have Benoit look over these -- cc'ing him here, bjacob can you just steal the review requests from me here?

Comment on attachment 485377 [details] [diff] [review] patch 4: remove unused functions from GLXLibrary Identical to part 3?

Comment on attachment 485386 [details] [diff] [review] patch 8: fix/simplify GLX version checking Why don't we need to check the client GLX version any more? glXQueryVersion appears to be identical to glXQueryServerString(.., GLX_VERSION). Can we also get rid of serverVersionStr?

Comment on attachment 485379 [details] [diff] [review] patch 6: don't repeatedly try to initialize GLXLibrary >+ PRBool mTriedInitializing; Initialize this in the constructor?

(In reply to comment #10) > Comment on attachment 485377 [details] [diff] [review] > patch 4: remove unused functions from GLXLibrary > > Identical to part 3? Oops, looks like I attached the wrong patch for one of them. (In reply to comment #11) > Comment on attachment 485386 [details] [diff] [review] > patch 8: fix/simplify GLX version checking > > Why don't we need to check the client GLX version any more? The documentation I read made it sound like it was the minimum of client and server versions: http://www.opengl.org/documentation/specs/glx/glx1.3.pdf > glXQueryVersion appears to be identical to glXQueryServerString(.., > GLX_VERSION). > Can we also get rid of serverVersionStr? No, since we look for "Chromium" in it below. (In reply to comment #12) > Comment on attachment 485379 [details] [diff] [review] > patch 6: don't repeatedly try to initialize GLXLibrary > > > >+ PRBool mTriedInitializing; > > Initialize this in the constructor? Will do. (Probably doesn't matter though since static storage is null-initialized.)

(Or should I send these review requests to mattwoodrow?)

> > Why don't we need to check the client GLX version any more? > > The documentation I read made it sound like it was the minimum of client and > server versions: http://www.opengl.org/documentation/specs/glx/glx1.3.pdf > Interesting, I was looking at http://www.opengl.org/sdk/docs/man/xhtml/glXQueryVersion.xml which only mentions the server version. I'd probably trust the spec pdf slightly more though.

It's definitely the minimum of the two. mattwoodrow is also fine for reviews, just would rather someone who uses X frequently review them :-)

Comment on attachment 485377 [details] [diff] [review] patch 4: remove unused functions from GLXLibrary r+ provided, of course, that it still builds :) (if it build without these symbols then by definition they were useless :)

Comment on attachment 485378 [details] [diff] [review] patch 5: add glXQueryExtensionsString to GLXLibrary r+, provided that this actually gets used in a subsequent patch

Comment on attachment 485384 [details] [diff] [review] patch 7: use extension alternatives for GLX 1.3 features r+ provided that: * you factor the code of your HasExtension() with the existing GLContext::IsExtensionSupported() * you have tested this on GLX 1.2, as I don't have the GLX knowledge to judge if that is going to work.

http://hg.mozilla.org/mozilla-central/rev/a9fef34b8c68 http://hg.mozilla.org/mozilla-central/rev/0c05af8eb212 http://hg.mozilla.org/mozilla-central/rev/83c59637f799 http://hg.mozilla.org/mozilla-central/rev/bf1af01820f8 http://hg.mozilla.org/mozilla-central/rev/c8d35a82db4e http://hg.mozilla.org/mozilla-central/rev/8b500020522c http://hg.mozilla.org/mozilla-central/rev/8b83d833cc95 http://hg.mozilla.org/mozilla-central/rev/f989d1ef7cb8

One other thing I found: I tried using accelerated layers, and to get it to work I need to make the other gIsATI conditions in GLContextProviderGLX::CreateForWindow match the one that I changed to use sGLXLibrary.xChooseFBConfig rather than sGLXLibrary.xGetFBConfigs. (I made that change because glXGetFBConfigs doesn't exist in 1.2.) Do those three conditions inherently need to match? (It's clear the latter two need to match each other; it's not clear to me that there's something inherent about the first needing to match the latter two.) Is the right thing to (a) change the other two if (gIsATI) to use the same condition as the one I changed, if (gIsATI || !GLXVersionCheck(1, 3)) or (b) to change the latter to to use an ATI-or-Intel test or (c) to use the gIsATI codepath unconditionally?