Last Comment Bug 686735 - Implement no-gfx-driver-workarounds mode
: Implement no-gfx-driver-workarounds mode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla14
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-14 12:07 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-09-27 15:26 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Dear XXX, here are all your driver bugs. Love, Mozilla (13.65 KB, patch)
2012-04-06 21:07 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
Dear (14.18 KB, text/plain)
2012-04-06 21:22 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
Dear XXX, here are all your driver bugs. Love, Mozilla (14.18 KB, patch)
2012-04-06 21:22 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
In no-workarounds mode, also don't work around NPOT FBO bug (6.05 KB, patch)
2012-04-07 08:03 PDT, Benoit Jacob [:bjacob] (mostly away)
joe: review+
Details | Diff | Review
Dear XXX, here are all your driver bugs. Love, Mozilla (15.53 KB, patch)
2012-04-07 12:47 PDT, Benoit Jacob [:bjacob] (mostly away)
ajuma.bugzilla: review+
Details | Diff | Review
Dear XXX, here are all your driver bugs. Love, Mozilla (16.51 KB, patch)
2012-04-07 14:19 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
jacob.benoit.1: review+
joe: review+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-09-14 12:07:10 PDT
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) 2012-04-06 20:13:32 PDT
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) 2012-04-06 20:15:05 PDT
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) 2012-04-06 21:07:04 PDT
Created attachment 613066 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-04-06 21:12:53 PDT
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) 2012-04-06 21:22:25 PDT
Created attachment 613067 [details]
Dear
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-04-06 21:22:58 PDT
Created attachment 613068 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-04-07 08:03:24 PDT
Created attachment 613110 [details] [diff] [review]
In no-workarounds mode, also don't work around NPOT FBO bug
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-04-07 08:04:32 PDT
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] 2012-04-07 10:59:48 PDT
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) 2012-04-07 11:04:34 PDT
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] 2012-04-07 11:14:41 PDT
(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) 2012-04-07 12:47:16 PDT
Created attachment 613132 [details] [diff] [review]
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] 2012-04-07 13:42:40 PDT
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) 2012-04-07 14:19:32 PDT
Created attachment 613137 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

oops, forgot to refresh that patch.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-04-07 14:20:00 PDT
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) 2012-04-07 14:20:59 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7927b837b0dd
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-04-08 13:49:40 PDT
Try is green.
Comment 18 Joe Drew (not getting mail) 2012-04-08 18:03:09 PDT
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) 2012-04-09 10:30:42 PDT
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 Jeff Gilbert [:jgilbert] 2012-04-09 15:58:50 PDT
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 23 Olli Pettay [:smaug] 2012-04-11 23:24:46 PDT
/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] 2012-04-11 23:50:41 PDT
https://hg.mozilla.org/mozilla-central/rev/8eb367e5b053
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2012-04-12 06:08:22 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.