686735 - Implement no-gfx-driver-workarounds mode

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

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

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.

Comment 1

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

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.

Comment 2

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

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.

Comment 3

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

Attachment: Dear XXX, here are all your driver bugs. Love, Mozilla

Comment 4

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

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.

Comment 5

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

Attachment: Dear

Comment 6

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

Attachment: Dear XXX, here are all your driver bugs. Love, Mozilla

Comment 7

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

Attachment: In no-workarounds mode, also don't work around NPOT FBO bug

Comment 8

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

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 9

[Ali Juma [:ajuma]]

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

Comment 10

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

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.

Comment 11

[Ali Juma [:ajuma]]

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

Comment 12

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

Attachment: Dear XXX, here are all your driver bugs. Love, Mozilla

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 13

[Ali Juma [:ajuma]]

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

Looks good to me.

Comment 14

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

Attachment: Dear XXX, here are all your driver bugs. Love, Mozilla

oops, forgot to refresh that patch.

Comment 15

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

Comment on attachment 613137 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

carrying over r+ from ajuma

Comment 16

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

https://tbpl.mozilla.org/?tree=Try&rev=7927b837b0dd

Comment 17

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

Try is green.

Comment 18

[Joe Drew (not getting mail)]

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 19

[Joe Drew (not getting mail)]

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 20

[Kelsey Gilbert [:jgilbert]]

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.

Comment 21

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

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

Comment 22

[Matt Brubeck (:mbrubeck)]

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

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

/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

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/8eb367e5b053

Comment 25

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

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.