It would be nice to have a (non-default!!) mode where WebGL does not use any work-arounds for driver bugs. One could add a new pref, webgl.use-workarounds, defaulting to true, in modules/libpref/src/init/all.js and then query it during WebGL context initialization, and only use our work-arounds if it's true, for example in WebGLContextGL.cpp we have // work around Mac OSX crash, see bug 631420 #ifdef XP_MACOSX if (mAttribBuffers[0].enabled && !mCurrentProgram->IsAttribInUse(0)) return VertexAttrib0Status::EmulatedUninitializedArray; #endif and we would only want to do that if this new "use_workarounds" variable is true.

Repurposing: this should be a general no-gfx-driver-workarounds mode, not specific to WebGL. Indeed, it happens that we move some WebGL workaround upstream to GLContext.

Also, I am very, very angry about the fact that a certain proprietary OS vendor, who is our main cause of trouble in WebGL, is not caring at all about our bug reports, and I want to have the ability to shame them publicly if they don't start caring soon. So, this is becoming a priority.

Hey Bas, the above patch introduces the gfx.work-around-driver-bugs pref but only really implements it in GLContext and WebGLContext. If there are any D3D driver workaround you're doing, you may want to make a follow-up patch here.

Note that the "In no-workarounds mode, also don't work around NPOT FBO bug" patch depends on the "kill USE_GLES2" patch in bug 741730

Comment on attachment 613068 [details] [diff] [review] Dear XXX, here are all your driver bugs. Love, Mozilla Review of attachment 613068 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +298,5 @@ > } > } > } > > + mWorkAroundDriverBugs = Preferences::GetBool("gfx.work-around-driver-bugs", true); With OMTC, we're creating a GL context on the compositor thread. But preferences should only be accessed on the main thread. So I think we need a better way to do this (e.g. we could follow the approach that LayerManagerOGL::Initialize uses to read the FPS counter preference).

Give me one place that's called on the main thread before any GL work is done? LayerManagerOGL isn't such a place, it's not currently used e.g. on Windows and Linux.

(In reply to Benoit Jacob [:bjacob] from comment #10) > Give me one place that's called on the main thread before any GL work is > done? LayerManagerOGL isn't such a place, it's not currently used e.g. on > Windows and Linux. If we want a single place, nsBaseWidget's constructor should work. Alternatively, we could do this check in the same places we read the layers acceleration prefs -- in GetLayerMangerPrefs in widget/windows/nsWindow.cpp, and in nsBaseWidget::GetShouldAccelerate.

The gfxPlatform singleton seemed like a nice home for this boolean, and gfxPlatform initialization like a nice place to read that pref, as other prefs are already being read from there.

Comment on attachment 613132 [details] [diff] [review] Dear XXX, here are all your driver bugs. Love, Mozilla Looks good to me.

oops, forgot to refresh that patch.

Comment on attachment 613137 [details] [diff] [review] Dear XXX, here are all your driver bugs. Love, Mozilla carrying over r+ from ajuma

Try is green.

Comment on attachment 613137 [details] [diff] [review] Dear XXX, here are all your driver bugs. Love, Mozilla Review of attachment 613137 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextGL.cpp @@ +2647,5 @@ > // See comment in ValidateProgram below. > + if (gl->WorkAroundDriverBugs()) > + i = 1; > + else > + gl->fGetProgramiv(progname, pname, &i); {} @@ +4517,5 @@ > + if (gl->WorkAroundDriverBugs()) { > + const PRUint32 maxSourceLength = (PRUint32(1)<<18) - 1; > + if (sourceCString.Length() > maxSourceLength) > + return ErrorInvalidValue("compileShader: source has more than %d characters", > + maxSourceLength); Is it true that GL has no defined limits on shader length? ::: modules/libpref/src/init/all.js @@ +244,5 @@ > #ifdef ANDROID > pref("gfx.textures.poweroftwo.force-enabled", false); > #endif > > +pref("gfx.work-around-driver-bugs", true); This will need to be added to b2g and mobile as well, AIUI

Comment on attachment 613110 [details] [diff] [review] In no-workarounds mode, also don't work around NPOT FBO bug Review of attachment 613110 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/LayerManagerOGL.cpp @@ +324,1 @@ > mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0); Should this be inside the if (WorkAroundDriverBugs)?

Comment on attachment 613137 [details] [diff] [review] Dear XXX, here are all your driver bugs. Love, Mozilla Review of attachment 613137 [details] [diff] [review]: ----------------------------------------------------------------- R+, since the nits are just a question and a suggestion. ::: gfx/gl/GLContext.h @@ +1653,5 @@ > // Useful for resizing offscreen buffers. > public: > void ClearSafely(); > > + bool WorkAroundDriverBugs() const { return mWorkAroundDriverBugs; } Since we need to define this for non-protected use, why not use this everywhere? If we ever want to add logic to this, it'll be a pain since most of our uses internal to GLContext use the member var, instead of the function. @@ -1683,5 @@ > PRInt32 maxAllowed = NS_MIN(mMaxRenderbufferSize, mMaxTextureSize); > return biggerDimension <= maxAllowed; > } > > -protected: Just checking that this was removed because it is redundant.

https://hg.mozilla.org/integration/mozilla-inbound/rev/469e2e5b18e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/971f7705b978

https://hg.mozilla.org/mozilla-central/rev/469e2e5b18e5 https://hg.mozilla.org/mozilla-central/rev/971f7705b978

/home/smaug/mozilla/hg/mozilla/gfx/layers/opengl/LayerManagerOGL.cpp:265:7: error: non-constant-expression cannot be narrowed from type 'int' to 'GLenum' (aka 'unsigned int') in initializer list

Sorry I've landed this without looking at the review comments :-( (In reply to Joe Drew (:JOEDREW!) from comment #19) > Comment on attachment 613110 [details] [diff] [review] > In no-workarounds mode, also don't work around NPOT FBO bug > > Review of attachment 613110 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/opengl/LayerManagerOGL.cpp > @@ +324,1 @@ > > mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0); > > Should this be inside the if (WorkAroundDriverBugs)? Apparently not, since the old code ended with an unconditional: // back to default framebuffer, to avoid confusion mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0); which my patch removed. (In reply to Jeff Gilbert [:jgilbert] from comment #20) > Comment on attachment 613137 [details] [diff] [review] > Dear XXX, here are all your driver bugs. Love, Mozilla > > Review of attachment 613137 [details] [diff] [review]: > ----------------------------------------------------------------- > > R+, since the nits are just a question and a suggestion. > > ::: gfx/gl/GLContext.h > @@ +1653,5 @@ > > // Useful for resizing offscreen buffers. > > public: > > void ClearSafely(); > > > > + bool WorkAroundDriverBugs() const { return mWorkAroundDriverBugs; } > > Since we need to define this for non-protected use, why not use this > everywhere? If we ever want to add logic to this, it'll be a pain since most > of our uses internal to GLContext use the member var, instead of the > function. You're right, I've hesitated a bit but this is better practice. Would make a good follow-up patch. > > @@ -1683,5 @@ > > PRInt32 maxAllowed = NS_MIN(mMaxRenderbufferSize, mMaxTextureSize); > > return biggerDimension <= maxAllowed; > > } > > > > -protected: > > Just checking that this was removed because it is redundant. Yes, it was redundant.